diff mbox series

[01/10] dt: bindings: clock: add mtmips SoCs clock device tree binding documentation

Message ID 20230320161823.1424278-2-sergio.paracuellos@gmail.com
State New
Headers show
Series [01/10] dt: bindings: clock: add mtmips SoCs clock device tree binding documentation | expand

Commit Message

Sergio Paracuellos March 20, 2023, 4:18 p.m. UTC
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.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../bindings/clock/mtmips-clock.yaml          | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mtmips-clock.yaml

Comments

Sergio Paracuellos March 21, 2023, 4:34 a.m. UTC | #1
On Mon, Mar 20, 2023 at 7:15 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 20/03/2023 19:09, Arınç ÜNAL wrote:
> >>> Would mediatek,mtmips-clock.yaml make sense?
> >>
> >> More, except:
> >> 1. This is not clock, but sysc.
> >
> > Sergio, beware.
>
> I meant, that's what I understood from what Sergio said. :)

Yes, you understood properly. I will use 'sysc' instead.

>
> >
> >> 2. mips sounds redundant. Do you have rt2xxx and mt7xxx chips which are ARM?
> >
> > All of the SoCs, RTXXXX, MT7620, MT7621, MT7628, MT7688 are MIPS. So I
> > decided to call this platform MTMIPS as I've seen MediaTek use this on
> > other projects like U-Boot. This is what I did on my pinctrl patch
> > series as well.
>
> Ah, but indeed there are newer Mediatek MT6xxx and MT8xxx SoCs which are
> ARM, so mediatek,mtmips-sysc would work.

I can use 'mediatek,mtmips-sysc.yaml' as the name but compatibles will
start with ralink. There are already some existent compatibles for
mt762x already having ralink as prefix, so to be coherent ralink
should be maintained as prefix.

>
> Best regards,
> Krzysztof
>

Thanks,
   Sergio Paracuellos
Krzysztof Kozlowski March 21, 2023, 6:32 a.m. UTC | #2
On 21/03/2023 05:34, Sergio Paracuellos wrote:
> On Mon, Mar 20, 2023 at 7:15 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 20/03/2023 19:09, Arınç ÜNAL wrote:
>>>>> Would mediatek,mtmips-clock.yaml make sense?
>>>>
>>>> More, except:
>>>> 1. This is not clock, but sysc.
>>>
>>> Sergio, beware.
>>
>> I meant, that's what I understood from what Sergio said. :)
> 
> Yes, you understood properly. I will use 'sysc' instead.
> 
>>
>>>
>>>> 2. mips sounds redundant. Do you have rt2xxx and mt7xxx chips which are ARM?
>>>
>>> All of the SoCs, RTXXXX, MT7620, MT7621, MT7628, MT7688 are MIPS. So I
>>> decided to call this platform MTMIPS as I've seen MediaTek use this on
>>> other projects like U-Boot. This is what I did on my pinctrl patch
>>> series as well.
>>
>> Ah, but indeed there are newer Mediatek MT6xxx and MT8xxx SoCs which are
>> ARM, so mediatek,mtmips-sysc would work.
> 
> I can use 'mediatek,mtmips-sysc.yaml' as the name but compatibles will
> start with ralink. There are already some existent compatibles for
> mt762x already having ralink as prefix, so to be coherent ralink
> should be maintained as prefix.

The compatibles I mentioned start already with mediatek, so why do you
want to introduce incorrect vendor name for these?

