2020-12-15 16:49:06

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 00/14] ACPI support for dpaa2 driver


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


2020-12-15 16:49:12

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

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

2020-12-15 16:50:45

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

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

2020-12-15 16:51:00

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions

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

2020-12-15 16:51:06

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 14/14] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver

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

2020-12-15 16:51:57

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 05/14] of: mdio: Refactor of_get_phy_id()

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

2020-12-15 16:52:27

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 12/14] net: phylink: Refactor phylink_of_phy_connect()

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

2020-12-15 16:54:06

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 11/14] phylink: introduce phylink_fwnode_phy_connect()

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

2020-12-15 16:55:55

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 13/14] net: phy: Introduce fwnode_mdio_find_device()

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

2020-12-15 16:56:09

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

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

2020-12-15 16:57:19

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

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

2020-12-15 16:58:28

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 03/14] of: mdio: Refactor of_phy_find_device()

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

2020-12-15 16:58:56

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()

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

2020-12-15 17:01:27

by Calvin Johnson

[permalink] [raw]
Subject: [net-next PATCH v2 07/14] of: mdio: Refactor of_mdiobus_register_phy()

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

2020-12-15 17:06:43

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

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

2020-12-15 17:29:24

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions

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

2020-12-15 17:31:16

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

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

2020-12-15 17:37:45

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

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

2020-12-15 17:47:38

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

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

2020-12-15 17:51:52

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

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

2020-12-15 17:56:24

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

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

2020-12-15 17:58:54

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()

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

2020-12-17 07:34:07

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 02/14] net: phy: Introduce phy related fwnode functions

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

2020-12-17 08:32:34

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

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

2020-12-17 09:46:30

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 04/14] net: phy: Introduce fwnode_get_phy_id()

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

2020-12-18 05:40:52

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

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

2020-12-18 05:44:44

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

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

2020-12-18 05:50:09

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 09/14] net/fsl: Use fwnode_mdiobus_register()

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

2020-12-18 06:16:07

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

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

2020-12-18 06:16:47

by Calvin Johnson

[permalink] [raw]
Subject: Re: [net-next PATCH v2 10/14] device property: Introduce fwnode_get_id()

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

2020-12-18 15:38:09

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 06/14] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

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

2020-12-18 15:39:19

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [net-next PATCH v2 08/14] net: mdiobus: Introduce fwnode_mdiobus_register()

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