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

From: Jerome Brunet <jbrunet@baylibre.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Yu Tu <yu.tu@amlogic.com>,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Subject: Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings
Date: Thu, 28 Jul 2022 11:09:52 +0200	[thread overview]
Message-ID: <1jmtctfuli.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <367cf98b-ef06-8f44-76c8-9099a1ec13dc@linaro.org>

On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 28/07/2022 10:50, Jerome Brunet wrote:
>> 
>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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

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

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
There might be holes in the IDs if not all bits have reset maps.
I don't think that would be a bug either.

>
> Best regards,
> Krzysztof