linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package
@ 2023-11-26  1:53 Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
                   ` (8 more replies)
  0 siblings, 9 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

This depends on another series for PHY package API change. [1]

Idea of this big series is to introduce the concept of PHY package in DT
and generalize the support of it by PHY driver.

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.

The main idea of those API is to permit the PHY to have a shared global
data and to run probe/config only once for the PHY package. There are
various example of this already in the kernel with the mscc, bcm54140
mediatek ge and micrle driver and they all follow the same pattern.

What is currently lacking is describing this in DT and better reference
the PHY in charge of global configuration of the PHY package. For the
already present PHY, the implementation is simple enough with only one
PHY having the required regs to apply global configuration.

This can be ok for simple PHY package but some Qcom PHY package on
""modern"" SoC have more complex implementation. One example is the PHY
for qca807x where some global regs are present in the so-called "combo"
port and everything about psgmii calibration is placed in a 5th port in
the PHY package.

Given these additional thing, the original phy_package API are extended
with support for multiple global PHY for configuration. Each PHY driver
will have an enum of the ID for the global PHY to reference and is
required to pass to the read/write function.

On top of this, it's added correct DT support for describing PHY
package.

One example is this:

        ethernet-phy-package@0 {
            compatible = "ethernet-phy-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.

If this is detected phy core will join each PHY present in the node and
use (if defined) the additional info in the PHY driver to probe/config
once the PHY package.

I hope this implementation is clean enough as I expect more and more of
these configuration to appear in the future.

(For Andrew, we are looking intro making this in at803x PHY driver and see
what functions can be reused, idea is to move the driver to a dedicated
directory and create something like at803x-common.c as the at803x PHY
driver is too bloated and splitting it it's a better approach)

[1] https://patchwork.kernel.org/project/netdevbpf/patch/20231126003748.9600-1-ansuelsmth@gmail.com/

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 (6):
  dt-bindings: net: document ethernet PHY package nodes
  net: phy: add initial support for PHY package in DT
  net: phy: add support for shared priv data size for PHY package in DT
  net: phy: add support for driver specific PHY package probe/config
  dt-bindings: net: Document Qcom QCA807x PHY package
  net: phy: qca807x: Add support for configurable LED

Robert Marko (2):
  dt-bindings: net: add QCA807x PHY defines
  net: phy: add Qualcom QCA807x driver

 .../bindings/net/ethernet-phy-package.yaml    |   75 +
 .../devicetree/bindings/net/qcom,qca807x.yaml |  136 ++
 drivers/net/mdio/of_mdio.c                    |   68 +-
 drivers/net/phy/Kconfig                       |    7 +
 drivers/net/phy/Makefile                      |    1 +
 drivers/net/phy/mdio_bus.c                    |   35 +-
 drivers/net/phy/phy_device.c                  |   54 +
 drivers/net/phy/qca807x.c                     | 1315 +++++++++++++++++
 include/dt-bindings/net/qcom-qca807x.h        |   30 +
 include/linux/phy.h                           |   27 +
 10 files changed, 1719 insertions(+), 29 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/qca807x.c
 create mode 100644 include/dt-bindings/net/qcom-qca807x.h

-- 
2.40.1


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-27 22:16   ` Rob Herring
  2024-01-07 18:00   ` Sergey Ryazanov
  2023-11-26  1:53 ` [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT Christian Marangi
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

Document ethernet PHY package nodes used to describe PHY shipped in
bundle of 4-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 package 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 and declare the
"ethrnet-phy-package" compatible.

Each reg of the PHY defined in the PHY package node is absolute and will
reference the real address of the PHY on the bus.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
 1 file changed, 75 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..244d4bc29164
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
@@ -0,0 +1,75 @@
+# 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:
+  This schema describe PHY package as simple container for
+  a bundle of PHYs that share the same properties and
+  contains the PHYs of the package themself.
+
+  Each reg of the PHYs defined in the PHY package node is
+  absolute and describe the real address of the PHY on the bus.
+
+properties:
+  $nodename:
+    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
+
+  compatible:
+    const: ethernet-phy-package
+
+  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 exist on the device (just not attached to anything).
+      The reg defined in the PHY package node might differ and
+      the related PHY might be not defined.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  ^ethernet-phy(@[a-f0-9]+)?$:
+    $ref: ethernet-phy.yaml#
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: true
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy-package@16 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "ethernet-phy-package";
+            reg = <0x16>;
+
+            ethernet-phy@16 {
+              reg = <0x16>;
+            };
+
+            phy4: ethernet-phy@1a {
+              reg = <0x1a>;
+            };
+        };
+    };
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-28  0:39   ` Andrew Lunn
  2023-11-26  1:53 ` [net-next PATCH RFC v3 3/8] net: phy: add support for shared priv data size " Christian Marangi
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

Add initial support for PHY package in DT.

Make it easier to define PHY package and describe the global PHY
directly in DT by refereincing them by phandles instead of custom
functions in each PHY driver.

Each PHY in a package needs to be defined in a dedicated node in the
mdio node. This dedicated node needs to have the node name with the
prefix "ethernet-phy-package" and compatible set to
"ethernet-phy-package".

With this defined, the generic PHY probe will join each PHY in this
dedicated node to the package.

PHY package will be joined based on the reg defined in the
ethernet-phy-package node.

mdio_bus.c and of_mdio.c is updated to now support and parse also
PHY package subnote by checking if the node compatible is
"ethernet-phy-package".

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/mdio/of_mdio.c   | 68 +++++++++++++++++++++++++-----------
 drivers/net/phy/mdio_bus.c   | 35 ++++++++++++++-----
 drivers/net/phy/phy_device.c | 35 +++++++++++++++++++
 include/linux/phy.h          |  3 ++
 4 files changed, 112 insertions(+), 29 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 64ebcb6d235c..c98e9f7fa3d4 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -139,6 +139,47 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
 }
 EXPORT_SYMBOL(of_mdiobus_child_is_phy);
 
+static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
+				   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_device_is_compatible(child, "ethernet-phy-package")) {
+			rc = __of_mdiobus_parse_phys(mdio, child, scanphys);
+			if (rc && rc != -ENODEV)
+				goto exit;
+
+			continue;
+		}
+
+		addr = of_mdio_parse_addr(&mdio->dev, child);
+		if (addr < 0) {
+			*scanphys = true;
+			continue;
+		}
+
+		if (of_mdiobus_child_is_phy(child))
+			rc = of_mdiobus_register_phy(mdio, child, addr);
+		else
+			rc = of_mdiobus_register_device(mdio, child, addr);
+
+		if (rc == -ENODEV)
+			dev_err(&mdio->dev,
+				"MDIO device at address %d is missing.\n",
+				addr);
+		else if (rc)
+			goto exit;
+	}
+
+	return 0;
+exit:
+	of_node_put(child);
+	return rc;
+}
+
 /**
  * __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -180,25 +221,9 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
 		return rc;
 
 	/* Loop over the child nodes and register a phy_device for each phy */
-	for_each_available_child_of_node(np, child) {
-		addr = of_mdio_parse_addr(&mdio->dev, child);
-		if (addr < 0) {
-			scanphys = true;
-			continue;
-		}
-
-		if (of_mdiobus_child_is_phy(child))
-			rc = of_mdiobus_register_phy(mdio, child, addr);
-		else
-			rc = of_mdiobus_register_device(mdio, child, addr);
-
-		if (rc == -ENODEV)
-			dev_err(&mdio->dev,
-				"MDIO device at address %d is missing.\n",
-				addr);
-		else if (rc)
-			goto unregister;
-	}
+	rc = __of_mdiobus_parse_phys(mdio, np, &scanphys);
+	if (rc)
+		goto unregister;
 
 	if (!scanphys)
 		return 0;
@@ -227,15 +252,16 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
 				if (!rc)
 					break;
 				if (rc != -ENODEV)
-					goto unregister;
+					goto put_unregister;
 			}
 		}
 	}
 
 	return 0;
 
-unregister:
+put_unregister:
 	of_node_put(child);
+unregister:
 	mdiobus_unregister(mdio);
 	return rc;
 }
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 25dcaa49ab8b..05f2e8e01a03 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -455,19 +455,25 @@ 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)
 {
-	struct device *dev = &mdiodev->dev;
 	struct device_node *child;
 
-	if (dev->of_node || !bus->dev.of_node)
-		return;
-
-	for_each_available_child_of_node(bus->dev.of_node, child) {
+	for_each_available_child_of_node(np, child) {
 		int addr;
 
+		if (of_device_is_compatible(child, "ethernet-phy-package")) {
+			if (!of_mdiobus_find_phy(dev, mdiodev, child)) {
+				of_node_put(child);
+				return 0;
+			}
+
+			continue;
+		}
+
 		addr = of_mdio_parse_addr(dev, child);
 		if (addr < 0)
 			continue;
@@ -477,9 +483,22 @@ static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
 			/* The refcount on "child" is passed to the mdio
 			 * device. Do _not_ use of_node_put(child) here.
 			 */
-			return;
+			return 0;
 		}
 	}
+
+	return -ENODEV;
+}
+
+static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
+				    struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+
+	if (dev->of_node || !bus->dev.of_node)
+		return;
+
+	of_mdiobus_find_phy(dev, mdiodev, bus->dev.of_node);
 }
 #else /* !IS_ENABLED(CONFIG_OF_MDIO) */
 static inline void of_mdiobus_link_mdiodev(struct mii_bus *mdio,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 823b25bb3e3e..f416f7434697 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3174,6 +3174,36 @@ static int of_phy_leds(struct phy_device *phydev)
 	return 0;
 }
 
