| Message ID | 20221003164624.4823-1-jerry.ray@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] dt-bindings: dsa: Add lan9303 yaml | expand |
On 03/10/2022 18:46, Jerry Ray wrote: > Adding the dt binding yaml for the lan9303 3-port ethernet switch. > The microchip lan9354 3-port ethernet switch will also use the > same binding. > > Signed-off-by: Jerry Ray <jerry.ray@microchip.com> > --- > v3->v4: > - Addressed v3 community feedback > v2->v3: > - removed cpu labels > - now patching against latest net-next > v1->v2: > - fixed dt_binding_check warning > - added max-speed setting on the switches 10/100 ports. > --- > .../devicetree/bindings/net/dsa/lan9303.txt | 100 ------------- > .../bindings/net/dsa/microchip,lan9303.yaml | 134 ++++++++++++++++++ > MAINTAINERS | 8 ++ > 3 files changed, 142 insertions(+), 100 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/net/dsa/lan9303.txt > create mode 100644 Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > > diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt > deleted file mode 100644 > index 46a732087f5c..000000000000 > --- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt > +++ /dev/null > @@ -1,100 +0,0 @@ > -SMSC/MicroChip LAN9303 three port ethernet switch > -------------------------------------------------- > - > -Required properties: > - > -- compatible: should be > - - "smsc,lan9303-i2c" for I2C managed mode > - or > - - "smsc,lan9303-mdio" for mdio managed mode > - > -Optional properties: > - > -- reset-gpios: GPIO to be used to reset the whole device > -- reset-duration: reset duration in milliseconds, defaults to 200 ms > - > -Subnodes: > - > -The integrated switch subnode should be specified according to the binding > -described in dsa/dsa.txt. The CPU port of this switch is always port 0. > - > -Note: always use 'reg = <0/1/2>;' for the three DSA ports, even if the device is > -configured to use 1/2/3 instead. This hardware configuration will be > -auto-detected and mapped accordingly. > - > -Example: > - > -I2C managed mode: > - > - master: masterdevice@X { > - > - fixed-link { /* RMII fixed link to LAN9303 */ > - speed = <100>; > - full-duplex; > - }; > - }; > - > - switch: switch@a { > - compatible = "smsc,lan9303-i2c"; > - reg = <0xa>; > - reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>; > - reset-duration = <200>; > - > - ports { > - #address-cells = <1>; > - #size-cells = <0>; > - > - port@0 { /* RMII fixed link to master */ > - reg = <0>; > - ethernet = <&master>; > - }; > - > - port@1 { /* external port 1 */ > - reg = <1>; > - label = "lan1"; > - }; > - > - port@2 { /* external port 2 */ > - reg = <2>; > - label = "lan2"; > - }; > - }; > - }; > - > -MDIO managed mode: > - > - master: masterdevice@X { > - phy-handle = <&switch>; > - > - mdio { > - #address-cells = <1>; > - #size-cells = <0>; > - > - switch: switch-phy@0 { > - compatible = "smsc,lan9303-mdio"; > - reg = <0>; > - reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>; > - reset-duration = <100>; > - > - ports { > - #address-cells = <1>; > - #size-cells = <0>; > - > - port@0 { > - reg = <0>; > - ethernet = <&master>; > - }; > - > - port@1 { /* external port 1 */ > - reg = <1>; > - label = "lan1"; > - }; > - > - port@2 { /* external port 2 */ > - reg = <2>; > - label = "lan2"; > - }; > - }; > - }; > - }; > - }; > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > new file mode 100644 > index 000000000000..ca6cbe83ba75 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > @@ -0,0 +1,134 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/dsa/microchip,lan9303.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: LAN9303 Ethernet Switch Series > + > +allOf: > + - $ref: dsa.yaml# > + > +maintainers: > + - UNGLinuxDriver@microchip.com > + > +properties: > + compatible: > + oneOf: > + - enum: > + - smsc,lan9303-mdio > + - microchip,lan9354-mdio > + - enum: > + - smsc,lan9303-i2c > + - microchip,lan9354-i2c This still does not make sense. It is one enum. Drop oneOf. > + > + reg: > + maxItems: 1 > + > + reset-gpios: > + description: Optional reset line > + maxItems: 1 > + > + reset-duration: > + description: Reset duration in milliseconds > + default: 200 This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my 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. > + > +required: > + - compatible > + - reg > + Best regards, Krzysztof
Hi Jerry,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Jerry-Ray/dt-bindings-dsa-Add-lan9303-yaml/20221004-004751
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 62c07983bef9d3e78e71189441e1a470f0d1e653
reproduce:
# https://github.com/intel-lab-lkp/linux/commit/4d563240917341c3ec85dc17b0a47c3977ee8875
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jerry-Ray/dt-bindings-dsa-Add-lan9303-yaml/20221004-004751
git checkout 4d563240917341c3ec85dc17b0a47c3977ee8875
make menuconfig
# enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
make htmldocs
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> Warning: Documentation/networking/dsa/lan9303.rst references a file that doesn't exist: Documentation/devicetree/bindings/net/dsa/lan9303.txt
On 03/10/2022 19:48, Krzysztof Kozlowski wrote: > On 03/10/2022 18:46, Jerry Ray wrote: >> Adding the dt binding yaml for the lan9303 3-port ethernet switch. >> The microchip lan9354 3-port ethernet switch will also use the >> same binding. >> >> Signed-off-by: Jerry Ray <jerry.ray@microchip.com> >> --- >> v3->v4: >> - Addressed v3 community feedback >> v2->v3: >> - removed cpu labels >> - now patching against latest net-next >> v1->v2: >> - fixed dt_binding_check warning >> - added max-speed setting on the switches 10/100 ports. >> --- >> .../devicetree/bindings/net/dsa/lan9303.txt | 100 ------------- Beside that you got errors reported by kernel test robot. Best regards, Krzysztof
On Mon, Oct 03, 2022 at 11:46:24AM -0500, Jerry Ray wrote: > --- > v3->v4: > - Addressed v3 community feedback More specifically? > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + // Ethernet switch connected via mdio to the host > + ethernet { > + #address-cells = <1>; > + #size-cells = <0>; > + phy-handle = <&lan9303switch>; > + phy-mode = "rmii"; > + fixed-link { > + speed = <100>; > + full-duplex; > + }; I see the phy-handle to the switch is inherited from the .txt dt-binding, but I don't understand it. The switch is an mdio_device, not a phy_device, so what will this do? Also, any reasonable host driver will error out if it finds a phy-handle and a fixed-link in its OF node. So one of phy-handle or fixed-link must be dropped, they are bogus. Even better, just stick to the mdio node as root and drop the DSA master OF node, like other DSA dt-binding examples do. You can have dangling phandles, so "ethernet = <ðernet>" below is not an issue. > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + lan9303switch: switch@0 { > + compatible = "smsc,lan9303-mdio"; > + reg = <0>; > + dsa,member = <0 0>; Redundant, please remove. > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + phy-mode = "rmii"; FWIW, RMII has a MAC mode and a PHY mode. Two RMII interfaces connected in MAC mode to one another don't work. You'll have problems if you also have an RMII PHY connected to one of the xMII ports, and you describe phy-mode = "rmii" for both. There exists a "rev-rmii" phy-mode to denote an RMII interface working in PHY mode. Wonder if you should be using that here. > + ethernet = <ðernet>; > + fixed-link { > + speed = <100>; > + full-duplex; > + }; > + }; > + port@1 { > + reg = <1>; > + max-speed = <100>; > + label = "lan1"; > + }; > + port@2 { > + reg = <2>; > + max-speed = <100>; > + label = "lan2"; > + }; > + }; > + }; > + }; > + }; > + > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + // Ethernet switch connected via i2c to the host > + ethernet { > + #address-cells = <1>; > + #size-cells = <0>; > + phy-mode = "rmii"; > + speed = <100>; > + fixed-link { > + full-duplex; > + }; > + }; No need for this node. > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + lan9303: switch@1a { > + compatible = "smsc,lan9303-i2c"; > + reg = <0x1a>; > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + phy-mode = "rmii"; > + ethernet = <ðernet>; > + fixed-link { > + speed = <100>; > + full-duplex; > + }; > + }; > + port@1 { > + reg = <1>; > + max-speed = <100>; > + label = "lan1"; > + }; > + port@2 { > + reg = <2>; > + max-speed = <100>; > + label = "lan2"; > + }; > + }; > + }; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index 5d58b55c5ae5..89055ff2838a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13386,6 +13386,14 @@ L: netdev@vger.kernel.org > S: Maintained > F: drivers/net/ethernet/microchip/lan743x_* > > +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER > +M: Jerry Ray <jerry.ray@microchip.com> > +M: UNGLinuxDriver@microchip.com > +L: netdev@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > +F: drivers/net/dsa/lan9303* > + Separate patch please? Changes to the MAINTAINERS file get applied to the "net" tree. > MICROCHIP LAN966X ETHERNET DRIVER > M: Horatiu Vultur <horatiu.vultur@microchip.com> > M: UNGLinuxDriver@microchip.com > -- > 2.25.1 >
On 09/10/2022 00:56, Vladimir Oltean wrote: >> >> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER >> +M: Jerry Ray <jerry.ray@microchip.com> >> +M: UNGLinuxDriver@microchip.com >> +L: netdev@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml >> +F: drivers/net/dsa/lan9303* >> + > > Separate patch please? Changes to the MAINTAINERS file get applied to > the "net" tree. This will also go via net tree, so there is no real need to split it. Best regards, Krzysztof
On Sun, Oct 09, 2022 at 05:20:03PM +0200, Krzysztof Kozlowski wrote: > On 09/10/2022 00:56, Vladimir Oltean wrote: > >> > >> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER > >> +M: Jerry Ray <jerry.ray@microchip.com> > >> +M: UNGLinuxDriver@microchip.com > >> +L: netdev@vger.kernel.org > >> +S: Maintained > >> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > >> +F: drivers/net/dsa/lan9303* > >> + > > > > Separate patch please? Changes to the MAINTAINERS file get applied to > > the "net" tree. > > This will also go via net tree, so there is no real need to split it. I meant exactly what I wrote, "net" tree as in the networking tree where fixes to the current master branch are sent: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git, or in other words, not net-next.git where new features are sent: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
On 09/10/2022 18:22, Vladimir Oltean wrote: > On Sun, Oct 09, 2022 at 05:20:03PM +0200, Krzysztof Kozlowski wrote: >> On 09/10/2022 00:56, Vladimir Oltean wrote: >>>> >>>> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER >>>> +M: Jerry Ray <jerry.ray@microchip.com> >>>> +M: UNGLinuxDriver@microchip.com >>>> +L: netdev@vger.kernel.org >>>> +S: Maintained >>>> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml >>>> +F: drivers/net/dsa/lan9303* >>>> + >>> >>> Separate patch please? Changes to the MAINTAINERS file get applied to >>> the "net" tree. >> >> This will also go via net tree, so there is no real need to split it. > > I meant exactly what I wrote, "net" tree as in the networking tree where > fixes to the current master branch are sent: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git, or in > other words, not net-next.git where new features are sent: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git Ah, but how it can go to fixes? It has invalid path (in the context of net-fixes) and it is not related to anything in the current cycle. Best regards, Krzysztof
On Mon, Oct 10, 2022 at 06:23:24AM -0400, Krzysztof Kozlowski wrote: > On 09/10/2022 18:22, Vladimir Oltean wrote: > > On Sun, Oct 09, 2022 at 05:20:03PM +0200, Krzysztof Kozlowski wrote: > >> On 09/10/2022 00:56, Vladimir Oltean wrote: > >>>> > >>>> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER > >>>> +M: Jerry Ray <jerry.ray@microchip.com> > >>>> +M: UNGLinuxDriver@microchip.com > >>>> +L: netdev@vger.kernel.org > >>>> +S: Maintained > >>>> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > >>>> +F: drivers/net/dsa/lan9303* > >>>> + > >>> > >>> Separate patch please? Changes to the MAINTAINERS file get applied to > >>> the "net" tree. > >> > >> This will also go via net tree, so there is no real need to split it. > > > > I meant exactly what I wrote, "net" tree as in the networking tree where > > fixes to the current master branch are sent: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git, or in > > other words, not net-next.git where new features are sent: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > > Ah, but how it can go to fixes? It has invalid path (in the context of > net-fixes) and it is not related to anything in the current cycle. Personally I'd split the patch into 2 pieces, the MAINTAINERS entry for the drivers/net/dsa/lan9303* portion, plus the current .txt schema, which goes to "net" right away, wait until the net tree gets merged back into net-next (happens when submissions for net-next reopen), then add the dt-bindings and rename the .txt schema from MAINTAINERS to .yaml.
>> + >> + reg: >> + maxItems: 1 >> + >> + reset-gpios: >> + description: Optional reset line >> + maxItems: 1 >> + >> + reset-duration: >> + description: Reset duration in milliseconds >> + default: 200 > >This is a friendly reminder during the review process. > >It seems my previous comments were not fully addressed. Maybe my >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. > I am documenting "what is" rather than what I think it should be. I would prefer there be a "-ms" suffix on the name, but that was not what was in the pre-existing code. I added the "default: 200" line and can add a "maxItems: 1", but begin getting errors when I attempt to further define this field as a uint32 type or anything like that. And no, I'm not getting any warnings or errors from the dt_bindings_check. I reviewed the v3 comments. The only other thing I missed in v4 was changing the comments from C++ style to C style comments. >> + >> +required: >> + - compatible >> + - reg >> + >Best regards, >Krzysztof > Regards, Jerry.
>> On 09/10/2022 18:22, Vladimir Oltean wrote: >> > On Sun, Oct 09, 2022 at 05:20:03PM +0200, Krzysztof Kozlowski wrote: >> >> On 09/10/2022 00:56, Vladimir Oltean wrote: >> >>>> >> >>>> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER >> >>>> +M: Jerry Ray <jerry.ray@microchip.com> >> >>>> +M: UNGLinuxDriver@microchip.com >> >>>> +L: netdev@vger.kernel.org >> >>>> +S: Maintained >> >>>> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml >> >>>> +F: drivers/net/dsa/lan9303* >> >>>> + >> >>> >> >>> Separate patch please? Changes to the MAINTAINERS file get applied to >> >>> the "net" tree. >> >> >> >> This will also go via net tree, so there is no real need to split it. >> > >> > I meant exactly what I wrote, "net" tree as in the networking tree where >> > fixes to the current master branch are sent: >> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git, or in >> > other words, not net-next.git where new features are sent: >> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git >> >> Ah, but how it can go to fixes? It has invalid path (in the context of >> net-fixes) and it is not related to anything in the current cycle. > >Personally I'd split the patch into 2 pieces, the MAINTAINERS entry for >the drivers/net/dsa/lan9303* portion, plus the current .txt schema, >which goes to "net" right away, wait until the net tree gets merged back >into net-next (happens when submissions for net-next reopen), then add >the dt-bindings and rename the .txt schema from MAINTAINERS to .yaml. > If this patch should be flagged [net] rather than [net-next], please tell me. I'm looking to add content to the driver going forward and assumed net-next. Splitting the patch into 2 steps didn't make a lot of sense to me. Splitting the patch into 2 patches targeting 2 different repos makes even less sense. I assume the net MAINTAINERS list to be updated from net-next contributions on the next cycle. As I'm now outright deleting the lan9303.txt file, I'm getting the test bot error about also needing to change the rst file that references lan9303.txt. I'll do so in the next revision. The alternative is to drop the yaml, simply add to the old txt file, and be done with it. Your call. Regards, Jerry.
On Mon, Oct 17, 2022 at 06:51:57PM +0000, Jerry.Ray@microchip.com wrote: > >> On 09/10/2022 18:22, Vladimir Oltean wrote: > >> > On Sun, Oct 09, 2022 at 05:20:03PM +0200, Krzysztof Kozlowski wrote: > >> >> On 09/10/2022 00:56, Vladimir Oltean wrote: > >> >>>> > >> >>>> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER > >> >>>> +M: Jerry Ray <jerry.ray@microchip.com> > >> >>>> +M: UNGLinuxDriver@microchip.com > >> >>>> +L: netdev@vger.kernel.org > >> >>>> +S: Maintained > >> >>>> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml > >> >>>> +F: drivers/net/dsa/lan9303* > >> >>>> + > >> >>> > >> >>> Separate patch please? Changes to the MAINTAINERS file get applied to > >> >>> the "net" tree. > >> >> > >> >> This will also go via net tree, so there is no real need to split it. > >> > > >> > I meant exactly what I wrote, "net" tree as in the networking tree where > >> > fixes to the current master branch are sent: > >> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git, or in > >> > other words, not net-next.git where new features are sent: > >> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > >> > >> Ah, but how it can go to fixes? It has invalid path (in the context of > >> net-fixes) and it is not related to anything in the current cycle. > > > >Personally I'd split the patch into 2 pieces, the MAINTAINERS entry for > >the drivers/net/dsa/lan9303* portion, plus the current .txt schema, > >which goes to "net" right away, wait until the net tree gets merged back > >into net-next (happens when submissions for net-next reopen), then add > >the dt-bindings and rename the .txt schema from MAINTAINERS to .yaml. > > > > If this patch should be flagged [net] rather than [net-next], please tell > me. I'm looking to add content to the driver going forward and assumed > net-next. Splitting the patch into 2 steps didn't make a lot of sense to > me. Splitting the patch into 2 patches targeting 2 different repos makes > even less sense. I assume the net MAINTAINERS list to be updated from > net-next contributions on the next cycle. > > As I'm now outright deleting the lan9303.txt file, I'm getting the test bot > error about also needing to change the rst file that references lan9303.txt. > I'll do so in the next revision. The alternative is to drop the yaml, simply > add to the old txt file, and be done with it. Your call. > > Regards, > Jerry. Ah, this is not a "my call" thing. The portion I highlighted of the change you're making includes your name into the output of $(./scripts/get_maintainer.pl drivers/net/dsa/lan9303-core.c). In other words, you're voluntarily subscribing to the responsibility of being a maintainer for the driver, getting emails from other developers, reviewing patches. Furthermore, you also maintain the code in the stable trees, hence your name also gets propagated there so people who use those kernels can report problems to you. The MAINTAINERS entry for lan9303 needs to go to the "net" tree, from where it can be backported. This covers the driver + schema files as they currently are. The change of the .txt to the .yaml schema then comes on top of that (and on "net-next").
On Mon, 17 Oct 2022 22:13:11 +0300 Vladimir Oltean wrote: > The portion I highlighted of the change you're making includes your name > into the output of $(./scripts/get_maintainer.pl drivers/net/dsa/lan9303-core.c). > In other words, you're voluntarily subscribing to the responsibility of > being a maintainer for the driver, getting emails from other developers, > reviewing patches. Furthermore, you also maintain the code in the stable > trees, hence your name also gets propagated there so people who use > those kernels can report problems to you. > > The MAINTAINERS entry for lan9303 needs to go to the "net" tree, from > where it can be backported. This covers the driver + schema files as > they currently are. The change of the .txt to the .yaml schema then > comes on top of that (and on "net-next"). And FWIW net gets merged into net-next every Thu so (compared to how long this patch had been in review) it won't be a large delay to wait for the MAINTAINERS patch to propagate.
>> --- >> v3->v4: >> - Addressed v3 community feedback > >More specifically? > - Old lan9303.txt file is totally removed rather than containing text that redirects the user to the microchip,lan9303.yaml source file. - Drop "Tree Bindings" from title - Drop quotes from dsa.yaml reference line. - Modified the compatible second enum to include a second string. (( I now realize this is not what was being asked for and have made it a single enum with 4 items, removing the oneOf. )) - Drop "gpio specifier for a" in reset-gpois description - added a default: property to the reset-duration item and set it to 200. - Drop "0" from the ethernet name. Split the MDIO and I2C examples into two so that the number is no longer needed. - Placed the reg property to be directly following the compatible string in the mdio node. >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + >> + // Ethernet switch connected via mdio to the host >> + ethernet { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + phy-handle = <&lan9303switch>; >> + phy-mode = "rmii"; >> + fixed-link { >> + speed = <100>; >> + full-duplex; >> + }; > >I see the phy-handle to the switch is inherited from the .txt dt-binding, >but I don't understand it. The switch is an mdio_device, not a phy_device, >so what will this do? > >Also, any reasonable host driver will error out if it finds a phy-handle >and a fixed-link in its OF node. So one of phy-handle or fixed-link must >be dropped, they are bogus. > >Even better, just stick to the mdio node as root and drop the DSA master >OF node, like other DSA dt-binding examples do. You can have dangling >phandles, so "ethernet = <ðernet>" below is not an issue. > I can remove the phy-handle, but I'm trying to establish the link between this ethernet port and port0 (the CPU port) of the lan9303. The lan9303 acts as the phy for this ethernet port and I want to force the speed and duplex of the link to be 100 / full-duplex. >> + mdio { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + lan9303switch: switch@0 { >> + compatible = "smsc,lan9303-mdio"; >> + reg = <0>; >> + dsa,member = <0 0>; > >Redundant, please remove. > Okay. I can remove the "dsa,member = <0,0>;" line. >> + ethernet-ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + port@0 { >> + reg = <0>; >> + phy-mode = "rmii"; > >FWIW, RMII has a MAC mode and a PHY mode. Two RMII interfaces connected >in MAC mode to one another don't work. You'll have problems if you also >have an RMII PHY connected to one of the xMII ports, and you describe >phy-mode = "rmii" for both. There exists a "rev-rmii" phy-mode to denote >an RMII interface working in PHY mode. Wonder if you should be using >that here. > "rev-rmii" does make more sense. And yes, in this configuration the rmii port of the lan9303 is acting as the PHY end. >> + ethernet = <ðernet>; >> + fixed-link { >> + speed = <100>; >> + full-duplex; >> + }; >> + }; >> + port@1 { >> + reg = <1>; >> + max-speed = <100>; >> + label = "lan1"; >> + }; >> + port@2 { >> + reg = <2>; >> + max-speed = <100>; >> + label = "lan2"; >> + }; >> + }; >> + }; >> + }; >> + }; >> + >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + >> + // Ethernet switch connected via i2c to the host >> + ethernet { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + phy-mode = "rmii"; >> + speed = <100>; >> + fixed-link { >> + full-duplex; >> + }; >> + }; > >No need for this node. > Without this, what does the port0 entry below have to point to? How do you establish the device tree linkage between the ethenet MAC and the rev-rmii PHY it connects to? >> + >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + lan9303: switch@1a { >> + compatible = "smsc,lan9303-i2c"; >> + reg = <0x1a>; >> + ethernet-ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + port@0 { >> + reg = <0>; >> + phy-mode = "rmii"; >> + ethernet = <ðernet>; >> + fixed-link { >> + speed = <100>; >> + full-duplex; >> + }; >> + }; >> + port@1 { >> + reg = <1>; >> + max-speed = <100>; >> + label = "lan1"; >> + }; >> + port@2 { >> + reg = <2>; >> + max-speed = <100>; >> + label = "lan2"; >> + }; >> + }; >> + }; >> + }; >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 5d58b55c5ae5..89055ff2838a 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -13386,6 +13386,14 @@ L: netdev@vger.kernel.org >> S: Maintained >> F: drivers/net/ethernet/microchip/lan743x_* >> >> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER >> +M: Jerry Ray <jerry.ray@microchip.com> >> +M: UNGLinuxDriver@microchip.com >> +L: netdev@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml >> +F: drivers/net/dsa/lan9303* >> + > >Separate patch please? Changes to the MAINTAINERS file get applied to >the "net" tree. > >> MICROCHIP LAN966X ETHERNET DRIVER >> M: Horatiu Vultur <horatiu.vultur@microchip.com> >> M: UNGLinuxDriver@microchip.com >> -- >> 2.25.1 >> > Regards, Jerry.
On Mon, Oct 17, 2022 at 08:00:13PM +0000, Jerry.Ray@microchip.com wrote: > >> --- > >> v3->v4: > >> - Addressed v3 community feedback > > > >More specifically? > > > > - Old lan9303.txt file is totally removed rather than containing text that > redirects the user to the microchip,lan9303.yaml source file. > - Drop "Tree Bindings" from title > - Drop quotes from dsa.yaml reference line. > - Modified the compatible second enum to include a second string. > (( I now realize this is not what was being asked for and have made > it a single enum with 4 items, removing the oneOf. )) > - Drop "gpio specifier for a" in reset-gpois description > - added a default: property to the reset-duration item and set it to 200. > - Drop "0" from the ethernet name. Split the MDIO and I2C examples into > two so that the number is no longer needed. > - Placed the reg property to be directly following the compatible string > in the mdio node. Please carry the change log for v3->v4 also for future versions. > >> +examples: > >> + - | > >> + #include <dt-bindings/gpio/gpio.h> > >> + > >> + // Ethernet switch connected via mdio to the host > >> + ethernet { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + phy-handle = <&lan9303switch>; > >> + phy-mode = "rmii"; > >> + fixed-link { > >> + speed = <100>; > >> + full-duplex; > >> + }; > > > >I see the phy-handle to the switch is inherited from the .txt dt-binding, > >but I don't understand it. The switch is an mdio_device, not a phy_device, > >so what will this do? > > > >Also, any reasonable host driver will error out if it finds a phy-handle > >and a fixed-link in its OF node. So one of phy-handle or fixed-link must > >be dropped, they are bogus. > > > >Even better, just stick to the mdio node as root and drop the DSA master > >OF node, like other DSA dt-binding examples do. You can have dangling > >phandles, so "ethernet = <ðernet>" below is not an issue. > > > > I can remove the phy-handle, but I'm trying to establish the link between > this ethernet port and port0 (the CPU port) of the lan9303. The lan9303 > acts as the phy for this ethernet port and I want to force the speed and > duplex of the link to be 100 / full-duplex. If the lan9303 acts as the PHY, then what do you need to force the speed and duplex for? PHYs have a standard MDIO register set which gives you that information. I can understand a switch acting as a PHY towards Linux if you want to hide the fact that it's a switch, and pretend it's just a regular port going to the outside world (and maybe the switch is self-managed via a microcontroller or something). But what purpose does this serve when Linux is already in control of both ends of the link? And furthermore, why would the MDIO-managed switch have a phy-handle towards it, but the I2C managed switch not? If the host Ethernet controller can tolerate not knowing the link state and being forced to a given speed/duplex when the switch is I2C controlled, why can it not also tolerate being forced when the switch has registers accessed in MDIO mode? Lastly, when you have a phy-handle towards the switch, there will run 2 driver instances in parallel which will access the same hardware. What PHY driver will phylib use for the RevMII/RevRMII emulated register map of the CPU port? Will the registers accessed by the PHY driver collide in any way with the registers accessed by the DSA driver? What if paged MDIO access is used; how is synchronization between the 2 drivers handled? > >> + #include <dt-bindings/gpio/gpio.h> > >> + > >> + // Ethernet switch connected via i2c to the host > >> + ethernet { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + phy-mode = "rmii"; > >> + speed = <100>; > >> + fixed-link { > >> + full-duplex; > >> + }; > >> + }; > > > >No need for this node. > > > > Without this, what does the port0 entry below have to point to? > How do you establish the device tree linkage between the ethenet > MAC and the rev-rmii PHY it connects to? To repeat myself, the ethernet = <ðernet> phandle in port@0 can be broken (not point to anything) in the dt-schema examples. Same as for interrupt-parent, gpios, clocks, etc etc. Check out any .example.dts generated by "make dt_binding_check", it has this at the top: /plugin/; // silence any missing phandle references
On 17/10/2022 14:33, Jerry.Ray@microchip.com wrote: >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + reset-gpios: >>> + description: Optional reset line >>> + maxItems: 1 >>> + >>> + reset-duration: >>> + description: Reset duration in milliseconds >>> + default: 200 >> >> This is a friendly reminder during the review process. >> >> It seems my previous comments were not fully addressed. Maybe my >> 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. >> > > I am documenting "what is" rather than what I think it should be. I > would prefer there be a "-ms" suffix on the name, but that was not > what was in the pre-existing code. > > I added the "default: 200" line and can add a "maxItems: 1", but begin > getting errors when I attempt to further define this field as a > uint32 type or anything like that. There are no errors after adding proper type. However I cannot help you for some unspecified code with unspecified warnings. > > And no, I'm not getting any warnings or errors from the dt_bindings_check. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt deleted file mode 100644 index 46a732087f5c..000000000000 --- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt +++ /dev/null @@ -1,100 +0,0 @@ -SMSC/MicroChip LAN9303 three port ethernet switch -------------------------------------------------- - -Required properties: - -- compatible: should be - - "smsc,lan9303-i2c" for I2C managed mode - or - - "smsc,lan9303-mdio" for mdio managed mode - -Optional properties: - -- reset-gpios: GPIO to be used to reset the whole device -- reset-duration: reset duration in milliseconds, defaults to 200 ms - -Subnodes: - -The integrated switch subnode should be specified according to the binding -described in dsa/dsa.txt. The CPU port of this switch is always port 0. - -Note: always use 'reg = <0/1/2>;' for the three DSA ports, even if the device is -configured to use 1/2/3 instead. This hardware configuration will be -auto-detected and mapped accordingly. - -Example: - -I2C managed mode: - - master: masterdevice@X { - - fixed-link { /* RMII fixed link to LAN9303 */ - speed = <100>; - full-duplex; - }; - }; - - switch: switch@a { - compatible = "smsc,lan9303-i2c"; - reg = <0xa>; - reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>; - reset-duration = <200>; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - port@0 { /* RMII fixed link to master */ - reg = <0>; - ethernet = <&master>; - }; - - port@1 { /* external port 1 */ - reg = <1>; - label = "lan1"; - }; - - port@2 { /* external port 2 */ - reg = <2>; - label = "lan2"; - }; - }; - }; - -MDIO managed mode: - - master: masterdevice@X { - phy-handle = <&switch>; - - mdio { - #address-cells = <1>; - #size-cells = <0>; - - switch: switch-phy@0 { - compatible = "smsc,lan9303-mdio"; - reg = <0>; - reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>; - reset-duration = <100>; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - port@0 { - reg = <0>; - ethernet = <&master>; - }; - - port@1 { /* external port 1 */ - reg = <1>; - label = "lan1"; - }; - - port@2 { /* external port 2 */ - reg = <2>; - label = "lan2"; - }; - }; - }; - }; - }; diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml new file mode 100644 index 000000000000..ca6cbe83ba75 --- /dev/null +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml @@ -0,0 +1,134 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/dsa/microchip,lan9303.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LAN9303 Ethernet Switch Series + +allOf: + - $ref: dsa.yaml# + +maintainers: + - UNGLinuxDriver@microchip.com + +properties: + compatible: + oneOf: + - enum: + - smsc,lan9303-mdio + - microchip,lan9354-mdio + - enum: + - smsc,lan9303-i2c + - microchip,lan9354-i2c + + reg: + maxItems: 1 + + reset-gpios: + description: Optional reset line + maxItems: 1 + + reset-duration: + description: Reset duration in milliseconds + default: 200 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + // Ethernet switch connected via mdio to the host + ethernet { + #address-cells = <1>; + #size-cells = <0>; + phy-handle = <&lan9303switch>; + phy-mode = "rmii"; + fixed-link { + speed = <100>; + full-duplex; + }; + mdio { + #address-cells = <1>; + #size-cells = <0>; + lan9303switch: switch@0 { + compatible = "smsc,lan9303-mdio"; + reg = <0>; + dsa,member = <0 0>; + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + phy-mode = "rmii"; + ethernet = <ðernet>; + fixed-link { + speed = <100>; + full-duplex; + }; + }; + port@1 { + reg = <1>; + max-speed = <100>; + label = "lan1"; + }; + port@2 { + reg = <2>; + max-speed = <100>; + label = "lan2"; + }; + }; + }; + }; + }; + + - | + #include <dt-bindings/gpio/gpio.h> + + // Ethernet switch connected via i2c to the host + ethernet { + #address-cells = <1>; + #size-cells = <0>; + phy-mode = "rmii"; + fixed-link { + speed = <100>; + full-duplex; + }; + }; + + i2c { + #address-cells = <1>; + #size-cells = <0>; + lan9303: switch@1a { + compatible = "smsc,lan9303-i2c"; + reg = <0x1a>; + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + phy-mode = "rmii"; + ethernet = <ðernet>; + fixed-link { + speed = <100>; + full-duplex; + }; + }; + port@1 { + reg = <1>; + max-speed = <100>; + label = "lan1"; + }; + port@2 { + reg = <2>; + max-speed = <100>; + label = "lan2"; + }; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 5d58b55c5ae5..89055ff2838a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13386,6 +13386,14 @@ L: netdev@vger.kernel.org S: Maintained F: drivers/net/ethernet/microchip/lan743x_* +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER +M: Jerry Ray <jerry.ray@microchip.com> +M: UNGLinuxDriver@microchip.com +L: netdev@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml +F: drivers/net/dsa/lan9303* + MICROCHIP LAN966X ETHERNET DRIVER M: Horatiu Vultur <horatiu.vultur@microchip.com> M: UNGLinuxDriver@microchip.com
Adding the dt binding yaml for the lan9303 3-port ethernet switch. The microchip lan9354 3-port ethernet switch will also use the same binding. Signed-off-by: Jerry Ray <jerry.ray@microchip.com> --- v3->v4: - Addressed v3 community feedback v2->v3: - removed cpu labels - now patching against latest net-next v1->v2: - fixed dt_binding_check warning - added max-speed setting on the switches 10/100 ports. --- .../devicetree/bindings/net/dsa/lan9303.txt | 100 ------------- .../bindings/net/dsa/microchip,lan9303.yaml | 134 ++++++++++++++++++ MAINTAINERS | 8 ++ 3 files changed, 142 insertions(+), 100 deletions(-) delete mode 100644 Documentation/devicetree/bindings/net/dsa/lan9303.txt create mode 100644 Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml