[net-next,v5,0/9] net: phy: Introduce PHY Package concept

  Idea of this big series is to introduce the concept of PHY package in DT
and give PHY drivers a way to derive the base address from DT.

The concept of PHY package is nothing new and is already a thing in the
kernel with the API phy_package_join/leave/read/write.

What is currently lacking is describing this in DT and better reference
a base address to calculate offset from.

In the scenario of a PHY package where multiple address are used and
there isn't a way to get the base address of the PHY package from some
regs, getting the information from DT is the only way.

A possible example to this problem is this:

        ethernet-phy-package@0 {
            compatible = "qcom,qca807x-package";
            #address-cells = <1>;
            #size-cells = <0>;

            reg = <0>;
            qcom,package-mode = "qsgmii";

            ethernet-phy@1 {
              reg = <1>;
            };

            phy4: ethernet-phy@4 {
              reg = <4>;
            };
        };

The mdio parse functions are changed to address for this additional
special node, the function is changed to simply detect this node and
search also in this. (we match the node name to be "ethernet-phy-package")

PHY driver can then use introduced helper of_phy_package_join to join the
PHY to the PHY package and derive the base address from DT.


The base addr + offset implementation adds additional complexity to
the code but I think will make DT reviwers happier since the absolute
reg implementation might make things confusing with having double reg
in the DTS. I'm open to any alternative implementation and also to
revert this to the absolute implementation.


Changes v5:
- Rebase on top of net-next
- Change implementation to base addr + offset in subnode
- Adapt to all the changes and cleanup done to at803x
Changes v4:
- Rework DT implementation
- Drop of autojoin support and rework to simple helper
- Rework PHY driver to the new implementation
- Add compatible for qca807x package
- Further cleanup patches
Changes v3:
- Add back compatible implementation
- Detach patch that can be handled separately (phy_package_mmd, 
  phy_package extended)
- Rework code to new simplified implementation with base addr + offset
- Improve documentation with additional info and description
Changes v2:
- Drop compatible "ethernet-phy-package", use node name prefix matching
  instead
- Improve DT example
- Add reg for ethernet-phy-package
- Drop phy-mode for ethernet-phy-package
- Drop patch for generalization of phy-mode
- Drop global-phy property (handle internally to the PHY driver)
- Rework OF phy package code and PHY driver to handle base address
- Fix missing of_node_put
- Add some missing docs for added variables in struct
- Move some define from dt-bindings include to PHY driver
- Handle qsgmii validation in PHY driver
- Fix wrong include for gpiolib
- Drop reduntant version.h include

Christian Marangi (7):
  dt-bindings: net: document ethernet PHY package nodes
  net: phy: add support for scanning PHY in PHY packages nodes
  net: phy: add devm/of_phy_package_join helper
  net: phy: qcom: move more function to shared library
  dt-bindings: net: Document Qcom QCA807x PHY package
  net: phy: qcom: generalize some qca808x LED functions
  net: phy: qca807x: add support for configurable LED

Robert Marko (2):
  dt-bindings: net: add QCA807x PHY defines
  net: phy: qcom: add support for QCA807x PHY Family

 .../bindings/net/ethernet-phy-package.yaml    |  55 ++
 .../devicetree/bindings/net/qcom,qca807x.yaml | 142 +++
 drivers/net/mdio/of_mdio.c                    |  75 +-
 drivers/net/phy/mdio_bus.c                    |  44 +-
 drivers/net/phy/phy_device.c                  |  84 ++
 drivers/net/phy/qcom/Kconfig                  |   8 +
 drivers/net/phy/qcom/Makefile                 |   1 +
 drivers/net/phy/qcom/at803x.c                 |  35 -
 drivers/net/phy/qcom/qca807x.c                | 832 ++++++++++++++++++
 drivers/net/phy/qcom/qca808x.c                | 311 +------
 drivers/net/phy/qcom/qcom-phy-lib.c           | 247 ++++++
 drivers/net/phy/qcom/qcom.h                   | 123 +++
 include/dt-bindings/net/qcom-qca807x.h        |  30 +
 include/linux/of_mdio.h                       |  26 +
 include/linux/phy.h                           |   6 +
 15 files changed, 1648 insertions(+), 371 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
 create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
 create mode 100644 drivers/net/phy/qcom/qca807x.c
 create mode 100644 include/dt-bindings/net/qcom-qca807x.h
  

