* [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept
@ 2024-02-01 15:17 Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
` (8 more replies)
0 siblings, 9 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Idea of this big series is to introduce the concept of PHY package in DT
and give PHY drivers a way to derive the base address from DT.
The concept of PHY package is nothing new and is already a thing in the
kernel with the API phy_package_join/leave/read/write.
What is currently lacking is describing this in DT and better reference
a base address to calculate offset from.
In the scenario of a PHY package where multiple address are used and
there isn't a way to get the base address of the PHY package from some
regs, getting the information from DT is the only way.
A possible example to this problem is this:
ethernet-phy-package@0 {
compatible = "qcom,qca807x-package";
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
qcom,package-mode = "qsgmii";
ethernet-phy@1 {
reg = <1>;
};
phy4: ethernet-phy@4 {
reg = <4>;
};
};
The mdio parse functions are changed to address for this additional
special node, the function is changed to simply detect this node and
search also in this. (we match the node name to be "ethernet-phy-package")
PHY driver can then use introduced helper of_phy_package_join to join the
PHY to the PHY package and derive the base address from DT.
The base addr + offset implementation adds additional complexity to
the code but I think will make DT reviwers happier since the absolute
reg implementation might make things confusing with having double reg
in the DTS. I'm open to any alternative implementation and also to
revert this to the absolute implementation.
Changes v5:
- Rebase on top of net-next
- Change implementation to base addr + offset in subnode
- Adapt to all the changes and cleanup done to at803x
Changes v4:
- Rework DT implementation
- Drop of autojoin support and rework to simple helper
- Rework PHY driver to the new implementation
- Add compatible for qca807x package
- Further cleanup patches
Changes v3:
- Add back compatible implementation
- Detach patch that can be handled separately (phy_package_mmd,
phy_package extended)
- Rework code to new simplified implementation with base addr + offset
- Improve documentation with additional info and description
Changes v2:
- Drop compatible "ethernet-phy-package", use node name prefix matching
instead
- Improve DT example
- Add reg for ethernet-phy-package
- Drop phy-mode for ethernet-phy-package
- Drop patch for generalization of phy-mode
- Drop global-phy property (handle internally to the PHY driver)
- Rework OF phy package code and PHY driver to handle base address
- Fix missing of_node_put
- Add some missing docs for added variables in struct
- Move some define from dt-bindings include to PHY driver
- Handle qsgmii validation in PHY driver
- Fix wrong include for gpiolib
- Drop reduntant version.h include
Christian Marangi (7):
dt-bindings: net: document ethernet PHY package nodes
net: phy: add support for scanning PHY in PHY packages nodes
net: phy: add devm/of_phy_package_join helper
net: phy: qcom: move more function to shared library
dt-bindings: net: Document Qcom QCA807x PHY package
net: phy: qcom: generalize some qca808x LED functions
net: phy: qca807x: add support for configurable LED
Robert Marko (2):
dt-bindings: net: add QCA807x PHY defines
net: phy: qcom: add support for QCA807x PHY Family
.../bindings/net/ethernet-phy-package.yaml | 55 ++
.../devicetree/bindings/net/qcom,qca807x.yaml | 142 +++
drivers/net/mdio/of_mdio.c | 75 +-
drivers/net/phy/mdio_bus.c | 44 +-
drivers/net/phy/phy_device.c | 84 ++
drivers/net/phy/qcom/Kconfig | 8 +
drivers/net/phy/qcom/Makefile | 1 +
drivers/net/phy/qcom/at803x.c | 35 -
drivers/net/phy/qcom/qca807x.c | 832 ++++++++++++++++++
drivers/net/phy/qcom/qca808x.c | 311 +------
drivers/net/phy/qcom/qcom-phy-lib.c | 247 ++++++
drivers/net/phy/qcom/qcom.h | 123 +++
include/dt-bindings/net/qcom-qca807x.h | 30 +
include/linux/of_mdio.h | 26 +
include/linux/phy.h | 6 +
15 files changed, 1648 insertions(+), 371 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
create mode 100644 drivers/net/phy/qcom/qca807x.c
create mode 100644 include/dt-bindings/net/qcom-qca807x.h
--
2.43.0
^ permalink raw reply [flat|nested] 41+ messages in thread
* [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-02 20:52 ` Rob Herring
2024-02-01 15:17 ` [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes Christian Marangi
` (7 subsequent siblings)
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Document ethernet PHY package nodes used to describe PHY shipped in
bundle of 2-5 PHY. The special node describe a container of PHY that
share common properties. This is a generic schema and PHY package
should create specialized version with the required additional shared
properties.
Example are PHY packages that have some regs only in one PHY of the
package and will affect every other PHY in the package, for example
related to PHY interface mode calibration or global PHY mode selection.
The PHY package node MUST declare the base address used by the PHY driver
for global configuration by calculating the offsets of the global PHY
based on the base address of the PHY package.
Each reg of the PHYs defined in the PHY Package node is an offset of the
PHY Package reg.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
.../bindings/net/ethernet-phy-package.yaml | 55 +++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
new file mode 100644
index 000000000000..d7cdbb1a4b3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ethernet PHY Package Common Properties
+
+maintainers:
+ - Christian Marangi <ansuelsmth@gmail.com>
+
+description:
+ PHY packages are multi-port Ethernet PHY of the same family
+ and each Ethernet PHY is affected by the global configuration
+ of the PHY package.
+
+ Each reg of the PHYs defined in the PHY Package node is
+ an offset of the PHY Package reg.
+
+ Each Ethernet PHYs defined in the PHY package node is
+ reachable in the MDIO bus at the address of the PHY
+ Package offset of the Ethernet PHY reg.
+
+properties:
+ $nodename:
+ pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
+
+ reg:
+ minimum: 0
+ maximum: 31
+ description:
+ The base ID number for the PHY package.
+ Commonly the ID of the first PHY in the PHY package.
+
+ Some PHY in the PHY package might be not defined but
+ still occupy ID on the device (just not attached to
+ anything) hence the PHY package reg might correspond
+ to a not attached PHY (offset 0).
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+patternProperties:
+ ^ethernet-phy(@[a-f0-9]+)?$:
+ $ref: ethernet-phy.yaml#
+
+required:
+ - reg
+ - '#address-cells'
+ - '#size-cells'
+
+additionalProperties: true
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-01 16:25 ` Antoine Tenart
2024-02-01 15:17 ` [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper Christian Marangi
` (6 subsequent siblings)
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Add support for scanning PHY in PHY packages nodes. PHY packages nodes
are just container for actual PHY on the MDIO bus.
Their PHY address is defined as offset of the PHY package base address
defined in DT. of_mdio_parse_addr_offset helper is introduced to
validate the final address is correct.
mdio_bus.c and of_mdio.c is updated to now support and parse also
PHY package subnote by checking if the node name match
"ethernet-phy-package".
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/mdio/of_mdio.c | 75 +++++++++++++++++++++++++++-----------
drivers/net/phy/mdio_bus.c | 44 +++++++++++++++++-----
include/linux/of_mdio.h | 26 +++++++++++++
3 files changed, 115 insertions(+), 30 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 64ebcb6d235c..58b54c644f11 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -139,6 +139,54 @@ 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,
+ int base_addr, bool *scanphys)
+{
+ struct device_node *child;
+ int addr, rc = 0;
+
+ /* Loop over the child nodes and register a phy_device for each phy */
+ for_each_available_child_of_node(np, child) {
+ if (of_node_name_eq(child, "ethernet-phy-package")) {
+ rc = of_property_read_u32(child, "reg", &addr);
+ if (rc)
+ goto exit;
+
+ rc = __of_mdiobus_parse_phys(mdio, child, addr, scanphys);
+ if (rc && rc != -ENODEV)
+ goto exit;
+
+ continue;
+ }
+
+ if (base_addr)
+ addr = of_mdio_parse_addr_offset(&mdio->dev, child, base_addr);
+ else
+ 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 +228,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, 0, &scanphys);
+ if (rc)
+ goto unregister;
if (!scanphys)
return 0;
@@ -227,15 +259,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 afbad1ad8683..7737d0101d7b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -459,20 +459,33 @@ EXPORT_SYMBOL(of_mdio_find_bus);
* found, set the of_node pointer for the mdio device. This allows
* auto-probed phy devices to be supplied with information passed in
* via DT.
+ * If a PHY package is found, PHY is searched also there.
*/
-static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
- struct mdio_device *mdiodev)
+static int of_mdiobus_find_phy(struct device *dev, struct mdio_device *mdiodev,
+ struct device_node *np, int base_addr)
{
- struct device *dev = &mdiodev->dev;
struct device_node *child;
- if (dev->of_node || !bus->dev.of_node)
- return;
+ for_each_available_child_of_node(np, child) {
+ int addr, ret;
- for_each_available_child_of_node(bus->dev.of_node, child) {
- int addr;
+ if (of_node_name_eq(child, "ethernet-phy-package")) {
+ ret = of_property_read_u32(child, "reg", &addr);
+ if (ret)
+ return ret;
- addr = of_mdio_parse_addr(dev, child);
+ if (!of_mdiobus_find_phy(dev, mdiodev, child, addr)) {
+ of_node_put(child);
+ return 0;
+ }
+
+ continue;
+ }
+
+ if (base_addr)
+ addr = of_mdio_parse_addr_offset(dev, child, base_addr);
+ else
+ addr = of_mdio_parse_addr(dev, child);
if (addr < 0)
continue;
@@ -481,9 +494,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, 0);
}
#else /* !IS_ENABLED(CONFIG_OF_MDIO) */
static inline void of_mdiobus_link_mdiodev(struct mii_bus *mdio,
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8a52ef2e6fa6..8566df2afbe6 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -72,6 +72,27 @@ static inline int of_mdio_parse_addr(struct device *dev,
return addr;
}
+static inline int of_mdio_parse_addr_offset(struct device *dev,
+ const struct device_node *np,
+ u16 offset)
+{
+ int addr;
+
+ addr = of_mdio_parse_addr(dev, np);
+ if (addr < 0)
+ return addr;
+
+ /* Validate final address with offset */
+ addr += offset;
+ if (addr >= PHY_MAX_ADDR) {
+ dev_err(dev, "%s PHY address offset %i is too large\n",
+ np->full_name, addr);
+ return -EINVAL;
+ }
+
+ return addr;
+}
+
#else /* CONFIG_OF_MDIO */
static inline bool of_mdiobus_child_is_phy(struct device_node *child)
{
@@ -130,6 +151,11 @@ static inline int of_mdio_parse_addr(struct device *dev,
{
return -ENOSYS;
}
+static inline int of_mdio_parse_addr_offset(struct device *dev,
+ const struct device_node *np)
+{
+ return -ENOSYS;
+}
static inline int of_phy_register_fixed_link(struct device_node *np)
{
return -ENOSYS;
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-01 16:40 ` Antoine Tenart
2024-02-01 15:17 ` [net-next PATCH v5 4/9] net: phy: qcom: move more function to shared library Christian Marangi
` (5 subsequent siblings)
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Add devm/of_phy_package_join helper to join PHYs in a PHY package. These
are variant of the manual phy_package_join with the difference that
these will use DT nodes to derive the base_addr instead of manually
passing an hardcoded value.
An additional value is added in phy_package_shared, "np" to reference
the PHY package node pointer in specific PHY driver probe_once and
config_init_once functions to make use of additional specific properties
defined in the PHY package node in DT.
The np value is filled only with of_phy_package_join if a valid PHY
package node is found. A valid PHY package node must have the node name
set to "ethernet-phy-package".
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/phy/phy_device.c | 84 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 6 +++
2 files changed, 90 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 52828d1c64f7..50874192e807 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1730,6 +1730,56 @@ int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size)
}
EXPORT_SYMBOL_GPL(phy_package_join);
+/**
+ * of_phy_package_join - join a common PHY group in PHY package
+ * @phydev: target phy_device struct
+ * @priv_size: if non-zero allocate this amount of bytes for private data
+ *
+ * This is a variant of phy_package_join for PHY package defined in DT.
+ *
+ * The parent node of the @phydev is checked as a valid PHY package node
+ * structure (by matching the node name "ethernet-phy-package") and the
+ * base_addr for the PHY package is passed to phy_package_join.
+ *
+ * With this configuration the shared struct will also have the np value
+ * filled to use additional DT defined properties in PHY specific
+ * probe_once and config_init_once PHY package OPs.
+ *
+ * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()
+ * with the same cookie but a different priv_size is an error. Or a parent
+ * node is not detected or is not valid or doesn't match the expected node
+ * name for PHY package.
+ */
+int of_phy_package_join(struct phy_device *phydev, size_t priv_size)
+{
+ struct device_node *node = phydev->mdio.dev.of_node;
+ struct device_node *package_node;
+ u32 base_addr;
+ int ret;
+
+ if (!node)
+ return -EINVAL;
+
+ package_node = of_get_parent(node);
+ if (!package_node)
+ return -EINVAL;
+
+ if (!of_node_name_eq(package_node, "ethernet-phy-package"))
+ return -EINVAL;
+
+ if (of_property_read_u32(package_node, "reg", &base_addr))
+ return -EINVAL;
+
+ ret = phy_package_join(phydev, base_addr, priv_size);
+ if (ret)
+ return ret;
+
+ phydev->shared->np = package_node;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_phy_package_join);
+
/**
* phy_package_leave - leave a common PHY group
* @phydev: target phy_device struct
@@ -1798,6 +1848,40 @@ int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
}
EXPORT_SYMBOL_GPL(devm_phy_package_join);
+/**
+ * devm_of_phy_package_join - resource managed of_phy_package_join()
+ * @dev: device that is registering this PHY package
+ * @phydev: target phy_device struct
+ * @priv_size: if non-zero allocate this amount of bytes for private data
+ *
+ * Managed of_phy_package_join(). Shared storage fetched by this function,
+ * phy_package_leave() is automatically called on driver detach. See
+ * of_phy_package_join() for more information.
+ */
+int devm_of_phy_package_join(struct device *dev, struct phy_device *phydev,
+ size_t priv_size)
+{
+ struct phy_device **ptr;
+ int ret;
+
+ ptr = devres_alloc(devm_phy_package_leave, sizeof(*ptr),
+ GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ ret = of_phy_package_join(phydev, priv_size);
+
+ if (!ret) {
+ *ptr = phydev;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_package_join);
+
/**
* phy_detach - detach a PHY device from its network device
* @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a66f07d3f5f4..2aed925e6c23 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 {
u8 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;
@@ -1999,9 +2002,12 @@ int phy_ethtool_set_link_ksettings(struct net_device *ndev,
const struct ethtool_link_ksettings *cmd);
int phy_ethtool_nway_reset(struct net_device *ndev);
int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size);
+int of_phy_package_join(struct phy_device *phydev, size_t priv_size);
void phy_package_leave(struct phy_device *phydev);
int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
int base_addr, size_t priv_size);
+int devm_of_phy_package_join(struct device *dev, struct phy_device *phydev,
+ size_t priv_size);
int __init mdio_bus_init(void);
void mdio_bus_exit(void);
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 4/9] net: phy: qcom: move more function to shared library
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
` (2 preceding siblings ...)
2024-02-01 15:17 ` [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines Christian Marangi
` (4 subsequent siblings)
8 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Move more function to shared library in preparation for introduction of
new PHY Family qca807x that will make use of both functions from at803x
and qca808x as it's a transition PHY with some implementation of at803x
and some from the new qca808x.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/phy/qcom/at803x.c | 35 -----
drivers/net/phy/qcom/qca808x.c | 205 ----------------------------
drivers/net/phy/qcom/qcom-phy-lib.c | 193 ++++++++++++++++++++++++++
drivers/net/phy/qcom/qcom.h | 51 +++++++
4 files changed, 244 insertions(+), 240 deletions(-)
diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index 36b70e88394e..3e3ee4c1d4bc 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -504,41 +504,6 @@ static void at803x_link_change_notify(struct phy_device *phydev)
}
}
-static int at803x_read_status(struct phy_device *phydev)
-{
- struct at803x_ss_mask ss_mask = { 0 };
- int 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;
-
- ss_mask.speed_mask = AT803X_SS_SPEED_MASK;
- ss_mask.speed_shift = __bf_shf(AT803X_SS_SPEED_MASK);
- err = at803x_read_specific_status(phydev, ss_mask);
- if (err < 0)
- return err;
-
- if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
- phy_resolve_aneg_pause(phydev);
-
- return 0;
-}
-
static int at803x_config_aneg(struct phy_device *phydev)
{
struct at803x_priv *priv = phydev->priv;
diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
index 0f549027d899..8124bc2d5e5f 100644
--- a/drivers/net/phy/qcom/qca808x.c
+++ b/drivers/net/phy/qcom/qca808x.c
@@ -2,7 +2,6 @@
#include <linux/phy.h>
#include <linux/module.h>
-#include <linux/ethtool_netlink.h>
#include "qcom.h"
@@ -63,55 +62,6 @@
#define QCA808X_DBG_AN_TEST 0xb
#define QCA808X_HIBERNATION_EN BIT(15)
-#define QCA808X_CDT_ENABLE_TEST BIT(15)
-#define QCA808X_CDT_INTER_CHECK_DIS BIT(13)
-#define QCA808X_CDT_STATUS BIT(11)
-#define QCA808X_CDT_LENGTH_UNIT BIT(10)
-
-#define QCA808X_MMD3_CDT_STATUS 0x8064
-#define QCA808X_MMD3_CDT_DIAG_PAIR_A 0x8065
-#define QCA808X_MMD3_CDT_DIAG_PAIR_B 0x8066
-#define QCA808X_MMD3_CDT_DIAG_PAIR_C 0x8067
-#define QCA808X_MMD3_CDT_DIAG_PAIR_D 0x8068
-#define QCA808X_CDT_DIAG_LENGTH_SAME_SHORT GENMASK(15, 8)
-#define QCA808X_CDT_DIAG_LENGTH_CROSS_SHORT GENMASK(7, 0)
-
-#define QCA808X_CDT_CODE_PAIR_A GENMASK(15, 12)
-#define QCA808X_CDT_CODE_PAIR_B GENMASK(11, 8)
-#define QCA808X_CDT_CODE_PAIR_C GENMASK(7, 4)
-#define QCA808X_CDT_CODE_PAIR_D GENMASK(3, 0)
-
-#define QCA808X_CDT_STATUS_STAT_TYPE GENMASK(1, 0)
-#define QCA808X_CDT_STATUS_STAT_FAIL FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 0)
-#define QCA808X_CDT_STATUS_STAT_NORMAL FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 1)
-#define QCA808X_CDT_STATUS_STAT_SAME_OPEN FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 2)
-#define QCA808X_CDT_STATUS_STAT_SAME_SHORT FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 3)
-
-#define QCA808X_CDT_STATUS_STAT_MDI GENMASK(3, 2)
-#define QCA808X_CDT_STATUS_STAT_MDI1 FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_MDI, 1)
-#define QCA808X_CDT_STATUS_STAT_MDI2 FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_MDI, 2)
-#define QCA808X_CDT_STATUS_STAT_MDI3 FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_MDI, 3)
-
-/* NORMAL are MDI with type set to 0 */
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_NORMAL QCA808X_CDT_STATUS_STAT_MDI1
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_OPEN (QCA808X_CDT_STATUS_STAT_SAME_OPEN |\
- QCA808X_CDT_STATUS_STAT_MDI1)
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_SHORT (QCA808X_CDT_STATUS_STAT_SAME_SHORT |\
- QCA808X_CDT_STATUS_STAT_MDI1)
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_NORMAL QCA808X_CDT_STATUS_STAT_MDI2
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_OPEN (QCA808X_CDT_STATUS_STAT_SAME_OPEN |\
- QCA808X_CDT_STATUS_STAT_MDI2)
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_SHORT (QCA808X_CDT_STATUS_STAT_SAME_SHORT |\
- QCA808X_CDT_STATUS_STAT_MDI2)
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_NORMAL QCA808X_CDT_STATUS_STAT_MDI3
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_OPEN (QCA808X_CDT_STATUS_STAT_SAME_OPEN |\
- QCA808X_CDT_STATUS_STAT_MDI3)
-#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_SHORT (QCA808X_CDT_STATUS_STAT_SAME_SHORT |\
- QCA808X_CDT_STATUS_STAT_MDI3)
-
-/* Added for reference of existence but should be handled by wait_for_completion already */
-#define QCA808X_CDT_STATUS_STAT_BUSY (BIT(1) | BIT(3))
-
#define QCA808X_MMD7_LED_GLOBAL 0x8073
#define QCA808X_LED_BLINK_1 GENMASK(11, 6)
#define QCA808X_LED_BLINK_2 GENMASK(5, 0)
@@ -396,86 +346,6 @@ static int qca808x_soft_reset(struct phy_device *phydev)
return ret;
}
-static bool qca808x_cdt_fault_length_valid(int cdt_code)
-{
- switch (cdt_code) {
- case QCA808X_CDT_STATUS_STAT_SAME_SHORT:
- case QCA808X_CDT_STATUS_STAT_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_NORMAL:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_NORMAL:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_NORMAL:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
- return true;
- default:
- return false;
- }
-}
-
-static int qca808x_cable_test_result_trans(int cdt_code)
-{
- switch (cdt_code) {
- case QCA808X_CDT_STATUS_STAT_NORMAL:
- return ETHTOOL_A_CABLE_RESULT_CODE_OK;
- case QCA808X_CDT_STATUS_STAT_SAME_SHORT:
- return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
- case QCA808X_CDT_STATUS_STAT_SAME_OPEN:
- return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_NORMAL:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_NORMAL:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_NORMAL:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
- case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
- return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
- case QCA808X_CDT_STATUS_STAT_FAIL:
- default:
- return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
- }
-}
-
-static int qca808x_cdt_fault_length(struct phy_device *phydev, int pair,
- int result)
-{
- int val;
- u32 cdt_length_reg = 0;
-
- switch (pair) {
- case ETHTOOL_A_CABLE_PAIR_A:
- cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_A;
- break;
- case ETHTOOL_A_CABLE_PAIR_B:
- cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_B;
- break;
- case ETHTOOL_A_CABLE_PAIR_C:
- cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_C;
- break;
- case ETHTOOL_A_CABLE_PAIR_D:
- cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_D;
- break;
- default:
- return -EINVAL;
- }
-
- val = phy_read_mmd(phydev, MDIO_MMD_PCS, cdt_length_reg);
- if (val < 0)
- return val;
-
- if (result == ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT)
- val = FIELD_GET(QCA808X_CDT_DIAG_LENGTH_SAME_SHORT, val);
- else
- val = FIELD_GET(QCA808X_CDT_DIAG_LENGTH_CROSS_SHORT, val);
-
- return at803x_cdt_fault_length(val);
-}
-
static int qca808x_cable_test_start(struct phy_device *phydev)
{
int ret;
@@ -517,81 +387,6 @@ static int qca808x_cable_test_start(struct phy_device *phydev)
return 0;
}
-static int qca808x_cable_test_get_pair_status(struct phy_device *phydev, u8 pair,
- u16 status)
-{
- int length, result;
- u16 pair_code;
-
- switch (pair) {
- case ETHTOOL_A_CABLE_PAIR_A:
- pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, status);
- break;
- case ETHTOOL_A_CABLE_PAIR_B:
- pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, status);
- break;
- case ETHTOOL_A_CABLE_PAIR_C:
- pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, status);
- break;
- case ETHTOOL_A_CABLE_PAIR_D:
- pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, status);
- break;
- default:
- return -EINVAL;
- }
-
- result = qca808x_cable_test_result_trans(pair_code);
- ethnl_cable_test_result(phydev, pair, result);
-
- if (qca808x_cdt_fault_length_valid(pair_code)) {
- length = qca808x_cdt_fault_length(phydev, pair, result);
- ethnl_cable_test_fault_length(phydev, pair, length);
- }
-
- return 0;
-}
-
-static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
-{
- int ret, val;
-
- *finished = false;
-
- val = QCA808X_CDT_ENABLE_TEST |
- QCA808X_CDT_LENGTH_UNIT;
- ret = at803x_cdt_start(phydev, val);
- if (ret)
- return ret;
-
- ret = at803x_cdt_wait_for_completion(phydev, QCA808X_CDT_ENABLE_TEST);
- if (ret)
- return ret;
-
- val = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA808X_MMD3_CDT_STATUS);
- if (val < 0)
- return val;
-
- ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
- if (ret)
- return ret;
-
- ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
- if (ret)
- return ret;
-
- ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
- if (ret)
- return ret;
-
- ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
- if (ret)
- return ret;
-
- *finished = true;
-
- return 0;
-}
-
static int qca808x_get_features(struct phy_device *phydev)
{
int ret;
diff --git a/drivers/net/phy/qcom/qcom-phy-lib.c b/drivers/net/phy/qcom/qcom-phy-lib.c
index e0295d4b4a51..786bfc39912c 100644
--- a/drivers/net/phy/qcom/qcom-phy-lib.c
+++ b/drivers/net/phy/qcom/qcom-phy-lib.c
@@ -5,6 +5,7 @@
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
+#include <linux/ethtool_netlink.h>
#include "qcom.h"
@@ -311,6 +312,42 @@ int at803x_prepare_config_aneg(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(at803x_prepare_config_aneg);
+int at803x_read_status(struct phy_device *phydev)
+{
+ struct at803x_ss_mask ss_mask = { 0 };
+ int 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;
+
+ ss_mask.speed_mask = AT803X_SS_SPEED_MASK;
+ ss_mask.speed_shift = __bf_shf(AT803X_SS_SPEED_MASK);
+ err = at803x_read_specific_status(phydev, ss_mask);
+ if (err < 0)
+ return err;
+
+ if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
+ phy_resolve_aneg_pause(phydev);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(at803x_read_status);
+
static int at803x_get_downshift(struct phy_device *phydev, u8 *d)
{
int val;
@@ -427,3 +464,159 @@ int at803x_cdt_wait_for_completion(struct phy_device *phydev,
return ret < 0 ? ret : 0;
}
EXPORT_SYMBOL_GPL(at803x_cdt_wait_for_completion);
+
+static bool qca808x_cdt_fault_length_valid(int cdt_code)
+{
+ switch (cdt_code) {
+ case QCA808X_CDT_STATUS_STAT_SAME_SHORT:
+ case QCA808X_CDT_STATUS_STAT_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_NORMAL:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_NORMAL:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_NORMAL:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static int qca808x_cable_test_result_trans(int cdt_code)
+{
+ switch (cdt_code) {
+ case QCA808X_CDT_STATUS_STAT_NORMAL:
+ return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+ case QCA808X_CDT_STATUS_STAT_SAME_SHORT:
+ return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+ case QCA808X_CDT_STATUS_STAT_SAME_OPEN:
+ return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_NORMAL:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_SHORT:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_NORMAL:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_SHORT:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_NORMAL:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_OPEN:
+ case QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_SHORT:
+ return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
+ case QCA808X_CDT_STATUS_STAT_FAIL:
+ default:
+ return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+ }
+}
+
+static int qca808x_cdt_fault_length(struct phy_device *phydev, int pair,
+ int result)
+{
+ int val;
+ u32 cdt_length_reg = 0;
+
+ switch (pair) {
+ case ETHTOOL_A_CABLE_PAIR_A:
+ cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_A;
+ break;
+ case ETHTOOL_A_CABLE_PAIR_B:
+ cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_B;
+ break;
+ case ETHTOOL_A_CABLE_PAIR_C:
+ cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_C;
+ break;
+ case ETHTOOL_A_CABLE_PAIR_D:
+ cdt_length_reg = QCA808X_MMD3_CDT_DIAG_PAIR_D;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, cdt_length_reg);
+ if (val < 0)
+ return val;
+
+ if (result == ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT)
+ val = FIELD_GET(QCA808X_CDT_DIAG_LENGTH_SAME_SHORT, val);
+ else
+ val = FIELD_GET(QCA808X_CDT_DIAG_LENGTH_CROSS_SHORT, val);
+
+ return at803x_cdt_fault_length(val);
+}
+
+static int qca808x_cable_test_get_pair_status(struct phy_device *phydev, u8 pair,
+ u16 status)
+{
+ int length, result;
+ u16 pair_code;
+
+ switch (pair) {
+ case ETHTOOL_A_CABLE_PAIR_A:
+ pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_A, status);
+ break;
+ case ETHTOOL_A_CABLE_PAIR_B:
+ pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_B, status);
+ break;
+ case ETHTOOL_A_CABLE_PAIR_C:
+ pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_C, status);
+ break;
+ case ETHTOOL_A_CABLE_PAIR_D:
+ pair_code = FIELD_GET(QCA808X_CDT_CODE_PAIR_D, status);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ result = qca808x_cable_test_result_trans(pair_code);
+ ethnl_cable_test_result(phydev, pair, result);
+
+ if (qca808x_cdt_fault_length_valid(pair_code)) {
+ length = qca808x_cdt_fault_length(phydev, pair, result);
+ ethnl_cable_test_fault_length(phydev, pair, length);
+ }
+
+ return 0;
+}
+
+int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
+{
+ int ret, val;
+
+ *finished = false;
+
+ val = QCA808X_CDT_ENABLE_TEST |
+ QCA808X_CDT_LENGTH_UNIT;
+ ret = at803x_cdt_start(phydev, val);
+ if (ret)
+ return ret;
+
+ ret = at803x_cdt_wait_for_completion(phydev, QCA808X_CDT_ENABLE_TEST);
+ if (ret)
+ return ret;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, QCA808X_MMD3_CDT_STATUS);
+ if (val < 0)
+ return val;
+
+ ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
+ if (ret)
+ return ret;
+
+ ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
+ if (ret)
+ return ret;
+
+ ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
+ if (ret)
+ return ret;
+
+ ret = qca808x_cable_test_get_pair_status(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
+ if (ret)
+ return ret;
+
+ *finished = true;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qca808x_cable_test_get_status);
diff --git a/drivers/net/phy/qcom/qcom.h b/drivers/net/phy/qcom/qcom.h
index c127d8f50f0f..dc259bbf0678 100644
--- a/drivers/net/phy/qcom/qcom.h
+++ b/drivers/net/phy/qcom/qcom.h
@@ -54,6 +54,55 @@
#define AT803X_CDT_STATUS_STAT_MASK GENMASK(9, 8)
#define AT803X_CDT_STATUS_DELTA_TIME_MASK GENMASK(7, 0)
+#define QCA808X_CDT_ENABLE_TEST BIT(15)
+#define QCA808X_CDT_INTER_CHECK_DIS BIT(13)
+#define QCA808X_CDT_STATUS BIT(11)
+#define QCA808X_CDT_LENGTH_UNIT BIT(10)
+
+#define QCA808X_MMD3_CDT_STATUS 0x8064
+#define QCA808X_MMD3_CDT_DIAG_PAIR_A 0x8065
+#define QCA808X_MMD3_CDT_DIAG_PAIR_B 0x8066
+#define QCA808X_MMD3_CDT_DIAG_PAIR_C 0x8067
+#define QCA808X_MMD3_CDT_DIAG_PAIR_D 0x8068
+#define QCA808X_CDT_DIAG_LENGTH_SAME_SHORT GENMASK(15, 8)
+#define QCA808X_CDT_DIAG_LENGTH_CROSS_SHORT GENMASK(7, 0)
+
+#define QCA808X_CDT_CODE_PAIR_A GENMASK(15, 12)
+#define QCA808X_CDT_CODE_PAIR_B GENMASK(11, 8)
+#define QCA808X_CDT_CODE_PAIR_C GENMASK(7, 4)
+#define QCA808X_CDT_CODE_PAIR_D GENMASK(3, 0)
+
+#define QCA808X_CDT_STATUS_STAT_TYPE GENMASK(1, 0)
+#define QCA808X_CDT_STATUS_STAT_FAIL FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 0)
+#define QCA808X_CDT_STATUS_STAT_NORMAL FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 1)
+#define QCA808X_CDT_STATUS_STAT_SAME_OPEN FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 2)
+#define QCA808X_CDT_STATUS_STAT_SAME_SHORT FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_TYPE, 3)
+
+#define QCA808X_CDT_STATUS_STAT_MDI GENMASK(3, 2)
+#define QCA808X_CDT_STATUS_STAT_MDI1 FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_MDI, 1)
+#define QCA808X_CDT_STATUS_STAT_MDI2 FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_MDI, 2)
+#define QCA808X_CDT_STATUS_STAT_MDI3 FIELD_PREP_CONST(QCA808X_CDT_STATUS_STAT_MDI, 3)
+
+/* NORMAL are MDI with type set to 0 */
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_NORMAL QCA808X_CDT_STATUS_STAT_MDI1
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_OPEN (QCA808X_CDT_STATUS_STAT_SAME_OPEN |\
+ QCA808X_CDT_STATUS_STAT_MDI1)
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI1_SAME_SHORT (QCA808X_CDT_STATUS_STAT_SAME_SHORT |\
+ QCA808X_CDT_STATUS_STAT_MDI1)
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_NORMAL QCA808X_CDT_STATUS_STAT_MDI2
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_OPEN (QCA808X_CDT_STATUS_STAT_SAME_OPEN |\
+ QCA808X_CDT_STATUS_STAT_MDI2)
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI2_SAME_SHORT (QCA808X_CDT_STATUS_STAT_SAME_SHORT |\
+ QCA808X_CDT_STATUS_STAT_MDI2)
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_NORMAL QCA808X_CDT_STATUS_STAT_MDI3
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_OPEN (QCA808X_CDT_STATUS_STAT_SAME_OPEN |\
+ QCA808X_CDT_STATUS_STAT_MDI3)
+#define QCA808X_CDT_STATUS_STAT_CROSS_SHORT_WITH_MDI3_SAME_SHORT (QCA808X_CDT_STATUS_STAT_SAME_SHORT |\
+ QCA808X_CDT_STATUS_STAT_MDI3)
+
+/* Added for reference of existence but should be handled by wait_for_completion already */
+#define QCA808X_CDT_STATUS_STAT_BUSY (BIT(1) | BIT(3))
+
#define AT803X_LOC_MAC_ADDR_0_15_OFFSET 0x804C
#define AT803X_LOC_MAC_ADDR_16_31_OFFSET 0x804B
#define AT803X_LOC_MAC_ADDR_32_47_OFFSET 0x804A
@@ -110,6 +159,7 @@ int at803x_read_specific_status(struct phy_device *phydev,
struct at803x_ss_mask ss_mask);
int at803x_config_mdix(struct phy_device *phydev, u8 ctrl);
int at803x_prepare_config_aneg(struct phy_device *phydev);
+int at803x_read_status(struct phy_device *phydev);
int at803x_get_tunable(struct phy_device *phydev,
struct ethtool_tunable *tuna, void *data);
int at803x_set_tunable(struct phy_device *phydev,
@@ -118,3 +168,4 @@ int at803x_cdt_fault_length(int dt);
int at803x_cdt_start(struct phy_device *phydev, u32 cdt_start);
int at803x_cdt_wait_for_completion(struct phy_device *phydev,
u32 cdt_en);
+int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished);
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
` (3 preceding siblings ...)
2024-02-01 15:17 ` [net-next PATCH v5 4/9] net: phy: qcom: move more function to shared library Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-02 7:41 ` Krzysztof Kozlowski
2024-02-01 15:17 ` [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
` (3 subsequent siblings)
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
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.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
` (4 preceding siblings ...)
2024-02-01 15:17 ` [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-02 1:16 ` Andrew Lunn
` (2 more replies)
2024-02-01 15:17 ` [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family Christian Marangi
` (2 subsequent siblings)
8 siblings, 3 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Document Qcom QCA807x PHY package.
Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
1000BASE-T PHY-s.
Document the required property to make the PHY package correctly
configure and work.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
.../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
new file mode 100644
index 000000000000..1c3692897b02
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm QCA807X Ethernet PHY
+
+maintainers:
+ - Christian Marangi <ansuelsmth@gmail.com>
+ - Robert Marko <robert.marko@sartura.hr>
+
+description: |
+ Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
+ IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
+ 1000BASE-T PHY-s.
+
+ They feature 2 SerDes, one for PSGMII or QSGMII connection with
+ MAC, while second one is SGMII for connection to MAC or fiber.
+
+ Both models have a combo port that supports 1000BASE-X and
+ 100BASE-FX fiber.
+
+ Each PHY inside of QCA807x series has 4 digitally controlled
+ output only pins that natively drive LED-s for up to 2 attached
+ LEDs. Some vendor also use these 4 output for GPIO usage without
+ attaching LEDs.
+
+ Note that output pins can be set to drive LEDs OR GPIO, mixed
+ definition are not accepted.
+
+ PHY package can be configured in 3 mode following this table:
+
+ First Serdes mode Second Serdes mode
+ Option 1 PSGMII for copper Disabled
+ ports 0-4
+ Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
+ ports 0-4
+ Option 3 QSGMII for copper SGMII for
+ ports 0-3 copper port 4
+
+$ref: ethernet-phy-package.yaml#
+
+properties:
+ compatible:
+ const: qcom,qca807x-package
+
+ qcom,package-mode:
+ enum:
+ - qsgmii
+ - psgmii
+
+ qcom,tx-driver-strength:
+ description: set the TX Amplifier value in mv.
+ If not defined, 600mw is set by default.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [140, 160, 180, 200, 220,
+ 240, 260, 280, 300, 320,
+ 400, 500, 600]
+
+patternProperties:
+ ^ethernet-phy(@[a-f0-9]+)?$:
+ $ref: ethernet-phy.yaml#
+
+ properties:
+ gpio-controller:
+ description: set the output lines as GPIO instead of LEDs
+ type: boolean
+
+ '#gpio-cells':
+ description: number of GPIO cells for the PHY
+ const: 2
+
+ dependencies:
+ gpio-controller: ['#gpio-cells']
+
+ if:
+ required:
+ - gpio-controller
+ then:
+ properties:
+ leds: false
+
+ unevaluatedProperties: false
+
+required:
+ - compatible
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethernet-phy-package@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "qcom,qca807x-package";
+ reg = <0>;
+
+ qcom,package-mode = "qsgmii";
+
+ ethernet-phy@0 {
+ reg = <0>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+ };
+ };
+
+ ethernet-phy@1 {
+ reg = <1>;
+ };
+
+ ethernet-phy@2 {
+ reg = <2>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
+ ethernet-phy@3 {
+ reg = <3>;
+ };
+
+ ethernet-phy@4 {
+ reg = <4>;
+ };
+ };
+ };
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
` (5 preceding siblings ...)
2024-02-01 15:17 ` [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-02 1:35 ` Andrew Lunn
2024-02-01 15:17 ` [net-next PATCH v5 8/9] net: phy: qcom: generalize some qca808x LED functions Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED Christian Marangi
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
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/qcom/Kconfig | 8 +
drivers/net/phy/qcom/Makefile | 1 +
drivers/net/phy/qcom/qca807x.c | 578 +++++++++++++++++++++++++++++++++
3 files changed, 587 insertions(+)
create mode 100644 drivers/net/phy/qcom/qca807x.c
diff --git a/drivers/net/phy/qcom/Kconfig b/drivers/net/phy/qcom/Kconfig
index 80db24deb689..570626cc8e14 100644
--- a/drivers/net/phy/qcom/Kconfig
+++ b/drivers/net/phy/qcom/Kconfig
@@ -20,3 +20,11 @@ config QCA808X_PHY
select QCOM_NET_PHYLIB
help
Currently supports the QCA8081 model
+
+config QCA807X_PHY
+ tristate "Qualcomm QCA807x PHYs"
+ select QCOM_NET_PHYLIB
+ depends on OF_MDIO
+ help
+ Currently supports the Qualcomm QCA8072, QCA8075 and the PSGMII
+ control PHY.
diff --git a/drivers/net/phy/qcom/Makefile b/drivers/net/phy/qcom/Makefile
index 0362d7ed47be..f24fb550babd 100644
--- a/drivers/net/phy/qcom/Makefile
+++ b/drivers/net/phy/qcom/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_QCOM_NET_PHYLIB) += qcom-phy-lib.o
obj-$(CONFIG_AT803X_PHY) += at803x.o
obj-$(CONFIG_QCA83XX_PHY) += qca83xx.o
obj-$(CONFIG_QCA808X_PHY) += qca808x.o
+obj-$(CONFIG_QCA807X_PHY) += qca807x.o
diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c
new file mode 100644
index 000000000000..5ff6ba9fa1a0
--- /dev/null
+++ b/drivers/net/phy/qcom/qca807x.c
@@ -0,0 +1,578 @@
+// 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/gpio/driver.h>
+#include <linux/sfp.h>
+
+#include "qcom.h"
+
+#include <dt-bindings/net/qcom-qca807x.h>
+
+#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_MMD7_LED_CTRL(x) (0x8074 + ((x) * 2))
+#define QCA807X_MMD7_LED_FORCE_CTRL(x) (0x8075 + ((x) * 2))
+
+#define QCA807X_GPIO_FORCE_EN BIT(15)
+#define QCA807X_GPIO_FORCE_MODE_MASK GENMASK(14, 13)
+
+#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
+
+/* 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 PHY_ID_QCA8072 0x004dd0b2
+#define PHY_ID_QCA8075 0x004dd0b1
+
+#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_cable_test_start(struct phy_device *phydev)
+{
+ /* we do all the (time consuming) work later */
+ return 0;
+}
+
+#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(struct gpio_chip *gc, unsigned int offset)
+{
+ struct qca807x_gpio_priv *priv = gpiochip_get_data(gc);
+ u16 reg;
+ int val;
+
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(offset);
+ val = phy_read_mmd(priv->phy, MDIO_MMD_AN, reg);
+
+ 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);
+ u16 reg;
+ int val;
+
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(offset);
+
+ val = phy_read_mmd(priv->phy, MDIO_MMD_AN, reg);
+ 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, reg, 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_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, AT803X_SPECIFIC_STATUS);
+ if (ss < 0)
+ return ss;
+
+ if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
+ switch (FIELD_GET(AT803X_SS_SPEED_MASK, ss)) {
+ case AT803X_SS_SPEED_100:
+ phydev->speed = SPEED_100;
+ break;
+ case AT803X_SS_SPEED_1000:
+ phydev->speed = SPEED_1000;
+ break;
+ }
+
+ if (ss & AT803X_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 at803x_read_status(phydev);
+ default:
+ return -EINVAL;
+ }
+ }
+
+ return at803x_read_status(phydev);
+}
+
+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 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_probe(struct phy_device *phydev)
+{
+ struct device_node *node = phydev->mdio.dev.of_node;
+ struct qca807x_shared_priv *shared_priv;
+ struct phy_package_shared *shared;
+ int ret;
+
+ ret = devm_of_phy_package_join(&phydev->mdio.dev, phydev,
+ sizeof(*shared_priv));
+ if (ret)
+ return ret;
+
+ if (phy_package_probe_once(phydev)) {
+ ret = qca807x_phy_package_probe_once(phydev);
+ if (ret)
+ return ret;
+ }
+
+ shared = phydev->shared;
+ shared_priv = shared->priv;
+
+ /* Make sure PHY follow PHY package mode if enforced */
+ if (shared_priv->package_mode != PHY_INTERFACE_MODE_NA &&
+ phydev->interface != shared_priv->package_mode)
+ return -EINVAL;
+
+ if (IS_ENABLED(CONFIG_GPIOLIB)) {
+ /* 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_config_init(struct phy_device *phydev)
+{
+ struct device_node *node = phydev->mdio.dev.of_node;
+ int control_dac, ret;
+ u32 of_control_dac;
+
+ if (phy_package_init_once(phydev)) {
+ ret = qca807x_phy_package_config_init_once(phydev);
+ if (ret)
+ return ret;
+ }
+
+ 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 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_init,
+ .read_status = qca807x_read_status,
+ .config_intr = at803x_config_intr,
+ .handle_interrupt = at803x_handle_interrupt,
+ .soft_reset = genphy_soft_reset,
+ .get_tunable = at803x_get_tunable,
+ .set_tunable = at803x_set_tunable,
+ .resume = genphy_resume,
+ .suspend = genphy_suspend,
+ .cable_test_start = qca807x_cable_test_start,
+ .cable_test_get_status = qca808x_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_init,
+ .read_status = qca807x_read_status,
+ .config_intr = at803x_config_intr,
+ .handle_interrupt = at803x_handle_interrupt,
+ .soft_reset = genphy_soft_reset,
+ .get_tunable = at803x_get_tunable,
+ .set_tunable = at803x_set_tunable,
+ .resume = genphy_resume,
+ .suspend = genphy_suspend,
+ .cable_test_start = qca807x_cable_test_start,
+ .cable_test_get_status = qca808x_cable_test_get_status,
+ },
+};
+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.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 8/9] net: phy: qcom: generalize some qca808x LED functions
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
` (6 preceding siblings ...)
2024-02-01 15:17 ` [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-02 1:38 ` Andrew Lunn
2024-02-01 15:17 ` [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED Christian Marangi
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
Generalize some qca808x LED functions in preparation for qca807x LED
support.
The LED implementation of qca808x and qca807x is the same but qca807x
supports also Fiber port and have different hw control bits for Fiber
port. To limit code duplication introduce micro functions that takes reg
instead of LED index to tweak all the supported LED modes.
Also move to shared header all the common LED bits.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/phy/qcom/qca808x.c | 106 ++--------------------------
drivers/net/phy/qcom/qcom-phy-lib.c | 54 ++++++++++++++
drivers/net/phy/qcom/qcom.h | 72 +++++++++++++++++++
3 files changed, 131 insertions(+), 101 deletions(-)
diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
index 8124bc2d5e5f..d25ab19b18bf 100644
--- a/drivers/net/phy/qcom/qca808x.c
+++ b/drivers/net/phy/qcom/qca808x.c
@@ -62,29 +62,6 @@
#define QCA808X_DBG_AN_TEST 0xb
#define QCA808X_HIBERNATION_EN BIT(15)
-#define QCA808X_MMD7_LED_GLOBAL 0x8073
-#define QCA808X_LED_BLINK_1 GENMASK(11, 6)
-#define QCA808X_LED_BLINK_2 GENMASK(5, 0)
-/* Values are the same for both BLINK_1 and BLINK_2 */
-#define QCA808X_LED_BLINK_FREQ_MASK GENMASK(5, 3)
-#define QCA808X_LED_BLINK_FREQ_2HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x0)
-#define QCA808X_LED_BLINK_FREQ_4HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x1)
-#define QCA808X_LED_BLINK_FREQ_8HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x2)
-#define QCA808X_LED_BLINK_FREQ_16HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x3)
-#define QCA808X_LED_BLINK_FREQ_32HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x4)
-#define QCA808X_LED_BLINK_FREQ_64HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x5)
-#define QCA808X_LED_BLINK_FREQ_128HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x6)
-#define QCA808X_LED_BLINK_FREQ_256HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x7)
-#define QCA808X_LED_BLINK_DUTY_MASK GENMASK(2, 0)
-#define QCA808X_LED_BLINK_DUTY_50_50 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x0)
-#define QCA808X_LED_BLINK_DUTY_75_25 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x1)
-#define QCA808X_LED_BLINK_DUTY_25_75 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x2)
-#define QCA808X_LED_BLINK_DUTY_33_67 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x3)
-#define QCA808X_LED_BLINK_DUTY_67_33 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x4)
-#define QCA808X_LED_BLINK_DUTY_17_83 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x5)
-#define QCA808X_LED_BLINK_DUTY_83_17 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x6)
-#define QCA808X_LED_BLINK_DUTY_8_92 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x7)
-
#define QCA808X_MMD7_LED2_CTRL 0x8074
#define QCA808X_MMD7_LED2_FORCE_CTRL 0x8075
#define QCA808X_MMD7_LED1_CTRL 0x8076
@@ -92,51 +69,9 @@
#define QCA808X_MMD7_LED0_CTRL 0x8078
#define QCA808X_MMD7_LED_CTRL(x) (0x8078 - ((x) * 2))
-/* LED hw control pattern is the same for every LED */
-#define QCA808X_LED_PATTERN_MASK GENMASK(15, 0)
-#define QCA808X_LED_SPEED2500_ON BIT(15)
-#define QCA808X_LED_SPEED2500_BLINK BIT(14)
-/* Follow blink trigger even if duplex or speed condition doesn't match */
-#define QCA808X_LED_BLINK_CHECK_BYPASS BIT(13)
-#define QCA808X_LED_FULL_DUPLEX_ON BIT(12)
-#define QCA808X_LED_HALF_DUPLEX_ON BIT(11)
-#define QCA808X_LED_TX_BLINK BIT(10)
-#define QCA808X_LED_RX_BLINK BIT(9)
-#define QCA808X_LED_TX_ON_10MS BIT(8)
-#define QCA808X_LED_RX_ON_10MS BIT(7)
-#define QCA808X_LED_SPEED1000_ON BIT(6)
-#define QCA808X_LED_SPEED100_ON BIT(5)
-#define QCA808X_LED_SPEED10_ON BIT(4)
-#define QCA808X_LED_COLLISION_BLINK BIT(3)
-#define QCA808X_LED_SPEED1000_BLINK BIT(2)
-#define QCA808X_LED_SPEED100_BLINK BIT(1)
-#define QCA808X_LED_SPEED10_BLINK BIT(0)
-
#define QCA808X_MMD7_LED0_FORCE_CTRL 0x8079
#define QCA808X_MMD7_LED_FORCE_CTRL(x) (0x8079 - ((x) * 2))
-/* LED force ctrl is the same for every LED
- * No documentation exist for this, not even internal one
- * with NDA as QCOM gives only info about configuring
- * hw control pattern rules and doesn't indicate any way
- * to force the LED to specific mode.
- * These define comes from reverse and testing and maybe
- * lack of some info or some info are not entirely correct.
- * For the basic LED control and hw control these finding
- * are enough to support LED control in all the required APIs.
- *
- * On doing some comparison with implementation with qca807x,
- * it was found that it's 1:1 equal to it and confirms all the
- * reverse done. It was also found further specification with the
- * force mode and the blink modes.
- */
-#define QCA808X_LED_FORCE_EN BIT(15)
-#define QCA808X_LED_FORCE_MODE_MASK GENMASK(14, 13)
-#define QCA808X_LED_FORCE_BLINK_1 FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x3)
-#define QCA808X_LED_FORCE_BLINK_2 FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x2)
-#define QCA808X_LED_FORCE_ON FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x1)
-#define QCA808X_LED_FORCE_OFF FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x0)
-
#define QCA808X_MMD7_LED_POLARITY_CTRL 0x901a
/* QSDK sets by default 0x46 to this reg that sets BIT 6 for
* LED to active high. It's not clear what BIT 3 and BIT 4 does.
@@ -492,9 +427,7 @@ static int qca808x_led_hw_control_enable(struct phy_device *phydev, u8 index)
return -EINVAL;
reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
-
- return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg,
- QCA808X_LED_FORCE_EN);
+ return qca808x_led_reg_hw_control_enable(phydev, reg);
}
static int qca808x_led_hw_is_supported(struct phy_device *phydev, u8 index,
@@ -535,16 +468,12 @@ static int qca808x_led_hw_control_set(struct phy_device *phydev, u8 index,
static bool qca808x_led_hw_control_status(struct phy_device *phydev, u8 index)
{
u16 reg;
- int val;
if (index > 2)
return false;
reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
-
- val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
-
- return !(val & QCA808X_LED_FORCE_EN);
+ return qca808x_led_reg_hw_control_status(phydev, reg);
}
static int qca808x_led_hw_control_get(struct phy_device *phydev, u8 index,
@@ -590,8 +519,7 @@ static int qca808x_led_hw_control_reset(struct phy_device *phydev, u8 index)
if (index > 2)
return -EINVAL;
- reg = QCA808X_MMD7_LED_CTRL(index);
-
+ reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg,
QCA808X_LED_PATTERN_MASK);
}
@@ -612,44 +540,20 @@ static int qca808x_led_brightness_set(struct phy_device *phydev,
}
reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
-
- return phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
- QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_MODE_MASK,
- QCA808X_LED_FORCE_EN | value ? QCA808X_LED_FORCE_ON :
- QCA808X_LED_FORCE_OFF);
+ return qca808x_led_reg_brightness_set(phydev, reg, value);
}
static int qca808x_led_blink_set(struct phy_device *phydev, u8 index,
unsigned long *delay_on,
unsigned long *delay_off)
{
- int ret;
u16 reg;
if (index > 2)
return -EINVAL;
reg = QCA808X_MMD7_LED_FORCE_CTRL(index);
-
- /* Set blink to 50% off, 50% on at 4Hz by default */
- ret = phy_modify_mmd(phydev, MDIO_MMD_AN, QCA808X_MMD7_LED_GLOBAL,
- QCA808X_LED_BLINK_FREQ_MASK | QCA808X_LED_BLINK_DUTY_MASK,
- QCA808X_LED_BLINK_FREQ_4HZ | QCA808X_LED_BLINK_DUTY_50_50);
- if (ret)
- return ret;
-
- /* We use BLINK_1 for normal blinking */
- ret = phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
- QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_MODE_MASK,
- QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_BLINK_1);
- if (ret)
- return ret;
-
- /* We set blink to 4Hz, aka 250ms */
- *delay_on = 250 / 2;
- *delay_off = 250 / 2;
-
- return 0;
+ return qca808x_led_reg_blink_set(phydev, reg, delay_on, delay_off);
}
static int qca808x_led_polarity_set(struct phy_device *phydev, int index,
diff --git a/drivers/net/phy/qcom/qcom-phy-lib.c b/drivers/net/phy/qcom/qcom-phy-lib.c
index 786bfc39912c..d28815ef56bb 100644
--- a/drivers/net/phy/qcom/qcom-phy-lib.c
+++ b/drivers/net/phy/qcom/qcom-phy-lib.c
@@ -620,3 +620,57 @@ int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished)
return 0;
}
EXPORT_SYMBOL_GPL(qca808x_cable_test_get_status);
+
+int qca808x_led_reg_hw_control_enable(struct phy_device *phydev, u16 reg)
+{
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg,
+ QCA808X_LED_FORCE_EN);
+}
+EXPORT_SYMBOL_GPL(qca808x_led_reg_hw_control_enable);
+
+bool qca808x_led_reg_hw_control_status(struct phy_device *phydev, u16 reg)
+{
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+ return !(val & QCA808X_LED_FORCE_EN);
+}
+EXPORT_SYMBOL_GPL(qca808x_led_reg_hw_control_status);
+
+int qca808x_led_reg_brightness_set(struct phy_device *phydev,
+ u16 reg, enum led_brightness value)
+{
+ return phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
+ QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_MODE_MASK,
+ QCA808X_LED_FORCE_EN | (value ? QCA808X_LED_FORCE_ON :
+ QCA808X_LED_FORCE_OFF));
+}
+EXPORT_SYMBOL_GPL(qca808x_led_reg_brightness_set);
+
+int qca808x_led_reg_blink_set(struct phy_device *phydev, u16 reg,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ int ret;
+
+ /* Set blink to 50% off, 50% on at 4Hz by default */
+ ret = phy_modify_mmd(phydev, MDIO_MMD_AN, QCA808X_MMD7_LED_GLOBAL,
+ QCA808X_LED_BLINK_FREQ_MASK | QCA808X_LED_BLINK_DUTY_MASK,
+ QCA808X_LED_BLINK_FREQ_4HZ | QCA808X_LED_BLINK_DUTY_50_50);
+ if (ret)
+ return ret;
+
+ /* We use BLINK_1 for normal blinking */
+ ret = phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
+ QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_MODE_MASK,
+ QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_BLINK_1);
+ if (ret)
+ return ret;
+
+ /* We set blink to 4Hz, aka 250ms */
+ *delay_on = 250 / 2;
+ *delay_off = 250 / 2;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qca808x_led_reg_blink_set);
diff --git a/drivers/net/phy/qcom/qcom.h b/drivers/net/phy/qcom/qcom.h
index dc259bbf0678..4bb541728846 100644
--- a/drivers/net/phy/qcom/qcom.h
+++ b/drivers/net/phy/qcom/qcom.h
@@ -103,6 +103,71 @@
/* Added for reference of existence but should be handled by wait_for_completion already */
#define QCA808X_CDT_STATUS_STAT_BUSY (BIT(1) | BIT(3))
+#define QCA808X_MMD7_LED_GLOBAL 0x8073
+#define QCA808X_LED_BLINK_1 GENMASK(11, 6)
+#define QCA808X_LED_BLINK_2 GENMASK(5, 0)
+/* Values are the same for both BLINK_1 and BLINK_2 */
+#define QCA808X_LED_BLINK_FREQ_MASK GENMASK(5, 3)
+#define QCA808X_LED_BLINK_FREQ_2HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x0)
+#define QCA808X_LED_BLINK_FREQ_4HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x1)
+#define QCA808X_LED_BLINK_FREQ_8HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x2)
+#define QCA808X_LED_BLINK_FREQ_16HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x3)
+#define QCA808X_LED_BLINK_FREQ_32HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x4)
+#define QCA808X_LED_BLINK_FREQ_64HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x5)
+#define QCA808X_LED_BLINK_FREQ_128HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x6)
+#define QCA808X_LED_BLINK_FREQ_256HZ FIELD_PREP(QCA808X_LED_BLINK_FREQ_MASK, 0x7)
+#define QCA808X_LED_BLINK_DUTY_MASK GENMASK(2, 0)
+#define QCA808X_LED_BLINK_DUTY_50_50 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x0)
+#define QCA808X_LED_BLINK_DUTY_75_25 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x1)
+#define QCA808X_LED_BLINK_DUTY_25_75 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x2)
+#define QCA808X_LED_BLINK_DUTY_33_67 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x3)
+#define QCA808X_LED_BLINK_DUTY_67_33 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x4)
+#define QCA808X_LED_BLINK_DUTY_17_83 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x5)
+#define QCA808X_LED_BLINK_DUTY_83_17 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x6)
+#define QCA808X_LED_BLINK_DUTY_8_92 FIELD_PREP(QCA808X_LED_BLINK_DUTY_MASK, 0x7)
+
+/* LED hw control pattern is the same for every LED */
+#define QCA808X_LED_PATTERN_MASK GENMASK(15, 0)
+#define QCA808X_LED_SPEED2500_ON BIT(15)
+#define QCA808X_LED_SPEED2500_BLINK BIT(14)
+/* Follow blink trigger even if duplex or speed condition doesn't match */
+#define QCA808X_LED_BLINK_CHECK_BYPASS BIT(13)
+#define QCA808X_LED_FULL_DUPLEX_ON BIT(12)
+#define QCA808X_LED_HALF_DUPLEX_ON BIT(11)
+#define QCA808X_LED_TX_BLINK BIT(10)
+#define QCA808X_LED_RX_BLINK BIT(9)
+#define QCA808X_LED_TX_ON_10MS BIT(8)
+#define QCA808X_LED_RX_ON_10MS BIT(7)
+#define QCA808X_LED_SPEED1000_ON BIT(6)
+#define QCA808X_LED_SPEED100_ON BIT(5)
+#define QCA808X_LED_SPEED10_ON BIT(4)
+#define QCA808X_LED_COLLISION_BLINK BIT(3)
+#define QCA808X_LED_SPEED1000_BLINK BIT(2)
+#define QCA808X_LED_SPEED100_BLINK BIT(1)
+#define QCA808X_LED_SPEED10_BLINK BIT(0)
+
+/* LED force ctrl is the same for every LED
+ * No documentation exist for this, not even internal one
+ * with NDA as QCOM gives only info about configuring
+ * hw control pattern rules and doesn't indicate any way
+ * to force the LED to specific mode.
+ * These define comes from reverse and testing and maybe
+ * lack of some info or some info are not entirely correct.
+ * For the basic LED control and hw control these finding
+ * are enough to support LED control in all the required APIs.
+ *
+ * On doing some comparison with implementation with qca807x,
+ * it was found that it's 1:1 equal to it and confirms all the
+ * reverse done. It was also found further specification with the
+ * force mode and the blink modes.
+ */
+#define QCA808X_LED_FORCE_EN BIT(15)
+#define QCA808X_LED_FORCE_MODE_MASK GENMASK(14, 13)
+#define QCA808X_LED_FORCE_BLINK_1 FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x3)
+#define QCA808X_LED_FORCE_BLINK_2 FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x2)
+#define QCA808X_LED_FORCE_ON FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x1)
+#define QCA808X_LED_FORCE_OFF FIELD_PREP(QCA808X_LED_FORCE_MODE_MASK, 0x0)
+
#define AT803X_LOC_MAC_ADDR_0_15_OFFSET 0x804C
#define AT803X_LOC_MAC_ADDR_16_31_OFFSET 0x804B
#define AT803X_LOC_MAC_ADDR_32_47_OFFSET 0x804A
@@ -169,3 +234,10 @@ int at803x_cdt_start(struct phy_device *phydev, u32 cdt_start);
int at803x_cdt_wait_for_completion(struct phy_device *phydev,
u32 cdt_en);
int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finished);
+int qca808x_led_reg_hw_control_enable(struct phy_device *phydev, u16 reg);
+bool qca808x_led_reg_hw_control_status(struct phy_device *phydev, u16 reg);
+int qca808x_led_reg_brightness_set(struct phy_device *phydev,
+ u16 reg, enum led_brightness value);
+int qca808x_led_reg_blink_set(struct phy_device *phydev, u16 reg,
+ unsigned long *delay_on,
+ unsigned long *delay_off);
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
` (7 preceding siblings ...)
2024-02-01 15:17 ` [net-next PATCH v5 8/9] net: phy: qcom: generalize some qca808x LED functions Christian Marangi
@ 2024-02-01 15:17 ` Christian Marangi
2024-02-02 1:43 ` Andrew Lunn
8 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 15:17 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Christian Marangi, Robert Marko, netdev,
devicetree, linux-kernel, linux-arm-msm
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/qcom/qca807x.c | 258 ++++++++++++++++++++++++++++++++-
1 file changed, 256 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c
index 5ff6ba9fa1a0..0d7fd1a9404e 100644
--- a/drivers/net/phy/qcom/qca807x.c
+++ b/drivers/net/phy/qcom/qca807x.c
@@ -47,8 +47,18 @@
#define QCA807X_MMD7_LED_CTRL(x) (0x8074 + ((x) * 2))
#define QCA807X_MMD7_LED_FORCE_CTRL(x) (0x8075 + ((x) * 2))
-#define QCA807X_GPIO_FORCE_EN BIT(15)
-#define QCA807X_GPIO_FORCE_MODE_MASK GENMASK(14, 13)
+/* LED hw control pattern for fiber port */
+#define QCA807X_LED_FIBER_PATTERN_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 QCA808X_LED_FORCE_EN
+#define QCA807X_GPIO_FORCE_MODE_MASK QCA808X_LED_FORCE_MODE_MASK
#define QCA807X_FUNCTION_CONTROL 0x10
#define QCA807X_FC_MDI_CROSSOVER_MODE_MASK GENMASK(6, 5)
@@ -105,6 +115,233 @@ static int qca807x_cable_test_start(struct phy_device *phydev)
return 0;
}
+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 |= QCA808X_LED_TX_BLINK;
+ if (test_bit(TRIGGER_NETDEV_RX, &rules))
+ *offload_trigger |= QCA808X_LED_RX_BLINK;
+ if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+ *offload_trigger |= QCA808X_LED_SPEED10_ON;
+ if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+ *offload_trigger |= QCA808X_LED_SPEED100_ON;
+ if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+ *offload_trigger |= QCA808X_LED_SPEED1000_ON;
+ if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+ *offload_trigger |= QCA808X_LED_HALF_DUPLEX_ON;
+ if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+ *offload_trigger |= QCA808X_LED_FULL_DUPLEX_ON;
+ 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)
+{
+ u16 reg;
+
+ if (index > 1)
+ return -EINVAL;
+
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ return qca808x_led_reg_hw_control_enable(phydev, reg);
+}
+
+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)
+{
+ u16 reg, mask, offload_trigger = 0;
+ int ret;
+
+ if (index > 1)
+ 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:
+ reg = QCA807X_MMD7_LED_CTRL(index);
+ mask = QCA808X_LED_PATTERN_MASK;
+ break;
+ case PORT_FIBRE:
+ /* HW control pattern bits are in LED FORCE reg */
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ mask = QCA807X_LED_FIBER_PATTERN_MASK;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return phy_modify_mmd(phydev, MDIO_MMD_AN, reg, mask,
+ offload_trigger);
+}
+
+static bool qca807x_led_hw_control_status(struct phy_device *phydev, u8 index)
+{
+ u16 reg;
+
+ if (index > 1)
+ return false;
+
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ return qca808x_led_reg_hw_control_status(phydev, reg);
+}
+
+static int qca807x_led_hw_control_get(struct phy_device *phydev, u8 index,
+ unsigned long *rules)
+{
+ u16 reg;
+ int val;
+
+ if (index > 1)
+ 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:
+ reg = QCA807X_MMD7_LED_CTRL(index);
+ val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
+ if (val & QCA808X_LED_TX_BLINK)
+ set_bit(TRIGGER_NETDEV_TX, rules);
+ if (val & QCA808X_LED_RX_BLINK)
+ set_bit(TRIGGER_NETDEV_RX, rules);
+ if (val & QCA808X_LED_SPEED10_ON)
+ set_bit(TRIGGER_NETDEV_LINK_10, rules);
+ if (val & QCA808X_LED_SPEED100_ON)
+ set_bit(TRIGGER_NETDEV_LINK_100, rules);
+ if (val & QCA808X_LED_SPEED1000_ON)
+ set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+ if (val & QCA808X_LED_HALF_DUPLEX_ON)
+ set_bit(TRIGGER_NETDEV_HALF_DUPLEX, rules);
+ if (val & QCA808X_LED_FULL_DUPLEX_ON)
+ set_bit(TRIGGER_NETDEV_FULL_DUPLEX, rules);
+ break;
+ case PORT_FIBRE:
+ /* HW control pattern bits are in LED FORCE reg */
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ val = phy_read_mmd(phydev, MDIO_MMD_AN, 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)
+{
+ u16 reg, mask;
+
+ if (index > 1)
+ return -EINVAL;
+
+ switch (phydev->port) {
+ case PORT_TP:
+ reg = QCA807X_MMD7_LED_CTRL(index);
+ mask = QCA808X_LED_PATTERN_MASK;
+ break;
+ case PORT_FIBRE:
+ /* HW control pattern bits are in LED FORCE reg */
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ mask = QCA807X_LED_FIBER_PATTERN_MASK;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, reg, mask);
+}
+
+static int qca807x_led_brightness_set(struct phy_device *phydev,
+ u8 index, enum led_brightness value)
+{
+ u16 reg;
+ int ret;
+
+ if (index > 1)
+ 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;
+ }
+
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ return qca808x_led_reg_brightness_set(phydev, reg, value);
+}
+
+static int qca807x_led_blink_set(struct phy_device *phydev, u8 index,
+ unsigned long *delay_on,
+ unsigned long *delay_off)
+{
+ u16 reg;
+
+ if (index > 1)
+ return -EINVAL;
+
+ reg = QCA807X_MMD7_LED_FORCE_CTRL(index);
+ return qca808x_led_reg_blink_set(phydev, reg, delay_on, delay_off);
+}
+
#ifdef CONFIG_GPIOLIB
static int qca807x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
{
@@ -481,11 +718,23 @@ static int qca807x_probe(struct phy_device *phydev)
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;
+
+ 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;
}
}
@@ -561,6 +810,11 @@ static struct phy_driver qca807x_drivers[] = {
.suspend = genphy_suspend,
.cable_test_start = qca807x_cable_test_start,
.cable_test_get_status = qca808x_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,
},
};
module_phy_driver(qca807x_drivers);
--
2.43.0
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes
2024-02-01 15:17 ` [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes Christian Marangi
@ 2024-02-01 16:25 ` Antoine Tenart
2024-02-01 17:20 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Antoine Tenart @ 2024-02-01 16:25 UTC (permalink / raw)
To: Andrew Lunn, Bjorn Andersson, Christian Marangi, Conor Dooley,
David S. Miller, Eric Dumazet, Frank Rowand, Heiner Kallweit,
Jakub Kicinski, Konrad Dybcio, Krzysztof Kozlowski, Paolo Abeni,
Rob Herring, Robert Marko, Russell King, devicetree,
linux-arm-msm, linux-kernel, netdev
Quoting Christian Marangi (2024-02-01 16:17:28)
>
> +static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
> + int base_addr, bool *scanphys)
> +{
> + struct device_node *child;
> + int addr, rc = 0;
> +
> + /* Loop over the child nodes and register a phy_device for each phy */
> + for_each_available_child_of_node(np, child) {
> + if (of_node_name_eq(child, "ethernet-phy-package")) {
> + rc = of_property_read_u32(child, "reg", &addr);
> + if (rc)
> + goto exit;
This means a PHY package node w/o a reg property will prevent all other
PHYs in the same parent node to be found?
> +
> + rc = __of_mdiobus_parse_phys(mdio, child, addr, scanphys);
You might want to save passing scanphys down, PHYs w/o a reg property in
a PHY package won't be "auto scanned" later.
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index afbad1ad8683..7737d0101d7b 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -459,20 +459,33 @@ EXPORT_SYMBOL(of_mdio_find_bus);
> * found, set the of_node pointer for the mdio device. This allows
> * auto-probed phy devices to be supplied with information passed in
> * via DT.
> + * If a PHY package is found, PHY is searched also there.
> */
> -static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
> - struct mdio_device *mdiodev)
> +static int of_mdiobus_find_phy(struct device *dev, struct mdio_device *mdiodev,
> + struct device_node *np, int base_addr)
> {
> - struct device *dev = &mdiodev->dev;
> struct device_node *child;
>
> - if (dev->of_node || !bus->dev.of_node)
> - return;
> + for_each_available_child_of_node(np, child) {
> + int addr, ret;
>
> - for_each_available_child_of_node(bus->dev.of_node, child) {
> - int addr;
> + if (of_node_name_eq(child, "ethernet-phy-package")) {
> + ret = of_property_read_u32(child, "reg", &addr);
> + if (ret)
> + return ret;
of_node_put
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper
2024-02-01 15:17 ` [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper Christian Marangi
@ 2024-02-01 16:40 ` Antoine Tenart
2024-02-01 16:48 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Antoine Tenart @ 2024-02-01 16:40 UTC (permalink / raw)
To: Andrew Lunn, Bjorn Andersson, Christian Marangi, Conor Dooley,
David S. Miller, Eric Dumazet, Frank Rowand, Heiner Kallweit,
Jakub Kicinski, Konrad Dybcio, Krzysztof Kozlowski, Paolo Abeni,
Rob Herring, Robert Marko, Russell King, devicetree,
linux-arm-msm, linux-kernel, netdev
Quoting Christian Marangi (2024-02-01 16:17:29)
> +/**
> + * of_phy_package_join - join a common PHY group in PHY package
> + * @phydev: target phy_device struct
> + * @priv_size: if non-zero allocate this amount of bytes for private data
> + *
> + * This is a variant of phy_package_join for PHY package defined in DT.
> + *
> + * The parent node of the @phydev is checked as a valid PHY package node
> + * structure (by matching the node name "ethernet-phy-package") and the
> + * base_addr for the PHY package is passed to phy_package_join.
> + *
> + * With this configuration the shared struct will also have the np value
> + * filled to use additional DT defined properties in PHY specific
> + * probe_once and config_init_once PHY package OPs.
> + *
> + * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()
So, < 0 on error ?
> +int of_phy_package_join(struct phy_device *phydev, size_t priv_size)
> +{
> + struct device_node *node = phydev->mdio.dev.of_node;
> + struct device_node *package_node;
> + u32 base_addr;
> + int ret;
> +
> + if (!node)
> + return -EINVAL;
> +
> + package_node = of_get_parent(node);
Is the node put on package leave?
> + if (!package_node)
> + return -EINVAL;
> +
> + if (!of_node_name_eq(package_node, "ethernet-phy-package")
of_put_node? + below.
> + return -EINVAL;
> +
> + if (of_property_read_u32(package_node, "reg", &base_addr))
> + return -EINVAL;
> +
> + ret = phy_package_join(phydev, base_addr, priv_size);
> + if (ret)
> + return ret;
> +
> + phydev->shared->np = package_node;
Just looked quickly, looks like ->np is uninitialized in the !of join
case.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_phy_package_join);
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper
2024-02-01 16:40 ` Antoine Tenart
@ 2024-02-01 16:48 ` Christian Marangi
0 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 16:48 UTC (permalink / raw)
To: Antoine Tenart
Cc: Andrew Lunn, Bjorn Andersson, Conor Dooley, David S. Miller,
Eric Dumazet, Frank Rowand, Heiner Kallweit, Jakub Kicinski,
Konrad Dybcio, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
Robert Marko, Russell King, devicetree, linux-arm-msm,
linux-kernel, netdev
On Thu, Feb 01, 2024 at 05:40:28PM +0100, Antoine Tenart wrote:
> Quoting Christian Marangi (2024-02-01 16:17:29)
> > +/**
> > + * of_phy_package_join - join a common PHY group in PHY package
> > + * @phydev: target phy_device struct
> > + * @priv_size: if non-zero allocate this amount of bytes for private data
> > + *
> > + * This is a variant of phy_package_join for PHY package defined in DT.
> > + *
> > + * The parent node of the @phydev is checked as a valid PHY package node
> > + * structure (by matching the node name "ethernet-phy-package") and the
> > + * base_addr for the PHY package is passed to phy_package_join.
> > + *
> > + * With this configuration the shared struct will also have the np value
> > + * filled to use additional DT defined properties in PHY specific
> > + * probe_once and config_init_once PHY package OPs.
> > + *
> > + * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()
>
> So, < 0 on error ?
>
> > +int of_phy_package_join(struct phy_device *phydev, size_t priv_size)
> > +{
> > + struct device_node *node = phydev->mdio.dev.of_node;
> > + struct device_node *package_node;
> > + u32 base_addr;
> > + int ret;
> > +
> > + if (!node)
> > + return -EINVAL;
> > +
> > + package_node = of_get_parent(node);
>
> Is the node put on package leave?
>
Didn't read the get_parent was incrementing the refcount... Will update
the leave accordingly!
> > + if (!package_node)
> > + return -EINVAL;
> > +
> > + if (!of_node_name_eq(package_node, "ethernet-phy-package")
>
> of_put_node? + below.
>
> > + return -EINVAL;
> > +
> > + if (of_property_read_u32(package_node, "reg", &base_addr))
> > + return -EINVAL;
> > +
> > + ret = phy_package_join(phydev, base_addr, priv_size);
> > + if (ret)
> > + return ret;
> > +
> > + phydev->shared->np = package_node;
>
> Just looked quickly, looks like ->np is uninitialized in the !of join
> case.
>
Correct, I will update the non of variant to inizialize ->np to NULL.
(in theory we alloc every struct as zero so it should not be a problem
but you are right with being safe on this.)
Thanks!
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_phy_package_join);
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes
2024-02-01 16:25 ` Antoine Tenart
@ 2024-02-01 17:20 ` Christian Marangi
2024-02-02 1:02 ` Andrew Lunn
2024-02-02 10:05 ` Antoine Tenart
0 siblings, 2 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-01 17:20 UTC (permalink / raw)
To: Antoine Tenart
Cc: Andrew Lunn, Bjorn Andersson, Conor Dooley, David S. Miller,
Eric Dumazet, Frank Rowand, Heiner Kallweit, Jakub Kicinski,
Konrad Dybcio, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
Robert Marko, Russell King, devicetree, linux-arm-msm,
linux-kernel, netdev
On Thu, Feb 01, 2024 at 05:25:36PM +0100, Antoine Tenart wrote:
> Quoting Christian Marangi (2024-02-01 16:17:28)
> >
> > +static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
> > + int base_addr, bool *scanphys)
> > +{
> > + struct device_node *child;
> > + int addr, rc = 0;
> > +
> > + /* Loop over the child nodes and register a phy_device for each phy */
> > + for_each_available_child_of_node(np, child) {
> > + if (of_node_name_eq(child, "ethernet-phy-package")) {
> > + rc = of_property_read_u32(child, "reg", &addr);
> > + if (rc)
> > + goto exit;
>
> This means a PHY package node w/o a reg property will prevent all other
> PHYs in the same parent node to be found?
>
Since this is something new, would it be a problem to make it mandatory
to define a reg? (And return error if we find something? Or print a
warn?)
> > +
> > + rc = __of_mdiobus_parse_phys(mdio, child, addr, scanphys);
>
> You might want to save passing scanphys down, PHYs w/o a reg property in
> a PHY package won't be "auto scanned" later.
>
I might be confused by this, but isn't this already done? (passing
scanphys in each recursive call so we can set it to true if needed?)
Also I think the scanphys should be skipped for the PHY package
(assuming we make reg mandatory, it would be an error condition and
should not be handled?)
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index afbad1ad8683..7737d0101d7b 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -459,20 +459,33 @@ EXPORT_SYMBOL(of_mdio_find_bus);
> > * found, set the of_node pointer for the mdio device. This allows
> > * auto-probed phy devices to be supplied with information passed in
> > * via DT.
> > + * If a PHY package is found, PHY is searched also there.
> > */
> > -static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
> > - struct mdio_device *mdiodev)
> > +static int of_mdiobus_find_phy(struct device *dev, struct mdio_device *mdiodev,
> > + struct device_node *np, int base_addr)
> > {
> > - struct device *dev = &mdiodev->dev;
> > struct device_node *child;
> >
> > - if (dev->of_node || !bus->dev.of_node)
> > - return;
> > + for_each_available_child_of_node(np, child) {
> > + int addr, ret;
> >
> > - for_each_available_child_of_node(bus->dev.of_node, child) {
> > - int addr;
> > + if (of_node_name_eq(child, "ethernet-phy-package")) {
> > + ret = of_property_read_u32(child, "reg", &addr);
> > + if (ret)
> > + return ret;
>
> of_node_put
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes
2024-02-01 17:20 ` Christian Marangi
@ 2024-02-02 1:02 ` Andrew Lunn
2024-02-02 10:05 ` Antoine Tenart
1 sibling, 0 replies; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 1:02 UTC (permalink / raw)
To: Christian Marangi
Cc: Antoine Tenart, Bjorn Andersson, Conor Dooley, David S. Miller,
Eric Dumazet, Frank Rowand, Heiner Kallweit, Jakub Kicinski,
Konrad Dybcio, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
Robert Marko, Russell King, devicetree, linux-arm-msm,
linux-kernel, netdev
On Thu, Feb 01, 2024 at 06:20:10PM +0100, Christian Marangi wrote:
> On Thu, Feb 01, 2024 at 05:25:36PM +0100, Antoine Tenart wrote:
> > Quoting Christian Marangi (2024-02-01 16:17:28)
> > >
> > > +static int __of_mdiobus_parse_phys(struct mii_bus *mdio, struct device_node *np,
> > > + int base_addr, bool *scanphys)
> > > +{
> > > + struct device_node *child;
> > > + int addr, rc = 0;
> > > +
> > > + /* Loop over the child nodes and register a phy_device for each phy */
> > > + for_each_available_child_of_node(np, child) {
> > > + if (of_node_name_eq(child, "ethernet-phy-package")) {
> > > + rc = of_property_read_u32(child, "reg", &addr);
> > > + if (rc)
> > > + goto exit;
> >
> > This means a PHY package node w/o a reg property will prevent all other
> > PHYs in the same parent node to be found?
> >
>
> Since this is something new, would it be a problem to make it mandatory
> to define a reg? (And return error if we find something? Or print a
> warn?)
Making reg mandatory within a package is reasonable. Please indicate
this in the DT schema.
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-01 15:17 ` [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
@ 2024-02-02 1:16 ` Andrew Lunn
2024-02-02 7:45 ` Krzysztof Kozlowski
2024-02-02 20:45 ` Rob Herring
2 siblings, 0 replies; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 1:16 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
> + PHY package can be configured in 3 mode following this table:
> +
> + First Serdes mode Second Serdes mode
> + Option 1 PSGMII for copper Disabled
> + ports 0-4
> + Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
> + ports 0-4
> + Option 3 QSGMII for copper SGMII for
> + ports 0-3 copper port 4
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> + compatible:
> + const: qcom,qca807x-package
> +
> + qcom,package-mode:
> + enum:
> + - qsgmii
> + - psgmii
There are three modes listed above, yet only two values here? Please
describe how they related.
> +
> + qcom,tx-driver-strength:
> + description: set the TX Amplifier value in mv.
> + If not defined, 600mw is set by default.
Looking at other bindings, it seems pretty normal to include the units
in the property name: qcom,tx-driver-strength-milliwatts.
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family
2024-02-01 15:17 ` [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family Christian Marangi
@ 2024-02-02 1:35 ` Andrew Lunn
2024-02-02 17:44 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 1:35 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
> +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);
> + }
This looks a lot like genphy_c37_read_status(). Can it be used?
> +
> + /* 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, AT803X_SPECIFIC_STATUS);
> + if (ss < 0)
> + return ss;
> +
> + if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
> + switch (FIELD_GET(AT803X_SS_SPEED_MASK, ss)) {
> + case AT803X_SS_SPEED_100:
> + phydev->speed = SPEED_100;
> + break;
> + case AT803X_SS_SPEED_1000:
> + phydev->speed = SPEED_1000;
> + break;
> + }
> +
> + if (ss & AT803X_SS_DUPLEX)
> + phydev->duplex = DUPLEX_FULL;
> + else
> + phydev->duplex = DUPLEX_HALF;
> + }
> +
> + return 0;
> +}
> +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:
...
> + case 500:
> + priv->tx_driver_strength = PQSGMII_TX_DRIVER_500MV;
> + break;
> + case 600:
> + default:
If its missing default to 600. But if its an invalid value, return
-EINVAL.
> + 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;
Again, return -EINVAL if it is neither.
> +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;
What about priv->package_mode == PHY_INTERFACE_MODE_NA;
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 8/9] net: phy: qcom: generalize some qca808x LED functions
2024-02-01 15:17 ` [net-next PATCH v5 8/9] net: phy: qcom: generalize some qca808x LED functions Christian Marangi
@ 2024-02-02 1:38 ` Andrew Lunn
0 siblings, 0 replies; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 1:38 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Thu, Feb 01, 2024 at 04:17:34PM +0100, Christian Marangi wrote:
> Generalize some qca808x LED functions in preparation for qca807x LED
> support.
>
> The LED implementation of qca808x and qca807x is the same but qca807x
> supports also Fiber port and have different hw control bits for Fiber
> port. To limit code duplication introduce micro functions that takes reg
> instead of LED index to tweak all the supported LED modes.
Please could you split this up. Do the move first, no changes. Then
add the macros and other refactoring. That will make this easier to
review.
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-01 15:17 ` [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED Christian Marangi
@ 2024-02-02 1:43 ` Andrew Lunn
2024-02-02 16:40 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 1:43 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
> +
> + 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;
I don't see how that works. You have multiple PHYs using this
driver. Some might have LEDs, some might have GPOs. But if you modify
the driver structure like this, you prevent all PHYs from having LEDs,
and maybe cause a Opps if a PHY device has already registered its
LEDs?
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines
2024-02-01 15:17 ` [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines Christian Marangi
@ 2024-02-02 7:41 ` Krzysztof Kozlowski
2024-02-02 15:19 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Krzysztof Kozlowski @ 2024-02-02 7:41 UTC (permalink / raw)
To: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
Russell King, Frank Rowand, Robert Marko, netdev, devicetree,
linux-kernel, linux-arm-msm
On 01/02/2024 16:17, Christian Marangi wrote:
> From: Robert Marko <robert.marko@sartura.hr>
>
> Add DT bindings defined for Qualcomm QCA807x PHY series related to
> calibration and DAC settings.
Nothing from this file is used and your commit msg does not provide
rationale "why", thus it does not look like something for bindings.
Otherwise please point me which patch with *driver* uses these bindings.
>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++
Use filename matching compatible, so vendor,device. No wildcards, unless
your compatible also has them.
> 1 file changed, 30 insertions(+)
> create mode 100644 include/dt-bindings/net/qcom-qca807x.h
>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-01 15:17 ` [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
2024-02-02 1:16 ` Andrew Lunn
@ 2024-02-02 7:45 ` Krzysztof Kozlowski
2024-02-02 15:12 ` Christian Marangi
2024-02-02 20:45 ` Rob Herring
2 siblings, 1 reply; 41+ messages in thread
From: Krzysztof Kozlowski @ 2024-02-02 7:45 UTC (permalink / raw)
To: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
Russell King, Frank Rowand, Robert Marko, netdev, devicetree,
linux-kernel, linux-arm-msm
On 01/02/2024 16:17, Christian Marangi wrote:
> Document Qcom QCA807x PHY package.
>
> Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> 1000BASE-T PHY-s.
>
> Document the required property to make the PHY package correctly
> configure and work.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
Your bindings header must be squashed here. Headers are not separate
thing from the bindings.
> 1 file changed, 142 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> new file mode 100644
> index 000000000000..1c3692897b02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QCA807X Ethernet PHY
What is "X"? Wildcards are usually not expected.
> +
> +maintainers:
> + - Christian Marangi <ansuelsmth@gmail.com>
> + - Robert Marko <robert.marko@sartura.hr>
> +
> +description: |
> + Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> + IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> + 1000BASE-T PHY-s.
> +
> + They feature 2 SerDes, one for PSGMII or QSGMII connection with
> + MAC, while second one is SGMII for connection to MAC or fiber.
> +
> + Both models have a combo port that supports 1000BASE-X and
> + 100BASE-FX fiber.
> +
> + Each PHY inside of QCA807x series has 4 digitally controlled
> + output only pins that natively drive LED-s for up to 2 attached
> + LEDs. Some vendor also use these 4 output for GPIO usage without
> + attaching LEDs.
> +
> + Note that output pins can be set to drive LEDs OR GPIO, mixed
> + definition are not accepted.
> +
> + PHY package can be configured in 3 mode following this table:
> +
> + First Serdes mode Second Serdes mode
> + Option 1 PSGMII for copper Disabled
> + ports 0-4
> + Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
> + ports 0-4
> + Option 3 QSGMII for copper SGMII for
> + ports 0-3 copper port 4
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> + compatible:
> + const: qcom,qca807x-package
> +
> + qcom,package-mode:
Where is definition of this property with type and description?
> + enum:
> + - qsgmii
> + - psgmii
> +
> + qcom,tx-driver-strength:
Use proper unit suffix.
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> + description: set the TX Amplifier value in mv.
> + If not defined, 600mw is set by default.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [140, 160, 180, 200, 220,
> + 240, 260, 280, 300, 320,
> + 400, 500, 600]
> +
> +patternProperties:
> + ^ethernet-phy(@[a-f0-9]+)?$:
> + $ref: ethernet-phy.yaml#
> +
> + properties:
> + gpio-controller:
> + description: set the output lines as GPIO instead of LEDs
> + type: boolean
> +
> + '#gpio-cells':
> + description: number of GPIO cells for the PHY
> + const: 2
> +
> + dependencies:
> + gpio-controller: ['#gpio-cells']
Why do you need it? None of gpio-controllers do it, I think.
> +
> + if:
> + required:
> + - gpio-controller
> + then:
> + properties:
> + leds: false
> +
> + unevaluatedProperties: false
> +
> +required:
> + - compatible
> +
> +unevaluatedProperties: false
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes
2024-02-01 17:20 ` Christian Marangi
2024-02-02 1:02 ` Andrew Lunn
@ 2024-02-02 10:05 ` Antoine Tenart
1 sibling, 0 replies; 41+ messages in thread
From: Antoine Tenart @ 2024-02-02 10:05 UTC (permalink / raw)
To: Christian Marangi
Cc: Andrew Lunn, Bjorn Andersson, Conor Dooley, David S. Miller,
Eric Dumazet, Frank Rowand, Heiner Kallweit, Jakub Kicinski,
Konrad Dybcio, Krzysztof Kozlowski, Paolo Abeni, Rob Herring,
Robert Marko, Russell King, devicetree, linux-arm-msm,
linux-kernel, netdev
Quoting Christian Marangi (2024-02-01 18:20:10)
> On Thu, Feb 01, 2024 at 05:25:36PM +0100, Antoine Tenart wrote:
> > Quoting Christian Marangi (2024-02-01 16:17:28)
> > > +
> > > + rc = __of_mdiobus_parse_phys(mdio, child, addr, scanphys);
> >
> > You might want to save passing scanphys down, PHYs w/o a reg property in
> > a PHY package won't be "auto scanned" later.
>
> I might be confused by this, but isn't this already done? (passing
> scanphys in each recursive call so we can set it to true if needed?)
>
> Also I think the scanphys should be skipped for the PHY package
> (assuming we make reg mandatory, it would be an error condition and
> should not be handled?)
Sorry if that wasn't clear, this is what I meant. scanphys doesn't need
to be set to true in a PHY package (both if we want reg to be mandatory
there and because my understanding of the auto-scan code is PHYs in a
PHY package won't be auto scanned anyway).
Thanks,
Antoine
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-02 7:45 ` Krzysztof Kozlowski
@ 2024-02-02 15:12 ` Christian Marangi
2024-02-02 20:39 ` Rob Herring
0 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 15:12 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Robert Marko, netdev, devicetree, linux-kernel,
linux-arm-msm
On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2024 16:17, Christian Marangi wrote:
> > Document Qcom QCA807x PHY package.
> >
> > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > 1000BASE-T PHY-s.
> >
> > Document the required property to make the PHY package correctly
> > configure and work.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
>
> Your bindings header must be squashed here. Headers are not separate
> thing from the bindings.
>
> > 1 file changed, 142 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > new file mode 100644
> > index 000000000000..1c3692897b02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > @@ -0,0 +1,142 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm QCA807X Ethernet PHY
>
> What is "X"? Wildcards are usually not expected.
>
It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
or qca8074 since they would refer to a more generic Family of devices.
What would be the correct way? We have many other case on net with
schema called qca8k that refer to the family of Ethernet Switch but in
it refer to qca8327 qca8337 qca8334...
> > +
> > +maintainers:
> > + - Christian Marangi <ansuelsmth@gmail.com>
> > + - Robert Marko <robert.marko@sartura.hr>
> > +
> > +description: |
> > + Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > + IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > + 1000BASE-T PHY-s.
> > +
> > + They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > + MAC, while second one is SGMII for connection to MAC or fiber.
> > +
> > + Both models have a combo port that supports 1000BASE-X and
> > + 100BASE-FX fiber.
> > +
> > + Each PHY inside of QCA807x series has 4 digitally controlled
> > + output only pins that natively drive LED-s for up to 2 attached
> > + LEDs. Some vendor also use these 4 output for GPIO usage without
> > + attaching LEDs.
> > +
> > + Note that output pins can be set to drive LEDs OR GPIO, mixed
> > + definition are not accepted.
> > +
> > + PHY package can be configured in 3 mode following this table:
> > +
> > + First Serdes mode Second Serdes mode
> > + Option 1 PSGMII for copper Disabled
> > + ports 0-4
> > + Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
> > + ports 0-4
> > + Option 3 QSGMII for copper SGMII for
> > + ports 0-3 copper port 4
> > +
> > +$ref: ethernet-phy-package.yaml#
> > +
> > +properties:
> > + compatible:
> > + const: qcom,qca807x-package
> > +
> > + qcom,package-mode:
>
> Where is definition of this property with type and description?
>
> > + enum:
> > + - qsgmii
> > + - psgmii
> > +
> > + qcom,tx-driver-strength:
>
> Use proper unit suffix.
>
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>
> > + description: set the TX Amplifier value in mv.
> > + If not defined, 600mw is set by default.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [140, 160, 180, 200, 220,
> > + 240, 260, 280, 300, 320,
> > + 400, 500, 600]
> > +
> > +patternProperties:
> > + ^ethernet-phy(@[a-f0-9]+)?$:
> > + $ref: ethernet-phy.yaml#
> > +
> > + properties:
> > + gpio-controller:
> > + description: set the output lines as GPIO instead of LEDs
> > + type: boolean
> > +
> > + '#gpio-cells':
> > + description: number of GPIO cells for the PHY
> > + const: 2
> > +
> > + dependencies:
> > + gpio-controller: ['#gpio-cells']
>
> Why do you need it? None of gpio-controllers do it, I think.
>
Well gpio-controller property is optional and having that declared and
#gpio-cells skipped will result in an error on probe. I think it should
be the opposite with other schema having to declare this.
In usual way for gpio-controller both property are defined as required
but you can see some (to-be-converted) txt files whith comments where
they say:
./mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.
Should I instead add this condition in the if right below?
> > +
> > + if:
> > + required:
> > + - gpio-controller
> > + then:
> > + properties:
> > + leds: false
> > +
> > + unevaluatedProperties: false
> > +
> > +required:
> > + - compatible
> > +
> > +unevaluatedProperties: false
>
>
> Best regards,
> Krzysztof
>
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines
2024-02-02 7:41 ` Krzysztof Kozlowski
@ 2024-02-02 15:19 ` Christian Marangi
2024-02-02 16:58 ` Conor Dooley
0 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 15:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Robert Marko, netdev, devicetree, linux-kernel,
linux-arm-msm
On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2024 16:17, Christian Marangi wrote:
> > From: Robert Marko <robert.marko@sartura.hr>
> >
> > Add DT bindings defined for Qualcomm QCA807x PHY series related to
> > calibration and DAC settings.
>
> Nothing from this file is used and your commit msg does not provide
> rationale "why", thus it does not look like something for bindings.
> Otherwise please point me which patch with *driver* uses these bindings.
>
Hi, since I have to squash this, I will include the reason in the schema
patch.
Anyway these are raw values used to configure the qcom,control-dac
property.
In the driver it's used by qca807x_config_init. We read what is set in
DT and we configure the reg accordingly.
If this is wrong should we use a more schema friendly approach with
declaring an enum of string and document that there?
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++
>
> Use filename matching compatible, so vendor,device. No wildcards, unless
> your compatible also has them.
>
> > 1 file changed, 30 insertions(+)
> > create mode 100644 include/dt-bindings/net/qcom-qca807x.h
> >
>
>
>
> Best regards,
> Krzysztof
>
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-02 1:43 ` Andrew Lunn
@ 2024-02-02 16:40 ` Christian Marangi
2024-02-02 17:04 ` Russell King (Oracle)
2024-02-02 17:08 ` Andrew Lunn
0 siblings, 2 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 16:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote:
> > +
> > + 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;
>
> I don't see how that works. You have multiple PHYs using this
> driver. Some might have LEDs, some might have GPOs. But if you modify
> the driver structure like this, you prevent all PHYs from having LEDs,
> and maybe cause a Opps if a PHY device has already registered its
> LEDs?
>
God you are right! Off-topic but given the effects this may cause, why
the thing is not const? I assume it wouldn't make sense to add OPS based
on the detected feature since it would have side effect on other PHYs
that use the same driver.
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines
2024-02-02 15:19 ` Christian Marangi
@ 2024-02-02 16:58 ` Conor Dooley
2024-02-02 17:03 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Conor Dooley @ 2024-02-02 16:58 UTC (permalink / raw)
To: Christian Marangi
Cc: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Konrad Dybcio, Andrew Lunn,
Heiner Kallweit, Russell King, Frank Rowand, Robert Marko,
netdev, devicetree, linux-kernel, linux-arm-msm
[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]
On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote:
> > On 01/02/2024 16:17, Christian Marangi wrote:
> > > From: Robert Marko <robert.marko@sartura.hr>
> > >
> > > Add DT bindings defined for Qualcomm QCA807x PHY series related to
> > > calibration and DAC settings.
> >
> > Nothing from this file is used and your commit msg does not provide
> > rationale "why", thus it does not look like something for bindings.
> > Otherwise please point me which patch with *driver* uses these bindings.
> >
>
> Hi, since I have to squash this, I will include the reason in the schema
> patch.
>
> Anyway these are raw values used to configure the qcom,control-dac
> property.
Maybe I am missing something, but a quick scan of the patchset and a
grep of the tree doesn't show this property being documented anywhere.
> In the driver it's used by qca807x_config_init. We read what is set in
> DT and we configure the reg accordingly.
>
> If this is wrong should we use a more schema friendly approach with
> declaring an enum of string and document that there?
Without any idea of what that property is used for it is hard to say,
but personally I much prefer enums of strings for what looks like a
property that holds register values.
That you felt it necessary to add defines for the values makes it more
like that that is the case.
Cheers,
Conor.
>
> > >
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++
> >
> > Use filename matching compatible, so vendor,device. No wildcards, unless
> > your compatible also has them.
> >
> > > 1 file changed, 30 insertions(+)
> > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h
> > >
> >
> >
> >
> > Best regards,
> > Krzysztof
> >
>
> --
> Ansuel
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines
2024-02-02 16:58 ` Conor Dooley
@ 2024-02-02 17:03 ` Christian Marangi
0 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 17:03 UTC (permalink / raw)
To: Conor Dooley
Cc: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Konrad Dybcio, Andrew Lunn,
Heiner Kallweit, Russell King, Frank Rowand, Robert Marko,
netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 04:58:32PM +0000, Conor Dooley wrote:
> On Fri, Feb 02, 2024 at 04:19:15PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 08:41:56AM +0100, Krzysztof Kozlowski wrote:
> > > On 01/02/2024 16:17, Christian Marangi wrote:
> > > > From: Robert Marko <robert.marko@sartura.hr>
> > > >
> > > > Add DT bindings defined for Qualcomm QCA807x PHY series related to
> > > > calibration and DAC settings.
> > >
> > > Nothing from this file is used and your commit msg does not provide
> > > rationale "why", thus it does not look like something for bindings.
> > > Otherwise please point me which patch with *driver* uses these bindings.
> > >
> >
> > Hi, since I have to squash this, I will include the reason in the schema
> > patch.
> >
> > Anyway these are raw values used to configure the qcom,control-dac
> > property.
>
> Maybe I am missing something, but a quick scan of the patchset and a
> grep of the tree doesn't show this property being documented anywhere.
>
> > In the driver it's used by qca807x_config_init. We read what is set in
> > DT and we configure the reg accordingly.
> >
> > If this is wrong should we use a more schema friendly approach with
> > declaring an enum of string and document that there?
>
> Without any idea of what that property is used for it is hard to say,
> but personally I much prefer enums of strings for what looks like a
> property that holds register values.
>
> That you felt it necessary to add defines for the values makes it more
> like that that is the case.
>
Ok, no problem. The big idea here was really to skip having to have a
big function that parse strings but I get that it's better handle with
string and have them documented directly in the yaml.
> >
> > > >
> > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > > include/dt-bindings/net/qcom-qca807x.h | 30 ++++++++++++++++++++++++++
> > >
> > > Use filename matching compatible, so vendor,device. No wildcards, unless
> > > your compatible also has them.
> > >
> > > > 1 file changed, 30 insertions(+)
> > > > create mode 100644 include/dt-bindings/net/qcom-qca807x.h
> > > >
> > >
> > >
> > >
> > > Best regards,
> > > Krzysztof
> > >
> >
> > --
> > Ansuel
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-02 16:40 ` Christian Marangi
@ 2024-02-02 17:04 ` Russell King (Oracle)
2024-02-02 17:07 ` Christian Marangi
2024-02-02 17:08 ` Andrew Lunn
1 sibling, 1 reply; 41+ messages in thread
From: Russell King (Oracle) @ 2024-02-02 17:04 UTC (permalink / raw)
To: Christian Marangi
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote:
> > > +
> > > + 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;
> >
> > I don't see how that works. You have multiple PHYs using this
> > driver. Some might have LEDs, some might have GPOs. But if you modify
> > the driver structure like this, you prevent all PHYs from having LEDs,
> > and maybe cause a Opps if a PHY device has already registered its
> > LEDs?
> >
>
> God you are right! Off-topic but given the effects this may cause, why
> the thing is not const? I assume it wouldn't make sense to add OPS based
> on the detected feature since it would have side effect on other PHYs
> that use the same driver.
Maybe phydev->drv should be const to avoid this kind of thing. It
doesn't look like it would be hard to do, and importantly doesn't
require casting away the const-ness anywhere. PHY drivers themselves
can't be const because the driver model needs to be able to modify
the embedded device_driver struct (e.g. see bus_add_driver().)
drivers/net/phy/phy.c | 3 +--
drivers/net/phy/phy_device.c | 4 ++--
drivers/net/phy/xilinx_gmii2rgmii.c | 2 +-
include/linux/phy.h | 2 +-
4 files changed, 5 insertions(+), 6 deletions(-)
Just build-testing it.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-02 17:04 ` Russell King (Oracle)
@ 2024-02-02 17:07 ` Christian Marangi
0 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 17:07 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 05:04:23PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote:
> > > > +
> > > > + 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;
> > >
> > > I don't see how that works. You have multiple PHYs using this
> > > driver. Some might have LEDs, some might have GPOs. But if you modify
> > > the driver structure like this, you prevent all PHYs from having LEDs,
> > > and maybe cause a Opps if a PHY device has already registered its
> > > LEDs?
> > >
> >
> > God you are right! Off-topic but given the effects this may cause, why
> > the thing is not const? I assume it wouldn't make sense to add OPS based
> > on the detected feature since it would have side effect on other PHYs
> > that use the same driver.
>
> Maybe phydev->drv should be const to avoid this kind of thing. It
> doesn't look like it would be hard to do, and importantly doesn't
> require casting away the const-ness anywhere. PHY drivers themselves
> can't be const because the driver model needs to be able to modify
> the embedded device_driver struct (e.g. see bus_add_driver().)
>
> drivers/net/phy/phy.c | 3 +--
> drivers/net/phy/phy_device.c | 4 ++--
> drivers/net/phy/xilinx_gmii2rgmii.c | 2 +-
> include/linux/phy.h | 2 +-
> 4 files changed, 5 insertions(+), 6 deletions(-)
>
> Just build-testing it.
>
Seems sensible to me. Also for everyone that does that (downstream or
driver that needs to be handled) it would result in a warning for
modifying const stuff. Maybe I'm wrong but I can only see benefits in
doing this change.
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-02 16:40 ` Christian Marangi
2024-02-02 17:04 ` Russell King (Oracle)
@ 2024-02-02 17:08 ` Andrew Lunn
2024-02-02 17:13 ` Christian Marangi
2024-02-02 17:30 ` Russell King (Oracle)
1 sibling, 2 replies; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 17:08 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote:
> > > +
> > > + 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;
> >
> > I don't see how that works. You have multiple PHYs using this
> > driver. Some might have LEDs, some might have GPOs. But if you modify
> > the driver structure like this, you prevent all PHYs from having LEDs,
> > and maybe cause a Opps if a PHY device has already registered its
> > LEDs?
> >
>
> God you are right! Off-topic but given the effects this may cause, why
> the thing is not const?
I would like it to be, but its not easy. There are fields in the
driver structure that phylib needs to modify. e.g. mdiodrv.driver gets
passed to the driver core when registering the driver, and it modifies
it. mdiodrv.flags is also manipulated. So we cannot make the whole
structure const.
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-02 17:08 ` Andrew Lunn
@ 2024-02-02 17:13 ` Christian Marangi
2024-02-02 17:30 ` Russell King (Oracle)
1 sibling, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 17:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 06:08:33PM +0100, Andrew Lunn wrote:
> On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote:
> > > > +
> > > > + 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;
> > >
> > > I don't see how that works. You have multiple PHYs using this
> > > driver. Some might have LEDs, some might have GPOs. But if you modify
> > > the driver structure like this, you prevent all PHYs from having LEDs,
> > > and maybe cause a Opps if a PHY device has already registered its
> > > LEDs?
> > >
> >
> > God you are right! Off-topic but given the effects this may cause, why
> > the thing is not const?
>
> I would like it to be, but its not easy. There are fields in the
> driver structure that phylib needs to modify. e.g. mdiodrv.driver gets
> passed to the driver core when registering the driver, and it modifies
> it. mdiodrv.flags is also manipulated. So we cannot make the whole
> structure const.
>
Maybe the ops part can be detached and just that made const? (and
introduce something like struct phy_driver_ops)
It would require a big conversion but assuming nobody adds OPs in probe
function everything should be static and easy to convert.
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED
2024-02-02 17:08 ` Andrew Lunn
2024-02-02 17:13 ` Christian Marangi
@ 2024-02-02 17:30 ` Russell King (Oracle)
1 sibling, 0 replies; 41+ messages in thread
From: Russell King (Oracle) @ 2024-02-02 17:30 UTC (permalink / raw)
To: Andrew Lunn
Cc: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 06:08:33PM +0100, Andrew Lunn wrote:
> On Fri, Feb 02, 2024 at 05:40:21PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 02:43:37AM +0100, Andrew Lunn wrote:
> > > > +
> > > > + 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;
> > >
> > > I don't see how that works. You have multiple PHYs using this
> > > driver. Some might have LEDs, some might have GPOs. But if you modify
> > > the driver structure like this, you prevent all PHYs from having LEDs,
> > > and maybe cause a Opps if a PHY device has already registered its
> > > LEDs?
> > >
> >
> > God you are right! Off-topic but given the effects this may cause, why
> > the thing is not const?
>
> I would like it to be, but its not easy. There are fields in the
> driver structure that phylib needs to modify. e.g. mdiodrv.driver gets
> passed to the driver core when registering the driver, and it modifies
> it. mdiodrv.flags is also manipulated. So we cannot make the whole
> structure const.
We can make phy_device's drv pointer const though, which would have the
effect of catching code such as the above. That doesn't impact the
driver model nor the mdio layer.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family
2024-02-02 1:35 ` Andrew Lunn
@ 2024-02-02 17:44 ` Christian Marangi
2024-02-03 16:25 ` Andrew Lunn
0 siblings, 1 reply; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 17:44 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 02:35:11AM +0100, Andrew Lunn wrote:
> > +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);
> > + }
>
> This looks a lot like genphy_c37_read_status(). Can it be used?
>
Yes but I had to expand genphy_c37_read_status. Hope it will be OK.
> > +
> > + /* 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, AT803X_SPECIFIC_STATUS);
> > + if (ss < 0)
> > + return ss;
> > +
> > + if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
> > + switch (FIELD_GET(AT803X_SS_SPEED_MASK, ss)) {
> > + case AT803X_SS_SPEED_100:
> > + phydev->speed = SPEED_100;
> > + break;
> > + case AT803X_SS_SPEED_1000:
> > + phydev->speed = SPEED_1000;
> > + break;
> > + }
> > +
> > + if (ss & AT803X_SS_DUPLEX)
> > + phydev->duplex = DUPLEX_FULL;
> > + else
> > + phydev->duplex = DUPLEX_HALF;
> > + }
> > +
> > + return 0;
> > +}
>
>
> > +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:
>
> ...
>
> > + case 500:
> > + priv->tx_driver_strength = PQSGMII_TX_DRIVER_500MV;
> > + break;
> > + case 600:
> > + default:
>
> If its missing default to 600. But if its an invalid value, return
> -EINVAL.
>
> > + 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;
>
> Again, return -EINVAL if it is neither.
>
> > +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;
>
> What about priv->package_mode == PHY_INTERFACE_MODE_NA;
>
I changed this to a switch and added some comments to make this more
clear. We default to PSGMII if package-mode is not defined. (will also
update schema with default value)
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-02 15:12 ` Christian Marangi
@ 2024-02-02 20:39 ` Rob Herring
2024-02-02 20:47 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Rob Herring @ 2024-02-02 20:39 UTC (permalink / raw)
To: Christian Marangi
Cc: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
Russell King, Frank Rowand, Robert Marko, netdev, devicetree,
linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 04:12:53PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> > On 01/02/2024 16:17, Christian Marangi wrote:
> > > Document Qcom QCA807x PHY package.
> > >
> > > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > 1000BASE-T PHY-s.
> > >
> > > Document the required property to make the PHY package correctly
> > > configure and work.
> > >
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > > .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> >
> > Your bindings header must be squashed here. Headers are not separate
> > thing from the bindings.
> >
> > > 1 file changed, 142 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > new file mode 100644
> > > index 000000000000..1c3692897b02
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > @@ -0,0 +1,142 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm QCA807X Ethernet PHY
> >
> > What is "X"? Wildcards are usually not expected.
> >
>
> It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
> or qca8074 since they would refer to a more generic Family of devices.
Declare them all or provide some justification such as the exact model
is discoverable (and better be sure power on is the same in order to do
discovery).
> What would be the correct way? We have many other case on net with
> schema called qca8k that refer to the family of Ethernet Switch but in
> it refer to qca8327 qca8337 qca8334...
>
> > > +
> > > +maintainers:
> > > + - Christian Marangi <ansuelsmth@gmail.com>
> > > + - Robert Marko <robert.marko@sartura.hr>
> > > +
> > > +description: |
> > > + Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > + IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > + 1000BASE-T PHY-s.
> > > +
> > > + They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > > + MAC, while second one is SGMII for connection to MAC or fiber.
> > > +
> > > + Both models have a combo port that supports 1000BASE-X and
> > > + 100BASE-FX fiber.
> > > +
> > > + Each PHY inside of QCA807x series has 4 digitally controlled
> > > + output only pins that natively drive LED-s for up to 2 attached
> > > + LEDs. Some vendor also use these 4 output for GPIO usage without
> > > + attaching LEDs.
> > > +
> > > + Note that output pins can be set to drive LEDs OR GPIO, mixed
> > > + definition are not accepted.
> > > +
> > > + PHY package can be configured in 3 mode following this table:
> > > +
> > > + First Serdes mode Second Serdes mode
> > > + Option 1 PSGMII for copper Disabled
> > > + ports 0-4
> > > + Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
> > > + ports 0-4
> > > + Option 3 QSGMII for copper SGMII for
> > > + ports 0-3 copper port 4
> > > +
> > > +$ref: ethernet-phy-package.yaml#
> > > +
> > > +properties:
> > > + compatible:
> > > + const: qcom,qca807x-package
> > > +
> > > + qcom,package-mode:
> >
> > Where is definition of this property with type and description?
> >
> > > + enum:
> > > + - qsgmii
> > > + - psgmii
> > > +
> > > + qcom,tx-driver-strength:
> >
> > Use proper unit suffix.
> >
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> >
> > > + description: set the TX Amplifier value in mv.
> > > + If not defined, 600mw is set by default.
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + enum: [140, 160, 180, 200, 220,
> > > + 240, 260, 280, 300, 320,
> > > + 400, 500, 600]
> > > +
> > > +patternProperties:
> > > + ^ethernet-phy(@[a-f0-9]+)?$:
> > > + $ref: ethernet-phy.yaml#
> > > +
> > > + properties:
> > > + gpio-controller:
> > > + description: set the output lines as GPIO instead of LEDs
> > > + type: boolean
You only need 'gpio-controller: true'. The core already defines the
type.
> > > +
> > > + '#gpio-cells':
> > > + description: number of GPIO cells for the PHY
Not a useful description. Normally, you'd describe what's in the cells,
but GPIO is standardized so no need (unless you are deviating from the
norm).
> > > + const: 2
> > > +
> > > + dependencies:
> > > + gpio-controller: ['#gpio-cells']
> >
> > Why do you need it? None of gpio-controllers do it, I think.
> >
>
> Well gpio-controller property is optional and having that declared and
> #gpio-cells skipped will result in an error on probe. I think it should
> be the opposite with other schema having to declare this.
If you think everything else is wrong, then please fix them. :)
>
> In usual way for gpio-controller both property are defined as required
> but you can see some (to-be-converted) txt files whith comments where
> they say:
>
> ./mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.
>
> Should I instead add this condition in the if right below?
The core schema enforces this dependency, so you don't need to.
Rob
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-01 15:17 ` [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
2024-02-02 1:16 ` Andrew Lunn
2024-02-02 7:45 ` Krzysztof Kozlowski
@ 2024-02-02 20:45 ` Rob Herring
2024-02-02 21:50 ` Andrew Lunn
2 siblings, 1 reply; 41+ messages in thread
From: Rob Herring @ 2024-02-02 20:45 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Robert Marko, netdev, devicetree, linux-kernel,
linux-arm-msm
On Thu, Feb 01, 2024 at 04:17:32PM +0100, Christian Marangi wrote:
> Document Qcom QCA807x PHY package.
>
> Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> 1000BASE-T PHY-s.
>
> Document the required property to make the PHY package correctly
> configure and work.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> 1 file changed, 142 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> new file mode 100644
> index 000000000000..1c3692897b02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QCA807X Ethernet PHY
> +
> +maintainers:
> + - Christian Marangi <ansuelsmth@gmail.com>
> + - Robert Marko <robert.marko@sartura.hr>
> +
> +description: |
> + Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> + IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> + 1000BASE-T PHY-s.
> +
> + They feature 2 SerDes, one for PSGMII or QSGMII connection with
> + MAC, while second one is SGMII for connection to MAC or fiber.
> +
> + Both models have a combo port that supports 1000BASE-X and
> + 100BASE-FX fiber.
> +
> + Each PHY inside of QCA807x series has 4 digitally controlled
> + output only pins that natively drive LED-s for up to 2 attached
> + LEDs. Some vendor also use these 4 output for GPIO usage without
> + attaching LEDs.
> +
> + Note that output pins can be set to drive LEDs OR GPIO, mixed
> + definition are not accepted.
> +
> + PHY package can be configured in 3 mode following this table:
> +
> + First Serdes mode Second Serdes mode
> + Option 1 PSGMII for copper Disabled
> + ports 0-4
> + Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
> + ports 0-4
> + Option 3 QSGMII for copper SGMII for
> + ports 0-3 copper port 4
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> + compatible:
> + const: qcom,qca807x-package
> +
> + qcom,package-mode:
> + enum:
> + - qsgmii
> + - psgmii
> +
> + qcom,tx-driver-strength:
> + description: set the TX Amplifier value in mv.
> + If not defined, 600mw is set by default.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [140, 160, 180, 200, 220,
> + 240, 260, 280, 300, 320,
> + 400, 500, 600]
> +
> +patternProperties:
> + ^ethernet-phy(@[a-f0-9]+)?$:
I don't get how an address is optional.
Rob
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-02 20:39 ` Rob Herring
@ 2024-02-02 20:47 ` Christian Marangi
0 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 20:47 UTC (permalink / raw)
To: Rob Herring
Cc: Krzysztof Kozlowski, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Andrew Lunn, Heiner Kallweit,
Russell King, Frank Rowand, Robert Marko, netdev, devicetree,
linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 02:39:15PM -0600, Rob Herring wrote:
> On Fri, Feb 02, 2024 at 04:12:53PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> > > On 01/02/2024 16:17, Christian Marangi wrote:
> > > > Document Qcom QCA807x PHY package.
> > > >
> > > > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > > 1000BASE-T PHY-s.
> > > >
> > > > Document the required property to make the PHY package correctly
> > > > configure and work.
> > > >
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > > .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> > >
> > > Your bindings header must be squashed here. Headers are not separate
> > > thing from the bindings.
> > >
> > > > 1 file changed, 142 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > > new file mode 100644
> > > > index 000000000000..1c3692897b02
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > > > @@ -0,0 +1,142 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Qualcomm QCA807X Ethernet PHY
> > >
> > > What is "X"? Wildcards are usually not expected.
> > >
> >
> > It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
> > or qca8074 since they would refer to a more generic Family of devices.
>
> Declare them all or provide some justification such as the exact model
> is discoverable (and better be sure power on is the same in order to do
> discovery).
>
> > What would be the correct way? We have many other case on net with
> > schema called qca8k that refer to the family of Ethernet Switch but in
> > it refer to qca8327 qca8337 qca8334...
> >
> > > > +
> > > > +maintainers:
> > > > + - Christian Marangi <ansuelsmth@gmail.com>
> > > > + - Robert Marko <robert.marko@sartura.hr>
> > > > +
> > > > +description: |
> > > > + Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > > > + IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > > > + 1000BASE-T PHY-s.
> > > > +
> > > > + They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > > > + MAC, while second one is SGMII for connection to MAC or fiber.
> > > > +
> > > > + Both models have a combo port that supports 1000BASE-X and
> > > > + 100BASE-FX fiber.
> > > > +
> > > > + Each PHY inside of QCA807x series has 4 digitally controlled
> > > > + output only pins that natively drive LED-s for up to 2 attached
> > > > + LEDs. Some vendor also use these 4 output for GPIO usage without
> > > > + attaching LEDs.
> > > > +
> > > > + Note that output pins can be set to drive LEDs OR GPIO, mixed
> > > > + definition are not accepted.
> > > > +
> > > > + PHY package can be configured in 3 mode following this table:
> > > > +
> > > > + First Serdes mode Second Serdes mode
> > > > + Option 1 PSGMII for copper Disabled
> > > > + ports 0-4
> > > > + Option 2 PSGMII for copper 1000BASE-X / 100BASE-FX
> > > > + ports 0-4
> > > > + Option 3 QSGMII for copper SGMII for
> > > > + ports 0-3 copper port 4
> > > > +
> > > > +$ref: ethernet-phy-package.yaml#
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + const: qcom,qca807x-package
> > > > +
> > > > + qcom,package-mode:
> > >
> > > Where is definition of this property with type and description?
> > >
> > > > + enum:
> > > > + - qsgmii
> > > > + - psgmii
> > > > +
> > > > + qcom,tx-driver-strength:
> > >
> > > Use proper unit suffix.
> > >
> > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> > >
> > > > + description: set the TX Amplifier value in mv.
> > > > + If not defined, 600mw is set by default.
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + enum: [140, 160, 180, 200, 220,
> > > > + 240, 260, 280, 300, 320,
> > > > + 400, 500, 600]
> > > > +
> > > > +patternProperties:
> > > > + ^ethernet-phy(@[a-f0-9]+)?$:
> > > > + $ref: ethernet-phy.yaml#
> > > > +
> > > > + properties:
> > > > + gpio-controller:
> > > > + description: set the output lines as GPIO instead of LEDs
> > > > + type: boolean
>
> You only need 'gpio-controller: true'. The core already defines the
> type.
>
> > > > +
> > > > + '#gpio-cells':
> > > > + description: number of GPIO cells for the PHY
>
> Not a useful description. Normally, you'd describe what's in the cells,
> but GPIO is standardized so no need (unless you are deviating from the
> norm).
>
> > > > + const: 2
> > > > +
> > > > + dependencies:
> > > > + gpio-controller: ['#gpio-cells']
> > >
> > > Why do you need it? None of gpio-controllers do it, I think.
> > >
> >
> > Well gpio-controller property is optional and having that declared and
> > #gpio-cells skipped will result in an error on probe. I think it should
> > be the opposite with other schema having to declare this.
>
> If you think everything else is wrong, then please fix them. :)
>
> >
> > In usual way for gpio-controller both property are defined as required
> > but you can see some (to-be-converted) txt files whith comments where
> > they say:
> >
> > ./mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.
> >
> > Should I instead add this condition in the if right below?
>
> The core schema enforces this dependency, so you don't need to.
>
Oh! No idea the dependency was already enforced, guess I don't have to
fix everything ahahahha
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes
2024-02-01 15:17 ` [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
@ 2024-02-02 20:52 ` Rob Herring
2024-02-02 20:58 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Rob Herring @ 2024-02-02 20:52 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Robert Marko, netdev, devicetree, linux-kernel,
linux-arm-msm
On Thu, Feb 01, 2024 at 04:17:27PM +0100, Christian Marangi wrote:
> Document ethernet PHY package nodes used to describe PHY shipped in
> bundle of 2-5 PHY. The special node describe a container of PHY that
> share common properties. This is a generic schema and PHY package
> should create specialized version with the required additional shared
> properties.
>
> Example are PHY packages that have some regs only in one PHY of the
> package and will affect every other PHY in the package, for example
> related to PHY interface mode calibration or global PHY mode selection.
>
> The PHY package node MUST declare the base address used by the PHY driver
> for global configuration by calculating the offsets of the global PHY
> based on the base address of the PHY package.
>
> Each reg of the PHYs defined in the PHY Package node is an offset of the
> PHY Package reg.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> .../bindings/net/ethernet-phy-package.yaml | 55 +++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> new file mode 100644
> index 000000000000..d7cdbb1a4b3e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ethernet PHY Package Common Properties
> +
> +maintainers:
> + - Christian Marangi <ansuelsmth@gmail.com>
> +
> +description:
> + PHY packages are multi-port Ethernet PHY of the same family
> + and each Ethernet PHY is affected by the global configuration
> + of the PHY package.
> +
> + Each reg of the PHYs defined in the PHY Package node is
> + an offset of the PHY Package reg.
> +
> + Each Ethernet PHYs defined in the PHY package node is
> + reachable in the MDIO bus at the address of the PHY
> + Package offset of the Ethernet PHY reg.
If the phys are addressed with an MDIO address, then just use those.
> +
> +properties:
> + $nodename:
> + pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
Can't be optional if 'reg' is required (which it should be).
> +
> + reg:
> + minimum: 0
> + maximum: 31
> + description:
> + The base ID number for the PHY package.
> + Commonly the ID of the first PHY in the PHY package.
> +
> + Some PHY in the PHY package might be not defined but
> + still occupy ID on the device (just not attached to
> + anything) hence the PHY package reg might correspond
> + to a not attached PHY (offset 0).
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +patternProperties:
> + ^ethernet-phy(@[a-f0-9]+)?$:
Same issue here.
> + $ref: ethernet-phy.yaml#
> +
> +required:
> + - reg
> + - '#address-cells'
> + - '#size-cells'
> +
> +additionalProperties: true
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes
2024-02-02 20:52 ` Rob Herring
@ 2024-02-02 20:58 ` Christian Marangi
0 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-02 20:58 UTC (permalink / raw)
To: Rob Herring
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Andrew Lunn, Heiner Kallweit, Russell King,
Frank Rowand, Robert Marko, netdev, devicetree, linux-kernel,
linux-arm-msm
On Fri, Feb 02, 2024 at 02:52:59PM -0600, Rob Herring wrote:
> On Thu, Feb 01, 2024 at 04:17:27PM +0100, Christian Marangi wrote:
> > Document ethernet PHY package nodes used to describe PHY shipped in
> > bundle of 2-5 PHY. The special node describe a container of PHY that
> > share common properties. This is a generic schema and PHY package
> > should create specialized version with the required additional shared
> > properties.
> >
> > Example are PHY packages that have some regs only in one PHY of the
> > package and will affect every other PHY in the package, for example
> > related to PHY interface mode calibration or global PHY mode selection.
> >
> > The PHY package node MUST declare the base address used by the PHY driver
> > for global configuration by calculating the offsets of the global PHY
> > based on the base address of the PHY package.
> >
> > Each reg of the PHYs defined in the PHY Package node is an offset of the
> > PHY Package reg.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > .../bindings/net/ethernet-phy-package.yaml | 55 +++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > new file mode 100644
> > index 000000000000..d7cdbb1a4b3e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/ethernet-phy-package.yaml
> > @@ -0,0 +1,55 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/ethernet-phy-package.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ethernet PHY Package Common Properties
> > +
> > +maintainers:
> > + - Christian Marangi <ansuelsmth@gmail.com>
> > +
> > +description:
> > + PHY packages are multi-port Ethernet PHY of the same family
> > + and each Ethernet PHY is affected by the global configuration
> > + of the PHY package.
> > +
> > + Each reg of the PHYs defined in the PHY Package node is
> > + an offset of the PHY Package reg.
> > +
> > + Each Ethernet PHYs defined in the PHY package node is
> > + reachable in the MDIO bus at the address of the PHY
> > + Package offset of the Ethernet PHY reg.
>
> If the phys are addressed with an MDIO address, then just use those.
>
Thanks a lot for the review Rob.
Just to make sure I got this right!
Are you ok to have the PHYs with absolute reg? (it would make thing soo
much easy)
It would result in this node example on real world scenario.
ethernet-phy-package@16 {
compatible = "qcom,qca8075-package";
#address-cells = <1>;
#size-cells = <0>;
reg = <16>;
qcom,package-mode = "qsgmii";
qca8075_16: ethernet-phy@16 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <16>;
};
qca8075_17: ethernet-phy@17 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <17>;
};
qca8075_18: ethernet-phy@18 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <18>;
};
qca8075_19: ethernet-phy@19 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <19>;
};
};
> > +
> > +properties:
> > + $nodename:
> > + pattern: "^ethernet-phy-package(@[a-f0-9]+)?$"
>
> Can't be optional if 'reg' is required (which it should be).
>
> > +
> > + reg:
> > + minimum: 0
> > + maximum: 31
> > + description:
> > + The base ID number for the PHY package.
> > + Commonly the ID of the first PHY in the PHY package.
> > +
> > + Some PHY in the PHY package might be not defined but
> > + still occupy ID on the device (just not attached to
> > + anything) hence the PHY package reg might correspond
> > + to a not attached PHY (offset 0).
> > +
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 0
> > +
> > +patternProperties:
> > + ^ethernet-phy(@[a-f0-9]+)?$:
>
> Same issue here.
>
> > + $ref: ethernet-phy.yaml#
> > +
> > +required:
> > + - reg
> > + - '#address-cells'
> > + - '#size-cells'
> > +
> > +additionalProperties: true
> > --
> > 2.43.0
> >
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package
2024-02-02 20:45 ` Rob Herring
@ 2024-02-02 21:50 ` Andrew Lunn
0 siblings, 0 replies; 41+ messages in thread
From: Andrew Lunn @ 2024-02-02 21:50 UTC (permalink / raw)
To: Rob Herring
Cc: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
> > +patternProperties:
> > + ^ethernet-phy(@[a-f0-9]+)?$:
>
> I don't get how an address is optional.
Its pretty unusual, but for example:
arch/arm/boot/dts/nxp/imx/imx6q-novena.dts
&fec {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_enet_novena>;
phy-mode = "rgmii";
phy-handle = <ðphy>;
phy-reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
status = "okay";
mdio {
#address-cells = <1>;
#size-cells = <0>;
ethphy: ethernet-phy {
compatible = "ethernet-phy-ieee802.3-c22";
rxc-skew-ps = <3000>;
rxdv-skew-ps = <0>;
txc-skew-ps = <3000>;
txen-skew-ps = <0>;
rxd0-skew-ps = <0>;
rxd1-skew-ps = <0>;
rxd2-skew-ps = <0>;
rxd3-skew-ps = <0>;
txd0-skew-ps = <3000>;
txd1-skew-ps = <3000>;
txd2-skew-ps = <3000>;
txd3-skew-ps = <3000>;
};
};
};
There is no reg property, because its optional. If there is no reg,
there is no address.
When phylib finds a DT blob like this, it enumerates the bus, and then
assigns the nodes to the devices it finds in the order it finds them.
Its old behaviour, from before the times of yaml validation, and
current best practices, etc. But because it works, it still used in
new bindings.
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family
2024-02-02 17:44 ` Christian Marangi
@ 2024-02-03 16:25 ` Andrew Lunn
2024-02-03 16:28 ` Christian Marangi
0 siblings, 1 reply; 41+ messages in thread
From: Andrew Lunn @ 2024-02-03 16:25 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Fri, Feb 02, 2024 at 06:44:22PM +0100, Christian Marangi wrote:
> On Fri, Feb 02, 2024 at 02:35:11AM +0100, Andrew Lunn wrote:
> > > +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);
> > > + }
> >
> > This looks a lot like genphy_c37_read_status(). Can it be used?
> >
>
> Yes but I had to expand genphy_c37_read_status. Hope it will be OK.
You can expand it, but please keep to what is defined within 802.3. We
don't want any vendor extensions in this common code. Vendor things
should be kept in the vendor driver. So you can call
genphy_c37_read_status() and then do any vendor specific fixups
needed.
Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family
2024-02-03 16:25 ` Andrew Lunn
@ 2024-02-03 16:28 ` Christian Marangi
0 siblings, 0 replies; 41+ messages in thread
From: Christian Marangi @ 2024-02-03 16:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Heiner Kallweit, Russell King, Frank Rowand,
Robert Marko, netdev, devicetree, linux-kernel, linux-arm-msm
On Sat, Feb 03, 2024 at 05:25:23PM +0100, Andrew Lunn wrote:
> On Fri, Feb 02, 2024 at 06:44:22PM +0100, Christian Marangi wrote:
> > On Fri, Feb 02, 2024 at 02:35:11AM +0100, Andrew Lunn wrote:
> > > > +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);
> > > > + }
> > >
> > > This looks a lot like genphy_c37_read_status(). Can it be used?
> > >
> >
> > Yes but I had to expand genphy_c37_read_status. Hope it will be OK.
>
> You can expand it, but please keep to what is defined within 802.3. We
> don't want any vendor extensions in this common code. Vendor things
> should be kept in the vendor driver. So you can call
> genphy_c37_read_status() and then do any vendor specific fixups
> needed.
>
Sure the expansion is just adding a bool signal if the link has changed
or not (to make it possible to exit early and skip the additional vendor
call...) I didn't add anything to the c37 function ifself.
Anyway of from this, the revision is ready, just need to understand if
Rob is ok with absolute or relative address for PHYs in the PHY package
node.
--
Ansuel
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2024-02-03 16:28 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-01 15:17 [net-next PATCH v5 0/9] net: phy: Introduce PHY Package concept Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 1/9] dt-bindings: net: document ethernet PHY package nodes Christian Marangi
2024-02-02 20:52 ` Rob Herring
2024-02-02 20:58 ` Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 2/9] net: phy: add support for scanning PHY in PHY packages nodes Christian Marangi
2024-02-01 16:25 ` Antoine Tenart
2024-02-01 17:20 ` Christian Marangi
2024-02-02 1:02 ` Andrew Lunn
2024-02-02 10:05 ` Antoine Tenart
2024-02-01 15:17 ` [net-next PATCH v5 3/9] net: phy: add devm/of_phy_package_join helper Christian Marangi
2024-02-01 16:40 ` Antoine Tenart
2024-02-01 16:48 ` Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 4/9] net: phy: qcom: move more function to shared library Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 5/9] dt-bindings: net: add QCA807x PHY defines Christian Marangi
2024-02-02 7:41 ` Krzysztof Kozlowski
2024-02-02 15:19 ` Christian Marangi
2024-02-02 16:58 ` Conor Dooley
2024-02-02 17:03 ` Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package Christian Marangi
2024-02-02 1:16 ` Andrew Lunn
2024-02-02 7:45 ` Krzysztof Kozlowski
2024-02-02 15:12 ` Christian Marangi
2024-02-02 20:39 ` Rob Herring
2024-02-02 20:47 ` Christian Marangi
2024-02-02 20:45 ` Rob Herring
2024-02-02 21:50 ` Andrew Lunn
2024-02-01 15:17 ` [net-next PATCH v5 7/9] net: phy: qcom: add support for QCA807x PHY Family Christian Marangi
2024-02-02 1:35 ` Andrew Lunn
2024-02-02 17:44 ` Christian Marangi
2024-02-03 16:25 ` Andrew Lunn
2024-02-03 16:28 ` Christian Marangi
2024-02-01 15:17 ` [net-next PATCH v5 8/9] net: phy: qcom: generalize some qca808x LED functions Christian Marangi
2024-02-02 1:38 ` Andrew Lunn
2024-02-01 15:17 ` [net-next PATCH v5 9/9] net: phy: qca807x: add support for configurable LED Christian Marangi
2024-02-02 1:43 ` Andrew Lunn
2024-02-02 16:40 ` Christian Marangi
2024-02-02 17:04 ` Russell King (Oracle)
2024-02-02 17:07 ` Christian Marangi
2024-02-02 17:08 ` Andrew Lunn
2024-02-02 17:13 ` Christian Marangi
2024-02-02 17:30 ` Russell King (Oracle)
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).