On Thu, 16 May 2019 09:58:17 +0000 Peng Fan <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Lukasz Majewski [mailto:[email protected]] > > > > > > Sent: 2019年5月8日 15:40 > > > > > > To: Peng Fan <[email protected]> > > > > > > Cc: [email protected]; [email protected]; dl-uboot-imx > > > > > > <[email protected]>; [email protected]; > > > > [email protected]; > > > > > > [email protected]; [email protected]; [email protected] > > > > > > 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 > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Lukasz Majewski [mailto:[email protected]] > > > > > > > > Sent: 2019年5月8日 14:46 > > > > > > > > To: Peng Fan <[email protected]> > > > > > > > > Cc: [email protected]; [email protected]; dl-uboot-imx > > > > > > > > <[email protected]>; [email protected]; > > > > > > [email protected]; > > > > > > > > [email protected]; [email protected]; [email protected] > > > > > > > > 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 > > > > > > > > <[email protected]> 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 <[email protected]> > > > > > > > > > > > --- > > > > > > > > > > > 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: > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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: [email protected] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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: [email protected] > > > > > > > > > > > > > > > > > > > > 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: [email protected] > > > > > > > > > > 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: > > [email protected] 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: [email protected]
pgpQHJnfDtABD.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
