Message-ID: <d0f74721-bf5a-62de-53dc-62e7e735e2dc@linaro.org>
Date:   Mon, 20 Mar 2023 18:36:02 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc:     linux-clk@...r.kernel.org, linux-mips@...r.kernel.org,
        tsbogend@...ha.franken.de, john@...ozen.org,
        linux-kernel@...r.kernel.org, p.zabel@...gutronix.de,
        mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        devicetree@...r.kernel.org, arinc.unal@...nc9.com
Subject: Re: [PATCH 01/10] dt: bindings: clock: add mtmips SoCs clock device
 tree binding documentation

On 20/03/2023 18:24, Sergio Paracuellos wrote:
> Hi Krzysztof,
> 
> On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@...aro.org> wrote:
>>
>> On 20/03/2023 17:18, Sergio Paracuellos wrote:
>>> Adds device tree binding documentation for clocks and resets in the
>>> Mediatek MIPS and Ralink SOCs. This covers RT2880, RT3050, RT3052, RT3350,
>>> RT3883, RT5350, MT7620, MT7628 and MT7688 SoCs.
>>
>> Use subject prefixes matching the subsystem (which you can get for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching).
>>
>> Subject: drop second/last, redundant "device tree binding
>> documentation". The "dt-bindings" prefix is already stating that these
>> are bindings.
> 
> Sure, will do. Sorry for the inconvenience.
> 
>> (BTW, that's the longest redundant component I ever saw)
> 
> I thought it was better to just list compatible strings inside one
> single file than adding the same binding in multiple files.

I don't understand how this is answers about redundant piece of subject.
Amount of files are not related to repeating pieces of subject prefix.

> 
>>
>>>
>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@...il.com>
>>> ---
>>>  .../bindings/clock/mtmips-clock.yaml          | 68 +++++++++++++++++++
>>>  1 file changed, 68 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/mtmips-clock.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/mtmips-clock.yaml b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml
>>> new file mode 100644
>>> index 000000000000..c92969ce231d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml
>>
>> Filename matching compatible, so vendor prefix and device name (or
>> family of names).
> 
> I used mtmips here but list compatibles starting with ralink. As I
> have said in the cover letter I am inspired by the last merged pinctrl
> series for these SoCs.
> See:
> - https://lore.kernel.org/linux-gpio/e9e6ad87-2db5-9767-ff39-64a302b06185@arinc9.com/T/#t

21 patches, so what exactly I should see (except that I was involved in
that discussions)?

Plus nothing from this thread warrants here exception from naming style.


> 
> Not all of compatible currently exist. 

Then clearly state this.

> All of these are at the end the
> way we can properly match compatible-data to write a proper driver.
> The current ralink dtsi files which are in tree now
> are totally incomplete and not documented so we are planning to align

Nothing like this was said in commit msg, so how can we know?

> all of this with openWRT used files and others soon. That's the reason
> we are not touching
> 'arch/mips/boot/dts' at all now. I don't think anybody is using any of
> this but mt7621 which is properly completed and documented.

Anyway, none of this explains exception from naming convention - vendor,
device or family name.

> 
>>
>>> @@ -0,0 +1,68 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/mtmips-clock.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MTMIPS SoCs Clock
>>
>> One clock? Are you sure these describe exactly one clock?
> 
> I will change this to 'Clocks'.

Then clock provider, but are you sure? You included there syscon and
reset controller.

> 
>>
>>> +
>>> +maintainers:
>>> +  - Sergio Paracuellos <sergio.paracuellos@...il.com>
>>> +
>>> +description: |
>>> +  MediaTek MIPS and Ralink SoCs have an XTAL from where the cpu clock is
>>> +  provided as well as derived clocks for the bus and the peripherals.
>>> +
>>> +  Each clock is assigned an identifier and client nodes use this identifier
>>> +  to specify the clock which they consume.
>>
>> Drop useless or obvious pieces of description. Describe the hardware.
>>
>>> +
>>> +  The clocks are provided inside a system controller node.
> 
>>
>> ???
> 
> I meant, this node is a syscon from where both clock and reset related
> registers are used. I think writing in this way was enough since it
> has a pretty similar description like the one in
> 'mediatek,mt7621-sysc.yaml'.

But what is a system controller node? Some separate device? This is
description for this device - called "Clock" or "Clocks" - and "system
controller" appears for the first time.

> 
>>
>>> +
>>> +  This node is also a reset provider for all the peripherals.
>>
>> ??? Does it mean it is not only "Clock" but also reset controller?
> 
> Yes, this node is a clock and reset controller for all the SoC.
> 
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - ralink,rt2880-sysc
>>> +          - ralink,rt3050-sysc
>>> +          - ralink,rt3052-sysc
>>> +          - ralink,rt3352-sysc
>>> +          - ralink,rt3883-sysc
>>> +          - ralink,rt5350-sysc
>>> +          - ralink,mt7620-sysc
>>> +          - ralink,mt7620a-sysc
>>> +          - ralink,mt7628-sysc'
>>> +          - ralink,mt7688-sysc
>>> +          - ralink,rt2880-reset
>>
>> That's odd. rt2880 is sysc and reset? One device with two compatibles?
> 
> This 'ralink,rt2880-reset' is for compatibility reasons. 

I don't understand why. It is used in DTS, so what this node represents
there?

> Reset related
> code was inside 'arch/mips/ralink' folder reset.c file but it is moved
> to this new driver, so we have maintained this reset stuff for the
> reset compatibility. All of the rest are the new possible stuff for
> both reset and clocks. 

We talk here about hardware, not drivers, so moving driver code around
does not help me understand the rationale behind bindings.

> Clock driver is instantiated in two phases. The
> earlier one set up the clocks via CLK_OF_DECLARE macro. Resets are set
> up as a platform driver. Is only inside this where
> 'ralink,rt2880-reset' is used. See patch 2 of the series for details.

Sure, but it is not related to bindings.

> 
>>
>> Also, order these by name.
> 
> All are ordered but I maintained the  'ralink,rt2880-reset' at the end.

No, it is not. m is before r in alphabet.

Best regards,
Krzysztof

Powered by blists - more mailing lists