diff mbox series

[net-next,v5,6/9] dt-bindings: net: Document Qcom QCA807x PHY package

Message ID 20240201151747.7524-7-ansuelsmth@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: Introduce PHY Package concept | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Christian Marangi Feb. 1, 2024, 3:17 p.m. UTC
Document Qcom QCA807x PHY package.

Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
1000BASE-T PHY-s.

Document the required property to make the PHY package correctly
configure and work.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml

Comments

Andrew Lunn Feb. 2, 2024, 1:16 a.m. UTC | #1
> +  PHY package can be configured in 3 mode following this table:
> +
> +                First Serdes mode       Second Serdes mode
> +  Option 1      PSGMII for copper       Disabled
> +                ports 0-4
> +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> +                ports 0-4
> +  Option 3      QSGMII for copper       SGMII for
> +                ports 0-3               copper port 4
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  compatible:
> +    const: qcom,qca807x-package
> +
> +  qcom,package-mode:
> +    enum:
> +      - qsgmii
> +      - psgmii

There are three modes listed above, yet only two values here? Please
describe how they related.

> +
> +  qcom,tx-driver-strength:
> +    description: set the TX Amplifier value in mv.
> +      If not defined, 600mw is set by default.

Looking at other bindings, it seems pretty normal to include the units
in the property name: qcom,tx-driver-strength-milliwatts.

   Andrew
Krzysztof Kozlowski Feb. 2, 2024, 7:45 a.m. UTC | #2
On 01/02/2024 16:17, Christian Marangi wrote:
> Document Qcom QCA807x PHY package.
> 
> Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> 1000BASE-T PHY-s.
> 
> Document the required property to make the PHY package correctly
> configure and work.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++

Your bindings header must be squashed here. Headers are not separate
thing from the bindings.

>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> new file mode 100644
> index 000000000000..1c3692897b02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QCA807X Ethernet PHY

What is "X"? Wildcards are usually not expected.

> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +  - Robert Marko <robert.marko@sartura.hr>
> +
> +description: |
> +  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> +  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> +  1000BASE-T PHY-s.
> +
> +  They feature 2 SerDes, one for PSGMII or QSGMII connection with
> +  MAC, while second one is SGMII for connection to MAC or fiber.
> +
> +  Both models have a combo port that supports 1000BASE-X and
> +  100BASE-FX fiber.
> +
> +  Each PHY inside of QCA807x series has 4 digitally controlled
> +  output only pins that natively drive LED-s for up to 2 attached
> +  LEDs. Some vendor also use these 4 output for GPIO usage without
> +  attaching LEDs.
> +
> +  Note that output pins can be set to drive LEDs OR GPIO, mixed
> +  definition are not accepted.
> +
> +  PHY package can be configured in 3 mode following this table:
> +
> +                First Serdes mode       Second Serdes mode
> +  Option 1      PSGMII for copper       Disabled
> +                ports 0-4
> +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> +                ports 0-4
> +  Option 3      QSGMII for copper       SGMII for
> +                ports 0-3               copper port 4
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  compatible:
> +    const: qcom,qca807x-package
> +
> +  qcom,package-mode:

Where is definition of this property with type and description?

> +    enum:
> +      - qsgmii
> +      - psgmii
> +
> +  qcom,tx-driver-strength:

Use proper unit suffix.

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> +    description: set the TX Amplifier value in mv.
> +      If not defined, 600mw is set by default.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [140, 160, 180, 200, 220,
> +           240, 260, 280, 300, 320,
> +           400, 500, 600]
> +
> +patternProperties:
> +  ^ethernet-phy(@[a-f0-9]+)?$:
> +    $ref: ethernet-phy.yaml#
> +
> +    properties:
> +      gpio-controller:
> +        description: set the output lines as GPIO instead of LEDs
> +        type: boolean
> +
> +      '#gpio-cells':
> +        description: number of GPIO cells for the PHY
> +        const: 2
> +
> +    dependencies:
> +      gpio-controller: ['#gpio-cells']

