To start capture on Microchip PDMC the enable bits for each supported
microphone need to be set. After this bit is set the PDMC starts to
receive data from microphones and it considers this data as valid data.
Thus if microphones are not ready the PDMC captures anyway data from its
lines. This data is interpreted by the human ear as poc noises.
To avoid this the following software workaround need to be applied when
starting capture:
1/ enable PDMC channel
2/ wait 150ms
3/ execute 16 dummy reads from RHR
4/ clear interrupts
5/ enable interrupts
6/ enable DMA channel
For this workaround to work step 6 need to be executed at the end.
For step 6 was added patch 1/3 from this series. With this, driver sets
its struct snd_dmaengine_pcm_config::start_dma_last = 1 and proper
action is taken based on this flag when starting DAI trigger vs DMA.
The other solution that was identified for this was to extend the already
existing mechanism around struct snd_soc_dai_link::stop_dma_first. The downside
of this was that a potential struct snd_soc_dai_link::start_dma_last
would have to be populated on sound card driver thus, had to be taken
into account in all sound card drivers. At the moment, the mchp-pdmc is used
only with simple-audio-card. In case of simple-audio-card a new DT
binding would had to be introduced to specify this action on dai-link
descriptions (as of my investigation).
Please advice what might be the best approach.
Thank you,
Claudiu Beznea
Claudiu Beznea (3):
ASoC: soc-generic-dmaengine-pcm: add option to start DMA after DAI
ASoC: dt-bindings: sama7g5-pdmc: add microchip,startup-delay-us
binding
ASoC: mchp-pdmc: fix poc noise at capture startup
.../sound/microchip,sama7g5-pdmc.yaml | 6 ++
include/sound/dmaengine_pcm.h | 1 +
include/sound/soc-component.h | 2 +
sound/soc/atmel/mchp-pdmc.c | 55 +++++++++++++++++--
sound/soc/soc-generic-dmaengine-pcm.c | 8 ++-
sound/soc/soc-pcm.c | 27 +++++++--
6 files changed, 86 insertions(+), 13 deletions(-)
--
2.34.1
Add option to start DMA component after DAI trigger. This is done
by filling the new struct snd_dmaengine_pcm_config::start_dma_last.
Signed-off-by: Claudiu Beznea <[email protected]>
---
include/sound/dmaengine_pcm.h | 1 +
include/sound/soc-component.h | 2 ++
sound/soc/soc-generic-dmaengine-pcm.c | 8 +++++---
sound/soc/soc-pcm.c | 27 ++++++++++++++++++++++-----
4 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index 2df54cf02cb3..4e3dcb94576d 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -149,6 +149,7 @@ struct snd_dmaengine_pcm_config {
const struct snd_pcm_hardware *pcm_hardware;
unsigned int prealloc_buffer_size;
+ unsigned int start_dma_last;
};
int snd_dmaengine_pcm_register(struct device *dev,
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 3203d35bc8c1..0814ed143864 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -190,6 +190,8 @@ struct snd_soc_component_driver {
bool use_dai_pcm_id; /* use DAI link PCM ID as PCM device number */
int be_pcm_base; /* base device ID for all BE PCMs */
+ unsigned int start_dma_last;
+
#ifdef CONFIG_DEBUG_FS
const char *debugfs_prefix;
#endif
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 3b99f619e37e..264e87af6b58 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -318,7 +318,7 @@ static int dmaengine_copy_user(struct snd_soc_component *component,
return 0;
}
-static const struct snd_soc_component_driver dmaengine_pcm_component = {
+static struct snd_soc_component_driver dmaengine_pcm_component = {
.name = SND_DMAENGINE_PCM_DRV_NAME,
.probe_order = SND_SOC_COMP_ORDER_LATE,
.open = dmaengine_pcm_open,
@@ -329,7 +329,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component = {
.pcm_construct = dmaengine_pcm_new,
};
-static const struct snd_soc_component_driver dmaengine_pcm_component_process = {
+static struct snd_soc_component_driver dmaengine_pcm_component_process = {
.name = SND_DMAENGINE_PCM_DRV_NAME,
.probe_order = SND_SOC_COMP_ORDER_LATE,
.open = dmaengine_pcm_open,
@@ -425,7 +425,7 @@ static const struct snd_dmaengine_pcm_config snd_dmaengine_pcm_default_config =
int snd_dmaengine_pcm_register(struct device *dev,
const struct snd_dmaengine_pcm_config *config, unsigned int flags)
{
- const struct snd_soc_component_driver *driver;
+ struct snd_soc_component_driver *driver;
struct dmaengine_pcm *pcm;
int ret;
@@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev,
else
driver = &dmaengine_pcm_component;
+ driver->start_dma_last = config->start_dma_last;
+
ret = snd_soc_component_initialize(&pcm->component, driver, dev);
if (ret)
goto err_free_dma;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 005b179a770a..ec429b93a4ee 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1088,22 +1088,39 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
- int ret = -EINVAL, _ret = 0;
+ struct snd_soc_component *component;
+ int ret = -EINVAL, _ret = 0, start_dma_last = 0, i;
int rollback = 0;
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ /* Do we need to start dma first? */
+ for_each_rtd_components(rtd, i, component) {
+ if (component->driver->start_dma_last) {
+ start_dma_last = 1;
+ break;
+ }
+ }
+
ret = snd_soc_link_trigger(substream, cmd, 0);
if (ret < 0)
goto start_err;
- ret = snd_soc_pcm_component_trigger(substream, cmd, 0);
- if (ret < 0)
- goto start_err;
+ if (start_dma_last) {
+ ret = snd_soc_pcm_dai_trigger(substream, cmd, 0);
+ if (ret < 0)
+ goto start_err;
+
+ ret = snd_soc_pcm_component_trigger(substream, cmd, 0);
+ } else {
+ ret = snd_soc_pcm_component_trigger(substream, cmd, 0);
+ if (ret < 0)
+ goto start_err;
- ret = snd_soc_pcm_dai_trigger(substream, cmd, 0);
+ ret = snd_soc_pcm_dai_trigger(substream, cmd, 0);
+ }
start_err:
if (ret < 0)
rollback = 1;
--
2.34.1
Add microchip,startup-delay-us binding to let PDMC users to specify
startup delay.
Signed-off-by: Claudiu Beznea <[email protected]>
---
.../devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
index c4cf1e5ab84b..9b40268537cb 100644
--- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
+++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
@@ -67,6 +67,12 @@ properties:
maxItems: 4
uniqueItems: true
+ microchip,startup-delay-us:
+ description: |
+ Specifies the delay in microseconds that needs to be applied after
+ enabling the PDMC microphones to avoid unwanted noise due to microphones
+ not being ready.
+
required:
- compatible
- reg
--
2.34.1
Microchip PDMC IP doesn't filter microphone noises on startup. By default,
it captures data received from digital microphones after
the MCHP_PDMC_MR.EN bits are set. Thus when enable is set on PDMC side the
digital microphones might not be ready yet and PDMC captures data from then
in this time. This data captured is poc noise. To avoid this the software
workaround is to the following:
1/ enable PDMC channel
2/ wait 150ms (on SAMA7G5-EK setup)
3/ execute 16 dummy reads from RHR
4/ clear interrupts
5/ enable interrupts
6/ enable DMA channel
Fixes: 50291652af52 ("ASoC: atmel: mchp-pdmc: add PDMC driver")
Signed-off-by: Claudiu Beznea <[email protected]>
---
sound/soc/atmel/mchp-pdmc.c | 55 +++++++++++++++++++++++++++++++++----
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/sound/soc/atmel/mchp-pdmc.c b/sound/soc/atmel/mchp-pdmc.c
index cf4084dcbd5e..c5f597ebfa47 100644
--- a/sound/soc/atmel/mchp-pdmc.c
+++ b/sound/soc/atmel/mchp-pdmc.c
@@ -114,6 +114,7 @@ struct mchp_pdmc {
struct clk *gclk;
u32 pdmcen;
u32 suspend_irq;
+ u32 startup_delay_us;
int mic_no;
int sinc_order;
bool audio_filter_en;
@@ -632,6 +633,29 @@ static int mchp_pdmc_hw_params(struct snd_pcm_substream *substream,
return 0;
}
+static void mchp_pdmc_noise_filter_workaround(struct mchp_pdmc *dd)
+{
+ u32 tmp, steps = 16;
+
+ /*
+ * PDMC doesn't wait for microphones' startup time thus the acquisition
+ * may start before the microphones are ready leading to poc noises at
+ * the beginning of capture. To avoid this, we need to wait 50ms (in
+ * normal startup procedure) or 150 ms (worst case after resume from sleep
+ * states) after microphones are enabled and then clear the FIFOs (by
+ * reading the RHR 16 times) and possible interrupts before continuing.
+ * Also, for this to work the DMA needs to be started after interrupts
+ * are enabled.
+ */
+ usleep_range(dd->startup_delay_us, dd->startup_delay_us + 5);
+
+ while (steps--)
+ regmap_read(dd->regmap, MCHP_PDMC_RHR, &tmp);
+
+ /* Clear interrupts. */
+ regmap_read(dd->regmap, MCHP_PDMC_ISR, &tmp);
+}
+
static int mchp_pdmc_trigger(struct snd_pcm_substream *substream,
int cmd, struct snd_soc_dai *dai)
{
@@ -644,15 +668,17 @@ static int mchp_pdmc_trigger(struct snd_pcm_substream *substream,
switch (cmd) {
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_START:
- /* Enable overrun and underrun error interrupts */
- regmap_write(dd->regmap, MCHP_PDMC_IER, dd->suspend_irq |
- MCHP_PDMC_IR_RXOVR | MCHP_PDMC_IR_RXUDR);
- dd->suspend_irq = 0;
- fallthrough;
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
snd_soc_component_update_bits(cpu, MCHP_PDMC_MR,
MCHP_PDMC_MR_PDMCEN_MASK,
dd->pdmcen);
+
+ mchp_pdmc_noise_filter_workaround(dd);
+
+ /* Enable interrupts. */
+ regmap_write(dd->regmap, MCHP_PDMC_IER, dd->suspend_irq |
+ MCHP_PDMC_IR_RXOVR | MCHP_PDMC_IR_RXUDR);
+ dd->suspend_irq = 0;
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
regmap_read(dd->regmap, MCHP_PDMC_IMR, &dd->suspend_irq);
@@ -796,6 +822,7 @@ static bool mchp_pdmc_readable_reg(struct device *dev, unsigned int reg)
case MCHP_PDMC_CFGR:
case MCHP_PDMC_IMR:
case MCHP_PDMC_ISR:
+ case MCHP_PDMC_RHR:
case MCHP_PDMC_VER:
return true;
default:
@@ -817,6 +844,17 @@ static bool mchp_pdmc_writeable_reg(struct device *dev, unsigned int reg)
}
}
+static bool mchp_pdmc_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MCHP_PDMC_ISR:
+ case MCHP_PDMC_RHR:
+ return true;
+ default:
+ return false;
+ }
+}
+
static bool mchp_pdmc_precious_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
@@ -836,6 +874,7 @@ static const struct regmap_config mchp_pdmc_regmap_config = {
.readable_reg = mchp_pdmc_readable_reg,
.writeable_reg = mchp_pdmc_writeable_reg,
.precious_reg = mchp_pdmc_precious_reg,
+ .volatile_reg = mchp_pdmc_volatile_reg,
.cache_type = REGCACHE_FLAT,
};
@@ -918,6 +957,11 @@ static int mchp_pdmc_dt_init(struct mchp_pdmc *dd)
dd->channel_mic_map[i].clk_edge = edge;
}
+ ret = of_property_read_u32(np, "microchip,startup-delay-us",
+ &dd->startup_delay_us);
+ if (ret)
+ dd->startup_delay_us = 150000;
+
return 0;
}
@@ -941,6 +985,7 @@ static int mchp_pdmc_process(struct snd_pcm_substream *substream,
static struct snd_dmaengine_pcm_config mchp_pdmc_config = {
.process = mchp_pdmc_process,
.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+ .start_dma_last = 1,
};
static int mchp_pdmc_runtime_suspend(struct device *dev)
--
2.34.1
On 2/14/23 08:14, Claudiu Beznea wrote:
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> index 3b99f619e37e..264e87af6b58 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -318,7 +318,7 @@ static int dmaengine_copy_user(struct snd_soc_component *component,
> return 0;
> }
>
> -static const struct snd_soc_component_driver dmaengine_pcm_component = {
> +static struct snd_soc_component_driver dmaengine_pcm_component = {
> .name = SND_DMAENGINE_PCM_DRV_NAME,
> .probe_order = SND_SOC_COMP_ORDER_LATE,
> .open = dmaengine_pcm_open,
> @@ -329,7 +329,7 @@ static const struct snd_soc_component_driver dmaengine_pcm_component = {
> .pcm_construct = dmaengine_pcm_new,
> };
>
> -static const struct snd_soc_component_driver dmaengine_pcm_component_process = {
> +static struct snd_soc_component_driver dmaengine_pcm_component_process = {
> .name = SND_DMAENGINE_PCM_DRV_NAME,
> .probe_order = SND_SOC_COMP_ORDER_LATE,
> .open = dmaengine_pcm_open,
> @@ -425,7 +425,7 @@ static const struct snd_dmaengine_pcm_config snd_dmaengine_pcm_default_config =
> int snd_dmaengine_pcm_register(struct device *dev,
> const struct snd_dmaengine_pcm_config *config, unsigned int flags)
> {
> - const struct snd_soc_component_driver *driver;
> + struct snd_soc_component_driver *driver;
> struct dmaengine_pcm *pcm;
> int ret;
>
> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev,
> else
> driver = &dmaengine_pcm_component;
>
> + driver->start_dma_last = config->start_dma_last;
This will break if you have multiple sound cards in the system.
dmaengine_pcm_component must stay const.
On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote:
> On 2/14/23 08:14, Claudiu Beznea wrote:
> > @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev,
> > else
> > driver = &dmaengine_pcm_component;
> > + driver->start_dma_last = config->start_dma_last;
> This will break if you have multiple sound cards in the system.
> dmaengine_pcm_component must stay const.
Right, if we need to modify it we either need to select which of
multiple const structs to register or to take a copy and modify
that. I've not looked at the actual changes yet.
On 14.02.2023 23:26, Mark Brown wrote:
> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote:
>> On 2/14/23 08:14, Claudiu Beznea wrote:
>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev,
>>> else
>>> driver = &dmaengine_pcm_component;
>>> + driver->start_dma_last = config->start_dma_last;
>> This will break if you have multiple sound cards in the system.
>> dmaengine_pcm_component must stay const.
> Right, if we need to modify it we either need to select which of
> multiple const structs to register or to take a copy and modify
> that. I've not looked at the actual changes yet.
OK, I will try that and return with a new patch.
On the other hand do you think the other solution presented in cover letter
would be better? From the cover letter:
"The other solution that was identified for this was to extend the already
existing mechanism around struct snd_soc_dai_link::stop_dma_first. The downside
of this was that a potential struct snd_soc_dai_link::start_dma_last
would have to be populated on sound card driver thus, had to be taken
into account in all sound card drivers. At the moment, the mchp-pdmc is
used only with simple-audio-card. In case of simple-audio-card a new DT
binding would had to be introduced to specify this action on dai-link
descriptions (as of my investigation)."
Thank you,
Claudiu
On 14/02/2023 17:14, Claudiu Beznea wrote:
> Add microchip,startup-delay-us binding to let PDMC users to specify
> startup delay.
>
> Signed-off-by: Claudiu Beznea <[email protected]>
> ---
> .../devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
> index c4cf1e5ab84b..9b40268537cb 100644
> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
> @@ -67,6 +67,12 @@ properties:
> maxItems: 4
> uniqueItems: true
>
> + microchip,startup-delay-us:
> + description: |
> + Specifies the delay in microseconds that needs to be applied after
> + enabling the PDMC microphones to avoid unwanted noise due to microphones
> + not being ready.
Is this some hardware delay? Or OS? If OS, why Linux specific delay is
put into DT?
Best regards,
Krzysztof
On 16.02.2023 12:04, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 14/02/2023 17:14, Claudiu Beznea wrote:
>> Add microchip,startup-delay-us binding to let PDMC users to specify
>> startup delay.
>>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>> ---
>> .../devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>> index c4cf1e5ab84b..9b40268537cb 100644
>> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>> @@ -67,6 +67,12 @@ properties:
>> maxItems: 4
>> uniqueItems: true
>>
>> + microchip,startup-delay-us:
>> + description: |
>> + Specifies the delay in microseconds that needs to be applied after
>> + enabling the PDMC microphones to avoid unwanted noise due to microphones
>> + not being ready.
>
> Is this some hardware delay? Or OS? If OS, why Linux specific delay is
> put into DT?
It's the delay used in software workaround that IP needs to filter noises.
The IP is not fully featured to do this kind of filtering on its own thus
this software workaround. This delay may depend on used microphones thus
for different kind of setups (PDMC + different microphones) I introduced
this in DT.
>
> Best regards,
> Krzysztof
>
On 16/02/2023 11:15, [email protected] wrote:
> On 16.02.2023 12:04, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 14/02/2023 17:14, Claudiu Beznea wrote:
>>> Add microchip,startup-delay-us binding to let PDMC users to specify
>>> startup delay.
>>>
>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>> ---
>>> .../devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>> index c4cf1e5ab84b..9b40268537cb 100644
>>> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>> @@ -67,6 +67,12 @@ properties:
>>> maxItems: 4
>>> uniqueItems: true
>>>
>>> + microchip,startup-delay-us:
>>> + description: |
>>> + Specifies the delay in microseconds that needs to be applied after
>>> + enabling the PDMC microphones to avoid unwanted noise due to microphones
>>> + not being ready.
>>
>> Is this some hardware delay? Or OS? If OS, why Linux specific delay is
>> put into DT?
>
> It's the delay used in software workaround that IP needs to filter noises.
Then this sounds like OS? Linux related properties usually do not belong
to DT.
> The IP is not fully featured to do this kind of filtering on its own thus
> this software workaround. This delay may depend on used microphones thus
> for different kind of setups (PDMC + different microphones) I introduced
> this in DT.
I understand your driver needs delay and I am not questioning this. I am
questioning why this is suitable for DT?
Best regards,
Krzysztof
On 16.02.2023 12:18, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 16/02/2023 11:15, [email protected] wrote:
>> On 16.02.2023 12:04, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 14/02/2023 17:14, Claudiu Beznea wrote:
>>>> Add microchip,startup-delay-us binding to let PDMC users to specify
>>>> startup delay.
>>>>
>>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>>> index c4cf1e5ab84b..9b40268537cb 100644
>>>> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>>> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>>> @@ -67,6 +67,12 @@ properties:
>>>> maxItems: 4
>>>> uniqueItems: true
>>>>
>>>> + microchip,startup-delay-us:
>>>> + description: |
>>>> + Specifies the delay in microseconds that needs to be applied after
>>>> + enabling the PDMC microphones to avoid unwanted noise due to microphones
>>>> + not being ready.
>>>
>>> Is this some hardware delay? Or OS? If OS, why Linux specific delay is
>>> put into DT?
>>
>> It's the delay used in software workaround that IP needs to filter noises.
>
> Then this sounds like OS? Linux related properties usually do not belong
> to DT.
>
>> The IP is not fully featured to do this kind of filtering on its own thus
>> this software workaround. This delay may depend on used microphones thus
>> for different kind of setups (PDMC + different microphones) I introduced
>> this in DT.
>
> I understand your driver needs delay and I am not questioning this. I am
> questioning why this is suitable for DT?
Because that delay may depend on the microphones that are used with PDMC.
Different boards may come with different microphones, thus the default
delay may not fit to fully filter the noise. Due to this I chose to add it
in DT.
>
>
> Best regards,
> Krzysztof
>
On 16/02/2023 11:41, [email protected] wrote:
> On 16.02.2023 12:18, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 16/02/2023 11:15, [email protected] wrote:
>>> On 16.02.2023 12:04, Krzysztof Kozlowski wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 14/02/2023 17:14, Claudiu Beznea wrote:
>>>>> Add microchip,startup-delay-us binding to let PDMC users to specify
>>>>> startup delay.
>>>>>
>>>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>>>> ---
>>>>> .../devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml | 6 ++++++
>>>>> 1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>>>> index c4cf1e5ab84b..9b40268537cb 100644
>>>>> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-pdmc.yaml
>>>>> @@ -67,6 +67,12 @@ properties:
>>>>> maxItems: 4
>>>>> uniqueItems: true
>>>>>
>>>>> + microchip,startup-delay-us:
>>>>> + description: |
>>>>> + Specifies the delay in microseconds that needs to be applied after
>>>>> + enabling the PDMC microphones to avoid unwanted noise due to microphones
>>>>> + not being ready.
>>>>
>>>> Is this some hardware delay? Or OS? If OS, why Linux specific delay is
>>>> put into DT?
>>>
>>> It's the delay used in software workaround that IP needs to filter noises.
>>
>> Then this sounds like OS? Linux related properties usually do not belong
>> to DT.
>>
>>> The IP is not fully featured to do this kind of filtering on its own thus
>>> this software workaround. This delay may depend on used microphones thus
>>> for different kind of setups (PDMC + different microphones) I introduced
>>> this in DT.
>>
>> I understand your driver needs delay and I am not questioning this. I am
>> questioning why this is suitable for DT?
>
> Because that delay may depend on the microphones that are used with PDMC.
> Different boards may come with different microphones, thus the default
> delay may not fit to fully filter the noise. Due to this I chose to add it
> in DT.
Ah, ok, that's good explanation. Thank you.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 2/16/23 01:49, [email protected] wrote:
> On 14.02.2023 23:26, Mark Brown wrote:
>> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote:
>>> On 2/14/23 08:14, Claudiu Beznea wrote:
>>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev,
>>>> else
>>>> driver = &dmaengine_pcm_component;
>>>> + driver->start_dma_last = config->start_dma_last;
>>> This will break if you have multiple sound cards in the system.
>>> dmaengine_pcm_component must stay const.
>> Right, if we need to modify it we either need to select which of
>> multiple const structs to register or to take a copy and modify
>> that. I've not looked at the actual changes yet.
> OK, I will try that and return with a new patch.
>
> On the other hand do you think the other solution presented in cover letter
> would be better? From the cover letter:
>
> "The other solution that was identified for this was to extend the already
> existing mechanism around struct snd_soc_dai_link::stop_dma_first. The downside
> of this was that a potential struct snd_soc_dai_link::start_dma_last
> would have to be populated on sound card driver thus, had to be taken
> into account in all sound card drivers. At the moment, the mchp-pdmc is
> used only with simple-audio-card. In case of simple-audio-card a new DT
> binding would had to be introduced to specify this action on dai-link
> descriptions (as of my investigation)."
>
Can't you just set `start_dma_last` on the `mchp_pdmc_dai_component`? In
your code you iterate over all the components of the link and if any of
them has it set the DMA is started last.
On Thu, Feb 16, 2023 at 11:18:16AM +0100, Krzysztof Kozlowski wrote:
> On 16/02/2023 11:15, [email protected] wrote:
> >>> + microchip,startup-delay-us:
> >>> + description: |
> >>> + Specifies the delay in microseconds that needs to be applied after
> >>> + enabling the PDMC microphones to avoid unwanted noise due to microphones
> >>> + not being ready.
> >> Is this some hardware delay? Or OS? If OS, why Linux specific delay is
> >> put into DT?
> > It's the delay used in software workaround that IP needs to filter noises.
> Then this sounds like OS? Linux related properties usually do not belong
> to DT.
This is a hardware property, it's the time needed for the input
to settle.
On 16.02.2023 15:53, Lars-Peter Clausen wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 2/16/23 01:49, [email protected] wrote:
>> On 14.02.2023 23:26, Mark Brown wrote:
>>> On Tue, Feb 14, 2023 at 10:14:28AM -0800, Lars-Peter Clausen wrote:
>>>> On 2/14/23 08:14, Claudiu Beznea wrote:
>>>>> @@ -450,6 +450,8 @@ int snd_dmaengine_pcm_register(struct device *dev,
>>>>> else
>>>>> driver = &dmaengine_pcm_component;
>>>>> + driver->start_dma_last = config->start_dma_last;
>>>> This will break if you have multiple sound cards in the system.
>>>> dmaengine_pcm_component must stay const.
>>> Right, if we need to modify it we either need to select which of
>>> multiple const structs to register or to take a copy and modify
>>> that. I've not looked at the actual changes yet.
>> OK, I will try that and return with a new patch.
>>
>> On the other hand do you think the other solution presented in cover letter
>> would be better? From the cover letter:
>>
>> "The other solution that was identified for this was to extend the already
>> existing mechanism around struct snd_soc_dai_link::stop_dma_first. The
>> downside
>> of this was that a potential struct snd_soc_dai_link::start_dma_last
>> would have to be populated on sound card driver thus, had to be taken
>> into account in all sound card drivers. At the moment, the mchp-pdmc is
>> used only with simple-audio-card. In case of simple-audio-card a new DT
>> binding would had to be introduced to specify this action on dai-link
>> descriptions (as of my investigation)."
>>
> Can't you just set `start_dma_last` on the `mchp_pdmc_dai_component`? In
> your code you iterate over all the components of the link and if any of
> them has it set the DMA is started last.
Yes, that is also working.
In this patch I chose to have it on DMA component as the operation is
specific to DMA... It looked better to me this way. But is true that having
it on mchp_pdmc_dai_component wouldn't affect the behavior.
Thank you,
Claudiu