This patch set provides ACPI support to DPAA2 network drivers.
It also introduces new fwnode based APIs to support phylink and phy
layers
Following functions are defined:
phylink_fwnode_phy_connect()
fwnode_mdiobus_register_phy()
fwnode_mdiobus_register()
fwnode_get_phy_id()
fwnode_phy_find_device()
device_phy_find_device()
fwnode_get_phy_node()
fwnode_mdio_find_device()
fwnode_get_id()
First one helps in connecting phy to phylink instance.
Next three helps in getting phy_id and registering phy to mdiobus
Next two help in finding a phy on a mdiobus.
Next one helps in getting phy_node from a fwnode.
Last one is used to get fwnode ID.
Corresponding OF functions are refactored.
END
Changes in v2:
- Updated with more description in document
- use reverse christmas tree ordering for local variables
- Refactor OF functions to use fwnode functions
Calvin Johnson (14):
Documentation: ACPI: DSD: Document MDIO PHY
net: phy: Introduce phy related fwnode functions
of: mdio: Refactor of_phy_find_device()
net: phy: Introduce fwnode_get_phy_id()
of: mdio: Refactor of_get_phy_id()
net: mdiobus: Introduce fwnode_mdiobus_register_phy()
of: mdio: Refactor of_mdiobus_register_phy()
net: mdiobus: Introduce fwnode_mdiobus_register()
net/fsl: Use fwnode_mdiobus_register()
device property: Introduce fwnode_get_id()
phylink: introduce phylink_fwnode_phy_connect()
net: phylink: Refactor phylink_of_phy_connect()
net: phy: Introduce fwnode_mdio_find_device()
net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
drivers/base/property.c | 26 ++++
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 86 +++++++-----
drivers/net/ethernet/freescale/xgmac_mdio.c | 14 +-
drivers/net/mdio/of_mdio.c | 79 +----------
drivers/net/phy/mdio_bus.c | 116 ++++++++++++++++
drivers/net/phy/phy_device.c | 108 +++++++++++++++
drivers/net/phy/phylink.c | 49 ++++---
include/linux/mdio.h | 2 +
include/linux/of_mdio.h | 6 +-
include/linux/phy.h | 32 +++++
include/linux/phylink.h | 3 +
include/linux/property.h | 1 +
13 files changed, 519 insertions(+), 132 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
--
2.17.1
Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
include/linux/phy.h | 5 +++++
2 files changed, 26 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c153273606c1..6fad89c02c5a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -846,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
return 0;
}
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-idAAAA.BBBB.
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ unsigned int upper, lower;
+ const char *cp;
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+ if (ret)
+ return ret;
+
+ if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
+ *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
+ return 0;
+ }
+ return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_get_phy_id);
+
/**
* get_phy_device - reads the specified PHY device and returns its @phy_device
* struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7790a9a56d0f..10a66b65a008 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1341,6 +1341,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
bool is_c45,
struct phy_c45_device_ids *c45_ids);
#if IS_ENABLED(CONFIG_PHYLIB)
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct phy_device *device_phy_find_device(struct device *dev);
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1348,6 +1349,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
int phy_device_register(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
#else
+static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ return 0;
+}
static inline
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
{
--
2.17.1
Using fwnode_get_id(), get the reg property value for DT node
and get the _ADR object value for ACPI node.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/base/property.c | 26 ++++++++++++++++++++++++++
include/linux/property.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 4c43d30145c6..1c50e17ae879 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
return fwnode_call_ptr_op(fwnode, get_name_prefix);
}
+/**
+ * fwnode_get_id - Get the id of a fwnode.
+ * @fwnode: firmware node
+ * @id: id of the fwnode
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+ unsigned long long adr;
+ acpi_status status;
+
+ if (is_of_node(fwnode)) {
+ return of_property_read_u32(to_of_node(fwnode), "reg", id);
+ } else if (is_acpi_node(fwnode)) {
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -ENODATA;
+ *id = (u32)adr;
+ return 0;
+ }
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_id);
+
/**
* fwnode_get_parent - Return parent firwmare node
* @fwnode: Firmware whose parent is retrieved
diff --git a/include/linux/property.h b/include/linux/property.h
index 2d4542629d80..92d405cf2b07 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
const char *fwnode_get_name(const struct fwnode_handle *fwnode);
const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
struct fwnode_handle *fwnode_get_next_parent(
struct fwnode_handle *fwnode);
--
2.17.1
Define fwnode_phy_find_device() to iterate an mdiobus and find the
phy device of the provided phy fwnode. Additionally define
device_phy_find_device() to find phy device of provided device.
Define fwnode_get_phy_node() to get phy_node using named reference.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2:
- use reverse christmas tree ordering for local variables
drivers/net/phy/phy_device.c | 64 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 20 +++++++++++
2 files changed, 84 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 80c2e646c093..c153273606c1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/acpi.h>
#include <linux/bitmap.h>
#include <linux/delay.h>
#include <linux/errno.h>
@@ -2829,6 +2830,69 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->handle_interrupt;
}
+/**
+ * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
+ * phy_fwnode.
+ * @phy_fwnode: Pointer to the phy's fwnode.
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+ struct mdio_device *mdiodev;
+ struct device *d;
+
+ if (!phy_fwnode)
+ return NULL;
+
+ d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
+ if (d) {
+ mdiodev = to_mdio_device(d);
+ if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+ return to_phy_device(d);
+ put_device(d);
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
+/**
+ * device_phy_find_device - For the given device, get the phy_device
+ * @dev: Pointer to the given device
+ *
+ * Refer return conditions of fwnode_phy_find_device().
+ */
+struct phy_device *device_phy_find_device(struct device *dev)
+{
+ return fwnode_phy_find_device(dev_fwnode(dev));
+}
+EXPORT_SYMBOL_GPL(device_phy_find_device);
+
+/**
+ * fwnode_get_phy_node - Get the phy_node using the named reference.
+ * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
+ *
+ * Refer return conditions of fwnode_find_reference().
+ * For ACPI, only "phy-handle" is supported. DT supports all the three
+ * named references to the phy node.
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *phy_node;
+
+ /* Only phy-handle is used for ACPI */
+ phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
+ if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
+ return phy_node;
+ phy_node = fwnode_find_reference(fwnode, "phy", 0);
+ if (IS_ERR(phy_node))
+ phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
+ return phy_node;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
+
/**
* phy_probe - probe and init a PHY device
* @dev: device to probe and init
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 381a95732b6a..7790a9a56d0f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1341,10 +1341,30 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
bool is_c45,
struct phy_c45_device_ids *c45_ids);
#if IS_ENABLED(CONFIG_PHYLIB)
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+struct phy_device *device_phy_find_device(struct device *dev);
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
int phy_device_register(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
#else
+static inline
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+ return NULL;
+}
+
+static inline struct phy_device *device_phy_find_device(struct device *dev)
+{
+ return NULL;
+}
+
+static inline
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+ return NULL;
+}
+
static inline
struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
{
--
2.17.1
Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.
Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.
Use helper function phylink_fwnode_phy_connect() to find phy_dev and
connect to mac->phylink.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2:
- Refactor OF functions to use fwnode functions
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 86 +++++++++++--------
1 file changed, 50 insertions(+), 36 deletions(-)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 828c177df03d..c242d5c2a9ed 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -1,6 +1,9 @@
// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
/* Copyright 2019 NXP */
+#include <linux/acpi.h>
+#include <linux/property.h>
+
#include "dpaa2-eth.h"
#include "dpaa2-mac.h"
@@ -34,39 +37,47 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
return 0;
}
-/* Caller must call of_node_put on the returned value */
-static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
+static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
+ u16 dpmac_id)
{
- struct device_node *dpmacs, *dpmac = NULL;
- u32 id;
+ struct device_node *dpmacs = NULL;
+ struct fwnode_handle *parent, *child = NULL;
int err;
+ u32 id;
- dpmacs = of_find_node_by_name(NULL, "dpmacs");
- if (!dpmacs)
- return NULL;
+ if (is_of_node(dev->parent->fwnode)) {
+ dpmacs = of_find_node_by_name(NULL, "dpmacs");
+ if (!dpmacs)
+ return NULL;
+ parent = of_fwnode_handle(dpmacs);
+ } else if (is_acpi_node(dev->parent->fwnode)) {
+ parent = dev->parent->fwnode;
+ }
- while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
- err = of_property_read_u32(dpmac, "reg", &id);
- if (err)
+ fwnode_for_each_child_node(parent, child) {
+ err = fwnode_get_id(child, &id);
+ if (err) {
continue;
- if (id == dpmac_id)
- break;
+ } else if (id == dpmac_id) {
+ if (is_of_node(dev->parent->fwnode))
+ of_node_put(dpmacs);
+ return child;
+ }
}
-
- of_node_put(dpmacs);
-
- return dpmac;
+ if (is_of_node(dev->parent->fwnode))
+ of_node_put(dpmacs);
+ return NULL;
}
-static int dpaa2_mac_get_if_mode(struct device_node *node,
+static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
struct dpmac_attr attr)
{
phy_interface_t if_mode;
int err;
- err = of_get_phy_mode(node, &if_mode);
- if (!err)
- return if_mode;
+ err = fwnode_get_phy_mode(dpmac_node);
+ if (err > 0)
+ return err;
err = phy_mode(attr.eth_if, &if_mode);
if (!err)
@@ -255,26 +266,27 @@ bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev,
}
static int dpaa2_pcs_create(struct dpaa2_mac *mac,
- struct device_node *dpmac_node, int id)
+ struct fwnode_handle *dpmac_node,
+ int id)
{
struct mdio_device *mdiodev;
- struct device_node *node;
+ struct fwnode_handle *node;
- node = of_parse_phandle(dpmac_node, "pcs-handle", 0);
- if (!node) {
+ node = fwnode_find_reference(dpmac_node, "pcs-handle", 0);
+ if (IS_ERR(node)) {
/* do not error out on old DTS files */
netdev_warn(mac->net_dev, "pcs-handle node not found\n");
return 0;
}
- if (!of_device_is_available(node)) {
+ if (!of_device_is_available(to_of_node(node))) {
netdev_err(mac->net_dev, "pcs-handle node not available\n");
- of_node_put(node);
+ of_node_put(to_of_node(node));
return -ENODEV;
}
- mdiodev = of_mdio_find_device(node);
- of_node_put(node);
+ mdiodev = fwnode_mdio_find_device(node);
+ fwnode_handle_put(node);
if (!mdiodev)
return -EPROBE_DEFER;
@@ -304,7 +316,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
{
struct fsl_mc_device *dpmac_dev = mac->mc_dev;
struct net_device *net_dev = mac->net_dev;
- struct device_node *dpmac_node;
+ struct fwnode_handle *dpmac_node = NULL;
struct phylink *phylink;
struct dpmac_attr attr;
int err;
@@ -324,7 +336,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
mac->if_link_type = attr.link_type;
- dpmac_node = dpaa2_mac_get_node(attr.id);
+ dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, attr.id);
if (!dpmac_node) {
netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id);
err = -ENODEV;
@@ -342,7 +354,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
* error out if the interface mode requests them and there is no PHY
* to act upon them
*/
- if (of_phy_is_fixed_link(dpmac_node) &&
+ if (of_phy_is_fixed_link(to_of_node(dpmac_node)) &&
(mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
@@ -362,7 +374,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
mac->phylink_config.type = PHYLINK_NETDEV;
phylink = phylink_create(&mac->phylink_config,
- of_fwnode_handle(dpmac_node), mac->if_mode,
+ dpmac_node, mac->if_mode,
&dpaa2_mac_phylink_ops);
if (IS_ERR(phylink)) {
err = PTR_ERR(phylink);
@@ -373,13 +385,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
if (mac->pcs)
phylink_set_pcs(mac->phylink, &mac->pcs->pcs);
- err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
+ err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0);
if (err) {
- netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err);
+ netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err);
goto err_phylink_destroy;
}
- of_node_put(dpmac_node);
+ if (is_of_node(dpmac_node))
+ fwnode_handle_put(dpmac_node);
return 0;
@@ -388,7 +401,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
err_pcs_destroy:
dpaa2_pcs_destroy(mac);
err_put_node:
- of_node_put(dpmac_node);
+ if (is_of_node(dpmac_node))
+ fwnode_handle_put(dpmac_node);
err_close_dpmac:
dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
return err;
--
2.17.1
With the introduction of fwnode_get_phy_id(), refactor of_get_phy_id()
to use fwnode equivalent.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/mdio/of_mdio.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index fde76bee78b3..31e6435dcc9f 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -29,17 +29,7 @@ MODULE_LICENSE("GPL");
* ethernet-phy-idAAAA.BBBB */
static int of_get_phy_id(struct device_node *device, u32 *phy_id)
{
- struct property *prop;
- const char *cp;
- unsigned int upper, lower;
-
- of_property_for_each_string(device, "compatible", prop, cp) {
- if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
- *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
- return 0;
- }
- }
- return -EINVAL;
+ return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
}
static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
--
2.17.1
Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/phy/phylink.c | 39 +--------------------------------------
1 file changed, 1 insertion(+), 38 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 389dc3ec165e..26f014f0ad42 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1080,44 +1080,7 @@ EXPORT_SYMBOL_GPL(phylink_connect_phy);
int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
u32 flags)
{
- struct device_node *phy_node;
- struct phy_device *phy_dev;
- int ret;
-
- /* Fixed links and 802.3z are handled without needing a PHY */
- if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
- (pl->cfg_link_an_mode == MLO_AN_INBAND &&
- phy_interface_mode_is_8023z(pl->link_interface)))
- return 0;
-
- phy_node = of_parse_phandle(dn, "phy-handle", 0);
- if (!phy_node)
- phy_node = of_parse_phandle(dn, "phy", 0);
- if (!phy_node)
- phy_node = of_parse_phandle(dn, "phy-device", 0);
-
- if (!phy_node) {
- if (pl->cfg_link_an_mode == MLO_AN_PHY)
- return -ENODEV;
- return 0;
- }
-
- phy_dev = of_phy_find_device(phy_node);
- /* We're done with the phy_node handle */
- of_node_put(phy_node);
- if (!phy_dev)
- return -ENODEV;
-
- ret = phy_attach_direct(pl->netdev, phy_dev, flags,
- pl->link_interface);
- if (ret)
- return ret;
-
- ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
- if (ret)
- phy_detach(phy_dev);
-
- return ret;
+ return phylink_fwnode_phy_connect(pl, of_fwnode_handle(dn), flags);
}
EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
--
2.17.1
Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/phy/phylink.c | 54 +++++++++++++++++++++++++++++++++++++++
include/linux/phylink.h | 3 +++
2 files changed, 57 insertions(+)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 84f6e197f965..389dc3ec165e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -5,6 +5,7 @@
*
* Copyright (C) 2015 Russell King
*/
+#include <linux/acpi.h>
#include <linux/ethtool.h>
#include <linux/export.h>
#include <linux/gpio/consumer.h>
@@ -1120,6 +1121,59 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
}
EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
+/**
+ * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode to the phylink instance specified
+ * by @pl.
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int phylink_fwnode_phy_connect(struct phylink *pl,
+ struct fwnode_handle *fwnode,
+ u32 flags)
+{
+ struct fwnode_handle *phy_fwnode;
+ struct phy_device *phy_dev;
+ int ret;
+
+ if (is_of_node(fwnode)) {
+ /* Fixed links and 802.3z are handled without needing a PHY */
+ if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
+ (pl->cfg_link_an_mode == MLO_AN_INBAND &&
+ phy_interface_mode_is_8023z(pl->link_interface)))
+ return 0;
+ }
+
+ phy_fwnode = fwnode_get_phy_node(fwnode);
+ if (IS_ERR(phy_fwnode)) {
+ if (pl->cfg_link_an_mode == MLO_AN_PHY)
+ return -ENODEV;
+ return 0;
+ }
+
+ phy_dev = fwnode_phy_find_device(phy_fwnode);
+ /* We're done with the phy_node handle */
+ fwnode_handle_put(phy_fwnode);
+ if (!phy_dev)
+ return -ENODEV;
+
+ ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+ pl->link_interface);
+ if (ret)
+ return ret;
+
+ ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
+ if (ret)
+ phy_detach(phy_dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
+
/**
* phylink_disconnect_phy() - disconnect any PHY attached to the phylink
* instance.
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index d81a714cfbbd..75d4f99090fd 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -439,6 +439,9 @@ void phylink_destroy(struct phylink *);
int phylink_connect_phy(struct phylink *, struct phy_device *);
int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+ struct fwnode_handle *fwnode,
+ u32 flags);
void phylink_disconnect_phy(struct phylink *);
void phylink_mac_change(struct phylink *, bool up);
--
2.17.1
Define fwnode_mdio_find_device() to get a pointer to the
mdio_device from fwnode passed to the function.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/mdio/of_mdio.c | 11 +----------
drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
include/linux/phy.h | 6 ++++++
3 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index fa914d285271..1b561849269e 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -300,16 +300,7 @@ EXPORT_SYMBOL(of_mdiobus_register);
*/
struct mdio_device *of_mdio_find_device(struct device_node *np)
{
- struct device *d;
-
- if (!np)
- return NULL;
-
- d = bus_find_device_by_of_node(&mdio_bus_type, np);
- if (!d)
- return NULL;
-
- return to_mdio_device(d);
+ return fwnode_mdio_find_device(of_fwnode_handle(np));
}
EXPORT_SYMBOL(of_mdio_find_device);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6fad89c02c5a..17d20e6d5416 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2851,6 +2851,29 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->handle_interrupt;
}
+/**
+ * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
+ * @np: pointer to the mdio_device's fwnode
+ *
+ * If successful, returns a pointer to the mdio_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ * The caller should call put_device() on the mdio_device after its use
+ */
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
+{
+ struct device *d;
+
+ if (!fwnode)
+ return NULL;
+
+ d = bus_find_device_by_fwnode(&mdio_bus_type, fwnode);
+ if (!d)
+ return NULL;
+
+ return to_mdio_device(d);
+}
+EXPORT_SYMBOL(fwnode_mdio_find_device);
+
/**
* fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
* phy_fwnode.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 67ea4ca6f76f..5a0c290ff0f4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1343,6 +1343,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
struct phy_c45_device_ids *c45_ids);
#if IS_ENABLED(CONFIG_PHYLIB)
int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct phy_device *device_phy_find_device(struct device *dev);
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1355,6 +1356,11 @@ static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
return 0;
}
static inline
+struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)
+{
+ return 0;
+}
+static inline
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
{
return NULL;
--
2.17.1
Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
If the fwnode is DT node, then call of_mdiobus_register().
If it is an ACPI node, then:
- disable auto probing of mdiobus
- register mdiobus
- save fwnode to mdio structure
- loop over child nodes & register a phy_device for each PHY
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/phy/mdio_bus.c | 50 ++++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 1 +
2 files changed, 51 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 3361a1a86e97..e7ad34908936 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -8,6 +8,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/acpi.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/errno.h>
@@ -567,6 +568,55 @@ static int mdiobus_create_device(struct mii_bus *bus,
return ret;
}
+/**
+ * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode of MDIO bus.
+ *
+ * This function registers the mii_bus structure and registers a phy_device
+ * for each child node of @fwnode.
+ */
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *child;
+ unsigned long long addr;
+ acpi_status status;
+ int ret;
+
+ if (is_of_node(fwnode)) {
+ return of_mdiobus_register(mdio, to_of_node(fwnode));
+ } else if (is_acpi_node(fwnode)) {
+ /* Mask out all PHYs from auto probing. */
+ mdio->phy_mask = ~0;
+ ret = mdiobus_register(mdio);
+ if (ret)
+ return ret;
+
+ mdio->dev.fwnode = fwnode;
+ /* Loop over the child nodes and register a phy_device for each PHY */
+ fwnode_for_each_child_node(fwnode, child) {
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
+ "_ADR", NULL, &addr);
+ if (ACPI_FAILURE(status)) {
+ pr_debug("_ADR returned %d\n", status);
+ continue;
+ }
+
+ if (addr < 0 || addr >= PHY_MAX_ADDR)
+ continue;
+
+ ret = fwnode_mdiobus_register_phy(mdio, child, addr);
+ if (ret == -ENODEV)
+ dev_err(&mdio->dev,
+ "MDIO device at address %lld is missing.\n",
+ addr);
+ }
+ return 0;
+ }
+ return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register);
+
/**
* __mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
* @bus: target mii_bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 10a66b65a008..67ea4ca6f76f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -383,6 +383,7 @@ static inline struct mii_bus *mdiobus_alloc(void)
return mdiobus_alloc_size(0);
}
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
int __mdiobus_register(struct mii_bus *bus, struct module *owner);
int __devm_mdiobus_register(struct device *dev, struct mii_bus *bus,
struct module *owner);
--
2.17.1
Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/phy/mdio_bus.c | 66 ++++++++++++++++++++++++++++++++++++++
include/linux/mdio.h | 2 ++
2 files changed, 68 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2b42e46066b4..3361a1a86e97 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -106,6 +106,72 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
}
EXPORT_SYMBOL(mdiobus_unregister_device);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct mii_timestamper *mii_ts;
+ struct phy_device *phy;
+ const char *cp;
+ bool is_c45;
+ u32 phy_id;
+ int rc;
+
+ if (is_of_node(child)) {
+ mii_ts = of_find_mii_timestamper(to_of_node(child));
+ if (IS_ERR(mii_ts))
+ return PTR_ERR(mii_ts);
+ }
+
+ rc = fwnode_property_read_string(child, "compatible", &cp);
+ is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
+
+ if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+ phy = get_phy_device(bus, addr, is_c45);
+ else
+ phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+ if (IS_ERR(phy)) {
+ if (mii_ts && is_of_node(child))
+ unregister_mii_timestamper(mii_ts);
+ return PTR_ERR(phy);
+ }
+
+ if (is_acpi_node(child)) {
+ phy->irq = bus->irq[addr];
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ phy->mdio.dev.fwnode = child;
+
+ /* All data is now stored in the phy struct, so register it */
+ rc = phy_device_register(phy);
+ if (rc) {
+ phy_device_free(phy);
+ fwnode_handle_put(phy->mdio.dev.fwnode);
+ return rc;
+ }
+
+ dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+ } else if (is_of_node(child)) {
+ rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
+ if (rc) {
+ if (mii_ts)
+ unregister_mii_timestamper(mii_ts);
+ phy_device_free(phy);
+ return rc;
+ }
+
+ /* phy->mii_ts may already be defined by the PHY driver. A
+ * mii_timestamper probed via the device tree will still have
+ * precedence.
+ */
+ if (mii_ts)
+ phy->mii_ts = mii_ts;
+ }
+ return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
{
struct mdio_device *mdiodev = bus->mdio_map[addr];
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index dbd69b3d170b..f138ec333725 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -368,6 +368,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
int mdiobus_unregister_device(struct mdio_device *mdiodev);
bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr);
/**
* mdio_module_driver() - Helper macro for registering mdio drivers
--
2.17.1
Refactor of_phy_find_device() to use fwnode_phy_find_device().
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/mdio/of_mdio.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 4daf94bb56a5..fde76bee78b3 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -369,18 +369,7 @@ EXPORT_SYMBOL(of_mdio_find_device);
*/
struct phy_device *of_phy_find_device(struct device_node *phy_np)
{
- struct mdio_device *mdiodev;
-
- mdiodev = of_mdio_find_device(phy_np);
- if (!mdiodev)
- return NULL;
-
- if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
- return to_phy_device(&mdiodev->dev);
-
- put_device(&mdiodev->dev);
-
- return NULL;
+ return fwnode_phy_find_device(of_fwnode_handle(phy_np));
}
EXPORT_SYMBOL(of_phy_find_device);
--
2.17.1
fwnode_mdiobus_register() internally takes care of both DT
and ACPI cases to register mdiobus. Replace existing
of_mdiobus_register() with fwnode_mdiobus_register().
Note: For both ACPI and DT cases, endianness of MDIO controller
need to be specified using "little-endian" property.
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/ethernet/freescale/xgmac_mdio.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index bfa2826c5545..ae2593abac96 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -2,6 +2,7 @@
* QorIQ 10G MDIO Controller
*
* Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2020 NXP
*
* Authors: Andy Fleming <[email protected]>
* Timur Tabi <[email protected]>
@@ -11,6 +12,7 @@
* kind, whether express or implied.
*/
+#include <linux/acpi.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/interrupt.h>
@@ -243,10 +245,9 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
static int xgmac_mdio_probe(struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
- struct mii_bus *bus;
- struct resource *res;
struct mdio_fsl_priv *priv;
+ struct resource *res;
+ struct mii_bus *bus;
int ret;
/* In DPAA-1, MDIO is one of the many FMan sub-devices. The FMan
@@ -279,13 +280,14 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
goto err_ioremap;
}
+ /* For both ACPI and DT cases, endianness of MDIO controller
+ * need to be specified using "little-endian" property.
+ */
priv->is_little_endian = device_property_read_bool(&pdev->dev,
"little-endian");
-
priv->has_a011043 = device_property_read_bool(&pdev->dev,
"fsl,erratum-a011043");
-
- ret = of_mdiobus_register(bus, np);
+ ret = fwnode_mdiobus_register(bus, pdev->dev.fwnode);
if (ret) {
dev_err(&pdev->dev, "cannot register MDIO bus\n");
goto err_registration;
--
2.17.1
Refactor of_mdiobus_register_phy() to use fwnode_mdiobus_register_phy().
Signed-off-by: Calvin Johnson <[email protected]>
---
Changes in v2: None
drivers/net/mdio/of_mdio.c | 43 +++-----------------------------------
include/linux/of_mdio.h | 6 +++++-
2 files changed, 8 insertions(+), 41 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 31e6435dcc9f..fa914d285271 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -32,7 +32,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
}
-static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
+struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
{
struct of_phandle_args arg;
int err;
@@ -49,6 +49,7 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
return register_mii_timestamper(arg.np, arg.args[0]);
}
+EXPORT_SYMBOL(of_find_mii_timestamper);
int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
struct device_node *child, u32 addr)
@@ -97,45 +98,7 @@ EXPORT_SYMBOL(of_mdiobus_phy_device_register);
static int of_mdiobus_register_phy(struct mii_bus *mdio,
struct device_node *child, u32 addr)
{
- struct mii_timestamper *mii_ts;
- struct phy_device *phy;
- bool is_c45;
- int rc;
- u32 phy_id;
-
- mii_ts = of_find_mii_timestamper(child);
- if (IS_ERR(mii_ts))
- return PTR_ERR(mii_ts);
-
- is_c45 = of_device_is_compatible(child,
- "ethernet-phy-ieee802.3-c45");
-
- if (!is_c45 && !of_get_phy_id(child, &phy_id))
- phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
- else
- phy = get_phy_device(mdio, addr, is_c45);
- if (IS_ERR(phy)) {
- if (mii_ts)
- unregister_mii_timestamper(mii_ts);
- return PTR_ERR(phy);
- }
-
- rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
- if (rc) {
- if (mii_ts)
- unregister_mii_timestamper(mii_ts);
- phy_device_free(phy);
- return rc;
- }
-
- /* phy->mii_ts may already be defined by the PHY driver. A
- * mii_timestamper probed via the device tree will still have
- * precedence.
- */
- if (mii_ts)
- phy->mii_ts = mii_ts;
-
- return 0;
+ return fwnode_mdiobus_register_phy(mdio, of_fwnode_handle(child), addr);
}
static int of_mdiobus_register_device(struct mii_bus *mdio,
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index cfe8c607a628..3b66016f18aa 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -34,6 +34,7 @@ struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
int of_phy_register_fixed_link(struct device_node *np);
void of_phy_deregister_fixed_link(struct device_node *np);
bool of_phy_is_fixed_link(struct device_node *np);
+struct mii_timestamper *of_find_mii_timestamper(struct device_node *np);
int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
struct device_node *child, u32 addr);
@@ -128,7 +129,10 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
{
return false;
}
-
+static inline struct mii_timestamper *of_find_mii_timestamper(struct device_node *np)
+{
+ return NULL;
+}
static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
struct phy_device *phy,
struct device_node *child, u32 addr)
--
2.17.1
Hi Calvin,
Thank you for the patch.
On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> Using fwnode_get_id(), get the reg property value for DT node
> and get the _ADR object value for ACPI node.
>
> Signed-off-by: Calvin Johnson <[email protected]>
> ---
>
> Changes in v2: None
>
> drivers/base/property.c | 26 ++++++++++++++++++++++++++
> include/linux/property.h | 1 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 4c43d30145c6..1c50e17ae879 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> return fwnode_call_ptr_op(fwnode, get_name_prefix);
> }
>
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
Is the concept of fwnode ID documented clearly somewhere ? I think this
function should otherwise have more documentation, at least to explain
what the ID is.
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> + unsigned long long adr;
> + acpi_status status;
> +
> + if (is_of_node(fwnode)) {
> + return of_property_read_u32(to_of_node(fwnode), "reg", id);
> + } else if (is_acpi_node(fwnode)) {
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> + METHOD_NAME__ADR, NULL, &adr);
> + if (ACPI_FAILURE(status))
> + return -ENODATA;
Would it make sense to standardize error codes ? of_property_read_u32()
can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
this function would be very interested to tell those three cases apart.
Maybe we should return -EINVAL in all error cases ? Or maybe different
error codes to mean "the backend doesn't support the concept of IDs",
and "the device doesn't have an ID" ?
> + *id = (u32)adr;
> + return 0;
> + }
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_id);
> +
> /**
> * fwnode_get_parent - Return parent firwmare node
> * @fwnode: Firmware whose parent is retrieved
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 2d4542629d80..92d405cf2b07 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
>
> const char *fwnode_get_name(const struct fwnode_handle *fwnode);
> const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id);
> struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
> struct fwnode_handle *fwnode_get_next_parent(
> struct fwnode_handle *fwnode);
--
Regards,
Laurent Pinchart
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<[email protected]> wrote:
>
> Define fwnode_phy_find_device() to iterate an mdiobus and find the
> phy device of the provided phy fwnode. Additionally define
> device_phy_find_device() to find phy device of provided device.
>
> Define fwnode_get_phy_node() to get phy_node using named reference.
...
> +#include <linux/acpi.h>
Not sure we need this. See below.
...
> +/**
> + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> + * phy_fwnode.
Can we keep a summary on one line?
> + * @phy_fwnode: Pointer to the phy's fwnode.
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.
> + */
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> + struct mdio_device *mdiodev;
> + struct device *d;
> + if (!phy_fwnode)
> + return NULL;
Why is this needed?
Perhaps a comment to the function description explains a case when
@phy_fwnode == NULL.
> + d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> + if (d) {
> + mdiodev = to_mdio_device(d);
> + if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> + return to_phy_device(d);
> + put_device(d);
> + }
> +
> + return NULL;
> +}
...
> + * For ACPI, only "phy-handle" is supported. DT supports all the three
> + * named references to the phy node.
...
> + /* Only phy-handle is used for ACPI */
> + phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> + if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> + return phy_node;
So, what is the problem with going through the rest on ACPI?
Usually we describe the restrictions in the documentation.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<[email protected]> wrote:
>
> Extract phy_id from compatible string. This will be used by
> fwnode_mdiobus_register_phy() to create phy device using the
> phy_id.
...
> + if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> + *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> + return 0;
> + }
> + return -EINVAL;
Perhaps traditional pattern, i.e.
if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) != 2)
return -EINVAL;
*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
return 0;
And perhaps GENMASK() ?
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<[email protected]> wrote:
>
> Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> mdiobus. From the compatible string, identify whether the PHY is
> c45 and based on this create a PHY device instance which is
> registered on the mdiobus.
...
> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> + struct fwnode_handle *child, u32 addr)
> +{
> + struct mii_timestamper *mii_ts;
> + struct phy_device *phy;
> + const char *cp;
> + bool is_c45;
> + u32 phy_id;
> + int rc;
> + if (is_of_node(child)) {
> + mii_ts = of_find_mii_timestamper(to_of_node(child));
> + if (IS_ERR(mii_ts))
> + return PTR_ERR(mii_ts);
> + }
Perhaps
mii_ts = of_find_mii_timestamper(to_of_node(child));
> +
> + rc = fwnode_property_read_string(child, "compatible", &cp);
> + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
> +
> + if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> + phy = get_phy_device(bus, addr, is_c45);
> + else
> + phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + if (IS_ERR(phy)) {
> + if (mii_ts && is_of_node(child))
> + unregister_mii_timestamper(mii_ts);
if (!IS_ERR_OR_NULL(mii_ts))
...
However it points to the question why unregister() doesn't handle the
above cases.
I would expect unconditional call to unregister() here.
> + return PTR_ERR(phy);
> + }
> +
> + if (is_acpi_node(child)) {
> + phy->irq = bus->irq[addr];
> +
> + /* Associate the fwnode with the device structure so it
> + * can be looked up later.
> + */
> + phy->mdio.dev.fwnode = child;
> +
> + /* All data is now stored in the phy struct, so register it */
> + rc = phy_device_register(phy);
> + if (rc) {
> + phy_device_free(phy);
> + fwnode_handle_put(phy->mdio.dev.fwnode);
> + return rc;
> + }
> +
> + dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> + } else if (is_of_node(child)) {
> + rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
> + if (rc) {
> + if (mii_ts)
> + unregister_mii_timestamper(mii_ts);
Ditto.
> + phy_device_free(phy);
> + return rc;
> + }
> +
> + /* phy->mii_ts may already be defined by the PHY driver. A
> + * mii_timestamper probed via the device tree will still have
> + * precedence.
> + */
> + if (mii_ts)
> + phy->mii_ts = mii_ts;
How is that defined? Do you need to do something with an old pointer?
> + }
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<[email protected]> wrote:
>
> Using fwnode_get_id(), get the reg property value for DT node
> and get the _ADR object value for ACPI node.
and -> or
...
> +/**
> + * fwnode_get_id - Get the id of a fwnode.
> + * @fwnode: firmware node
> + * @id: id of the fwnode
> + *
> + * Returns 0 on success or a negative errno.
> + */
> +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> +{
> + unsigned long long adr;
> + acpi_status status;
> +
> + if (is_of_node(fwnode)) {
> + return of_property_read_u32(to_of_node(fwnode), "reg", id);
ACPI nodes can hold reg property as well. I would rather think about
ret = fwnode_property_read_u32(fwnode, "reg", id)
if (!(ret && is_acpi_node(fwnode)))
return ret;
> + } else if (is_acpi_node(fwnode)) {
Redundant 'else'
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> + METHOD_NAME__ADR, NULL, &adr);
> + if (ACPI_FAILURE(status))
> + return -ENODATA;
I'm wondering if it compiles when CONFIG_ACPI=n.
> + *id = (u32)adr;
> + return 0;
> + }
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 7:00 PM Laurent Pinchart
<[email protected]> wrote:
> On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.
...
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
>
> Is the concept of fwnode ID documented clearly somewhere ? I think this
> function should otherwise have more documentation, at least to explain
> what the ID is.
I'm afraid that OF has no clear concept of this either. It's usually
used as a unique ID for the children of some device (like MFD) and can
represent a lot of things. But I agree it should be documented.
Rob, any ideas about this?
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > + unsigned long long adr;
> > + acpi_status status;
> > +
> > + if (is_of_node(fwnode)) {
> > + return of_property_read_u32(to_of_node(fwnode), "reg", id);
> > + } else if (is_acpi_node(fwnode)) {
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > + METHOD_NAME__ADR, NULL, &adr);
> > + if (ACPI_FAILURE(status))
> > + return -ENODATA;
>
> Would it make sense to standardize error codes ? of_property_read_u32()
> can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
> this function would be very interested to tell those three cases apart.
> Maybe we should return -EINVAL in all error cases ? Or maybe different
> error codes to mean "the backend doesn't support the concept of IDs",
> and "the device doesn't have an ID" ?
We may actually get mapping to all three if first we check for the
method/name existence followed by value check.
But I don't think we need to bloat this simple one.
> > + *id = (u32)adr;
> > + return 0;
> > + }
> > + return -EINVAL;
> > +}
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<[email protected]> wrote:
>
> Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
> If the fwnode is DT node, then call of_mdiobus_register().
> If it is an ACPI node, then:
> - disable auto probing of mdiobus
> - register mdiobus
> - save fwnode to mdio structure
> - loop over child nodes & register a phy_device for each PHY
...
> +/**
> + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> + * @mdio: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode of MDIO bus.
> + *
> + * This function registers the mii_bus structure and registers a phy_device
> + * for each child node of @fwnode.
> + */
> +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> +{
> + struct fwnode_handle *child;
> + unsigned long long addr;
> + acpi_status status;
> + int ret;
> +
> + if (is_of_node(fwnode)) {
> + return of_mdiobus_register(mdio, to_of_node(fwnode));
> + } else if (is_acpi_node(fwnode)) {
I would rather see this as simple as
if (is_of_node(fwnode))
return of_mdiobus_register(mdio, to_of_node(fwnode));
if (is_acpi_node(fwnode))
return acpi_mdiobus_register(mdio, fwnode);
where the latter one is defined somewhere in drivers/acpi/.
> + /* Mask out all PHYs from auto probing. */
> + mdio->phy_mask = ~0;
> + ret = mdiobus_register(mdio);
> + if (ret)
> + return ret;
> +
> + mdio->dev.fwnode = fwnode;
> + /* Loop over the child nodes and register a phy_device for each PHY */
> + fwnode_for_each_child_node(fwnode, child) {
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> + "_ADR", NULL, &addr);
> + if (ACPI_FAILURE(status)) {
Isn't it fwnode_get_id() now?
> + pr_debug("_ADR returned %d\n", status);
> + continue;
> + }
> + if (addr < 0 || addr >= PHY_MAX_ADDR)
> + continue;
addr can't be less than 0.
> + ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> + if (ret == -ENODEV)
> + dev_err(&mdio->dev,
> + "MDIO device at address %lld is missing.\n",
> + addr);
> + }
> + return 0;
> + }
> + return -EINVAL;
> +}
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
<[email protected]> wrote:
>
> fwnode_mdiobus_register() internally takes care of both DT
> and ACPI cases to register mdiobus. Replace existing
> of_mdiobus_register() with fwnode_mdiobus_register().
>
> Note: For both ACPI and DT cases, endianness of MDIO controller
> need to be specified using "little-endian" property.
...
> @@ -2,6 +2,7 @@
> * QorIQ 10G MDIO Controller
> *
> * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2020 NXP
> *
> * Authors: Andy Fleming <[email protected]>
> * Timur Tabi <[email protected]>
> @@ -11,6 +12,7 @@
I guess this...
> priv->is_little_endian = device_property_read_bool(&pdev->dev,
> "little-endian");
> -
> priv->has_a011043 = device_property_read_bool(&pdev->dev,
> "fsl,erratum-a011043");
...this...
> -
...and this changes can go to a separate patch.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 07:23:26PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Define fwnode_phy_find_device() to iterate an mdiobus and find the
> > phy device of the provided phy fwnode. Additionally define
> > device_phy_find_device() to find phy device of provided device.
> >
> > Define fwnode_get_phy_node() to get phy_node using named reference.
>
> ...
>
> > +#include <linux/acpi.h>
>
> Not sure we need this. See below.
This is required to use is_acpi_node().
>
> ...
>
> > +/**
> > + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> > + * phy_fwnode.
>
> Can we keep a summary on one line?
Ok
>
> > + * @phy_fwnode: Pointer to the phy's fwnode.
> > + *
> > + * If successful, returns a pointer to the phy_device with the embedded
> > + * struct device refcount incremented by one, or NULL on failure.
> > + */
> > +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> > +{
> > + struct mdio_device *mdiodev;
> > + struct device *d;
>
> > + if (!phy_fwnode)
> > + return NULL;
>
> Why is this needed?
> Perhaps a comment to the function description explains a case when
> @phy_fwnode == NULL.
I think this function should be modified to follow of_phy_find_device() which
has NULL check. I'll add fwnode_mdio_find_device() also.
Here is a case where of_phy_find_device() is called without checking phy_np.
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-phy.c#L145
>
> > + d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> > + if (d) {
> > + mdiodev = to_mdio_device(d);
> > + if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> > + return to_phy_device(d);
> > + put_device(d);
> > + }
> > +
> > + return NULL;
> > +}
>
> ...
>
> > + * For ACPI, only "phy-handle" is supported. DT supports all the three
> > + * named references to the phy node.
>
> ...
>
> > + /* Only phy-handle is used for ACPI */
> > + phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
> > + if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
> > + return phy_node;
>
> So, what is the problem with going through the rest on ACPI?
> Usually we describe the restrictions in the documentation.
Others are legacy DT properties which are not intended to be supported
in ACPI. I can add this info in the document.
Thanks for the review, Andy!
Regards
Calvin
On Tue, Dec 15, 2020 at 07:28:10PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Extract phy_id from compatible string. This will be used by
> > fwnode_mdiobus_register_phy() to create phy device using the
> > phy_id.
>
> ...
>
> > + if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> > + *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> > + return 0;
> > + }
> > + return -EINVAL;
>
> Perhaps traditional pattern, i.e.
> if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) != 2)
> return -EINVAL;
>
> *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> return 0;
>
> And perhaps GENMASK() ?
Sure. Will rewrite accordingly.
Thanks
Calvin
On Thu, Dec 17, 2020 at 10:28 AM Calvin Johnson
<[email protected]> wrote:
> On Tue, Dec 15, 2020 at 07:28:10PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > <[email protected]> wrote:
...
> > > + if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> > *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> > And perhaps GENMASK() ?
>
> Sure. Will rewrite accordingly.
Reading this again I'm now not sure these masks are needed at all.
--
With Best Regards,
Andy Shevchenko
On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> > mdiobus. From the compatible string, identify whether the PHY is
> > c45 and based on this create a PHY device instance which is
> > registered on the mdiobus.
>
> ...
>
> > +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > + struct fwnode_handle *child, u32 addr)
> > +{
> > + struct mii_timestamper *mii_ts;
> > + struct phy_device *phy;
> > + const char *cp;
> > + bool is_c45;
> > + u32 phy_id;
> > + int rc;
>
> > + if (is_of_node(child)) {
> > + mii_ts = of_find_mii_timestamper(to_of_node(child));
> > + if (IS_ERR(mii_ts))
> > + return PTR_ERR(mii_ts);
> > + }
>
> Perhaps
>
> mii_ts = of_find_mii_timestamper(to_of_node(child));
>
> > +
> > + rc = fwnode_property_read_string(child, "compatible", &cp);
> > + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45"));
> > +
> > + if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> > + phy = get_phy_device(bus, addr, is_c45);
> > + else
> > + phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> > + if (IS_ERR(phy)) {
>
> > + if (mii_ts && is_of_node(child))
> > + unregister_mii_timestamper(mii_ts);
>
> if (!IS_ERR_OR_NULL(mii_ts))
> ...
>
> However it points to the question why unregister() doesn't handle the
> above cases.
> I would expect unconditional call to unregister() here.
This is following the logic defined in of_mdiobus_register_phy().
https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/of_mdio.c#L107
mii_ts = of_find_mii_timestamper(child);
if (IS_ERR(mii_ts))
return PTR_ERR(mii_ts);
I think the logic above is correct because this function returns only if
of_find_mii_timestamper() returns error. On NULL return, it proceeds further.
>
> > + return PTR_ERR(phy);
> > + }
> > +
> > + if (is_acpi_node(child)) {
> > + phy->irq = bus->irq[addr];
> > +
> > + /* Associate the fwnode with the device structure so it
> > + * can be looked up later.
> > + */
> > + phy->mdio.dev.fwnode = child;
> > +
> > + /* All data is now stored in the phy struct, so register it */
> > + rc = phy_device_register(phy);
> > + if (rc) {
> > + phy_device_free(phy);
> > + fwnode_handle_put(phy->mdio.dev.fwnode);
> > + return rc;
> > + }
> > +
> > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> > + } else if (is_of_node(child)) {
> > + rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr);
> > + if (rc) {
>
> > + if (mii_ts)
> > + unregister_mii_timestamper(mii_ts);
>
> Ditto.
>
> > + phy_device_free(phy);
> > + return rc;
> > + }
> > +
> > + /* phy->mii_ts may already be defined by the PHY driver. A
> > + * mii_timestamper probed via the device tree will still have
> > + * precedence.
> > + */
>
> > + if (mii_ts)
> > + phy->mii_ts = mii_ts;
>
> How is that defined? Do you need to do something with an old pointer?
As the comment says, I think PHY drivers which got invoked before calling
of_mdiobus_register_phy() may have defined phy->mii_ts.
>
> > + }
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
On Tue, Dec 15, 2020 at 07:53:26PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
> > If the fwnode is DT node, then call of_mdiobus_register().
> > If it is an ACPI node, then:
> > - disable auto probing of mdiobus
> > - register mdiobus
> > - save fwnode to mdio structure
> > - loop over child nodes & register a phy_device for each PHY
>
> ...
>
> > +/**
> > + * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
> > + * @mdio: pointer to mii_bus structure
> > + * @fwnode: pointer to fwnode of MDIO bus.
> > + *
> > + * This function registers the mii_bus structure and registers a phy_device
> > + * for each child node of @fwnode.
> > + */
> > +int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
> > +{
> > + struct fwnode_handle *child;
> > + unsigned long long addr;
> > + acpi_status status;
> > + int ret;
> > +
> > + if (is_of_node(fwnode)) {
> > + return of_mdiobus_register(mdio, to_of_node(fwnode));
> > + } else if (is_acpi_node(fwnode)) {
>
> I would rather see this as simple as
>
> if (is_of_node(fwnode))
> return of_mdiobus_register(mdio, to_of_node(fwnode));
> if (is_acpi_node(fwnode))
> return acpi_mdiobus_register(mdio, fwnode);
>
> where the latter one is defined somewhere in drivers/acpi/.
Makes sense. I'll do it. But I think it will be better to place
acpi_mdiobus_register() here itself in the network subsystem, maybe
/drivers/net/mdio/acpi_mdio.c.
>
> > + /* Mask out all PHYs from auto probing. */
> > + mdio->phy_mask = ~0;
> > + ret = mdiobus_register(mdio);
> > + if (ret)
> > + return ret;
> > +
> > + mdio->dev.fwnode = fwnode;
> > + /* Loop over the child nodes and register a phy_device for each PHY */
> > + fwnode_for_each_child_node(fwnode, child) {
>
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> > + "_ADR", NULL, &addr);
> > + if (ACPI_FAILURE(status)) {
>
> Isn't it fwnode_get_id() now?
Yes. Will change it.
>
> > + pr_debug("_ADR returned %d\n", status);
> > + continue;
> > + }
>
> > + if (addr < 0 || addr >= PHY_MAX_ADDR)
> > + continue;
>
> addr can't be less than 0.
Yes. will update in v3.
>
> > + ret = fwnode_mdiobus_register_phy(mdio, child, addr);
> > + if (ret == -ENODEV)
> > + dev_err(&mdio->dev,
> > + "MDIO device at address %lld is missing.\n",
> > + addr);
> > + }
> > + return 0;
> > + }
> > + return -EINVAL;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
On Tue, Dec 15, 2020 at 07:55:01PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > fwnode_mdiobus_register() internally takes care of both DT
> > and ACPI cases to register mdiobus. Replace existing
> > of_mdiobus_register() with fwnode_mdiobus_register().
> >
> > Note: For both ACPI and DT cases, endianness of MDIO controller
> > need to be specified using "little-endian" property.
>
> ...
>
> > @@ -2,6 +2,7 @@
> > * QorIQ 10G MDIO Controller
> > *
> > * Copyright 2012 Freescale Semiconductor, Inc.
> > + * Copyright 2020 NXP
> > *
> > * Authors: Andy Fleming <[email protected]>
> > * Timur Tabi <[email protected]>
> > @@ -11,6 +12,7 @@
>
> I guess this...
>
> > priv->is_little_endian = device_property_read_bool(&pdev->dev,
> > "little-endian");
> > -
> > priv->has_a011043 = device_property_read_bool(&pdev->dev,
> > "fsl,erratum-a011043");
>
> ...this...
>
> > -
>
> ...and this changes can go to a separate patch.
I think I'll remove the unnecessary cleanup.
Regards
Calvin
Hi Laurent,
Thanks for reviewing.
On Tue, Dec 15, 2020 at 07:00:28PM +0200, Laurent Pinchart wrote:
> Hi Calvin,
>
> Thank you for the patch.
>
> On Tue, Dec 15, 2020 at 10:13:11PM +0530, Calvin Johnson wrote:
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.
> >
> > Signed-off-by: Calvin Johnson <[email protected]>
> > ---
> >
> > Changes in v2: None
> >
> > drivers/base/property.c | 26 ++++++++++++++++++++++++++
> > include/linux/property.h | 1 +
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 4c43d30145c6..1c50e17ae879 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -580,6 +580,32 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
> > return fwnode_call_ptr_op(fwnode, get_name_prefix);
> > }
> >
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
>
> Is the concept of fwnode ID documented clearly somewhere ? I think this
> function should otherwise have more documentation, at least to explain
> what the ID is.
Agree. Will add more info here.
>
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > + unsigned long long adr;
> > + acpi_status status;
> > +
> > + if (is_of_node(fwnode)) {
> > + return of_property_read_u32(to_of_node(fwnode), "reg", id);
> > + } else if (is_acpi_node(fwnode)) {
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > + METHOD_NAME__ADR, NULL, &adr);
> > + if (ACPI_FAILURE(status))
> > + return -ENODATA;
>
> Would it make sense to standardize error codes ? of_property_read_u32()
> can return -EINVAL, -ENODATA or -EOVERFLOW. I don't think the caller of
> this function would be very interested to tell those three cases apart.
> Maybe we should return -EINVAL in all error cases ? Or maybe different
> error codes to mean "the backend doesn't support the concept of IDs",
> and "the device doesn't have an ID" ?
I think it make sense to return just -EINVAL. Will take care in v3.
Thanks
Calvin
On Tue, Dec 15, 2020 at 07:45:16PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > and get the _ADR object value for ACPI node.
>
> and -> or
>
> ...
>
> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > + unsigned long long adr;
> > + acpi_status status;
> > +
> > + if (is_of_node(fwnode)) {
> > + return of_property_read_u32(to_of_node(fwnode), "reg", id);
>
> ACPI nodes can hold reg property as well. I would rather think about
>
> ret = fwnode_property_read_u32(fwnode, "reg", id)
> if (!(ret && is_acpi_node(fwnode)))
> return ret;
>
Got it. Will rework on it.
> > + } else if (is_acpi_node(fwnode)) {
>
> Redundant 'else'
>
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > + METHOD_NAME__ADR, NULL, &adr);
> > + if (ACPI_FAILURE(status))
> > + return -ENODATA;
>
> I'm wondering if it compiles when CONFIG_ACPI=n.
Correct. It doesn't compile for non-ACPI case. Will resolve it.
Thanks
Calvin
On Fri, Dec 18, 2020 at 7:34 AM Calvin Johnson
<[email protected]> wrote:
>
> On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > <[email protected]> wrote:
...
> > > + /* phy->mii_ts may already be defined by the PHY driver. A
> > > + * mii_timestamper probed via the device tree will still have
> > > + * precedence.
> > > + */
> >
> > > + if (mii_ts)
> > > + phy->mii_ts = mii_ts;
> >
> > How is that defined? Do you need to do something with an old pointer?
>
> As the comment says, I think PHY drivers which got invoked before calling
> of_mdiobus_register_phy() may have defined phy->mii_ts.
What I meant here is that the old pointer might need some care (freeing?).
> > > + }
--
With Best Regards,
Andy Shevchenko
On Fri, Dec 18, 2020 at 7:40 AM Calvin Johnson
<[email protected]> wrote:
> On Tue, Dec 15, 2020 at 07:53:26PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson
> > <[email protected]> wrote:
...
> > I would rather see this as simple as
> >
> > if (is_of_node(fwnode))
> > return of_mdiobus_register(mdio, to_of_node(fwnode));
> > if (is_acpi_node(fwnode))
> > return acpi_mdiobus_register(mdio, fwnode);
> >
> > where the latter one is defined somewhere in drivers/acpi/.
> Makes sense. I'll do it. But I think it will be better to place
> acpi_mdiobus_register() here itself in the network subsystem, maybe
> /drivers/net/mdio/acpi_mdio.c.
Even better, thanks!
--
With Best Regards,
Andy Shevchenko