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

[Liam Girdwood]

On Mon, 2018-05-07 at 17:38 +0300, Seppo Ingalsuo wrote:
> 
> 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 at 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.
> 

These were all ready anyway, no need to expend more effort.

> >   
> > 
> > 
> > > +
> > > +#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.

SSP probably had this to make sure playback/capture clocking were the same i.e.
the first user (playback or capture) sets the clock speed/format and the second
user has no option but to use this.

Keyon, can you refine this in SSP when you have time ?

> 
> 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?

In general the first user has free reign to set the global DMIC configuration
for the controllers Mic A/B ports (for the current streams). This should be
atomic in terms of IPC (i.e. 1 IPC call) and may require locking if it can be
interrupted and changed for any reason (e.g. dmic irq handler, pipeline
scheduling, anything that has the ability to change DMIC state).

> 
> >   
> > 
> > > +	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.

Yes.

Thanks

Liam

> 
> 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 at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
> > 
> 
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware