| Message ID | 20240909014707.2003091-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mips: realtek: Add reboot support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Mon, Sep 09, 2024 at 01:47:06PM +1200, Chris Packham wrote: > Add device tree schema for the Realtek switch. Currently the only > supported feature is the syscon-reboot which is needed to be able to > reboot the board. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > .../bindings/mfd/realtek,switch.yaml | 50 +++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml Use compatible as filename. > > diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml > new file mode 100644 > index 000000000000..84b57f87bd3a > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml > @@ -0,0 +1,50 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/realtek,switch.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Realtek Switch with Internal CPU What sort of Switch? Like network switch? Then this should be placed in respective net (or deeper, e.g. net/dsa/) directory. Maintainers go here. See example-schema. > + > +description: > + The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port > + networking switch that supports many interfaces. Additionally, the device can > + support MDIO, SPI and I2C busses. I don't get why syscon node is called switch. This looks incomplete or you used description from some other device. But if this is DSA, then you miss dsa ref and dsa-related properties. > + > +maintainers: > + - Chris Packham <chris.packham@alliedtelesis.co.nz> > + > +properties: > + compatible: > + items: > + - enum: > + - realtek,rtl9302c-switch > + - const: syscon > + - const: simple-mfd > + > + reg: > + maxItems: 1 > + > + reboot: > + $ref: /schemas/power/reset/syscon-reboot.yaml# > + > +required: > + - compatible > + - reg > + - reboot > + > +additionalProperties: false > + > +examples: > + - | > + switch0: ethernet-switch@1b000000 { Drop unused label. Best regards, Krzysztof
Hi Krzysztof, On 9/09/24 18:38, Krzysztof Kozlowski wrote: > On Mon, Sep 09, 2024 at 01:47:06PM +1200, Chris Packham wrote: >> Add device tree schema for the Realtek switch. Currently the only >> supported feature is the syscon-reboot which is needed to be able to >> reboot the board. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> .../bindings/mfd/realtek,switch.yaml | 50 +++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml > Use compatible as filename. My hope was eventually that this would support multiple Realtek switches. But sure for now at least I can name it after the one in front of me. > >> diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml >> new file mode 100644 >> index 000000000000..84b57f87bd3a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml >> @@ -0,0 +1,50 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://scanmail.trustwave.com/?c=20988&d=55fe5gyquxahZ_dJqiHMxmkDG8M1MWjoNtZN70yrng&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmfd%2frealtek%2cswitch%2eyaml%23 >> +$schema: http://scanmail.trustwave.com/?c=20988&d=55fe5gyquxahZ_dJqiHMxmkDG8M1MWjoNoNFvkz8nA&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23 >> + >> +title: Realtek Switch with Internal CPU > What sort of Switch? Like network switch? Then this should be placed in > respective net (or deeper, e.g. net/dsa/) directory. Yes network switch. But this is one of those all in one chips that has a CPU, network switch and various peripherals. MFD seemed appropriate. > > Maintainers go here. See example-schema. Ack. >> + >> +description: >> + The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port >> + networking switch that supports many interfaces. Additionally, the device can >> + support MDIO, SPI and I2C busses. > I don't get why syscon node is called switch. This looks incomplete or > you used description from some other device. Yes I did take a lot of inspiration from the mscc,ocelot. I am working on more support for the switch and some of the other peripherals so I figured I'd word it towards that end goal. If you prefer I could word this more towards the one function (reboot) that is supported right now. > But if this is DSA, then you miss dsa ref and dsa-related properties. So far I'm resisting DSA. The usage of the RTL9300 as a SoC+Switch doesn't really lend itself to the DSA architecture (there is a external CPU mode that would). I think eventually we'd end up with something like the mscc,oscelot where both switchdev and DSA usage is supported. There would be some properties (e.g. port/phy arrangement) that apply to both uses. I have got a (kind of) working proof of concept switchdev driver which has some of the support you've mentioned. It's not really ready so I didn't include the dt-binding for that stuff in this patch. >> + >> +maintainers: >> + - Chris Packham <chris.packham@alliedtelesis.co.nz> >> + >> +properties: >> + compatible: >> + items: >> + - enum: >> + - realtek,rtl9302c-switch >> + - const: syscon >> + - const: simple-mfd >> + >> + reg: >> + maxItems: 1 >> + >> + reboot: >> + $ref: /schemas/power/reset/syscon-reboot.yaml# >> + >> +required: >> + - compatible >> + - reg >> + - reboot >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + switch0: ethernet-switch@1b000000 { > Drop unused label. Ack. > > Best regards, > Krzysztof >
On 09/09/2024 22:36, Chris Packham wrote: > Hi Krzysztof, > > On 9/09/24 18:38, Krzysztof Kozlowski wrote: >> On Mon, Sep 09, 2024 at 01:47:06PM +1200, Chris Packham wrote: >>> Add device tree schema for the Realtek switch. Currently the only >>> supported feature is the syscon-reboot which is needed to be able to >>> reboot the board. >>> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>> --- >>> .../bindings/mfd/realtek,switch.yaml | 50 +++++++++++++++++++ >>> 1 file changed, 50 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml >> Use compatible as filename. > > My hope was eventually that this would support multiple Realtek > switches. But sure for now at least I can name it after the one in front > of me. This might never happen, so unless you document more models now, I strongly suggest using compatible as filename. > >> >>> diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml >>> new file mode 100644 >>> index 000000000000..84b57f87bd3a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml >>> @@ -0,0 +1,50 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://scanmail.trustwave.com/?c=20988&d=55fe5gyquxahZ_dJqiHMxmkDG8M1MWjoNtZN70yrng&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmfd%2frealtek%2cswitch%2eyaml%23 >>> +$schema: http://scanmail.trustwave.com/?c=20988&d=55fe5gyquxahZ_dJqiHMxmkDG8M1MWjoNoNFvkz8nA&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23 >>> + >>> +title: Realtek Switch with Internal CPU >> What sort of Switch? Like network switch? Then this should be placed in >> respective net (or deeper, e.g. net/dsa/) directory. > Yes network switch. But this is one of those all in one chips that has a > CPU, network switch and various peripherals. MFD seemed appropriate. There is no such device as MFD. MFD is a Linux framework. >> >> Maintainers go here. See example-schema. > > Ack. > >>> + >>> +description: >>> + The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port >>> + networking switch that supports many interfaces. Additionally, the device can >>> + support MDIO, SPI and I2C busses. >> I don't get why syscon node is called switch. This looks incomplete or >> you used description from some other device. > > Yes I did take a lot of inspiration from the mscc,ocelot. I am working > on more support for the switch and some of the other peripherals so I > figured I'd word it towards that end goal. If you prefer I could word > this more towards the one function (reboot) that is supported right now. Your commit msg is not explaining here much. And "Currently the only supported" feels like a driver description. We expect bindings to be complete. It's fine to bring partial description of hardware, but this should be explained in the commit msg and entire binding should be still created to accommodate that full description. However such complex devices like Ocelot should be described fully so we can easily see how you organize entire binding. > >> But if this is DSA, then you miss dsa ref and dsa-related properties. > > So far I'm resisting DSA. The usage of the RTL9300 as a SoC+Switch > doesn't really lend itself to the DSA architecture (there is a external > CPU mode that would). I think eventually we'd end up with something like > the mscc,oscelot where both switchdev and DSA usage is supported. There > would be some properties (e.g. port/phy arrangement) that apply to both > uses. This feels ok, although you really should create complete binding here. > > I have got a (kind of) working proof of concept switchdev driver which > has some of the support you've mentioned. It's not really ready so I > didn't include the dt-binding for that stuff in this patch. Best regards, Krzysztof
Hi Krzysztof, (sorry, re-sending as plain text) On 10/09/24 19:26, Krzysztof Kozlowski wrote: > On 09/09/2024 22:36, Chris Packham wrote: >> Hi Krzysztof, >> >> On 9/09/24 18:38, Krzysztof Kozlowski wrote: >>> On Mon, Sep 09, 2024 at 01:47:06PM +1200, Chris Packham wrote: >>>> Add device tree schema for the Realtek switch. Currently the only >>>> supported feature is the syscon-reboot which is needed to be able to >>>> reboot the board. >>>> >>>> Signed-off-by: Chris Packham<chris.packham@alliedtelesis.co.nz> >>>> --- >>>> .../bindings/mfd/realtek,switch.yaml | 50 +++++++++++++++++++ >>>> 1 file changed, 50 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml >>> Use compatible as filename. >> My hope was eventually that this would support multiple Realtek >> switches. But sure for now at least I can name it after the one in front >> of me. > This might never happen, so unless you document more models now, I > strongly suggest using compatible as filename. Agreed. >>>> diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml >>>> new file mode 100644 >>>> index 000000000000..84b57f87bd3a >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml >>>> @@ -0,0 +1,50 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id:http://scanmail.trustwave.com/?c=20988&d=s_Tf5n4B2HBkm1hFlKUTA3UKwK2Jg2EPHeQMRCQHXQ&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmfd%2frealtek%2cswitch%2eyaml%23 >>>> +$schema:http://scanmail.trustwave.com/?c=20988&d=s_Tf5n4B2HBkm1hFlKUTA3UKwK2Jg2EPHbEEFSRQXw&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23 >>>> + >>>> +title: Realtek Switch with Internal CPU >>> What sort of Switch? Like network switch? Then this should be placed in >>> respective net (or deeper, e.g. net/dsa/) directory. >> Yes network switch. But this is one of those all in one chips that has a >> CPU, network switch and various peripherals. MFD seemed appropriate. > There is no such device as MFD. MFD is a Linux framework. What I meant was the RTL9302 is a similar class of device to the mscc,ocelot which has a binding in Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml. If it's a case of that being historical baggage then Documentation/devicetree/bindings/mips/ might be appropriate for the SoC type properties and Documentation/devicetree/bindings/net/ for the switch portion. >>> Maintainers go here. See example-schema. >> Ack. >> >>>> + >>>> +description: >>>> + The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port >>>> + networking switch that supports many interfaces. Additionally, the device can >>>> + support MDIO, SPI and I2C busses. >>> I don't get why syscon node is called switch. This looks incomplete or >>> you used description from some other device. >> Yes I did take a lot of inspiration from the mscc,ocelot. I am working >> on more support for the switch and some of the other peripherals so I >> figured I'd word it towards that end goal. If you prefer I could word >> this more towards the one function (reboot) that is supported right now. > Your commit msg is not explaining here much. And "Currently the only > supported" feels like a driver description. We expect bindings to be > complete. It's fine to bring partial description of hardware, but this > should be explained in the commit msg and entire binding should be still > created to accommodate that full description. > > However such complex devices like Ocelot should be described fully so we > can easily see how you organize entire binding. I can definitely do a better job of explaining myself in the commit message. Right now I just want to have a working reboot. I'm not really using the "realtek,rtl9302c-switch" compatible for anything but I gather using a "syscon" node without a more specific compatible is frowned upon (please tell me if I'm wrong). In terms of the binding should I just make the description something terse like "The RTL9302 ethernet switch has an internal CPU. Some peripherals are accessed via a common register block". >>> But if this is DSA, then you miss dsa ref and dsa-related properties. >> So far I'm resisting DSA. The usage of the RTL9300 as a SoC+Switch >> doesn't really lend itself to the DSA architecture (there is a external >> CPU mode that would). I think eventually we'd end up with something like >> the mscc,oscelot where both switchdev and DSA usage is supported. There >> would be some properties (e.g. port/phy arrangement) that apply to both >> uses. > This feels ok, although you really should create complete binding here. This is a bit of a chicken and egg thing. I don't want to commit to a binding until I have more code to back it up, but of course I don't want to spend a bunch of time writing code for a binding that then needs to change when that binding is reviewed. >> I have got a (kind of) working proof of concept switchdev driver which >> has some of the support you've mentioned. It's not really ready so I >> didn't include the dt-binding for that stuff in this patch. > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml new file mode 100644 index 000000000000..84b57f87bd3a --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/realtek,switch.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Realtek Switch with Internal CPU + +description: + The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port + networking switch that supports many interfaces. Additionally, the device can + support MDIO, SPI and I2C busses. + +maintainers: + - Chris Packham <chris.packham@alliedtelesis.co.nz> + +properties: + compatible: + items: + - enum: + - realtek,rtl9302c-switch + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + reboot: + $ref: /schemas/power/reset/syscon-reboot.yaml# + +required: + - compatible + - reg + - reboot + +additionalProperties: false + +examples: + - | + switch0: ethernet-switch@1b000000 { + compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd"; + reg = <0x1b000000 0x10000>; + + reboot { + compatible = "syscon-reboot"; + offset = <0x0c>; + value = <0x01>; + }; + }; +
Add device tree schema for the Realtek switch. Currently the only supported feature is the syscon-reboot which is needed to be able to reboot the board. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- .../bindings/mfd/realtek,switch.yaml | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml