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 <[email protected]>
---
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:
+ - [email protected]
+
+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: [email protected]
S: Maintained
F: drivers/net/ethernet/microchip/lan743x_*
+MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER
+M: Jerry Ray <[email protected]>
+M: [email protected]
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml
+F: drivers/net/dsa/lan9303*
+
MICROCHIP LAN966X ETHERNET DRIVER
M: Horatiu Vultur <[email protected]>
M: [email protected]
--
2.25.1
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 <[email protected]>
> ---
> 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:
> + - [email protected]
> +
> +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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
>> ---
>> 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: [email protected]
> S: Maintained
> F: drivers/net/ethernet/microchip/lan743x_*
>
> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER
> +M: Jerry Ray <[email protected]>
> +M: [email protected]
> +L: [email protected]
> +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 <[email protected]>
> M: [email protected]
> --
> 2.25.1
>
On 09/10/2022 00:56, Vladimir Oltean wrote:
>>
>> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER
>> +M: Jerry Ray <[email protected]>
>> +M: [email protected]
>> +L: [email protected]
>> +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 <[email protected]>
> >> +M: [email protected]
> >> +L: [email protected]
> >> +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 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 <[email protected]>
> >>>> +M: [email protected]
> >>>> +L: [email protected]
> >>>> +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.
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 <[email protected]>
>>>> +M: [email protected]
>>>> +L: [email protected]
>>>> +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
>> +
>> + 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 <[email protected]>
>> >>>> +M: [email protected]
>> >>>> +L: [email protected]
>> >>>> +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, 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.
On Mon, Oct 17, 2022 at 06:51:57PM +0000, [email protected] 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 <[email protected]>
> >> >>>> +M: [email protected]
> >> >>>> +L: [email protected]
> >> >>>> +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").
>> ---
>> 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: [email protected]
>> S: Maintained
>> F: drivers/net/ethernet/microchip/lan743x_*
>>
>> +MICROCHIP LAN9303/LAN9354 ETHERNET SWITCH DRIVER
>> +M: Jerry Ray <[email protected]>
>> +M: [email protected]
>> +L: [email protected]
>> +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 <[email protected]>
>> M: [email protected]
>> --
>> 2.25.1
>>
>
Regards,
Jerry.
On Mon, Oct 17, 2022 at 08:00:13PM +0000, [email protected] 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, [email protected] 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