diff mbox series

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

Message ID 20240314232201.2102178-14-jan.dakinevich@salutedevices.com (mailing list archive)
State New, archived
Headers show
Series Introduce support of audio for Amlogic A1 SoC family | expand

Commit Message

Jan Dakinevich March 14, 2024, 11:21 p.m. UTC
This option allow to redefine the rate of DSP system clock.

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

Comments

Krzysztof Kozlowski March 15, 2024, 10 a.m. UTC | #1
On 15/03/2024 00:21, Jan Dakinevich wrote:
> 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.

> 
> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
> ---
>  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.

Best regards,
Krzysztof
Jerome Brunet March 15, 2024, 10:22 a.m. UTC | #2
On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 15/03/2024 00:21, Jan Dakinevich wrote:
>> 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.
>
>> 
>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>> ---
>>  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.

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

>
> Best regards,
> Krzysztof
Jan Dakinevich March 17, 2024, 3:52 p.m. UTC | #3
On 3/15/24 13:22, Jerome Brunet wrote:
> 
> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>> 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.
>>
>>>
>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>> ---
>>>  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'.

> Then you don't have to deal with it in the device driver.
> 
>>
>> Best regards,
>> Krzysztof
> 
>
Jan Dakinevich March 17, 2024, 3:55 p.m. UTC | #4
On 3/15/24 13:00, Krzysztof Kozlowski wrote:
> On 15/03/2024 00:21, Jan Dakinevich wrote:
>> 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.
> 

What do you mean? I am adding some new property and should describe it
in dt-bindinds. Isn't it?

>>
>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>> ---
>>  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.
> 

Answered in next message.

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski March 17, 2024, 4:27 p.m. UTC | #5
On 17/03/2024 16:55, Jan Dakinevich wrote:
> 
> 
> On 3/15/24 13:00, Krzysztof Kozlowski wrote:
>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>> 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.
>>
> 
> What do you mean? I am adding some new property and should describe it
> in dt-bindinds. Isn't it?

No, if the property is not suitable for bindings, you should not add it
in the first place. So again: explain what sort of hardware, not driver,
problem you are solving here, so we can understand why do you need new
property. Otherwise use existing properties or no properties, because we
do not define all possible clocks in the bindings.

Let's be clear: with such commit msg explanation as you have, my answer
is: no, driver should set clock frequency and you do not need this
property at all.

Best regards,
Krzysztof
Jan Dakinevich March 17, 2024, 4:35 p.m. UTC | #6
On 3/17/24 19:27, Krzysztof Kozlowski wrote:
> On 17/03/2024 16:55, Jan Dakinevich wrote:
>>
>>
>> On 3/15/24 13:00, Krzysztof Kozlowski wrote:
>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>> 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.
>>>
>>
>> What do you mean? I am adding some new property and should describe it
>> in dt-bindinds. Isn't it?
> 
> No, if the property is not suitable for bindings, you should not add it
> in the first place. So again: explain what sort of hardware, not driver,
> problem you are solving here, so we can understand why do you need new
> property. Otherwise use existing properties or no properties, because we
> do not define all possible clocks in the bindings.
> 
> Let's be clear: with such commit msg explanation as you have, my answer
> is: no, driver should set clock frequency and you do not need this
> property at all.
> 

Could you please take a look on answer to "Jerome Brunet
<jbrunet@baylibre.com>"'s message on the same thread. There, I am trying
to explain what I am solving by this commit.

I would be happy to avoid this w/a, but currently this is the best I
came up with.

> Best regards,
> Krzysztof
>
Jerome Brunet March 18, 2024, 10:55 a.m. UTC | #7
On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:

> On 3/15/24 13:22, Jerome Brunet wrote:
>> 
>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> 
>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>> 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.
>>>
>>>>
>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>>> ---
>>>>  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 ?

