[U-Boot] [PATCH v4 1/9] sunxi: clk: enable clk and reset for CCU devices
Jagan Teki
jagan at amarulasolutions.com
Wed Jan 30 14:47:56 UTC 2019
On Wed, Jan 30, 2019 at 8:02 PM Andre Przywara <andre.przywara at arm.com> wrote:
>
> 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.
Yes, I know but we have to wait some time till CLK become more mature. isn't it?
>
> So what is your actual problem with this patch here (v4 1/9)?
> Are you afraid that it "hurts" the other CCUs?
Yes.
More information about the U-Boot
mailing list