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 |
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 |
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
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
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
> > > - 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
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.
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
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 --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>; + }; + }; + };
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