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

[Jerome Brunet]

On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:

> 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.
>

I totally agree. Bindings ID are abstract numbers.
We do follow that. We even document it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118

It is just a choice to not expose some IDs.
It is not tied to the implementation at all.
I think we actually follow the rules and the idea behind it.

We can expose then all If you still think what we are doing is not appropriate.

I'd like things to be consistent though. So if the decision is to
expose everything, I'll probably end up doing the same for the old SoCs.