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?
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?
> + 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]
pgpN0hH2B1fNR.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
