[U-Boot] [PATCH v4 1/9] sunxi: clk: enable clk and reset for CCU devices
Andre Przywara
andre.przywara at arm.com
Wed Jan 30 14:31:56 UTC 2019
On Wed, 30 Jan 2019 19:49:23 +0530
Jagan Teki <jagan at amarulasolutions.com> wrote:
> On Wed, Jan 30, 2019 at 7:13 PM Andre Przywara
> <andre.przywara at arm.com> wrote:
> >
> > On Wed, 30 Jan 2019 18:16:44 +0530
> > Jagan Teki <jagan at amarulasolutions.com> wrote:
> >
> > > On Wed, Jan 30, 2019 at 4:26 PM Andre Przywara
> > > <andre.przywara at arm.com> wrote:
> > > >
> > > > 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)
> > >
> > > Ahh.. It didn't effect other clk drivers except A80?
> >
> > It affects every CCU device, but most (all?) of them just don't
> > specify any resets in the DT and the only clocks most of them have
> > there are fixed clocks.
> > And even if they would, enabling clocks and de-asserting resets
> > sounds like a reasonable thing to do anyway.
> > So the reset_get_bulk() call will fail (which we don't care about),
> > and the clk_get_bulk() call will return the two fixed clocks, for
> > which enable calls are a NOP.
> > So it works.
>
> This certainly not so understand to be part of common probe, instead I
> would prefer to be part of a80 [1] as of now till CLK framework become
> more mature. Let me know, we can push all these to send PR to Tom.
So you now have this *generic* code exposed to the A80 CCU as well
(which is very similar to all the other CCUs), not only to the A80 MMC
config clock. Sounds like combining the worst parts together: Having an
extra probe routine (which is still generic(!) and duplicates code), but
also exposing it to the CCU which doesn't need it.
So what is your actual problem with this patch here (v4 1/9)?
Are you afraid that it "hurts" the other CCUs?
Cheers,
Andre.
> [1] https://paste.ubuntu.com/p/KRgx8kFQjD/
> > Cheers,
> > Andre.
> >
> >
> > >
> > > > 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()
> > > > -----------------------
> > >
> > > So these bulk enable /deasserts in sunxi_clk_probe will call
> > > respective ops for A80 only and for other the behavior is as
> > > before. it it?
> >
>
>
More information about the U-Boot
mailing list