| Message ID | 20211214233432.22580-3-kabel@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Common PHY to YAML + tx-amplitude property | expand |
On Wed, Dec 15, 2021 at 12:34:32AM +0100, Marek Behún wrote: > Common PHYs often have the possibility to specify peak-to-peak voltage > on the differential pair - the default voltage sometimes needs to be > changed for a particular board. Hi Marek Common PHYs are not the only user of this. Ethernet PHYs can also use it, as well as SERDESes embedded within Ethernet switches. That is why i suggested these properties go into something like serdes.yaml. That can then be included into Common PHY, Ethernet PHYs, switch drivers etc. Please could you make such a split? Andrew
On Wed, 15 Dec 2021 11:55:00 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Dec 15, 2021 at 12:34:32AM +0100, Marek Behún wrote: > > Common PHYs often have the possibility to specify peak-to-peak voltage > > on the differential pair - the default voltage sometimes needs to be > > changed for a particular board. > > Hi Marek > > Common PHYs are not the only user of this. Ethernet PHYs can also use > it, as well as SERDESes embedded within Ethernet switches. > > That is why i suggested these properties go into something like > serdes.yaml. That can then be included into Common PHY, Ethernet PHYs, > switch drivers etc. > > Please could you make such a split? > > Andrew Hi Andrew, and where (into which directory) should this serdes.yaml file go? My idea was to put the properties into common PHY and then refer to them from other places, so for example this would be put into ethernet PHY binding: serdes-tx-amplitude-microvolt: $ref: '/schemas/phy/phy.yaml#/properties/tx-amplitude-microvolt' Marek
> Hi Andrew, > > and where (into which directory) should this serdes.yaml file go? > > My idea was to put the properties into common PHY and then refer to > them from other places, so for example this would be put into ethernet > PHY binding: > > serdes-tx-amplitude-microvolt: > $ref: '/schemas/phy/phy.yaml#/properties/tx-amplitude-microvolt' I don't know much about yaml. If that works, that is fine by me. But we should probably check with Rob this is best practice. Andrew
On Wed, Dec 15, 2021 at 12:34:32AM +0100, Marek Behún wrote: > Common PHYs often have the possibility to specify peak-to-peak voltage > on the differential pair - the default voltage sometimes needs to be > changed for a particular board. I can envision needing this, but I can't say that I've seen custom properties being proposed for this purpose. > > Add properties `tx-amplitude-microvolt` and > `tx-amplitude-microvolt-names` for this purpose. The second property is > needed to specify Is the amplitude peak to peak? You just said it was, but perhaps make the property name more clearly defined: tx-p2p-microvolt > > Example usage with only one voltage (it will be used for all supported > PHY modes, the `tx-amplitude-microvolt-names` property is not needed in > this case): > > tx-amplitude-microvolt = <915000>; > > Example usage with voltages for multiple modes: > > tx-amplitude-microvolt = <915000>, <1100000>, <1200000>; > tx-amplitude-microvolt-names = "2500base-x", "usb", "pcie"; I'm not wild about the -names, but I think outside of ethernet most cases will only be 1 entry. For a phy provider with multiple phys, what if each one needs a different voltage (for the same mode)? > > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > > I wanted to constrain the values allowed in the > `tx-amplitude-microvolt-names` property to: > - ethernet SerDes modes (sgmii, qsgmii, 10gbase-r, 2500base-x, ...) > - PCIe modes (pattern: ^pcie[1-6]?$) > - USB modes (pattern: ^usb((-host|-device|-otg)?-(ls|fs|hs|ss|ss\+|4))?$) > - DisplayPort modes (pattern: ^dp(-rbr|-hbr[23]?|-uhbr-(10|13.5|20))?$) > - Camera modes (mipi-dphy, mipi-dphy-univ, mipi-dphy-v2.5-univ) > - Storage modes (sata, ufs-hs, ufs-hs-a, ufs-hs-b) > > But was unable to. The '-names' suffix implies string-array type, and > string-array type does not allow to specify a type for all items in a > simple way, i.e.: > items: > enum: > - sgmii > - sata > - usb > ... Works here: Documentation/devicetree/bindings/arm/samsung/pmu.yaml:56 The requirement is you need to constrain the size with maxItems. It can be a 'should be enough for anyone' value. > Such constraint does work when changing ethernet controller's > `phy-connection-type` to array, see > https://lore.kernel.org/netdev/20211123164027.15618-2-kabel@kernel.org/ > > But it seems that string-array type prohibits this. > > --- > .../devicetree/bindings/phy/phy.yaml | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/phy.yaml b/Documentation/devicetree/bindings/phy/phy.yaml > index a5b6b04cff5b..8915065cf6c2 100644 > --- a/Documentation/devicetree/bindings/phy/phy.yaml > +++ b/Documentation/devicetree/bindings/phy/phy.yaml > @@ -31,9 +31,29 @@ properties: > Phandle to a regulator that provides power to the PHY. This regulator > will be managed during the PHY power on/off sequence. > > + tx-amplitude-microvolt: > + description: > + Transmit amplitude voltages in microvolts, peak-to-peak. If this property > + contains multiple values for various PHY modes, the > + 'tx-amplitude-microvolt-names' property must be provided and contain > + corresponding mode names. > + > + tx-amplitude-microvolt-names: > + description: | > + Names of the modes corresponding to voltages in the > + 'tx-amplitude-microvolt' property. Required only if multiple voltages > + are provided. > + > + If a value of 'default' is provided, the system should use it for any PHY > + mode that is otherwise not defined here. If 'default' is not provided, the > + system should use manufacturer default value. > + > required: > - '#phy-cells' > > +dependencies: > + tx-amplitude-microvolt-names: [ tx-amplitude-microvolt ] > + > additionalProperties: true > > examples: > @@ -46,6 +66,8 @@ examples: > phy: phy { > #phy-cells = <1>; > phy-supply = <&phy_regulator>; > + tx-amplitude-microvolt = <915000>, <1100000>, <1200000>; > + tx-amplitude-microvolt-names = "2500base-x", "usb-hs", "usb-ss"; > }; > > ethernet-controller { > -- > 2.32.0 > >
On Wed, Dec 15, 2021 at 06:22:22PM +0100, Marek Behún wrote: > On Wed, 15 Dec 2021 11:55:00 +0100 > Andrew Lunn <andrew@lunn.ch> wrote: > > > On Wed, Dec 15, 2021 at 12:34:32AM +0100, Marek Behún wrote: > > > Common PHYs often have the possibility to specify peak-to-peak voltage > > > on the differential pair - the default voltage sometimes needs to be > > > changed for a particular board. > > > > Hi Marek > > > > Common PHYs are not the only user of this. Ethernet PHYs can also use > > it, as well as SERDESes embedded within Ethernet switches. > > > > That is why i suggested these properties go into something like > > serdes.yaml. That can then be included into Common PHY, Ethernet PHYs, > > switch drivers etc. > > > > Please could you make such a split? > > > > Andrew > > Hi Andrew, > > and where (into which directory) should this serdes.yaml file go? > > My idea was to put the properties into common PHY and then refer to > them from other places, so for example this would be put into ethernet > PHY binding: > > serdes-tx-amplitude-microvolt: Why do you need a different name here? > $ref: '/schemas/phy/phy.yaml#/properties/tx-amplitude-microvolt' This is a pattern I try to avoid generally. Put what's common in 1 schema file and then reference all of it from the top level. Rob
Hello Rob, On Wed, 15 Dec 2021 15:58:40 -0600 Rob Herring <robh@kernel.org> wrote: > On Wed, Dec 15, 2021 at 12:34:32AM +0100, Marek Behún wrote: > > Common PHYs often have the possibility to specify peak-to-peak voltage > > on the differential pair - the default voltage sometimes needs to be > > changed for a particular board. > > I can envision needing this, but I can't say that I've seen custom > properties being proposed for this purpose. > > > > > Add properties `tx-amplitude-microvolt` and > > `tx-amplitude-microvolt-names` for this purpose. The second property is > > needed to specify > > Is the amplitude peak to peak? You just said it was, but perhaps make > the property name more clearly defined: tx-p2p-microvolt Yes, it is peak to peak. > > > > Example usage with only one voltage (it will be used for all supported > > PHY modes, the `tx-amplitude-microvolt-names` property is not needed in > > this case): > > > > tx-amplitude-microvolt = <915000>; > > > > Example usage with voltages for multiple modes: > > > > tx-amplitude-microvolt = <915000>, <1100000>, <1200000>; > > tx-amplitude-microvolt-names = "2500base-x", "usb", "pcie"; > > I'm not wild about the -names, but I think outside of ethernet most > cases will only be 1 entry. > > For a phy provider with multiple phys, what if each one needs a > different voltage (for the same mode)? For such a provider I think the best way would be to have the different PHYs each have a subnode: phy-provider { phy@0 { tx-p2p-microvolt = ...; }; } > > > > > Signed-off-by: Marek Behún <kabel@kernel.org> > > --- > > > > I wanted to constrain the values allowed in the > > `tx-amplitude-microvolt-names` property to: > > - ethernet SerDes modes (sgmii, qsgmii, 10gbase-r, 2500base-x, ...) > > - PCIe modes (pattern: ^pcie[1-6]?$) > > - USB modes (pattern: ^usb((-host|-device|-otg)?-(ls|fs|hs|ss|ss\+|4))?$) > > - DisplayPort modes (pattern: ^dp(-rbr|-hbr[23]?|-uhbr-(10|13.5|20))?$) > > - Camera modes (mipi-dphy, mipi-dphy-univ, mipi-dphy-v2.5-univ) > > - Storage modes (sata, ufs-hs, ufs-hs-a, ufs-hs-b) > > > > But was unable to. The '-names' suffix implies string-array type, and > > string-array type does not allow to specify a type for all items in a > > simple way, i.e.: > > items: > > enum: > > - sgmii > > - sata > > - usb > > ... > > Works here: Documentation/devicetree/bindings/arm/samsung/pmu.yaml:56 > > The requirement is you need to constrain the size with maxItems. It can > be a 'should be enough for anyone' value. Thx. Marek
diff --git a/Documentation/devicetree/bindings/phy/phy.yaml b/Documentation/devicetree/bindings/phy/phy.yaml index a5b6b04cff5b..8915065cf6c2 100644 --- a/Documentation/devicetree/bindings/phy/phy.yaml +++ b/Documentation/devicetree/bindings/phy/phy.yaml @@ -31,9 +31,29 @@ properties: Phandle to a regulator that provides power to the PHY. This regulator will be managed during the PHY power on/off sequence. + tx-amplitude-microvolt: + description: + Transmit amplitude voltages in microvolts, peak-to-peak. If this property + contains multiple values for various PHY modes, the + 'tx-amplitude-microvolt-names' property must be provided and contain + corresponding mode names. + + tx-amplitude-microvolt-names: + description: | + Names of the modes corresponding to voltages in the + 'tx-amplitude-microvolt' property. Required only if multiple voltages + are provided. + + If a value of 'default' is provided, the system should use it for any PHY + mode that is otherwise not defined here. If 'default' is not provided, the + system should use manufacturer default value. + required: - '#phy-cells' +dependencies: + tx-amplitude-microvolt-names: [ tx-amplitude-microvolt ] + additionalProperties: true examples: @@ -46,6 +66,8 @@ examples: phy: phy { #phy-cells = <1>; phy-supply = <&phy_regulator>; + tx-amplitude-microvolt = <915000>, <1100000>, <1200000>; + tx-amplitude-microvolt-names = "2500base-x", "usb-hs", "usb-ss"; }; ethernet-controller {
Common PHYs often have the possibility to specify peak-to-peak voltage on the differential pair - the default voltage sometimes needs to be changed for a particular board. Add properties `tx-amplitude-microvolt` and `tx-amplitude-microvolt-names` for this purpose. The second property is needed to specify Example usage with only one voltage (it will be used for all supported PHY modes, the `tx-amplitude-microvolt-names` property is not needed in this case): tx-amplitude-microvolt = <915000>; Example usage with voltages for multiple modes: tx-amplitude-microvolt = <915000>, <1100000>, <1200000>; tx-amplitude-microvolt-names = "2500base-x", "usb", "pcie"; Signed-off-by: Marek Behún <kabel@kernel.org> --- I wanted to constrain the values allowed in the `tx-amplitude-microvolt-names` property to: - ethernet SerDes modes (sgmii, qsgmii, 10gbase-r, 2500base-x, ...) - PCIe modes (pattern: ^pcie[1-6]?$) - USB modes (pattern: ^usb((-host|-device|-otg)?-(ls|fs|hs|ss|ss\+|4))?$) - DisplayPort modes (pattern: ^dp(-rbr|-hbr[23]?|-uhbr-(10|13.5|20))?$) - Camera modes (mipi-dphy, mipi-dphy-univ, mipi-dphy-v2.5-univ) - Storage modes (sata, ufs-hs, ufs-hs-a, ufs-hs-b) But was unable to. The '-names' suffix implies string-array type, and string-array type does not allow to specify a type for all items in a simple way, i.e.: items: enum: - sgmii - sata - usb ... Such constraint does work when changing ethernet controller's `phy-connection-type` to array, see https://lore.kernel.org/netdev/20211123164027.15618-2-kabel@kernel.org/ But it seems that string-array type prohibits this. --- .../devicetree/bindings/phy/phy.yaml | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)