[PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Sylwester Nawrocki]

Add more definitions to the exynos5433_aud_pll_rates table so the
AUD_PLL can be used as a root clock source for the I2S1 and the HDMI
interface.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index db270908037a..74b70ddab4d6 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -765,6 +765,14 @@ static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initcons
 	PLL_36XX_RATE(294912000U,  98, 1, 3,  19923),
 	PLL_36XX_RATE(288000000U,  96, 1, 3,      0),
 	PLL_36XX_RATE(252000000U,  84, 1, 3,      0),
+	PLL_36XX_RATE(200000000U, 200, 3, 3,      0),
+	PLL_36XX_RATE(196608001U, 197, 3, 3, -25690),
+	PLL_36XX_RATE(180633609U, 301, 5, 3,   3671),
+	PLL_36XX_RATE(131072006U, 131, 3, 3,   4719),
+	PLL_36XX_RATE(100000000U, 200, 3, 4,      0),
+	PLL_36XX_RATE(65536003U,  131, 3, 4,   4719),
+	PLL_36XX_RATE(49152000U,  197, 3, 5, -25690),
+	PLL_36XX_RATE(32768001U,  131, 3, 5,   4719),
 	{ /* sentinel */ }
 };

--
2.14.2

[PATCH 2/3] clk: exynos5433: Allow audio subsystem clock rate propagation

[Sylwester Nawrocki]

