| Message ID | 20230320161823.1424278-2-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mips: ralink: add complete clock and reset driver for mtmips SoCs | expand |
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. (BTW, that's the longest redundant component I ever saw) > > 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 > > 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). > @@ -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? > + > +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. Drop useless or obvious pieces of description. Describe the hardware. > + > + The clocks are provided inside a system controller node. ??? > + > + This node is also a reset provider for all the peripherals. ??? Does it mean it is not only "Clock" but also reset controller? > + > +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? Also, order these by name. > + - 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 Wait, only rt2880-reset is reset controller? This is confusing. > + > +required: > + - compatible > + - reg > + - '#clock-cells' > + - '#reset-cells' > + > +additionalProperties: false > + > +examples: > + - | > + sysc: sysc@0 { Drop label. Node names should be generic, clock-controller or reset-controller or system-controller sometimes. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "ralink,rt5350-sysc", "syscon"; > + reg = <0x0 0x100>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + }; Best regards, Krzysztof
On 20.03.2023 19:36, Krzysztof Kozlowski 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. > (BTW, that's the longest redundant component I ever saw) > >> >> 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 >> >> 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 influenced Sergio to use MTMIPS here as I want to designate it the family name for the MediaTek MIPS and Ralink SoCs. We can't change the compatible string as it's established from my pinctrl patch series we don't do that. Arınç
On 20/03/2023 17:43, Arınç ÜNAL wrote: > On 20.03.2023 19:36, Krzysztof Kozlowski 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. >> (BTW, that's the longest redundant component I ever saw) >> >>> >>> 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 >>> >>> 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 influenced Sergio to use MTMIPS here as I want to designate it the > family name for the MediaTek MIPS and Ralink SoCs. I don't know how to respond to this. Is it argument for not using naming style? > We can't change the > compatible string as it's established from my pinctrl patch series we > don't do that. The patch did not say it is documenting existing compatibles in the kernel DTS. And if we are at this, ralink,rt2880-reset does not look like single clock nor like clock controller. Best regards, Krzysztof
Hi Krzysztof, On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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. > > > > > 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 > > > > 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 Not all of compatible currently exist. 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 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. > > > @@ -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'. > > > + > > +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. > > 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'. > > > + > > + 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. 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. 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. > > Also, order these by name. All are ordered but I maintained the 'ralink,rt2880-reset' at the end. > > > > + - 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 > > Wait, only rt2880-reset is reset controller? This is confusing. No, that is the reset compatibility one. All the rest are both clock and reset controllers from now on. > > > + > > +required: > > + - compatible > > + - reg > > + - '#clock-cells' > > + - '#reset-cells' > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + sysc: sysc@0 { > > Drop label. Sure, thanks. > > Node names should be generic, clock-controller or reset-controller or > system-controller sometimes. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > > + compatible = "ralink,rt5350-sysc", "syscon"; > > + reg = <0x0 0x100>; > > + #clock-cells = <1>; > > + #reset-cells = <1>; > > + }; Ok, so I will set this as 'syscon@' if you are ok with it. > > Best regards, > Krzysztof > Thanks to you for the review. Best regards, Sergio Paracuellos
On 20/03/2023 18:24, Sergio Paracuellos wrote: > Hi Krzysztof, > > On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.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@gmail.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@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. >> >> 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
On 20.03.2023 20:36, Krzysztof Kozlowski wrote: > On 20/03/2023 18:24, Sergio Paracuellos wrote: >> Hi Krzysztof, >> >> On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.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@gmail.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. Would mediatek,mtmips-clock.yaml make sense? Arınç
On 20/03/2023 17:18, Sergio Paracuellos wrote: > +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 One more comment - this and maybe other compatibles - have wrong vendor prefix. This is mediatek, not ralink. Best regards, Krzysztof
On 20/03/2023 18:57, Arınç ÜNAL wrote: >>> 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. > > Would mediatek,mtmips-clock.yaml make sense? More, except: 1. This is not clock, but sysc. 2. mips sounds redundant. Do you have rt2xxx and mt7xxx chips which are ARM? Best regards, Krzysztof
On 20.03.2023 21:01, Krzysztof Kozlowski wrote: > On 20/03/2023 17:18, Sergio Paracuellos wrote: >> +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 > > One more comment - this and maybe other compatibles - have wrong vendor > prefix. This is mediatek, not ralink. This platform was acquired from Ralink by MediaTek. I couldn't change some existing ralink compatible strings to mediatek as Rob explained on my pinctrl patch series that we don't do that. The compatible strings on this patch series here are new but I'd rather keep the compatible strings ralink to keep things consistent. Arınç
On 20.03.2023 21:02, Krzysztof Kozlowski wrote: > On 20/03/2023 18:57, Arınç ÜNAL wrote: >>>> 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. >> >> Would mediatek,mtmips-clock.yaml make sense? > > More, except: > 1. This is not clock, but sysc. Sergio, beware. > 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. Arınç
On 20/03/2023 19:07, Arınç ÜNAL wrote: > On 20.03.2023 21:01, Krzysztof Kozlowski wrote: >> On 20/03/2023 17:18, Sergio Paracuellos wrote: >>> +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 >> >> One more comment - this and maybe other compatibles - have wrong vendor >> prefix. This is mediatek, not ralink. > > This platform was acquired from Ralink by MediaTek. I couldn't change > some existing ralink compatible strings to mediatek as Rob explained on > my pinctrl patch series that we don't do that. The compatible strings on > this patch series here are new but I'd rather keep the compatible > strings ralink to keep things consistent. The comment that you cannot change existing compatibles does not apply to these, because these are new. However indeed some SoCs have already compatibles with ralink, so it's fine for these. mt7620 and mt7628 are already used with mediatek, so these should be rather corrected to new prefix. Best regards, Krzysztof
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. :) > >> 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. Best regards, Krzysztof
On 20.03.2023 21:11, Krzysztof Kozlowski wrote: > On 20/03/2023 19:07, Arınç ÜNAL wrote: >> On 20.03.2023 21:01, Krzysztof Kozlowski wrote: >>> On 20/03/2023 17:18, Sergio Paracuellos wrote: >>>> +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 >>> >>> One more comment - this and maybe other compatibles - have wrong vendor >>> prefix. This is mediatek, not ralink. >> >> This platform was acquired from Ralink by MediaTek. I couldn't change >> some existing ralink compatible strings to mediatek as Rob explained on >> my pinctrl patch series that we don't do that. The compatible strings on >> this patch series here are new but I'd rather keep the compatible >> strings ralink to keep things consistent. > > The comment that you cannot change existing compatibles does not apply > to these, because these are new. However indeed some SoCs have already > compatibles with ralink, so it's fine for these. mt7620 and mt7628 are > already used with mediatek, so these should be rather corrected to new > prefix. If you're talking about the pinctrl schemas for MT7620 and MT7628, it's just the name of the yaml files that have mediatek. The compatible string is still ralink so it should be kept ralink here as well. Arınç
On Mon, Mar 20, 2023 at 6:36 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 20/03/2023 18:24, Sergio Paracuellos wrote: > > Hi Krzysztof, > > > > On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.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. True :). > > > > >> > >>> > >>> 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 > >>> > >>> 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. Sure. Will do in the commit message. > > > 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? Will add a comment about this also. > > > 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. You are right. This is a system controller node even though it provides clocks and resets for the rest of the world. > > > > >> > >>> + > >>> +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. > >> > >> 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? True, this only has to be present in driver code. Not related to bindings at all. Will drop it, then. > > > 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. Understood. > > > 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. In my mind all of these were ordered taking into account the year since they exist :). So you are right. I will order this alphabetically. > > Best regards, > Krzysztof > Thanks, Sergio Paracuellos
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 20/03/2023 19:23, Arınç ÜNAL wrote: > On 20.03.2023 21:11, Krzysztof Kozlowski wrote: >> On 20/03/2023 19:07, Arınç ÜNAL wrote: >>> On 20.03.2023 21:01, Krzysztof Kozlowski wrote: >>>> On 20/03/2023 17:18, Sergio Paracuellos wrote: >>>>> +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 >>>> >>>> One more comment - this and maybe other compatibles - have wrong vendor >>>> prefix. This is mediatek, not ralink. >>> >>> This platform was acquired from Ralink by MediaTek. I couldn't change >>> some existing ralink compatible strings to mediatek as Rob explained on >>> my pinctrl patch series that we don't do that. The compatible strings on >>> this patch series here are new but I'd rather keep the compatible >>> strings ralink to keep things consistent. >> >> The comment that you cannot change existing compatibles does not apply >> to these, because these are new. However indeed some SoCs have already >> compatibles with ralink, so it's fine for these. mt7620 and mt7628 are >> already used with mediatek, so these should be rather corrected to new >> prefix. > > If you're talking about the pinctrl schemas for MT7620 and MT7628, it's > just the name of the yaml files that have mediatek. The compatible > string is still ralink so it should be kept ralink here as well. No, I am talking about compatibles. 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 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). Best regards, Krzysztof
On Tue, Mar 21, 2023 at 8:20 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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. Ah, ok. That reason was from where all of my arguments were coming from. But if it does not matter, I don't have problems using the 'mediatek' prefix in the new stuff. > > > > > 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). Ok, I see your point now. Thanks for clarification. I will maintain 'ralink,mt7620a-sysc' since it already exists and change the new stuff to use mediatek as prefix. These are 'mediatek,mt7620-sysc', 'mediatek,mt7628-sysc' and 'mediatek,mt7688-sysc'. Doing so it will properly match the 'mediatek,mtmips-sysc.yaml' file name. > > > 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 21.03.2023 11:04, Krzysztof Kozlowski wrote: > 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... I couldn't change ralink -> mediatek because company acquisitions don't grant the change. I don't see any reason to prevent changing mediatek -> ralink without breaking the ABI on the existing schemas. Arınç
On 21/03/2023 09:24, Arınç ÜNAL wrote: >>> >>> 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... > > I couldn't change ralink -> mediatek because company acquisitions don't > grant the change. I don't see any reason to prevent changing mediatek -> > ralink without breaking the ABI on the existing schemas. You cannot change mediatek->ralink without breaking the ABI for the same reasons. Best regards, Krzysztof
On 21.03.2023 11:27, Krzysztof Kozlowski wrote: > On 21/03/2023 09:24, Arınç ÜNAL wrote: >>>> >>>> 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... >> >> I couldn't change ralink -> mediatek because company acquisitions don't >> grant the change. I don't see any reason to prevent changing mediatek -> >> ralink without breaking the ABI on the existing schemas. > > You cannot change mediatek->ralink without breaking the ABI for the same > reasons. Then this is where I ask for an exception. The current solution only complicates things more. https://github.com/paraka/linux/pull/1/files#diff-0ae6c456898d08536ce987c32f23f2eb6f4a0f7c38bff9a61bdf3d0daa3f6549R21 Arınç
On 21/03/2023 09:33, Arınç ÜNAL wrote: > On 21.03.2023 11:27, Krzysztof Kozlowski wrote: >> On 21/03/2023 09:24, Arınç ÜNAL wrote: >>>>> >>>>> 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... >>> >>> I couldn't change ralink -> mediatek because company acquisitions don't >>> grant the change. I don't see any reason to prevent changing mediatek -> >>> ralink without breaking the ABI on the existing schemas. >> >> You cannot change mediatek->ralink without breaking the ABI for the same >> reasons. > > Then this is where I ask for an exception. > > The current solution only complicates things more. > > https://github.com/paraka/linux/pull/1/files#diff-0ae6c456898d08536ce987c32f23f2eb6f4a0f7c38bff9a61bdf3d0daa3f6549R21 Sorry, I don't understand what's under this link and how some Github repo pull helps in this discussion. I don't see there any text, which could help. I also do not understand why this pull proves that you can change existing mediatek compatibles (we talk also about ARM, which is shipped to million of devices) to ralink without breaking the ABI. I do not see how choosing one variant for compatibles having two variants of prefixes, complicates things. Following this argument choosing "ralink" also complicates! Best regards, Krzysztof
On 21.03.2023 11:39, Krzysztof Kozlowski wrote: > On 21/03/2023 09:33, Arınç ÜNAL wrote: >> On 21.03.2023 11:27, Krzysztof Kozlowski wrote: >>> On 21/03/2023 09:24, Arınç ÜNAL wrote: >>>>>> >>>>>> 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... >>>> >>>> I couldn't change ralink -> mediatek because company acquisitions don't >>>> grant the change. I don't see any reason to prevent changing mediatek -> >>>> ralink without breaking the ABI on the existing schemas. >>> >>> You cannot change mediatek->ralink without breaking the ABI for the same >>> reasons. >> >> Then this is where I ask for an exception. >> >> The current solution only complicates things more. >> >> https://github.com/paraka/linux/pull/1/files#diff-0ae6c456898d08536ce987c32f23f2eb6f4a0f7c38bff9a61bdf3d0daa3f6549R21 > > Sorry, I don't understand what's under this link and how some Github > repo pull helps in this discussion. I don't see there any text, which > could help. That's Sergio's current branch, before he sends out a new version of the patch series. So that's the current solution, having mediatek,mt7620-sysc and ralink,mt7620a-sysc on the schema. > > I also do not understand why this pull proves that you can change > existing mediatek compatibles (we talk also about ARM, which is shipped > to million of devices) to ralink without breaking the ABI. No no, I only want to do this on schemas that concern the MTMIPS platform. It doesn't concern the MediaTek ARM schemas. > > 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. Arınç
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. Best regards, Krzysztof
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. Arınç
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