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

[Simon Glass]

Hi Neil,

On 30 March 2018 at 15:53, Neil Armstrong <narmstrong at baylibre.com> wrote:
> On 30/03/2018 00:41, Simon Glass wrote:
>> 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.
>>
>
> 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