[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 16:13:35 UTC 2019
On Wed, 30 Jan 2019 20:17:56 +0530
Jagan Teki <jagan at amarulasolutions.com> wrote:
> 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?
I don't understand how this would matter. Actually it's a good argument
for merging a simple solution first, then refining it later, if
needed.
> > So what is your actual problem with this patch here (v4 1/9)?
> > Are you afraid that it "hurts" the other CCUs?
>
> Yes.
I don't see how, and even if, we will know very soon and can still fix
it. If we have the code in for every SoC, we will spot issues easier.
So let's go with a simple and generic solution first.
Cheers,
Andre.
More information about the U-Boot
mailing list