| 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 |
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
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
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ç
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
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
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ç
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
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
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
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 --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>; + };
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