[U-Boot] [i.MX8MM+CCF 11/41] clk: fixed_rate: export clk_fixed_rate

[Lukasz Majewski <lukma@denx.de>]

On Thu, 16 May 2019 09:58:17 +0000
Peng Fan <peng.fan@nxp.com> wrote:

> > Subject: Re: [i.MX8MM+CCF 11/41] clk: fixed_rate: export
> > clk_fixed_rate
> > 
> > On Thu, 9 May 2019 01:13:15 +0000
> > Peng Fan <peng.fan@nxp.com> wrote:
> >   
> > > > Subject: Re: [i.MX8MM+CCF 11/41] clk: fixed_rate: export
> > > > clk_fixed_rate
> > > >
> > > > On Wed, 8 May 2019 07:45:39 +0000
> > > > Peng Fan <peng.fan@nxp.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > > Sent: 2019年5月8日 15:40
> > > > > > To: Peng Fan <peng.fan@nxp.com>
> > > > > > Cc: sbabic at denx.de; festevam at gmail.com; dl-uboot-imx
> > > > > > <uboot-imx@nxp.com>; sjg at chromium.org;  
> > > > jagan at amarulasolutions.com;  
> > > > > > sr at denx.de; u-boot at lists.denx.de; trini at konsulko.com
> > > > > > Subject: Re: [i.MX8MM+CCF 11/41] clk: fixed_rate: export
> > > > > > clk_fixed_rate
> > > > > >
> > > > > > On Wed, 8 May 2019 06:51:46 +0000 Peng Fan
> > > > > > <peng.fan@nxp.com> wrote:
> > > > > >  
> > > > > > > > -----Original Message-----
> > > > > > > > From: Lukasz Majewski [mailto:lukma at denx.de]
> > > > > > > > Sent: 2019年5月8日 14:46
> > > > > > > > To: Peng Fan <peng.fan@nxp.com>
> > > > > > > > Cc: sbabic at denx.de; festevam at gmail.com; dl-uboot-imx
> > > > > > > > <uboot-imx@nxp.com>; sjg at chromium.org;  
> > > > > > jagan at amarulasolutions.com;  
> > > > > > > > sr at denx.de; u-boot at lists.denx.de; trini at konsulko.com
> > > > > > > > Subject: Re: [i.MX8MM+CCF 11/41] clk: fixed_rate: export
> > > > > > > > clk_fixed_rate
> > > > > > > >
> > > > > > > > On Tue, 7 May 2019 13:27:45 +0000 Peng Fan
> > > > > > > > <peng.fan@nxp.com> wrote:
> > > > > > > >  
> > > > > > > > > > Subject: Re: [i.MX8MM+CCF 11/41] clk: fixed_rate:
> > > > > > > > > > export clk_fixed_rate
> > > > > > > > > >
> > > > > > > > > > Hi Peng,
> > > > > > > > > >  
> > > > > > > > > > > Export the structure for others to use.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/clk/clk_fixed_rate.c | 8 +-------
> > > > > > > > > > > include/linux/clk-provider.h | 7 +++++++
> > > > > > > > > > >  2 files changed, 8 insertions(+), 7 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/clk/clk_fixed_rate.c
> > > > > > > > > > > b/drivers/clk/clk_fixed_rate.c index
> > > > > > > > > > > 089f060a23..069e643fbc 100644 ---
> > > > > > > > > > > a/drivers/clk/clk_fixed_rate.c +++
> > > > > > > > > > > b/drivers/clk/clk_fixed_rate.c @@ -6,13 +6,7 @@
> > > > > > > > > > > #include <common.h>  #include <clk-uclass.h>
> > > > > > > > > > > #include <dm.h> - -struct clk_fixed_rate {
> > > > > > > > > > > -	struct clk clk;
> > > > > > > > > > > -	unsigned long fixed_rate;
> > > > > > > > > > > -};
> > > > > > > > > > > -
> > > > > > > > > > > -#define to_clk_fixed_rate(dev)	((struct
> > > > > > > > > > > clk_fixed_rate *)dev_get_platdata(dev)) +#include
> > > > > > > > > > > <linux/clk-provider.h>
> > > > > > > > > > >
> > > > > > > > > > >  static ulong clk_fixed_rate_get_rate(struct clk
> > > > > > > > > > > *clk) { diff --git a/include/linux/clk-provider.h
> > > > > > > > > > > b/include/linux/clk-provider.h index
> > > > > > > > > > > 3ed0db86d2..b2bed768b6 100644 ---
> > > > > > > > > > > a/include/linux/clk-provider.h +++
> > > > > > > > > > > b/include/linux/clk-provider.h @@ -112,6 +112,13
> > > > > > > > > > > @@ struct clk_fixed_factor {  #define
> > > > > > > > > > > to_clk_fixed_factor(_clk) container_of(_clk,
> > > > > > > > > > > struct clk_fixed_factor,\ clk)
> > > > > > > > > > >
> > > > > > > > > > > +struct clk_fixed_rate {
> > > > > > > > > > > +	struct clk clk;
> > > > > > > > > > > +	unsigned long fixed_rate; };  
> > > > > > > > > >
> > > > > > > > > > I think that this struct shall stay where it was.
> > > > > > > > > > Moreover, the clk-provider.h is not the API to be
> > > > > > > > > > used by other parts of the clock API.
> > > > > > > > > >
> > > > > > > > > > The clk_fixed_rate shall be accessed via get_rate()
> > > > > > > > > > only and in IMX6Q it is available in early SPL
> > > > > > > > > > (parsed from dts /clocks property
> > > > > > > > > > - the 24MHz OSC)  
> > > > > > > > > > > +
> > > > > > > > > > > +#define to_clk_fixed_rate(dev)	((struct
> > > > > > > > > > > clk_fixed_rate *)dev_get_platdata(dev)) +  int
> > > > > > > > > > > clk_register(struct clk *clk, const char
> > > > > > > > > > > *drv_name, ulong drv_data, const char *name,
> > > > > > > > > > >  		 const char *parent_name);  
> > > > > > > > > >
> > > > > > > > > > Please explain why iMX8MM needs such global
> > > > > > > > > > export?  
> > > > > > > > >
> > > > > > > > > In clk-imx8mm.c, first configure ARM clk to osc24M
> > > > > > > > > fixed clk to change pll clock.
> > > > > > > > > +       /* Configure ARM to osc24M */
> > > > > > > > > +       clk_get_by_id(IMX8MM_CLK_A53_SRC, &clkp);
> > > > > > > > > +       uclass_get_device_by_name(UCLASS_CLK,
> > > > > > > > > "clock-osc-24m",  
> > > > > > > > &devp);  
> > > > > > > > > +       clkp1 = &to_clk_fixed_rate(devp)->clk;
> > > > > > > > > +       clk_set_parent(clkp, clkp1);  
> > > > > > > >
> > > > > > > > This code looks a bit strange to me. Why imx8mm sets
> > > > > > > > parent here?  
> > > > > > >
> > > > > > > The A53 clk could not change on the fly. There is a mux
> > > > > > > here, one is PLL, one is OSC, And there are others. If we
> > > > > > > want to change the pll clock which is currently being
> > > > > > > used by A53, we need first switch the A53 clk to source
> > > > > > > from OSC, then change pll clock, then switch A53 clk back
> > > > > > > to PLL.  
> > > > > >
> > > > > > The above description looks like a "standard" procedure for
> > > > > > bypassing PLL when it is going to be locked.
> > > > > >
> > > > > > The same is also performed on IMX6Q:
> > > > > >
> > > > > > https://elixir.bootlin.com/linux/v5.1/source/drivers/clk/imx/clk
> > > > > > -imx
> > > > > > 6q.c#L88
> > > > > >
> > > > > > But I've not ported that part from the original Linux source
> > > > > > code.  
> > > > >
> > > > > i.MX8MM is different.
> > > > >
> > > > > This is how Linux change cpu's clock.
> > > > > https://elixir.bootlin.com/linux/v5.1/source/drivers/clk/imx/clk-i
> > > > > mx8m
> > > > > m.c#L655
> > > > >
> > > > > It has a new cpu clk driver, I do not want to introduce that
> > > > > complexity in U-Boot.  
> > > >
> > > > Ok.
> > > >  
> > > > >
> > > > > This patch is just a simplified step of the upper Linux
> > > > > code.  
> > > >
> > > > My point is that this driver pollutes the fixed-clock code and
> > > > makes it imx8 specific.  
> > >
> > > If i.MX7 switch to CCF, it also has such requirement.
> > >
> > > I am not sure others, Linux exports
> > > grep "to_clk_fix" ./include/linux/clk-provider.h -rn 313:#define
> > > to_clk_fixed_rate(_hw) container_of(_hw, struct clk_fixed_rate,
> > > hw) 575:#define to_clk_fixed_factor(_hw) container_of(_hw, struct
> > > clk_fixed_factor, hw)
> > >
> > > I think it does not hurt for uboot also export it.
> > >  
> > > >  
> > > > >
> > > > > The reason to configure the clk here is that ROM not
> > > > > configure the clock following datasheet even it not hurt, but
> > > > > better to configure it right in SPL.  
> > > >
> > > > So this code is for fixing bugs (incomplete initialisation) in
> > > > SoC's ROM?  
> > >
> > > Strictly speaking it is not a bug. The datasheet opp has 1.2GHz,
> > > but not 1GHz configured by ROM. But actually when power up, the
> > > initial freq is 24MHz. Just configure in uboot to follow
> > > datasheet and might make Linux kernel happy.  
> > 
> > For IMX6Q there is a table in the User Manual, which describes the
> > state (i.e. the clock frequencies of the IP blocks) in which is the
> > board when ROM handles the execution to SPL.
> > 
> > I also assume that 24 MHz Core frequency is too low and will be
> > fixed in next ROM (SoC) revisions ?  
> 
> When power up, before ROM configure PLL, the core will runs at osc
> clock, After ROM configures ARM PLL, the core will runs at higher
> frequency.
> 
> It is just ROM not configure ARM PLL following datasheet, but it is
> not actually an issue here. U-Boot just following spec to reconfigure
> the ARM PLL.

