diff mbox series

[03/18] clk: imx: clk-imxrt1050: setup PLL5 for video in non-SPL

Message ID 20200226171601.31142-4-giulio.benetti@benettiengineering.com
State Superseded
Headers show
Series i.MXRT1050 add LCDIF support | expand

Commit Message

Giulio Benetti Feb. 26, 2020, 5:15 p.m. UTC 
mxsfb needs PLL5 as source, so let's setup it and set it as source for
mxsfb(lcdif).

Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
---
 drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Fabio Estevam Feb. 26, 2020, 5:37 p.m. UTC | #1 
Hi Giulio,

On Wed, Feb 26, 2020 at 2:16 PM Giulio Benetti
<giulio.benetti at benettiengineering.com> wrote:
>
> mxsfb needs PLL5 as source, so let's setup it and set it as source for
> mxsfb(lcdif).
>
> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
> ---
>  drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/imx/clk-imxrt1050.c b/drivers/clk/imx/clk-imxrt1050.c
> index e33d426363..2819ffb9ac 100644
> --- a/drivers/clk/imx/clk-imxrt1050.c
> +++ b/drivers/clk/imx/clk-imxrt1050.c
> @@ -238,9 +238,9 @@ static int imxrt1050_clk_probe(struct udevice *dev)
>         clk_dm(IMXRT1050_CLK_LCDIF,
>                imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70, 28));
>
> -#ifdef CONFIG_SPL_BUILD
>         struct clk *clk, *clk1;
>
> +#ifdef CONFIG_SPL_BUILD
>         /* bypass pll1 before setting its rate */
>         clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
>         clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
> @@ -271,7 +271,18 @@ static int imxrt1050_clk_probe(struct udevice *dev)
>
>         clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
>         clk_set_parent(clk1, clk);
> +#else
> +       /* Set PLL5 for LCDIF to its default 650Mhz */
> +       clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
> +       clk_enable(clk);
> +       clk_set_rate(clk, 650000000UL);
> +
> +       clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
> +       clk_set_parent(clk1, clk);
>
> +       /* Configure PLL5 as LCDIF source */
> +       clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
> +       clk_set_parent(clk1, clk);

This is more like a board design decision and IMHO should not be
hardcoded as part of the clock driver.

Other users may want to use a different clock source for the eLCDIF driver.

Setting the clock parent in board device tree makes more sense.
Giulio Benetti Feb. 26, 2020, 5:54 p.m. UTC | #2 
Hi Fabio,

On 2/26/20 6:37 PM, Fabio Estevam wrote:
> Hi Giulio,
> 
> On Wed, Feb 26, 2020 at 2:16 PM Giulio Benetti
> <giulio.benetti at benettiengineering.com> wrote:
>>
>> mxsfb needs PLL5 as source, so let's setup it and set it as source for
>> mxsfb(lcdif).
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
>> ---
>>   drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/imx/clk-imxrt1050.c b/drivers/clk/imx/clk-imxrt1050.c
>> index e33d426363..2819ffb9ac 100644
>> --- a/drivers/clk/imx/clk-imxrt1050.c
>> +++ b/drivers/clk/imx/clk-imxrt1050.c
>> @@ -238,9 +238,9 @@ static int imxrt1050_clk_probe(struct udevice *dev)
>>          clk_dm(IMXRT1050_CLK_LCDIF,
>>                 imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70, 28));
>>
>> -#ifdef CONFIG_SPL_BUILD
>>          struct clk *clk, *clk1;
>>
>> +#ifdef CONFIG_SPL_BUILD
>>          /* bypass pll1 before setting its rate */
>>          clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
>>          clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
>> @@ -271,7 +271,18 @@ static int imxrt1050_clk_probe(struct udevice *dev)
>>
>>          clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
>>          clk_set_parent(clk1, clk);
>> +#else
>> +       /* Set PLL5 for LCDIF to its default 650Mhz */
>> +       clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
>> +       clk_enable(clk);
>> +       clk_set_rate(clk, 650000000UL);
>> +
>> +       clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
>> +       clk_set_parent(clk1, clk);
>>
>> +       /* Configure PLL5 as LCDIF source */
>> +       clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
>> +       clk_set_parent(clk1, clk);
> 
> This is more like a board design decision and IMHO should not be
> hardcoded as part of the clock driver.
> 
> Other users may want to use a different clock source for the eLCDIF driver.
> 
> Setting the clock parent in board device tree makes more sense.

Yes, it's a good idea. Doing this I've taken this[1] as example.
So I don't know where in u-boot PLLs are initialized according to a dts 
file, can you please provide me an example? I will be happy to modify 
this according to that!

Thank you