+static int of_phy_package(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *package_node;
+	u32 base_addr;
+	int ret;
+
+	if (!node)
+		return 0;
+
+	package_node = of_get_parent(node);
+	if (!package_node)
+		return 0;
+
+	if (!of_device_is_compatible(package_node, "ethernet-phy-package"))
+		return 0;
+
+	if (of_property_read_u32(package_node, "reg", &base_addr))
+		return -EINVAL;
+
+	ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
+				    base_addr, 0);
+	if (ret)
+		return ret;
+
+	phydev->shared->np = package_node;
+
+	return ret;
+}
+
 /**
  * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
  * @fwnode: pointer to the mdio_device's fwnode
@@ -3282,6 +3312,11 @@ static int phy_probe(struct device *dev)
 	if (phydrv->flags & PHY_IS_INTERNAL)
 		phydev->is_internal = true;
 
+	/* Parse DT to detect PHY package and join them */
+	err = of_phy_package(phydev);
+	if (err)
+		goto out;
+
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 342f750e8a30..80a4adaeb817 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -329,6 +329,7 @@ struct mdio_bus_stats {
  * struct phy_package_shared - Shared information in PHY packages
  * @base_addr: Base PHY address of PHY package used to combine PHYs
  *   in one package and for offset calculation of phy_package_read/write
+ * @np: Pointer to the Device Node if PHY package defined in DT
  * @refcnt: Number of PHYs connected to this shared data
  * @flags: Initialization of PHY package
  * @priv_size: Size of the shared private data @priv
@@ -340,6 +341,8 @@ struct mdio_bus_stats {
  */
 struct phy_package_shared {
 	int base_addr;
+	/* With PHY package defined in DT this points to the PHY package node */
+	struct device_node *np;
 	refcount_t refcnt;
 	unsigned long flags;
 	size_t priv_size;
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 3/8] net: phy: add support for shared priv data size for PHY package in DT
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 4/8] net: phy: add support for driver specific PHY package probe/config Christian Marangi
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

Add support for defining shared data size for PHY package defined in DT.

A PHY driver has to define the value .phy_package_priv_data_size to make
the generic OF PHY package function alloc priv data in the shared struct
for the PHY package.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_device.c | 7 ++++++-
 include/linux/phy.h          | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f416f7434697..87f06b4ecbfe 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3178,6 +3178,7 @@ static int of_phy_package(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
 	struct device_node *package_node;
+	int shared_priv_data_size;
 	u32 base_addr;
 	int ret;
 
@@ -3194,8 +3195,12 @@ static int of_phy_package(struct phy_device *phydev)
 	if (of_property_read_u32(package_node, "reg", &base_addr))
 		return -EINVAL;
 
+	shared_priv_data_size = 0;
+	if (phydev->drv->phy_package_priv_data_size)
+		shared_priv_data_size = phydev->drv->phy_package_priv_data_size;
+
 	ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
-				    base_addr, 0);
+				    base_addr, shared_priv_data_size);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 80a4adaeb817..c4e6d0b3a86c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -884,6 +884,8 @@ struct phy_led {
  * @flags: A bitfield defining certain other features this PHY
  *   supports (like interrupts)
  * @driver_data: Static driver data
+ * @phy_package_priv_data_size: Size of the priv data to alloc
+ *   for shared struct of PHY package.
  *
  * All functions are optional. If config_aneg or read_status
  * are not implemented, the phy core uses the genphy versions.
@@ -901,6 +903,7 @@ struct phy_driver {
 	const unsigned long * const features;
 	u32 flags;
 	const void *driver_data;
+	unsigned int phy_package_priv_data_size;
 
 	/**
 	 * @soft_reset: Called to issue a PHY software reset
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 4/8] net: phy: add support for driver specific PHY package probe/config
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
                   ` (2 preceding siblings ...)
  2023-11-26  1:53 ` [net-next PATCH RFC v3 3/8] net: phy: add support for shared priv data size " Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 5/8] dt-bindings: net: add QCA807x PHY defines Christian Marangi
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

Add PHY driver specific function to probe and configure PHY package.
These function are run only once before the PHY probe and config_init.

They are used in conjunction with DT PHY package define for basic PHY
package implementation to setup and probe PHY package with simple
functions directly defined in the PHY driver struct.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 include/linux/phy.h          | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 87f06b4ecbfe..d52eb4ff4dac 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1246,6 +1246,13 @@ int phy_init_hw(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	if (phydev->drv->phy_package_config_init_once &&
+	    phy_package_init_once(phydev)) {
+		ret = phydev->drv->phy_package_config_init_once(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (phydev->drv->config_init) {
 		ret = phydev->drv->config_init(phydev);
 		if (ret < 0)
@@ -3325,6 +3332,13 @@ static int phy_probe(struct device *dev)
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
+	if (phydev->drv->phy_package_probe_once &&
+	    phy_package_probe_once(phydev)) {
+		err = phydev->drv->phy_package_probe_once(phydev);
+		if (err)
+			goto out;
+	}
+
 	if (phydev->drv->probe) {
 		err = phydev->drv->probe(phydev);
 		if (err)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c4e6d0b3a86c..7df7a854fdeb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -910,12 +910,33 @@ struct phy_driver {
 	 */
 	int (*soft_reset)(struct phy_device *phydev);
 
+	/**
+	 * @phy_package_config_init_once: Driver specific PHY package
+	 *   config init call
+	 * @def: PHY device to use to probe PHY package
+	 *
+	 * Called to initialize the PHY package, including after
+	 * a reset.
+	 * Called BEFORE PHY config_init.
+	 */
+	int (*phy_package_config_init_once)(struct phy_device *dev);
+
 	/**
 	 * @config_init: Called to initialize the PHY,
 	 * including after a reset
 	 */
 	int (*config_init)(struct phy_device *phydev);
 
+	/**
+	 * @phy_package_probe_once: Driver specific PHY package probe
+	 * @def: PHY device to use to probe PHY package
+	 *
+	 * Called during discovery once per PHY package. Used to set
+	 * up device-specific PHY package structures, if any.
+	 * Called BEFORE PHY probe.
+	 */
+	int (*phy_package_probe_once)(struct phy_device *dev);
+
 	/**
 	 * @probe: Called during discovery.  Used to set
 	 * up device-specific structures, if any
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 5/8] dt-bindings: net: add QCA807x PHY defines
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
                   ` (3 preceding siblings ...)
  2023-11-26  1:53 ` [net-next PATCH RFC v3 4/8] net: phy: add support for driver specific PHY package probe/config Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-27 22:36   ` Rob Herring
  2023-11-26  1:53 ` [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

From: Robert Marko <robert.marko@sartura.hr>

Add DT bindings defined for Qualcomm QCA807x PHY series related to
calibration and DAC settings.

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 ++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 include/dt-bindings/net/qcom-qca807x.h

diff --git a/include/dt-bindings/net/qcom-qca807x.h b/include/dt-bindings/net/qcom-qca807x.h
new file mode 100644
index 000000000000..e7d4d09b7dd4
--- /dev/null
+++ b/include/dt-bindings/net/qcom-qca807x.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
+/*
+ * Device Tree constants for the Qualcomm QCA807X PHYs
+ */
+
+#ifndef _DT_BINDINGS_QCOM_QCA807X_H
+#define _DT_BINDINGS_QCOM_QCA807X_H
+
+/* Full amplitude, full bias current */
+#define QCA807X_CONTROL_DAC_FULL_VOLT_BIAS		0
+/* Amplitude follow DSP (amplitude is adjusted based on cable length), half bias current */
+#define QCA807X_CONTROL_DAC_DSP_VOLT_HALF_BIAS		1
+/* Full amplitude, bias current follow DSP (bias current is adjusted based on cable length) */
+#define QCA807X_CONTROL_DAC_FULL_VOLT_DSP_BIAS		2
+/* Both amplitude and bias current follow DSP */
+#define QCA807X_CONTROL_DAC_DSP_VOLT_BIAS		3
+/* Full amplitude, half bias current */
+#define QCA807X_CONTROL_DAC_FULL_VOLT_HALF_BIAS		4
+/* Amplitude follow DSP setting; 1/4 bias current when cable<10m,
+ * otherwise half bias current
+ */
+#define QCA807X_CONTROL_DAC_DSP_VOLT_QUARTER_BIAS	5
+/* Full amplitude; same bias current setting with “010” and “011”,
+ * but half more bias is reduced when cable <10m
+ */
+#define QCA807X_CONTROL_DAC_FULL_VOLT_HALF_BIAS_SHORT	6
+/* Amplitude follow DSP; same bias current setting with “110”, default value */
+#define QCA807X_CONTROL_DAC_DSP_VOLT_HALF_BIAS_SHORT	7
+
+#endif
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
                   ` (4 preceding siblings ...)
  2023-11-26  1:53 ` [net-next PATCH RFC v3 5/8] dt-bindings: net: add QCA807x PHY defines Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-27 22:35   ` Rob Herring
  2023-11-28  0:30   ` Andrew Lunn
  2023-11-26  1:53 ` [net-next PATCH RFC v3 7/8] net: phy: add Qualcom QCA807x driver Christian Marangi
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

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 | 136 ++++++++++++++++++
 1 file changed, 136 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..136ba2128b73
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
@@ -0,0 +1,136 @@
+# 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:
+  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
+
+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 = "ethernet-phy-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>;
+            };
+        };
+    };
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 7/8] net: phy: add Qualcom QCA807x driver
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
                   ` (5 preceding siblings ...)
  2023-11-26  1:53 ` [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-26  1:53 ` [net-next PATCH RFC v3 8/8] net: phy: qca807x: Add support for configurable LED Christian Marangi
  2023-11-28  0:20 ` [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Andrew Lunn
  8 siblings, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

From: Robert Marko <robert.marko@sartura.hr>

This adds driver for the Qualcomm QCA8072 and QCA8075 PHY-s.

They are 2 or 5 port 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.

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

Each PHY inside of QCA807x series has 4 digitally controlled
output only pins that natively drive LED-s.
But some vendors used these to driver generic LED-s controlled
by userspace, so lets enable registering each PHY as GPIO
controller and add driver for it.

These are commonly used in Qualcomm IPQ40xx, IPQ60xx and IPQ807x
boards.

Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/Kconfig   |   7 +
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/qca807x.c | 947 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 955 insertions(+)
 create mode 100644 drivers/net/phy/qca807x.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 25cfc5ded1da..5ad85bd978a0 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -325,6 +325,13 @@ config AT803X_PHY
 	  Currently supports the AR8030, AR8031, AR8033, AR8035 and internal
 	  QCA8337(Internal qca8k PHY) model
 
+config QCA807X_PHY
+	tristate "Qualcomm QCA807x PHYs"
+	depends on OF_MDIO
+	help
+	  Currently supports the Qualcomm QCA8072, QCA8075 and the PSGMII
+	  control PHY.
+
 config QSEMI_PHY
 	tristate "Quality Semiconductor PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f65e85c91fc1..a4da4f127b23 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_NCN26000_PHY)	+= ncn26000.o
 obj-$(CONFIG_NXP_C45_TJA11XX_PHY)	+= nxp-c45-tja11xx.o
 obj-$(CONFIG_NXP_CBTX_PHY)	+= nxp-cbtx.o
 obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
+obj-$(CONFIG_QCA807X_PHY)		+= qca807x.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
diff --git a/drivers/net/phy/qca807x.c b/drivers/net/phy/qca807x.c
new file mode 100644
index 000000000000..be039d6de1fb
--- /dev/null
+++ b/drivers/net/phy/qca807x.c
@@ -0,0 +1,947 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Sartura Ltd.
+ *
+ * Author: Robert Marko <robert.marko@sartura.hr>
+ *         Christian Marangi <ansuelsmth@gmail.com>
+ *
+ * Qualcomm QCA8072 and QCA8075 PHY driver
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/bitfield.h>
+#include <linux/ethtool_netlink.h>
+#include <linux/gpio/driver.h>
+#include <linux/sfp.h>
+
+#include <dt-bindings/net/qcom-qca807x.h>
+
+#define PHY_ID_QCA8072		0x004dd0b2
+#define PHY_ID_QCA8075		0x004dd0b1
+
+/* Downshift */
+#define QCA807X_SMARTSPEED_EN			BIT(5)
+#define QCA807X_SMARTSPEED_RETRY_LIMIT_MASK	GENMASK(4, 2)
+#define QCA807X_SMARTSPEED_RETRY_LIMIT_DEFAULT	5
+#define QCA807X_SMARTSPEED_RETRY_LIMIT_MIN	2
+#define QCA807X_SMARTSPEED_RETRY_LIMIT_MAX	9
+
+/* Cable diagnostic test (CDT) */
+#define QCA807X_CDT						0x16
+#define QCA807X_CDT_ENABLE					BIT(15)
+#define QCA807X_CDT_ENABLE_INTER_PAIR_SHORT			BIT(13)
+#define QCA807X_CDT_STATUS					BIT(11)
+#define QCA807X_CDT_MMD3_STATUS					0x8064
+#define QCA807X_CDT_MDI0_STATUS_MASK				GENMASK(15, 12)
+#define QCA807X_CDT_MDI1_STATUS_MASK				GENMASK(11, 8)
+#define QCA807X_CDT_MDI2_STATUS_MASK				GENMASK(7, 4)
+#define QCA807X_CDT_MDI3_STATUS_MASK				GENMASK(3, 0)
+#define QCA807X_CDT_RESULTS_INVALID				0x0
+#define QCA807X_CDT_RESULTS_OK					0x1
+#define QCA807X_CDT_RESULTS_OPEN				0x2
+#define QCA807X_CDT_RESULTS_SAME_SHORT				0x3
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OK	0x4
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OK	0x8
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OK	0xc
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OPEN	0x6
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OPEN	0xa
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OPEN	0xe
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_SHORT	0x7
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_SHORT	0xb
+#define QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_SHORT	0xf
+#define QCA807X_CDT_RESULTS_BUSY				0x9
+#define QCA807X_CDT_MMD3_MDI0_LENGTH				0x8065
+#define QCA807X_CDT_MMD3_MDI1_LENGTH				0x8066
+#define QCA807X_CDT_MMD3_MDI2_LENGTH				0x8067
+#define QCA807X_CDT_MMD3_MDI3_LENGTH				0x8068
+#define QCA807X_CDT_SAME_SHORT_LENGTH_MASK			GENMASK(15, 8)
+#define QCA807X_CDT_CROSS_SHORT_LENGTH_MASK			GENMASK(7, 0)
+
+#define QCA807X_CHIP_CONFIGURATION				0x1f
+#define QCA807X_BT_BX_REG_SEL					BIT(15)
+#define QCA807X_BT_BX_REG_SEL_FIBER				0
+#define QCA807X_BT_BX_REG_SEL_COPPER				1
+#define QCA807X_CHIP_CONFIGURATION_MODE_CFG_MASK		GENMASK(3, 0)
+#define QCA807X_CHIP_CONFIGURATION_MODE_QSGMII_SGMII		4
+#define QCA807X_CHIP_CONFIGURATION_MODE_PSGMII_FIBER		3
+#define QCA807X_CHIP_CONFIGURATION_MODE_PSGMII_ALL_COPPER	0
+
+#define QCA807X_MEDIA_SELECT_STATUS				0x1a
+#define QCA807X_MEDIA_DETECTED_COPPER				BIT(5)
+#define QCA807X_MEDIA_DETECTED_1000_BASE_X			BIT(4)
+#define QCA807X_MEDIA_DETECTED_100_BASE_FX			BIT(3)
+
+#define QCA807X_MMD7_FIBER_MODE_AUTO_DETECTION			0x807e
+#define QCA807X_MMD7_FIBER_MODE_AUTO_DETECTION_EN		BIT(0)
+
+#define QCA807X_MMD7_1000BASE_T_POWER_SAVE_PER_CABLE_LENGTH	0x801a
+#define QCA807X_CONTROL_DAC_MASK				GENMASK(2, 0)
+
+#define QCA807X_MMD7_LED_100N_1				0x8074
+#define QCA807X_MMD7_LED_100N_2				0x8075
+#define QCA807X_MMD7_LED_1000N_1			0x8076
+#define QCA807X_MMD7_LED_1000N_2			0x8077
+#define QCA807X_LED_TXACT_BLK_EN_2			BIT(10)
+#define QCA807X_LED_RXACT_BLK_EN_2			BIT(9)
+#define QCA807X_LED_GT_ON_EN_2				BIT(6)
+#define QCA807X_LED_HT_ON_EN_2				BIT(5)
+#define QCA807X_LED_BT_ON_EN_2				BIT(4)
+#define QCA807X_GPIO_FORCE_EN				BIT(15)
+#define QCA807X_GPIO_FORCE_MODE_MASK			GENMASK(14, 13)
+
+#define QCA807X_INTR_ENABLE				0x12
+#define QCA807X_INTR_STATUS				0x13
+#define QCA807X_INTR_ENABLE_AUTONEG_ERR			BIT(15)
+#define QCA807X_INTR_ENABLE_SPEED_CHANGED		BIT(14)
+#define QCA807X_INTR_ENABLE_DUPLEX_CHANGED		BIT(13)
+#define QCA807X_INTR_ENABLE_LINK_FAIL			BIT(11)
+#define QCA807X_INTR_ENABLE_LINK_SUCCESS		BIT(10)
+
+#define QCA807X_FUNCTION_CONTROL			0x10
+#define QCA807X_FC_MDI_CROSSOVER_MODE_MASK		GENMASK(6, 5)
+#define QCA807X_FC_MDI_CROSSOVER_AUTO			3
+#define QCA807X_FC_MDI_CROSSOVER_MANUAL_MDIX		1
+#define QCA807X_FC_MDI_CROSSOVER_MANUAL_MDI		0
+
+#define QCA807X_PHY_SPECIFIC_STATUS			0x11
+#define QCA807X_SS_SPEED_AND_DUPLEX_RESOLVED		BIT(11)
+#define QCA807X_SS_SPEED_MASK				GENMASK(15, 14)
+#define QCA807X_SS_SPEED_1000				2
+#define QCA807X_SS_SPEED_100				1
+#define QCA807X_SS_SPEED_10				0
+#define QCA807X_SS_DUPLEX				BIT(13)
+#define QCA807X_SS_MDIX					BIT(6)
+
+/* PQSGMII Analog PHY specific */
+#define PQSGMII_CTRL_REG				0x0
+#define PQSGMII_ANALOG_SW_RESET				BIT(6)
+#define PQSGMII_DRIVE_CONTROL_1				0xb
+#define PQSGMII_TX_DRIVER_MASK				GENMASK(7, 4)
+#define PQSGMII_TX_DRIVER_140MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x0)
+#define PQSGMII_TX_DRIVER_160MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x1)
+#define PQSGMII_TX_DRIVER_180MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x2)
+#define PQSGMII_TX_DRIVER_200MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x3)
+#define PQSGMII_TX_DRIVER_220MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x4)
+#define PQSGMII_TX_DRIVER_240MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x5)
+#define PQSGMII_TX_DRIVER_260MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x6)
+#define PQSGMII_TX_DRIVER_280MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x7)
+#define PQSGMII_TX_DRIVER_300MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x8)
+#define PQSGMII_TX_DRIVER_320MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0x9)
+#define PQSGMII_TX_DRIVER_400MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0xa)
+#define PQSGMII_TX_DRIVER_500MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0xb)
+#define PQSGMII_TX_DRIVER_600MV				FIELD_PREP(PQSGMII_TX_DRIVER_MASK, 0xc)
+#define PQSGMII_MODE_CTRL				0x6d
+#define PQSGMII_MODE_CTRL_AZ_WORKAROUND_MASK		BIT(0)
+#define PQSGMII_MMD3_SERDES_CONTROL			0x805a
+
+#define QCA807X_COMBO_ADDR_OFFSET			4
+#define QCA807X_PQSGMII_ADDR_OFFSET			5
+#define SERDES_RESET_SLEEP				100
+
+enum qca807x_global_phy {
+	QCA807X_COMBO_ADDR = 4,
+	QCA807X_PQSGMII_ADDR = 5,
+};
+
+struct qca807x_shared_priv {
+	unsigned int package_mode;
+	u32 tx_driver_strength;
+};
+
+struct qca807x_gpio_priv {
+	struct phy_device *phy;
+};
+
+static int qca807x_get_downshift(struct phy_device *phydev, u8 *data)
+{
+	int val, cnt, enable;
+
+	val = phy_read(phydev, MII_NWAYTEST);
+	if (val < 0)
+		return val;
+
+	enable = FIELD_GET(QCA807X_SMARTSPEED_EN, val);
+	cnt = FIELD_GET(QCA807X_SMARTSPEED_RETRY_LIMIT_MASK, val) + 2;
+
+	*data = enable ? cnt : DOWNSHIFT_DEV_DISABLE;
+
+	return 0;
+}
+
+static int qca807x_set_downshift(struct phy_device *phydev, u8 cnt)
+{
+	int ret, val;
+
+	if (cnt > QCA807X_SMARTSPEED_RETRY_LIMIT_MAX ||
+	    (cnt < QCA807X_SMARTSPEED_RETRY_LIMIT_MIN && cnt != DOWNSHIFT_DEV_DISABLE))
+		return -EINVAL;
+
+	if (!cnt) {
+		ret = phy_clear_bits(phydev, MII_NWAYTEST, QCA807X_SMARTSPEED_EN);
+	} else {
+		val = QCA807X_SMARTSPEED_EN;
+		val |= FIELD_PREP(QCA807X_SMARTSPEED_RETRY_LIMIT_MASK, cnt - 2);
+
+		phy_modify(phydev, MII_NWAYTEST,
+			   QCA807X_SMARTSPEED_EN |
+			   QCA807X_SMARTSPEED_RETRY_LIMIT_MASK,
+			   val);
+	}
+
+	ret = genphy_soft_reset(phydev);
+
+	return ret;
+}
+
+static int qca807x_get_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return qca807x_get_downshift(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int qca807x_set_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return qca807x_set_downshift(phydev, *(const u8 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static bool qca807x_distance_valid(int result)
+{
+	switch (result) {
+	case QCA807X_CDT_RESULTS_OPEN:
+	case QCA807X_CDT_RESULTS_SAME_SHORT:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OK:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OK:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OK:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int qca807x_report_length(struct phy_device *phydev,
+				 int pair, int result)
+{
+	int length;
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA807X_CDT_MMD3_MDI0_LENGTH + pair);
+	if (ret < 0)
+		return ret;
+
+	switch (result) {
+	case ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT:
+		length = (FIELD_GET(QCA807X_CDT_SAME_SHORT_LENGTH_MASK, ret) * 800) / 10;
+		break;
+	case ETHTOOL_A_CABLE_RESULT_CODE_OPEN:
+	case ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT:
+		length = (FIELD_GET(QCA807X_CDT_CROSS_SHORT_LENGTH_MASK, ret) * 800) / 10;
+		break;
+	}
+
+	ethnl_cable_test_fault_length(phydev, pair, length);
+
+	return 0;
+}
+
+static int qca807x_cable_test_report_trans(int result)
+{
+	switch (result) {
+	case QCA807X_CDT_RESULTS_OK:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case QCA807X_CDT_RESULTS_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case QCA807X_CDT_RESULTS_SAME_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OK:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OK:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OK:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
+	case QCA807X_CDT_RESULTS_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static int qca807x_cable_test_report(struct phy_device *phydev)
+{
+	int pair0, pair1, pair2, pair3;
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA807X_CDT_MMD3_STATUS);
+	if (ret < 0)
+		return ret;
+
+	pair0 = FIELD_GET(QCA807X_CDT_MDI0_STATUS_MASK, ret);
+	pair1 = FIELD_GET(QCA807X_CDT_MDI1_STATUS_MASK, ret);
+	pair2 = FIELD_GET(QCA807X_CDT_MDI2_STATUS_MASK, ret);
+	pair3 = FIELD_GET(QCA807X_CDT_MDI3_STATUS_MASK, ret);
+
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				qca807x_cable_test_report_trans(pair0));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				qca807x_cable_test_report_trans(pair1));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				qca807x_cable_test_report_trans(pair2));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				qca807x_cable_test_report_trans(pair3));
+
+	if (qca807x_distance_valid(pair0))
+		qca807x_report_length(phydev, 0, qca807x_cable_test_report_trans(pair0));
+	if (qca807x_distance_valid(pair1))
+		qca807x_report_length(phydev, 1, qca807x_cable_test_report_trans(pair1));
+	if (qca807x_distance_valid(pair2))
+		qca807x_report_length(phydev, 2, qca807x_cable_test_report_trans(pair2));
+	if (qca807x_distance_valid(pair3))
+		qca807x_report_length(phydev, 3, qca807x_cable_test_report_trans(pair3));
+
+	return 0;
+}
+
+static int qca807x_cable_test_get_status(struct phy_device *phydev,
+					 bool *finished)
+{
+	int val;
+
+	*finished = false;
+
+	val = phy_read(phydev, QCA807X_CDT);
+	if (!((val & QCA807X_CDT_ENABLE) && (val & QCA807X_CDT_STATUS))) {
+		*finished = true;
+
+		return qca807x_cable_test_report(phydev);
+	}
+
+	return 0;
+}
+
+static int qca807x_cable_test_start(struct phy_device *phydev)
+{
+	int val, ret;
+
+	val = phy_read(phydev, QCA807X_CDT);
+	/* Enable inter-pair short check as well */
+	val &= ~QCA807X_CDT_ENABLE_INTER_PAIR_SHORT;
+	val |= QCA807X_CDT_ENABLE;
+	ret = phy_write(phydev, QCA807X_CDT, val);
+
+	return ret;
+}
+
+#ifdef CONFIG_GPIOLIB
+static int qca807x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int qca807x_gpio_get_reg(unsigned int offset)
+{
+	return QCA807X_MMD7_LED_100N_2 + (offset % 2) * 2;
+}
+
+static int qca807x_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct qca807x_gpio_priv *priv = gpiochip_get_data(gc);
+	int val;
+
+	val = phy_read_mmd(priv->phy, MDIO_MMD_AN, qca807x_gpio_get_reg(offset));
+
+	return FIELD_GET(QCA807X_GPIO_FORCE_MODE_MASK, val);
+}
+
+static void qca807x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct qca807x_gpio_priv *priv = gpiochip_get_data(gc);
+	int val;
+
+	val = phy_read_mmd(priv->phy, MDIO_MMD_AN, qca807x_gpio_get_reg(offset));
+	val &= ~QCA807X_GPIO_FORCE_MODE_MASK;
+	val |= QCA807X_GPIO_FORCE_EN;
+	val |= FIELD_PREP(QCA807X_GPIO_FORCE_MODE_MASK, value);
+
+	phy_write_mmd(priv->phy, MDIO_MMD_AN, qca807x_gpio_get_reg(offset), val);
+}
+
+static int qca807x_gpio_dir_out(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	qca807x_gpio_set(gc, offset, value);
+
+	return 0;
+}
+
+static int qca807x_gpio(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct qca807x_gpio_priv *priv;
+	struct gpio_chip *gc;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->phy = phydev;
+
+	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
+	if (!gc)
+		return -ENOMEM;
+
+	gc->label = dev_name(dev);
+	gc->base = -1;
+	gc->ngpio = 2;
+	gc->parent = dev;
+	gc->owner = THIS_MODULE;
+	gc->can_sleep = true;
+	gc->get_direction = qca807x_gpio_get_direction;
+	gc->direction_output = qca807x_gpio_dir_out;
+	gc->get = qca807x_gpio_get;
+	gc->set = qca807x_gpio_set;
+
+	return devm_gpiochip_add_data(dev, gc, priv);
+}
+#endif
+
+static int qca807x_read_copper_status(struct phy_device *phydev)
+{
+	int ss, err, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	err = genphy_read_lpa(phydev);
+	if (err < 0)
+		return err;
+
+	/* Read the QCA807x PHY-Specific Status register copper page,
+	 * which indicates the speed and duplex that the PHY is actually
+	 * using, irrespective of whether we are in autoneg mode or not.
+	 */
+	ss = phy_read(phydev, QCA807X_PHY_SPECIFIC_STATUS);
+	if (ss < 0)
+		return ss;
+
+	if (ss & QCA807X_SS_SPEED_AND_DUPLEX_RESOLVED) {
+		int sfc;
+
+		sfc = phy_read(phydev, QCA807X_FUNCTION_CONTROL);
+		if (sfc < 0)
+			return sfc;
+
+		switch (FIELD_GET(QCA807X_SS_SPEED_MASK, ss)) {
+		case QCA807X_SS_SPEED_10:
+			phydev->speed = SPEED_10;
+			break;
+		case QCA807X_SS_SPEED_100:
+			phydev->speed = SPEED_100;
+			break;
+		case QCA807X_SS_SPEED_1000:
+			phydev->speed = SPEED_1000;
+			break;
+		}
+		if (ss & QCA807X_SS_DUPLEX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+
+		if (ss & QCA807X_SS_MDIX)
+			phydev->mdix = ETH_TP_MDI_X;
+		else
+			phydev->mdix = ETH_TP_MDI;
+
+		switch (FIELD_GET(QCA807X_FC_MDI_CROSSOVER_MODE_MASK, sfc)) {
+		case QCA807X_FC_MDI_CROSSOVER_MANUAL_MDI:
+			phydev->mdix_ctrl = ETH_TP_MDI;
+			break;
+		case QCA807X_FC_MDI_CROSSOVER_MANUAL_MDIX:
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+			break;
+		case QCA807X_FC_MDI_CROSSOVER_AUTO:
+			phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+			break;
+		}
+	}
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
+		phy_resolve_aneg_pause(phydev);
+
+	return 0;
+}
+
+static int qca807x_read_fiber_status(struct phy_device *phydev)
+{
+	int ss, err, lpa, old_link = phydev->link;
+
+	/* Update the link, but return if there was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	/* why bother the PHY if nothing can have changed */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		return 0;
+
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+		lpa = phy_read(phydev, MII_LPA);
+		if (lpa < 0)
+			return lpa;
+
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+				 phydev->lp_advertising, lpa & LPA_LPACK);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				 phydev->lp_advertising, lpa & LPA_1000XFULL);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				 phydev->lp_advertising, lpa & LPA_1000XPAUSE);
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				 phydev->lp_advertising,
+				 lpa & LPA_1000XPAUSE_ASYM);
+
+		phy_resolve_aneg_linkmode(phydev);
+	}
+
+	/* Read the QCA807x PHY-Specific Status register fiber page,
+	 * which indicates the speed and duplex that the PHY is actually
+	 * using, irrespective of whether we are in autoneg mode or not.
+	 */
+	ss = phy_read(phydev, QCA807X_PHY_SPECIFIC_STATUS);
+	if (ss < 0)
+		return ss;
+
+	if (ss & QCA807X_SS_SPEED_AND_DUPLEX_RESOLVED) {
+		switch (FIELD_GET(QCA807X_SS_SPEED_MASK, ss)) {
+		case QCA807X_SS_SPEED_100:
+			phydev->speed = SPEED_100;
+			break;
+		case QCA807X_SS_SPEED_1000:
+			phydev->speed = SPEED_1000;
+			break;
+		}
+
+		if (ss & QCA807X_SS_DUPLEX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+	}
+
+	return 0;
+}
+
+static int qca807x_read_status(struct phy_device *phydev)
+{
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported)) {
+		switch (phydev->port) {
+		case PORT_FIBRE:
+			return qca807x_read_fiber_status(phydev);
+		case PORT_TP:
+			return qca807x_read_copper_status(phydev);
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return qca807x_read_copper_status(phydev);
+}
+
+static int qca807x_config_intr(struct phy_device *phydev)
+{
+	int ret, val;
+
+	val = phy_read(phydev, QCA807X_INTR_ENABLE);
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/* Check for combo port as it has fewer interrupts */
+		if (phy_read(phydev, QCA807X_CHIP_CONFIGURATION)) {
+			val |= QCA807X_INTR_ENABLE_SPEED_CHANGED;
+			val |= QCA807X_INTR_ENABLE_LINK_FAIL;
+			val |= QCA807X_INTR_ENABLE_LINK_SUCCESS;
+		} else {
+			val |= QCA807X_INTR_ENABLE_AUTONEG_ERR;
+			val |= QCA807X_INTR_ENABLE_SPEED_CHANGED;
+			val |= QCA807X_INTR_ENABLE_DUPLEX_CHANGED;
+			val |= QCA807X_INTR_ENABLE_LINK_FAIL;
+			val |= QCA807X_INTR_ENABLE_LINK_SUCCESS;
+		}
+		ret = phy_write(phydev, QCA807X_INTR_ENABLE, val);
+	} else {
+		ret = phy_write(phydev, QCA807X_INTR_ENABLE, 0);
+	}
+
+	return ret;
+}
+
+static irqreturn_t qca807x_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status, int_enabled;
+
+	irq_status = phy_read(phydev, QCA807X_INTR_STATUS);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* Read the current enabled interrupts */
+	int_enabled = phy_read(phydev, QCA807X_INTR_ENABLE);
+	if (int_enabled < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* See if this was one of our enabled interrupts */
+	if (!(irq_status & int_enabled))
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
+static int qca807x_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
+{
+	struct phy_device *phydev = upstream;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
+	phy_interface_t iface;
+	int ret;
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+
+	sfp_parse_support(phydev->sfp_bus, id, support, interfaces);
+	iface = sfp_select_interface(phydev->sfp_bus, support);
+
+	dev_info(&phydev->mdio.dev, "%s SFP module inserted\n", phy_modes(iface));
+
+	switch (iface) {
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_100BASEX:
+		/* Set PHY mode to PSGMII combo (1/4 copper + combo ports) mode */
+		ret = phy_modify(phydev,
+				 QCA807X_CHIP_CONFIGURATION,
+				 QCA807X_CHIP_CONFIGURATION_MODE_CFG_MASK,
+				 QCA807X_CHIP_CONFIGURATION_MODE_PSGMII_FIBER);
+		/* Enable fiber mode autodection (1000Base-X or 100Base-FX) */
+		ret = phy_set_bits_mmd(phydev,
+				       MDIO_MMD_AN,
+				       QCA807X_MMD7_FIBER_MODE_AUTO_DETECTION,
+				       QCA807X_MMD7_FIBER_MODE_AUTO_DETECTION_EN);
+		/* Select fiber page */
+		ret = phy_clear_bits(phydev,
+				     QCA807X_CHIP_CONFIGURATION,
+				     QCA807X_BT_BX_REG_SEL);
+
+		phydev->port = PORT_FIBRE;
+		break;
+	default:
+		dev_err(&phydev->mdio.dev, "Incompatible SFP module inserted\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static void qca807x_sfp_remove(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+
+	/* Select copper page */
+	phy_set_bits(phydev,
+		     QCA807X_CHIP_CONFIGURATION,
+		     QCA807X_BT_BX_REG_SEL);
+
+	phydev->port = PORT_TP;
+}
+
+static const struct sfp_upstream_ops qca807x_sfp_ops = {
+	.attach = phy_sfp_attach,
+	.detach = phy_sfp_detach,
+	.module_insert = qca807x_sfp_insert,
+	.module_remove = qca807x_sfp_remove,
+};
+
+static int qca807x_config(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	int control_dac, ret = 0;
+	u32 of_control_dac;
+
+	if (of_property_read_u32(node, "qcom,control-dac", &of_control_dac))
+		of_control_dac = QCA807X_CONTROL_DAC_DSP_VOLT_QUARTER_BIAS;
+
+	control_dac = phy_read_mmd(phydev, MDIO_MMD_AN,
+				   QCA807X_MMD7_1000BASE_T_POWER_SAVE_PER_CABLE_LENGTH);
+	control_dac &= ~QCA807X_CONTROL_DAC_MASK;
+	control_dac |= FIELD_PREP(QCA807X_CONTROL_DAC_MASK, of_control_dac);
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN,
+			    QCA807X_MMD7_1000BASE_T_POWER_SAVE_PER_CABLE_LENGTH,
+			    control_dac);
+
+	return ret;
+}
+
+static int qca807x_probe(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct phy_package_shared *shared = phydev->shared;
+	int ret = 0;
+
+	if (shared) {
+		struct qca807x_shared_priv *shared_priv = shared->priv;
+
+		/* Make sure PHY follow PHY package mode if enforced */
+		if (shared_priv &&
+		    shared_priv->package_mode != PHY_INTERFACE_MODE_NA &&
+		    phydev->interface != shared_priv->package_mode)
+			return -EINVAL;
+	}
+
+	if (IS_ENABLED(CONFIG_GPIOLIB)) {
+		if (of_find_property(node, "leds", NULL) &&
+		    of_find_property(node, "gpio-controller", NULL)) {
+			phydev_err(phydev, "Invalid property detected. LEDs and gpio-controller are mutually exclusive.");
+			return -EINVAL;
+		}
+
+		/* Do not register a GPIO controller unless flagged for it */
+		if (of_property_read_bool(node, "gpio-controller")) {
+			ret = qca807x_gpio(phydev);
+			if (ret)
+				return ret;
+		}
+	}
+
+	/* Attach SFP bus on combo port*/
+	if (phy_read(phydev, QCA807X_CHIP_CONFIGURATION)) {
+		ret = phy_sfp_probe(phydev, &qca807x_sfp_ops);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->advertising);
+	}
+
+	return ret;
+}
+
+static int qca807x_phy_package_probe_once(struct phy_device *phydev)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	struct qca807x_shared_priv *priv = shared->priv;
+	unsigned int tx_driver_strength = 0;
+	const char *package_mode_name;
+
+	of_property_read_u32(shared->np, "qcom,tx-driver-strength",
+			     &tx_driver_strength);
+	switch (tx_driver_strength) {
+	case 140:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_140MV;
+		break;
+	case 160:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_160MV;
+		break;
+	case 180:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_180MV;
+		break;
+	case 200:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_200MV;
+		break;
+	case 220:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_220MV;
+		break;
+	case 240:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_240MV;
+		break;
+	case 260:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_260MV;
+		break;
+	case 280:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_280MV;
+		break;
+	case 300:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_300MV;
+		break;
+	case 320:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_320MV;
+		break;
+	case 400:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_400MV;
+		break;
+	case 500:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_500MV;
+		break;
+	case 600:
+	default:
+		priv->tx_driver_strength = PQSGMII_TX_DRIVER_600MV;
+	}
+
+	priv->package_mode = PHY_INTERFACE_MODE_NA;
+	if (!of_property_read_string(shared->np, "qcom,package-mode",
+				     &package_mode_name)) {
+		if (!strcasecmp(package_mode_name,
+				phy_modes(PHY_INTERFACE_MODE_PSGMII)))
+			priv->package_mode = PHY_INTERFACE_MODE_PSGMII;
+
+		if (!strcasecmp(package_mode_name,
+				phy_modes(PHY_INTERFACE_MODE_QSGMII)))
+			priv->package_mode = PHY_INTERFACE_MODE_QSGMII;
+	}
+
+	return 0;
+}
+
+static int qca807x_phy_package_config_init_once(struct phy_device *phydev)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	struct qca807x_shared_priv *priv = shared->priv;
+	int val, ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	/* Set correct PHY package mode */
+	val = __phy_package_read(phydev, QCA807X_COMBO_ADDR,
+				 QCA807X_CHIP_CONFIGURATION);
+	val &= ~QCA807X_CHIP_CONFIGURATION_MODE_CFG_MASK;
+	if (priv->package_mode == PHY_INTERFACE_MODE_QSGMII)
+		val |= QCA807X_CHIP_CONFIGURATION_MODE_QSGMII_SGMII;
+	else
+		val |= QCA807X_CHIP_CONFIGURATION_MODE_PSGMII_ALL_COPPER;
+	ret = __phy_package_write(phydev, QCA807X_COMBO_ADDR,
+				  QCA807X_CHIP_CONFIGURATION, val);
+	if (ret)
+		goto exit;
+
+	/* After mode change Serdes reset is required */
+	val = __phy_package_read(phydev, QCA807X_PQSGMII_ADDR,
+				 PQSGMII_CTRL_REG);
+	val &= ~PQSGMII_ANALOG_SW_RESET;
+	ret = __phy_package_write(phydev, QCA807X_PQSGMII_ADDR,
+				  PQSGMII_CTRL_REG, val);
+	if (ret)
+		goto exit;
+
+	msleep(SERDES_RESET_SLEEP);
+
+	val = __phy_package_read(phydev, QCA807X_PQSGMII_ADDR,
+				 PQSGMII_CTRL_REG);
+	val |= PQSGMII_ANALOG_SW_RESET;
+	ret = __phy_package_write(phydev, QCA807X_PQSGMII_ADDR,
+				  PQSGMII_CTRL_REG, val);
+	if (ret)
+		goto exit;
+
+	/* Workaround to enable AZ transmitting ability */
+	val = __phy_package_read_mmd(phydev, QCA807X_PQSGMII_ADDR,
+				     MDIO_MMD_PMAPMD, PQSGMII_MODE_CTRL);
+	val &= ~PQSGMII_MODE_CTRL_AZ_WORKAROUND_MASK;
+	ret = __phy_package_write_mmd(phydev, QCA807X_PQSGMII_ADDR,
+				      MDIO_MMD_PMAPMD, PQSGMII_MODE_CTRL, val);
+	if (ret)
+		goto exit;
+
+	/* Set PQSGMII TX AMP strength */
+	val = __phy_package_read(phydev, QCA807X_PQSGMII_ADDR,
+				 PQSGMII_DRIVE_CONTROL_1);
+	val &= ~PQSGMII_TX_DRIVER_MASK;
+	val |= FIELD_PREP(PQSGMII_TX_DRIVER_MASK, priv->tx_driver_strength);
+	ret = __phy_package_write(phydev, QCA807X_PQSGMII_ADDR,
+				  PQSGMII_DRIVE_CONTROL_1, val);
+	if (ret)
+		goto exit;
+
+	/* Prevent PSGMII going into hibernation via PSGMII self test */
+	val = __phy_package_read_mmd(phydev, QCA807X_COMBO_ADDR,
+				     MDIO_MMD_PCS, PQSGMII_MMD3_SERDES_CONTROL);
+	val &= ~BIT(1);
+	ret = __phy_package_write_mmd(phydev, QCA807X_COMBO_ADDR,
+				      MDIO_MMD_PCS, PQSGMII_MMD3_SERDES_CONTROL, val);
+
+exit:
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
+static struct phy_driver qca807x_drivers[] = {
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_QCA8072),
+		.name           = "Qualcomm QCA8072",
+		.flags		= PHY_POLL_CABLE_TEST,
+		/* PHY_GBIT_FEATURES */
+		.probe		= qca807x_probe,
+		.config_init	= qca807x_config,
+		.read_status	= qca807x_read_status,
+		.config_intr	= qca807x_config_intr,
+		.handle_interrupt = qca807x_handle_interrupt,
+		.soft_reset	= genphy_soft_reset,
+		.get_tunable	= qca807x_get_tunable,
+		.set_tunable	= qca807x_set_tunable,
+		.resume		= genphy_resume,
+		.suspend	= genphy_suspend,
+		.cable_test_start	= qca807x_cable_test_start,
+		.cable_test_get_status	= qca807x_cable_test_get_status,
+	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_QCA8075),
+		.name           = "Qualcomm QCA8075",
+		.flags		= PHY_POLL_CABLE_TEST,
+		/* PHY_GBIT_FEATURES */
+		.probe		= qca807x_probe,
+		.config_init	= qca807x_config,
+		.read_status	= qca807x_read_status,
+		.config_intr	= qca807x_config_intr,
+		.handle_interrupt = qca807x_handle_interrupt,
+		.soft_reset	= genphy_soft_reset,
+		.get_tunable	= qca807x_get_tunable,
+		.set_tunable	= qca807x_set_tunable,
+		.resume		= genphy_resume,
+		.suspend	= genphy_suspend,
+		.cable_test_start	= qca807x_cable_test_start,
+		.cable_test_get_status	= qca807x_cable_test_get_status,
+		/* PHY package define */
+		.phy_package_priv_data_size = sizeof(struct qca807x_shared_priv),
+		.phy_package_probe_once = qca807x_phy_package_probe_once,
+		.phy_package_config_init_once = qca807x_phy_package_config_init_once,
+	},
+};
+module_phy_driver(qca807x_drivers);
+
+static struct mdio_device_id __maybe_unused qca807x_tbl[] = {
+	{ PHY_ID_MATCH_EXACT(PHY_ID_QCA8072) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_QCA8075) },
+	{ }
+};
+
+MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
+MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
+MODULE_DESCRIPTION("Qualcomm QCA807x PHY driver");
+MODULE_DEVICE_TABLE(mdio, qca807x_tbl);
+MODULE_LICENSE("GPL");
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [net-next PATCH RFC v3 8/8] net: phy: qca807x: Add support for configurable LED
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
                   ` (6 preceding siblings ...)
  2023-11-26  1:53 ` [net-next PATCH RFC v3 7/8] net: phy: add Qualcom QCA807x driver Christian Marangi
@ 2023-11-26  1:53 ` Christian Marangi
  2023-11-28  0:20 ` [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Andrew Lunn
  8 siblings, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-26  1:53 UTC (permalink / raw)
  To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	Christian Marangi, Robert Marko, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

QCA8072/5 have up to 2 LEDs attached for PHY.

LEDs can be configured to be ON/hw blink or be set to HW control.

Hw blink mode is set to blink at 4Hz or 250ms.

PHY can support both copper (TP) or fiber (FIBRE) kind and supports
different HW control modes based on the port type.

HW control modes supported for netdev trigger for copper ports are:
- LINK_10
- LINK_100
- LINK_1000
- TX
- RX
- FULL_DUPLEX
- HALF_DUPLEX

HW control modes supported for netdev trigger for fiber ports are:
- LINK_100
- LINK_1000
- TX
- RX
- FULL_DUPLEX
- HALF_DUPLEX

LED support conflicts with GPIO controller feature and must be disabled
if gpio-controller is used for the PHY.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/qca807x.c | 382 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 375 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/qca807x.c b/drivers/net/phy/qca807x.c
index be039d6de1fb..3c11b39ebe7e 100644
--- a/drivers/net/phy/qca807x.c
+++ b/drivers/net/phy/qca807x.c
@@ -79,17 +79,60 @@
 #define QCA807X_MMD7_1000BASE_T_POWER_SAVE_PER_CABLE_LENGTH	0x801a
 #define QCA807X_CONTROL_DAC_MASK				GENMASK(2, 0)
 
+#define QCA807X_MMD7_LED_GLOBAL				0x8073
+#define QCA807X_LED_BLINK_1				GENMASK(11, 6)
+#define QCA807X_LED_BLINK_2				GENMASK(5, 0)
+/* Values are the same for both BLINK_1 and BLINK_2 */
+#define QCA807X_LED_BLINK_FREQ_MASK			GENMASK(5, 3)
+#define QCA807X_LED_BLINK_FREQ_2HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x0)
+#define QCA807X_LED_BLINK_FREQ_4HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x1)
+#define QCA807X_LED_BLINK_FREQ_8HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x2)
+#define QCA807X_LED_BLINK_FREQ_16HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x3)
+#define QCA807X_LED_BLINK_FREQ_32HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x4)
+#define QCA807X_LED_BLINK_FREQ_64HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x5)
+#define QCA807X_LED_BLINK_FREQ_128HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x6)
+#define QCA807X_LED_BLINK_FREQ_256HZ			FIELD_PREP(QCA807X_LED_BLINK_FREQ_MASK, 0x7)
+#define QCA807X_LED_BLINK_DUTY_MASK			GENMASK(2, 0)
+#define QCA807X_LED_BLINK_DUTY_50_50			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x0)
+#define QCA807X_LED_BLINK_DUTY_75_25			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x1)
+#define QCA807X_LED_BLINK_DUTY_25_75			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x2)
+#define QCA807X_LED_BLINK_DUTY_33_67			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x3)
+#define QCA807X_LED_BLINK_DUTY_67_33			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x4)
+#define QCA807X_LED_BLINK_DUTY_17_83			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x5)
+#define QCA807X_LED_BLINK_DUTY_83_17			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x6)
+#define QCA807X_LED_BLINK_DUTY_8_92			FIELD_PREP(QCA807X_LED_BLINK_DUTY_MASK, 0x7)
 #define QCA807X_MMD7_LED_100N_1				0x8074
 #define QCA807X_MMD7_LED_100N_2				0x8075
 #define QCA807X_MMD7_LED_1000N_1			0x8076
 #define QCA807X_MMD7_LED_1000N_2			0x8077
-#define QCA807X_LED_TXACT_BLK_EN_2			BIT(10)
-#define QCA807X_LED_RXACT_BLK_EN_2			BIT(9)
-#define QCA807X_LED_GT_ON_EN_2				BIT(6)
-#define QCA807X_LED_HT_ON_EN_2				BIT(5)
-#define QCA807X_LED_BT_ON_EN_2				BIT(4)
-#define QCA807X_GPIO_FORCE_EN				BIT(15)
-#define QCA807X_GPIO_FORCE_MODE_MASK			GENMASK(14, 13)
+/* Values are the same for LED1 and LED2 */
+/* Values for control 1 */
+#define QCA807X_LED_COPPER_ON_BLINK_MASK		GENMASK(12, 0)
+#define QCA807X_LED_FDX_ON_EN				BIT(12)
+#define QCA807X_LED_HDX_ON_EN				BIT(11)
+#define QCA807X_LED_TXACT_BLK_EN			BIT(10)
+#define QCA807X_LED_RXACT_BLK_EN			BIT(9)
+#define QCA807X_LED_GT_ON_EN				BIT(6)
+#define QCA807X_LED_HT_ON_EN				BIT(5)
+#define QCA807X_LED_BT_ON_EN				BIT(4)
+/* Values for control 2 */
+#define QCA807X_LED_FORCE_EN				BIT(15)
+#define QCA807X_LED_FORCE_MODE_MASK			GENMASK(14, 13)
+#define QCA807X_LED_FORCE_BLINK_1			FIELD_PREP(QCA807X_LED_FORCE_MODE_MASK, 0x3)
+#define QCA807X_LED_FORCE_BLINK_2			FIELD_PREP(QCA807X_LED_FORCE_MODE_MASK, 0x2)
+#define QCA807X_LED_FORCE_ON				FIELD_PREP(QCA807X_LED_FORCE_MODE_MASK, 0x1)
+#define QCA807X_LED_FORCE_OFF				FIELD_PREP(QCA807X_LED_FORCE_MODE_MASK, 0x0)
+#define QCA807X_LED_FIBER_ON_BLINK_MASK			GENMASK(11, 1)
+#define QCA807X_LED_FIBER_TXACT_BLK_EN			BIT(10)
+#define QCA807X_LED_FIBER_RXACT_BLK_EN			BIT(9)
+#define QCA807X_LED_FIBER_FDX_ON_EN			BIT(6)
+#define QCA807X_LED_FIBER_HDX_ON_EN			BIT(5)
+#define QCA807X_LED_FIBER_1000BX_ON_EN			BIT(2)
+#define QCA807X_LED_FIBER_100FX_ON_EN			BIT(1)
+
+/* Some device repurpose the LED as GPIO out */
+#define QCA807X_GPIO_FORCE_EN				QCA807X_LED_FORCE_EN
+#define QCA807X_GPIO_FORCE_MODE_MASK			QCA807X_LED_FORCE_MODE_MASK
 
 #define QCA807X_INTR_ENABLE				0x12
 #define QCA807X_INTR_STATUS				0x13
@@ -350,6 +393,320 @@ static int qca807x_cable_test_start(struct phy_device *phydev)
 	return ret;
 }
 
+static int qca807x_led_parse_netdev(struct phy_device *phydev, unsigned long rules,
+				    u16 *offload_trigger)
+{
+	/* Parsing specific to netdev trigger */
+	switch (phydev->port) {
+	case PORT_TP:
+		if (test_bit(TRIGGER_NETDEV_TX, &rules))
+			*offload_trigger |= QCA807X_LED_TXACT_BLK_EN;
+		if (test_bit(TRIGGER_NETDEV_RX, &rules))
+			*offload_trigger |= QCA807X_LED_RXACT_BLK_EN;
+		if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+			*offload_trigger |= QCA807X_LED_BT_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+			*offload_trigger |= QCA807X_LED_HT_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+			*offload_trigger |= QCA807X_LED_GT_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+			*offload_trigger |= QCA807X_LED_HDX_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+			*offload_trigger |= QCA807X_LED_FDX_ON_EN;
+		break;
+	case PORT_FIBRE:
+		if (test_bit(TRIGGER_NETDEV_TX, &rules))
+			*offload_trigger |= QCA807X_LED_FIBER_TXACT_BLK_EN;
+		if (test_bit(TRIGGER_NETDEV_RX, &rules))
+			*offload_trigger |= QCA807X_LED_FIBER_RXACT_BLK_EN;
+		if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+			*offload_trigger |= QCA807X_LED_FIBER_100FX_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+			*offload_trigger |= QCA807X_LED_FIBER_1000BX_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+			*offload_trigger |= QCA807X_LED_FIBER_HDX_ON_EN;
+		if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+			*offload_trigger |= QCA807X_LED_FIBER_FDX_ON_EN;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (rules && !*offload_trigger)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static int qca807x_led_hw_control_enable(struct phy_device *phydev, u8 index)
+{
+	int val, reg, ret;
+
+	switch (index) {
+	case 0:
+		reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		reg = QCA807X_MMD7_LED_1000N_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+	val &= ~QCA807X_LED_FORCE_EN;
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, reg, val);
+
+	return ret;
+}
+
+static int qca807x_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	u16 offload_trigger = 0;
+
+	if (index > 1)
+		return -EINVAL;
+
+	return qca807x_led_parse_netdev(phydev, rules, &offload_trigger);
+}
+
+static int qca807x_led_hw_control_set(struct phy_device *phydev, u8 index,
+				      unsigned long rules)
+{
+	int val, ret, copper_reg, fibre_reg;
+	u16 offload_trigger = 0;
+
+	switch (index) {
+	case 0:
+		copper_reg = QCA807X_MMD7_LED_100N_1;
+		fibre_reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		copper_reg = QCA807X_MMD7_LED_1000N_1;
+		fibre_reg = QCA807X_MMD7_LED_1000N_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = qca807x_led_parse_netdev(phydev, rules, &offload_trigger);
+	if (ret)
+		return ret;
+
+	ret = qca807x_led_hw_control_enable(phydev, index);
+	if (ret)
+		return ret;
+
+	switch (phydev->port) {
+	case PORT_TP:
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, copper_reg);
+		val &= ~QCA807X_LED_COPPER_ON_BLINK_MASK;
+		val |= offload_trigger;
+		ret = phy_write_mmd(phydev, MDIO_MMD_AN, copper_reg, val);
+		break;
+	case PORT_FIBRE:
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, fibre_reg);
+		val &= ~QCA807X_LED_FIBER_ON_BLINK_MASK;
+		val |= offload_trigger;
+		ret = phy_write_mmd(phydev, MDIO_MMD_AN, fibre_reg, val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static bool qca807x_led_hw_control_status(struct phy_device *phydev, u8 index)
+{
+	int val, reg;
+
+	switch (index) {
+	case 0:
+		reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		reg = QCA807X_MMD7_LED_1000N_2;
+		break;
+	default:
+		return false;
+	}
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+
+	return !(val & QCA807X_LED_FORCE_EN);
+}
+
+static int qca807x_led_hw_control_get(struct phy_device *phydev, u8 index,
+				      unsigned long *rules)
+{
+	int val, copper_reg, fibre_reg;
+
+	switch (index) {
+	case 0:
+		copper_reg = QCA807X_MMD7_LED_100N_1;
+		fibre_reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		copper_reg = QCA807X_MMD7_LED_1000N_1;
+		fibre_reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Check if we have hw control enabled */
+	if (qca807x_led_hw_control_status(phydev, index))
+		return -EINVAL;
+
+	/* Parsing specific to netdev trigger */
+	switch (phydev->port) {
+	case PORT_TP:
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, copper_reg);
+		if (val & QCA807X_LED_TXACT_BLK_EN)
+			set_bit(TRIGGER_NETDEV_TX, rules);
+		if (val & QCA807X_LED_RXACT_BLK_EN)
+			set_bit(TRIGGER_NETDEV_RX, rules);
+		if (val & QCA807X_LED_BT_ON_EN)
+			set_bit(TRIGGER_NETDEV_LINK_10, rules);
+		if (val & QCA807X_LED_HT_ON_EN)
+			set_bit(TRIGGER_NETDEV_LINK_100, rules);
+		if (val & QCA807X_LED_GT_ON_EN)
+			set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+		if (val & QCA807X_LED_HDX_ON_EN)
+			set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
+		if (val & QCA807X_LED_FDX_ON_EN)
+			set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
+		break;
+	case PORT_FIBRE:
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, fibre_reg);
+		if (val & QCA807X_LED_FIBER_TXACT_BLK_EN)
+			set_bit(TRIGGER_NETDEV_TX, rules);
+		if (val & QCA807X_LED_FIBER_RXACT_BLK_EN)
+			set_bit(TRIGGER_NETDEV_RX, rules);
+		if (val & QCA807X_LED_FIBER_100FX_ON_EN)
+			set_bit(TRIGGER_NETDEV_LINK_100, rules);
+		if (val & QCA807X_LED_FIBER_1000BX_ON_EN)
+			set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+		if (val & QCA807X_LED_FIBER_HDX_ON_EN)
+			set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
+		if (val & QCA807X_LED_FIBER_FDX_ON_EN)
+			set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qca807x_led_hw_control_reset(struct phy_device *phydev, u8 index)
+{
+	int val, copper_reg, fibre_reg, ret;
+
+	switch (index) {
+	case 0:
+		copper_reg = QCA807X_MMD7_LED_100N_1;
+		fibre_reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		copper_reg = QCA807X_MMD7_LED_1000N_1;
+		fibre_reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (phydev->port) {
+	case PORT_TP:
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, copper_reg);
+		val &= ~QCA807X_LED_COPPER_ON_BLINK_MASK;
+		ret = phy_write_mmd(phydev, MDIO_MMD_AN, copper_reg, val);
+		break;
+	case PORT_FIBRE:
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, fibre_reg);
+		val &= ~QCA807X_LED_FIBER_ON_BLINK_MASK;
+		ret = phy_write_mmd(phydev, MDIO_MMD_AN, fibre_reg, val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int qca807x_led_brightness_set(struct phy_device *phydev,
+				      u8 index, enum led_brightness value)
+{
+	int val, ret;
+	u16 reg;
+
+	switch (index) {
+	case 0:
+		reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		reg = QCA807X_MMD7_LED_1000N_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* If we are setting off the LED reset any hw control rule */
+	if (!value) {
+		ret = qca807x_led_hw_control_reset(phydev, index);
+		if (ret)
+			return ret;
+	}
+
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+	val &= ~(QCA807X_LED_FORCE_EN | QCA807X_LED_FORCE_MODE_MASK);
+	val |= QCA807X_LED_FORCE_EN;
+	if (value)
+		val |= QCA807X_LED_FORCE_ON;
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, reg, val);
+
+	return ret;
+}
+
+static int qca807x_led_blink_set(struct phy_device *phydev, u8 index,
+				 unsigned long *delay_on,
+				 unsigned long *delay_off)
+{
+	int val, ret;
+	u16 reg;
+
+	switch (index) {
+	case 0:
+		reg = QCA807X_MMD7_LED_100N_2;
+		break;
+	case 1:
+		reg = QCA807X_MMD7_LED_1000N_2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Set blink to 50% off, 50% on at 4Hz by default */
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, QCA807X_MMD7_LED_GLOBAL);
+	val &= ~(QCA807X_LED_BLINK_FREQ_MASK | QCA807X_LED_BLINK_DUTY_MASK);
+	val |= QCA807X_LED_BLINK_FREQ_4HZ | QCA807X_LED_BLINK_DUTY_50_50;
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, QCA807X_MMD7_LED_GLOBAL, val);
+
+	/* We use BLINK_1 for normal blinking */
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+	val &= ~(QCA807X_LED_FORCE_EN | QCA807X_LED_FORCE_MODE_MASK);
+	val |= QCA807X_LED_FORCE_EN | QCA807X_LED_FORCE_BLINK_1;
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, reg, val);
+
+	/* We set blink to 4Hz, aka 250ms */
+	*delay_on = 250 / 2;
+	*delay_off = 250 / 2;
+
+	return ret;
+}
+
 #ifdef CONFIG_GPIOLIB
 static int qca807x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
@@ -739,6 +1096,12 @@ static int qca807x_probe(struct phy_device *phydev)
 			ret = qca807x_gpio(phydev);
 			if (ret)
 				return ret;
+
+			phydev->drv->led_brightness_set = NULL;
+			phydev->drv->led_blink_set = NULL;
+			phydev->drv->led_hw_is_supported = NULL;
+			phydev->drv->led_hw_control_set = NULL;
+			phydev->drv->led_hw_control_get = NULL;
 		}
 	}
 
@@ -926,6 +1289,11 @@ static struct phy_driver qca807x_drivers[] = {
 		.suspend	= genphy_suspend,
 		.cable_test_start	= qca807x_cable_test_start,
 		.cable_test_get_status	= qca807x_cable_test_get_status,
+		.led_brightness_set = qca807x_led_brightness_set,
+		.led_blink_set = qca807x_led_blink_set,
+		.led_hw_is_supported = qca807x_led_hw_is_supported,
+		.led_hw_control_set = qca807x_led_hw_control_set,
+		.led_hw_control_get = qca807x_led_hw_control_get,
 		/* PHY package define */
 		.phy_package_priv_data_size = sizeof(struct qca807x_shared_priv),
 		.phy_package_probe_once = qca807x_phy_package_probe_once,
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2023-11-26  1:53 ` [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
@ 2023-11-27 22:16   ` Rob Herring
  2023-11-28  0:09     ` Andrew Lunn
  2024-01-07 18:00   ` Sergey Ryazanov
  1 sibling, 1 reply; 33+ messages in thread
From: Rob Herring @ 2023-11-27 22:16 UTC (permalink / raw)
  To: Christian Marangi
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

On Sun, Nov 26, 2023 at 02:53:39AM +0100, Christian Marangi wrote:
> Document ethernet PHY package nodes used to describe PHY shipped in
> bundle of 4-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 package 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 and declare the
> "ethrnet-phy-package" compatible.
> 
> Each reg of the PHY defined in the PHY package node is absolute and will
> reference the real address of the PHY on the bus.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
>  1 file changed, 75 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..244d4bc29164
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> @@ -0,0 +1,75 @@
> +# 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:
> +  This schema describe PHY package as simple container for
> +  a bundle of PHYs that share the same properties and
> +  contains the PHYs of the package themself.
> +
> +  Each reg of the PHYs defined in the PHY package node is
> +  absolute and describe the real address of the PHY on the bus.
> +
> +properties:
> +  $nodename:
> +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> +
> +  compatible:
> +    const: ethernet-phy-package

In case I wasn't clear, but that compatible is a NAK.

> +
> +  reg:
> +    minimum: 0
> +    maximum: 31

Pretty sure the bus binding already provides these constraints.

> +    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 exist on the device (just not attached to anything).
> +      The reg defined in the PHY package node might differ and
> +      the related PHY might be not defined.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0

You are implementing a secondary MDIO bus within this node. It needs a 
$ref to mdio.yaml instead of defining the bus again implicitly.

> +
> +patternProperties:
> +  ^ethernet-phy(@[a-f0-9]+)?$:
> +    $ref: ethernet-phy.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy-package@16 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "ethernet-phy-package";
> +            reg = <0x16>;
> +
> +            ethernet-phy@16 {
> +              reg = <0x16>;
> +            };
> +
> +            phy4: ethernet-phy@1a {
> +              reg = <0x1a>;
> +            };

This example on its own doesn't make sense. It can't be fully validated 
because you allow any additional properties. Drop it.

> +        };
> +    };
> -- 
> 2.40.1
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package
  2023-11-26  1:53 ` [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
@ 2023-11-27 22:35   ` Rob Herring
  2023-11-28  0:30   ` Andrew Lunn
  1 sibling, 0 replies; 33+ messages in thread
From: Rob Herring @ 2023-11-27 22:35 UTC (permalink / raw)
  To: Christian Marangi
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

On Sun, Nov 26, 2023 at 02:53:44AM +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 | 136 ++++++++++++++++++
>  1 file changed, 136 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..136ba2128b73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> @@ -0,0 +1,136 @@
> +# 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.

This so far is about the only thing that convinces me of having child 
nodes.

> +
> +  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#
> +

Did you test this binding? No, because there is nothing to select it. 
Just put a wrong value for qcom,package-mode and see.

ethernet-phy-package.yaml will be applied, but since it allows any extra 
properties not much is tested.

> +properties:
> +  qcom,package-mode:
> +    enum:
> +      - qsgmii
> +      - psgmii

description needed.

I don't understand how the 3 modes works with only 2 modes defined here.

> +
> +  qcom,tx-driver-strength:
> +    description: set the TX Amplifier value in mv.

millivolts or...

> +      If not defined, 600mw is set by default.

milliwatts?

Whatever it is, use standard unit suffix.

> +    $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'm confused. The addresses are MDIO, but you have just 0-4. Might be 
correct or might not?

In any case, the unit-address should be restricted to '@1?[0-9a-f]$'. 
Though that belongs in mdio.yaml and here '^ethernet-phy@' is sufficient 
(once there's a reference to mdio.yaml).

> +    $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']

No need for this, the common gpio schema does this.

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

Move under $ref.

> +
> +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 = "ethernet-phy-package";
> +            reg = <0>;
> +
> +            qcom,package-mode = "qsgmii";

Nothing in here tells me this is a QCA807X other than this property 
(sort of). You need a specific compatible.

> +
> +            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>;
> +            };
> +        };
> +    };
> -- 
> 2.40.1
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 5/8] dt-bindings: net: add QCA807x PHY defines
  2023-11-26  1:53 ` [net-next PATCH RFC v3 5/8] dt-bindings: net: add QCA807x PHY defines Christian Marangi
@ 2023-11-27 22:36   ` Rob Herring
  0 siblings, 0 replies; 33+ messages in thread
From: Rob Herring @ 2023-11-27 22:36 UTC (permalink / raw)
  To: Christian Marangi
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

On Sun, Nov 26, 2023 at 02:53:43AM +0100, 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.
> 
> 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 ++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 include/dt-bindings/net/qcom-qca807x.h

How is this used? There's nothing in the binding about it. Binding 
headers go with the binding schema that define how they are used.

> 
> diff --git a/include/dt-bindings/net/qcom-qca807x.h b/include/dt-bindings/net/qcom-qca807x.h
> new file mode 100644
> index 000000000000..e7d4d09b7dd4
> --- /dev/null
> +++ b/include/dt-bindings/net/qcom-qca807x.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Device Tree constants for the Qualcomm QCA807X PHYs
> + */
> +
> +#ifndef _DT_BINDINGS_QCOM_QCA807X_H
> +#define _DT_BINDINGS_QCOM_QCA807X_H
> +
> +/* Full amplitude, full bias current */
> +#define QCA807X_CONTROL_DAC_FULL_VOLT_BIAS		0
> +/* Amplitude follow DSP (amplitude is adjusted based on cable length), half bias current */
> +#define QCA807X_CONTROL_DAC_DSP_VOLT_HALF_BIAS		1
> +/* Full amplitude, bias current follow DSP (bias current is adjusted based on cable length) */
> +#define QCA807X_CONTROL_DAC_FULL_VOLT_DSP_BIAS		2
> +/* Both amplitude and bias current follow DSP */
> +#define QCA807X_CONTROL_DAC_DSP_VOLT_BIAS		3
> +/* Full amplitude, half bias current */
> +#define QCA807X_CONTROL_DAC_FULL_VOLT_HALF_BIAS		4
> +/* Amplitude follow DSP setting; 1/4 bias current when cable<10m,
> + * otherwise half bias current
> + */
> +#define QCA807X_CONTROL_DAC_DSP_VOLT_QUARTER_BIAS	5
> +/* Full amplitude; same bias current setting with “010” and “011”,
> + * but half more bias is reduced when cable <10m
> + */
> +#define QCA807X_CONTROL_DAC_FULL_VOLT_HALF_BIAS_SHORT	6
> +/* Amplitude follow DSP; same bias current setting with “110”, default value */
> +#define QCA807X_CONTROL_DAC_DSP_VOLT_HALF_BIAS_SHORT	7
> +
> +#endif
> -- 
> 2.40.1
> 

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2023-11-27 22:16   ` Rob Herring
@ 2023-11-28  0:09     ` Andrew Lunn
  0 siblings, 0 replies; 33+ messages in thread
From: Andrew Lunn @ 2023-11-28  0:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

> > +    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 exist on the device (just not attached to anything).
> > +      The reg defined in the PHY package node might differ and
> > +      the related PHY might be not defined.
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> 
> You are implementing a secondary MDIO bus within this node. It needs a 
> $ref to mdio.yaml instead of defining the bus again implicitly.

This is where i think this is questionable. It is not implemented in
the kernel as a secondary bus. The devices within this container are
just devices on the MDIO bus. The value of reg inside the container
and outside the container refer to the same bus.

However, i do agree about referring to mdio.yaml inside the container.

> > +patternProperties:
> > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > +    $ref: ethernet-phy.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    mdio {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ethernet-phy-package@16 {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "ethernet-phy-package";

Christian, this needs a specific compatible to the
package. e.g. 'qca807x-package', and that needs its own .yaml file
indicating what properties this package can have.

	   Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package
  2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
                   ` (7 preceding siblings ...)
  2023-11-26  1:53 ` [net-next PATCH RFC v3 8/8] net: phy: qca807x: Add support for configurable LED Christian Marangi
@ 2023-11-28  0:20 ` Andrew Lunn
  8 siblings, 0 replies; 33+ messages in thread
From: Andrew Lunn @ 2023-11-28  0:20 UTC (permalink / raw)
  To: Christian Marangi
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

On Sun, Nov 26, 2023 at 02:53:38AM +0100, Christian Marangi wrote:
> This depends on another series for PHY package API change. [1]
> 
> Idea of this big series is to introduce the concept of PHY package in DT
> and generalize the support of it by PHY driver.
> 
> 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.
> 
> The main idea of those API is to permit the PHY to have a shared global
> data and to run probe/config only once for the PHY package. There are
> various example of this already in the kernel with the mscc, bcm54140
> mediatek ge and micrle driver and they all follow the same pattern.
> 
> What is currently lacking is describing this in DT and better reference
> the PHY in charge of global configuration of the PHY package. For the
> already present PHY, the implementation is simple enough with only one
> PHY having the required regs to apply global configuration.
> 
> This can be ok for simple PHY package but some Qcom PHY package on
> ""modern"" SoC have more complex implementation. One example is the PHY
> for qca807x where some global regs are present in the so-called "combo"
> port and everything about psgmii calibration is placed in a 5th port in
> the PHY package.
> 
> Given these additional thing, the original phy_package API are extended
> with support for multiple global PHY for configuration. Each PHY driver
> will have an enum of the ID for the global PHY to reference and is
> required to pass to the read/write function.

Please update the text. As far as i see, a lot of this is not relevant
for this patch set. phy_package_join() etc has no relation to DT,
since the driver knows how many devices are in its package, it knows
its base address, etc.

The DT is only about properties which are shared by all PHYs within
the package, e.g reset, regulators, maybe the MODE_CFG register for
this particular PHY package.

> 
> On top of this, it's added correct DT support for describing PHY
> package.
> 
> One example is this:
> 
>         ethernet-phy-package@0 {
>             compatible = "ethernet-phy-package";

This needs a compatible for this particular PHY package.

>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             reg = <0>;
>             qcom,package-mode = "qsgmii";

This property it not useful. Why PCA does it apply to, when there are
two? It makes much more sense to describe the overall configuration
mode, from which you can derive what interface mode each port should
be using, and thus validate the phy-mode in DT.

   Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package
  2023-11-26  1:53 ` [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
  2023-11-27 22:35   ` Rob Herring
@ 2023-11-28  0:30   ` Andrew Lunn
  1 sibling, 0 replies; 33+ messages in thread
From: Andrew Lunn @ 2023-11-28  0:30 UTC (permalink / raw)
  To: Christian Marangi
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  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]

This is O.K, you are describing package properties, even if i don't
agree about qcom,package-mode.

> +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

But now you are describing PHY properties. These belong on a .yaml of
its own, just like qca,ar803x.yaml defines properties for the AR803x
PHY.

	Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT
  2023-11-26  1:53 ` [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT Christian Marangi
@ 2023-11-28  0:39   ` Andrew Lunn
  2023-11-28 12:09     ` Christian Marangi
  0 siblings, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2023-11-28  0:39 UTC (permalink / raw)
  To: Christian Marangi
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

> +static int of_phy_package(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *package_node;
> +	u32 base_addr;
> +	int ret;
> +
> +	if (!node)
> +		return 0;
> +
> +	package_node = of_get_parent(node);
> +	if (!package_node)
> +		return 0;
> +
> +	if (!of_device_is_compatible(package_node, "ethernet-phy-package"))
> +		return 0;
> +
> +	if (of_property_read_u32(package_node, "reg", &base_addr))
> +		return -EINVAL;
> +
> +	ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
> +				    base_addr, 0);

No don't do this. It is just going to lead to errors. The PHY driver
knows how many PHYs are in the package. So it can figure out what the
base address is and create the package. It can add each PHY as they
probe. That cannot go wrong.

If you create the package based on DT you have to validate that the DT
is correct. You need the same information, the base address, how many
packages are in the PHY, etc. So DT gains your nothing except more
potential to get it wrong.

Please use DT just for properties for the package, nothing else.

       Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT
  2023-11-28  0:39   ` Andrew Lunn
@ 2023-11-28 12:09     ` Christian Marangi
  0 siblings, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2023-11-28 12:09 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

On Tue, Nov 28, 2023 at 01:39:02AM +0100, Andrew Lunn wrote:
> > +static int of_phy_package(struct phy_device *phydev)
> > +{
> > +	struct device_node *node = phydev->mdio.dev.of_node;
> > +	struct device_node *package_node;
> > +	u32 base_addr;
> > +	int ret;
> > +
> > +	if (!node)
> > +		return 0;
> > +
> > +	package_node = of_get_parent(node);
> > +	if (!package_node)
> > +		return 0;
> > +
> > +	if (!of_device_is_compatible(package_node, "ethernet-phy-package"))
> > +		return 0;
> > +
> > +	if (of_property_read_u32(package_node, "reg", &base_addr))
> > +		return -EINVAL;
> > +
> > +	ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
> > +				    base_addr, 0);
> 
> No don't do this. It is just going to lead to errors. The PHY driver
> knows how many PHYs are in the package. So it can figure out what the
> base address is and create the package. It can add each PHY as they
> probe. That cannot go wrong.
>

No it can't and we experiment this with a funny implementation on the
QSDK. Maybe I'm confused?

Example on QSDK they were all based on a value first_phy_addr. This was
filled with the first phy addr found (for the package).

All OEM followed a template with declaring all sort of bloat and they
probably have no idea what they are actually putting in DT. We reverse
all the properties and we gave a meaning to all the values...

We quikly notice that the parsing was very fragile and on some strange
device (an example a Xiaomi 36000) the first PHY for the package was
actually not attached to anything. Resulting in all this logic of
"first_phy_addr" failing as by removing the first PHY, the value was set
to a wrong addr resulting in all sort of problems.

This changed a lot (the original series used a more robust way with
phandle, but it was asked to use base_addr and use offset in the PHY
addr, less robust still OK)

If we revert to PHY driver making the PHY package then we lose all kind
of ""automation"" of having a correct base addr. In PHY driver we would
have to manually parse with parent (as we do here) and check the value?

Why not do the thing directly on PHY core?

By making the PHY driver creating the package, we are back on all kind
of bloat in the PHY driver doing the same thing (parsing package nodes,
probe_once check, config_once check) instead of handling them directly
in PHY core.

Also just to point out, the way the current PHY driver derive the base
addr is problematic. All of them use special regs to find the base one
but I can totally see a PHY driver in the future assuming the first PHY
being the first in the package to be probed, set the base addr on the
first PHY and also assuming that it's always define in DT.

If we really don't want the OF parsing in PHY core and autojoin them,
is at least OK to introduce an helper to get the base addr from a PHY
package node structure? (to at least try to minimaze the bloat that PHY
driver will have?)

Also with the OF support dropped, I guess also the added API in this
series has to be dropped. (as they would be called after the first PHY
probe and that is problematic if for some reason only one PHY of the
package is declared in DT) (an alternative might be moving the
probe_once after the PHY is probed as we expect the phy_package_join
call done in the PHY probe call)

> If you create the package based on DT you have to validate that the DT
> is correct. You need the same information, the base address, how many
> packages are in the PHY, etc. So DT gains your nothing except more
> potential to get it wrong.
> 

For the sake of package join, only the reg has to be validated and the
validation is just if addrs makes sense. Everything else can be done by
PHY package probe once.

> Please use DT just for properties for the package, nothing else.
> 

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2023-11-26  1:53 ` [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
  2023-11-27 22:16   ` Rob Herring
@ 2024-01-07 18:00   ` Sergey Ryazanov
  2024-01-07 18:30     ` Christian Marangi
  2024-01-07 18:31     ` Andrew Lunn
  1 sibling, 2 replies; 33+ messages in thread
From: Sergey Ryazanov @ 2024-01-07 18:00 UTC (permalink / raw)
  To: Christian Marangi, Robert Marko, Andrew Lunn, Vladimir Oltean,
	Rob Herring
  Cc: Luo Jie, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

Hi Christian and Robert,

thank you for this great work!

Let me ask a couple of questions regarding the phy package conception. 
Please find them below.

On 26.11.2023 03:53, Christian Marangi wrote:
> Document ethernet PHY package nodes used to describe PHY shipped in
> bundle of 4-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 package 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 and declare the
> "ethrnet-phy-package" compatible.
> 
> Each reg of the PHY defined in the PHY package node is absolute and will
> reference the real address of the PHY on the bus.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>   .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
>   1 file changed, 75 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..244d4bc29164
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> @@ -0,0 +1,75 @@
> +# 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:
> +  This schema describe PHY package as simple container for
> +  a bundle of PHYs that share the same properties and
> +  contains the PHYs of the package themself.
> +
> +  Each reg of the PHYs defined in the PHY package node is
> +  absolute and describe the real address of the PHY on the bus.
> +
> +properties:
> +  $nodename:
> +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> +
> +  compatible:
> +    const: ethernet-phy-package
> +
> +  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 exist on the device (just not attached to anything).
> +      The reg defined in the PHY package node might differ and
> +      the related PHY might be not defined.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  ^ethernet-phy(@[a-f0-9]+)?$:
> +    $ref: ethernet-phy.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy-package@16 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "ethernet-phy-package";
> +            reg = <0x16>;
> +
> +            ethernet-phy@16 {
> +              reg = <0x16>;
> +            };
> +
> +            phy4: ethernet-phy@1a {
> +              reg = <0x1a>;
> +            };
> +        };
> +    };

So, we ended up on a design where we use the predefined compatible 
string 'ethernet-phy-package' to recognize a phy package inside the 
of_mdiobus_register() function. During the V1 discussion, Vladimir came 
up with the idea of 'ranges' property usage [1]. Can we use 'ranges' to 
recognize a phy package in of_mdiobus_register()? IMHO this will give us 
a clear DT solution. I mean 'ranges' clearly indicates that child nodes 
are in the same address range as the parent node. Also we can list all 
child addresses in 'reg' to mark them occupied.

   mdio {
     ...

     ethernet-phy-package@16 {
       compatible = "qcom,qca8075";
       reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
       ranges;
       ...

       ethernet-phy@16 {
         reg = <0x16>;
       };

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

Did you find some issues with the 'ranges' conception?


And I would like to ask you about another issue raised by Vladimir [1]. 
These phy chips become SoC with all these built-in PHYs, PCSs, clocks, 
interrupt controllers, etc. Should we address this now? Or should we go 
with the proposed solution for now and postpone modeling of other 
peripherals until we get a real hardware, as Andrew suggested?

I'm asking because it looks like we have got a real hardware. Luo 
currently trying to push QCA8084 (multi-phy/switch chip) support, and 
this chip exactly contains a huge clock/reset controller [2,3].


1. https://lore.kernel.org/lkml/20231124165923.p2iozsrnwlogjzua@skbuf // 
Re: [net-next RFC PATCH 03/14] dt-bindings: net: document ethernet PHY 
package nodes
2. 
https://lore.kernel.org/lkml/20231215074005.26976-1-quic_luoj@quicinc.com 
  // [PATCH v8 00/14] add qca8084 ethernet phy driver
3. 
https://lore.kernel.org/lkml/20231104034858.9159-1-quic_luoj@quicinc.com 
  // [PATCH v12 0/4] add clock controller of qca8386/qca8084

--
Sergey

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 18:00   ` Sergey Ryazanov
@ 2024-01-07 18:30     ` Christian Marangi
  2024-01-07 18:44       ` Andrew Lunn
  2024-01-07 21:49       ` Sergey Ryazanov
  2024-01-07 18:31     ` Andrew Lunn
  1 sibling, 2 replies; 33+ messages in thread
From: Christian Marangi @ 2024-01-07 18:30 UTC (permalink / raw)
  To: Sergey Ryazanov
  Cc: Robert Marko, Andrew Lunn, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
> Hi Christian and Robert,
> 
> thank you for this great work!
> 
> Let me ask a couple of questions regarding the phy package conception.
> Please find them below.
> 
> On 26.11.2023 03:53, Christian Marangi wrote:
> > Document ethernet PHY package nodes used to describe PHY shipped in
> > bundle of 4-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 package 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 and declare the
> > "ethrnet-phy-package" compatible.
> > 
> > Each reg of the PHY defined in the PHY package node is absolute and will
> > reference the real address of the PHY on the bus.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
> >   1 file changed, 75 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..244d4bc29164
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > @@ -0,0 +1,75 @@
> > +# 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:
> > +  This schema describe PHY package as simple container for
> > +  a bundle of PHYs that share the same properties and
> > +  contains the PHYs of the package themself.
> > +
> > +  Each reg of the PHYs defined in the PHY package node is
> > +  absolute and describe the real address of the PHY on the bus.
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> > +
> > +  compatible:
> > +    const: ethernet-phy-package
> > +
> > +  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 exist on the device (just not attached to anything).
> > +      The reg defined in the PHY package node might differ and
> > +      the related PHY might be not defined.
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > +    $ref: ethernet-phy.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    mdio {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ethernet-phy-package@16 {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            compatible = "ethernet-phy-package";
> > +            reg = <0x16>;
> > +
> > +            ethernet-phy@16 {
> > +              reg = <0x16>;
> > +            };
> > +
> > +            phy4: ethernet-phy@1a {
> > +              reg = <0x1a>;
> > +            };
> > +        };
> > +    };
> 
> So, we ended up on a design where we use the predefined compatible string
> 'ethernet-phy-package' to recognize a phy package inside the
> of_mdiobus_register() function. During the V1 discussion, Vladimir came up
> with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
> recognize a phy package in of_mdiobus_register()? IMHO this will give us a
> clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
> the same address range as the parent node. Also we can list all child
> addresses in 'reg' to mark them occupied.
> 
>   mdio {
>     ...
> 
>     ethernet-phy-package@16 {
>       compatible = "qcom,qca8075";
>       reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
>       ranges;
>       ...
> 
>       ethernet-phy@16 {
>         reg = <0x16>;
>       };
> 
>       ethernet-phy@1a {
>         reg = <0x1a>;
>       };
>     };
>   };
> 
> Did you find some issues with the 'ranges' conception?
>

Nope it's ok but it might pose some confusion with the idea that the
very first element MUST be THE STARTING ADDR of the PHY package. (people
might think that it's just the list of the PHYs in the package and
remove the hardware unconnected ones... but that would be fault of who
write the DT anyway.)

> 
> And I would like to ask you about another issue raised by Vladimir [1].
> These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
> interrupt controllers, etc. Should we address this now? Or should we go with
> the proposed solution for now and postpone modeling of other peripherals
> until we get a real hardware, as Andrew suggested?
> 

Honestly I would postpone untile we have a clear idea of what is
actually part of the PHY and what can be handled externally... Example
setting the clock in gcc, writing a specific driver...

It's a random idea but maybe most of the stuff required for that PHY is
just when it's connected to a switch... In that case it would all be
handled in the switch driver (tobe extended qca8k) and all these extra
stuff would be placed in that node instead of bloating phy nodes with
all kind of clk and other stuff.

This series still require 2 more series (at803x splint and cleanup) to be
actually proposed so we have some time to better define this.

What do you think?

> I'm asking because it looks like we have got a real hardware. Luo currently
> trying to push QCA8084 (multi-phy/switch chip) support, and this chip
> exactly contains a huge clock/reset controller [2,3].
> 
> 
> 1. https://lore.kernel.org/lkml/20231124165923.p2iozsrnwlogjzua@skbuf // Re:
> [net-next RFC PATCH 03/14] dt-bindings: net: document ethernet PHY package
> nodes
> 2. https://lore.kernel.org/lkml/20231215074005.26976-1-quic_luoj@quicinc.com
> // [PATCH v8 00/14] add qca8084 ethernet phy driver
> 3. https://lore.kernel.org/lkml/20231104034858.9159-1-quic_luoj@quicinc.com
> // [PATCH v12 0/4] add clock controller of qca8386/qca8084
> 
> --
> Sergey

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 18:00   ` Sergey Ryazanov
  2024-01-07 18:30     ` Christian Marangi
@ 2024-01-07 18:31     ` Andrew Lunn
  2024-01-08 11:13       ` Jie Luo
  1 sibling, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2024-01-07 18:31 UTC (permalink / raw)
  To: Sergey Ryazanov
  Cc: Christian Marangi, Robert Marko, Vladimir Oltean, Rob Herring,
	Luo Jie, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

> And I would like to ask you about another issue raised by Vladimir [1].
> These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
> interrupt controllers, etc. Should we address this now? Or should we go with
> the proposed solution for now and postpone modeling of other peripherals
> until we get a real hardware, as Andrew suggested?
> 
> I'm asking because it looks like we have got a real hardware. Luo currently
> trying to push QCA8084 (multi-phy/switch chip) support, and this chip
> exactly contains a huge clock/reset controller [2,3].

Ideally the reset controller is modelled as a Linux reset
controller. The clock part of it is modelled using the common clock
framework. When done correctly, the PHY should not really care. All it
does is ask for its clock to be enabled, and its reset to be disabled.

Also, given how difficult it is proving to be to make any progress at
all, i want to get one little part correctly described, the pure
PHY. Once we have that, we can start on the next little part. So long
as we keep to the Linux architecture of blocks or hardware with
standard Linux drivers, and DT to glue them together, a small step by
step approach should work out.

     Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 18:30     ` Christian Marangi
@ 2024-01-07 18:44       ` Andrew Lunn
  2024-01-07 21:57         ` Sergey Ryazanov
  2024-01-07 21:49       ` Sergey Ryazanov
  1 sibling, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2024-01-07 18:44 UTC (permalink / raw)
  To: Christian Marangi
  Cc: Sergey Ryazanov, Robert Marko, Vladimir Oltean, Rob Herring,
	Luo Jie, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

> Honestly I would postpone untile we have a clear idea of what is
> actually part of the PHY and what can be handled externally... Example
> setting the clock in gcc, writing a specific driver...

That is really core of the problem here. We have no reliable
description of the hardware. The datasheet often starts with a block
diagram of the PHY package. Sometimes there is a product brief with
the same diagram. We need that published. I'm not asking for the full
data sheet, just the introductory text which gives a high level
overview of what the hardware actually is. Then we can sketch out a
high level Linux architecture for the PHY package.

     Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 21:49       ` Sergey Ryazanov
@ 2024-01-07 21:45         ` Christian Marangi
  2024-01-17  0:36         ` Christian Marangi
  1 sibling, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2024-01-07 21:45 UTC (permalink / raw)
  To: Sergey Ryazanov
  Cc: Robert Marko, Andrew Lunn, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

On Sun, Jan 07, 2024 at 11:49:12PM +0200, Sergey Ryazanov wrote:
> Hi Christian,
> 
> On 07.01.2024 20:30, Christian Marangi wrote:
> > On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
> > > On 26.11.2023 03:53, Christian Marangi wrote:
> > > > Document ethernet PHY package nodes used to describe PHY shipped in
> > > > bundle of 4-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 package 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 and declare the
> > > > "ethrnet-phy-package" compatible.
> > > > 
> > > > Each reg of the PHY defined in the PHY package node is absolute and will
> > > > reference the real address of the PHY on the bus.
> > > > 
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > >    .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
> > > >    1 file changed, 75 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..244d4bc29164
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > > > @@ -0,0 +1,75 @@
> > > > +# 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:
> > > > +  This schema describe PHY package as simple container for
> > > > +  a bundle of PHYs that share the same properties and
> > > > +  contains the PHYs of the package themself.
> > > > +
> > > > +  Each reg of the PHYs defined in the PHY package node is
> > > > +  absolute and describe the real address of the PHY on the bus.
> > > > +
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> > > > +
> > > > +  compatible:
> > > > +    const: ethernet-phy-package
> > > > +
> > > > +  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 exist on the device (just not attached to anything).
> > > > +      The reg defined in the PHY package node might differ and
> > > > +      the related PHY might be not defined.
> > > > +
> > > > +  '#address-cells':
> > > > +    const: 1
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > > > +
> > > > +patternProperties:
> > > > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > > > +    $ref: ethernet-phy.yaml#
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +additionalProperties: true
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    mdio {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        ethernet-phy-package@16 {
> > > > +            #address-cells = <1>;
> > > > +            #size-cells = <0>;
> > > > +            compatible = "ethernet-phy-package";
> > > > +            reg = <0x16>;
> > > > +
> > > > +            ethernet-phy@16 {
> > > > +              reg = <0x16>;
> > > > +            };
> > > > +
> > > > +            phy4: ethernet-phy@1a {
> > > > +              reg = <0x1a>;
> > > > +            };
> > > > +        };
> > > > +    };
> > > 
> > > So, we ended up on a design where we use the predefined compatible string
> > > 'ethernet-phy-package' to recognize a phy package inside the
> > > of_mdiobus_register() function. During the V1 discussion, Vladimir came up
> > > with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
> > > recognize a phy package in of_mdiobus_register()? IMHO this will give us a
> > > clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
> > > the same address range as the parent node. Also we can list all child
> > > addresses in 'reg' to mark them occupied.
> > > 
> > >    mdio {
> > >      ...
> > > 
> > >      ethernet-phy-package@16 {
> > >        compatible = "qcom,qca8075";
> > >        reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
> > >        ranges;
> > >        ...
> > > 
> > >        ethernet-phy@16 {
> > >          reg = <0x16>;
> > >        };
> > > 
> > >        ethernet-phy@1a {
> > >          reg = <0x1a>;
> > >        };
> > >      };
> > >    };
> > > 
> > > Did you find some issues with the 'ranges' conception?
> > 
> > Nope it's ok but it might pose some confusion with the idea that the
> > very first element MUST be THE STARTING ADDR of the PHY package. (people
> > might think that it's just the list of the PHYs in the package and
> > remove the hardware unconnected ones... but that would be fault of who
> > write the DT anyway.)
> 
> Make sense. I do not insist on addresses listing. Mainly I'm thinking of a
> proper way to show that child nodes are accessible directly on the parent
> bus, and introducing the special compatibility string, while we already have
> the 'ranges' property.
> 
> But it's good to know Rob's opinion on whether it is conceptually right to
> use 'ranges' here.
>

I like the ideas of ranges and I will try to propose it... Another idea
might be declare something like <0x16 0x4> where the additional cell
would declare the amount of address occupiaed by the package. But I
think your way is much better and less ""custom"". Yes would also love
some feedback from Rob about this.

> > > And I would like to ask you about another issue raised by Vladimir [1].
> > > These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
> > > interrupt controllers, etc. Should we address this now? Or should we go with
> > > the proposed solution for now and postpone modeling of other peripherals
> > > until we get a real hardware, as Andrew suggested?
> > 
> > Honestly I would postpone untile we have a clear idea of what is
> > actually part of the PHY and what can be handled externally... Example
> > setting the clock in gcc, writing a specific driver...
> > 
> > It's a random idea but maybe most of the stuff required for that PHY is
> > just when it's connected to a switch... In that case it would all be
> > handled in the switch driver (tobe extended qca8k) and all these extra
> > stuff would be placed in that node instead of bloating phy nodes with
> > all kind of clk and other stuff.
> > 
> > This series still require 2 more series (at803x splint and cleanup) to be
> > actually proposed so we have some time to better define this.
> > 
> > What do you think?
> 
> Fair enough! Let's postpone until we really need it. I noticed this
> PHY-like-SoC discussion in the V1 comments, and it was not finished there
> neither addressed in the latest patch comment. So I asked just to be sure
> that we were finished with this. Thank you for the clarification.
> 
> --
> Sergey
> 

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 18:30     ` Christian Marangi
  2024-01-07 18:44       ` Andrew Lunn
@ 2024-01-07 21:49       ` Sergey Ryazanov
  2024-01-07 21:45         ` Christian Marangi
  2024-01-17  0:36         ` Christian Marangi
  1 sibling, 2 replies; 33+ messages in thread
From: Sergey Ryazanov @ 2024-01-07 21:49 UTC (permalink / raw)
  To: Christian Marangi
  Cc: Robert Marko, Andrew Lunn, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

Hi Christian,

On 07.01.2024 20:30, Christian Marangi wrote:
> On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
>> On 26.11.2023 03:53, Christian Marangi wrote:
>>> Document ethernet PHY package nodes used to describe PHY shipped in
>>> bundle of 4-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 package 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 and declare the
>>> "ethrnet-phy-package" compatible.
>>>
>>> Each reg of the PHY defined in the PHY package node is absolute and will
>>> reference the real address of the PHY on the bus.
>>>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>> ---
>>>    .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
>>>    1 file changed, 75 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..244d4bc29164
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
>>> @@ -0,0 +1,75 @@
>>> +# 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:
>>> +  This schema describe PHY package as simple container for
>>> +  a bundle of PHYs that share the same properties and
>>> +  contains the PHYs of the package themself.
>>> +
>>> +  Each reg of the PHYs defined in the PHY package node is
>>> +  absolute and describe the real address of the PHY on the bus.
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
>>> +
>>> +  compatible:
>>> +    const: ethernet-phy-package
>>> +
>>> +  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 exist on the device (just not attached to anything).
>>> +      The reg defined in the PHY package node might differ and
>>> +      the related PHY might be not defined.
>>> +
>>> +  '#address-cells':
>>> +    const: 1
>>> +
>>> +  '#size-cells':
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  ^ethernet-phy(@[a-f0-9]+)?$:
>>> +    $ref: ethernet-phy.yaml#
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +additionalProperties: true
>>> +
>>> +examples:
>>> +  - |
>>> +    mdio {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        ethernet-phy-package@16 {
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +            compatible = "ethernet-phy-package";
>>> +            reg = <0x16>;
>>> +
>>> +            ethernet-phy@16 {
>>> +              reg = <0x16>;
>>> +            };
>>> +
>>> +            phy4: ethernet-phy@1a {
>>> +              reg = <0x1a>;
>>> +            };
>>> +        };
>>> +    };
>>
>> So, we ended up on a design where we use the predefined compatible string
>> 'ethernet-phy-package' to recognize a phy package inside the
>> of_mdiobus_register() function. During the V1 discussion, Vladimir came up
>> with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
>> recognize a phy package in of_mdiobus_register()? IMHO this will give us a
>> clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
>> the same address range as the parent node. Also we can list all child
>> addresses in 'reg' to mark them occupied.
>>
>>    mdio {
>>      ...
>>
>>      ethernet-phy-package@16 {
>>        compatible = "qcom,qca8075";
>>        reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
>>        ranges;
>>        ...
>>
>>        ethernet-phy@16 {
>>          reg = <0x16>;
>>        };
>>
>>        ethernet-phy@1a {
>>          reg = <0x1a>;
>>        };
>>      };
>>    };
>>
>> Did you find some issues with the 'ranges' conception?
> 
> Nope it's ok but it might pose some confusion with the idea that the
> very first element MUST be THE STARTING ADDR of the PHY package. (people
> might think that it's just the list of the PHYs in the package and
> remove the hardware unconnected ones... but that would be fault of who
> write the DT anyway.)

Make sense. I do not insist on addresses listing. Mainly I'm thinking of 
a proper way to show that child nodes are accessible directly on the 
parent bus, and introducing the special compatibility string, while we 
already have the 'ranges' property.

But it's good to know Rob's opinion on whether it is conceptually right 
to use 'ranges' here.

>> And I would like to ask you about another issue raised by Vladimir [1].
>> These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
>> interrupt controllers, etc. Should we address this now? Or should we go with
>> the proposed solution for now and postpone modeling of other peripherals
>> until we get a real hardware, as Andrew suggested?
> 
> Honestly I would postpone untile we have a clear idea of what is
> actually part of the PHY and what can be handled externally... Example
> setting the clock in gcc, writing a specific driver...
> 
> It's a random idea but maybe most of the stuff required for that PHY is
> just when it's connected to a switch... In that case it would all be
> handled in the switch driver (tobe extended qca8k) and all these extra
> stuff would be placed in that node instead of bloating phy nodes with
> all kind of clk and other stuff.
> 
> This series still require 2 more series (at803x splint and cleanup) to be
> actually proposed so we have some time to better define this.
> 
> What do you think?

Fair enough! Let's postpone until we really need it. I noticed this 
PHY-like-SoC discussion in the V1 comments, and it was not finished 
there neither addressed in the latest patch comment. So I asked just to 
be sure that we were finished with this. Thank you for the clarification.

--
Sergey


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 18:44       ` Andrew Lunn
@ 2024-01-07 21:57         ` Sergey Ryazanov
  0 siblings, 0 replies; 33+ messages in thread
From: Sergey Ryazanov @ 2024-01-07 21:57 UTC (permalink / raw)
  To: Andrew Lunn, Christian Marangi
  Cc: Robert Marko, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

Hi Andrew,

On 07.01.2024 20:44, Andrew Lunn wrote:
>> Honestly I would postpone untile we have a clear idea of what is
>> actually part of the PHY and what can be handled externally... Example
>> setting the clock in gcc, writing a specific driver...
> 
> That is really core of the problem here. We have no reliable
> description of the hardware. The datasheet often starts with a block
> diagram of the PHY package. Sometimes there is a product brief with
> the same diagram. We need that published. I'm not asking for the full
> data sheet, just the introductory text which gives a high level
> overview of what the hardware actually is. Then we can sketch out a
> high level Linux architecture for the PHY package.

True. I am with you on this. Can we put these words over a mailing list 
entrance door? It will save tons of time for both maintainers and 
submitters.

--
Sergey

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 18:31     ` Andrew Lunn
@ 2024-01-08 11:13       ` Jie Luo
  2024-01-08 13:19         ` Andrew Lunn
  0 siblings, 1 reply; 33+ messages in thread
From: Jie Luo @ 2024-01-08 11:13 UTC (permalink / raw)
  To: Andrew Lunn, Sergey Ryazanov
  Cc: Christian Marangi, Robert Marko, Vladimir Oltean, Rob Herring,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek



On 1/8/2024 2:31 AM, Andrew Lunn wrote:
>> And I would like to ask you about another issue raised by Vladimir [1].
>> These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
>> interrupt controllers, etc. Should we address this now? Or should we go with
>> the proposed solution for now and postpone modeling of other peripherals
>> until we get a real hardware, as Andrew suggested?
>>
>> I'm asking because it looks like we have got a real hardware. Luo currently
>> trying to push QCA8084 (multi-phy/switch chip) support, and this chip
>> exactly contains a huge clock/reset controller [2,3].
> 
> Ideally the reset controller is modelled as a Linux reset
> controller. The clock part of it is modelled using the common clock
> framework. When done correctly, the PHY should not really care. All it
> does is ask for its clock to be enabled, and its reset to be disabled.
> 
> Also, given how difficult it is proving to be to make any progress at
> all, i want to get one little part correctly described, the pure
> PHY. Once we have that, we can start on the next little part. So long
> as we keep to the Linux architecture of blocks or hardware with
> standard Linux drivers, and DT to glue them together, a small step by
> step approach should work out.
> 
>       Andrew

Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
but qca8084 also includes the integrated clock controller, this is the
first qcom PHY chip integrating the clock controller internally.
can we also consider designing the clocks and resets DT models in the
PHY package DT.

For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
it integrates the clock controller that generates the clocks to be used
by the link of PHYs, the integrated controller also provides the resets
to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
the clocks and resets for the qca8084 PHY package, these clocks and
resets are generated by the NSSCC, even for PURE phy chip qca8084, there
is also some PHY package level clocks needs to be initialized.

here is the diagram of qca8084.
__| |_______________| |__
| PCS0 |          |PCS1 |
|______|          |_____|
|_________________      |
|                |      |
|     NSSCC      |      |
|________________|      |
|_______________________|
|     |     |     |     |
|PHY1 |PHY2 |PHY3 |PHY4 |
|_____|_____|_____|_____|

let me example the initial clocks and resets for the pure PHY chip 
qca8084 as below, the clocks and resets should be put into the first
MDIO node to be initialized firstly before qca8084 PHY will work.

ethernet-phy-package@0 { 

         #address-cells = <1>; 

         #size-cells = <0>; 

         compatible = "ethernet-phy-package"; 

         reg = <0>; 

 

         /* initial PHY package level clocks */ 

         clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>, 

                <&qca8k_nsscc NSS_CC_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_TLMM_CLK>, 

                <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>, 

                <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>; 

         clock-names = "apb_bridge", 

                 "ahb", 

                 "sec_ctrl_ahb", 

                 "tlmm", 

                 "tlmm_ahb", 

                 "cnoc_ahb", 

                 "mdio_ahb"; 

 

         /* initial PHY package level reset */ 

         resets = <&qca8k_nsscc NSS_CC_DSP_ARES>; 

         reset-names = "gephy_dsp"; 

 

         /* initial clocks and resets for first phy */ 

         phy0 { 

                 reg = <0>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>; 

                 clock-names = "gephy0_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY0_ARES>; 

                 reset-names = "gephy0_sys", 

                         "gephy0_soft"; 

         }; 

 

         /* initial clocks and resets for second phy */ 

         phy1 { 

                 reg = <1>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>; 

                 clock-names = "gephy1_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY1_ARES>; 

                 reset-names = "gephy1_sys", 

                         "gephy1_soft"; 

         }; 

 

         /* initial clocks and resets for third phy */ 

         phy2 { 

                 reg = <2>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>; 

                 clock-names = "gephy2_sys";
                 resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY2_ARES>; 

                 reset-names = "gephy2_sys", 

                         "gephy2_soft"; 

         }; 

 

         /* initial clocks and resets for fourth phy */ 

         phy3 { 

                 reg = <3>; 

                 clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>; 

                 clock-names = "gephy3_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>, 

                        <&qca8k_nsscc NSS_CC_GEPHY3_ARES>; 

                 reset-names = "gephy3_sys", 

                         "gephy3_soft"; 

         }; 

 

         /* initial clocks and resets for pcs0. */ 

         pcs0 { 

                 reg = <4>; 

                 clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>; 

                 clock-names = "srds0_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>; 

                 reset-names = "srds0_sys"; 

         }; 

 

         /* initial clocks and resets for pcs1. */ 

         pcs1 { 

                 reg = <5>; 

                 clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>; 

                 clock-names = "srds1_sys"; 

                 resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>; 

                 reset-names = "srds1_sys"; 

         }; 
 

};

appreciated for the PHY package level DT model design.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-08 11:13       ` Jie Luo
@ 2024-01-08 13:19         ` Andrew Lunn
  2024-01-09 11:44           ` Jie Luo
  0 siblings, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2024-01-08 13:19 UTC (permalink / raw)
  To: Jie Luo
  Cc: Sergey Ryazanov, Christian Marangi, Robert Marko,
	Vladimir Oltean, Rob Herring, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

> Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
> but qca8084 also includes the integrated clock controller, this is the
> first qcom PHY chip integrating the clock controller internally.
> can we also consider designing the clocks and resets DT models in the
> PHY package DT.
> 
> For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
> it integrates the clock controller that generates the clocks to be used
> by the link of PHYs, the integrated controller also provides the resets
> to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
> same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
> the clocks and resets for the qca8084 PHY package, these clocks and
> resets are generated by the NSSCC, even for PURE phy chip qca8084, there
> is also some PHY package level clocks needs to be initialized.
> 
> here is the diagram of qca8084.
> __| |_______________| |__
> | PCS0 |          |PCS1 |
> |______|          |_____|
> |_________________      |
> |                |      |
> |     NSSCC      |      |
> |________________|      |
> |_______________________|
> |     |     |     |     |
> |PHY1 |PHY2 |PHY3 |PHY4 |
> |_____|_____|_____|_____|

Please add to the diagram the external clocks and external resets.

Additionally, add the resets and clocks between the NSSCC and the
individual PHYs. Typically, the internal clocks and resets are not in
DT, at last not for a single PHY. For a quad PHY in a package, it
might make sense to add them. Before we can decide that, we need a
clear idea what the hardware looks like.

> let me example the initial clocks and resets for the pure PHY chip qca8084
> as below, the clocks and resets should be put into the first
> MDIO node to be initialized firstly before qca8084 PHY will work.
> 
> ethernet-phy-package@0 {
> 
>         #address-cells = <1>;
> 
>         #size-cells = <0>;
> 
>         compatible = "ethernet-phy-package";
> 
>         reg = <0>;
> 
> 
> 
>         /* initial PHY package level clocks */
> 
>         clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_TLMM_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
> 
>                <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>;

Device tree effectively defined devices on bus, in a tree, and how
they interconnect. Does the NSSCC have its own address on the MDIO
bus? Or does it share an address with one of the PHYs? It could be we
want to describe the NSSCC as a DT node of its own within the
package. It is probably both a clock consumer, and a clock provider.
The individual PHYs are then clock consumers, of the clocks the NSSCC
exports. Same for resets.

> 
>         clock-names = "apb_bridge",
> 
>                 "ahb",
> 
>                 "sec_ctrl_ahb",
> 
>                 "tlmm",
> 
>                 "tlmm_ahb",
> 
>                 "cnoc_ahb",
> 
>                 "mdio_ahb";
> 
> 
> 
>         /* initial PHY package level reset */
> 
>         resets = <&qca8k_nsscc NSS_CC_DSP_ARES>;
> 
>         reset-names = "gephy_dsp";
> 
> 
> 
>         /* initial clocks and resets for first phy */
> 
>         phy0 {
> 
>                 reg = <0>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
> 
>                 clock-names = "gephy0_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
> 
>                 reset-names = "gephy0_sys",
> 
>                         "gephy0_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for second phy */
> 
>         phy1 {
> 
>                 reg = <1>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
> 
>                 clock-names = "gephy1_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
> 
>                 reset-names = "gephy1_sys",
> 
>                         "gephy1_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for third phy */
> 
>         phy2 {
> 
>                 reg = <2>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
> 
>                 clock-names = "gephy2_sys";
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
> 
>                 reset-names = "gephy2_sys",
> 
>                         "gephy2_soft";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for fourth phy */
> 
>         phy3 {
> 
>                 reg = <3>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
> 
>                 clock-names = "gephy3_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>,
> 
>                        <&qca8k_nsscc NSS_CC_GEPHY3_ARES>;
> 
>                 reset-names = "gephy3_sys",
> 
>                         "gephy3_soft";
> 
>         };

This is starting to look O.K.

>         /* initial clocks and resets for pcs0. */
> 
>         pcs0 {
> 
>                 reg = <4>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>;
> 
>                 clock-names = "srds0_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>;
> 
>                 reset-names = "srds0_sys";
> 
>         };
> 
> 
> 
>         /* initial clocks and resets for pcs1. */
> 
>         pcs1 {
> 
>                 reg = <5>;
> 
>                 clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
> 
>                 clock-names = "srds1_sys";
> 
>                 resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>;
> 
>                 reset-names = "srds1_sys";
> 
>         };

PCS will need further work and thinking about. Typically, they are not
described in DT for a PHY. In general, a PCS in a PHY does not have a
driver of its own, the firmware in the PHY mostly controls it, not
Linux. For the moment, lets leave them as they are, and we will come
back to them once we get the clocks and resets correctly described.

     Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-08 13:19         ` Andrew Lunn
@ 2024-01-09 11:44           ` Jie Luo
  2024-01-09 13:48             ` Andrew Lunn
  0 siblings, 1 reply; 33+ messages in thread
From: Jie Luo @ 2024-01-09 11:44 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Sergey Ryazanov, Christian Marangi, Robert Marko,
	Vladimir Oltean, Rob Herring, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek



On 1/8/2024 9:19 PM, Andrew Lunn wrote:
>> Since qca8075 PHY is also multiple port PHY, which is same as qca8084,
>> but qca8084 also includes the integrated clock controller, this is the
>> first qcom PHY chip integrating the clock controller internally.
>> can we also consider designing the clocks and resets DT models in the
>> PHY package DT.
>>
>> For qca8084 PURE PHY chip, which is the quad PHY chip and two PCSes,
>> it integrates the clock controller that generates the clocks to be used
>> by the link of PHYs, the integrated controller also provides the resets
>> to the PHY,  the clock controller(NSSCC) driver of qca8084 works at the
>> same way of the GCC of SoC(IPQ), qca8084 needs to be initialized with
>> the clocks and resets for the qca8084 PHY package, these clocks and
>> resets are generated by the NSSCC, even for PURE phy chip qca8084, there
>> is also some PHY package level clocks needs to be initialized.
>>
>> here is the diagram of qca8084.
>> __| |_______________| |__
>> | PCS0 |          |PCS1 |
>> |______|          |_____|
>> |_________________      |
>> |                |      |
>> |     NSSCC      |      |
>> |________________|      |
>> |_______________________|
>> |     |     |     |     |
>> |PHY1 |PHY2 |PHY3 |PHY4 |
>> |_____|_____|_____|_____|
> 
> Please add to the diagram the external clocks and external resets.

__| |_______________| |__
| PCS0 |          |PCS1 |
|______|          |_____|
|_______                |<---- REF clock 50MHZ
|      |------------    |
|NSSCC |    |clks  |rsts|<---- GPIO reset
|______|    |      |    |
|           V      V    |
|_______________________|
|     |     |     |     |
|PHY1 |PHY2 |PHY3 |PHY4 |
|_____|_____|_____|_____|

There are difference clock trees generated from NSSCC for the different
PHYs, all clocks and resets for qca8084 CHIP working are internally
provided by the NSSCC.

> 
> Additionally, add the resets and clocks between the NSSCC and the
> individual PHYs. Typically, the internal clocks and resets are not in
> DT, at last not for a single PHY. For a quad PHY in a package, it
> might make sense to add them. Before we can decide that, we need a
> clear idea what the hardware looks like.

Yes, that is true.
The reason why i add the DTs for the clocks and resets is these clocks
and resets are provided by the internal NSSCC provider driver, in this
chip, both the clock provider and clock consumer are located in the
qca8084 PHY chip.

> 
>> let me example the initial clocks and resets for the pure PHY chip qca8084
>> as below, the clocks and resets should be put into the first
>> MDIO node to be initialized firstly before qca8084 PHY will work.
>>
>> ethernet-phy-package@0 {
>>
>>          #address-cells = <1>;
>>
>>          #size-cells = <0>;
>>
>>          compatible = "ethernet-phy-package";
>>
>>          reg = <0>;
>>
>>
>>
>>          /* initial PHY package level clocks */
>>
>>          clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_TLMM_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
>>
>>                 <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>;
> 
> Device tree effectively defined devices on bus, in a tree, and how
> they interconnect. Does the NSSCC have its own address on the MDIO
> bus? Or does it share an address with one of the PHYs? It could be we
> want to describe the NSSCC as a DT node of its own within the
> package. It is probably both a clock consumer, and a clock provider.
> The individual PHYs are then clock consumers, of the clocks the NSSCC
> exports. Same for resets.

Yes, Andrew, the NSSCC provider driver is probed based on the MDIO
device, the PHY CHIP occupies the MDIO addresses, so the NSSCC is
registered as the MDIO device.

The following is the DT of the NSSCC driver, normally we should not
enable the clocks in the clock provider driver. The clocks also can't
be configured during the NSSCC driver probed because of the initial
clocks and resets(mentioned in this DT example ethernet-phy-package@0)
are not configured.

DT of the NSSCC device node:
mdio {
       #address-cells = <1>;
       #size-cells = <0>;

       clock-controller@18 {
         compatible = "qcom,qca8084-nsscc";
         reg = <0x18>;
         clocks = <&qca8k_xo>,
                  <&qca8k_uniphy0_rx>,
                  <&qca8k_uniphy0_tx>,
                  <&qca8k_uniphy1_rx>,
                  <&qca8k_uniphy1_tx>,
                  <&qca8k_uniphy1_rx312p5m>,
                  <&qca8k_uniphy1_tx312p5m>;
         #clock-cells = <1>;
         #reset-cells = <1>;
         #power-domain-cells = <1>;
       };
     };


> 
>>
>>          clock-names = "apb_bridge",
>>
>>                  "ahb",
>>
>>                  "sec_ctrl_ahb",
>>
>>                  "tlmm",
>>
>>                  "tlmm_ahb",
>>
>>                  "cnoc_ahb",
>>
>>                  "mdio_ahb";
>>
>>
>>
>>          /* initial PHY package level reset */
>>
>>          resets = <&qca8k_nsscc NSS_CC_DSP_ARES>;
>>
>>          reset-names = "gephy_dsp";
>>
>>
>>
>>          /* initial clocks and resets for first phy */
>>
>>          phy0 {
>>
>>                  reg = <0>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
>>
>>                  clock-names = "gephy0_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
>>
>>                  reset-names = "gephy0_sys",
>>
>>                          "gephy0_soft";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for second phy */
>>
>>          phy1 {
>>
>>                  reg = <1>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>;
>>
>>                  clock-names = "gephy1_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
>>
>>                  reset-names = "gephy1_sys",
>>
>>                          "gephy1_soft";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for third phy */
>>
>>          phy2 {
>>
>>                  reg = <2>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>;
>>
>>                  clock-names = "gephy2_sys";
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
>>
>>                  reset-names = "gephy2_sys",
>>
>>                          "gephy2_soft";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for fourth phy */
>>
>>          phy3 {
>>
>>                  reg = <3>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>;
>>
>>                  clock-names = "gephy3_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>,
>>
>>                         <&qca8k_nsscc NSS_CC_GEPHY3_ARES>;
>>
>>                  reset-names = "gephy3_sys",
>>
>>                          "gephy3_soft";
>>
>>          };
> 
> This is starting to look O.K.
> 
>>          /* initial clocks and resets for pcs0. */
>>
>>          pcs0 {
>>
>>                  reg = <4>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>;
>>
>>                  clock-names = "srds0_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>;
>>
>>                  reset-names = "srds0_sys";
>>
>>          };
>>
>>
>>
>>          /* initial clocks and resets for pcs1. */
>>
>>          pcs1 {
>>
>>                  reg = <5>;
>>
>>                  clocks = <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
>>
>>                  clock-names = "srds1_sys";
>>
>>                  resets = <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>;
>>
>>                  reset-names = "srds1_sys";
>>
>>          };
> 
> PCS will need further work and thinking about. Typically, they are not
> described in DT for a PHY. In general, a PCS in a PHY does not have a
> driver of its own, the firmware in the PHY mostly controls it, not
> Linux. For the moment, lets leave them as they are, and we will come
> back to them once we get the clocks and resets correctly described.
> 
>       Andrew

Got it.
Since there is no firmware located in the qca8084 PHY, the qca8084 PHY
is only configured by the Linux driver to make it working.

Thanks Andrew for the comments and suggestions.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-09 11:44           ` Jie Luo
@ 2024-01-09 13:48             ` Andrew Lunn
  2024-01-10  3:13               ` Jie Luo
  0 siblings, 1 reply; 33+ messages in thread
From: Andrew Lunn @ 2024-01-09 13:48 UTC (permalink / raw)
  To: Jie Luo
  Cc: Sergey Ryazanov, Christian Marangi, Robert Marko,
	Vladimir Oltean, Rob Herring, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek

> 
> __| |_______________| |__
> | PCS0 |          |PCS1 |
> |______|          |_____|
> |_______                |<---- REF clock 50MHZ
> |      |------------    |
> |NSSCC |    |clks  |rsts|<---- GPIO reset
> |______|    |      |    |
> |           V      V    |
> |_______________________|
> |     |     |     |     |
> |PHY1 |PHY2 |PHY3 |PHY4 |
> |_____|_____|_____|_____|

Not the best of improvements. So the ref clock goes to the package,
and then magically somehow gets to the NSSCC? Are there any more
blocks it goes through before reaching the NSSCC? How does the GPIO
reset get converted into multiple reset inside the package? Details,
details, details.

> There are difference clock trees generated from NSSCC for the different
> PHYs, all clocks and resets for qca8084 CHIP working are internally
> provided by the NSSCC.

So show this in the block diagram.

> Yes, Andrew, the NSSCC provider driver is probed based on the MDIO
> device, the PHY CHIP occupies the MDIO addresses, so the NSSCC is
> registered as the MDIO device.
> 
> DT of the NSSCC device node:
> mdio {
>       #address-cells = <1>;
>       #size-cells = <0>;
> 
>       clock-controller@18 {
>         compatible = "qcom,qca8084-nsscc";
>         reg = <0x18>;
>         clocks = <&qca8k_xo>,
>                  <&qca8k_uniphy0_rx>,
>                  <&qca8k_uniphy0_tx>,
>                  <&qca8k_uniphy1_rx>,
>                  <&qca8k_uniphy1_tx>,
>                  <&qca8k_uniphy1_rx312p5m>,
>                  <&qca8k_uniphy1_tx312p5m>;
>         #clock-cells = <1>;
>         #reset-cells = <1>;
>         #power-domain-cells = <1>;
>       };
>     };
 
This does not make any sense. You have one clock input, 50MHz. So why
are you listing 6 consumer clocks, not one? And where are the clocks
this clock controller provides, clock-output-names=<...>;

I give up. Please consider this PHY driver NACKed.

Get Linaro, or some other organisation with a lot of experience with
mainline to take over the work.

	 Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-09 13:48             ` Andrew Lunn
@ 2024-01-10  3:13               ` Jie Luo
  0 siblings, 0 replies; 33+ messages in thread
From: Jie Luo @ 2024-01-10  3:13 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Sergey Ryazanov, Christian Marangi, Robert Marko,
	Vladimir Oltean, Rob Herring, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley,
	Andy Gross, Bjorn Andersson, Konrad Dybcio, Heiner Kallweit,
	Russell King, Matthias Brugger, AngeloGioacchino Del Regno,
	netdev, devicetree, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-mediatek



On 1/9/2024 9:48 PM, Andrew Lunn wrote:
>>
>> __| |_______________| |__
>> | PCS0 |          |PCS1 |
>> |______|          |_____|
>> |_______                |<---- REF clock 50MHZ
>> |      |------------    |
>> |NSSCC |    |clks  |rsts|<---- GPIO reset
>> |______|    |      |    |
>> |           V      V    |
>> |_______________________|
>> |     |     |     |     |
>> |PHY1 |PHY2 |PHY3 |PHY4 |
>> |_____|_____|_____|_____|
> 
> Not the best of improvements. So the ref clock goes to the package,
> and then magically somehow gets to the NSSCC? Are there any more
> blocks it goes through before reaching the NSSCC? How does the GPIO
> reset get converted into multiple reset inside the package? Details,
> details, details.

The GPIO reset for the whole CHIP, like a GPIO reset on the single port
PHY qca8081, after the GPIO reset, the whole qca8084 chip is in the cold
reset state, so the initial clocks ans resets need to be initialized as
we are designing the clock model here.

As for the reference clock 50M input, this reference clock 50MHZ is
PLLed by the PCS1 block of qca8084, then there are fix 312.5MHZ is
generated to the NSSCC, NSSCC includes multiple RCG clock trees to
generate the clocks to the PHY and PCS, all these blocks(PCS and PHY)
are located in the qca8084 PHY chip, sorry for missed this diagram
info, updated the diagram as below, since the PCS generates the clocks
to NSSCC, there are initial clocks ans resets included for PCS need
to be configured before CHIP to work.

So the parent clocks are PCS0(uniphy0) and PCS1(uniphy1) for NSSCC,
as i provide the DT of NSSCC provider below, the parent clock rate
of PCS is changed according to the PHY link speed, and there are
also fix clock rate 312.5MHZ from PCS1, these parent clocks are
the root clock of RCG clock tree in NSSCC, the output clocks of
the RCG are consumed by the PHYs(PHY1-PHY4 and PCS0, PCS1).

    REF 50MHZ
       |
       |
______V__________________
| PCS1 |       ____|PCS0|
|______|      |    |____|
|  |  ________|         |
|  | |                  |<----------GPIO reset
|__V_V__                |
|      |------------    |
|NSSCC |    |clks  |rsts|
|______|    |      |    |
|           V      V    |
|_______________________|
|     |     |     |     |
|PHY1 |PHY2 |PHY3 |PHY4 |
|_____|_____|_____|_____|

> 
>> There are difference clock trees generated from NSSCC for the different
>> PHYs, all clocks and resets for qca8084 CHIP working are internally
>> provided by the NSSCC.
> 
> So show this in the block diagram.
> 
>> Yes, Andrew, the NSSCC provider driver is probed based on the MDIO
>> device, the PHY CHIP occupies the MDIO addresses, so the NSSCC is
>> registered as the MDIO device.
>>
>> DT of the NSSCC device node:
>> mdio {
>>        #address-cells = <1>;
>>        #size-cells = <0>;
>>
>>        clock-controller@18 {
>>          compatible = "qcom,qca8084-nsscc";
>>          reg = <0x18>;
>>          clocks = <&qca8k_xo>,
>>                   <&qca8k_uniphy0_rx>,
>>                   <&qca8k_uniphy0_tx>,
>>                   <&qca8k_uniphy1_rx>,
>>                   <&qca8k_uniphy1_tx>,
>>                   <&qca8k_uniphy1_rx312p5m>,
>>                   <&qca8k_uniphy1_tx312p5m>;
>>          #clock-cells = <1>;
>>          #reset-cells = <1>;
>>          #power-domain-cells = <1>;
>>        };
>>      };
>   
> This does not make any sense. You have one clock input, 50MHz. So why
> are you listing 6 consumer clocks, not one? And where are the clocks
> this clock controller provides, clock-output-names=<...>;

Hi Andrew,
Sorry for the confusion.
the DT of the NSSCC device node is from the NSSCC provider driver, which
is under review currently as the link below.
https://lore.kernel.org/lkml/20231104034858.9159-2-quic_luoj@quicinc.com/T/#m204c22d14be8f9dda7cd7f666ed726b8fc3301ef

the property "clocks" are the parent clock list of RCG clock tree in
the NSSCC driver, for qca8084 chip, there are two UNIPHY(PCS) as
mentioned in the diagram, there are also fix rate clocks 312.5MHZ,
the output clocks of NSSCC are not listed in the DT, which is same
as the GCC(IPQ SoC) DT.

The output clocks of NSSCC are provided by the DT of the clock consumer
such as the DT of qca8084 as below.
phy0 {
         reg = <0>;
         clocks = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>;
         clock-names = "gephy0_sys";
         resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
                  <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
         reset-names = "gephy0_sys", "gephy0_soft";
      };

In the DT node above, the qca8k_nssc refers to the NSSCC clock provider
DT node "clock-controller@18", the clock ID "NSS_CC_GEPHY0_SYS_CLK" is
also provided by the NSSCC clock provider driver.

The different PHY(PHY1-PHY4) of qca8084 has the different clock
tree(RCG), the clock consumer driver needs to define the DT properties
clocks and clcok-names to get the clocks from the NSSCC clock provider.

> 
> I give up. Please consider this PHY driver NACKed.
> 
> Get Linaro, or some other organisation with a lot of experience with
> mainline to take over the work.
> 
> 	 Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-07 21:49       ` Sergey Ryazanov
  2024-01-07 21:45         ` Christian Marangi
@ 2024-01-17  0:36         ` Christian Marangi
  2024-01-17 19:39           ` Sergey Ryazanov
  1 sibling, 1 reply; 33+ messages in thread
From: Christian Marangi @ 2024-01-17  0:36 UTC (permalink / raw)
  To: Sergey Ryazanov
  Cc: Robert Marko, Andrew Lunn, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

On Sun, Jan 07, 2024 at 11:49:12PM +0200, Sergey Ryazanov wrote:
> Hi Christian,
> 
> On 07.01.2024 20:30, Christian Marangi wrote:
> > On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
> > > On 26.11.2023 03:53, Christian Marangi wrote:
> > > > Document ethernet PHY package nodes used to describe PHY shipped in
> > > > bundle of 4-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 package 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 and declare the
> > > > "ethrnet-phy-package" compatible.
> > > > 
> > > > Each reg of the PHY defined in the PHY package node is absolute and will
> > > > reference the real address of the PHY on the bus.
> > > > 
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > >    .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
> > > >    1 file changed, 75 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..244d4bc29164
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > > > @@ -0,0 +1,75 @@
> > > > +# 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:
> > > > +  This schema describe PHY package as simple container for
> > > > +  a bundle of PHYs that share the same properties and
> > > > +  contains the PHYs of the package themself.
> > > > +
> > > > +  Each reg of the PHYs defined in the PHY package node is
> > > > +  absolute and describe the real address of the PHY on the bus.
> > > > +
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> > > > +
> > > > +  compatible:
> > > > +    const: ethernet-phy-package
> > > > +
> > > > +  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 exist on the device (just not attached to anything).
> > > > +      The reg defined in the PHY package node might differ and
> > > > +      the related PHY might be not defined.
> > > > +
> > > > +  '#address-cells':
> > > > +    const: 1
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > > > +
> > > > +patternProperties:
> > > > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > > > +    $ref: ethernet-phy.yaml#
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +additionalProperties: true
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    mdio {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        ethernet-phy-package@16 {
> > > > +            #address-cells = <1>;
> > > > +            #size-cells = <0>;
> > > > +            compatible = "ethernet-phy-package";
> > > > +            reg = <0x16>;
> > > > +
> > > > +            ethernet-phy@16 {
> > > > +              reg = <0x16>;
> > > > +            };
> > > > +
> > > > +            phy4: ethernet-phy@1a {
> > > > +              reg = <0x1a>;
> > > > +            };
> > > > +        };
> > > > +    };
> > > 
> > > So, we ended up on a design where we use the predefined compatible string
> > > 'ethernet-phy-package' to recognize a phy package inside the
> > > of_mdiobus_register() function. During the V1 discussion, Vladimir came up
> > > with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
> > > recognize a phy package in of_mdiobus_register()? IMHO this will give us a
> > > clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
> > > the same address range as the parent node. Also we can list all child
> > > addresses in 'reg' to mark them occupied.
> > > 
> > >    mdio {
> > >      ...
> > > 
> > >      ethernet-phy-package@16 {
> > >        compatible = "qcom,qca8075";
> > >        reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
> > >        ranges;
> > >        ...
> > > 
> > >        ethernet-phy@16 {
> > >          reg = <0x16>;
> > >        };
> > > 
> > >        ethernet-phy@1a {
> > >          reg = <0x1a>;
> > >        };
> > >      };
> > >    };
> > > 
> > > Did you find some issues with the 'ranges' conception?
> > 
> > Nope it's ok but it might pose some confusion with the idea that the
> > very first element MUST be THE STARTING ADDR of the PHY package. (people
> > might think that it's just the list of the PHYs in the package and
> > remove the hardware unconnected ones... but that would be fault of who
> > write the DT anyway.)
> 
> Make sense. I do not insist on addresses listing. Mainly I'm thinking of a
> proper way to show that child nodes are accessible directly on the parent
> bus, and introducing the special compatibility string, while we already have
> the 'ranges' property.
> 
> But it's good to know Rob's opinion on whether it is conceptually right to
> use 'ranges' here.
>

I wonder if something like this might make sense... Thing is that with
the ranges property we would have the define the address in the PHY
Package node as offsets...

An example would be

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

        ethernet-phy-package@10 {
            #address-cells = <1>;
            #size-cells = <0>;
            compatible = "qcom,qca807x-package";
            reg = <0x10>; 
            ranges = <0x0 0x10 0x5>;

            qcom,package-mode = "qsgmii";

            ethernet-phy@0 {
                reg = <0>;

                leds {

		...

With a PHY Package at 0x10, that span 5 address and the child starts at
0x0 offset.

This way we would be very precise on describing the amount of address
used by the PHY Package without having to define the PHY not actually
connected.

PHY needs to be at an offset to make sense of the ranges first element
property (0x0). With a non offset way we would have to have something
like

ranges = <0x10 0x10 0x5>;

With the child and tha parent always matching.

(this is easy to handle in the parsing and probe as we will just
calculate the real address based on the base address of the PHY package
+ offset)

I hope Rob can give more feedback about this, is this what you were
thinking with the usage of ranges property?

(this has also the bonus point of introducing some validation in the PHY
core code to make sure the right amount of PHY are defined in the
package by checking if the number of PHY doesn't exceed the value set in
ranges.)

> > > And I would like to ask you about another issue raised by Vladimir [1].
> > > These phy chips become SoC with all these built-in PHYs, PCSs, clocks,
> > > interrupt controllers, etc. Should we address this now? Or should we go with
> > > the proposed solution for now and postpone modeling of other peripherals
> > > until we get a real hardware, as Andrew suggested?
> > 
> > Honestly I would postpone untile we have a clear idea of what is
> > actually part of the PHY and what can be handled externally... Example
> > setting the clock in gcc, writing a specific driver...
> > 
> > It's a random idea but maybe most of the stuff required for that PHY is
> > just when it's connected to a switch... In that case it would all be
> > handled in the switch driver (tobe extended qca8k) and all these extra
> > stuff would be placed in that node instead of bloating phy nodes with
> > all kind of clk and other stuff.
> > 
> > This series still require 2 more series (at803x splint and cleanup) to be
> > actually proposed so we have some time to better define this.
> > 
> > What do you think?
> 
> Fair enough! Let's postpone until we really need it. I noticed this
> PHY-like-SoC discussion in the V1 comments, and it was not finished there
> neither addressed in the latest patch comment. So I asked just to be sure
> that we were finished with this. Thank you for the clarification.
> 
> --
> Sergey
> 

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-17  0:36         ` Christian Marangi
@ 2024-01-17 19:39           ` Sergey Ryazanov
  2024-01-17 20:03             ` Christian Marangi
  2024-01-17 20:05             ` Andrew Lunn
  0 siblings, 2 replies; 33+ messages in thread
From: Sergey Ryazanov @ 2024-01-17 19:39 UTC (permalink / raw)
  To: Christian Marangi
  Cc: Robert Marko, Andrew Lunn, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

Hi Christian,

On 17.01.2024 02:36, Christian Marangi wrote:
> On Sun, Jan 07, 2024 at 11:49:12PM +0200, Sergey Ryazanov wrote:
>> Hi Christian,
>>
>> On 07.01.2024 20:30, Christian Marangi wrote:
>>> On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
>>>> On 26.11.2023 03:53, Christian Marangi wrote:
>>>>> Document ethernet PHY package nodes used to describe PHY shipped in
>>>>> bundle of 4-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 package 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 and declare the
>>>>> "ethrnet-phy-package" compatible.
>>>>>
>>>>> Each reg of the PHY defined in the PHY package node is absolute and will
>>>>> reference the real address of the PHY on the bus.
>>>>>
>>>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>>>> ---
>>>>>     .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
>>>>>     1 file changed, 75 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..244d4bc29164
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
>>>>> @@ -0,0 +1,75 @@
>>>>> +# 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:
>>>>> +  This schema describe PHY package as simple container for
>>>>> +  a bundle of PHYs that share the same properties and
>>>>> +  contains the PHYs of the package themself.
>>>>> +
>>>>> +  Each reg of the PHYs defined in the PHY package node is
>>>>> +  absolute and describe the real address of the PHY on the bus.
>>>>> +
>>>>> +properties:
>>>>> +  $nodename:
>>>>> +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
>>>>> +
>>>>> +  compatible:
>>>>> +    const: ethernet-phy-package
>>>>> +
>>>>> +  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 exist on the device (just not attached to anything).
>>>>> +      The reg defined in the PHY package node might differ and
>>>>> +      the related PHY might be not defined.
>>>>> +
>>>>> +  '#address-cells':
>>>>> +    const: 1
>>>>> +
>>>>> +  '#size-cells':
>>>>> +    const: 0
>>>>> +
>>>>> +patternProperties:
>>>>> +  ^ethernet-phy(@[a-f0-9]+)?$:
>>>>> +    $ref: ethernet-phy.yaml#
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +
>>>>> +additionalProperties: true
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    mdio {
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +
>>>>> +        ethernet-phy-package@16 {
>>>>> +            #address-cells = <1>;
>>>>> +            #size-cells = <0>;
>>>>> +            compatible = "ethernet-phy-package";
>>>>> +            reg = <0x16>;
>>>>> +
>>>>> +            ethernet-phy@16 {
>>>>> +              reg = <0x16>;
>>>>> +            };
>>>>> +
>>>>> +            phy4: ethernet-phy@1a {
>>>>> +              reg = <0x1a>;
>>>>> +            };
>>>>> +        };
>>>>> +    };
>>>>
>>>> So, we ended up on a design where we use the predefined compatible string
>>>> 'ethernet-phy-package' to recognize a phy package inside the
>>>> of_mdiobus_register() function. During the V1 discussion, Vladimir came up
>>>> with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
>>>> recognize a phy package in of_mdiobus_register()? IMHO this will give us a
>>>> clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
>>>> the same address range as the parent node. Also we can list all child
>>>> addresses in 'reg' to mark them occupied.
>>>>
>>>>     mdio {
>>>>       ...
>>>>
>>>>       ethernet-phy-package@16 {
>>>>         compatible = "qcom,qca8075";
>>>>         reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
>>>>         ranges;
>>>>         ...
>>>>
>>>>         ethernet-phy@16 {
>>>>           reg = <0x16>;
>>>>         };
>>>>
>>>>         ethernet-phy@1a {
>>>>           reg = <0x1a>;
>>>>         };
>>>>       };
>>>>     };
>>>>
>>>> Did you find some issues with the 'ranges' conception?
>>>
>>> Nope it's ok but it might pose some confusion with the idea that the
>>> very first element MUST be THE STARTING ADDR of the PHY package. (people
>>> might think that it's just the list of the PHYs in the package and
>>> remove the hardware unconnected ones... but that would be fault of who
>>> write the DT anyway.)
>>
>> Make sense. I do not insist on addresses listing. Mainly I'm thinking of a
>> proper way to show that child nodes are accessible directly on the parent
>> bus, and introducing the special compatibility string, while we already have
>> the 'ranges' property.
>>
>> But it's good to know Rob's opinion on whether it is conceptually right to
>> use 'ranges' here.
>>
> 
> I wonder if something like this might make sense... Thing is that with
> the ranges property we would have the define the address in the PHY
> Package node as offsets...
> 
> An example would be
> 
>      mdio {
>          #address-cells = <1>;
>          #size-cells = <0>;
> 
>          ethernet-phy-package@10 {
>              #address-cells = <1>;
>              #size-cells = <0>;
>              compatible = "qcom,qca807x-package";
>              reg = <0x10>;
>              ranges = <0x0 0x10 0x5>;
> 
>              qcom,package-mode = "qsgmii";
> 
>              ethernet-phy@0 {
>                  reg = <0>;
> 
>                  leds {
> 
> 		...
> 
> With a PHY Package at 0x10, that span 5 address and the child starts at
> 0x0 offset.
> 
> This way we would be very precise on describing the amount of address
> used by the PHY Package without having to define the PHY not actually
> connected.
> 
> PHY needs to be at an offset to make sense of the ranges first element
> property (0x0). With a non offset way we would have to have something
> like
> 
> ranges = <0x10 0x10 0x5>;
> 
> With the child and tha parent always matching.
> 
> (this is easy to handle in the parsing and probe as we will just
> calculate the real address based on the base address of the PHY package
> + offset)

On one hand it makes sense and looks useful for software development. On 
another, it looks like a violation of the main DT designing rule, when 
DT should be used to describe that hardware properties, which can not be 
learnt from other sources.

As far as I understand this specific chip, each of embedded PHYs has its 
own MDIO bus address and not an offset from a main common address. 
Correct me please, if I am got it wrong.

> I hope Rob can give more feedback about this, is this what you were
> thinking with the usage of ranges property?
> 
> (this has also the bonus point of introducing some validation in the PHY
> core code to make sure the right amount of PHY are defined in the
> package by checking if the number of PHY doesn't exceed the value set in
> ranges.)

Yep, I am also would like to hear some clarification from Rob regarding 
acceptable 'range' property usage and may be some advice on how to 
specify the size of occupied addresses. Rob?

--
Sergey

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-17 19:39           ` Sergey Ryazanov
@ 2024-01-17 20:03             ` Christian Marangi
  2024-01-17 20:05             ` Andrew Lunn
  1 sibling, 0 replies; 33+ messages in thread
From: Christian Marangi @ 2024-01-17 20:03 UTC (permalink / raw)
  To: Sergey Ryazanov
  Cc: Robert Marko, Andrew Lunn, Vladimir Oltean, Rob Herring, Luo Jie,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Krzysztof Kozlowski, Conor Dooley, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, Heiner Kallweit, Russell King, Matthias Brugger,
	AngeloGioacchino Del Regno, netdev, devicetree, linux-kernel,
	linux-arm-msm, linux-arm-kernel, linux-mediatek

On Wed, Jan 17, 2024 at 09:39:25PM +0200, Sergey Ryazanov wrote:
> Hi Christian,
> 
> On 17.01.2024 02:36, Christian Marangi wrote:
> > On Sun, Jan 07, 2024 at 11:49:12PM +0200, Sergey Ryazanov wrote:
> > > Hi Christian,
> > > 
> > > On 07.01.2024 20:30, Christian Marangi wrote:
> > > > On Sun, Jan 07, 2024 at 08:00:33PM +0200, Sergey Ryazanov wrote:
> > > > > On 26.11.2023 03:53, Christian Marangi wrote:
> > > > > > Document ethernet PHY package nodes used to describe PHY shipped in
> > > > > > bundle of 4-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 package 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 and declare the
> > > > > > "ethrnet-phy-package" compatible.
> > > > > > 
> > > > > > Each reg of the PHY defined in the PHY package node is absolute and will
> > > > > > reference the real address of the PHY on the bus.
> > > > > > 
> > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > ---
> > > > > >     .../bindings/net/ethernet-phy-package.yaml    | 75 +++++++++++++++++++
> > > > > >     1 file changed, 75 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..244d4bc29164
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > > > > > @@ -0,0 +1,75 @@
> > > > > > +# 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:
> > > > > > +  This schema describe PHY package as simple container for
> > > > > > +  a bundle of PHYs that share the same properties and
> > > > > > +  contains the PHYs of the package themself.
> > > > > > +
> > > > > > +  Each reg of the PHYs defined in the PHY package node is
> > > > > > +  absolute and describe the real address of the PHY on the bus.
> > > > > > +
> > > > > > +properties:
> > > > > > +  $nodename:
> > > > > > +    pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
> > > > > > +
> > > > > > +  compatible:
> > > > > > +    const: ethernet-phy-package
> > > > > > +
> > > > > > +  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 exist on the device (just not attached to anything).
> > > > > > +      The reg defined in the PHY package node might differ and
> > > > > > +      the related PHY might be not defined.
> > > > > > +
> > > > > > +  '#address-cells':
> > > > > > +    const: 1
> > > > > > +
> > > > > > +  '#size-cells':
> > > > > > +    const: 0
> > > > > > +
> > > > > > +patternProperties:
> > > > > > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > > > > > +    $ref: ethernet-phy.yaml#
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - reg
> > > > > > +
> > > > > > +additionalProperties: true
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    mdio {
> > > > > > +        #address-cells = <1>;
> > > > > > +        #size-cells = <0>;
> > > > > > +
> > > > > > +        ethernet-phy-package@16 {
> > > > > > +            #address-cells = <1>;
> > > > > > +            #size-cells = <0>;
> > > > > > +            compatible = "ethernet-phy-package";
> > > > > > +            reg = <0x16>;
> > > > > > +
> > > > > > +            ethernet-phy@16 {
> > > > > > +              reg = <0x16>;
> > > > > > +            };
> > > > > > +
> > > > > > +            phy4: ethernet-phy@1a {
> > > > > > +              reg = <0x1a>;
> > > > > > +            };
> > > > > > +        };
> > > > > > +    };
> > > > > 
> > > > > So, we ended up on a design where we use the predefined compatible string
> > > > > 'ethernet-phy-package' to recognize a phy package inside the
> > > > > of_mdiobus_register() function. During the V1 discussion, Vladimir came up
> > > > > with the idea of 'ranges' property usage [1]. Can we use 'ranges' to
> > > > > recognize a phy package in of_mdiobus_register()? IMHO this will give us a
> > > > > clear DT solution. I mean 'ranges' clearly indicates that child nodes are in
> > > > > the same address range as the parent node. Also we can list all child
> > > > > addresses in 'reg' to mark them occupied.
> > > > > 
> > > > >     mdio {
> > > > >       ...
> > > > > 
> > > > >       ethernet-phy-package@16 {
> > > > >         compatible = "qcom,qca8075";
> > > > >         reg = <0x16>, <0x17>, <0x18>, <0x19>, <0x1a>;
> > > > >         ranges;
> > > > >         ...
> > > > > 
> > > > >         ethernet-phy@16 {
> > > > >           reg = <0x16>;
> > > > >         };
> > > > > 
> > > > >         ethernet-phy@1a {
> > > > >           reg = <0x1a>;
> > > > >         };
> > > > >       };
> > > > >     };
> > > > > 
> > > > > Did you find some issues with the 'ranges' conception?
> > > > 
> > > > Nope it's ok but it might pose some confusion with the idea that the
> > > > very first element MUST be THE STARTING ADDR of the PHY package. (people
> > > > might think that it's just the list of the PHYs in the package and
> > > > remove the hardware unconnected ones... but that would be fault of who
> > > > write the DT anyway.)
> > > 
> > > Make sense. I do not insist on addresses listing. Mainly I'm thinking of a
> > > proper way to show that child nodes are accessible directly on the parent
> > > bus, and introducing the special compatibility string, while we already have
> > > the 'ranges' property.
> > > 
> > > But it's good to know Rob's opinion on whether it is conceptually right to
> > > use 'ranges' here.
> > > 
> > 
> > I wonder if something like this might make sense... Thing is that with
> > the ranges property we would have the define the address in the PHY
> > Package node as offsets...
> > 
> > An example would be
> > 
> >      mdio {
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> > 
> >          ethernet-phy-package@10 {
> >              #address-cells = <1>;
> >              #size-cells = <0>;
> >              compatible = "qcom,qca807x-package";
> >              reg = <0x10>;
> >              ranges = <0x0 0x10 0x5>;
> > 
> >              qcom,package-mode = "qsgmii";
> > 
> >              ethernet-phy@0 {
> >                  reg = <0>;
> > 
> >                  leds {
> > 
> > 		...
> > 
> > With a PHY Package at 0x10, that span 5 address and the child starts at
> > 0x0 offset.
> > 
> > This way we would be very precise on describing the amount of address
> > used by the PHY Package without having to define the PHY not actually
> > connected.
> > 
> > PHY needs to be at an offset to make sense of the ranges first element
> > property (0x0). With a non offset way we would have to have something
> > like
> > 
> > ranges = <0x10 0x10 0x5>;
> > 
> > With the child and tha parent always matching.
> > 
> > (this is easy to handle in the parsing and probe as we will just
> > calculate the real address based on the base address of the PHY package
> > + offset)
> 
> On one hand it makes sense and looks useful for software development. On
> another, it looks like a violation of the main DT designing rule, when DT
> should be used to describe that hardware properties, which can not be learnt
> from other sources.
> 
> As far as I understand this specific chip, each of embedded PHYs has its own
> MDIO bus address and not an offset from a main common address. Correct me
> please, if I am got it wrong.
>

Correct, it's a quad PHY but each PHY have it's own address and they
can't be changed (they are incremental from the base one)

We have lots of device that have this (it's present in 99% of the the
ipq40xx and ipq807x SoC) and the common setup is putting the quad PHY
and connect only some port to it. (the address are occupiaed anyway just
the ethernet port is not connected)

Honestly I don't think it's a violation, IMHO quite the opposite. If DT
is there to describe the hardware, defining each PHY of the package as a
separate one is wrong and actually cause confusion by dropping entirely
any description that they are indeed in a package and that on the BUS
there may be address used without anything connected.

I'm more convinced with the ranges property as we can't put a size to
the reg property. (due to mdio node having address-cell to 0, changing
this will affect also every other phy node and that is problematic)

> > I hope Rob can give more feedback about this, is this what you were
> > thinking with the usage of ranges property?
> > 
> > (this has also the bonus point of introducing some validation in the PHY
> > core code to make sure the right amount of PHY are defined in the
> > package by checking if the number of PHY doesn't exceed the value set in
> > ranges.)
> 
> Yep, I am also would like to hear some clarification from Rob regarding
> acceptable 'range' property usage and may be some advice on how to specify
> the size of occupied addresses. Rob?
> 
> --
> Sergey

-- 
	Ansuel

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes
  2024-01-17 19:39           ` Sergey Ryazanov
  2024-01-17 20:03             ` Christian Marangi
@ 2024-01-17 20:05             ` Andrew Lunn
  1 sibling, 0 replies; 33+ messages in thread
From: Andrew Lunn @ 2024-01-17 20:05 UTC (permalink / raw)
  To: Sergey Ryazanov
  Cc: Christian Marangi, Robert Marko, Vladimir Oltean, Rob Herring,
	Luo Jie, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
	Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
	Matthias Brugger, AngeloGioacchino Del Regno, netdev, devicetree,
	linux-kernel, linux-arm-msm, linux-arm-kernel, linux-mediatek

> On one hand it makes sense and looks useful for software development. On
> another, it looks like a violation of the main DT designing rule, when DT
> should be used to describe that hardware properties, which can not be learnt
> from other sources.
> 
> As far as I understand this specific chip, each of embedded PHYs has its own
> MDIO bus address and not an offset from a main common address. Correct me
> please, if I am got it wrong.

I don't have the datasheet for this specific PHY. But the concept of a
quad PHY in one package is well known. Take for example the
VSC8584. The datasheet is open on Microchips website. It has four
strapping pins to determine the addresses of the PHYs in the
package. The PHY number, 0 - 3 determine bits 0-1 of the MDIO
address. The 4 strapping pins then determine bit 2-5 of the address.

In theory, each PHY could have its own strapping pins, allowing it to
be set to any address, and two PHYs could even have the same
address. But i doubt anybody actually builds hardware like that. I
expect the base addresses is set at the package level, and then PHYs
are just offsets from this.

So to me, a range property does seem reasonable. However, I agree, we
need acceptance from the DT Maintainers.

     Andrew

^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2024-01-17 20:26 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-26  1:53 [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Christian Marangi
2023-11-26  1:53 ` [net-next PATCH RFC v3 1/8] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
2023-11-27 22:16   ` Rob Herring
2023-11-28  0:09     ` Andrew Lunn
2024-01-07 18:00   ` Sergey Ryazanov
2024-01-07 18:30     ` Christian Marangi
2024-01-07 18:44       ` Andrew Lunn
2024-01-07 21:57         ` Sergey Ryazanov
2024-01-07 21:49       ` Sergey Ryazanov
2024-01-07 21:45         ` Christian Marangi
2024-01-17  0:36         ` Christian Marangi
2024-01-17 19:39           ` Sergey Ryazanov
2024-01-17 20:03             ` Christian Marangi
2024-01-17 20:05             ` Andrew Lunn
2024-01-07 18:31     ` Andrew Lunn
2024-01-08 11:13       ` Jie Luo
2024-01-08 13:19         ` Andrew Lunn
2024-01-09 11:44           ` Jie Luo
2024-01-09 13:48             ` Andrew Lunn
2024-01-10  3:13               ` Jie Luo
2023-11-26  1:53 ` [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT Christian Marangi
2023-11-28  0:39   ` Andrew Lunn
2023-11-28 12:09     ` Christian Marangi
2023-11-26  1:53 ` [net-next PATCH RFC v3 3/8] net: phy: add support for shared priv data size " Christian Marangi
2023-11-26  1:53 ` [net-next PATCH RFC v3 4/8] net: phy: add support for driver specific PHY package probe/config Christian Marangi
2023-11-26  1:53 ` [net-next PATCH RFC v3 5/8] dt-bindings: net: add QCA807x PHY defines Christian Marangi
2023-11-27 22:36   ` Rob Herring
2023-11-26  1:53 ` [net-next PATCH RFC v3 6/8] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
2023-11-27 22:35   ` Rob Herring
2023-11-28  0:30   ` Andrew Lunn
2023-11-26  1:53 ` [net-next PATCH RFC v3 7/8] net: phy: add Qualcom QCA807x driver Christian Marangi
2023-11-26  1:53 ` [net-next PATCH RFC v3 8/8] net: phy: qca807x: Add support for configurable LED Christian Marangi
2023-11-28  0:20 ` [net-next PATCH RFC v3 0/8] net: phy: Support DT PHY package Andrew Lunn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).