| Message ID | 20200226171601.31142-4-giulio.benetti@benettiengineering.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | i.MXRT1050 add LCDIF support | expand |
Hi Giulio, On Wed, Feb 26, 2020 at 2:16 PM Giulio Benetti <giulio.benetti@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@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.
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@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@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
Hi Giulio, On Wed, Feb 26, 2020 at 2:54 PM Giulio Benetti <giulio.benetti@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
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@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
Hi Giulio, On Wed, Feb 26, 2020 at 3:16 PM Giulio Benetti <giulio.benetti@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.
On Wed, 26 Feb 2020 18:15:46 +0100 Giulio Benetti <giulio.benetti@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@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@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@denx.de
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@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@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
On Sun, 8 Mar 2020 22:05:42 +0100 Giulio Benetti <giulio.benetti@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@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@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@denx.de
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@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@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@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 --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;
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@benettiengineering.com> --- drivers/clk/imx/clk-imxrt1050.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)