[1]: 
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-imx8mm.c#L398-409

Best regards
Fabio Estevam Feb. 26, 2020, 5:59 p.m. UTC | #3 
Hi Giulio,

On Wed, Feb 26, 2020 at 2:54 PM Giulio Benetti
<giulio.benetti at benettiengineering.com> wrote:

> Yes, it's a good idea. Doing this I've taken this[1] as example.
> So I don't know where in u-boot PLLs are initialized according to a dts
> file, can you please provide me an example? I will be happy to modify
> this according to that!

In the kernel device trees we use the 'assigned-clocks' and
'assigned-clock-parents' properties to establish a clock parent
relationship.

I suggest we follow the same approach in U-Boot.

Thanks
Giulio Benetti Feb. 26, 2020, 6:16 p.m. UTC | #4 
On 2/26/20 6:59 PM, Fabio Estevam wrote:
> Hi Giulio,
> 
> On Wed, Feb 26, 2020 at 2:54 PM Giulio Benetti
> <giulio.benetti at benettiengineering.com> wrote:
> 
>> Yes, it's a good idea. Doing this I've taken this[1] as example.
>> So I don't know where in u-boot PLLs are initialized according to a dts
>> file, can you please provide me an example? I will be happy to modify
>> this according to that!
> 
> In the kernel device trees we use the 'assigned-clocks' and
> 'assigned-clock-parents' properties to establish a clock parent
> relationship.
> 
> I suggest we follow the same approach in U-Boot.

Oh, I've seen now, need to study it before, but now in my mind it's 
getting more clear how that works. But will this work even if shrinked 
CCF in u-boot can't set parent clocks(at least this is what I've 
understood)? I mean, basically here for LCDIF I see that only last 
divider get set for achieving pixel-clock, while all parents are get 
only to recalcute the "last divider parent clock".

Also, I can't understand, is it ok setting PLL5 to 650Mhz and un-bypass 
it? The problem is only about clk_set_parent() for LCDIF?

Because if a peripheral would set a PLL5 frequency and another 
peripheral use it as parent, then it would set it again.

Best regards
Fabio Estevam Feb. 27, 2020, 6:31 p.m. UTC | #5 
Hi Giulio,

On Wed, Feb 26, 2020 at 3:16 PM Giulio Benetti
<giulio.benetti at benettiengineering.com> wrote:

> Oh, I've seen now, need to study it before, but now in my mind it's
> getting more clear how that works. But will this work even if shrinked
> CCF in u-boot can't set parent clocks(at least this is what I've

I haven't checked whether 'assigned-clock-parents' works in U-Boot.

> understood)? I mean, basically here for LCDIF I see that only last
> divider get set for achieving pixel-clock, while all parents are get
> only to recalcute the "last divider parent clock".
>
> Also, I can't understand, is it ok setting PLL5 to 650Mhz and un-bypass
> it? The problem is only about clk_set_parent() for LCDIF?

The problem I saw was about hard coding the parent of LCDIF inside the
clock driver.

> Because if a peripheral would set a PLL5 frequency and another
> peripheral use it as parent, then it would set it again.

Yes, but if we leave the correct clock parent decision to be made in
the board dts, we are safe.
Lukasz Majewski March 8, 2020, 8:27 p.m. UTC | #6 
On Wed, 26 Feb 2020 18:15:46 +0100
Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:

> mxsfb needs PLL5 as source, so let's setup it and set it as source for
> mxsfb(lcdif).
> 
> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
> ---
>  drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imxrt1050.c
> b/drivers/clk/imx/clk-imxrt1050.c index e33d426363..2819ffb9ac 100644
> --- a/drivers/clk/imx/clk-imxrt1050.c
> +++ b/drivers/clk/imx/clk-imxrt1050.c
> @@ -238,9 +238,9 @@ static int imxrt1050_clk_probe(struct udevice
> *dev) clk_dm(IMXRT1050_CLK_LCDIF,
>  	       imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70,
> 28)); 
> -#ifdef CONFIG_SPL_BUILD
>  	struct clk *clk, *clk1;
>  
> +#ifdef CONFIG_SPL_BUILD
>  	/* bypass pll1 before setting its rate */
>  	clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
>  	clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
> @@ -271,7 +271,18 @@ static int imxrt1050_clk_probe(struct udevice
> *dev) 
>  	clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
>  	clk_set_parent(clk1, clk);
> +#else
> +	/* Set PLL5 for LCDIF to its default 650Mhz */
> +	clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
> +	clk_enable(clk);
> +	clk_set_rate(clk, 650000000UL);
> +
> +	clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
> +	clk_set_parent(clk1, clk);
>  
> +	/* Configure PLL5 as LCDIF source */
> +	clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
> +	clk_set_parent(clk1, clk);
>  #endif
>  
>  	return 0;

