| Message ID | 20241012134834.1306992-2-markus.stockhausen@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: Realtek Otto SerDes: add new driver | expand |
On Sat, Oct 12, 2024 at 09:48:33AM -0400, Markus Stockhausen wrote: > Add bindings for the SerDes of the Realtek Otto platform. These are > MIPS based network Switch SoCs with up to 52 ports divided into four > different model lines. > > Changes in v3 > <form letter> This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. </form letter> > - renamed to realtek,rtl8380m-serdes.yaml > - removed parameter controlled-ports > - verified with make dt_binding_check > - recipient list according to get_maintainers > > Changes in v2: > > - new subject > - removed patch command sequences > - renamed parameter controlled-ports to realtek,controlled-ports > > Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de> > --- > .../bindings/phy/realtek,rtl8380m-serdes.yaml | 67 +++++++++++++++++++ > 1 file changed, 67 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml > > diff --git a/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml b/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml > new file mode 100644 > index 000000000000..c1deef8ec63c > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml > @@ -0,0 +1,67 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/realtek,rtl8380m-serdes.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Realtek Otto SerDes controller > + > +maintainers: > + - Markus Stockhausen <markus.stockhausen@gmx.de> > + > +description: > + The MIPS based Realtek Switch SoCs of the Realtek RTL838x, RTL839x, RTL930x > + and RTL931x series have multiple SerDes built in. They are linked to single, > + quad or octa PHYs like the RTL8218B, RTL8218D or RTL8214FC and are one of > + the integral part of the up-to-52-port switch architecture. Although these > + SerDes controllers have common basics they are designed differently in the > + SoC families. > + > +properties: > + $nodename: > + pattern: "^phy@[0-9a-f]+$" > + > + compatible: > + items: > + - enum: > + - realtek,rtl8380m-serdes > + - realtek,rtl8392m-serdes > + - realtek,rtl9302b-serdes > + - realtek,rtl9311-serdes > + > + reg: > + items: > + description: > + The primary register memory location. On RTL83xx devices this is the > + address to the I/O register range, on RTL93xx devices this is the > + address of the MDIO style command/data registers. Not much improved, still missing constraints. I don't understand why you introduce changes like this. > + > + "#phy-cells": > + const: 4 > + description: > + The first number defines the SerDes to use. The second number a linked > + SerDes. E.g. if a octa 1G PHY is attached to two QSGMII SerDes. The third > + number is the first switch port this SerDes is working for, the fourth > + number is the last switch port the SerDes is working for. > + > + firmware-name: > + maxItems: 1 > + description: > + An alternative name of the SerDes firmware image file located in the > + firmware search path. Set to "" to disable firmware loading. Missing property, not empty string, should rather indicate unneeded firmware. > + > +required: > + - compatible > + - reg > + - "#phy-cells" > + > +additionalProperties: false > + > +examples: > + - | > + serdes: phy@1b00e780 { > + compatible = "realtek,rtl9302b-serdes"; > + reg = <0x1b0003b0 0x8>; This does notmatch unit address... and again: this was not an issue in previous version. Your new versions of patchset introduce errors and bugs. This is not how the process should look like. Review should improve the patch, not reduce its quality. Please start carefully reviewing your changes *before* you post. Best regards, Krzysztof
> -----Ursprüngliche Nachricht----- > Von: Krzysztof Kozlowski <krzk@kernel.org> > Gesendet: Montag, 14. Oktober 2024 09:18 > An: Markus Stockhausen <markus.stockhausen@gmx.de> > Cc: vkoul@kernel.org; kishon@kernel.org; robh@kernel.org; krzk+dt@kernel.org; > conor+dt@kernel.org; linux-phy@lists.infradead.org; devicetree@vger.kernel.org; > chris.packham@alliedtelesis.co.nz > Betreff: Re: [PATCH v3 1/2] dt-bindings: phy: add realtek,rtl8380m-serdes > > On Sat, Oct 12, 2024 at 09:48:33AM -0400, Markus Stockhausen wrote: > > Add bindings for the SerDes of the Realtek Otto platform. These are > > MIPS based network Switch SoCs with up to 52 ports divided into four > > different model lines. > > ... > > + reg: > > + items: > > + description: > > + The primary register memory location. On RTL83xx devices this is the > > + address to the I/O register range, on RTL93xx devices this is the > > + address of the MDIO style command/data registers. > > Not much improved, still missing constraints. > > I don't understand why you introduce changes like this. Hm, two stupid changes in the last two versions. This was only to get some more meaningfull description there. Not proud of it and somehow lost what will be right. Looking at other places a simple reg: maxItems: 1 should be sufficient. Ok with that? > > + > > + "#phy-cells": > > + const: 4 > > + description: > > + The first number defines the SerDes to use. The second number a linked > > + SerDes. E.g. if a octa 1G PHY is attached to two QSGMII SerDes. The third > > + number is the first switch port this SerDes is working for, the fourth > > + number is the last switch port the SerDes is working for. > > + > > + firmware-name: > > + maxItems: 1 > > + description: > > + An alternative name of the SerDes firmware image file located in the > > + firmware search path. Set to "" to disable firmware loading. > > Missing property, not empty string, should rather indicate unneeded firmware. Indeed more intuitive. Will drop the hardcoded firmware names in the driver and only load if firmware-name is set. > > + > > +examples: > > + - | > > + serdes: phy@1b00e780 { > > + compatible = "realtek,rtl9302b-serdes"; > > + reg = <0x1b0003b0 0x8>; > > This does notmatch unit address... and again: this was not an issue in previous version. > Your new versions of patchset introduce errors and bugs. This is not how the process > should look like. Review should improve the patch, not reduce its quality. Agree will fix. This was wrongly mixed when I removed 3 of the samples as requested by Rob and cleaned the rest afterwards to have the firmware example. Markus
diff --git a/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml b/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml new file mode 100644 index 000000000000..c1deef8ec63c --- /dev/null +++ b/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/realtek,rtl8380m-serdes.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Realtek Otto SerDes controller + +maintainers: + - Markus Stockhausen <markus.stockhausen@gmx.de> + +description: + The MIPS based Realtek Switch SoCs of the Realtek RTL838x, RTL839x, RTL930x + and RTL931x series have multiple SerDes built in. They are linked to single, + quad or octa PHYs like the RTL8218B, RTL8218D or RTL8214FC and are one of + the integral part of the up-to-52-port switch architecture. Although these + SerDes controllers have common basics they are designed differently in the + SoC families. + +properties: + $nodename: + pattern: "^phy@[0-9a-f]+$" + + compatible: + items: + - enum: + - realtek,rtl8380m-serdes + - realtek,rtl8392m-serdes + - realtek,rtl9302b-serdes + - realtek,rtl9311-serdes + + reg: + items: + description: + The primary register memory location. On RTL83xx devices this is the + address to the I/O register range, on RTL93xx devices this is the + address of the MDIO style command/data registers. + + "#phy-cells": + const: 4 + description: + The first number defines the SerDes to use. The second number a linked + SerDes. E.g. if a octa 1G PHY is attached to two QSGMII SerDes. The third + number is the first switch port this SerDes is working for, the fourth + number is the last switch port the SerDes is working for. + + firmware-name: + maxItems: 1 + description: + An alternative name of the SerDes firmware image file located in the + firmware search path. Set to "" to disable firmware loading. + +required: + - compatible + - reg + - "#phy-cells" + +additionalProperties: false + +examples: + - | + serdes: phy@1b00e780 { + compatible = "realtek,rtl9302b-serdes"; + reg = <0x1b0003b0 0x8>; + firmware-name = "zyxel-xgs1210-12-serdes.fw"; + #phy-cells = <4>; + };
Add bindings for the SerDes of the Realtek Otto platform. These are MIPS based network Switch SoCs with up to 52 ports divided into four different model lines. Changes in v3 - renamed to realtek,rtl8380m-serdes.yaml - removed parameter controlled-ports - verified with make dt_binding_check - recipient list according to get_maintainers Changes in v2: - new subject - removed patch command sequences - renamed parameter controlled-ports to realtek,controlled-ports Signed-off-by: Markus Stockhausen <markus.stockhausen@gmx.de> --- .../bindings/phy/realtek,rtl8380m-serdes.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml -- 2.46.2