Quoting Christian Marangi (2024-02-01 16:17:28)
> 
> +static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
> +                                  int base_addr, bool *scanphys)
> +{
> +       struct device_node *child;
> +       int addr, rc = 0;
> +
> +       /* Loop over the child nodes and register a phy_device for each phy */
> +       for_each_available_child_of_node(np, child) {
> +               if (of_node_name_eq(child, "ethernet-phy-package")) {
> +                       rc = of_property_read_u32(child, "reg", &addr);
> +                       if (rc)
> +                               goto exit;

This means a PHY package node w/o a reg property will prevent all other
PHYs in the same parent node to be found?

> +
> +                       rc = __of_mdiobus_parse_phys(mdio, child, addr, scanphys);

You might want to save passing scanphys down, PHYs w/o a reg property in
a PHY package won't be "auto scanned" later.

> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index afbad1ad8683..7737d0101d7b 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -459,20 +459,33 @@ EXPORT_SYMBOL(of_mdio_find_bus);
>   * found, set the of_node pointer for the mdio device. This allows
>   * auto-probed phy devices to be supplied with information passed in
>   * via DT.
> + * If a PHY package is found, PHY is searched also there.
>   */
> -static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
> -                                   struct mdio_device *mdiodev)
> +static int of_mdiobus_find_phy(struct device *dev, struct mdio_device *mdiodev,
> +                              struct device_node *np, int base_addr)
>  {
> -       struct device *dev = &mdiodev->dev;
>         struct device_node *child;
>  
> -       if (dev->of_node || !bus->dev.of_node)
> -               return;
> +       for_each_available_child_of_node(np, child) {
> +               int addr, ret;
>  
> -       for_each_available_child_of_node(bus->dev.of_node, child) {
> -               int addr;
> +               if (of_node_name_eq(child, "ethernet-phy-package")) {
> +                       ret = of_property_read_u32(child, "reg", &addr);
> +                       if (ret)
> +                               return ret;

of_node_put
  

Quoting Christian Marangi (2024-02-01 16:17:29)
> +/**
> + * of_phy_package_join - join a common PHY group in PHY package
> + * @phydev: target phy_device struct
> + * @priv_size: if non-zero allocate this amount of bytes for private data
> + *
> + * This is a variant of phy_package_join for PHY package defined in DT.
> + *
> + * The parent node of the @phydev is checked as a valid PHY package node
> + * structure (by matching the node name "ethernet-phy-package") and the
> + * base_addr for the PHY package is passed to phy_package_join.
> + *
> + * With this configuration the shared struct will also have the np value
> + * filled to use additional DT defined properties in PHY specific
> + * probe_once and config_init_once PHY package OPs.
> + *
> + * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()

So, < 0 on error ?

> +int of_phy_package_join(struct phy_device *phydev, size_t priv_size)
> +{
> +       struct device_node *node = phydev->mdio.dev.of_node;
> +       struct device_node *package_node;
> +       u32 base_addr;
> +       int ret;
> +
> +       if (!node)
> +               return -EINVAL;
> +
> +       package_node = of_get_parent(node);

Is the node put on package leave?

> +       if (!package_node)
> +               return -EINVAL;
> +
> +       if (!of_node_name_eq(package_node, "ethernet-phy-package")

of_put_node? + below.

> +               return -EINVAL;
> +
> +       if (of_property_read_u32(package_node, "reg", &base_addr))
> +               return -EINVAL;
> +
> +       ret = phy_package_join(phydev, base_addr, priv_size);
> +       if (ret)
> +               return ret;
> +
> +       phydev->shared->np = package_node;

Just looked quickly, looks like ->np is uninitialized in the !of join
case.

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_phy_package_join);
  

On 01/02/2024 16:17, Christian Marangi wrote:
> From: Robert Marko <robert.marko@sartura.hr>
> 
> Add DT bindings defined for Qualcomm QCA807x PHY series related to
> calibration and DAC settings.

Nothing from this file is used and your commit msg does not provide
rationale "why", thus it does not look like something for bindings.
Otherwise please point me which patch with *driver* uses these bindings.

> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++

Use filename matching compatible, so vendor,device. No wildcards, unless
your compatible also has them.

>  1 file changed, 30 insertions(+)
>  create mode 100644 include/dt-bindings/net/qcom-qca807x.h
> 



Best regards,
Krzysztof
  

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
  

Quoting Christian Marangi (2024-02-01 18:20:10)
> On Thu, Feb 01, 2024 at 05:25:36PM +0100, Antoine Tenart wrote:
> > Quoting Christian Marangi (2024-02-01 16:17:28)
> > > +
> > > +                       rc = __of_mdiobus_parse_phys(mdio, child, addr, scanphys);
> > 
> > You might want to save passing scanphys down, PHYs w/o a reg property in
> > a PHY package won't be "auto scanned" later.
> 
> I might be confused by this, but isn't this already done? (passing
> scanphys in each recursive call so we can set it to true if needed?)
> 
> Also I think the scanphys should be skipped for the PHY package
> (assuming we make reg mandatory, it would be an error condition and
> should not be handled?)

Sorry if that wasn't clear, this is what I meant. scanphys doesn't need
to be set to true in a PHY package (both if we want reg to be mandatory
there and because my understanding of the auto-scan code is PHYs in a
PHY package won't be auto scanned anyway).

Thanks,
Antoine
  

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
>
  

On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote:
> > On 01/02/2024 16:17, Christian Marangi wrote:
> > > From: Robert Marko <robert.marko@sartura.hr>
> > > 
> > > Add DT bindings defined for Qualcomm QCA807x PHY series related to
> > > calibration and DAC settings.
> > 
> > Nothing from this file is used and your commit msg does not provide
> > rationale "why", thus it does not look like something for bindings.
> > Otherwise please point me which patch with *driver* uses these bindings.
> >
> 
> Hi, since I have to squash this, I will include the reason in the schema
> patch.
> 
> Anyway these are raw values used to configure the qcom,control-dac
> property.

Maybe I am missing something, but a quick scan of the patchset and a
grep of the tree doesn't show this property being documented anywhere.

> In the driver it's used by qca807x_config_init. We read what is set in
> DT and we configure the reg accordingly.
> 
> If this is wrong should we use a more schema friendly approach with
> declaring an enum of string and document that there?

Without any idea of what that property is used for it is hard to say,
but personally I much prefer enums of strings for what looks like a
property that holds register values.

That you felt it necessary to add defines for the values makes it more
like that that is the case.

Cheers,
Conor.

> 
> > > 
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++
> > 
> > Use filename matching compatible, so vendor,device. No wildcards, unless
> > your compatible also has them.
> > 
> > >  1 file changed, 30 insertions(+)
> > >  create mode 100644 include/dt-bindings/net/qcom-qca807x.h
> > > 
> > 
> > 
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> -- 
> 	Ansuel
  

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
  

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
  

On Thu, Feb 01, 2024 at 04:17:27PM +0100, Christian Marangi wrote:
> Document ethernet PHY package nodes used to describe PHY shipped in
> bundle of 2-5 PHY. The special node describe a container of PHY that
> share common properties. This is a generic schema and PHY package
> should create specialized version with the required additional shared
> properties.
> 
> Example are PHY packages that have some regs only in one PHY of the
> package and will affect every other PHY in the package, for example
> related to PHY interface mode calibration or global PHY mode selection.
> 
> The PHY package node MUST declare the base address used by the PHY driver
> for global configuration by calculating the offsets of the global PHY
> based on the base address of the PHY package.
> 
> Each reg of the PHYs defined in the PHY Package node is an offset of the
> PHY Package reg.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../bindings/net/ethernet-phy-package.yaml    | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> new file mode 100644
> index 000000000000..d7cdbb1a4b3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ethernet PHY Package Common Properties
> +
> +maintainers:
> +  - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> +  PHY packages are multi-port Ethernet PHY of the same family
> +  and each Ethernet PHY is affected by the global configuration
> +  of the PHY package.
> +
> +  Each reg of the PHYs defined in the PHY Package node is
> +  an offset of the PHY Package reg.
> +
> +  Each Ethernet PHYs defined in the PHY package node is
> +  reachable in the MDIO bus at the address of the PHY
> +  Package offset of the Ethernet PHY reg.

If the phys are addressed with an MDIO address, then just use those.

> +
> +properties:
> +  $nodename:
> +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"

Can't be optional if 'reg' is required (which it should be).

> +
> +  reg:
> +    minimum: 0
> +    maximum: 31
> +    description:
> +      The base ID number for the PHY package.
> +      Commonly the ID of the first PHY in the PHY package.
> +
> +      Some PHY in the PHY package might be not defined but
> +      still occupy ID on the device (just not attached to
> +      anything) hence the PHY package reg might correspond
> +      to a not attached PHY (offset 0).
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  ^ethernet-phy(@[a-f0-9]+)?$:

Same issue here.

> +    $ref: ethernet-phy.yaml#
> +
> +required:
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +additionalProperties: true
> -- 
> 2.43.0
>