[U-Boot] [PATCH v4 1/9] sunxi: clk: enable clk and reset for CCU devices

[Andre Przywara]

On Wed, 30 Jan 2019 16:08:14 +0530
Jagan Teki <jagan at amarulasolutions.com> wrote:

> On Wed, Jan 30, 2019 at 4:04 PM Andre Przywara
> <andre.przywara at arm.com> wrote:
> >
> > On Tue, 29 Jan 2019 23:56:44 +0530
> > Jagan Teki <jagan at amarulasolutions.com> wrote:
> >
> > Hi,
> >  
> > > On Tue, Jan 29, 2019 at 11:47 PM Andre Przywara
> > > <andre.przywara at foss.arm.com> wrote:  
> > > >
> > > > On Tue, 29 Jan 2019 23:40:26 +0530
> > > > Jagan Teki <jagan at amarulasolutions.com> wrote:
> > > >  
> > > > > On Tue, Jan 29, 2019 at 9:25 PM Andre Przywara
> > > > > <andre.przywara at arm.com> wrote:  
> > > > > >
> > > > > > Some Allwinner clock devices have parent clocks and reset
> > > > > > gates itself, which need to be activated for them to work.
> > > > > >
> > > > > > Add some code to just assert all resets and enable all
> > > > > > clocks given. This should enable the A80 MMC config clock,
> > > > > > which requires both to be activated. The full CCU devices
> > > > > > typically don't require resets, and have just fixed clocks
> > > > > > as their parents. Since we treat both as optional and
> > > > > > enabling fixed clocks is a NOP, this works for all cases,
> > > > > > without the need to differentiate between those clock types.
> > > > > >
> > > > > > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > > > > > ---
> > > > > >  drivers/clk/sunxi/clk_sunxi.c | 12 ++++++++++++
> > > > > >  1 file changed, 12 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/clk/sunxi/clk_sunxi.c
> > > > > > b/drivers/clk/sunxi/clk_sunxi.c index 62ce2994e4..6d4aeb5315
> > > > > > 100644 --- a/drivers/clk/sunxi/clk_sunxi.c
> > > > > > +++ b/drivers/clk/sunxi/clk_sunxi.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > >  #include <clk-uclass.h>
> > > > > >  #include <dm.h>
> > > > > >  #include <errno.h>
> > > > > > +#include <reset.h>
> > > > > >  #include <asm/io.h>
> > > > > >  #include <asm/arch/ccu.h>
> > > > > >  #include <linux/log2.h>
> > > > > > @@ -61,6 +62,9 @@ struct clk_ops sunxi_clk_ops = {
> > > > > >  int sunxi_clk_probe(struct udevice *dev)
> > > > > >  {
> > > > > >         struct ccu_priv *priv = dev_get_priv(dev);
> > > > > > +       struct clk_bulk clk_bulk;
> > > > > > +       struct reset_ctl_bulk rst_bulk;
> > > > > > +       int ret;
> > > > > >
> > > > > >         priv->base = dev_read_addr_ptr(dev);
> > > > > >         if (!priv->base)
> > > > > > @@ -70,5 +74,13 @@ int sunxi_clk_probe(struct udevice *dev)
> > > > > >         if (!priv->desc)
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > +       ret = clk_get_bulk(dev, &clk_bulk);
> > > > > > +       if (!ret)
> > > > > > +               clk_enable_bulk(&clk_bulk);
> > > > > > +
> > > > > > +       ret = reset_get_bulk(dev, &rst_bulk);
> > > > > > +       if (!ret)
> > > > > > +               reset_deassert_bulk(&rst_bulk);
> > > > > > +  
> > > > >
> > > > > Can't we do this locally to clk_a80 probe?  
> > > >
> > > > That's the point: there is no such thing. For all SoCs we use
> > > > the shared sunxi_clk_probe() function. Doing this only for the
> > > > A80 would mean to split this up, which is duplicating a lot of
> > > > code for very little effect. The code here just enables every
> > > > clock and reset given, which is generic and should always be
> > > > the right thing.  
> > >
> > > But enable and dessert of clock and reset is job respective IP
> > > driver isn't it?  
> >
> > Which IP driver are you thinking about? This is "the IP driver"
> > for those clock, isn't it?  
> 
> IP can be any peripheral like USB, MMC, UART and it those drivers job
> to get and enable the clock isn't it?

Yes, using the DM_CLK framework. This is what we do: the A80 MMC DT
node refers to the MMC config clock (instead of the generic CCU), and
the MMC driver doesn't care about any requirement this clock has
*itself*.
This is the responsibility of the *A80 MMC config clock driver*, which
we introduce in patch 3/9. So in *this* driver's probe function you
would need to enable the parent clocks, which is exactly what we do.
Just by re-using the existing sunxi_clk_probe() function.

> I assume this code would do the same thing what these peripheral
> driver do?

It does, it's just one layer in between.
-----------------------
A64 MMC driver   A64 CCU driver           fixed clock driver
                 sunxi_clk_probe()
                     clk_enable()    ->   (NULL)
clk_enable()  -> sunxi_clk_enable()
-----------------------
A80 MMC driver   A80 MMC cfg clk driver   A80 CCU driver     fixed clk
                                          sunxi_clk_probe()
                                             clk_enable() -> (NULL)
                 sunxi_clk_probe()
                     clk_enable()    ->   sunxi_clk_enable()
clk_enable()  -> sunxi_clk_enable()
-----------------------

Hope that makes it clearer.

Cheers,
Andre.

> 
> > > > > > +       ret = clk_get_bulk(dev, &clk_bulk);
> > > > > > +       if (!ret)
> > > > > > +               clk_enable_bulk(&clk_bulk);
> > > > > > +
> > > > > > +       ret = reset_get_bulk(dev, &rst_bulk);
> > > > > > +       if (!ret)
> > > > > > +               reset_deassert_bulk(&rst_bulk);