CLK_SET_RATE_PARENT flag is added to definitions of clocks on a path
starting from CLK_SCLK_I2S1 up to AUD_PLL in order to allow setting
required audio root clock frequency for the I2S1 block. This is now
only done for the I2S1 block, related CMU_TOP, CMU_PERIC clock
definitions are changed.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 74b70ddab4d6..d74361736e64 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -246,14 +246,14 @@ static const struct samsung_fixed_rate_clock top_fixed_clks[] __initconst = {
 
 static const struct samsung_mux_clock top_mux_clks[] __initconst = {
 	/* MUX_SEL_TOP0 */
-	MUX(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
-			4, 1),
+	MUX_F(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
+	      4, 1, CLK_SET_RATE_PARENT, 0),
 	MUX(CLK_MOUT_ISP_PLL, "mout_isp_pll", mout_isp_pll_p, MUX_SEL_TOP0,
 			0, 1),
 
 	/* MUX_SEL_TOP1 */
-	MUX(CLK_MOUT_AUD_PLL_USER_T, "mout_aud_pll_user_t",
-			mout_aud_pll_user_p, MUX_SEL_TOP1, 12, 1),
+	MUX_F(CLK_MOUT_AUD_PLL_USER_T, "mout_aud_pll_user_t",
+	      mout_aud_pll_user_p, MUX_SEL_TOP1, 12, 1, CLK_SET_RATE_PARENT, 0),
 	MUX(CLK_MOUT_MPHY_PLL_USER, "mout_mphy_pll_user", mout_mphy_pll_user_p,
 			MUX_SEL_TOP1, 8, 1),
 	MUX(CLK_MOUT_MFC_PLL_USER, "mout_mfc_pll_user", mout_mfc_pll_user_p,
@@ -370,8 +370,8 @@ static const struct samsung_mux_clock top_mux_clks[] __initconst = {
 			MUX_SEL_TOP_PERIC1, 16, 1),
 	MUX(CLK_MOUT_SCLK_SPDIF, "mout_sclk_spdif", mout_sclk_spdif_p,
 			MUX_SEL_TOP_PERIC1, 12, 2),
-	MUX(CLK_MOUT_SCLK_AUDIO1, "mout_sclk_audio1", mout_sclk_audio1_p,
-			MUX_SEL_TOP_PERIC1, 4, 2),
+	MUX_F(CLK_MOUT_SCLK_AUDIO1, "mout_sclk_audio1", mout_sclk_audio1_p,
+			MUX_SEL_TOP_PERIC1, 4, 2, CLK_SET_RATE_PARENT, 0),
 	MUX(CLK_MOUT_SCLK_AUDIO0, "mout_sclk_audio0", mout_sclk_audio0_p,
 			MUX_SEL_TOP_PERIC1, 0, 2),
 
@@ -524,12 +524,12 @@ static const struct samsung_div_clock top_div_clks[] __initconst = {
 			DIV_TOP_PERIC2, 0, 4),
 
 	/* DIV_TOP_PERIC3 */
-	DIV(CLK_DIV_SCLK_I2S1, "div_sclk_i2s1", "sclk_audio1",
-			DIV_TOP_PERIC3, 16, 6),
+	DIV_F(CLK_DIV_SCLK_I2S1, "div_sclk_i2s1", "sclk_audio1",
+			DIV_TOP_PERIC3, 16, 6, CLK_SET_RATE_PARENT, 0),
 	DIV(CLK_DIV_SCLK_PCM1, "div_sclk_pcm1", "sclk_audio1",
 			DIV_TOP_PERIC3, 8, 8),
-	DIV(CLK_DIV_SCLK_AUDIO1, "div_sclk_audio1", "mout_sclk_audio1",
-			DIV_TOP_PERIC3, 4, 4),
+	DIV_F(CLK_DIV_SCLK_AUDIO1, "div_sclk_audio1", "mout_sclk_audio1",
+			DIV_TOP_PERIC3, 4, 4, CLK_SET_RATE_PARENT, 0),
 	DIV(CLK_DIV_SCLK_AUDIO0, "div_sclk_audio0", "mout_sclk_audio0",
 			DIV_TOP_PERIC3, 0, 4),
 
@@ -693,7 +693,7 @@ static const struct samsung_gate_clock top_gate_clks[] __initconst = {
 	GATE(CLK_SCLK_SLIMBUS, "sclk_slimbus", "mout_sclk_slimbus",
 			MUX_ENABLE_TOP_PERIC1, 16, 0, 0),
 	GATE(CLK_SCLK_AUDIO1, "sclk_audio1", "div_sclk_audio1",
-			MUX_ENABLE_TOP_PERIC1, 4, 0, 0),
+			MUX_ENABLE_TOP_PERIC1, 4, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_AUDIO0, "sclk_audio0", "div_sclk_audio0",
 			MUX_ENABLE_TOP_PERIC1, 0, 0, 0),
 };
-- 
2.14.2

Re: [PATCH 2/3] clk: exynos5433: Allow audio subsystem clock rate propagation

[Chanwoo Choi]

Hi Sylwester,

On 2018년 02월 05일 23:22, Sylwester Nawrocki wrote:
> CLK_SET_RATE_PARENT flag is added to definitions of clocks on a path
> starting from CLK_SCLK_I2S1 up to AUD_PLL in order to allow setting
> required audio root clock frequency for the I2S1 block. This is now
> only done for the I2S1 block, related CMU_TOP, CMU_PERIC clock
> definitions are changed.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index 74b70ddab4d6..d74361736e64 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -246,14 +246,14 @@ static const struct samsung_fixed_rate_clock top_fixed_clks[] __initconst = {
>  
>  static const struct samsung_mux_clock top_mux_clks[] __initconst = {
>  	/* MUX_SEL_TOP0 */
> -	MUX(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
> -			4, 1),
> +	MUX_F(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
> +	      4, 1, CLK_SET_RATE_PARENT, 0),

If you add CLK_SET_RATE_PARENT to 'mout_aud_pll' and mout_aud_pll changes the rate,
fout_aud_pll's rate will be changed. But, fout_aud_pll is also the parent
of 'mout_aud_pll_user'. It might change the rate of children of mout_aud_pll_user.
mout_aud_pll_user would not want to change the parent's clock.

    fout_aud_pll                          2            2   196608009          0 0  
       mout_aud_pll_user                  1            1   196608009          0 0  
       mout_aud_pll                       0            0   196608009          0 0  


>  	MUX(CLK_MOUT_ISP_PLL, "mout_isp_pll", mout_isp_pll_p, MUX_SEL_TOP0,
>  			0, 1),
>  
>  	/* MUX_SEL_TOP1 */
> -	MUX(CLK_MOUT_AUD_PLL_USER_T, "mout_aud_pll_user_t",
> -			mout_aud_pll_user_p, MUX_SEL_TOP1, 12, 1),
> +	MUX_F(CLK_MOUT_AUD_PLL_USER_T, "mout_aud_pll_user_t",
> +	      mout_aud_pll_user_p, MUX_SEL_TOP1, 12, 1, CLK_SET_RATE_PARENT, 0),
>  	MUX(CLK_MOUT_MPHY_PLL_USER, "mout_mphy_pll_user", mout_mphy_pll_user_p,
>  			MUX_SEL_TOP1, 8, 1),
>  	MUX(CLK_MOUT_MFC_PLL_USER, "mout_mfc_pll_user", mout_mfc_pll_user_p,
> @@ -370,8 +370,8 @@ static const struct samsung_mux_clock top_mux_clks[] __initconst = {
>  			MUX_SEL_TOP_PERIC1, 16, 1),
>  	MUX(CLK_MOUT_SCLK_SPDIF, "mout_sclk_spdif", mout_sclk_spdif_p,
>  			MUX_SEL_TOP_PERIC1, 12, 2),
> -	MUX(CLK_MOUT_SCLK_AUDIO1, "mout_sclk_audio1", mout_sclk_audio1_p,
> -			MUX_SEL_TOP_PERIC1, 4, 2),
> +	MUX_F(CLK_MOUT_SCLK_AUDIO1, "mout_sclk_audio1", mout_sclk_audio1_p,
> +			MUX_SEL_TOP_PERIC1, 4, 2, CLK_SET_RATE_PARENT, 0),
>  	MUX(CLK_MOUT_SCLK_AUDIO0, "mout_sclk_audio0", mout_sclk_audio0_p,
>  			MUX_SEL_TOP_PERIC1, 0, 2),
>  
> @@ -524,12 +524,12 @@ static const struct samsung_div_clock top_div_clks[] __initconst = {
>  			DIV_TOP_PERIC2, 0, 4),
>  
>  	/* DIV_TOP_PERIC3 */
> -	DIV(CLK_DIV_SCLK_I2S1, "div_sclk_i2s1", "sclk_audio1",
> -			DIV_TOP_PERIC3, 16, 6),
> +	DIV_F(CLK_DIV_SCLK_I2S1, "div_sclk_i2s1", "sclk_audio1",
> +			DIV_TOP_PERIC3, 16, 6, CLK_SET_RATE_PARENT, 0),
>  	DIV(CLK_DIV_SCLK_PCM1, "div_sclk_pcm1", "sclk_audio1",
>  			DIV_TOP_PERIC3, 8, 8),
> -	DIV(CLK_DIV_SCLK_AUDIO1, "div_sclk_audio1", "mout_sclk_audio1",
> -			DIV_TOP_PERIC3, 4, 4),
> +	DIV_F(CLK_DIV_SCLK_AUDIO1, "div_sclk_audio1", "mout_sclk_audio1",
> +			DIV_TOP_PERIC3, 4, 4, CLK_SET_RATE_PARENT, 0),
>  	DIV(CLK_DIV_SCLK_AUDIO0, "div_sclk_audio0", "mout_sclk_audio0",
>  			DIV_TOP_PERIC3, 0, 4),
>  
> @@ -693,7 +693,7 @@ static const struct samsung_gate_clock top_gate_clks[] __initconst = {
>  	GATE(CLK_SCLK_SLIMBUS, "sclk_slimbus", "mout_sclk_slimbus",
>  			MUX_ENABLE_TOP_PERIC1, 16, 0, 0),
>  	GATE(CLK_SCLK_AUDIO1, "sclk_audio1", "div_sclk_audio1",
> -			MUX_ENABLE_TOP_PERIC1, 4, 0, 0),
> +			MUX_ENABLE_TOP_PERIC1, 4, CLK_SET_RATE_PARENT, 0),
>  	GATE(CLK_SCLK_AUDIO0, "sclk_audio0", "div_sclk_audio0",
>  			MUX_ENABLE_TOP_PERIC1, 0, 0, 0),
>  };
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/3] clk: exynos5433: Allow audio subsystem clock rate propagation

[Sylwester Nawrocki]

Hi Chanwoo,

On 02/06/2018 05:06 AM, Chanwoo Choi wrote:
>>  drivers/clk/samsung/clk-exynos5433.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
>> index 74b70ddab4d6..d74361736e64 100644
>> --- a/drivers/clk/samsung/clk-exynos5433.c
>> +++ b/drivers/clk/samsung/clk-exynos5433.c
>> @@ -246,14 +246,14 @@ static const struct samsung_fixed_rate_clock top_fixed_clks[] __initconst = {
>>  
>>  static const struct samsung_mux_clock top_mux_clks[] __initconst = {
>>  	/* MUX_SEL_TOP0 */
>> -	MUX(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>> -			4, 1),
>> +	MUX_F(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>> +	      4, 1, CLK_SET_RATE_PARENT, 0),
>
> If you add CLK_SET_RATE_PARENT to 'mout_aud_pll' and mout_aud_pll changes the rate,
> fout_aud_pll's rate will be changed. But, fout_aud_pll is also the parent
> of 'mout_aud_pll_user'. It might change the rate of children of mout_aud_pll_user.
> mout_aud_pll_user would not want to change the parent's clock.
> 
>     fout_aud_pll                          2            2   196608009          0 0  
>        mout_aud_pll_user                  1            1   196608009          0 0  
>        mout_aud_pll                       0            0   196608009          0 0  

I'd say the range of changes is such that the consumers of the affected child 
clocks can cope and could adjust to the changed frequencies. Those consumer 
devices are all components/peripherals of the audio subsystem (LPASS) and, 
for example, in case of TM2 there is no issues at all with varying the AUD PLL
frequency depending on the HDMI audio sample rate. The other audio path uses
the audio CODEC's internal PLL as the root clock source. The AUD PLL frequency
will need to be adjusted somehow anyway, we could also get the PLL clock 
directly and set it's rate, instead of relying on that rate propagation 
algorithm.  I think we could also export a function from the exynos-lpass mfd 
driver for setting the PLL's rate directly, after listing the AUD PLL clock 
in the lpass DT node. That would be more flexible API, easier to adopt for 
various use cases/boards, now we have only TM2. I can't list the PLL clock 
in the sound node, that would not have passed the DT maintainters' review. 

-- 
Regards,
Sylwester

Re: [PATCH 2/3] clk: exynos5433: Allow audio subsystem clock rate propagation

[Chanwoo Choi]

Hi Sylwester,

On 2018년 02월 08일 00:18, Sylwester Nawrocki wrote:
> Hi Chanwoo,
> 
> On 02/06/2018 05:06 AM, Chanwoo Choi wrote:
>>>  drivers/clk/samsung/clk-exynos5433.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
>>> index 74b70ddab4d6..d74361736e64 100644
>>> --- a/drivers/clk/samsung/clk-exynos5433.c
>>> +++ b/drivers/clk/samsung/clk-exynos5433.c
>>> @@ -246,14 +246,14 @@ static const struct samsung_fixed_rate_clock top_fixed_clks[] __initconst = {
>>>  
>>>  static const struct samsung_mux_clock top_mux_clks[] __initconst = {
>>>  	/* MUX_SEL_TOP0 */
>>> -	MUX(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>>> -			4, 1),
>>> +	MUX_F(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>>> +	      4, 1, CLK_SET_RATE_PARENT, 0),
>>
>> If you add CLK_SET_RATE_PARENT to 'mout_aud_pll' and mout_aud_pll changes the rate,
>> fout_aud_pll's rate will be changed. But, fout_aud_pll is also the parent
>> of 'mout_aud_pll_user'. It might change the rate of children of mout_aud_pll_user.
>> mout_aud_pll_user would not want to change the parent's clock.
>>
>>     fout_aud_pll                          2            2   196608009          0 0  
>>        mout_aud_pll_user                  1            1   196608009          0 0  
>>        mout_aud_pll                       0            0   196608009          0 0  
> 
> I'd say the range of changes is such that the consumers of the affected child 
> clocks can cope and could adjust to the changed frequencies. Those consumer 
> devices are all components/peripherals of the audio subsystem (LPASS) and, 

The mout_aud_pll_user has the child clock of serial_3.
serial_3 was used for bluetooth on TM2. If you change the aud_pll
with CLK_SET_RATE_PARENT, it might affect the bluetooth operation.
The bluetooth is only used for transfering the data.

Actually, I'm not sure that this patch might affect bluetooth operation or not.

> for example, in case of TM2 there is no issues at all with varying the AUD PLL
> frequency depending on the HDMI audio sample rate. The other audio path uses
> the audio CODEC's internal PLL as the root clock source. The AUD PLL frequency
> will need to be adjusted somehow anyway, we could also get the PLL clock 
> directly and set it's rate, instead of relying on that rate propagation 
> algorithm.  I think we could also export a function from the exynos-lpass mfd 
> driver for setting the PLL's rate directly, after listing the AUD PLL clock 
> in the lpass DT node. That would be more flexible API, easier to adopt for 
> various use cases/boards, now we have only TM2. I can't list the PLL clock 
> in the sound node, that would not have passed the DT maintainters' review. 
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/3] clk: exynos5433: Allow audio subsystem clock rate propagation

[Sylwester Nawrocki]

Hi Chanwoo,

On 02/09/2018 08:36 AM, Chanwoo Choi wrote:
> On 2018년 02월 08일 00:18, Sylwester Nawrocki wrote:
>> On 02/06/2018 05:06 AM, Chanwoo Choi wrote:
>>>>  drivers/clk/samsung/clk-exynos5433.c | 22 +++++++++++-----------
>>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
>>>> index 74b70ddab4d6..d74361736e64 100644
>>>> --- a/drivers/clk/samsung/clk-exynos5433.c
>>>> +++ b/drivers/clk/samsung/clk-exynos5433.c
>>>> @@ -246,14 +246,14 @@ static const struct samsung_fixed_rate_clock top_fixed_clks[] __initconst = {
>>>>  
>>>>  static const struct samsung_mux_clock top_mux_clks[] __initconst = {
>>>>  	/* MUX_SEL_TOP0 */
>>>> -	MUX(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>>>> -			4, 1),
>>>> +	MUX_F(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>>>> +	      4, 1, CLK_SET_RATE_PARENT, 0),
>>> If you add CLK_SET_RATE_PARENT to 'mout_aud_pll' and mout_aud_pll changes the rate,
>>> fout_aud_pll's rate will be changed. But, fout_aud_pll is also the parent
>>> of 'mout_aud_pll_user'. It might change the rate of children of mout_aud_pll_user.
>>> mout_aud_pll_user would not want to change the parent's clock.
>>>
>>>     fout_aud_pll                          2            2   196608009          0 0  
>>>        mout_aud_pll_user                  1            1   196608009          0 0  
>>>        mout_aud_pll                       0            0   196608009          0 0  
>> I'd say the range of changes is such that the consumers of the affected child 
>> clocks can cope and could adjust to the changed frequencies. Those consumer 
>> devices are all components/peripherals of the audio subsystem (LPASS) and, 
>
> The mout_aud_pll_user has the child clock of serial_3.
> serial_3 was used for bluetooth on TM2. If you change the aud_pll
> with CLK_SET_RATE_PARENT, it might affect the bluetooth operation.
> The bluetooth is only used for transfering the data.
> 
> Actually, I'm not sure that this patch might affect bluetooth operation or not.

You are right, the AUD PLL frequency adjustments would break the bluetooth's 
operation. I double checked and in the downstream kernel only one AUD PLL 
frequency can be set - 196608009. So I will drop this patch and add just
a single PLL_36XX_RATE() entry for that frequency, the PMS values have been
confirmed by the HW team. Only 48000/9600/192000 sample rates will then be 
supported natively and others could be through software rate conversion.

-- 
Regards,
Sylwester

Re: [PATCH 2/3] clk: exynos5433: Allow audio subsystem clock rate propagation

[Chanwoo Choi]

Hi Sylwester,

On 2018년 02월 12일 20:45, Sylwester Nawrocki wrote:
> Hi Chanwoo,
> 
> On 02/09/2018 08:36 AM, Chanwoo Choi wrote:
>> On 2018년 02월 08일 00:18, Sylwester Nawrocki wrote:
>>> On 02/06/2018 05:06 AM, Chanwoo Choi wrote:
>>>>>  drivers/clk/samsung/clk-exynos5433.c | 22 +++++++++++-----------
>>>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
>>>>> index 74b70ddab4d6..d74361736e64 100644
>>>>> --- a/drivers/clk/samsung/clk-exynos5433.c
>>>>> +++ b/drivers/clk/samsung/clk-exynos5433.c
>>>>> @@ -246,14 +246,14 @@ static const struct samsung_fixed_rate_clock top_fixed_clks[] __initconst = {
>>>>>  
>>>>>  static const struct samsung_mux_clock top_mux_clks[] __initconst = {
>>>>>  	/* MUX_SEL_TOP0 */
>>>>> -	MUX(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>>>>> -			4, 1),
>>>>> +	MUX_F(CLK_MOUT_AUD_PLL, "mout_aud_pll", mout_aud_pll_p, MUX_SEL_TOP0,
>>>>> +	      4, 1, CLK_SET_RATE_PARENT, 0),
>>>> If you add CLK_SET_RATE_PARENT to 'mout_aud_pll' and mout_aud_pll changes the rate,
>>>> fout_aud_pll's rate will be changed. But, fout_aud_pll is also the parent
>>>> of 'mout_aud_pll_user'. It might change the rate of children of mout_aud_pll_user.
>>>> mout_aud_pll_user would not want to change the parent's clock.
>>>>
>>>>     fout_aud_pll                          2            2   196608009          0 0  
>>>>        mout_aud_pll_user                  1            1   196608009          0 0  
>>>>        mout_aud_pll                       0            0   196608009          0 0  
>>> I'd say the range of changes is such that the consumers of the affected child 
>>> clocks can cope and could adjust to the changed frequencies. Those consumer 
>>> devices are all components/peripherals of the audio subsystem (LPASS) and, 
>>
>> The mout_aud_pll_user has the child clock of serial_3.
>> serial_3 was used for bluetooth on TM2. If you change the aud_pll
>> with CLK_SET_RATE_PARENT, it might affect the bluetooth operation.
>> The bluetooth is only used for transfering the data.
>>
>> Actually, I'm not sure that this patch might affect bluetooth operation or not.
> 
> You are right, the AUD PLL frequency adjustments would break the bluetooth's 
> operation. I double checked and in the downstream kernel only one AUD PLL 
> frequency can be set - 196608009. So I will drop this patch and add just
> a single PLL_36XX_RATE() entry for that frequency, the PMS values have been
> confirmed by the HW team. Only 48000/9600/192000 sample rates will then be 
> supported natively and others could be through software rate conversion.
> 

OK. Thanks for your check.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH 3/3] clk: exynos5433: Add CLK_IGNORE_UNUSED flag to sclk_ioclk_i2s1_bclk

[Sylwester Nawrocki]

The sclk_ioclk_i2s1_bclk clock is not currently handled by any driver
and disabling this clock by the clk core prevents proper operation
of the I2S1 block. CLK_IGNORE_UNUSED flag is added as a temporary fix.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index d74361736e64..15177f74e125 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -1680,7 +1680,7 @@ static const struct samsung_gate_clock peric_gate_clks[] __initconst = {
 			ENABLE_SCLK_PERIC, 11, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_IOCLK_I2S1_BCLK, "sclk_ioclk_i2s1_bclk",
 			"ioclk_i2s1_bclk_in", ENABLE_SCLK_PERIC, 10,
-			CLK_SET_RATE_PARENT, 0),
+			CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, 0),
 	GATE(CLK_SCLK_SPDIF, "sclk_spdif", "sclk_spdif_peric",
 			ENABLE_SCLK_PERIC, 8, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_PCM1, "sclk_pcm1", "sclk_pcm1_peric",
-- 
2.14.2

Re: [PATCH 3/3] clk: exynos5433: Add CLK_IGNORE_UNUSED flag to sclk_ioclk_i2s1_bclk

[Chanwoo Choi]

On 2018년 02월 05일 23:22, Sylwester Nawrocki wrote:
> The sclk_ioclk_i2s1_bclk clock is not currently handled by any driver
> and disabling this clock by the clk core prevents proper operation
> of the I2S1 block. CLK_IGNORE_UNUSED flag is added as a temporary fix.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index d74361736e64..15177f74e125 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -1680,7 +1680,7 @@ static const struct samsung_gate_clock peric_gate_clks[] __initconst = {
>  			ENABLE_SCLK_PERIC, 11, CLK_SET_RATE_PARENT, 0),
>  	GATE(CLK_SCLK_IOCLK_I2S1_BCLK, "sclk_ioclk_i2s1_bclk",
>  			"ioclk_i2s1_bclk_in", ENABLE_SCLK_PERIC, 10,
> -			CLK_SET_RATE_PARENT, 0),
> +			CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, 0),
>  	GATE(CLK_SCLK_SPDIF, "sclk_spdif", "sclk_spdif_peric",
>  			ENABLE_SCLK_PERIC, 8, CLK_SET_RATE_PARENT, 0),
>  	GATE(CLK_SCLK_PCM1, "sclk_pcm1", "sclk_pcm1_peric",
> 

If any device driver handles the sclk_ioclk_i2s1_bclk on later,
CLK_IGNORE_UNUSED will be ignored. Looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 3/3] clk: exynos5433: Add CLK_IGNORE_UNUSED flag to sclk_ioclk_i2s1_bclk

[Sylwester Nawrocki]

On 02/06/2018 05:08 AM, Chanwoo Choi wrote:
> On 2018년 02월 05일 23:22, Sylwester Nawrocki wrote:
>> The sclk_ioclk_i2s1_bclk clock is not currently handled by any driver
>> and disabling this clock by the clk core prevents proper operation
>> of the I2S1 block. CLK_IGNORE_UNUSED flag is added as a temporary fix.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos5433.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
>> index d74361736e64..15177f74e125 100644
>> --- a/drivers/clk/samsung/clk-exynos5433.c
>> +++ b/drivers/clk/samsung/clk-exynos5433.c
>> @@ -1680,7 +1680,7 @@ static const struct samsung_gate_clock peric_gate_clks[] __initconst = {
>>  			ENABLE_SCLK_PERIC, 11, CLK_SET_RATE_PARENT, 0),
>>  	GATE(CLK_SCLK_IOCLK_I2S1_BCLK, "sclk_ioclk_i2s1_bclk",
>>  			"ioclk_i2s1_bclk_in", ENABLE_SCLK_PERIC, 10,
>> -			CLK_SET_RATE_PARENT, 0),
>> +			CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, 0),
>>  	GATE(CLK_SCLK_SPDIF, "sclk_spdif", "sclk_spdif_peric",
>>  			ENABLE_SCLK_PERIC, 8, CLK_SET_RATE_PARENT, 0),
>>  	GATE(CLK_SCLK_PCM1, "sclk_pcm1", "sclk_pcm1_peric",
>>
> If any device driver handles the sclk_ioclk_i2s1_bclk on later,
> CLK_IGNORE_UNUSED will be ignored. Looks good to me.
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks, I have applied the patch to clk/samsung tree.

Re: [PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Chanwoo Choi]

Hi Sylwester,

When I developed the clk-exynos5433.c I referred to the following description.
TRM specified that "Samsung recommends only the values
between 252MH ~ 400MHz in the PMS2460X PMS value" for aud_pll.

It looks like that you refer to clk-exynos5420.c driver.
But, I'm wondering exynos5433 might not guarantee the additional clock
of this patch as the stable clock.

On 2018년 02월 05일 23:22, Sylwester Nawrocki wrote:
> Add more definitions to the exynos5433_aud_pll_rates table so the
> AUD_PLL can be used as a root clock source for the I2S1 and the HDMI
> interface.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index db270908037a..74b70ddab4d6 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -765,6 +765,14 @@ static const struct samsung_pll_rate_table exynos5433_aud_pll_rates[] __initcons
>  	PLL_36XX_RATE(294912000U,  98, 1, 3,  19923),
>  	PLL_36XX_RATE(288000000U,  96, 1, 3,      0),
>  	PLL_36XX_RATE(252000000U,  84, 1, 3,      0),
> +	PLL_36XX_RATE(200000000U, 200, 3, 3,      0),
> +	PLL_36XX_RATE(196608001U, 197, 3, 3, -25690),
> +	PLL_36XX_RATE(180633609U, 301, 5, 3,   3671),
> +	PLL_36XX_RATE(131072006U, 131, 3, 3,   4719),
> +	PLL_36XX_RATE(100000000U, 200, 3, 4,      0),
> +	PLL_36XX_RATE(65536003U,  131, 3, 4,   4719),
> +	PLL_36XX_RATE(49152000U,  197, 3, 5, -25690),
> +	PLL_36XX_RATE(32768001U,  131, 3, 5,   4719),
>  	{ /* sentinel */ }
>  };
> 
> --
> 2.14.2
> 
> 
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Sylwester Nawrocki]

On 02/06/2018 03:44 AM, Chanwoo Choi wrote:
> When I developed the clk-exynos5433.c I referred to the following description.
> TRM specified that "Samsung recommends only the values
> between 252MH ~ 400MHz in the PMS2460X PMS value" for aud_pll.

Thanks, I somehow missed it. There is also another sentence pointing to
a contact person if other values are needed.

> It looks like that you refer to clk-exynos5420.c driver.
> But, I'm wondering exynos5433 might not guarantee the additional clock
> of this patch as the stable clock.

I took the values from downstream SM-N910C_LL kernel, I would say they
are well tested and reliable. How about adding just these 3 entries:

+	PLL_36XX_RATE(196608001U, 197, 3, 3, -25690),
+	PLL_36XX_RATE(180633609U, 301, 5, 3,   3671),
+	PLL_36XX_RATE(131072006U, 131, 3, 3,   4719),
?
 
They are needed for audio sample rates that are multiple of 32000, 44100
or 48000 Hz. The AUD PLL frequency values which are currently defined
are not useful for anything except fs 48000 (fpll = 393216000 Hz).

We could add new PLL rates: 361267218, 262144000, but I'm not convinced
these would be any better than values used in a shipped product. We would 
need to confirm below values with the HW team, and I'm not sure what 
would be the P, M, S, K set for 262144000 frequency.

	PLL_36XX_RATE(361267218U, 301, 5, 2,   3671),

As it turns out, the 393216000 Hz entry needs correction, the actual 
frequency value from the P, M, S, K equation is 393216003:

-	PLL_36XX_RATE(393216000U, 197, 3, 2, -25690),
+	PLL_36XX_RATE(393216003U, 197, 3, 2, -25690),

Hmm, I have tested with the PLL frequency set to 393216003 Hz and there
are some glitches during audio playback, I can't get it to work properly.

--
Regards,
Sylwester

Re: [PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Chanwoo Choi]

On 2018년 02월 07일 19:29, Sylwester Nawrocki wrote:
> On 02/06/2018 03:44 AM, Chanwoo Choi wrote:
>> When I developed the clk-exynos5433.c I referred to the following description.
>> TRM specified that "Samsung recommends only the values
>> between 252MH ~ 400MHz in the PMS2460X PMS value" for aud_pll.
> 
> Thanks, I somehow missed it. There is also another sentence pointing to
> a contact person if other values are needed.
> 
>> It looks like that you refer to clk-exynos5420.c driver.
>> But, I'm wondering exynos5433 might not guarantee the additional clock
>> of this patch as the stable clock.
> 
> I took the values from downstream SM-N910C_LL kernel, I would say they
> are well tested and reliable. How about adding just these 3 entries:
> 
> +	PLL_36XX_RATE(196608001U, 197, 3, 3, -25690),
> +	PLL_36XX_RATE(180633609U, 301, 5, 3,   3671),
> +	PLL_36XX_RATE(131072006U, 131, 3, 3,   4719),
> ?
>  
> They are needed for audio sample rates that are multiple of 32000, 44100
> or 48000 Hz. The AUD PLL frequency values which are currently defined
> are not useful for anything except fs 48000 (fpll = 393216000 Hz).

If you referred to SM-N910C released kernel, I agree.

> 
> We could add new PLL rates: 361267218, 262144000, but I'm not convinced
> these would be any better than values used in a shipped product. We would 
> need to confirm below values with the HW team, and I'm not sure what 
> would be the P, M, S, K set for 262144000 frequency.
> 
> 	PLL_36XX_RATE(361267218U, 301, 5, 2,   3671),
> 
> As it turns out, the 393216000 Hz entry needs correction, the actual 
> frequency value from the P, M, S, K equation is 393216003:
> 
> -	PLL_36XX_RATE(393216000U, 197, 3, 2, -25690),
> +	PLL_36XX_RATE(393216003U, 197, 3, 2, -25690),

As you described, 393216003 might be more correct than 393216000,
IMHO, I think that TRM specified 393216000 instead of 393216003,
because the following equation is clean without any decimal point.
- "393216000 / 8192 = 48000"

I have no any objection, if 393216003 is correct result.

Could you share your equation?
because your result is a little bit different of my result.
- my equation : ((mdiv + kdiv/65535) x 24MHz) / (pdiv x POWER(2,sdiv))

> 
> Hmm, I have tested with the PLL frequency set to 393216003 Hz and there
> are some glitches during audio playback, I can't get it to work properly.
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Sylwester Nawrocki]

On 02/07/2018 12:24 PM, Chanwoo Choi wrote:
> Could you share your equation?
> because your result is a little bit different of my result.
> - my equation : ((mdiv + kdiv/65535) x 24MHz) / (pdiv x POWER(2,sdiv))

It resembles the code from samsung_pll36xx_recalc_rate():

(24 * 10^6 * (M * 2^16 + K)) / (P * 2^S) / 2^16

and a more accurate one

ROUNDDOWN(ROUNDDOWN(24 * 10^6 * (M * 2^16 + K), 0) / ROUNDDOWN(P * 2^S, 0) / 2^16, 0)

Shouldn't you substitute 65535 with 65536?

-- 
Regards,
Sylwester

Re: [PATCH 1/3] clk: exynos5433: Extend list of available AUD_PLL output frequencies

[Chanwoo Choi]

On 2018년 02월 07일 22:04, Sylwester Nawrocki wrote:
> On 02/07/2018 12:24 PM, Chanwoo Choi wrote:
>> Could you share your equation?
>> because your result is a little bit different of my result.
>> - my equation : ((mdiv + kdiv/65535) x 24MHz) / (pdiv x POWER(2,sdiv))
> 
> It resembles the code from samsung_pll36xx_recalc_rate():
> 
> (24 * 10^6 * (M * 2^16 + K)) / (P * 2^S) / 2^16
> 
> and a more accurate one
> 
> ROUNDDOWN(ROUNDDOWN(24 * 10^6 * (M * 2^16 + K), 0) / ROUNDDOWN(P * 2^S, 0) / 2^16, 0)
> 
> Shouldn't you substitute 65535 with 65536?

65536 is right. It is my mistake using 65535.
Thanks for your share.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics