| Message ID | 20241113-sparx5-lan969x-switch-driver-4-v2-8-0db98ac096d1@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan969x: add RGMII support | expand |
On Wed, Nov 13, 2024 at 10:11:16PM +0100, Daniel Machon wrote: > The lan969x switch device supports two RGMII port interfaces that can be > configured for MAC level rx and tx delays. > > Document two new properties {rx,tx}-internal-delay-ps. Make them > required properties, if the phy-mode is one of: rgmii, rgmii_id, > rgmii-rxid or rgmii-txid. Also specify accepted values. > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > --- > .../bindings/net/microchip,sparx5-switch.yaml | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > index dedfad526666..a3f2b70c5c77 100644 > --- a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > +++ b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > @@ -129,6 +129,26 @@ properties: > minimum: 0 > maximum: 383 > > + allOf: > + - if: > + properties: > + phy-mode: > + contains: > + enum: > + - rgmii > + - rgmii-rxid > + - rgmii-txid > + - rgmii-id > + then: > + properties: > + rx-internal-delay-ps: > + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] > + tx-internal-delay-ps: > + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] Properties should be define at the top level and constrained in the if/then parts. Please move the property definitions out, and just leave the required: bit here. > + required: > + - rx-internal-delay-ps > + - tx-internal-delay-ps You've got no else, so these properties are valid even for !rgmii? > + > required: > - reg > - phys Additionally, please move the conditional bits below the required property list. Cheers, Conor.
On Wed, Nov 13, 2024 at 10:11:16PM +0100, Daniel Machon wrote: > The lan969x switch device supports two RGMII port interfaces that can be > configured for MAC level rx and tx delays. > > Document two new properties {rx,tx}-internal-delay-ps. Make them > required properties, if the phy-mode is one of: rgmii, rgmii_id, > rgmii-rxid or rgmii-txid. Also specify accepted values. This is unusual if you look at other uses of {rt}x-internal-delay-ps. It is generally an optional parameter, and states it defaults to 0 if missing, and is ignored by the driver if phy-mode is not an rgmii variant. Andrew
Hi Andrew, > > The lan969x switch device supports two RGMII port interfaces that can be > > configured for MAC level rx and tx delays. > > > > Document two new properties {rx,tx}-internal-delay-ps. Make them > > required properties, if the phy-mode is one of: rgmii, rgmii_id, > > rgmii-rxid or rgmii-txid. Also specify accepted values. > > This is unusual if you look at other uses of {rt}x-internal-delay-ps. > It is generally an optional parameter, and states it defaults to 0 if > missing, and is ignored by the driver if phy-mode is not an rgmii > variant. Is unusual bad? :-) I thought that requiring the properties would make misconfigurations (mismatching phy-modes and MAC delays) more obvious, as you were forced to specify exactly what combination you want in the DT. Maybe not. I can change it, no problem. /Daniel > > Andrew
Hi Conor, > > The lan969x switch device supports two RGMII port interfaces that can be > > configured for MAC level rx and tx delays. > > > > Document two new properties {rx,tx}-internal-delay-ps. Make them > > required properties, if the phy-mode is one of: rgmii, rgmii_id, > > rgmii-rxid or rgmii-txid. Also specify accepted values. > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > .../bindings/net/microchip,sparx5-switch.yaml | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > > index dedfad526666..a3f2b70c5c77 100644 > > --- a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > > +++ b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > > @@ -129,6 +129,26 @@ properties: > > minimum: 0 > > maximum: 383 > > > > + allOf: > > + - if: > > + properties: > > + phy-mode: > > + contains: > > + enum: > > + - rgmii > > + - rgmii-rxid > > + - rgmii-txid > > + - rgmii-id > > + then: > > + properties: > > + rx-internal-delay-ps: > > + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] > > + tx-internal-delay-ps: > > + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] > > Properties should be define at the top level and constrained in the > if/then parts. Please move the property definitions out, and just leave > the required: bit here. > > > + required: > > + - rx-internal-delay-ps > > + - tx-internal-delay-ps > > You've got no else, so these properties are valid even for !rgmii? > > > + > > required: > > - reg > > - phys > > Additionally, please move the conditional bits below the required > property list. > > Cheers, > Conor. I will be getting rid of the 'required' constraints in v3. What I hear you say, is that the two {rx,tx}-internal-delay-ps properties (incl. their enum values) should be moved out of the if/else and to the top-level - can you confirm this? Is specifying the values a property can take not considered a constraint? Thanks, Daniel
On Fri, Nov 15, 2024 at 09:22:37AM +0000, Daniel Machon wrote: > Hi Andrew, > > > > The lan969x switch device supports two RGMII port interfaces that can be > > > configured for MAC level rx and tx delays. > > > > > > Document two new properties {rx,tx}-internal-delay-ps. Make them > > > required properties, if the phy-mode is one of: rgmii, rgmii_id, > > > rgmii-rxid or rgmii-txid. Also specify accepted values. > > > > This is unusual if you look at other uses of {rt}x-internal-delay-ps. > > It is generally an optional parameter, and states it defaults to 0 if > > missing, and is ignored by the driver if phy-mode is not an rgmii > > variant. > > Is unusual bad? :-) Depends. Having a uniform usage is good, it causes less confusion. But strict enforcement also has its plus side. > I thought that requiring the properties would make > misconfigurations (mismatching phy-modes and MAC delays) more obvious, > as you were forced to specify exactly what combination you want in the > DT. Maybe not. I can change it, no problem. Do these ports only support RGMII? The general pattern is that ports supporting RGMII also support other modes, GMII, MII, rev-GMII, rev-MII etc. For these other modes RGMII delays are meaningless. The general pattern is that they are allowed in DT, but are just ignored. If the LAN969x ports only support RGMII, and you are enforcing the four RGMII modes in DT, you could also enforce the delays are present and only have all allowed values. But i would not have the enforcement any more strict than the other ports. Do you enforce the phy-modes for the ports with a PCS? Andrew
> > Hi Andrew, > > > > > > The lan969x switch device supports two RGMII port interfaces that can be > > > > configured for MAC level rx and tx delays. > > > > > > > > Document two new properties {rx,tx}-internal-delay-ps. Make them > > > > required properties, if the phy-mode is one of: rgmii, rgmii_id, > > > > rgmii-rxid or rgmii-txid. Also specify accepted values. > > > > > > This is unusual if you look at other uses of {rt}x-internal-delay-ps. > > > It is generally an optional parameter, and states it defaults to 0 if > > > missing, and is ignored by the driver if phy-mode is not an rgmii > > > variant. > > > > Is unusual bad? :-) > > Depends. Having a uniform usage is good, it causes less confusion. But > strict enforcement also has its plus side. > > > I thought that requiring the properties would make > > misconfigurations (mismatching phy-modes and MAC delays) more obvious, > > as you were forced to specify exactly what combination you want in the > > DT. Maybe not. I can change it, no problem. > > Do these ports only support RGMII? The general pattern is that ports > supporting RGMII also support other modes, GMII, MII, rev-GMII, > rev-MII etc. For these other modes RGMII delays are meaningless. The > general pattern is that they are allowed in DT, but are just ignored. RGMII and RMII. > > If the LAN969x ports only support RGMII, and you are enforcing the > four RGMII modes in DT, you could also enforce the delays are present > and only have all allowed values. But i would not have the enforcement > any more strict than the other ports. Do you enforce the phy-modes for > the ports with a PCS? No, we do not enforce that in the DT. For the PCS ports, you can specify whatever phy-mode in the DT, and if that phy-mode is not advertised in the driver, it will just be rejected. I decided to go ahead with v3 (which needs to be reposted when net-next opens), where the properties are not required. > > Andrew Thanks for your feedback. /Daniel
On Mon, Nov 18, 2024 at 10:50:25AM +0000, Daniel Machon wrote: > Hi Conor, > > > > The lan969x switch device supports two RGMII port interfaces that can be > > > configured for MAC level rx and tx delays. > > > > > > Document two new properties {rx,tx}-internal-delay-ps. Make them > > > required properties, if the phy-mode is one of: rgmii, rgmii_id, > > > rgmii-rxid or rgmii-txid. Also specify accepted values. > > > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > > --- > > > .../bindings/net/microchip,sparx5-switch.yaml | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > > > index dedfad526666..a3f2b70c5c77 100644 > > > --- a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > > > +++ b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml > > > @@ -129,6 +129,26 @@ properties: > > > minimum: 0 > > > maximum: 383 > > > > > > + allOf: > > > + - if: > > > + properties: > > > + phy-mode: > > > + contains: > > > + enum: > > > + - rgmii > > > + - rgmii-rxid > > > + - rgmii-txid > > > + - rgmii-id > > > + then: > > > + properties: > > > + rx-internal-delay-ps: > > > + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] > > > + tx-internal-delay-ps: > > > + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] > > > > Properties should be define at the top level and constrained in the > > if/then parts. Please move the property definitions out, and just leave > > the required: bit here. > > > > > + required: > > > + - rx-internal-delay-ps > > > + - tx-internal-delay-ps > > > > You've got no else, so these properties are valid even for !rgmii? > > > > > + > > > required: > > > - reg > > > - phys > > > > Additionally, please move the conditional bits below the required > > property list. > > > > Cheers, > > Conor. > > I will be getting rid of the 'required' constraints in v3. What I hear > you say, is that the two {rx,tx}-internal-delay-ps properties (incl. > their enum values) should be moved out of the if/else and to the > top-level - can you confirm this? > Is specifying the values > a property can take not considered a constraint? Actually, in this case the property isn't even defined (per ethernet-controller.yaml) if the phy-mode wasn't an rgmii one, so what you had here was probably fine. Ordinarily, that's not the case, so you'd have been setting constraints for only rgmii phy-modes and no constraints at all for non-rgmii phy-modes.
diff --git a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml index dedfad526666..a3f2b70c5c77 100644 --- a/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml +++ b/Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml @@ -129,6 +129,26 @@ properties: minimum: 0 maximum: 383 + allOf: + - if: + properties: + phy-mode: + contains: + enum: + - rgmii + - rgmii-rxid + - rgmii-txid + - rgmii-id + then: + properties: + rx-internal-delay-ps: + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] + tx-internal-delay-ps: + enum: [0, 1000, 1700, 2000, 2500, 3000, 3300] + required: + - rx-internal-delay-ps + - tx-internal-delay-ps + required: - reg - phys
The lan969x switch device supports two RGMII port interfaces that can be configured for MAC level rx and tx delays. Document two new properties {rx,tx}-internal-delay-ps. Make them required properties, if the phy-mode is one of: rgmii, rgmii_id, rgmii-rxid or rgmii-txid. Also specify accepted values. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- .../bindings/net/microchip,sparx5-switch.yaml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)