| Message ID | 20220310145200.3645763-3-ioana.ciornei@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dpaa2-mac: add support for changing the protocol at runtime | expand |
| Context | Check | Description |
|---|---|---|
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
On 10/03/2022 15:51, Ioana Ciornei wrote: > Describe the "fsl,lynx-28g" compatible used by the Lynx 28G SerDes PHY > driver on Layerscape based SoCs. The message is a bit misleading, because it suggests you add only compatible to existing bindings. Instead please look at the git log how people usually describe it in subject and message. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > Changes in v2: > - none > Changes in v3: > - 2/8: fix 'make dt_binding_check' errors > > .../devicetree/bindings/phy/fsl,lynx-28g.yaml | 98 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > > diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > new file mode 100644 > index 000000000000..e98339ec83a7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml > @@ -0,0 +1,98 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/fsl,lynx-28g.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Freescale Lynx 28G SerDes PHY binding > + > +maintainers: > + - Ioana Ciornei <ioana.ciornei@nxp.com> > + > +properties: > + compatible: > + enum: > + - fsl,lynx-28g > + > + reg: > + maxItems: 1 > + > + "#phy-cells": > + const: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > +required: > + - compatible > + - reg > + - "#phy-cells" > + - "#address-cells" > + - "#size-cells" > + > +patternProperties: > + '^phy@[0-9a-f]$': > + type: object > + properties: > + reg: > + description: > + Number of the SerDes lane. > + minimum: 0 > + maximum: 7 > + > + "#phy-cells": > + const: 0 Why do you need all these children? You just enumerated them, without statuses, resources or any properties. This should be rather just index of lynx-28g phy. > + > + additionalProperties: false > + > +additionalProperties: false > + > +examples: > + - | > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + serdes_1: serdes_phy@1ea0000 { node name just "phy" > + compatible = "fsl,lynx-28g"; > + reg = <0x0 0x1ea0000 0x0 0x1e30>; > + #address-cells = <1>; > + #size-cells = <0>; > + #phy-cells = <1>; > + > + serdes1_lane_a: phy@0 { > + reg = <0>; > + #phy-cells = <0>; > + }; Best regards, Krzysztof
On Thu, Mar 10, 2022 at 05:47:31PM +0100, Krzysztof Kozlowski wrote: > On 10/03/2022 15:51, Ioana Ciornei wrote: > > Describe the "fsl,lynx-28g" compatible used by the Lynx 28G SerDes PHY > > driver on Layerscape based SoCs. > > The message is a bit misleading, because it suggests you add only > compatible to existing bindings. Instead please look at the git log how > people usually describe it in subject and message. Sure, I can change the title and commit message. > > +patternProperties: > > + '^phy@[0-9a-f]$': > > + type: object > > + properties: > > + reg: > > + description: > > + Number of the SerDes lane. > > + minimum: 0 > > + maximum: 7 > > + > > + "#phy-cells": > > + const: 0 > > Why do you need all these children? You just enumerated them, without > statuses, resources or any properties. This should be rather just index > of lynx-28g phy. I am just describing each lane of the SerDes block so that each ethernet dts node references it directly. Since I am new to the generic PHY infrastructure I was using the COMPHY for the Marvell MVEBU SoCs (phy-mvebu-comphy.txt) as a loose example. Each lane there is described as a different child node as well. The only difference from the COMPHY is that Lynx 28G does not need #phy-cells = <1> to reference the input port, we just use '#phy-cells = <0>' on each lane. What is wrong with this approach? Or better, is there an easier way to do this? > > > + > > + additionalProperties: false > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + serdes_1: serdes_phy@1ea0000 { > > node name just "phy" Sure. Ioana
On Thu, Mar 10, 2022 at 05:47:31PM +0100, Krzysztof Kozlowski wrote: > > +patternProperties: > > + '^phy@[0-9a-f]$': > > + type: object > > + properties: > > + reg: > > + description: > > + Number of the SerDes lane. > > + minimum: 0 > > + maximum: 7 > > + > > + "#phy-cells": > > + const: 0 > > Why do you need all these children? You just enumerated them, without > statuses, resources or any properties. This should be rather just index > of lynx-28g phy. There is good reason why the Marvell driver does it this way, and that is because there are shared registers amongst all the comphys on the SoC. Where that isn't the case, and there is no other reason, I would suggest creating multiple phy modes, one per physical PHY in DT, giving their address would be a saner approach. That way, the driver isn't locked in to a model of "we have N PHYs which are spaced by such-and-such apart", and you don't have this "maximum: 7" thing above either.
On Thu, Mar 10, 2022 at 05:58:07PM +0000, Russell King (Oracle) wrote: > On Thu, Mar 10, 2022 at 05:47:31PM +0100, Krzysztof Kozlowski wrote: > > > +patternProperties: > > > + '^phy@[0-9a-f]$': > > > + type: object > > > + properties: > > > + reg: > > > + description: > > > + Number of the SerDes lane. > > > + minimum: 0 > > > + maximum: 7 > > > + > > > + "#phy-cells": > > > + const: 0 > > > > Why do you need all these children? You just enumerated them, without > > statuses, resources or any properties. This should be rather just index > > of lynx-28g phy. > > There is good reason why the Marvell driver does it this way, and that > is because there are shared registers amongst all the comphys on the > SoC. > The Lynx SerDes block also has shared registers between the lanes as well as per lane registers. For example, I can configure the PLL to be used, the equalization parameters etc by using per lane registers but the protocol registers are shared among all the lanes. > Where that isn't the case, and there is no other reason, I would suggest > creating multiple phy modes, I suppose here you intended 'multiple phy nodes', right? > one per physical PHY in DT, giving their > address would be a saner approach. That way, the driver isn't locked > in to a model of "we have N PHYs which are spaced by such-and-such > apart", and you don't have this "maximum: 7" thing above either. > I don't think the model of separate driver instances per lane is applicable here. Ioana
On 10/03/2022 18:32, Ioana Ciornei wrote: > On Thu, Mar 10, 2022 at 05:47:31PM +0100, Krzysztof Kozlowski wrote: >> On 10/03/2022 15:51, Ioana Ciornei wrote: >>> Describe the "fsl,lynx-28g" compatible used by the Lynx 28G SerDes PHY >>> driver on Layerscape based SoCs. >> >> The message is a bit misleading, because it suggests you add only >> compatible to existing bindings. Instead please look at the git log how >> people usually describe it in subject and message. > > Sure, I can change the title and commit message. > >>> +patternProperties: >>> + '^phy@[0-9a-f]$': >>> + type: object >>> + properties: >>> + reg: >>> + description: >>> + Number of the SerDes lane. >>> + minimum: 0 >>> + maximum: 7 >>> + >>> + "#phy-cells": >>> + const: 0 >> >> Why do you need all these children? You just enumerated them, without >> statuses, resources or any properties. This should be rather just index >> of lynx-28g phy. > > I am just describing each lane of the SerDes block so that each ethernet > dts node references it directly. Instead, phy user should reference phy device node and phy ID. Just like we do for other providers (everything with #xxxxx-cells). > Since I am new to the generic PHY infrastructure I was using the COMPHY > for the Marvell MVEBU SoCs (phy-mvebu-comphy.txt) as a loose example. I don't know it but it might not be the best example... Just because we have already some solution it does not mean it is good. :) > Each lane there is described as a different child node as well. The only > difference from the COMPHY is that Lynx 28G does not need #phy-cells = > <1> to reference the input port, we just use '#phy-cells = <0>' on each > lane. > > What is wrong with this approach? Or better, is there an easier way to > do this? Because the nodes look artificial. It looks like you have nodes only differentiate by index. As I said before - there are no other properties in these nodes. Imagine now a clock provider with 500 clocks like this... The easier approach, especially since you have a shared registers, is to use phy-cells = 1, without artificial nodes just to pass the index. It would be entirely different if you actually had any properties in the children. IOW, if these were actually some blocks with their own characteristics and programming model. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml new file mode 100644 index 000000000000..e98339ec83a7 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml @@ -0,0 +1,98 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/fsl,lynx-28g.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Freescale Lynx 28G SerDes PHY binding + +maintainers: + - Ioana Ciornei <ioana.ciornei@nxp.com> + +properties: + compatible: + enum: + - fsl,lynx-28g + + reg: + maxItems: 1 + + "#phy-cells": + const: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +required: + - compatible + - reg + - "#phy-cells" + - "#address-cells" + - "#size-cells" + +patternProperties: + '^phy@[0-9a-f]$': + type: object + properties: + reg: + description: + Number of the SerDes lane. + minimum: 0 + maximum: 7 + + "#phy-cells": + const: 0 + + additionalProperties: false + +additionalProperties: false + +examples: + - | + soc { + #address-cells = <2>; + #size-cells = <2>; + serdes_1: serdes_phy@1ea0000 { + compatible = "fsl,lynx-28g"; + reg = <0x0 0x1ea0000 0x0 0x1e30>; + #address-cells = <1>; + #size-cells = <0>; + #phy-cells = <1>; + + serdes1_lane_a: phy@0 { + reg = <0>; + #phy-cells = <0>; + }; + serdes1_lane_b: phy@1 { + reg = <1>; + #phy-cells = <0>; + }; + serdes1_lane_c: phy@2 { + reg = <2>; + #phy-cells = <0>; + }; + serdes1_lane_d: phy@3 { + reg = <3>; + #phy-cells = <0>; + }; + serdes1_lane_e: phy@4 { + reg = <4>; + #phy-cells = <0>; + }; + serdes1_lane_f: phy@5 { + reg = <5>; + #phy-cells = <0>; + }; + serdes1_lane_g: phy@6 { + reg = <6>; + #phy-cells = <0>; + }; + serdes1_lane_h: phy@7 { + reg = <7>; + #phy-cells = <0>; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 383c4754096e..15670690527b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11340,6 +11340,7 @@ LYNX 28G SERDES PHY DRIVER M: Ioana Ciornei <ioana.ciornei@nxp.com> L: netdev@vger.kernel.org S: Supported +F: Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml F: drivers/phy/freescale/phy-fsl-lynx-28g.c LYNX PCS MODULE
Describe the "fsl,lynx-28g" compatible used by the Lynx 28G SerDes PHY driver on Layerscape based SoCs. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- Changes in v2: - none Changes in v3: - 2/8: fix 'make dt_binding_check' errors .../devicetree/bindings/phy/fsl,lynx-28g.yaml | 98 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-28g.yaml