diff mbox series

[v3,1/8] dt-bindings: net: add schema for ASIX USB Ethernet controllers

Message ID 20220215080937.2263111-1-o.rempel@pengutronix.de
State Superseded, archived
Headers show
Series [v3,1/8] dt-bindings: net: add schema for ASIX USB Ethernet controllers | expand

Checks

Context Check Description
robh/patch-applied success
robh/checkpatch warning total: 0 errors, 1 warnings, 68 lines checked
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Oleksij Rempel Feb. 15, 2022, 8:09 a.m. UTC 
Create schema for ASIX USB Ethernet controllers and import some of
currently supported USB IDs form drivers/net/usb/asix_devices.c

This devices are already used in some of DTs. So, this schema makes it official.
NOTE: there was no previously documented txt based DT binding for this
controllers.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../devicetree/bindings/net/asix,ax88178.yaml | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/asix,ax88178.yaml

Comments

Marc Kleine-Budde Feb. 15, 2022, 8:12 a.m. UTC | #1 
On 15.02.2022 09:09:34, Oleksij Rempel wrote:
> The node name of Ethernet controller should be "ethernet" instead of
> "usbether"
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  arch/arm/boot/dts/exynos4412-odroidu3.dts       | 4 ++--
>  arch/arm/boot/dts/exynos4412-odroidx.dts        | 8 ++++----
>  arch/arm/boot/dts/exynos5410-odroidxu.dts       | 4 ++--
>  arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts | 6 +++---
>  arch/arm/boot/dts/exynos5422-odroidxu3.dts      | 6 +++---
>  5 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos4412-odroidu3.dts b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> index efaf7533e84f..36c369c42b77 100644
> --- a/arch/arm/boot/dts/exynos4412-odroidu3.dts
> +++ b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> @@ -119,8 +119,8 @@ &ehci {
>  	phys = <&exynos_usbphy 2>, <&exynos_usbphy 3>;
>  	phy-names = "hsic0", "hsic1";
>  
> -	ethernet: usbether@2 {
> -		compatible = "usb0424,9730";
> +	ethernet: ethernet@2 {
> +		compatible = "usb424,9730";

The change of the compatible is not mentioned in the patch description.
Is this intentional?

Marc
Oleksij Rempel Feb. 15, 2022, 8:16 a.m. UTC | #2 
On Tue, Feb 15, 2022 at 09:12:40AM +0100, Marc Kleine-Budde wrote:
> On 15.02.2022 09:09:34, Oleksij Rempel wrote:
> > The node name of Ethernet controller should be "ethernet" instead of
> > "usbether"
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  arch/arm/boot/dts/exynos4412-odroidu3.dts       | 4 ++--
> >  arch/arm/boot/dts/exynos4412-odroidx.dts        | 8 ++++----
> >  arch/arm/boot/dts/exynos5410-odroidxu.dts       | 4 ++--
> >  arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts | 6 +++---
> >  arch/arm/boot/dts/exynos5422-odroidxu3.dts      | 6 +++---
> >  5 files changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/exynos4412-odroidu3.dts b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> > index efaf7533e84f..36c369c42b77 100644
> > --- a/arch/arm/boot/dts/exynos4412-odroidu3.dts
> > +++ b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> > @@ -119,8 +119,8 @@ &ehci {
> >  	phys = <&exynos_usbphy 2>, <&exynos_usbphy 3>;
> >  	phy-names = "hsic0", "hsic1";
> >  
> > -	ethernet: usbether@2 {
> > -		compatible = "usb0424,9730";
> > +	ethernet: ethernet@2 {
> > +		compatible = "usb424,9730";
> 
> The change of the compatible is not mentioned in the patch description.
> Is this intentional?

No, I forgot to mentione it. According to the USB schema 0 should be
removed. So, this compatible was incorrect as well. With leading zero
present yaml schema was not able to detect and validate this node.

Regards,
Oleksij
Krzysztof Kozlowski Feb. 15, 2022, 8:28 a.m. UTC | #3 
On 15/02/2022 09:09, Oleksij Rempel wrote:
> The node name of Ethernet controller should be "ethernet" instead of
> "usbether"

Missing full stop.
Please also mention why this should be "ethernet" (e.g. because Ethernet
dtschema requires it). This applies to other DTS patches as well.

Plus comments from Marc.

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  arch/arm/boot/dts/exynos4412-odroidu3.dts       | 4 ++--
>  arch/arm/boot/dts/exynos4412-odroidx.dts        | 8 ++++----
>  arch/arm/boot/dts/exynos5410-odroidxu.dts       | 4 ++--
>  arch/arm/boot/dts/exynos5422-odroidxu3-lite.dts | 6 +++---
>  arch/arm/boot/dts/exynos5422-odroidxu3.dts      | 6 +++---
>  5 files changed, 14 insertions(+), 14 deletions(-)
> 

Best regards,
Krzysztof
Andrew Lunn Feb. 15, 2022, 8:56 p.m. UTC | #4 
> > > -	ethernet: usbether@2 {
> > > -		compatible = "usb0424,9730";
> > > +	ethernet: ethernet@2 {
> > > +		compatible = "usb424,9730";
> > 
> > The change of the compatible is not mentioned in the patch description.
> > Is this intentional?
> 
> No, I forgot to mentione it. According to the USB schema 0 should be
> removed. So, this compatible was incorrect as well. With leading zero
> present yaml schema was not able to detect and validate this node.

Does the current code not actually care about a leading 0? It will
match with or without it? It would be good to mention that as well in
the commit message, otherwise somebody like me is going to ask if this
breaks backwards compatibility, since normally compatible is an exact
string match.

And i actually think this is the sort of change which should be as a
patch of its own. If this causes a regression, a git bisect would then
tell you if it is the change of usbether -> ethernet, or 0424 to
424. That is part of why we ask for lots of small changes.


       Andrew
Florian Fainelli Feb. 15, 2022, 9:01 p.m. UTC | #5 
On 2/15/22 12:09 AM, Oleksij Rempel wrote:
> It should be "ethernet@x" instead of "usbether@x"
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

This looks like, a quick grep on the u-boot source code seems to suggest
that only one file is assuming that 'usbether@1' is to be used as a node
name and the error message does not even match the code it is patching:

board/liebherr/xea/xea.c:
  #ifdef CONFIG_OF_BOARD_SETUP
  static int fdt_fixup_l2switch(void *blob)
  {
          u8 ethaddr[6];
          int ret;

          if (eth_env_get_enetaddr("ethaddr", ethaddr)) {
                  ret = fdt_find_and_setprop(blob,

"/ahb@80080000/switch@800f0000",
                                             "local-mac-address",
ethaddr, 6, 1);
                  if (ret < 0)
                          printf("%s: can't find usbether@1 node: %d\n",
                                 __func__, ret);
          }

          return 0;
  }

I will wait for the other maintainers on the other patches to provide
some feedback, but if all is well, will apply this one soon.
Oleksij Rempel Feb. 16, 2022, 6:15 a.m. UTC | #6 
On Tue, Feb 15, 2022 at 09:56:50PM +0100, Andrew Lunn wrote:
> > > > -	ethernet: usbether@2 {
> > > > -		compatible = "usb0424,9730";
> > > > +	ethernet: ethernet@2 {
> > > > +		compatible = "usb424,9730";
> > > 
> > > The change of the compatible is not mentioned in the patch description.
> > > Is this intentional?
> > 
> > No, I forgot to mentione it. According to the USB schema 0 should be
> > removed. So, this compatible was incorrect as well. With leading zero
> > present yaml schema was not able to detect and validate this node.
> 
> Does the current code not actually care about a leading 0? It will
> match with or without it? It would be good to mention that as well in
> the commit message, otherwise somebody like me is going to ask if this
> breaks backwards compatibility, since normally compatible is an exact
> string match.

Current kernel code do not care about exact this compatibles. There is
no driver matching against it. The USB Ethernet driver will take the
node provided by the USB core drivers without validating the compatible
against USB ID.
See:
drivers/usb/core/of.c
drivers/usb/core/message.c:2093

On other hand, DT validations tools do care about it and this nodes was
not detected automatically. I found it accidentally by grepping the
sources.

> And i actually think this is the sort of change which should be as a
> patch of its own. If this causes a regression, a git bisect would then
> tell you if it is the change of usbether -> ethernet, or 0424 to
> 424. That is part of why we ask for lots of small changes.

Sounds good, I'll update it.

Regards,
Oleksij
Oleksij Rempel Feb. 16, 2022, 7:16 a.m. UTC | #7 
On Tue, Feb 15, 2022 at 01:01:06PM -0800, Florian Fainelli wrote:
> On 2/15/22 12:09 AM, Oleksij Rempel wrote:
> > It should be "ethernet@x" instead of "usbether@x"
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> This looks like, a quick grep on the u-boot source code seems to suggest
> that only one file is assuming that 'usbether@1' is to be used as a node
> name and the error message does not even match the code it is patching:
> 
> board/liebherr/xea/xea.c:
>   #ifdef CONFIG_OF_BOARD_SETUP
>   static int fdt_fixup_l2switch(void *blob)
>   {
>           u8 ethaddr[6];
>           int ret;
> 
>           if (eth_env_get_enetaddr("ethaddr", ethaddr)) {
>                   ret = fdt_find_and_setprop(blob,
> 
> "/ahb@80080000/switch@800f0000",
>                                              "local-mac-address",
> ethaddr, 6, 1);
>                   if (ret < 0)
>                           printf("%s: can't find usbether@1 node: %d\n",
>                                  __func__, ret);
>           }

\o/ :)

>           return 0;
>   }
> 
> I will wait for the other maintainers on the other patches to provide
> some feedback, but if all is well, will apply this one soon.

Full path fdt matching has proven to be not stable enough. Especially on
chips with early DT adaptation like iMX. It is better to use aliases
where possible. 

Regards,
Oleksij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/asix,ax88178.yaml b/Documentation/devicetree/bindings/net/asix,ax88178.yaml
new file mode 100644
index 000000000000..1af52358de4c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/asix,ax88178.yaml
@@ -0,0 +1,68 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/asix,ax88178.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: The device tree bindings for the USB Ethernet controllers
+
+maintainers:
+  - Oleksij Rempel <o.rempel@pengutronix.de>
+
+description: |
+  Device tree properties for hard wired USB Ethernet devices.
+
+allOf:
+  - $ref: ethernet-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - usbb95,1720   # ASIX AX88172
+          - usbb95,172a   # ASIX AX88172A
+          - usbb95,1780   # ASIX AX88178
+          - usbb95,7720   # ASIX AX88772
+          - usbb95,772a   # ASIX AX88772A
+          - usbb95,772b   # ASIX AX88772B
+          - usbb95,7e2b   # ASIX AX88772B
+
+  reg: true
+  local-mac-address: true
+  mac-address: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    usb {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet@1 {
+            compatible = "usbb95,7e2b";
+            reg = <1>;
+            local-mac-address = [00 00 00 00 00 00];
+        };
+    };
+  - |
+    usb {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        usb1@1 {
+            compatible = "usb1234,5678";
+            reg = <1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            ethernet@1 {
+               compatible = "usbb95,772b";
+               reg = <1>;
+            };
+        };
+    };