Hi Neil, On 29 March 2018 at 16:42, Neil Armstrong <narmstr...@baylibre.com> wrote: > Hi Beniamino, > > On 03/12/2017 10:17, Beniamino Galvani wrote: >> Introduce a basic clock driver for Amlogic Meson SoCs which supports >> enabling/disabling clock gates and getting their frequency. >> >> Signed-off-by: Beniamino Galvani <b.galv...@gmail.com> >> --- >> arch/arm/mach-meson/Kconfig | 2 + >> drivers/clk/Makefile | 1 + >> drivers/clk/clk_meson.c | 196 >> ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 199 insertions(+) >> create mode 100644 drivers/clk/clk_meson.c >> >> diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig >> index d4bd230be3..7acee3bc5c 100644 >> --- a/arch/arm/mach-meson/Kconfig >> +++ b/arch/arm/mach-meson/Kconfig > > [...] >> + >> +static int meson_set_gate(struct clk *clk, bool on) >> +{ >> + struct meson_clk *priv = dev_get_priv(clk->dev); >> + struct meson_gate *gate; >> + >> + if (clk->id >= ARRAY_SIZE(gates)) >> + return -ENOENT; > > This should be -ENOSYS, otherwise it breaks the ethernet driver since it > waits -ENOSYS if clock cannot be enabled.
Perhaps, but this is a genuine error, so it is OK to break the Ethernet driver, isn't it? We don't want errors to be silently ignored. Not having a device is one thing, but having a device that does not work is bad. Also I have tried to keep -ENOSYS for cases where the driver does not support the operation. We should be very clear in clk-uclass.h as to what errors are returned. At present I don't see ENOSYS mentioned. At the very least we should update the docs if certain behaviour is expected. I would also expect us to have a sandbox test for it. > >> + >> + gate = &gates[clk->id]; >> + >> + if (gate->reg == 0) >> + return -ENOENT; > > Same here -ENOSYS > >> + >> + clrsetbits_le32(priv->addr + gate->reg, >> + BIT(gate->bit), on ? BIT(gate->bit) : 0); >> + return 0; >> +} >> + >> +static int meson_clk_enable(struct clk *clk) >> +{ >> + return meson_set_gate(clk, true); >> +} >> + >> +static int meson_clk_disable(struct clk *clk) >> +{ >> + return meson_set_gate(clk, false); >> +} >> + >> +static ulong meson_clk_get_rate(struct clk *clk) >> +{ >> + struct meson_clk *priv = dev_get_priv(clk->dev); >> + >> + if (clk->id != CLKID_CLK81) { >> + if (clk->id >= ARRAY_SIZE(gates)) >> + return -ENOENT; > > Same here -ENOSYS > >> + if (gates[clk->id].reg == 0) >> + return -ENOENT; > > Same here -ENOSYS > >> + } >> + >> + /* Use cached value if available */ >> + if (priv->rate) >> + return priv->rate; >> + >> + priv->rate = meson_measure_clk_rate(CLK_81); >> + >> + return priv->rate; >> +} >> + > > [...] > > Neil Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot