On Wed, Jan 30, 2019 at 9:43 PM Andre Przywara <[email protected]> wrote: > > On Wed, 30 Jan 2019 20:17:56 +0530 > Jagan Teki <[email protected]> wrote: > > > On Wed, Jan 30, 2019 at 8:02 PM Andre Przywara > > <[email protected]> wrote: > > > > > > On Wed, 30 Jan 2019 19:49:23 +0530 > > > Jagan Teki <[email protected]> wrote: > > > > > > > On Wed, Jan 30, 2019 at 7:13 PM Andre Przywara > > > > <[email protected]> wrote: > > > > > > > > > > On Wed, 30 Jan 2019 18:16:44 +0530 > > > > > Jagan Teki <[email protected]> wrote: > > > > > > > > > > > On Wed, Jan 30, 2019 at 4:26 PM Andre Przywara > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > On Wed, 30 Jan 2019 16:08:14 +0530 > > > > > > > Jagan Teki <[email protected]> wrote: > > > > > > > > > > > > > > > On Wed, Jan 30, 2019 at 4:04 PM Andre Przywara > > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > > > On Tue, 29 Jan 2019 23:56:44 +0530 > > > > > > > > > Jagan Teki <[email protected]> wrote: > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > On Tue, Jan 29, 2019 at 11:47 PM Andre Przywara > > > > > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > > > > > > > > > On Tue, 29 Jan 2019 23:40:26 +0530 > > > > > > > > > > > Jagan Teki <[email protected]> wrote: > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jan 29, 2019 at 9:25 PM Andre Przywara > > > > > > > > > > > > <[email protected]> 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 > > > > > > > > > > > > > <[email protected]> --- > > > > > > > > > > > > > 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.
Applied to u-boot-sunxi/master _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
