[U-Boot] [PATCH v1 06/15] dm: clk: imx: Add support for controlling imx6q clocks via Driver Model

[Jagan Teki]

On Tue, Jan 29, 2019 at 1:35 PM Lukasz Majewski <lukma at denx.de> wrote:
>
> Hi Jagan,
>
> > On Tue, Jan 29, 2019 at 12:46 PM Lukasz Majewski <lukma at denx.de>
> > wrote:
> > >
> > > Hi Stefano, Fabio,
> > >
> > > > Hi Lukasz,
> > > >
> > > > On 21/01/19 15:19, Lukasz Majewski wrote:
> > > > > Hi Fabio,
> > > > >
> > > > >> Hi Lukasz,
> > > > >>
> > > > >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma at denx.de>
> > > > >> wrote:
> > > > >>> +static ulong imx6q_clk_get_rate(struct clk *clk)
> > > > >>> +{
> > > > >>> +       ulong rate = 0;
> > > > >>> +
> > > > >>> +       debug("%s(#%lu)\n", __func__, clk->id);
> > > > >>> +
> > > > >>> +       switch (clk->id) {
> > > > >>> +       case IMX6QDL_CLK_ECSPI1:
> > > > >>> +       case IMX6QDL_CLK_ECSPI2:
> > > > >>> +       case IMX6QDL_CLK_ECSPI3:
> > > > >>> +       case IMX6QDL_CLK_ECSPI4:
> > > > >>> +               return imx6_get_cspi_clk();
> > > > >>> +
> > > > >>> +       case IMX6QDL_CLK_USDHC1:
> > > > >>> +       case IMX6QDL_CLK_USDHC2:
> > > > >>> +       case IMX6QDL_CLK_USDHC3:
> > > > >>> +       case IMX6QDL_CLK_USDHC4:
> > > > >>> +               return imx6_get_usdhc_clk(clk->id -
> > > > >>> IMX6QDL_CLK_USDHC1);
> > > > >>
> > > > >> I don't think this scales well as this needs to grow for all
> > > > >> other peripherals and for each port instance.
> > > > >
> > > >
> > > > I am hiiting the same doubts as Fabio when I reviewed Peng's
> > > > patches with clocks for i.MX8M. The current approach was
> > > > introduced some years ago with i.MX5, but it does not fit well
> > > > now and it does not scale.
> > > >
> > > >
> > > > > The rationale regarding this approach:
> > > > >
> > > > > 1. Reuse the clock.c code for iMX6Q as much as possible.
> > > > >
> > > > > 2, This code is based on the clk-imx8q.c file -  hence the
> > > > > question why the Linux clock API was not ported for this new
> > > > > SoC?.
> > > >
> > > > Many reasons, first there was already a set of functions to get /
> > > > set clocks coming from previos i.MX platforms, and this API was
> > > > simply reused. And nobody was in charge to port the clock API.
> > > >
> > > > >
> > > > >>
> > > > >> If we are adding a clock driver for mx6, why don't we add it
> > > > >> just like the kernel one?
> > > > >
> > > > > I can try to port the Linux code, but IMHO it would be feasible
> > > > > to port only relevant (ECSPI, USDHC) parts of it (not all as I
> > > > > cannot test it all properly).
> > > >
> > > > Fully agree. Further parts will be added on demand. Less is
> > > > better. I disagree to add not tested code.
> > >
> > > The work is in progress - I will add the same directory structure as
> > > Linux's Common Clock Framework [CCF]. I shall finish in 2-3 days.
> > >
> > > There are a few problems to tackle (when porting code from Linux):
> >
> > Just to add my experience with previous CLK support series[1]. Do we
> > really need to port it from Linux, because I'm thinking we can manage
> > it as simple as other SoC support in U-Boot.(I have few clocks
> > addition on this series other than ENET)
> >
> > What do you think?
>
> This patch series seems to me like the one which reuses the "large"
> switch/case approach with "fsl,imx6q-ccm" driver [*].
>
> The ENET shall also be handled by the CCF from Linux.
>
> Another problem - the U-boot's clock DM API is not supporting passing
> parent clock rate, which is important in the hierarchical clock tree.
> The Linux CCF is a hierarchical one built in the driver's [*] probe.

Understand, we need to taken care the parent clock rates as well for
set and get rates. In fact we tried similar thing with sunxi, patch[2]
implements get_rate by taking parent clock rates in to account by
using switch statements. and this change[3] do add same but with
recursive calls. Since U-Boot need minimal clocks and most of them run
with default parents it better to implement as smooth as possible
otherwise it would ended-up size and complex issues, IMHO.

I think other platforms like mediatek clocks are also following same.

>
>
>
> Do we need to port the (trimmed) Common Clock Framework [CCF] from
> Linux?
>
> With the first version of this patch I also wanted to re-use the code
> from clock.c imx6 file. This would be good enough for now.
>
> However, in the long term (and taking into consideration the fact that
> imx6 needs to be converted to DM anyway) it may be beneficial to re-use
> CCF.
>
> The _real_ problem is to fit it and reuse in SPL (to avoid footprint
> size regression). The CCF port will work correctly in u-boot proper,
> but for SPL we would need some hacks (as e.g. by default we now strip
> clock properties from DTS when appending to SPL).
>
> Now SPLs for IMX6 use solid code without any unneeded stuff (and
> hence we didn't need TPL...) .

Yes, all these size issues might occur even in future if we port large
things from Linux.

[2] https://patchwork.ozlabs.org/patch/1019673/
[3] https://gist.github.com/apritzel/db93dd06b4defb46504bccbfe4fc2c20#file-sunxi_clk-c-L86-L112