Ach... OK. I must have misunderstood the last e-mail. The issue is with
difference between 1.2 GHz and 1.0 GHz PLL configuration.

> 
> Regards,
> Peng.
> 
> >   
> > >
> > > Thanks,
> > > Peng.
> > >  
> > > >  
> > > > >
> > > > > Thanks,
> > > > > Peng.
> > > > >  
> > > > > >  
> > > > > > >  
> > > > > > > >
> > > > > > > > In the imx6q I write "osc" as a part of the clock tree:
> > > > > > > >
> > > > > > > >         clk_dm(IMX6QDL_CLK_PLL2,
> > > > > > > >                imx_clk_pllv3(IMX_PLLV3_GENERIC,  
> > "pll2_bus",  
> > > > > > > > "osc", base + 0x30, 0x1));
> > > > > > > >
> > > > > > > > And here the parent is set as "osc".
> > > > > > > >
> > > > > > > > Moreover, the "osc" or in your case "clock-osc-24m"
> > > > > > > > shall be available in "clocks" DTS node and hence
> > > > > > > > parsed / setup from there (and be available in the clk
> > > > > > > > uclass list). 
> > > > > > > > > +
> > > > > > > > > +       /* Configure ARM PLL to 1.2GHz */
> > > > > > > > > +       clk_get_by_id(IMX8MM_ARM_PLL, &clkp1);
> > > > > > > > > +       clk_set_rate(clkp1, 1200000000UL);  
> > > > > > > >
> > > > > > > > Shouldn't this be set in DTS ? As it is now - it seems
> > > > > > > > like a hardcoded value for early SPL clock setup. Am I
> > > > > > > > right?  
> > > > > > >
> > > > > > > You mean get freq from cpu opp and set cpu freq? I do not
> > > > > > > have good idea how to set in DTS.  
> > > > > >
> > > > > > I'm thinking about following description of the "osc" fixed
> > > > > > clock:
> > > > > > https://elixir.bootlin.com/linux/v5.1/source/arch/arm/boot/dts/i
> > > > > > mx6q
> > > > > > dl.dtsi# L64
> > > > > >
> > > > > > and
> > > > > >
> > > > > >  
> > > >  
> > http://git.denx.de/?p=u-boot/u-boot-imx.git;a=blob;f=arch/arm/dts/imx6qdl.  
> > > > > > dtsi;h=c0a94780087382a6e2805b7d6572f3e5e294e302;hb=HEAD#l53
> > > > > >
> > > > > > The "fixed-clock" code has been adjusted to comply with the
> > > > > > above DTS (which is provided in pre-reloc SPL - even before
> > > > > > console and ddr setup).
> > > > > >
> > > > > > (I had to use debug uart for debugging).
> > > > > >  
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Peng.
> > > > > > >  
> > > > > > > >  
> > > > > > > > > +       clk_get_by_id(IMX8MM_ARM_PLL_OUT, &clkp1);
> > > > > > > > > +       clk_enable(clkp1);
> > > > > > > > > +       clk_set_parent(clkp, clkp1);
> > > > > > > > > +
> > > > > > > > > +       /* Configure DIV to 1.2GHz */
> > > > > > > > > +       clk_get_by_id(IMX8MM_CLK_A53_DIV, &clkp1);
> > > > > > > > > +       clk_set_rate(clkp1, 1200000000UL);
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Peng.
> > > > > > > > >  
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 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  
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > 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  
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > 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  
> > > >
> > > >
> > > >
> > > >
> > > > 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  
> > 
> > 
> > 
> > 
> > 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  




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: <http://lists.denx.de/pipermail/u-boot/attachments/20190516/ddd985ed/attachment.sig>