Why do you need it? None of gpio-controllers do it, I think.

> +
> +    if:
> +      required:
> +        - gpio-controller
> +    then:
> +      properties:
> +        leds: false
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false


Best regards,
Krzysztof
Christian Marangi Feb. 2, 2024, 3:12 p.m. UTC | #3
On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2024 16:17, Christian Marangi wrote:
> > Document Qcom QCA807x PHY package.
> > 
> > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > 1000BASE-T PHY-s.
> > 
> > Document the required property to make the PHY package correctly
> > configure and work.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> 
> Your bindings header must be squashed here. Headers are not separate
> thing from the bindings.
> 
> >  1 file changed, 142 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > new file mode 100644
> > index 000000000000..1c3692897b02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > @@ -0,0 +1,142 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm QCA807X Ethernet PHY
> 
> What is "X"? Wildcards are usually not expected.
>

It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
or qca8074 since they would refer to a more generic Family of devices.

What would be the correct way? We have many other case on net with
schema called qca8k that refer to the family of Ethernet Switch but in
it refer to qca8327 qca8337 qca8334...

> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@gmail.com>
> > +  - Robert Marko <robert.marko@sartura.hr>
> > +
> > +description: |
> > +  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > +  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > +  1000BASE-T PHY-s.
> > +
> > +  They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > +  MAC, while second one is SGMII for connection to MAC or fiber.
> > +
> > +  Both models have a combo port that supports 1000BASE-X and
> > +  100BASE-FX fiber.
> > +
> > +  Each PHY inside of QCA807x series has 4 digitally controlled
> > +  output only pins that natively drive LED-s for up to 2 attached
> > +  LEDs. Some vendor also use these 4 output for GPIO usage without
> > +  attaching LEDs.
> > +
> > +  Note that output pins can be set to drive LEDs OR GPIO, mixed
> > +  definition are not accepted.
> > +
> > +  PHY package can be configured in 3 mode following this table:
> > +
> > +                First Serdes mode       Second Serdes mode
> > +  Option 1      PSGMII for copper       Disabled
> > +                ports 0-4
> > +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> > +                ports 0-4
> > +  Option 3      QSGMII for copper       SGMII for
> > +                ports 0-3               copper port 4
> > +
> > +$ref: ethernet-phy-package.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,qca807x-package
> > +
> > +  qcom,package-mode:
> 
> Where is definition of this property with type and description?
> 
> > +    enum:
> > +      - qsgmii
> > +      - psgmii
> > +
> > +  qcom,tx-driver-strength:
> 
> Use proper unit suffix.
> 
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> > +    description: set the TX Amplifier value in mv.
> > +      If not defined, 600mw is set by default.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [140, 160, 180, 200, 220,
> > +           240, 260, 280, 300, 320,
> > +           400, 500, 600]
> > +
> > +patternProperties:
> > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > +    $ref: ethernet-phy.yaml#
> > +
> > +    properties:
> > +      gpio-controller:
> > +        description: set the output lines as GPIO instead of LEDs
> > +        type: boolean
> > +
> > +      '#gpio-cells':
> > +        description: number of GPIO cells for the PHY
> > +        const: 2
> > +
> > +    dependencies:
> > +      gpio-controller: ['#gpio-cells']
> 
> Why do you need it? None of gpio-controllers do it, I think.
> 

Well gpio-controller property is optional and having that declared and
#gpio-cells skipped will result in an error on probe. I think it should
be the opposite with other schema having to declare this.

In usual way for gpio-controller both property are defined as required
but you can see some (to-be-converted) txt files whith comments where
they say:

./mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.

Should I instead add this condition in the if right below?

> > +
> > +    if:
> > +      required:
> > +        - gpio-controller
> > +    then:
> > +      properties:
> > +        leds: false
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +
> > +unevaluatedProperties: false
> 
> 
> Best regards,
> Krzysztof
>
Rob Herring Feb. 2, 2024, 8:39 p.m. UTC | #4
On Fri, Feb 02, 2024 at 04:12:53PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> > On 01/02/2024 16:17, Christian Marangi wrote:
> > > Document Qcom QCA807x PHY package.
> > > 
> > > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > 1000BASE-T PHY-s.
> > > 
> > > Document the required property to make the PHY package correctly
> > > configure and work.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> > 
> > Your bindings header must be squashed here. Headers are not separate
> > thing from the bindings.
> > 
> > >  1 file changed, 142 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > new file mode 100644
> > > index 000000000000..1c3692897b02
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > @@ -0,0 +1,142 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm QCA807X Ethernet PHY
> > 
> > What is "X"? Wildcards are usually not expected.
> >
> 
> It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
> or qca8074 since they would refer to a more generic Family of devices.

Declare them all or provide some justification such as the exact model 
is discoverable (and better be sure power on is the same in order to do 
discovery).

> What would be the correct way? We have many other case on net with
> schema called qca8k that refer to the family of Ethernet Switch but in
> it refer to qca8327 qca8337 qca8334...
> 
> > > +
> > > +maintainers:
> > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > +  - Robert Marko <robert.marko@sartura.hr>
> > > +
> > > +description: |
> > > +  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > +  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > +  1000BASE-T PHY-s.
> > > +
> > > +  They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > > +  MAC, while second one is SGMII for connection to MAC or fiber.
> > > +
> > > +  Both models have a combo port that supports 1000BASE-X and
> > > +  100BASE-FX fiber.
> > > +
> > > +  Each PHY inside of QCA807x series has 4 digitally controlled
> > > +  output only pins that natively drive LED-s for up to 2 attached
> > > +  LEDs. Some vendor also use these 4 output for GPIO usage without
> > > +  attaching LEDs.
> > > +
> > > +  Note that output pins can be set to drive LEDs OR GPIO, mixed
> > > +  definition are not accepted.
> > > +
> > > +  PHY package can be configured in 3 mode following this table:
> > > +
> > > +                First Serdes mode       Second Serdes mode
> > > +  Option 1      PSGMII for copper       Disabled
> > > +                ports 0-4
> > > +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> > > +                ports 0-4
> > > +  Option 3      QSGMII for copper       SGMII for
> > > +                ports 0-3               copper port 4
> > > +
> > > +$ref: ethernet-phy-package.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: qcom,qca807x-package
> > > +
> > > +  qcom,package-mode:
> > 
> > Where is definition of this property with type and description?
> > 
> > > +    enum:
> > > +      - qsgmii
> > > +      - psgmii
> > > +
> > > +  qcom,tx-driver-strength:
> > 
> > Use proper unit suffix.
> > 
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> > 
> > > +    description: set the TX Amplifier value in mv.
> > > +      If not defined, 600mw is set by default.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [140, 160, 180, 200, 220,
> > > +           240, 260, 280, 300, 320,
> > > +           400, 500, 600]
> > > +
> > > +patternProperties:
> > > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > > +    $ref: ethernet-phy.yaml#
> > > +
> > > +    properties:
> > > +      gpio-controller:
> > > +        description: set the output lines as GPIO instead of LEDs
> > > +        type: boolean

You only need 'gpio-controller: true'. The core already defines the 
type.

> > > +
> > > +      '#gpio-cells':
> > > +        description: number of GPIO cells for the PHY

Not a useful description. Normally, you'd describe what's in the cells, 
but GPIO is standardized so no need (unless you are deviating from the 
norm).

> > > +        const: 2
> > > +
> > > +    dependencies:
> > > +      gpio-controller: ['#gpio-cells']
> > 
> > Why do you need it? None of gpio-controllers do it, I think.
> > 
> 
> Well gpio-controller property is optional and having that declared and
> #gpio-cells skipped will result in an error on probe. I think it should
> be the opposite with other schema having to declare this.