Reviewed-by: Lukasz Majewski <lukma at denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200308/27e185ac/attachment.sig>
Giulio Benetti March 8, 2020, 9:05 p.m. UTC | #7 
Hi Lukasz,

On 3/8/20 9:27 PM, Lukasz Majewski wrote:
> On Wed, 26 Feb 2020 18:15:46 +0100
> Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:
> 
>> mxsfb needs PLL5 as source, so let's setup it and set it as source for
>> mxsfb(lcdif).
>>
>> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
>> ---
>>   drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/imx/clk-imxrt1050.c
>> b/drivers/clk/imx/clk-imxrt1050.c index e33d426363..2819ffb9ac 100644
>> --- a/drivers/clk/imx/clk-imxrt1050.c
>> +++ b/drivers/clk/imx/clk-imxrt1050.c
>> @@ -238,9 +238,9 @@ static int imxrt1050_clk_probe(struct udevice
>> *dev) clk_dm(IMXRT1050_CLK_LCDIF,
>>   	       imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70,
>> 28));
>> -#ifdef CONFIG_SPL_BUILD
>>   	struct clk *clk, *clk1;
>>   
>> +#ifdef CONFIG_SPL_BUILD
>>   	/* bypass pll1 before setting its rate */
>>   	clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
>>   	clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
>> @@ -271,7 +271,18 @@ static int imxrt1050_clk_probe(struct udevice
>> *dev)
>>   	clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
>>   	clk_set_parent(clk1, clk);
>> +#else
>> +	/* Set PLL5 for LCDIF to its default 650Mhz */
>> +	clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
>> +	clk_enable(clk);
>> +	clk_set_rate(clk, 650000000UL);
>> +
>> +	clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
>> +	clk_set_parent(clk1, clk);
>>   
>> +	/* Configure PLL5 as LCDIF source */
>> +	clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
>> +	clk_set_parent(clk1, clk);

As pointed by Fabio, this ^^^ should be substituted with a using 
assigned-parent-clocks in dts instead of being hardcoded here.
What do you think about it?

Thanks for reviewing and
best regards
Lukasz Majewski March 9, 2020, 9:11 a.m. UTC | #8 
On Sun, 8 Mar 2020 22:05:42 +0100
Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:

> Hi Lukasz,
> 
> On 3/8/20 9:27 PM, Lukasz Majewski wrote:
> > On Wed, 26 Feb 2020 18:15:46 +0100
> > Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:
> >   
> >> mxsfb needs PLL5 as source, so let's setup it and set it as source
> >> for mxsfb(lcdif).
> >>
> >> Signed-off-by: Giulio Benetti
> >> <giulio.benetti at benettiengineering.com> ---
> >>   drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
> >>   1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/imx/clk-imxrt1050.c
> >> b/drivers/clk/imx/clk-imxrt1050.c index e33d426363..2819ffb9ac
> >> 100644 --- a/drivers/clk/imx/clk-imxrt1050.c
> >> +++ b/drivers/clk/imx/clk-imxrt1050.c
> >> @@ -238,9 +238,9 @@ static int imxrt1050_clk_probe(struct udevice
> >> *dev) clk_dm(IMXRT1050_CLK_LCDIF,
> >>   	       imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70,
> >> 28));
> >> -#ifdef CONFIG_SPL_BUILD
> >>   	struct clk *clk, *clk1;
> >>   
> >> +#ifdef CONFIG_SPL_BUILD
> >>   	/* bypass pll1 before setting its rate */
> >>   	clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
> >>   	clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
> >> @@ -271,7 +271,18 @@ static int imxrt1050_clk_probe(struct udevice
> >> *dev)
> >>   	clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
> >>   	clk_set_parent(clk1, clk);
> >> +#else
> >> +	/* Set PLL5 for LCDIF to its default 650Mhz */
> >> +	clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
> >> +	clk_enable(clk);
> >> +	clk_set_rate(clk, 650000000UL);
> >> +
> >> +	clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
> >> +	clk_set_parent(clk1, clk);
> >>   
> >> +	/* Configure PLL5 as LCDIF source */
> >> +	clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
> >> +	clk_set_parent(clk1, clk);  
> 
> As pointed by Fabio, this ^^^ should be substituted with a using 
> assigned-parent-clocks in dts instead of being hardcoded here.

Upss.. Apparently I've missed the conversation. Thanks for pointing
this out.

> What do you think about it?

If it is relatively easy to do then I'm for it.

