Re: [Sound-open-firmware] [PATCH 01/15] [RFC][v2]DMIC: Add PDM microphones (DMIC) support to DAI

On 07.05.2018 14:37, Liam Girdwood wrote:

On Fri, 2018-05-04 at 18:01 +0300, Seppo Ingalsuo wrote:

...

This patch adds the DMIC audio capture driver for SOF DAI component use. The DMIC feature allows to directly attach one to (typically) four PDM digital microphones into Intel SoC without a separate codec IC. This is supported by APL and most successor platforms.

Corresponding patches are needed for kernel driver and topology to enable this feature.

Tested in APL UP squared board without connected microphones SOF git master 3ad69eb715a09de9a0b91c56c9cca8a79ead00a9

Signed-off-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com

src/drivers/dmic.c | 1383 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 1383 insertions(+) create mode 100644 src/drivers/dmic.c

I've applied 2,5,7,8,9,10,11,14 and 15. The rest are good but needs

Great, it was RFC but no objections! It saves me a lot of effort for next patch version.

...

+#if defined MODULE_TEST +#include <stdio.h> +#define QTOF(x, q) ((float)(x) / ((int64_t)1 << (q))) +#endif

Does this need to go in a maths header ?

Yep, it will be useful since the new platforms have float capability and there will be more float <-> fractional conversions needed when we have float pipelines support. Also, this is not optimized at all but can work as reference.

+#if DMIC_HW_VERSION == 1

•   ipm_helper(&ipm, stereo, swap, dmic);
  

•   val = OUTCONTROL0_TIE(0) |
  

•           OUTCONTROL0_SIP(0) |
  

•           OUTCONTROL0_FINIT(1) |
  

•           OUTCONTROL0_FCI(0) |
  

•           OUTCONTROL0_BFTH(bfth) |
  

•           OUTCONTROL0_OF(of0) |
  

•           OUTCONTROL0_IPM(ipm) |
  

•           OUTCONTROL0_TH(th);
  

•   dmic_write(dai, OUTCONTROL0, val);
  

•   trace_value(val);
  

•   val = OUTCONTROL1_TIE(0) |
  

•           OUTCONTROL1_SIP(0) |
  

•           OUTCONTROL1_FINIT(1) |
  

•           OUTCONTROL1_FCI(0) |
  

•           OUTCONTROL1_BFTH(bfth) |
  

•           OUTCONTROL1_OF(of1) |
  

•           OUTCONTROL1_IPM(ipm) |
  

•           OUTCONTROL1_TH(th);
  

•   dmic_write(dai, OUTCONTROL1, val);
  

•   trace_value(val);
  

+#endif

+#if DMIC_HW_VERSION == 2

•   source_ipm_helper(source, &ipm, stereo, swap, dmic);
  
  
  

Best to use #elif

+/* start the DMIC for capture */ +static void dmic_start(struct dai *dai, int direction) +{

•   struct dmic_pdata *dmic = dai_get_drvdata(dai);
  

•   int i;
  

•   int mic_a;
  

•   int mic_b;
  

•   int fir_a;
  

•   int fir_b;
  

•   if (direction != DAI_DIR_CAPTURE)
  

•           return;
  

•   /* enable port */
  

•   spin_lock(&dmic->lock);
  

•   trace_dmic("sta");
  

•   dmic->state = COMP_STATE_ACTIVE;
  
  
  

Regarding the spinlock, was this to be used to guarantee that both mic A and B would use a compatible configuration at start ?

I only copied the spinlock practice from SSP so not sure about need. Microphones A (left) and B (right) control for each PDM controller should be OK since the HW re-samples the signals to not mess up internal processing pipeline. Concerning FIFOs A and B this may not be sufficient.

I'd assume the code could work 1. capture via FIFO A. 2. stop capture, 3. capture via FIFO B so no support for simultaneous or overlapping usage. Some next driver version should address that.

What is your proposal?

...

• if (direction != DAI_DIR_CAPTURE) {

• trace_dmic_error("cap");
  

• return -EINVAL;
  

• }

You need to do this here :-

ret = comp_set_state(dev, cmd); if (ret < 0) return ret;

This does all state checking/setting so no need to do it below.

OK.  Keyon is also is checking the same with SSP so I can follow how it's done for SSP.

...

• switch (cmd) {
• case COMP_TRIGGER_START:

• if (dmic->state == COMP_STATE_PREPARE ||
  

• 	dmic->state == COMP_STATE_PAUSED) {
  

• 	dmic_start(dai, direction);
  

• } else {
  

• 	trace_dmic_error("cst");
  

• 	trace_value(dmic->state);
  

• }
  

• break;
  

• case COMP_TRIGGER_RELEASE:

• if (dmic->state == COMP_STATE_PREPARE) {
  

• 	dmic_start(dai, direction);
  

• } else {
  

• 	trace_dmic_error("crl");
  

• 	trace_value(dmic->state);
  

• }
  

• break;
  

• case COMP_TRIGGER_STOP:
• case COMP_TRIGGER_PAUSE:

• dmic->state = COMP_STATE_PAUSED;
  

• dmic_stop(dai);
  

• break;
  

• case COMP_TRIGGER_RESUME:

• dmic_context_restore(dai);
  

• break;
  

• case COMP_TRIGGER_SUSPEND:

• dmic_context_store(dai);
  

• break;
  

• default:

• break;
  

• }
• return 0;

+}

+/* TODO: No idea what should be done here. Currently only trace the

• • status register that contains a few status and error bit fields.
• */

The intention here is to emit any DMIC errors via trace errors to help with debug.

OK, sounds then just removing the TODO comment is enough. When I had DMA mis-programmed I saw these DMIC irq events.

Thanks, Seppo

...

+static void dmic_irq_handler(void *data) +{

• struct dai *dai = data;
• uint32_t val;
• /* Trace OUTSTAT0 register */
• val = dmic_read(dai, OUTSTAT0);
• trace_dmic("irq");
• if (val & OUTSTAT0_ROR_BIT)

• trace_dmic_error("eor"); /* Full fifo or PDM overrrun */
  

• trace_value(dmic_read(dai, OUTSTAT0));
• /* clear IRQ */
• platform_interrupt_clear(dmic_irq(dai), 1);

+}

Liam _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware