Re: [PATCH 1/8] dt-bindings: clock: Document Loongson 2K0300 clock controller

[Krzysztof Kozlowski]

On 23/05/2025 15:28, Yao Zi wrote:
> On Fri, May 23, 2025 at 08:30:57PM +0800, Binbin Zhou wrote:
>> On Fri, May 23, 2025 at 6:46 PM Yao Zi <ziyao@xxxxxxxxxxx> wrote:
>>>
>>> Document the clock controller shipped in Loongson 2K0300 SoC, which
>>> generates various clock signals for SoC peripherals.
>>>
>>> Signed-off-by: Yao Zi <ziyao@xxxxxxxxxxx>
>>> ---
>>>  .../bindings/clock/loongson,ls2k0300-clk.yaml | 52 ++++++++++++++++++
>>>  .../dt-bindings/clock/loongson,ls2k0300-clk.h | 54 +++++++++++++++++++
>>>  2 files changed, 106 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls2k0300-clk.yaml
>>
>> I don't think a new binding file for 2K0300 is needed. Adding
>> compatible entries to loongson,ls2k-clk.yaml would be more appropriate
>> as they are almost all similar.
> 
> Originally I've tried to integrate the 2K0300 stuff with
> loongson,ls2k-clk.yaml, but found it's hard to describe some properties.
> 
> For example, currently in loongson,ls2k-clk.yaml, the clocks property is
> described as
> 
>   clocks:
>     items:
>       - description: 100m ref
> 
> what should the description look like with 2K0300 introduced, whose
> reference clock runs at 120MHz instead of 100MHz? It'll be hard to
> describe things correctly without losing existing information. "120MHz
> reference clock for Loongson 2K0300, or 100MHz reference clock for other
> SoCs" sounds even a worse idea.

Drop the frequency anyway, it is kind of pointless here.

> 
> Another example is about the description of clock IDs. loongson,ls2k-clk.yaml
> describes available clock IDs as
> 
>   '#clock-cells':
>     const: 1
>     description:
>       The clock consumer should specify the desired clock by having the clock
>       ID in its "clocks" phandle cell. See include/dt-bindings/clock/loongson,ls2k-clk.h
>       for the full list of Loongson-2 SoC clock IDs.
> 
> what should the description look like if we add 2K0300 support? With a
> different header being introduced, the description will be messy.

No, just list the headers.

> 
> I think keeping SoCs peripherals that are different in hardware design
> in the same binding is really a bad idea. Yes, these clock controllers
> are similar enough to reuse the clock hardware driver, but they have
> different clock tree structures and register definitions, making them
> essentially different things. Trying to keep everything in the same
> place only makes the binding messy.

How is binding messy if you only add one compatible?


Best regards,
Krzysztof