Re: [PATCH 2/2] ARM: dts: exynos: Fix initial clocks configuration on Odroid X/X2/U3 boards

Re: [PATCH 2/2] ARM: dts: exynos: Fix initial clocks configuration on Odroid X/X2/U3 boards

[Sylwester Nawrocki]

On 12/08/2016 05:44 PM, Krzysztof Kozlowski wrote:
> On Thu, Dec 08, 2016 at 12:01:54PM +0100, Marek Szyprowski wrote:
>> Move assigned clocks properties from sound node to audio subsystem clock
>> controller node. This way clocks topology and rates are set just after
>> probing audio clocks controller. Leaving those properties under sound
>> node doesn't guarantee to configure them before they are being used
>> (for example i2s hardware module can be probed in parallel and it also
>> require proper audio clocks configuration).
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> Looks correct, for the reference:
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Does this fixes any encountered issue (real one)? I wonder whether this
> should go to fixes or not.

With current code there is no issues, and in fact there should be no
such timing dependencies in DT to rely on.  However, the binding convention
is to put assigned-clock* properties in a node of a device they belong
to and in this case the 'clock_audss' is more appropriate than the 'sound'
node.  This patch doesn't fix any bug with current code but it might save
us some trouble with future changes.

-- 
Thanks,
Sylwester

> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index 7815efd..b6b0f50 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -43,16 +43,6 @@
>
>  	sound: sound {
>  		compatible = "simple-audio-card";
> -		assigned-clocks = <&clock_audss EXYNOS_MOUT_AUDSS>,
> -				<&clock_audss EXYNOS_MOUT_I2S>,
> -				<&clock_audss EXYNOS_DOUT_SRP>,
> -				<&clock_audss EXYNOS_DOUT_AUD_BUS>;
> -		assigned-clock-parents = <&clock CLK_FOUT_EPLL>,
> -				<&clock_audss EXYNOS_MOUT_AUDSS>;
> -		assigned-clock-rates = <0>,
> -				<0>,
> -				<192000000>,
> -				<19200000>;
>
>  		simple-audio-card,format = "i2s";
>  		simple-audio-card,bitclock-master = <&link0_codec>;
> @@ -157,6 +147,16 @@
>  	status = "okay";
>  };
>
> +&clock_audss {
> +	assigned-clocks = <&clock_audss EXYNOS_MOUT_AUDSS>,
> +			<&clock_audss EXYNOS_MOUT_I2S>,
> +			<&clock_audss EXYNOS_DOUT_SRP>,
> +			<&clock_audss EXYNOS_DOUT_AUD_BUS>;
> +	assigned-clock-parents = <&clock CLK_FOUT_EPLL>,
> +			<&clock_audss EXYNOS_MOUT_AUDSS>;
> +	assigned-clock-rates = <0>, <0>, <192000000>, <19200000>;
> +};
> +
>  &cpu0 {
>  	cpu0-supply = <&buck2_reg>;
>  };
>

Re: [PATCH 2/2] ARM: dts: exynos: Fix initial clocks configuration on Odroid X/X2/U3 boards

[Krzysztof Kozlowski]

On Thu, Dec 08, 2016 at 06:07:02PM +0100, Sylwester Nawrocki wrote:
> On 12/08/2016 05:44 PM, Krzysztof Kozlowski wrote:
> > On Thu, Dec 08, 2016 at 12:01:54PM +0100, Marek Szyprowski wrote:
> >> Move assigned clocks properties from sound node to audio subsystem clock
> >> controller node. This way clocks topology and rates are set just after
> >> probing audio clocks controller. Leaving those properties under sound
> >> node doesn't guarantee to configure them before they are being used
> >> (for example i2s hardware module can be probed in parallel and it also
> >> require proper audio clocks configuration).
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 20 ++++++++++----------
> >>  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > Looks correct, for the reference:
> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> > 
> > Does this fixes any encountered issue (real one)? I wonder whether this
> > should go to fixes or not.
> 
> With current code there is no issues, and in fact there should be no
> such timing dependencies in DT to rely on.  However, the binding convention
> is to put assigned-clock* properties in a node of a device they belong
> to and in this case the 'clock_audss' is more appropriate than the 'sound'
> node.  This patch doesn't fix any bug with current code but it might save
> us some trouble with future changes.
>

Thanks for explanation. I am not planning any pull requests for this
cycle so the patch will have to wait till the end of 4.10-rc1
merge window.

Best regards,
Krzysztof