If you think everything else is wrong, then please fix them. :)

> 
> In usual way for gpio-controller both property are defined as required
> but you can see some (to-be-converted) txt files whith comments where
> they say:
> 
> ./mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.
> 
> Should I instead add this condition in the if right below?

The core schema enforces this dependency, so you don't need to.

Rob
Rob Herring Feb. 2, 2024, 8:45 p.m. UTC | #5
On Thu, Feb 01, 2024 at 04:17:32PM +0100, Christian Marangi wrote:
> Document Qcom QCA807x PHY package.
> 
> Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> 1000BASE-T PHY-s.
> 
> Document the required property to make the PHY package correctly
> configure and work.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> new file mode 100644
> index 000000000000..1c3692897b02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QCA807X Ethernet PHY
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +  - Robert Marko <robert.marko@sartura.hr>
> +
> +description: |
> +  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> +  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> +  1000BASE-T PHY-s.
> +
> +  They feature 2 SerDes, one for PSGMII or QSGMII connection with
> +  MAC, while second one is SGMII for connection to MAC or fiber.
> +
> +  Both models have a combo port that supports 1000BASE-X and
> +  100BASE-FX fiber.
> +
> +  Each PHY inside of QCA807x series has 4 digitally controlled
> +  output only pins that natively drive LED-s for up to 2 attached
> +  LEDs. Some vendor also use these 4 output for GPIO usage without
> +  attaching LEDs.
> +
> +  Note that output pins can be set to drive LEDs OR GPIO, mixed
> +  definition are not accepted.
> +
> +  PHY package can be configured in 3 mode following this table:
> +
> +                First Serdes mode       Second Serdes mode
> +  Option 1      PSGMII for copper       Disabled
> +                ports 0-4
> +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> +                ports 0-4
> +  Option 3      QSGMII for copper       SGMII for
> +                ports 0-3               copper port 4
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  compatible:
> +    const: qcom,qca807x-package
> +
> +  qcom,package-mode:
> +    enum:
> +      - qsgmii
> +      - psgmii
> +
> +  qcom,tx-driver-strength:
> +    description: set the TX Amplifier value in mv.
> +      If not defined, 600mw is set by default.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [140, 160, 180, 200, 220,
> +           240, 260, 280, 300, 320,
> +           400, 500, 600]
> +
> +patternProperties:
> +  ^ethernet-phy(@[a-f0-9]+)?$:

I don't get how an address is optional.

Rob
Christian Marangi Feb. 2, 2024, 8:47 p.m. UTC | #6
On Fri, Feb 02, 2024 at 02:39:15PM -0600, Rob Herring wrote:
> On Fri, Feb 02, 2024 at 04:12:53PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> > > On 01/02/2024 16:17, Christian Marangi wrote:
> > > > Document Qcom QCA807x PHY package.
> > > > 
> > > > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > > 1000BASE-T PHY-s.
> > > > 
> > > > Document the required property to make the PHY package correctly
> > > > configure and work.
> > > > 
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > >  .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> > > 
> > > Your bindings header must be squashed here. Headers are not separate
> > > thing from the bindings.
> > > 
> > > >  1 file changed, 142 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > > new file mode 100644
> > > > index 000000000000..1c3692897b02
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > > @@ -0,0 +1,142 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Qualcomm QCA807X Ethernet PHY
> > > 
> > > What is "X"? Wildcards are usually not expected.
> > >
> > 
> > It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
> > or qca8074 since they would refer to a more generic Family of devices.
> 
> Declare them all or provide some justification such as the exact model 
> is discoverable (and better be sure power on is the same in order to do 
> discovery).
> 
> > What would be the correct way? We have many other case on net with
> > schema called qca8k that refer to the family of Ethernet Switch but in
> > it refer to qca8327 qca8337 qca8334...
> > 
> > > > +
> > > > +maintainers:
> > > > +  - Christian Marangi <ansuelsmth@gmail.com>
> > > > +  - Robert Marko <robert.marko@sartura.hr>
> > > > +
> > > > +description: |
> > > > +  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > > +  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > > +  1000BASE-T PHY-s.
> > > > +
> > > > +  They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > > > +  MAC, while second one is SGMII for connection to MAC or fiber.
> > > > +
> > > > +  Both models have a combo port that supports 1000BASE-X and
> > > > +  100BASE-FX fiber.
> > > > +
> > > > +  Each PHY inside of QCA807x series has 4 digitally controlled
> > > > +  output only pins that natively drive LED-s for up to 2 attached
> > > > +  LEDs. Some vendor also use these 4 output for GPIO usage without
> > > > +  attaching LEDs.
> > > > +
> > > > +  Note that output pins can be set to drive LEDs OR GPIO, mixed
> > > > +  definition are not accepted.
> > > > +
> > > > +  PHY package can be configured in 3 mode following this table:
> > > > +
> > > > +                First Serdes mode       Second Serdes mode
> > > > +  Option 1      PSGMII for copper       Disabled
> > > > +                ports 0-4
> > > > +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> > > > +                ports 0-4
> > > > +  Option 3      QSGMII for copper       SGMII for
> > > > +                ports 0-3               copper port 4
> > > > +
> > > > +$ref: ethernet-phy-package.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: qcom,qca807x-package
> > > > +
> > > > +  qcom,package-mode:
> > > 
> > > Where is definition of this property with type and description?
> > > 
> > > > +    enum:
> > > > +      - qsgmii
> > > > +      - psgmii
> > > > +
> > > > +  qcom,tx-driver-strength:
> > > 
> > > Use proper unit suffix.
> > > 
> > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> > > 
> > > > +    description: set the TX Amplifier value in mv.
> > > > +      If not defined, 600mw is set by default.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [140, 160, 180, 200, 220,
> > > > +           240, 260, 280, 300, 320,
> > > > +           400, 500, 600]
> > > > +
> > > > +patternProperties:
> > > > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > > > +    $ref: ethernet-phy.yaml#
> > > > +
> > > > +    properties:
> > > > +      gpio-controller:
> > > > +        description: set the output lines as GPIO instead of LEDs
> > > > +        type: boolean
> 
> You only need 'gpio-controller: true'. The core already defines the 
> type.
> 
> > > > +
> > > > +      '#gpio-cells':
> > > > +        description: number of GPIO cells for the PHY
> 
> Not a useful description. Normally, you'd describe what's in the cells, 
> but GPIO is standardized so no need (unless you are deviating from the 
> norm).
> 
> > > > +        const: 2
> > > > +
> > > > +    dependencies:
> > > > +      gpio-controller: ['#gpio-cells']
> > > 
> > > Why do you need it? None of gpio-controllers do it, I think.
> > > 
> > 
> > Well gpio-controller property is optional and having that declared and
> > #gpio-cells skipped will result in an error on probe. I think it should
> > be the opposite with other schema having to declare this.
> 
> If you think everything else is wrong, then please fix them. :)
> 
> > 
> > In usual way for gpio-controller both property are defined as required
> > but you can see some (to-be-converted) txt files whith comments where
> > they say:
> > 
> > ./mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.
> > 
> > Should I instead add this condition in the if right below?
> 
> The core schema enforces this dependency, so you don't need to.
>

Oh! No idea the dependency was already enforced, guess I don't have to
fix everything ahahahha
Andrew Lunn Feb. 2, 2024, 9:50 p.m. UTC | #7
> > +patternProperties:
> > +  ^ethernet-phy(@[a-f0-9]+)?$:
> 
> I don't get how an address is optional.

Its pretty unusual, but for example:

arch/arm/boot/dts/nxp/imx/imx6q-novena.dts