>
>> Then you don't have to deal with it in the device driver.
>> 
>>>
>>> Best regards,
>>> Krzysztof
>> 
>>
Jerome Brunet March 18, 2024, 12:19 p.m. UTC | #8
On Mon 18 Mar 2024 at 11:55, Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
>
>> On 3/15/24 13:22, Jerome Brunet wrote:
>>> 
>>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>> 
>>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>>> 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.
>>>>
>>>>>
>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>>>> ---
>>>>>  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.

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.

>
>>
>>> Then you don't have to deal with it in the device driver.
>>> 
>>>>
>>>> Best regards,
>>>> Krzysztof
>>> 
>>>
Jan Dakinevich March 19, 2024, 12:30 a.m. UTC | #9
On 3/18/24 15:19, Jerome Brunet wrote:
> 
> On Mon 18 Mar 2024 at 11:55, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
>> On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
>>
>>> On 3/15/24 13:22, Jerome Brunet wrote:
>>>>
>>>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>>>> 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.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>>>>> ---
>>>>>>  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.

>>
>>>
>>>> Then you don't have to deal with it in the device driver.
>>>>
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>
>>>>
> 
>
Jan Dakinevich March 19, 2024, 12:35 a.m. UTC | #10
On 3/18/24 13:55, Jerome Brunet wrote:
> 
> On Sun 17 Mar 2024 at 18:52, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote:
> 
>> On 3/15/24 13:22, Jerome Brunet wrote:
>>>
>>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>>> 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.
>>>>
>>>>>
>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>>>> ---
>>>>>  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 ?
> 

Ok, in next version I will try to remove this unconditional setting of
rate that spoils my clock hierarchy.

>>
>>> Then you don't have to deal with it in the device driver.
>>>
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>>
> 
>
Krzysztof Kozlowski March 19, 2024, 5:17 a.m. UTC | #11
On 17/03/2024 17:35, Jan Dakinevich wrote:
> 
> 
> On 3/17/24 19:27, Krzysztof Kozlowski wrote:
>> On 17/03/2024 16:55, Jan Dakinevich wrote:
>>>
>>>
>>> On 3/15/24 13:00, Krzysztof Kozlowski wrote:
>>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>>> 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.
>>>>
>>>
>>> What do you mean? I am adding some new property and should describe it
>>> in dt-bindinds. Isn't it?
>>
>> No, if the property is not suitable for bindings, you should not add it
>> in the first place. So again: explain what sort of hardware, not driver,
>> problem you are solving here, so we can understand why do you need new
>> property. Otherwise use existing properties or no properties, because we
>> do not define all possible clocks in the bindings.
>>
>> Let's be clear: with such commit msg explanation as you have, my answer
>> is: no, driver should set clock frequency and you do not need this
>> property at all.
>>
> 
> Could you please take a look on answer to "Jerome Brunet
> <jbrunet@baylibre.com>"'s message on the same thread. There, I am trying
> to explain what I am solving by this commit.

How is this answer here? You asked "What do you mean", so apparently you
did not understand why I am responding and why you cannot just document
whatever you wish, because that "whatever you wish" is not correct. I
explained that but now you respond that I should read other part of
emails. Really?

So again, do you understand that commit msg should provide rationale why
you think this describes hardware and why this is suitable for bindings?


Best regards,
Krzysztof
Krzysztof Kozlowski March 19, 2024, 5:17 a.m. UTC | #12
On 17/03/2024 16:52, Jan Dakinevich wrote:
> 
> 
> On 3/15/24 13:22, Jerome Brunet wrote:
>>
>> On Fri 15 Mar 2024 at 11:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>>> On 15/03/2024 00:21, Jan Dakinevich wrote:
>>>> 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.
>>>
>>>>
>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com>
>>>> ---
>>>>  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

That's driver's problem. You do not change bindings, just because your
driver behaves differently. Just fix driver.

> 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 does it have to do with bindings? Change driver if you are not
happy how it operates.

Best regards,
Krzysztof
diff mbox series

Patch

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
+
 required:
   - compatible
   - reg