Best regards,
Krzysztof
Arınç ÜNAL March 21, 2023, 6:38 a.m. UTC | #3
On 21.03.2023 09:32, Krzysztof Kozlowski wrote:
> On 21/03/2023 05:34, Sergio Paracuellos wrote:
>> On Mon, Mar 20, 2023 at 7:15 PM Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 20/03/2023 19:09, Arınç ÜNAL wrote:
>>>>>> Would mediatek,mtmips-clock.yaml make sense?
>>>>>
>>>>> More, except:
>>>>> 1. This is not clock, but sysc.
>>>>
>>>> Sergio, beware.
>>>
>>> I meant, that's what I understood from what Sergio said. :)
>>
>> Yes, you understood properly. I will use 'sysc' instead.
>>
>>>
>>>>
>>>>> 2. mips sounds redundant. Do you have rt2xxx and mt7xxx chips which are ARM?
>>>>
>>>> All of the SoCs, RTXXXX, MT7620, MT7621, MT7628, MT7688 are MIPS. So I
>>>> decided to call this platform MTMIPS as I've seen MediaTek use this on
>>>> other projects like U-Boot. This is what I did on my pinctrl patch
>>>> series as well.
>>>
>>> Ah, but indeed there are newer Mediatek MT6xxx and MT8xxx SoCs which are
>>> ARM, so mediatek,mtmips-sysc would work.
>>
>> I can use 'mediatek,mtmips-sysc.yaml' as the name but compatibles will
>> start with ralink. There are already some existent compatibles for
>> mt762x already having ralink as prefix, so to be coherent ralink
>> should be maintained as prefix.
> 
> The compatibles I mentioned start already with mediatek, so why do you
> want to introduce incorrect vendor name for these?

Can you point out where these compatible strings for mt7620 and mt7628 are?

Arınç
Krzysztof Kozlowski March 21, 2023, 6:43 a.m. UTC | #4
On 21/03/2023 07:38, Arınç ÜNAL wrote:
>>>>
>>>> Ah, but indeed there are newer Mediatek MT6xxx and MT8xxx SoCs which are
>>>> ARM, so mediatek,mtmips-sysc would work.
>>>
>>> I can use 'mediatek,mtmips-sysc.yaml' as the name but compatibles will
>>> start with ralink. There are already some existent compatibles for
>>> mt762x already having ralink as prefix, so to be coherent ralink
>>> should be maintained as prefix.
>>
>> The compatibles I mentioned start already with mediatek, so why do you
>> want to introduce incorrect vendor name for these?
> 
> Can you point out where these compatible strings for mt7620 and mt7628 are?

git grep

Best regards,
Krzysztof
Sergio Paracuellos March 21, 2023, 6:56 a.m. UTC | #5
On Tue, Mar 21, 2023 at 7:43 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 21/03/2023 07:38, Arınç ÜNAL wrote:
> >>>>
> >>>> Ah, but indeed there are newer Mediatek MT6xxx and MT8xxx SoCs which are
> >>>> ARM, so mediatek,mtmips-sysc would work.
> >>>
> >>> I can use 'mediatek,mtmips-sysc.yaml' as the name but compatibles will
> >>> start with ralink. There are already some existent compatibles for
> >>> mt762x already having ralink as prefix, so to be coherent ralink
> >>> should be maintained as prefix.
> >>
> >> The compatibles I mentioned start already with mediatek, so why do you
> >> want to introduce incorrect vendor name for these?
> >
> > Can you point out where these compatible strings for mt7620 and mt7628 are?
>
> git grep

Not for *-sysc nodes. The only current one in use (from git grep):

arch/mips/ralink/mt7620.c:      rt_sysc_membase =
plat_of_remap_node("ralink,mt7620a-sysc");

That's the reason I also used prefix ralink for the rest.

Does it make sense to you to maintain this one as ralink,mt7620a-sysc
and add the following with mediatek prefix?

mediatek,mt7620-sysc
mediatek,mt7628-sysc
mediatek,mt7688-sysc

That would be weird IMHO.

> Best regards,
> Krzysztof

Thanks,
    Sergio Paracuellos
