Re: [PATCH 13/25] ASoC: dt-bindings: meson: axg-pdm: document 'sysrate' property

On 3/18/24 15:19, Jerome Brunet wrote:

On Mon 18 Mar 2024 at 11:55, Jerome Brunet [off-list ref] wrote:

quoted

On Sun 17 Mar 2024 at 18:52, Jan Dakinevich [off-list ref] wrote:

quoted

On 3/15/24 13:22, Jerome Brunet wrote:

quoted

On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski [off-list ref] wrote:

quoted

On 15/03/2024 00:21, Jan Dakinevich wrote:

quoted

This option allow to redefine the rate of DSP system clock.

And why is it suitable for bindings? Describe the hardware, not what you
want to do in the driver.

quoted

Signed-off-by: Jan Dakinevich <redacted>
---
 Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml
index df21dd72fc65..d2f23a59a6b6 100644
--- a/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml
+++ b/Documentation/devicetree/bindings/sound/amlogic,axg-pdm.yaml

@@ -40,6 +40,10 @@ properties:
   resets:
     maxItems: 1
 
+  sysrate:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: redefine rate of DSP system clock

No vendor prefix, so is it a generic property? Also, missing unit
suffix, but more importantly I don't understand why this is a property
of hardware.

+1.

The appropriate way to set rate of the clock before the driver take over
is 'assigned-rate', if you need to customize this for different
platform.

It would be great, but it doesn't work. Below, is what I want to see:

	assigned-clocks =
		<&clkc_audio AUD2_CLKID_PDM_SYSCLK_SEL>,
		<&clkc_audio AUD2_CLKID_PDM_SYSCLK_DIV>;
	assigned-clock-parents =
		<&clkc_pll CLKID_FCLK_DIV3>,
		<0>;
	assigned-clock-rates =
		<0>,
		<256000000>;

But regardles of this declaration, PDM's driver unconditionally sets
sysclk'rate to 250MHz and throws away everything that was configured
before, reparents audio2_pdm_sysclk_mux to hifi_pll and changes
hifi_pll's rate.

This value 250MHz is declared here:

static const struct axg_pdm_cfg axg_pdm_config = {
	.filters = &axg_default_filters,
	.sys_rate = 250000000,
};

The property 'sysrate' is intended to redefine hardcoded 'sys_rate'
value in 'axg_pdm_config'.

What is stopping you from removing that from the driver and adding
assigned-rate to 250M is the existing platform ?

... Also, considering how PDM does work, I'm not sure I get the point of
the doing all this to go from 250MHz to 256Mhz.

The point is to use fclk_div3 clock as source for PDM's sysclock and
keep hiff_pll clock free for TDM. Because, I can get 256MHz from any
hifi_pll and fclk_div3, but only hifi_pll is able to provide accurate
48kHz (after several divider).

PDM value is sampled at ~75% of the half period. That clock basically
feeds a counter and the threshold is adjusted based on the clock rate.

So there is no need to change the rate. Changing it is only necessary
when the captured audio rate is extremely slow (<8kHz) and the counter
may overflow. The driver already adjust this automatically.

So changing the input rate from 250MHz to 256MHz should not make any
difference.

Thank you for the explanation.

quoted

quoted

quoted

Then you don't have to deal with it in the device driver.

quoted

Best regards,
Krzysztof

-- 
Best regards
Jan Dakinevich

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel