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

[Peng Fan]

> -----Original Message-----
> From: Lukasz Majewski [mailto:lukma at denx.de]
> Sent: 2019年5月8日 14:46
> To: Peng Fan <peng.fan at nxp.com>
> Cc: sbabic at denx.de; festevam at gmail.com; dl-uboot-imx
> <uboot-imx at 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 at 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 at 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.

> 
> 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.

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