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_get_phy_id()
fwnode_phy_find_device()
device_phy_find_device()
fwnode_get_phy_node()
First one helps in connecting phy to phylink instance.
Next two 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.
Calvin Johnson (7):
Documentation: ACPI: DSD: Document MDIO PHY
net: phy: Introduce phy related fwnode functions
net: phy: Introduce fwnode_get_phy_id()
net: mdiobus: Introduce fwnode_mdiobus_register_phy()
phylink: introduce phylink_fwnode_phy_connect()
net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
net/fsl: Use _ADR ACPI object to register PHYs
Documentation/firmware-guide/acpi/dsd/phy.rst | 78 ++++++++++++++++++
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 +++++++++++-------
drivers/net/ethernet/freescale/xgmac_mdio.c | 48 ++++++++++-
drivers/net/phy/mdio_bus.c | 40 +++++++++
drivers/net/phy/phy_device.c | 82 +++++++++++++++++++
drivers/net/phy/phylink.c | 51 ++++++++++++
include/linux/mdio.h | 2 +
include/linux/phy.h | 25 ++++++
include/linux/phylink.h | 3 +
9 files changed, 375 insertions(+), 33 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
--
2.17.1
Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.
Describe properties "phy-handle" and "phy-mode".
Signed-off-by: Calvin Johnson <[email protected]>
---
Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
1 file changed, 78 insertions(+)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
new file mode 100644
index 000000000000..f10feb24ec1c
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,78 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+MDIO bus and PHYs in ACPI
+=========================
+
+The PHYs on an mdiobus are probed and registered using
+fwnode_mdiobus_register_phy().
+Later, for connecting these PHYs to MAC, the PHYs registered on the
+mdiobus have to be referenced.
+
+phy-handle
+-----------
+For each MAC node, a property "phy-handle" is used to reference the
+PHY that is registered on an MDIO bus.
+
+phy-mode
+--------
+Property "phy-mode" defines the type of PHY interface.
+
+An example of this is shown below::
+
+DSDT entry for MACs where PHY nodes are referenced
+--------------------------------------------------
+ Scope(\_SB.MCE0.PR17) // 1G
+ {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package (2) {"phy-mode", "rgmii-id"},
+ Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
+ }
+ })
+ }
+
+ Scope(\_SB.MCE0.PR18) // 1G
+ {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package (2) {"phy-mode", "rgmii-id"},
+ Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
+ }
+ })
+ }
+
+DSDT entry for MDIO node
+------------------------
+a) Silicon Component
+--------------------
+ Scope(_SB)
+ {
+ Device(MDI0) {
+ Name(_HID, "NXP0006")
+ Name(_CCA, 1)
+ Name(_UID, 0)
+ Name(_CRS, ResourceTemplate() {
+ Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
+ Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
+ {
+ MDI0_IT
+ }
+ }) // end of _CRS for MDI0
+ } // end of MDI0
+ }
+
+b) Platform Component
+---------------------
+ Scope(\_SB.MDI0)
+ {
+ Device(PHY1) {
+ Name (_ADR, 0x1)
+ } // end of PHY1
+
+ Device(PHY2) {
+ Name (_ADR, 0x2)
+ } // end of PHY2
+ }
--
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]>
---
drivers/net/phy/phy_device.c | 52 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 20 ++++++++++++++
2 files changed, 72 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..c4aec56d0a95 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2818,6 +2818,58 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
return phydrv->config_intr && phydrv->ack_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 device *d;
+ struct mdio_device *mdiodev;
+
+ 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().
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+ return fwnode_find_reference(fwnode, "phy-handle", 0);
+}
+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 eb3cb1a98b45..7b1bf3d46fd3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1378,10 +1378,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
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]>
---
drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
include/linux/phy.h | 5 +++++
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c4aec56d0a95..162abde6223d 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>
@@ -845,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
@@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
*/
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
{
- return fwnode_find_reference(fwnode, "phy-handle", 0);
+ struct fwnode_handle *phy_node;
+
+ 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);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7b1bf3d46fd3..b6814e04092f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1378,6 +1378,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);
@@ -1385,6 +1386,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
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]>
---
drivers/net/phy/mdio_bus.c | 40 ++++++++++++++++++++++++++++++++++++++
include/linux/mdio.h | 2 ++
2 files changed, 42 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 0af20faad69d..693eb752cbf7 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -106,6 +106,46 @@ 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 phy_device *phy;
+ bool is_c45;
+ const char *cp;
+ u32 phy_id;
+ int rc;
+
+ 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))
+ return PTR_ERR(phy);
+
+ 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);
+
+ 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
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]>
---
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 ++++++++++++-------
1 file changed, 50 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 90cd243070d7..18502ee83e46 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -3,6 +3,7 @@
#include "dpaa2-eth.h"
#include "dpaa2-mac.h"
+#include <linux/acpi.h>
#define phylink_to_dpaa2_mac(config) \
container_of((config), struct dpaa2_mac, phylink_config)
@@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
}
/* 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 acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct fwnode_handle *dpmacs, *dpmac = NULL;
+ unsigned long long adr;
+ acpi_status status;
int err;
+ u32 id;
- dpmacs = of_find_node_by_name(NULL, "dpmacs");
- if (!dpmacs)
- return NULL;
+ if (is_of_node(dev->parent->fwnode)) {
+ dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
+ if (!dpmacs)
+ return NULL;
+
+ while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
+ err = fwnode_property_read_u32(dpmac, "reg", &id);
+ if (err)
+ continue;
+ if (id == dpmac_id)
+ return dpmac;
+ }
- while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
- err = of_property_read_u32(dpmac, "reg", &id);
- if (err)
- continue;
- if (id == dpmac_id)
- break;
+ } else if (is_acpi_node(dev->parent->fwnode)) {
+ device_for_each_child_node(dev->parent, dpmac) {
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
+ "_ADR", NULL, &adr);
+ if (ACPI_FAILURE(status)) {
+ pr_debug("_ADR returned %d on %s\n",
+ status, (char *)buffer.pointer);
+ continue;
+ } else {
+ id = (u32)adr;
+ if (id == dpmac_id)
+ return dpmac;
+ }
+ }
}
-
- of_node_put(dpmacs);
-
- return dpmac;
+ 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)
@@ -303,7 +322,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;
@@ -323,7 +342,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;
@@ -341,7 +360,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)) {
@@ -352,7 +371,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
if (attr.link_type == DPMAC_LINK_TYPE_PHY &&
attr.eth_if != DPMAC_ETH_IF_RGMII) {
- err = dpaa2_pcs_create(mac, dpmac_node, attr.id);
+ err = dpaa2_pcs_create(mac, to_of_node(dpmac_node), attr.id);
if (err)
goto err_put_node;
}
@@ -361,7 +380,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);
@@ -372,13 +391,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))
+ of_node_put(to_of_node(dpmac_node));
return 0;
@@ -387,7 +407,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))
+ of_node_put(to_of_node(dpmac_node));
err_close_dpmac:
dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
return err;
--
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]>
---
drivers/net/phy/phylink.c | 51 +++++++++++++++++++++++++++++++++++++++
include/linux/phylink.h | 3 +++
2 files changed, 54 insertions(+)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fe2296fdda19..ca88bd90d605 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,56 @@ 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))
+ return phylink_of_phy_connect(pl, to_of_node(fwnode), flags);
+ if (is_acpi_device_node(fwnode)) {
+ 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);
+ 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;
+ }
+
+ return -EINVAL;
+}
+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
PHYs on an mdio bus has address which can be obtained from ACPI
DSDT table using the _ADR object.
DSDT Eg: PHYs connected to MDI0 bus.
-------------------------
Scope(\_SB.MDI0)
{
Device(PHY1) {
Name (_ADR, 0x1)
} // end of PHY1
Device(PHY2) {
Name (_ADR, 0x2)
} // end of PHY2
} // end of MDI0
-------------------------
Signed-off-by: Calvin Johnson <[email protected]>
---
drivers/net/ethernet/freescale/xgmac_mdio.c | 48 +++++++++++++++++++--
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 98be51d8b08c..fb272564855e 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>
@@ -248,6 +250,10 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
struct resource *res;
struct mdio_fsl_priv *priv;
int ret;
+ struct fwnode_handle *fwnode;
+ struct fwnode_handle *child;
+ unsigned long long addr;
+ acpi_status status;
/* In DPAA-1, MDIO is one of the many FMan sub-devices. The FMan
* defines a register space that spans a large area, covering all the
@@ -284,10 +290,44 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
priv->has_a011043 = device_property_read_bool(&pdev->dev,
"fsl,erratum-a011043");
-
- ret = of_mdiobus_register(bus, np);
- if (ret) {
- dev_err(&pdev->dev, "cannot register MDIO bus\n");
+ if (is_of_node(pdev->dev.fwnode)) {
+ ret = of_mdiobus_register(bus, np);
+ if (ret) {
+ dev_err(&pdev->dev, "cannot register MDIO bus\n");
+ goto err_registration;
+ }
+ } else if (is_acpi_node(pdev->dev.fwnode)) {
+ priv->is_little_endian = true;
+ /* Mask out all PHYs from auto probing. */
+ bus->phy_mask = ~0;
+ ret = mdiobus_register(bus);
+ if (ret) {
+ dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret);
+ return ret;
+ }
+
+ fwnode = pdev->dev.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(bus, child, addr);
+ if (ret == -ENODEV)
+ dev_err(&bus->dev,
+ "MDIO device at address %lld is missing.\n",
+ addr);
+ }
+ } else {
+ dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n");
+ ret = -ENXIO;
goto err_registration;
}
--
2.17.1
Hi Calvin
> priv->has_a011043 = device_property_read_bool(&pdev->dev,
> "fsl,erratum-a011043");
> -
> - ret = of_mdiobus_register(bus, np);
> - if (ret) {
> - dev_err(&pdev->dev, "cannot register MDIO bus\n");
> + if (is_of_node(pdev->dev.fwnode)) {
> + ret = of_mdiobus_register(bus, np);
> + if (ret) {
> + dev_err(&pdev->dev, "cannot register MDIO bus\n");
> + goto err_registration;
> + }
> + } else if (is_acpi_node(pdev->dev.fwnode)) {
> + priv->is_little_endian = true;
> + /* Mask out all PHYs from auto probing. */
> + bus->phy_mask = ~0;
> + ret = mdiobus_register(bus);
> + if (ret) {
> + dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret);
> + return ret;
> + }
> +
> + fwnode = pdev->dev.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(bus, child, addr);
> + if (ret == -ENODEV)
> + dev_err(&bus->dev,
> + "MDIO device at address %lld is missing.\n",
> + addr);
> + }
Hi Calvin
This looping over the properties should be in the core, in the same
way of_mdiobus_register() loops over the OF properties in the core.
We don't want MDIO drivers doing this in their own way, with their own
bugs.
Andrew
> +/* 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);
Hi Calvin
Do you really need this? Do you have a board with a broken PHY ID?
> /**
> * get_phy_device - reads the specified PHY device and returns its @phy_device
> * struct
> @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> */
> struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> {
> - return fwnode_find_reference(fwnode, "phy-handle", 0);
> + struct fwnode_handle *phy_node;
> +
> + 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;
Why do you have three different ways to reference a PHY?
Andrew
On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
<[email protected]> wrote:
>
> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> provide them to be connected to MAC.
>
> Describe properties "phy-handle" and "phy-mode".
>
> Signed-off-by: Calvin Johnson <[email protected]>
> ---
>
> Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
> 1 file changed, 78 insertions(+)
> create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
>
> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> new file mode 100644
> index 000000000000..f10feb24ec1c
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> @@ -0,0 +1,78 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +MDIO bus and PHYs in ACPI
> +=========================
> +
> +The PHYs on an mdiobus are probed and registered using
> +fwnode_mdiobus_register_phy().
> +Later, for connecting these PHYs to MAC, the PHYs registered on the
> +mdiobus have to be referenced.
> +
> +phy-handle
> +-----------
> +For each MAC node, a property "phy-handle" is used to reference the
> +PHY that is registered on an MDIO bus.
It is not clear what "a property" means in this context.
This should refer to the documents introducing the _DSD-based generic
device properties rules, including the GUID used below.
You need to say whether or not the property is mandatory and if it
isn't mandatory, you need to say what the lack of it means.
> +
> +phy-mode
> +--------
> +Property "phy-mode" defines the type of PHY interface.
This needs to be more detailed too, IMO. At the very least, please
list all of the possible values of it and document their meaning.
> +
> +An example of this is shown below::
> +
> +DSDT entry for MACs where PHY nodes are referenced
> +--------------------------------------------------
> + Scope(\_SB.MCE0.PR17) // 1G
> + {
> + Name (_DSD, Package () {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package () {
> + Package (2) {"phy-mode", "rgmii-id"},
> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
What is "phy-handle"?
You haven't introduced it above.
> + }
> + })
> + }
> +
> + Scope(\_SB.MCE0.PR18) // 1G
> + {
> + Name (_DSD, Package () {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package () {
> + Package (2) {"phy-mode", "rgmii-id"},
> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
> + }
> + })
> + }
> +
> +DSDT entry for MDIO node
> +------------------------
> +a) Silicon Component
What is this device, exactly?
> +--------------------
> + Scope(_SB)
> + {
> + Device(MDI0) {
> + Name(_HID, "NXP0006")
> + Name(_CCA, 1)
> + Name(_UID, 0)
> + Name(_CRS, ResourceTemplate() {
> + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
> + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> + {
> + MDI0_IT
> + }
> + }) // end of _CRS for MDI0
> + } // end of MDI0
> + }
> +
> +b) Platform Component
> +---------------------
> + Scope(\_SB.MDI0)
> + {
> + Device(PHY1) {
> + Name (_ADR, 0x1)
> + } // end of PHY1
> +
> + Device(PHY2) {
> + Name (_ADR, 0x2)
> + } // end of PHY2
> + }
> --
What is the connection between the last two pieces of ASL and the _DSD
definitions above?
On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
> > @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> > */
> > struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> > {
> > - return fwnode_find_reference(fwnode, "phy-handle", 0);
> > + struct fwnode_handle *phy_node;
> > +
> > + 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;
>
> Why do you have three different ways to reference a PHY?
Compatibility with the DT version - note that "phy" and "phy-device"
are only used for non-ACPI fwnodes. This should allow us to convert
drivers where necessary without fear of causing DT regressions.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
> > > @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> > > */
> > > struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> > > {
> > > - return fwnode_find_reference(fwnode, "phy-handle", 0);
> > > + struct fwnode_handle *phy_node;
> > > +
> > > + 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;
> >
> > Why do you have three different ways to reference a PHY?
>
> Compatibility with the DT version - note that "phy" and "phy-device"
> are only used for non-ACPI fwnodes. This should allow us to convert
> drivers where necessary without fear of causing DT regressions.
Ah.
A comment would be good here.
Andrew
From: Calvin Johnson <[email protected]>
Date: Wed, 30 Sep 2020 21:34:25 +0530
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> + struct device *d;
> + struct mdio_device *mdiodev;
Please use reverse christmas tree ordering for local variables.
From: Calvin Johnson <[email protected]>
Date: Wed, 30 Sep 2020 21:34:27 +0530
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -106,6 +106,46 @@ 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 phy_device *phy;
> + bool is_c45;
> + const char *cp;
> + u32 phy_id;
> + int rc;
Reverse christmas tree here please.
From: Calvin Johnson <[email protected]>
Date: Wed, 30 Sep 2020 21:34:30 +0530
> @@ -248,6 +250,10 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
> struct resource *res;
> struct mdio_fsl_priv *priv;
> int ret;
> + struct fwnode_handle *fwnode;
> + struct fwnode_handle *child;
> + unsigned long long addr;
> + acpi_status status;
>
Reverse chrstimas tree here too, please.
On Wed, Sep 30, 2020 at 03:03:49PM -0700, David Miller wrote:
> From: Calvin Johnson <[email protected]>
> Date: Wed, 30 Sep 2020 21:34:25 +0530
>
> > +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> > +{
> > + struct device *d;
> > + struct mdio_device *mdiodev;
>
> Please use reverse christmas tree ordering for local variables.
Sure. In next rev, I'll make sure all the patches follow this.
Thanks
Calvin
On Wed, Sep 30, 2020 at 08:19:02PM +0200, Andrew Lunn wrote:
> On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
> > > > @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> > > > */
> > > > struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> > > > {
> > > > - return fwnode_find_reference(fwnode, "phy-handle", 0);
> > > > + struct fwnode_handle *phy_node;
> > > > +
> > > > + 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;
> > >
> > > Why do you have three different ways to reference a PHY?
> >
> > Compatibility with the DT version - note that "phy" and "phy-device"
> > are only used for non-ACPI fwnodes. This should allow us to convert
> > drivers where necessary without fear of causing DT regressions.
>
> Ah.
>
> A comment would be good here.
Sure. Will add comment.
Thanks
Calvin
Hi Rafael,
On Wed, Sep 30, 2020 at 06:37:09PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> > provide them to be connected to MAC.
> >
> > Describe properties "phy-handle" and "phy-mode".
> >
> > Signed-off-by: Calvin Johnson <[email protected]>
> > ---
> >
> > Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> > create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
> >
> > diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> > new file mode 100644
> > index 000000000000..f10feb24ec1c
> > --- /dev/null
> > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> > @@ -0,0 +1,78 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=========================
> > +MDIO bus and PHYs in ACPI
> > +=========================
> > +
> > +The PHYs on an mdiobus are probed and registered using
> > +fwnode_mdiobus_register_phy().
> > +Later, for connecting these PHYs to MAC, the PHYs registered on the
> > +mdiobus have to be referenced.
> > +
> > +phy-handle
> > +-----------
> > +For each MAC node, a property "phy-handle" is used to reference the
> > +PHY that is registered on an MDIO bus.
>
> It is not clear what "a property" means in this context.
>
In rev-2, I'll add more info on this.
During the MDIO bus driver initialization, PHYs on this bus are probed
using the _ADR object as shown below and are registered on the mdio bus.
Scope(\_SB.MDI0)
{
Device(PHY1) {
Name (_ADR, 0x1)
} // end of PHY1
Device(PHY2) {
Name (_ADR, 0x2)
} // end of PHY2
}
Later, during the MAC driver initialization, the registered PHY devices
have to be retrieved from the mdio bus. For this, MAC driver needs reference
to the previously registered PHYs which are provided using reference to the
device as {\_SB.MDI0.PHY1}.
> This should refer to the documents introducing the _DSD-based generic
> device properties rules, including the GUID used below.
>
Sure. I'll refer "Documentation/firmware-guide/acpi/DSD-properties-rules.rst"
> You need to say whether or not the property is mandatory and if it
> isn't mandatory, you need to say what the lack of it means.
>
I'll do that.
> > +
> > +phy-mode
> > +--------
> > +Property "phy-mode" defines the type of PHY interface.
>
> This needs to be more detailed too, IMO. At the very least, please
> list all of the possible values of it and document their meaning.
>
> > +
> > +An example of this is shown below::
> > +
> > +DSDT entry for MACs where PHY nodes are referenced
> > +--------------------------------------------------
> > + Scope(\_SB.MCE0.PR17) // 1G
> > + {
> > + Name (_DSD, Package () {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package () {
> > + Package (2) {"phy-mode", "rgmii-id"},
> > + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
>
> What is "phy-handle"?
>
> You haven't introduced it above.
I thought I introduced it earlier in this document as a property. Ofcourse,
more info needs to be added as you mentioned. Other than that am I missing
something?
I've a correction here.
Based on referring some more documents, I'll be using
Package (2) {"phy-handle",\_SB.MDI0.PHY1}
instead of
Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
.
> > + }
> > + })
> > + }
> > +
> > + Scope(\_SB.MCE0.PR18) // 1G
> > + {
> > + Name (_DSD, Package () {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package () {
> > + Package (2) {"phy-mode", "rgmii-id"},
> > + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
> > + }
> > + })
> > + }
> > +
> > +DSDT entry for MDIO node
> > +------------------------
> > +a) Silicon Component
>
> What is this device, exactly?
I'll explain it more clearly.
>
> > +--------------------
> > + Scope(_SB)
> > + {
> > + Device(MDI0) {
> > + Name(_HID, "NXP0006")
> > + Name(_CCA, 1)
> > + Name(_UID, 0)
> > + Name(_CRS, ResourceTemplate() {
> > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
> > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
> > + {
> > + MDI0_IT
> > + }
> > + }) // end of _CRS for MDI0
> > + } // end of MDI0
> > + }
> > +
> > +b) Platform Component
> > +---------------------
> > + Scope(\_SB.MDI0)
> > + {
> > + Device(PHY1) {
> > + Name (_ADR, 0x1)
> > + } // end of PHY1
> > +
> > + Device(PHY2) {
> > + Name (_ADR, 0x2)
> > + } // end of PHY2
> > + }
> > --
>
> What is the connection between the last two pieces of ASL and the _DSD
> definitions above?
In rev-2, I'll explain the relation between these pieces. What I tried to show
is that the MDIO bus has an SoC component(mdio controller) and a platform
component(PHYs on the mdiobus).
In the MAC nodes, the PHYs are referenced and that is done using the "phy-handle"
property.
Thanks
Calvin
On Wed, Sep 30, 2020 at 7:06 PM Calvin Johnson
<[email protected]> wrote:
>
> 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.
...
> #include "dpaa2-eth.h"
> #include "dpaa2-mac.h"
> +#include <linux/acpi.h>
Please, put generic headers first.
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct fwnode_handle *dpmacs, *dpmac = NULL;
> + unsigned long long adr;
> + acpi_status status;
> int err;
> + u32 id;
>
> - dpmacs = of_find_node_by_name(NULL, "dpmacs");
> - if (!dpmacs)
> - return NULL;
> + if (is_of_node(dev->parent->fwnode)) {
> + dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> + if (!dpmacs)
> + return NULL;
> +
> + while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> + err = fwnode_property_read_u32(dpmac, "reg", &id);
> + if (err)
> + continue;
> + if (id == dpmac_id)
> + return dpmac;
> + }
>
> + } else if (is_acpi_node(dev->parent->fwnode)) {
> + device_for_each_child_node(dev->parent, dpmac) {
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> + "_ADR", NULL, &adr);
> + if (ACPI_FAILURE(status)) {
> + pr_debug("_ADR returned %d on %s\n",
> + status, (char *)buffer.pointer);
> + continue;
> + } else {
> + id = (u32)adr;
> + if (id == dpmac_id)
> + return dpmac;
> + }
> + }
Can you rather implement generic one which will be
int fwnode_get_child_id(struct fwnode_handle *fwnode, u64 *id);
and put the logic of retrieving 'reg' or _ADR? Also, for the latter we
have a special macro
METHOD_NAME__ADR.
See [1] as well. Same idea I have shared already.
[1]: https://lore.kernel.org/linux-iio/[email protected]/T/#m5f61921fa67a5b40522b7f7b17216e0d204647be
...
> - of_node_put(dpmac_node);
> + if (is_of_node(dpmac_node))
> + of_node_put(to_of_node(dpmac_node));
I'm not sure why you can't use fwnode_handle_put()?
> + if (is_of_node(dpmac_node))
> + of_node_put(to_of_node(dpmac_node));
Ditto.
--
With Best Regards,
Andy Shevchenko
On 01/10/2020 05:00, Calvin Johnson wrote:
> On Wed, Sep 30, 2020 at 08:19:02PM +0200, Andrew Lunn wrote:
>> On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
>>> On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
>>>>> @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
>>>>> */
>>>>> struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>>>>> {
>>>>> - return fwnode_find_reference(fwnode, "phy-handle", 0);
>>>>> + struct fwnode_handle *phy_node;
>>>>> +
>>>>> + 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;
>>>>
>>>> Why do you have three different ways to reference a PHY?
>>>
>>> Compatibility with the DT version - note that "phy" and "phy-device"
>>> are only used for non-ACPI fwnodes. This should allow us to convert
>>> drivers where necessary without fear of causing DT regressions.
>>
>> Ah.
>>
>> A comment would be good here.
>
> Sure. Will add comment.
Actually, I agree with Andrew. I don't see why new properties are being
defined for following a reference from a property to a PHY node. This
function shouldn't need to change at all.
g.
On 30/09/2020 17:04, Calvin Johnson wrote:
> 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]>
> ---
>
> drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> include/linux/phy.h | 5 +++++
> 2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c4aec56d0a95..162abde6223d 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>
> @@ -845,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);
This block, and the changes in patch 4 duplicate functions from
drivers/of/of_mdio.c, but it doesn't refactor anything in
drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
the parsing in these functions of "compatible" into the ACPI code path?
If so, then the existing code path needs to be refactored to work with
fwnode_handle instead of device_node.
If not, then the DT path in these functions should call out to of_mdio,
while the ACPI path only does what is necessary.
> +
> /**
> * get_phy_device - reads the specified PHY device and returns its @phy_device
> * struct
> @@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
> */
> struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> {
> - return fwnode_find_reference(fwnode, "phy-handle", 0);
> + struct fwnode_handle *phy_node;
> +
> + 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);
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 7b1bf3d46fd3..b6814e04092f 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1378,6 +1378,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);
> @@ -1385,6 +1386,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)
> {
>
On 30/09/2020 17:37, Rafael J. Wysocki wrote:
> On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
> <[email protected]> wrote:
>>
>> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
>> provide them to be connected to MAC.
>>
>> Describe properties "phy-handle" and "phy-mode".
>>
>> Signed-off-by: Calvin Johnson <[email protected]>
>> ---
>>
>> Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
>> 1 file changed, 78 insertions(+)
>> create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
>>
>> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
>> new file mode 100644
>> index 000000000000..f10feb24ec1c
>> --- /dev/null
>> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
>> @@ -0,0 +1,78 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=========================
>> +MDIO bus and PHYs in ACPI
>> +=========================
>> +
>> +The PHYs on an mdiobus are probed and registered using
>> +fwnode_mdiobus_register_phy().
>> +Later, for connecting these PHYs to MAC, the PHYs registered on the
>> +mdiobus have to be referenced.
>> +
>> +phy-handle
>> +-----------
>> +For each MAC node, a property "phy-handle" is used to reference the
>> +PHY that is registered on an MDIO bus.
>
> It is not clear what "a property" means in this context.
>
> This should refer to the documents introducing the _DSD-based generic
> device properties rules, including the GUID used below.
>
> You need to say whether or not the property is mandatory and if it
> isn't mandatory, you need to say what the lack of it means.
>
>> +
>> +phy-mode
>> +--------
>> +Property "phy-mode" defines the type of PHY interface.
>
> This needs to be more detailed too, IMO. At the very least, please
> list all of the possible values of it and document their meaning.
If the goal is to align with DT, it would be appropriate to point to
where those properties are defined for DT rather than to have a separate
description here. I suggest something along the lines of:
The "phy-mode" _DSD property is used to describe the connection to
the PHY. The valid values for "phy-mode" are defined in
Documentation/devicetree/bindings/ethernet-controller.yaml
>
>> +
>> +An example of this is shown below::
>> +
>> +DSDT entry for MACs where PHY nodes are referenced
>> +--------------------------------------------------
>> + Scope(\_SB.MCE0.PR17) // 1G
>> + {
>> + Name (_DSD, Package () {
>> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> + Package () {
>> + Package (2) {"phy-mode", "rgmii-id"},
>> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
>
> What is "phy-handle"?
>
> You haven't introduced it above.
Can you elaborate? "phy-handle" has a section to itself in this
document. Agree that it needs to be defined more, but it does read to me
as having been defined.
>
>> + }
>> + })
>> + }
>> +
>> + Scope(\_SB.MCE0.PR18) // 1G
>> + {
>> + Name (_DSD, Package () {
>> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> + Package () {
>> + Package (2) {"phy-mode", "rgmii-id"},
>> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
>> + }
>> + })
>> + }
>> +
>> +DSDT entry for MDIO node
>> +------------------------
>> +a) Silicon Component
>
> What is this device, exactly?
>
>> +--------------------
>> + Scope(_SB)
>> + {
>> + Device(MDI0) {
>> + Name(_HID, "NXP0006")
>> + Name(_CCA, 1)
>> + Name(_UID, 0)
>> + Name(_CRS, ResourceTemplate() {
>> + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
>> + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
>> + {
>> + MDI0_IT
>> + }
>> + }) // end of _CRS for MDI0
>> + } // end of MDI0
>> + }
>> +
>> +b) Platform Component
>> +---------------------
>> + Scope(\_SB.MDI0)
>> + {
>> + Device(PHY1) {
>> + Name (_ADR, 0x1)
>> + } // end of PHY1
>> +
>> + Device(PHY2) {
>> + Name (_ADR, 0x2)
>> + } // end of PHY2
>> + }
>> --
>
> What is the connection between the last two pieces of ASL and the _DSD
> definitions above?
>
On 30/09/2020 17:04, Calvin Johnson wrote:
> 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]>
> ---
>
> .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 ++++++++++++-------
> 1 file changed, 50 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index 90cd243070d7..18502ee83e46 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -3,6 +3,7 @@
>
> #include "dpaa2-eth.h"
> #include "dpaa2-mac.h"
> +#include <linux/acpi.h>
>
> #define phylink_to_dpaa2_mac(config) \
> container_of((config), struct dpaa2_mac, phylink_config)
> @@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
> }
>
> /* 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 acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct fwnode_handle *dpmacs, *dpmac = NULL;
> + unsigned long long adr;
> + acpi_status status;
> int err;
> + u32 id;
>
> - dpmacs = of_find_node_by_name(NULL, "dpmacs");
> - if (!dpmacs)
> - return NULL;
> + if (is_of_node(dev->parent->fwnode)) {
> + dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> + if (!dpmacs)
> + return NULL;
> +
> + while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> + err = fwnode_property_read_u32(dpmac, "reg", &id);
> + if (err)
> + continue;
> + if (id == dpmac_id)
> + return dpmac;
> + }
There is a change of behaviour here that isn't described in the patch
description, so I'm having trouble following the intent. The original
code searches the tree for a node named "dpmacs", but the new code
constrains the search to just the parent device.
Also, because the new code path is only used in the DT case, I don't see
why the behaviour change is required. If it is a bug fix, it should be
broken out into a separate patch. If it is something else, can you
describe what the reasoning is?
I also see that the change to the code has dropped the of_node_put() on
the exit path.
>
> - while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> - err = of_property_read_u32(dpmac, "reg", &id);
> - if (err)
> - continue;
> - if (id == dpmac_id)
> - break;
> + } else if (is_acpi_node(dev->parent->fwnode)) {
> + device_for_each_child_node(dev->parent, dpmac) {
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> + "_ADR", NULL, &adr);
> + if (ACPI_FAILURE(status)) {
> + pr_debug("_ADR returned %d on %s\n",
> + status, (char *)buffer.pointer);
> + continue;
> + } else {
> + id = (u32)adr;
> + if (id == dpmac_id)
> + return dpmac;
> + }
> + }
> }
> -
> - of_node_put(dpmacs);
> -
> - return dpmac;
> + 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;
Is this correct? The function prototype from patch 2 is:
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
It returns struct fwnode_handle *. Does this compile?
>
> err = phy_mode(attr.eth_if, &if_mode);
> if (!err)
> @@ -303,7 +322,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;
> @@ -323,7 +342,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;
> @@ -341,7 +360,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)) {
> @@ -352,7 +371,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
>
> if (attr.link_type == DPMAC_LINK_TYPE_PHY &&
> attr.eth_if != DPMAC_ETH_IF_RGMII) {
> - err = dpaa2_pcs_create(mac, dpmac_node, attr.id);
> + err = dpaa2_pcs_create(mac, to_of_node(dpmac_node), attr.id);
> if (err)
> goto err_put_node;
> }
> @@ -361,7 +380,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);
> @@ -372,13 +391,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))
> + of_node_put(to_of_node(dpmac_node));
>
> return 0;
>
> @@ -387,7 +407,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))
> + of_node_put(to_of_node(dpmac_node));
> err_close_dpmac:
> dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
> return err;
>
On Fri, Oct 2, 2020 at 1:09 PM Grant Likely <[email protected]> wrote:
>
>
>
> On 30/09/2020 17:37, Rafael J. Wysocki wrote:
> > On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
> > <[email protected]> wrote:
> >>
> >> Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
> >> provide them to be connected to MAC.
> >>
> >> Describe properties "phy-handle" and "phy-mode".
> >>
> >> Signed-off-by: Calvin Johnson <[email protected]>
> >> ---
> >>
> >> Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++
> >> 1 file changed, 78 insertions(+)
> >> create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
> >>
> >> diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst
> >> new file mode 100644
> >> index 000000000000..f10feb24ec1c
> >> --- /dev/null
> >> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
> >> @@ -0,0 +1,78 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +=========================
> >> +MDIO bus and PHYs in ACPI
> >> +=========================
> >> +
> >> +The PHYs on an mdiobus are probed and registered using
> >> +fwnode_mdiobus_register_phy().
> >> +Later, for connecting these PHYs to MAC, the PHYs registered on the
> >> +mdiobus have to be referenced.
> >> +
> >> +phy-handle
> >> +-----------
> >> +For each MAC node, a property "phy-handle" is used to reference the
> >> +PHY that is registered on an MDIO bus.
> >
> > It is not clear what "a property" means in this context.
> >
> > This should refer to the documents introducing the _DSD-based generic
> > device properties rules, including the GUID used below.
> >
> > You need to say whether or not the property is mandatory and if it
> > isn't mandatory, you need to say what the lack of it means.
> >
> >> +
> >> +phy-mode
> >> +--------
> >> +Property "phy-mode" defines the type of PHY interface.
> >
> > This needs to be more detailed too, IMO. At the very least, please
> > list all of the possible values of it and document their meaning.
>
> If the goal is to align with DT, it would be appropriate to point to
> where those properties are defined for DT rather than to have a separate
> description here. I suggest something along the lines of:
>
> The "phy-mode" _DSD property is used to describe the connection to
> the PHY. The valid values for "phy-mode" are defined in
> Documentation/devicetree/bindings/ethernet-controller.yaml
>
> >
> >> +
> >> +An example of this is shown below::
> >> +
> >> +DSDT entry for MACs where PHY nodes are referenced
> >> +--------------------------------------------------
> >> + Scope(\_SB.MCE0.PR17) // 1G
> >> + {
> >> + Name (_DSD, Package () {
> >> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> + Package () {
> >> + Package (2) {"phy-mode", "rgmii-id"},
> >> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
> >
> > What is "phy-handle"?
> >
> > You haven't introduced it above.
>
> Can you elaborate? "phy-handle" has a section to itself in this
> document.
Yes, it does.
I overlooked it, sorry.
> Agree that it needs to be defined more, but it does read to me
> as having been defined.
Yup.
Cheers!
On 10/2/2020 4:05 AM, Grant Likely wrote:
>
>
> On 30/09/2020 17:04, Calvin Johnson wrote:
>> 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]>
>> ---
>>
>> drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
>> include/linux/phy.h | 5 +++++
>> 2 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index c4aec56d0a95..162abde6223d 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>
>> @@ -845,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);
>
> This block, and the changes in patch 4 duplicate functions from
> drivers/of/of_mdio.c, but it doesn't refactor anything in
> drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
> the parsing in these functions of "compatible" into the ACPI code path?
>
> If so, then the existing code path needs to be refactored to work with
> fwnode_handle instead of device_node.
>
> If not, then the DT path in these functions should call out to of_mdio,
> while the ACPI path only does what is necessary.
Rob has been asking before to have drivers/of/of_mdio.c be merged or at
least relocated within drivers/net/phy where it would naturally belong.
As a preliminary step towards ACPI support that would seem reasonable to do.
Then, as Grant suggests you can start re-factoring as much as possible
with using fwnode_handle.
--
Florian
On Fri, Oct 02, 2020 at 08:14:07AM -0700, Florian Fainelli wrote:
> On 10/2/2020 4:05 AM, Grant Likely wrote:
> > On 30/09/2020 17:04, Calvin Johnson wrote:
> > > 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]>
> > > ---
> > >
> > > ? drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> > > ? include/linux/phy.h????????? |? 5 +++++
> > > ? 2 files changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index c4aec56d0a95..162abde6223d 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>
> > > @@ -845,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);
> >
> > This block, and the changes in patch 4 duplicate functions from
> > drivers/of/of_mdio.c, but it doesn't refactor anything in
> > drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
> > the parsing in these functions of "compatible" into the ACPI code path?
> >
> > If so, then the existing code path needs to be refactored to work with
> > fwnode_handle instead of device_node.
> >
> > If not, then the DT path in these functions should call out to of_mdio,
> > while the ACPI path only does what is necessary.
>
> Rob has been asking before to have drivers/of/of_mdio.c be merged or at
> least relocated within drivers/net/phy where it would naturally belong. As a
> preliminary step towards ACPI support that would seem reasonable to do.
I think even I have commented on specific functions while reviewing
patches from NXP that the DT/ACPI code should use common bases...
I have been planning that if that doesn't get done, then I'd do it,
but really NXP should do it being the ones adding this infrastructure;
they should do the job properly and not take advantage of volunteers
in the community cleaning up their resulting submissions.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Andy,
On Thu, Oct 01, 2020 at 06:36:06PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 30, 2020 at 7:06 PM Calvin Johnson
> <[email protected]> wrote:
> >
> > 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.
>
> ...
>
> > #include "dpaa2-eth.h"
> > #include "dpaa2-mac.h"
>
> > +#include <linux/acpi.h>
>
> Please, put generic headers first.
>
> > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > + struct fwnode_handle *dpmacs, *dpmac = NULL;
> > + unsigned long long adr;
> > + acpi_status status;
> > int err;
> > + u32 id;
> >
> > - dpmacs = of_find_node_by_name(NULL, "dpmacs");
> > - if (!dpmacs)
> > - return NULL;
> > + if (is_of_node(dev->parent->fwnode)) {
> > + dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> > + if (!dpmacs)
> > + return NULL;
> > +
> > + while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> > + err = fwnode_property_read_u32(dpmac, "reg", &id);
> > + if (err)
> > + continue;
> > + if (id == dpmac_id)
> > + return dpmac;
> > + }
> >
> > + } else if (is_acpi_node(dev->parent->fwnode)) {
> > + device_for_each_child_node(dev->parent, dpmac) {
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> > + "_ADR", NULL, &adr);
> > + if (ACPI_FAILURE(status)) {
> > + pr_debug("_ADR returned %d on %s\n",
> > + status, (char *)buffer.pointer);
> > + continue;
> > + } else {
> > + id = (u32)adr;
> > + if (id == dpmac_id)
> > + return dpmac;
> > + }
> > + }
>
> Can you rather implement generic one which will be
>
> int fwnode_get_child_id(struct fwnode_handle *fwnode, u64 *id);
>
> and put the logic of retrieving 'reg' or _ADR? Also, for the latter we
> have a special macro
> METHOD_NAME__ADR.
>
> See [1] as well. Same idea I have shared already.
>
> [1]: https://lore.kernel.org/linux-iio/[email protected]/T/#m5f61921fa67a5b40522b7f7b17216e0d204647be
>
> ...
>
> > - of_node_put(dpmac_node);
> > + if (is_of_node(dpmac_node))
> > + of_node_put(to_of_node(dpmac_node));
>
> I'm not sure why you can't use fwnode_handle_put()?
>
> > + if (is_of_node(dpmac_node))
> > + of_node_put(to_of_node(dpmac_node));
>
> Ditto.
Sure. I'll take care of these comments.
Thanks
Calvin
Hi Grant,
On Fri, Oct 02, 2020 at 12:22:37PM +0100, Grant Likely wrote:
>
>
> On 30/09/2020 17:04, Calvin Johnson wrote:
> > 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]>
> > ---
> >
> > .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 ++++++++++++-------
> > 1 file changed, 50 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > index 90cd243070d7..18502ee83e46 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > @@ -3,6 +3,7 @@
> > #include "dpaa2-eth.h"
> > #include "dpaa2-mac.h"
> > +#include <linux/acpi.h>
> > #define phylink_to_dpaa2_mac(config) \
> > container_of((config), struct dpaa2_mac, phylink_config)
> > @@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)
> > }
> > /* 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 acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > + struct fwnode_handle *dpmacs, *dpmac = NULL;
> > + unsigned long long adr;
> > + acpi_status status;
> > int err;
> > + u32 id;
> > - dpmacs = of_find_node_by_name(NULL, "dpmacs");
> > - if (!dpmacs)
> > - return NULL;
> > + if (is_of_node(dev->parent->fwnode)) {
> > + dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
> > + if (!dpmacs)
> > + return NULL;
> > +
> > + while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
> > + err = fwnode_property_read_u32(dpmac, "reg", &id);
> > + if (err)
> > + continue;
> > + if (id == dpmac_id)
> > + return dpmac;
> > + }
> There is a change of behaviour here that isn't described in the patch
> description, so I'm having trouble following the intent. The original code
> searches the tree for a node named "dpmacs", but the new code constrains the
> search to just the parent device.
>
> Also, because the new code path is only used in the DT case, I don't see why
> the behaviour change is required. If it is a bug fix, it should be broken
> out into a separate patch. If it is something else, can you describe what
> the reasoning is?
Yes, the behaviour for ACPI had to be changed as I couldn't find an ACPI method
to find named nodes. I did this change some time back and it didn't work for
ACPI. I'll revisit this once again and keep original code if needed.
Behaviour for DT hasn't changed although the APIs changed.
>
> I also see that the change to the code has dropped the of_node_put() on the
> exit path.
Sure, I'll fix it.
>
> > - while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
> > - err = of_property_read_u32(dpmac, "reg", &id);
> > - if (err)
> > - continue;
> > - if (id == dpmac_id)
> > - break;
> > + } else if (is_acpi_node(dev->parent->fwnode)) {
> > + device_for_each_child_node(dev->parent, dpmac) {
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
> > + "_ADR", NULL, &adr);
> > + if (ACPI_FAILURE(status)) {
> > + pr_debug("_ADR returned %d on %s\n",
> > + status, (char *)buffer.pointer);
> > + continue;
> > + } else {
> > + id = (u32)adr;
> > + if (id == dpmac_id)
> > + return dpmac;
> > + }
> > + }
> > }
> > -
> > - of_node_put(dpmacs);
> > -
> > - return dpmac;
> > + 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;
>
> Is this correct? The function prototype from patch 2 is:
>
> struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>
> It returns struct fwnode_handle *. Does this compile?
Will correct this.
Thanks
Calvin
On Fri, Oct 02, 2020 at 12:05:14PM +0100, Grant Likely wrote:
>
>
> On 30/09/2020 17:04, Calvin Johnson wrote:
> > 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]>
> > ---
> >
> > drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> > include/linux/phy.h | 5 +++++
> > 2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index c4aec56d0a95..162abde6223d 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>
> > @@ -845,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);
>
> This block, and the changes in patch 4 duplicate functions from
> drivers/of/of_mdio.c, but it doesn't refactor anything in
> drivers/of/of_mdio.c to use the new path. Is your intent to bring all of the
> parsing in these functions of "compatible" into the ACPI code path?
>
> If so, then the existing code path needs to be refactored to work with
> fwnode_handle instead of device_node.
>
> If not, then the DT path in these functions should call out to of_mdio,
> while the ACPI path only does what is necessary.
I'll work on refactoring as Florian and Rob are also suggesting the same.
Thanks
Calvin
Hi Russell and Florian,
On Fri, Oct 02, 2020 at 04:50:26PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Oct 02, 2020 at 08:14:07AM -0700, Florian Fainelli wrote:
> > On 10/2/2020 4:05 AM, Grant Likely wrote:
> > > On 30/09/2020 17:04, Calvin Johnson wrote:
> > > > 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]>
> > > > ---
> > > >
> > > > ? drivers/net/phy/phy_device.c | 32 +++++++++++++++++++++++++++++++-
> > > > ? include/linux/phy.h????????? |? 5 +++++
> > > > ? 2 files changed, 36 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > > index c4aec56d0a95..162abde6223d 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>
> > > > @@ -845,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);
> > >
> > > This block, and the changes in patch 4 duplicate functions from
> > > drivers/of/of_mdio.c, but it doesn't refactor anything in
> > > drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
> > > the parsing in these functions of "compatible" into the ACPI code path?
> > >
> > > If so, then the existing code path needs to be refactored to work with
> > > fwnode_handle instead of device_node.
> > >
> > > If not, then the DT path in these functions should call out to of_mdio,
> > > while the ACPI path only does what is necessary.
> >
> > Rob has been asking before to have drivers/of/of_mdio.c be merged or at
> > least relocated within drivers/net/phy where it would naturally belong. As a
> > preliminary step towards ACPI support that would seem reasonable to do.
>
> I think even I have commented on specific functions while reviewing
> patches from NXP that the DT/ACPI code should use common bases...
>
> I have been planning that if that doesn't get done, then I'd do it,
> but really NXP should do it being the ones adding this infrastructure;
> they should do the job properly and not take advantage of volunteers
> in the community cleaning up their resulting submissions.
I'll work on it.
Thanks
Calvin
On Sat, Oct 03, 2020 at 11:09:49PM +0530, Calvin Johnson wrote:
> Hi Grant,
>
> On Fri, Oct 02, 2020 at 12:22:37PM +0100, Grant Likely wrote:
> >
> >
<snip>
> > > -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;
> >
> > Is this correct? The function prototype from patch 2 is:
> >
> > struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> >
> > It returns struct fwnode_handle *. Does this compile?
>
> Will correct this.
Sorry, this change looks correct to me. Actully, the function prototype is:
int fwnode_get_phy_mode(struct fwnode_handle *fwnode);
It is from drivers/base/property.c.
fwnode_get_phy_node() defined in patch-2 is used in phylink_fwnode_phy_connect()
The confusion maybe due to one letter difference in the fn names.
Thanks
Calvin