Arınç ÜNAL March 21, 2023, 7:39 a.m. UTC | #6
On 21.03.2023 10:19, Krzysztof Kozlowski wrote:
> On 21/03/2023 07:56, Sergio Paracuellos wrote:
>> On Tue, Mar 21, 2023 at 7:43 AM Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 21/03/2023 07:38, Arınç ÜNAL wrote:
>>>>>>>
>>>>>>> Ah, but indeed there are newer Mediatek MT6xxx and MT8xxx SoCs which are
>>>>>>> ARM, so mediatek,mtmips-sysc would work.
>>>>>>
>>>>>> I can use 'mediatek,mtmips-sysc.yaml' as the name but compatibles will
>>>>>> start with ralink. There are already some existent compatibles for
>>>>>> mt762x already having ralink as prefix, so to be coherent ralink
>>>>>> should be maintained as prefix.
>>>>>
>>>>> The compatibles I mentioned start already with mediatek, so why do you
>>>>> want to introduce incorrect vendor name for these?
>>>>
>>>> Can you point out where these compatible strings for mt7620 and mt7628 are?
>>>
>>> git grep
>>
>> Not for *-sysc nodes. The only current one in use (from git grep):
> 
> We do not talk about sysc nodes at all. They do not matter.
> 
>>
>> arch/mips/ralink/mt7620.c:      rt_sysc_membase =
>> plat_of_remap_node("ralink,mt7620a-sysc");
>>
>> That's the reason I also used prefix ralink for the rest.
>>
>> Does it make sense to you to maintain this one as ralink,mt7620a-sysc
>> and add the following with mediatek prefix?
>>
>> mediatek,mt7620-sysc
>> mediatek,mt7628-sysc
>> mediatek,mt7688-sysc
>>
>> That would be weird IMHO.
> 
> What exactly would be weird? Did you read the discussion about vendor
> prefix from Arinc? mt7620 is not a Ralink product, so what would be
> weird is to use "ralink" vendor prefix. This was never a Ralink. However
> since there are compatibles using "ralink" for non-ralink devices, we
> agreed not to change them.
> 
> These though use at least in one place mediatek, so the above argument
> does not apply. (and before you say "but they also use ralink and
> mediatek", it does not matter - it is already inconsistent thus we can
> choose whatever we want and ralink is not correct).

My argument was that your point being Ralink is now Mediatek, thus there 
is no conflict and no issues with different vendor used. It's the next 
best thing to be able to address the inconsistency, call everything of 
the MTMIPS platform ralink on the compatible strings.

If we take the calling new things mediatek route, we will never get to 
the bottom of fixing the naming inconsistency.

Arınç
Krzysztof Kozlowski March 21, 2023, 8:04 a.m. UTC | #7
On 21/03/2023 08:39, Arınç ÜNAL wrote:
>>>
>>> arch/mips/ralink/mt7620.c:      rt_sysc_membase =
>>> plat_of_remap_node("ralink,mt7620a-sysc");
>>>
>>> That's the reason I also used prefix ralink for the rest.
>>>
>>> Does it make sense to you to maintain this one as ralink,mt7620a-sysc
>>> and add the following with mediatek prefix?
>>>
>>> mediatek,mt7620-sysc
>>> mediatek,mt7628-sysc
>>> mediatek,mt7688-sysc
>>>
>>> That would be weird IMHO.
>>
>> What exactly would be weird? Did you read the discussion about vendor
>> prefix from Arinc? mt7620 is not a Ralink product, so what would be
>> weird is to use "ralink" vendor prefix. This was never a Ralink. However
>> since there are compatibles using "ralink" for non-ralink devices, we
>> agreed not to change them.
>>
>> These though use at least in one place mediatek, so the above argument
>> does not apply. (and before you say "but they also use ralink and
>> mediatek", it does not matter - it is already inconsistent thus we can
>> choose whatever we want and ralink is not correct).
> 
> My argument was that your point being Ralink is now Mediatek, thus there 
> is no conflict and no issues with different vendor used. It's the next 
> best thing to be able to address the inconsistency, call everything of 
> the MTMIPS platform ralink on the compatible strings.

And how does it help consistency? The mt7620 is used also with mediatek
prefix and adding more variants of realtek does not make the
inconsistency smaller. It's still inconsistent.

> 
> If we take the calling new things mediatek route, we will never get to 
> the bottom of fixing the naming inconsistency.

All new things, so new SoCs, should be called mediatek, because there is
no ralink and mediatek is already used for them. So why some new
Mediatek SoCs are "mediatek" but some other also new SoCs are "ralink"?

You can do nothing (and no actual need) about existing inconsistency...