> 
> Thanks for reviewing and
> best regards




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200309/5b3fddc6/attachment.sig>
Giulio Benetti March 22, 2020, 6:25 p.m. UTC | #9 
Hi Lukasz, Fabio,

On 3/9/20 10:11 AM, Lukasz Majewski wrote:
> On Sun, 8 Mar 2020 22:05:42 +0100
> Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:
> 
>> Hi Lukasz,
>>
>> On 3/8/20 9:27 PM, Lukasz Majewski wrote:
>>> On Wed, 26 Feb 2020 18:15:46 +0100
>>> Giulio Benetti <giulio.benetti at benettiengineering.com> wrote:
>>>    
>>>> mxsfb needs PLL5 as source, so let's setup it and set it as source
>>>> for mxsfb(lcdif).
>>>>
>>>> Signed-off-by: Giulio Benetti
>>>> <giulio.benetti at benettiengineering.com> ---
>>>>    drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++-
>>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/imx/clk-imxrt1050.c
>>>> b/drivers/clk/imx/clk-imxrt1050.c index e33d426363..2819ffb9ac
>>>> 100644 --- a/drivers/clk/imx/clk-imxrt1050.c
>>>> +++ b/drivers/clk/imx/clk-imxrt1050.c
>>>> @@ -238,9 +238,9 @@ static int imxrt1050_clk_probe(struct udevice
>>>> *dev) clk_dm(IMXRT1050_CLK_LCDIF,
>>>>    	       imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70,
>>>> 28));
>>>> -#ifdef CONFIG_SPL_BUILD
>>>>    	struct clk *clk, *clk1;
>>>>    
>>>> +#ifdef CONFIG_SPL_BUILD
>>>>    	/* bypass pll1 before setting its rate */
>>>>    	clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
>>>>    	clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
>>>> @@ -271,7 +271,18 @@ static int imxrt1050_clk_probe(struct udevice
>>>> *dev)
>>>>    	clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
>>>>    	clk_set_parent(clk1, clk);
>>>> +#else
>>>> +	/* Set PLL5 for LCDIF to its default 650Mhz */
>>>> +	clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
>>>> +	clk_enable(clk);
>>>> +	clk_set_rate(clk, 650000000UL);
>>>> +
>>>> +	clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
>>>> +	clk_set_parent(clk1, clk);
>>>>    
>>>> +	/* Configure PLL5 as LCDIF source */
>>>> +	clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
>>>> +	clk_set_parent(clk1, clk);
>>
>> As pointed by Fabio, this ^^^ should be substituted with a using
>> assigned-parent-clocks in dts instead of being hardcoded here.
> 
> Upss.. Apparently I've missed the conversation. Thanks for pointing
> this out.
> 
>> What do you think about it?
> 
> If it is relatively easy to do then I'm for it.

Yes, I've done it.

I'm going to send v2 series soon.

Best regards
diff mbox series

Patch

diff --git a/drivers/clk/imx/clk-imxrt1050.c b/drivers/clk/imx/clk-imxrt1050.c
index e33d426363..2819ffb9ac 100644
--- a/drivers/clk/imx/clk-imxrt1050.c
+++ b/drivers/clk/imx/clk-imxrt1050.c
@@ -238,9 +238,9 @@  static int imxrt1050_clk_probe(struct udevice *dev)
 	clk_dm(IMXRT1050_CLK_LCDIF,
 	       imx_clk_gate2("lcdif", "lcdif_podf", base + 0x70, 28));
 
-#ifdef CONFIG_SPL_BUILD
 	struct clk *clk, *clk1;
 
+#ifdef CONFIG_SPL_BUILD
 	/* bypass pll1 before setting its rate */
 	clk_get_by_id(IMXRT1050_CLK_PLL1_REF_SEL, &clk);
 	clk_get_by_id(IMXRT1050_CLK_PLL1_BYPASS, &clk1);
@@ -271,7 +271,18 @@  static int imxrt1050_clk_probe(struct udevice *dev)
 
 	clk_get_by_id(IMXRT1050_CLK_PLL3_BYPASS, &clk1);
 	clk_set_parent(clk1, clk);
+#else
+	/* Set PLL5 for LCDIF to its default 650Mhz */
+	clk_get_by_id(IMXRT1050_CLK_PLL5_VIDEO, &clk);
+	clk_enable(clk);
+	clk_set_rate(clk, 650000000UL);
+
+	clk_get_by_id(IMXRT1050_CLK_PLL5_BYPASS, &clk1);
+	clk_set_parent(clk1, clk);
 
+	/* Configure PLL5 as LCDIF source */
+	clk_get_by_id(IMXRT1050_CLK_LCDIF_SEL, &clk1);
+	clk_set_parent(clk1, clk);
 #endif
 
 	return 0;