linux-kernel - Re: [PATCH v2 1/9] dt-bindings: clock: add mtmips SoCs system controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMhs-H_r0F=ewQqakzyo=XaUwCOXZACdWTuXZSXpueDy6xp=7g@mail.gmail.com>
Date:   Wed, 22 Mar 2023 09:57:13 +0100
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Arınç ÜNAL <arinc.unal@...nc9.com>
Cc:     Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        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,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/9] dt-bindings: clock: add mtmips SoCs system controller

On Wed, Mar 22, 2023 at 9:36 AM Arınç ÜNAL <arinc.unal@...nc9.com> wrote:
>
> On 22.03.2023 01:18, Rob Herring wrote:
> > On Tue, Mar 21, 2023 at 10:09:59AM +0300, Arınç ÜNAL wrote:
> >> On 21.03.2023 10:00, Sergio Paracuellos wrote:
> >>> On Tue, Mar 21, 2023 at 7:45 AM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@...aro.org> wrote:
> >>>>
> >>>> On 21/03/2023 06:00, Sergio Paracuellos wrote:
> >>>>> Adds device tree binding documentation for system controller node present
> >>>>> in Mediatek MIPS and Ralink SOCs. This node is a clock and reset provider
> >>>>> for the rest of the world. This covers RT2880, RT3050, RT3052, RT3350,
> >>>>> RT3883, RT5350, MT7620, MT7628 and MT7688 SoCs.
> >>>>>
> >>>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@...il.com>
> >>>>> ---
> >>>>>    .../bindings/clock/mediatek,mtmips-sysc.yaml  | 65 +++++++++++++++++++
> >>>>>    1 file changed, 65 insertions(+)
> >>>>>    create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml b/Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..f07e1652723b
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml
> >>>>> @@ -0,0 +1,65 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/clock/mediatek,mtmips-sysc.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: MTMIPS SoCs System Controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Sergio Paracuellos <sergio.paracuellos@...il.com>
> >>>>> +
> >>>>> +description: |
> >>>>> +  MediaTek MIPS and Ralink SoCs provides a system controller to allow
> >>>>> +  to access to system control registers. These registers include clock
> >>>>> +  and reset related ones so this node is both clock and reset provider
> >>>>> +  for the rest of the world.
> >>>>> +
> >>>>> +  These SoCs have an XTAL from where the cpu clock is
> >>>>> +  provided as well as derived clocks for the bus and the peripherals.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    items:
> >>>>> +      - enum:
> >>>>> +          - ralink,mt7620-sysc
> >>>>
> >>>> Since you decided to send it before we finish discussion:
> >>>> NAK - this is already used as mediatek
> >>>
> >>> Sorry, there was too much stuff commented so I preferred to clean up
> >>> all of them while maintaining the compatibles with the ralink prefix
> >>> instead since that was where the current discussion was at that point.
> >>>
> >>>>
> >>>>> +          - ralink,mt7620a-sysc
> >>>
> >>> As I have said, this one exists:
> >>>
> >>> arch/mips/ralink/mt7620.c:      rt_sysc_membase =
> >>> plat_of_remap_node("ralink,mt7620a-sysc");
> >>>
> >>>
> >>>>> +          - ralink,mt7628-sysc
> >>>>
> >>>> Same here.
> >>>>
> >>>>> +          - ralink,mt7688-sysc
> >>>>
> >>>> I expect you to check the others.
> >>>
> >>> I can change others to mediatek but that would be a bit weird, don't you think?
> >>
> >> I've seen some parts of the MTMIPS platform use mediatek compatible strings
> >> thanks to Krzysztof pointing them out. I don't like having some parts of the
> >> MTMIPS platform (pci, mmc, usbphy, etc.) with mediatek compatible string
> >> while others are ralink.
> >
> > That's unfortunate, but again, compatibles are just unique identifiers.
> > They are only wrong if they aren't unique...
>
> Understood. Sergio, please keep the new strings here ralink.

So if I have to maintain this as "ralink" I think this patch is ok as
it is now? If that is the case, I prefer to get Reviewed-by for this
patch as it is now by Krzysztof or Rob before changing anything in my
current code.

>
> >
> >> Like Krzysztof said [0], Ralink is now Mediatek, thus there is no conflict
> >> and no issues with different vendor used. So I'd rather keep new things
> >> Ralink and gradually change these mediatek strings to ralink.
> >
> > So break the ABI multiple times slowly. Again, either you live with
> > *all* the existing compatible strings or you declare it is fine to break
> > the ABI on these platforms and switch everything at once. Carrying both
> > strings (in bindings or drivers) and breaking the ABI is lose-lose.
>
> If removing the mediatek strings from the drivers and bindings is better
> than keeping both strings on the drivers except the bindings, which
> would keep the ABI intact, I'll do the prior and do it all at once.
>
> Arınç

Thanks,
    Sergio Paracuellos