Best regards,
Krzysztof
Rob Herring (Arm) March 24, 2023, 10:10 p.m. UTC | #8
On Tue, Mar 21, 2023 at 12:02:47PM +0300, Arınç ÜNAL wrote:
> On 21.03.2023 12:01, Krzysztof Kozlowski wrote:
> > On 21/03/2023 09:53, Arınç ÜNAL wrote:
> > > > 
> > > > I do not see how choosing one variant for compatibles having two
> > > > variants of prefixes, complicates things. Following this argument
> > > > choosing "ralink" also complicates!
> > > 
> > > The idea is to make every compatible string of MTMIPS to have the ralink
> > > prefix so it's not mediatek on some schemas and ralink on others. Simpler.
> > 
> > Which is an ABI break, so you cannot do it.
> 
> No, both strings stay on the driver, it's the schemas that will only keep
> ralink.

But you are adding one of the strings to the driver, right? Still an ABI 
break, but only if you have an old kernel and new DT. That can be 
somewhat mitigated with a stable backport of the new id, but still an 
ABI break.

Rob
Rob Herring (Arm) March 24, 2023, 10:13 p.m. UTC | #9
On Tue, Mar 21, 2023 at 12:02:47PM +0300, Arınç ÜNAL wrote:
> On 21.03.2023 12:01, Krzysztof Kozlowski wrote:
> > On 21/03/2023 09:53, Arınç ÜNAL wrote:
> > > > 
> > > > I do not see how choosing one variant for compatibles having two
> > > > variants of prefixes, complicates things. Following this argument
> > > > choosing "ralink" also complicates!
> > > 
> > > The idea is to make every compatible string of MTMIPS to have the ralink
> > > prefix so it's not mediatek on some schemas and ralink on others. Simpler.
> > 
> > Which is an ABI break, so you cannot do it.
> 
> No, both strings stay on the driver, it's the schemas that will only keep
> ralink.

Whatever is in the driver should be in the schema too. 'make 
dt_compatible_check' will check this. And some day, I'd like that list 
to get to 0.

Rob
Arınç ÜNAL March 24, 2023, 11:15 p.m. UTC | #10
On 25.03.2023 01:10, Rob Herring wrote:
> On Tue, Mar 21, 2023 at 12:02:47PM +0300, Arınç ÜNAL wrote:
>> On 21.03.2023 12:01, Krzysztof Kozlowski wrote:
>>> On 21/03/2023 09:53, Arınç ÜNAL wrote:
>>>>>
>>>>> I do not see how choosing one variant for compatibles having two
>>>>> variants of prefixes, complicates things. Following this argument
>>>>> choosing "ralink" also complicates!
>>>>
>>>> The idea is to make every compatible string of MTMIPS to have the ralink
>>>> prefix so it's not mediatek on some schemas and ralink on others. Simpler.
>>>
>>> Which is an ABI break, so you cannot do it.
>>
>> No, both strings stay on the driver, it's the schemas that will only keep
>> ralink.
> 
> But you are adding one of the strings to the driver, right? Still an ABI
> break, but only if you have an old kernel and new DT. That can be
> somewhat mitigated with a stable backport of the new id, but still an
> ABI break.

Ah, that makes sense. Yes, I'd be adding new strings to the driver.

> Whatever is in the driver should be in the schema too. 'make 
> dt_compatible_check' will check this. And some day, I'd like that list 
> to get to 0.

I'll keep this in mind for the schemas I maintain. I will add 
ralink,rt2880-pinmux as deprecated on the pinctrl schemas so it would 
disappear from 'make dt_compatible check'. I believe I'm supposed to do 
it like this?

properties:
   compatible:
     enum:
       - ralink,rt2880-pinctrl
       - ralink,rt2880-pinmux
     deprecated:
       items:
         - const: ralink,rt2880-pinmux
           deprecated: true

Arınç
diff mbox series

Patch

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
@@ -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
+
+maintainers:
+  - Sergio Paracuellos <sergio.paracuellos@gmail.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.
+
+  The clocks are provided inside a system controller node.
+
+  This node is also a reset provider for all the peripherals.
+
+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
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    description:
+      The first cell indicates the clock number.
+    const: 1
+
+  '#reset-cells':
+    description:
+      The first cell indicates the reset bit within the register.
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+  - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    sysc: sysc@0 {
+      compatible = "ralink,rt5350-sysc", "syscon";
+      reg = <0x0 0x100>;
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+    };