Hi Neil,
On 30 March 2018 at 15:53, Neil Armstrong narmstrong@baylibre.com wrote:
On 30/03/2018 00:41, Simon Glass wrote:
Hi Neil,
On 29 March 2018 at 16:42, Neil Armstrong narmstrong@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.galvani@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.
The driver only manages the gates, enabling and disabling them and getting their freq. The missing clocks of the ethernet driver in this case are dividers of the PLL, thus we don't need to enable them, and for now the driver don't need the freq.
Yes but -ENOSYS means that the driver does not support the operation at all. Put it another way: you can't return -ENOSYS for some parameters and not for others.
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.
In this case, the driver does not support the operation for these clocks, but maybe it would be better to add them with reg=0 and return -ENOSYS in the following return :
I don't like that - ENOENT seems better.
gate = &gates[clk->id];if (gate->reg == 0)return -ENOENT;Same here -ENOSYS
Here thsi means it's not a gate.
Yes, but the driver still supports the operation. The problem is that its inputs are invalid. So use -ENOENT to mean that.
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
Regards, Simon