[PATCH 3/4] clk: add Amlogic meson clock driver

[Simon Glass]

Hi Neil,

On 29 March 2018 at 16:42, Neil Armstrong <narmstrong at 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 at 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