&fec {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_enet_novena>;
        phy-mode = "rgmii";
        phy-handle = <&ethphy>;
        phy-reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
        status = "okay";

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                ethphy: ethernet-phy {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        rxc-skew-ps = <3000>;
                        rxdv-skew-ps = <0>;
                        txc-skew-ps = <3000>;
                        txen-skew-ps = <0>;
                        rxd0-skew-ps = <0>;
                        rxd1-skew-ps = <0>;
                        rxd2-skew-ps = <0>;
                        rxd3-skew-ps = <0>;
                        txd0-skew-ps = <3000>;
                        txd1-skew-ps = <3000>;
                        txd2-skew-ps = <3000>;
                        txd3-skew-ps = <3000>;
                };
        };
};

There is no reg property, because its optional. If there is no reg,
there is no address.

When phylib finds a DT blob like this, it enumerates the bus, and then
assigns the nodes to the devices it finds in the order it finds them.

Its old behaviour, from before the times of yaml validation, and
current best practices, etc. But because it works, it still used in
new bindings.

    Andrew
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
new file mode 100644
index 000000000000..1c3692897b02
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
@@ -0,0 +1,142 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm QCA807X Ethernet PHY
+
+maintainers:
+  - Christian Marangi <ansuelsmth@gmail.com>
+  - Robert Marko <robert.marko@sartura.hr>
+
+description: |
+  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
+  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
+  1000BASE-T PHY-s.
+
+  They feature 2 SerDes, one for PSGMII or QSGMII connection with
+  MAC, while second one is SGMII for connection to MAC or fiber.
+
+  Both models have a combo port that supports 1000BASE-X and
+  100BASE-FX fiber.
+
+  Each PHY inside of QCA807x series has 4 digitally controlled
+  output only pins that natively drive LED-s for up to 2 attached
+  LEDs. Some vendor also use these 4 output for GPIO usage without
+  attaching LEDs.
+
+  Note that output pins can be set to drive LEDs OR GPIO, mixed
+  definition are not accepted.
+
+  PHY package can be configured in 3 mode following this table:
+
+                First Serdes mode       Second Serdes mode
+  Option 1      PSGMII for copper       Disabled
+                ports 0-4
+  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
+                ports 0-4
+  Option 3      QSGMII for copper       SGMII for
+                ports 0-3               copper port 4
+
+$ref: ethernet-phy-package.yaml#
+
+properties:
+  compatible:
+    const: qcom,qca807x-package
+
+  qcom,package-mode:
+    enum:
+      - qsgmii
+      - psgmii
+
+  qcom,tx-driver-strength:
+    description: set the TX Amplifier value in mv.
+      If not defined, 600mw is set by default.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [140, 160, 180, 200, 220,
+           240, 260, 280, 300, 320,
+           400, 500, 600]
+
+patternProperties:
+  ^ethernet-phy(@[a-f0-9]+)?$:
+    $ref: ethernet-phy.yaml#
+
+    properties:
+      gpio-controller:
+        description: set the output lines as GPIO instead of LEDs
+        type: boolean
+
+      '#gpio-cells':
+        description: number of GPIO cells for the PHY
+        const: 2
+
+    dependencies:
+      gpio-controller: ['#gpio-cells']
+
+    if:
+      required:
+        - gpio-controller
+    then:
+      properties:
+        leds: false
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy-package@0 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "qcom,qca807x-package";
+            reg = <0>;
+
+            qcom,package-mode = "qsgmii";
+
+            ethernet-phy@0 {
+                reg = <0>;
+
+                leds {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+
+                    led@0 {
+                        reg = <0>;
+                        color = <LED_COLOR_ID_GREEN>;
+                        function = LED_FUNCTION_LAN;
+                        default-state = "keep";
+                    };
+                };
+            };
+
+            ethernet-phy@1 {
+                reg = <1>;
+            };
+
+            ethernet-phy@2 {
+                reg = <2>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            ethernet-phy@3 {
+                reg = <3>;
+            };
+
+            ethernet-phy@4 {
+                reg = <4>;
+            };
+        };
+    };