Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 11:09, Jerome Brunet wrote:
> 
> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> 
>> On 28/07/2022 10:50, Jerome Brunet wrote:
>>>
>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>
>>>> On 28/07/2022 07:42, Yu Tu wrote:
> [...]
>>>>> +/*
>>>>> + * CLKID index values
>>>>> + */
>>>>> +
>>>>> +#define CLKID_FIXED_PLL			1
>>>>> +#define CLKID_FCLK_DIV2			3
>>>>> +#define CLKID_FCLK_DIV3			5
>>>>> +#define CLKID_FCLK_DIV4			7
>>>>> +#define CLKID_FCLK_DIV5			9
>>>>> +#define CLKID_FCLK_DIV7			11
>>>>
>>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>>
>>>
>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>>> For example, with composite 'mux / div / gate' assembly, we usually need
>>> only the leaf.
>>
>> I understand you do not expose them all, but that is not the reason to
>> increment ID by 2 or 3... Otherwise these are not IDs and you are not
>> expected to put register offsets into the bindings (you do not bindings
>> in such case).
> 
> Why is it not an IDs if it not continuous in the bindings ?
> 
> If there is technical reason, we'll probably end up exposing everything. It
> would not be a dramatic change. I asked for this over v1 because we have
> done that is the past and I think it makes sense.
> 
> I'm happy to be convinced to do things differently. Just looking for the
> technical reason that require contiuous exposed IDs.
> 
> The other IDs exists, but we do not expose them as bindings.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125

https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@xxxxxxxxxxxxxx/

https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@xxxxxxxxxxxxxx/

https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@xxxxxxxxxx/

The IDs are abstract numbers, where the number does not matter because
it is not tied to driver implementation or device programming model. The
driver maps ID to respective clock.

Using some meaningful numbers as these IDs, means you tied bindings to
your implementation and any change in implementation requires change in
the bindings. This contradicts the idea of bindings.

> 
>>
>>
>>> Same has been done for the other AML controllers:
>>> For ex:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h
>>
>> This cannot be fixed now, but it is very poor argument. Like saying "we
>> had a bug in other driver, so we implemented the bug here as well".
> 
> I agree, "done before" is not a good argument. I was trying to provide a
> better picutre. I'm just surprised to have this new requirement that IDs
> have to be incremented by 1 (in the bindings) and I'd like to understand
> why what we had done could be considered a bug now.

It was always, just no one ever enforced it. And almost all clock and
reset providers follow it. There are just literally few exceptions.

> For example the simple-reset driver compute the reset offset from the IDs:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-simple.c

This is one of the exceptions where it actually made sense, but I would
argue it still contradicts the bindings. You have now binding which is
tied to both Linux implementation and to device programming model.

However fixing it would require creating huge mapping tables for each
SoC, so obviously this exception is quite reasonable.

Clock drivers require tables and translation anyway. Almost all clock
drivers did it, so such exception is not justified.

> There might be holes in the IDs if not all bits have reset maps.
> I don't think that would be a bug either.

Bug was of course highly exaggerated example. :)

Best regards,
Krzysztof