| Message ID | 20240812031114.3798487-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] dt-bindings: net: mdio: Add negative patten match for child node | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sun, Aug 11, 2024 at 11:11:14PM -0400, Frank Li wrote: > mdio.yaml wrong parser mdio controller's address instead phy's address when > mdio-mux exist. > > For example: > mdio-mux-emi1@54 { > compatible = "mdio-mux-mmioreg", "mdio-mux"; > > mdio@20 { > reg = <0x20>; > ^^^ This is mdio controller register > > ethernet-phy@2 { > reg = <0x2>; > ^^^ This phy's address > }; > }; > }; > > Only phy's address is limited to 31 because MDIO bus defination. nit: definition Also, in subject: patten -> pattern Flagged by checkpatch.pl --codespell > But CHECK_DTBS report below warning: > > arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dtb: mdio-mux-emi1@54: > mdio@20:reg:0:0: 32 is greater than the maximum of 31 > > The reason is that "mdio@20" match "patternProperties: '@[0-9a-f]+$'" in > mdio.yaml. > > Change to '^(?!mdio@).*@[0-9a-f]+$' to avoid match parent's mdio > controller's address. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> ...
On Sun, Aug 11, 2024 at 11:11:14PM -0400, Frank Li wrote: > mdio.yaml wrong parser mdio controller's address instead phy's address when > mdio-mux exist. > > For example: > mdio-mux-emi1@54 { > compatible = "mdio-mux-mmioreg", "mdio-mux"; > > mdio@20 { > reg = <0x20>; > ^^^ This is mdio controller register > > ethernet-phy@2 { > reg = <0x2>; > ^^^ This phy's address > }; > }; > }; I don't understand MDIO well enough to know the answer - does this actually solve the problem? It seems to me that the problem is that mdio.yaml is applied to the mdio-mux node because it matches the pattern "^mdio(@.*)?" that applies the binding based on node-names. If the properties in mdio.yaml do not apply to mdio muxes, then the binding should not be applied and the patch here is only treating a symptom rather than the actual problem. From a quick check, I don't see any of the mdio-mux-mmioreg nodes using the properties from mdio.yaml, so should the binding be applied to them at all? Cheers, Conor. FWIW, adding a $ after the ? in the pattern I linked would stop the binding being applied to the mdio-mux nodes, but if something like that were done, all mdio nodes would need to be checked to ensure they match the new pattern... > > Only phy's address is limited to 31 because MDIO bus defination. > > But CHECK_DTBS report below warning: > > arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dtb: mdio-mux-emi1@54: > mdio@20:reg:0:0: 32 is greater than the maximum of 31 > > The reason is that "mdio@20" match "patternProperties: '@[0-9a-f]+$'" in > mdio.yaml. > > Change to '^(?!mdio@).*@[0-9a-f]+$' to avoid match parent's mdio > controller's address. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Documentation/devicetree/bindings/net/mdio.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml > index a266ade918ca7..a7def3eb4674d 100644 > --- a/Documentation/devicetree/bindings/net/mdio.yaml > +++ b/Documentation/devicetree/bindings/net/mdio.yaml > @@ -59,7 +59,7 @@ properties: > type: boolean > > patternProperties: > - '@[0-9a-f]+$': > + '^(?!mdio@).*@[0-9a-f]+$': > type: object > > properties: > -- > 2.34.1 >
On Mon, Aug 12, 2024 at 05:44:50PM +0100, Conor Dooley wrote: > On Sun, Aug 11, 2024 at 11:11:14PM -0400, Frank Li wrote: > > mdio.yaml wrong parser mdio controller's address instead phy's address when > > mdio-mux exist. > > > > For example: > > mdio-mux-emi1@54 { > > compatible = "mdio-mux-mmioreg", "mdio-mux"; > > > > mdio@20 { > > reg = <0x20>; > > ^^^ This is mdio controller register > > > > ethernet-phy@2 { > > reg = <0x2>; > > ^^^ This phy's address > > }; > > }; > > }; > > I don't understand MDIO well enough to know the answer - does this > actually solve the problem? It seems to me that the problem is that > mdio.yaml is applied to the mdio-mux node because it matches the pattern > "^mdio(@.*)?" that applies the binding based on node-names. If the > properties in mdio.yaml do not apply to mdio muxes, then the binding > should not be applied and the patch here is only treating a symptom > rather than the actual problem. after I change above example 0x2 to 0x22, mdio.yaml can report exceed 31. Only issue should be only when "ethernet-phy" use node name "mdio". > > From a quick check, I don't see any of the mdio-mux-mmioreg nodes using > the properties from mdio.yaml, so should the binding be applied to them > at all? in mdio-mux-mmioreg.yaml line 18: allOf: - $ref: /schemas/net/mdio-mux.yaml# in mdio-mux.yaml patternProperties: '^mdio@[0-9a-f]+$': -> A $ref: mdio.yaml# In mdio.yaml properties: $nodename: pattern: "^mdio(@.*)? ^^^^^ mdio@20 Match condition A, it should correct. patternProperties: '@[0-9a-f]+$': ^^^^ mdio@20 also match this one. but I suppose it should search mdio@20's child node, such as ethernet-phy@2, but not. Frank > > Cheers, > Conor. > > > FWIW, adding a $ after the ? in the pattern I linked would stop the > binding being applied to the mdio-mux nodes, but if something like that > were done, all mdio nodes would need to be checked to ensure they match > the new pattern... > > > > > > Only phy's address is limited to 31 because MDIO bus defination. > > > > But CHECK_DTBS report below warning: > > > > arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dtb: mdio-mux-emi1@54: > > mdio@20:reg:0:0: 32 is greater than the maximum of 31 > > > > The reason is that "mdio@20" match "patternProperties: '@[0-9a-f]+$'" in > > mdio.yaml. > > > > Change to '^(?!mdio@).*@[0-9a-f]+$' to avoid match parent's mdio > > controller's address. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Documentation/devicetree/bindings/net/mdio.yaml | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml > > index a266ade918ca7..a7def3eb4674d 100644 > > --- a/Documentation/devicetree/bindings/net/mdio.yaml > > +++ b/Documentation/devicetree/bindings/net/mdio.yaml > > @@ -59,7 +59,7 @@ properties: > > type: boolean > > > > patternProperties: > > - '@[0-9a-f]+$': > > + '^(?!mdio@).*@[0-9a-f]+$': > > type: object > > > > properties: > > -- > > 2.34.1 > >
On Sun, Aug 11, 2024 at 11:11:14PM -0400, Frank Li wrote: > mdio.yaml wrong parser mdio controller's address instead phy's address when > mdio-mux exist. > > For example: > mdio-mux-emi1@54 { > compatible = "mdio-mux-mmioreg", "mdio-mux"; > > mdio@20 { > reg = <0x20>; > ^^^ This is mdio controller register > > ethernet-phy@2 { > reg = <0x2>; > ^^^ This phy's address > }; > }; > }; > > Only phy's address is limited to 31 because MDIO bus defination. > > But CHECK_DTBS report below warning: > > arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dtb: mdio-mux-emi1@54: > mdio@20:reg:0:0: 32 is greater than the maximum of 31 > > The reason is that "mdio@20" match "patternProperties: '@[0-9a-f]+$'" in > mdio.yaml. > > Change to '^(?!mdio@).*@[0-9a-f]+$' to avoid match parent's mdio > controller's address. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Documentation/devicetree/bindings/net/mdio.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml > index a266ade918ca7..a7def3eb4674d 100644 > --- a/Documentation/devicetree/bindings/net/mdio.yaml > +++ b/Documentation/devicetree/bindings/net/mdio.yaml > @@ -59,7 +59,7 @@ properties: > type: boolean > > patternProperties: > - '@[0-9a-f]+$': > + '^(?!mdio@).*@[0-9a-f]+$': This is at the wrong spot. The problem is up a level where the $nodename matched mdio-mux-emi1@54. I think what we want for the $nodename pattern is: '^mdio(-(bus|external))?(@.+|-([0-9]+))$' There's lots of pinctrl nodes named 'mdio...' we need to avoid and we aren't currently. I'd prefer not to support 'mdio-external', but there's already 1 documented case. I think the only node name fix we'd need with this is 'mdio-gpio' which should be just 'mdio' or 'mdio-N' like all other bitbanged implementations. Rob
On Mon, Aug 12, 2024 at 04:36:11PM -0600, Rob Herring wrote: > On Sun, Aug 11, 2024 at 11:11:14PM -0400, Frank Li wrote: > > mdio.yaml wrong parser mdio controller's address instead phy's address when > > mdio-mux exist. > > > > For example: > > mdio-mux-emi1@54 { > > compatible = "mdio-mux-mmioreg", "mdio-mux"; > > > > mdio@20 { > > reg = <0x20>; > > ^^^ This is mdio controller register > > > > ethernet-phy@2 { > > reg = <0x2>; > > ^^^ This phy's address > > }; > > }; > > }; > > > > Only phy's address is limited to 31 because MDIO bus defination. > > > > But CHECK_DTBS report below warning: > > > > arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dtb: mdio-mux-emi1@54: > > mdio@20:reg:0:0: 32 is greater than the maximum of 31 > > > > The reason is that "mdio@20" match "patternProperties: '@[0-9a-f]+$'" in > > mdio.yaml. > > > > Change to '^(?!mdio@).*@[0-9a-f]+$' to avoid match parent's mdio > > controller's address. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Documentation/devicetree/bindings/net/mdio.yaml | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml > > index a266ade918ca7..a7def3eb4674d 100644 > > --- a/Documentation/devicetree/bindings/net/mdio.yaml > > +++ b/Documentation/devicetree/bindings/net/mdio.yaml > > @@ -59,7 +59,7 @@ properties: > > type: boolean > > > > patternProperties: > > - '@[0-9a-f]+$': > > + '^(?!mdio@).*@[0-9a-f]+$': > > This is at the wrong spot. The problem is up a level where the $nodename > matched mdio-mux-emi1@54. > > I think what we want for the $nodename pattern is: > > '^mdio(-(bus|external))?(@.+|-([0-9]+))$' > > There's lots of pinctrl nodes named 'mdio...' we need to avoid and we > aren't currently. > > I'd prefer not to support 'mdio-external', but there's already 1 > documented case. I think the only node name fix we'd need with this is > 'mdio-gpio' which should be just 'mdio' or 'mdio-N' like all other > bitbanged implementations. --- a/Documentation/devicetree/bindings/net/mdio.yaml +++ b/Documentation/devicetree/bindings/net/mdio.yaml @@ -19,7 +19,7 @@ description: properties: $nodename: - pattern: "^mdio(@.*)?" + pattern: '^mdio(-(bus|external))?(@.+|-([0-9]+))$' You are right. Above patch can work. Frank > > Rob
diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml index a266ade918ca7..a7def3eb4674d 100644 --- a/Documentation/devicetree/bindings/net/mdio.yaml +++ b/Documentation/devicetree/bindings/net/mdio.yaml @@ -59,7 +59,7 @@ properties: type: boolean patternProperties: - '@[0-9a-f]+$': + '^(?!mdio@).*@[0-9a-f]+$': type: object properties:
mdio.yaml wrong parser mdio controller's address instead phy's address when mdio-mux exist. For example: mdio-mux-emi1@54 { compatible = "mdio-mux-mmioreg", "mdio-mux"; mdio@20 { reg = <0x20>; ^^^ This is mdio controller register ethernet-phy@2 { reg = <0x2>; ^^^ This phy's address }; }; }; Only phy's address is limited to 31 because MDIO bus defination. But CHECK_DTBS report below warning: arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dtb: mdio-mux-emi1@54: mdio@20:reg:0:0: 32 is greater than the maximum of 31 The reason is that "mdio@20" match "patternProperties: '@[0-9a-f]+$'" in mdio.yaml. Change to '^(?!mdio@).*@[0-9a-f]+$' to avoid match parent's mdio controller's address. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Documentation/devicetree/bindings/net/mdio.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)