On Thu, 9 May 2019 01:13:15 +0000 Peng Fan <peng....@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....@nxp.com> wrote: > > > > > > -----Original Message----- > > > > From: Lukasz Majewski [mailto:lu...@denx.de] > > > > Sent: 2019年5月8日 15:40 > > > > To: Peng Fan <peng....@nxp.com> > > > > Cc: sba...@denx.de; feste...@gmail.com; dl-uboot-imx > > > > <uboot-...@nxp.com>; s...@chromium.org; > > ja...@amarulasolutions.com; > > > > s...@denx.de; u-boot@lists.denx.de; tr...@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....@nxp.com> wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Lukasz Majewski [mailto:lu...@denx.de] > > > > > > Sent: 2019年5月8日 14:46 > > > > > > To: Peng Fan <peng....@nxp.com> > > > > > > Cc: sba...@denx.de; feste...@gmail.com; dl-uboot-imx > > > > > > <uboot-...@nxp.com>; s...@chromium.org; > > > > ja...@amarulasolutions.com; > > > > > > s...@denx.de; u-boot@lists.denx.de; tr...@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....@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....@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-imx8m > > > 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 ? > > 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/imx6q > > > > 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: lu...@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: lu...@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: lu...@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: > > lu...@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: lu...@denx.de
pgpZuD8sISnyW.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot