The current phy reset handling is broken in a way that it needs
pre-running firmware to setup the phy initially. Since the very first
step is to readout the PHYID1/2 registers before doing anything else.
The whole dection logic will fall apart if the pre-running firmware
don't setup the phy accordingly or the kernel boot resets GPIOs states
or disables clocks. In such cases the PHYID1/2 read access will fail and
so the whole detection will fail.
I fixed this via this series, the fix will include a new kernel API
called phy_device_atomic_register() which will do all necessary things
and return a 'struct phy_device' on success. So setting up a phy and the
phy state machine is more convenient.
I tested the series on a i.MX8MP-EVK and a custom board which have a
TJA1102 dual-port ethernet phy. Other testers are welcome :)
Regards,
Marco
Signed-off-by: Marco Felsch <[email protected]>
---
Marco Felsch (12):
net: phy: refactor phy_device_create function
net: phy: refactor get_phy_device function
net: phy: add phy_device_set_miits helper
net: phy: unify get_phy_device and phy_device_create parameter list
net: phy: add phy_id_broken support
net: phy: add phy_device_atomic_register helper
net: mdio: make use of phy_device_atomic_register helper
net: phy: add possibility to specify mdio device parent
net: phy: nxp-tja11xx: make use of phy_device_atomic_register()
of: mdio: remove now unused of_mdiobus_phy_device_register()
net: mdiobus: remove now unused fwnode helpers
net: phy: add default gpio assert/deassert delay
Documentation/firmware-guide/acpi/dsd/phy.rst | 2 +-
MAINTAINERS | 1 -
drivers/net/ethernet/adi/adin1110.c | 6 +-
drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +-
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +-
drivers/net/ethernet/socionext/netsec.c | 7 +-
drivers/net/mdio/Kconfig | 7 -
drivers/net/mdio/Makefile | 1 -
drivers/net/mdio/acpi_mdio.c | 20 +-
drivers/net/mdio/fwnode_mdio.c | 183 ------------
drivers/net/mdio/mdio-xgene.c | 6 +-
drivers/net/mdio/of_mdio.c | 23 +-
drivers/net/phy/bcm-phy-ptp.c | 2 +-
drivers/net/phy/dp83640.c | 2 +-
drivers/net/phy/fixed_phy.c | 6 +-
drivers/net/phy/mdio_bus.c | 7 +-
drivers/net/phy/micrel.c | 2 +-
drivers/net/phy/mscc/mscc_ptp.c | 2 +-
drivers/net/phy/nxp-c45-tja11xx.c | 2 +-
drivers/net/phy/nxp-tja11xx.c | 47 ++-
drivers/net/phy/phy_device.c | 348 +++++++++++++++++++---
drivers/net/phy/sfp.c | 7 +-
include/linux/fwnode_mdio.h | 35 ---
include/linux/of_mdio.h | 8 -
include/linux/phy.h | 46 ++-
25 files changed, 442 insertions(+), 347 deletions(-)
---
base-commit: 054fbf7ff8143d35ca7d3bb5414bb44ee1574194
change-id: 20230405-net-next-topic-net-phy-reset-4f79ff7df4a0
Best regards,
--
Marco Felsch <[email protected]>
There are no references to of_mdiobus_phy_device_register() anymore so
we can remove the code.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/mdio/of_mdio.c | 9 ---------
include/linux/of_mdio.h | 8 --------
2 files changed, 17 deletions(-)
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 10dd45c3bde0..e85be8a72978 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -33,15 +33,6 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id)
return fwnode_get_phy_id(of_fwnode_handle(device), phy_id);
}
-int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
- struct device_node *child, u32 addr)
-{
- return fwnode_mdiobus_phy_device_register(mdio, phy,
- of_fwnode_handle(child),
- addr);
-}
-EXPORT_SYMBOL(of_mdiobus_phy_device_register);
-
static int of_mdiobus_register_phy(struct mii_bus *mdio,
struct device_node *child, u32 addr)
{
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8a52ef2e6fa6..ee1fe034f3fe 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -47,8 +47,6 @@ 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);
-int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
- struct device_node *child, u32 addr);
static inline int of_mdio_parse_addr(struct device *dev,
const struct device_node *np)
@@ -142,12 +140,6 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
return false;
}
-static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
- struct phy_device *phy,
- struct device_node *child, u32 addr)
-{
- return -ENOSYS;
-}
#endif
--
2.39.2
Split get_phy_device() into local helper function. This commit is in
preparation of fixing the phy reset handling. No functional changes for
the public API users.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/phy/phy_device.c | 44 +++++++++++++++++++++++++++-----------------
1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index dd0aaa866d17..64292e47e3fc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -938,6 +938,32 @@ int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
}
EXPORT_SYMBOL(fwnode_get_phy_id);
+static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
+ u32 *phy_id, struct phy_c45_device_ids *c45_ids)
+{
+ int r;
+
+ if (*is_c45)
+ r = get_phy_c45_ids(bus, addr, c45_ids);
+ else
+ r = get_phy_c22_id(bus, addr, phy_id);
+
+ if (r)
+ return r;
+
+ /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
+ * of 0 when probed using get_phy_c22_id() with no error. Proceed to
+ * probe with C45 to see if we're able to get a valid PHY ID in the C45
+ * space, if successful, create the C45 PHY device.
+ */
+ if (!*is_c45 && *phy_id == 0 && bus->read_c45) {
+ *is_c45 = true;
+ return get_phy_c45_ids(bus, addr, c45_ids);
+ }
+
+ return 0;
+}
+
/**
* get_phy_device - reads the specified PHY device and returns its @phy_device
* struct
@@ -967,26 +993,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
c45_ids.mmds_present = 0;
memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
- if (is_c45)
- r = get_phy_c45_ids(bus, addr, &c45_ids);
- else
- r = get_phy_c22_id(bus, addr, &phy_id);
-
+ r = phy_device_detect(bus, addr, &is_c45, &phy_id, &c45_ids);
if (r)
return ERR_PTR(r);
- /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
- * of 0 when probed using get_phy_c22_id() with no error. Proceed to
- * probe with C45 to see if we're able to get a valid PHY ID in the C45
- * space, if successful, create the C45 PHY device.
- */
- if (!is_c45 && phy_id == 0 && bus->read_c45) {
- r = get_phy_c45_ids(bus, addr, &c45_ids);
- if (!r)
- return phy_device_create(bus, addr, phy_id,
- true, &c45_ids);
- }
-
return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
}
EXPORT_SYMBOL(get_phy_device);
--
2.39.2
These APIs are broken since the very first day because they assume that
the phy is accessible to read the PHYID registers. If this requirement
is not meet the code fails to add the required phys.
The newly added phy_device_atomic_register() API have fixed this and
supports firmware parsing as well. After we switched all in kernel users
of the fwnode API we now can remove this part from the kernel.
Signed-off-by: Marco Felsch <[email protected]>
---
MAINTAINERS | 1 -
drivers/net/mdio/Kconfig | 7 --
drivers/net/mdio/Makefile | 1 -
drivers/net/mdio/acpi_mdio.c | 2 +-
drivers/net/mdio/fwnode_mdio.c | 186 -----------------------------------------
drivers/net/mdio/of_mdio.c | 1 -
include/linux/fwnode_mdio.h | 35 --------
7 files changed, 1 insertion(+), 232 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7812f0e251ad..2894b456c0a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7666,7 +7666,6 @@ F: Documentation/devicetree/bindings/net/qca,ar803x.yaml
F: Documentation/networking/phy.rst
F: drivers/net/mdio/
F: drivers/net/mdio/acpi_mdio.c
-F: drivers/net/mdio/fwnode_mdio.c
F: drivers/net/mdio/of_mdio.c
F: drivers/net/pcs/
F: drivers/net/phy/
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 90309980686e..d0d19666f099 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -19,13 +19,6 @@ config MDIO_BUS
reflects whether the mdio_bus/mdio_device code is built as a
loadable module or built-in.
-config FWNODE_MDIO
- def_tristate PHYLIB
- depends on (ACPI || OF) || COMPILE_TEST
- select FIXED_PHY
- help
- FWNODE MDIO bus (Ethernet PHY) accessors
-
config OF_MDIO
def_tristate PHYLIB
depends on OF
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 7d4cb4c11e4e..798ede184766 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -2,7 +2,6 @@
# Makefile for Linux MDIO bus drivers
obj-$(CONFIG_ACPI_MDIO) += acpi_mdio.o
-obj-$(CONFIG_FWNODE_MDIO) += fwnode_mdio.o
obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_MDIO_ASPEED) += mdio-aspeed.o
diff --git a/drivers/net/mdio/acpi_mdio.c b/drivers/net/mdio/acpi_mdio.c
index 25feb571bd1f..727b83bf3a15 100644
--- a/drivers/net/mdio/acpi_mdio.c
+++ b/drivers/net/mdio/acpi_mdio.c
@@ -10,8 +10,8 @@
#include <linux/acpi_mdio.h>
#include <linux/bits.h>
#include <linux/dev_printk.h>
-#include <linux/fwnode_mdio.h>
#include <linux/module.h>
+#include <linux/phy.h>
#include <linux/types.h>
MODULE_AUTHOR("Calvin Johnson <[email protected]>");
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
deleted file mode 100644
index 47ef702d4ffd..000000000000
--- a/drivers/net/mdio/fwnode_mdio.c
+++ /dev/null
@@ -1,186 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * fwnode helpers for the MDIO (Ethernet PHY) API
- *
- * This file provides helper functions for extracting PHY device information
- * out of the fwnode and using it to populate an mii_bus.
- */
-
-#include <linux/acpi.h>
-#include <linux/fwnode_mdio.h>
-#include <linux/of.h>
-#include <linux/phy.h>
-#include <linux/pse-pd/pse.h>
-
-MODULE_AUTHOR("Calvin Johnson <[email protected]>");
-MODULE_LICENSE("GPL");
-
-static struct pse_control *
-fwnode_find_pse_control(struct fwnode_handle *fwnode)
-{
- struct pse_control *psec;
- struct device_node *np;
-
- if (!IS_ENABLED(CONFIG_PSE_CONTROLLER))
- return NULL;
-
- np = to_of_node(fwnode);
- if (!np)
- return NULL;
-
- psec = of_pse_control_get(np);
- if (PTR_ERR(psec) == -ENOENT)
- return NULL;
-
- return psec;
-}
-
-static struct mii_timestamper *
-fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
-{
- struct of_phandle_args arg;
- int err;
-
- if (is_acpi_node(fwnode))
- return NULL;
-
- err = of_parse_phandle_with_fixed_args(to_of_node(fwnode),
- "timestamper", 1, 0, &arg);
- if (err == -ENOENT)
- return NULL;
- else if (err)
- return ERR_PTR(err);
-
- if (arg.args_count != 1)
- return ERR_PTR(-EINVAL);
-
- return register_mii_timestamper(arg.np, arg.args[0]);
-}
-
-int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
- struct phy_device *phy,
- struct fwnode_handle *child, u32 addr)
-{
- int rc;
-
- rc = fwnode_irq_get(child, 0);
- /* Don't wait forever if the IRQ provider doesn't become available,
- * just fall back to poll mode
- */
- if (rc == -EPROBE_DEFER)
- rc = driver_deferred_probe_check_state(&phy->mdio.dev);
- if (rc == -EPROBE_DEFER)
- return rc;
-
- if (rc > 0) {
- phy->irq = rc;
- mdio->irq[addr] = rc;
- } else {
- phy->irq = mdio->irq[addr];
- }
-
- if (fwnode_property_read_bool(child, "broken-turn-around"))
- mdio->phy_ignore_ta_mask |= 1 << addr;
-
- fwnode_property_read_u32(child, "reset-assert-us",
- &phy->mdio.reset_assert_delay);
- fwnode_property_read_u32(child, "reset-deassert-us",
- &phy->mdio.reset_deassert_delay);
-
- /* Associate the fwnode with the device structure so it
- * can be looked up later
- */
- fwnode_handle_get(child);
- device_set_node(&phy->mdio.dev, child);
-
- /* All data is now stored in the phy struct;
- * register it
- */
- rc = phy_device_register(phy);
- if (rc) {
- device_set_node(&phy->mdio.dev, NULL);
- fwnode_handle_put(child);
- return rc;
- }
-
- dev_dbg(&mdio->dev, "registered phy %p fwnode at address %i\n",
- child, addr);
- return 0;
-}
-EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register);
-
-int fwnode_mdiobus_register_phy(struct mii_bus *bus,
- struct fwnode_handle *child, u32 addr)
-{
- struct phy_device_config config = {
- .mii_bus = bus,
- .phy_addr = addr,
- };
- struct mii_timestamper *mii_ts = NULL;
- struct pse_control *psec = NULL;
- struct phy_device *phy;
- u32 phy_id;
- int rc;
-
- psec = fwnode_find_pse_control(child);
- if (IS_ERR(psec))
- return PTR_ERR(psec);
-
- mii_ts = fwnode_find_mii_timestamper(child);
- if (IS_ERR(mii_ts)) {
- rc = PTR_ERR(mii_ts);
- goto clean_pse;
- }
-
- config.is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
- if (config.is_c45 || fwnode_get_phy_id(child, &config.phy_id))
- phy = get_phy_device(&config);
- else
- phy = phy_device_create(&config);
- if (IS_ERR(phy)) {
- rc = PTR_ERR(phy);
- goto clean_mii_ts;
- }
-
- 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 = fwnode_handle_get(child);
-
- /* All data is now stored in the phy struct, so register it */
- rc = phy_device_register(phy);
- if (rc) {
- phy->mdio.dev.fwnode = NULL;
- fwnode_handle_put(child);
- goto clean_phy;
- }
- } else if (is_of_node(child)) {
- rc = fwnode_mdiobus_phy_device_register(bus, phy, child, addr);
- if (rc)
- goto clean_phy;
- }
-
- phy->psec = psec;
-
- /* 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;
-
-clean_phy:
- phy_device_free(phy);
-clean_mii_ts:
- unregister_mii_timestamper(mii_ts);
-clean_pse:
- pse_control_put(psec);
-
- return rc;
-}
-EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index e85be8a72978..15ae968ef186 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -10,7 +10,6 @@
#include <linux/device.h>
#include <linux/err.h>
-#include <linux/fwnode_mdio.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/netdevice.h>
diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h
deleted file mode 100644
index faf603c48c86..000000000000
--- a/include/linux/fwnode_mdio.h
+++ /dev/null
@@ -1,35 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * FWNODE helper for the MDIO (Ethernet PHY) API
- */
-
-#ifndef __LINUX_FWNODE_MDIO_H
-#define __LINUX_FWNODE_MDIO_H
-
-#include <linux/phy.h>
-
-#if IS_ENABLED(CONFIG_FWNODE_MDIO)
-int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
- struct phy_device *phy,
- struct fwnode_handle *child, u32 addr);
-
-int fwnode_mdiobus_register_phy(struct mii_bus *bus,
- struct fwnode_handle *child, u32 addr);
-
-#else /* CONFIG_FWNODE_MDIO */
-int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
- struct phy_device *phy,
- struct fwnode_handle *child, u32 addr)
-{
- return -EINVAL;
-}
-
-static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus,
- struct fwnode_handle *child,
- u32 addr)
-{
- return -EINVAL;
-}
-#endif
-
-#endif /* __LINUX_FWNODE_MDIO_H */
--
2.39.2
Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port
report 0 for the 2nd port. To fix this a driver needs to supply the
phyid instead and tell the phy framework to not try to readout the
phyid. The latter case is done via the new 'phy_id_broken' flag which
tells the phy framework to skip phyid readout for the corresponding phy.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/phy/nxp-tja11xx.c | 1 +
drivers/net/phy/phy_device.c | 3 +++
include/linux/phy.h | 3 +++
3 files changed, 7 insertions(+)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b5e03d66b7f5..2a4c0f6d74eb 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -560,6 +560,7 @@ static void tja1102_p1_register(struct work_struct *work)
.mii_bus = bus,
/* Real PHY ID of Port 1 is 0 */
.phy_id = PHY_ID_TJA1102,
+ .phy_id_broken = true,
};
struct phy_device *phy;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bb465a324eef..7e4b3b3caba9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -969,6 +969,9 @@ static int phy_device_detect(struct phy_device_config *config)
int addr = config->phy_addr;
int r;
+ if (config->phy_id_broken)
+ return 0;
+
if (is_c45)
r = get_phy_c45_ids(bus, addr, c45_ids);
else
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 498f4dc7669d..0f0cb72a08ab 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -764,6 +764,8 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
* @phy_id: UID for this device found during discovery
* @c45_ids: 802.3-c45 Device Identifiers if is_c45.
* @is_c45: If true the PHY uses the 802.3 clause 45 protocol
+ * @phy_id_broken: Skip the phy_id detection instead use the supplied phy_id or
+ * c45_ids.
*
* The struct contain possible configuration parameters for a PHY device which
* are used to setup the struct phy_device.
@@ -775,6 +777,7 @@ struct phy_device_config {
u32 phy_id;
struct phy_c45_device_ids c45_ids;
bool is_c45;
+ bool phy_id_broken;
};
/**
--
2.39.2
There are phy's not mention any assert/deassert delay within their
datasheets but the real world showed that this is not true. They need at
least a few us to be accessible and to readout the register values. So
add a sane default value of 1000us for both assert and deassert to fix
this in a global matter.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/phy/phy_device.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 30b3ac9818b1..08f2657110e0 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3197,6 +3197,9 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
return register_mii_timestamper(arg.np, arg.args[0]);
}
+#define DEFAULT_GPIO_RESET_ASSERT_DELAY_US 1000
+#define DEFAULT_GPIO_RESET_DEASSERT_DELAY_US 1000
+
static int
phy_device_parse_fwnode(struct phy_device *phydev,
struct phy_device_config *config)
@@ -3223,8 +3226,11 @@ phy_device_parse_fwnode(struct phy_device *phydev,
if (fwnode_property_read_bool(fwnode, "broken-turn-around"))
bus->phy_ignore_ta_mask |= 1 << addr;
+
+ phydev->mdio.reset_assert_delay = DEFAULT_GPIO_RESET_ASSERT_DELAY_US;
fwnode_property_read_u32(fwnode, "reset-assert-us",
&phydev->mdio.reset_assert_delay);
+ phydev->mdio.reset_deassert_delay = DEFAULT_GPIO_RESET_DEASSERT_DELAY_US;
fwnode_property_read_u32(fwnode, "reset-deassert-us",
&phydev->mdio.reset_deassert_delay);
--
2.39.2
Split phy_device_create() into local helper functions. This commit is in
preparation of fixing the phy reset handling. No functional changes for
the public API users.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/phy/phy_device.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 917ba84105fc..dd0aaa866d17 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -629,13 +629,10 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
return 0;
}
-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)
+static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr)
{
struct phy_device *dev;
struct mdio_device *mdiodev;
- int ret = 0;
/* We allocate the device, and initialize the default values */
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
@@ -653,6 +650,15 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
mdiodev->device_free = phy_mdio_device_free;
mdiodev->device_remove = phy_mdio_device_remove;
+ dev_set_name(&mdiodev->dev, PHY_ID_FMT, bus->id, addr);
+ device_initialize(&mdiodev->dev);
+
+ return dev;
+}
+
+static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45,
+ struct phy_c45_device_ids *c45_ids)
+{
dev->speed = SPEED_UNKNOWN;
dev->duplex = DUPLEX_UNKNOWN;
dev->pause = 0;
@@ -670,9 +676,6 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
dev->c45_ids = *c45_ids;
dev->irq = bus->irq[addr];
- dev_set_name(&mdiodev->dev, PHY_ID_FMT, bus->id, addr);
- device_initialize(&mdiodev->dev);
-
dev->state = PHY_DOWN;
mutex_init(&dev->lock);
@@ -690,7 +693,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
*/
if (is_c45 && c45_ids) {
const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
- int i;
+ int ret, i;
for (i = 1; i < num_ids; i++) {
if (c45_ids->device_ids[i] == 0xffffffff)
@@ -699,14 +702,29 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
ret = phy_request_driver_module(dev,
c45_ids->device_ids[i]);
if (ret)
- break;
+ return ret;
}
- } else {
- ret = phy_request_driver_module(dev, phy_id);
+
+ return 0;
}
+ return phy_request_driver_module(dev, phy_id);
+}
+
+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)
+{
+ struct phy_device *dev;
+ int ret;
+
+ dev = phy_device_alloc(bus, addr);
+ if (IS_ERR(dev))
+ return dev;
+
+ ret = phy_device_init(dev, phy_id, is_c45, c45_ids);
if (ret) {
- put_device(&mdiodev->dev);
+ put_device(&dev->mdio.dev);
dev = ERR_PTR(ret);
}
--
2.39.2
Currently the usually way to probe and setup a phy is done via:
1) get_phy_device()/phy_device_create()
2) phy_device_register.
During get_phy_device() the PHYID1/2 registers are read which assumes
that the phy is already accessible. This is not always the case, e.g.
- if the pre-running firmware did not initialize the phy or
- if the kernel does gate important clocks while booting and the phy
isn't accessible after the pre-running firmware anymore.
To fix this we need to:
- parse the phy's fwnode first,
- do some basic setup like: bring it out of the reset state and
- finally read the PHYID1/2 registers to probe the correct driver
This patch adds a new helper called phy_device_atomic_register() to not
break exisiting running systems based on the current mdio/phy handling.
This new helper bundles all required steps into a single function to
make it easier for driver developers.
To bundle the phy firmware parsing step within phx_device.c the commit
copies the required code from fwnode_mdio.c. After we converterd all
callers of fwnode_mdiobus_* to this new API we can remove the support
from fwnode_mdio.c.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/phy/phy_device.c | 208 +++++++++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 9 ++
2 files changed, 217 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7e4b3b3caba9..a784ac06e6a9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3124,6 +3124,214 @@ struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode)
}
EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
+static int fwnode_setup_phy_irq(struct phy_device *phydev, struct mii_bus *bus,
+ struct fwnode_handle *fwnode)
+{
+ u32 addr = phydev->mdio.addr;
+ int ret;
+
+ if (is_acpi_node(fwnode)) {
+ phydev->irq = bus->irq[addr];
+ return 0;
+ }
+
+ /* of_node */
+ ret = fwnode_irq_get(fwnode, 0);
+ /* Don't wait forever if the IRQ provider doesn't become available,
+ * just fall back to poll mode
+ */
+ if (ret == -EPROBE_DEFER)
+ ret = driver_deferred_probe_check_state(&phydev->mdio.dev);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
+ if (ret > 0) {
+ phydev->irq = ret;
+ bus->irq[addr] = ret;
+ } else {
+ phydev->irq = bus->irq[addr];
+ }
+
+ return 0;
+}
+
+static struct pse_control *
+fwnode_find_pse_control(struct fwnode_handle *fwnode)
+{
+ struct pse_control *psec;
+ struct device_node *np;
+
+ if (!IS_ENABLED(CONFIG_PSE_CONTROLLER))
+ return NULL;
+
+ np = to_of_node(fwnode);
+ if (!np)
+ return NULL;
+
+ psec = of_pse_control_get(np);
+ if (PTR_ERR(psec) == -ENOENT)
+ return NULL;
+
+ return psec;
+}
+
+static struct mii_timestamper *
+fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
+{
+ struct of_phandle_args arg;
+ int err;
+
+ if (is_acpi_node(fwnode))
+ return NULL;
+
+ err = of_parse_phandle_with_fixed_args(to_of_node(fwnode),
+ "timestamper", 1, 0, &arg);
+ if (err == -ENOENT)
+ return NULL;
+ else if (err)
+ return ERR_PTR(err);
+
+ if (arg.args_count != 1)
+ return ERR_PTR(-EINVAL);
+
+ return register_mii_timestamper(arg.np, arg.args[0]);
+}
+
+static int
+phy_device_parse_fwnode(struct phy_device *phydev,
+ struct phy_device_config *config)
+{
+ struct fwnode_handle *fwnode = config->fwnode;
+ struct mii_bus *bus = config->mii_bus;
+ u32 addr = phydev->mdio.addr;
+ int ret;
+
+ if (!fwnode)
+ return 0;
+
+ if (!is_acpi_node(fwnode) && !is_of_node(fwnode))
+ return 0;
+
+ ret = fwnode_setup_phy_irq(phydev, bus, fwnode);
+ if (ret)
+ return ret;
+
+ ret = fwnode_property_match_string(fwnode, "compatible",
+ "ethernet-phy-ieee802.3-c45");
+ if (ret >= 0)
+ config->is_c45 = true;
+
+ if (fwnode_property_read_bool(fwnode, "broken-turn-around"))
+ bus->phy_ignore_ta_mask |= 1 << addr;
+ fwnode_property_read_u32(fwnode, "reset-assert-us",
+ &phydev->mdio.reset_assert_delay);
+ fwnode_property_read_u32(fwnode, "reset-deassert-us",
+ &phydev->mdio.reset_deassert_delay);
+
+ fwnode_handle_get(fwnode);
+ if (is_acpi_node(fwnode))
+ phydev->mdio.dev.fwnode = fwnode;
+ else if (is_of_node(fwnode))
+ device_set_node(&phydev->mdio.dev, fwnode);
+
+ phydev->psec = fwnode_find_pse_control(fwnode);
+ if (IS_ERR(phydev->psec)) {
+ ret = PTR_ERR(phydev->psec);
+ goto put_fwnode;
+ }
+
+ /* A mii_timestamper probed via the device tree will have precedence. */
+ phydev->mii_ts = fwnode_find_mii_timestamper(fwnode);
+ if (IS_ERR(phydev->mii_ts)) {
+ ret = PTR_ERR(phydev->mii_ts);
+ goto put_pse;
+ }
+
+ return 0;
+
+put_pse:
+ pse_control_put(phydev->psec);
+put_fwnode:
+ fwnode_handle_put(phydev->mdio.dev.fwnode);
+
+ return ret;
+}
+
+/**
+ * phy_device_atomic_register - Setup, init and register a PHY on the MDIO bus
+ * @config: The PHY config
+ *
+ * Probe, initialise and register a PHY at @addr on @bus.
+ *
+ * Returns an allocated and registered &struct phy_device on success.
+ */
+struct phy_device *phy_device_atomic_register(struct phy_device_config *config)
+{
+ struct phy_c45_device_ids *c45_ids = &config->c45_ids;
+ struct phy_device *phydev;
+ int err;
+
+ phydev = phy_device_alloc(config);
+ if (IS_ERR(phydev))
+ return ERR_CAST(phydev);
+
+ err = phy_device_parse_fwnode(phydev, config);
+ if (err) {
+ phydev_err(phydev, "failed to parse fwnode\n");
+ goto err_free_phydev;
+ }
+
+ err = mdiobus_register_device(&phydev->mdio);
+ if (err) {
+ phydev_err(phydev, "pre-init step failed\n");
+ goto err_free_fwnode;
+ }
+
+ phy_device_reset(phydev, 0);
+
+ memset(c45_ids->device_ids, 0xff, sizeof(c45_ids->device_ids));
+
+ err = phy_device_detect(config);
+ if (err) {
+ phydev_err(phydev, "failed to query the phyid\n");
+ goto err_unregister_mdiodev;
+ }
+
+ err = phy_device_init(phydev, config);
+ if (err) {
+ phydev_err(phydev, "failed to initialize\n");
+ goto err_unregister_mdiodev;
+ }
+
+ err = phy_scan_fixups(phydev);
+ if (err) {
+ phydev_err(phydev, "failed to apply fixups\n");
+ goto err_unregister_mdiodev;
+ }
+
+ err = device_add(&phydev->mdio.dev);
+ if (err) {
+ phydev_err(phydev, "failed to add\n");
+ goto err_out;
+ }
+
+ return 0;
+
+err_out:
+ phy_device_reset(phydev, 1);
+err_unregister_mdiodev:
+ mdiobus_unregister_device(&phydev->mdio);
+err_free_fwnode:
+ unregister_mii_timestamper(phydev->mii_ts);
+ pse_control_put(phydev->psec);
+ fwnode_handle_put(phydev->mdio.dev.fwnode);
+err_free_phydev:
+ kfree(phydev);
+
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL(phy_device_atomic_register);
+
/**
* 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 0f0cb72a08ab..bdf6d27faefb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -761,6 +761,7 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
*
* @mii_bus: The target MII bus the PHY is connected to
* @phy_addr: PHY address on the MII bus
+ * @fwnode: The PHY firmware handle
* @phy_id: UID for this device found during discovery
* @c45_ids: 802.3-c45 Device Identifiers if is_c45.
* @is_c45: If true the PHY uses the 802.3 clause 45 protocol
@@ -774,6 +775,7 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
struct phy_device_config {
struct mii_bus *mii_bus;
int phy_addr;
+ struct fwnode_handle *fwnode;
u32 phy_id;
struct phy_c45_device_ids c45_ids;
bool is_c45;
@@ -1573,6 +1575,7 @@ struct phy_device *device_phy_find_device(struct device *dev);
struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
struct phy_device *get_phy_device(struct phy_device_config *config);
int phy_device_register(struct phy_device *phy);
+struct phy_device *phy_device_atomic_register(struct phy_device_config *config);
void phy_device_free(struct phy_device *phydev);
#else
static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
@@ -1613,6 +1616,12 @@ static inline int phy_device_register(struct phy_device *phy)
return 0;
}
+static inline
+struct phy_device *phy_device_atomic_register(struct phy_device_config *config)
+{
+ return NULL;
+}
+
static inline void phy_device_free(struct phy_device *phydev) { }
#endif /* CONFIG_PHYLIB */
void phy_device_remove(struct phy_device *phydev);
--
2.39.2
Use the new atomic API to setup and register the phy accordingly.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/phy/nxp-tja11xx.c | 31 +++++++------------------------
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index 2a4c0f6d74eb..af9cb5e1a7ee 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -561,6 +561,8 @@ static void tja1102_p1_register(struct work_struct *work)
/* Real PHY ID of Port 1 is 0 */
.phy_id = PHY_ID_TJA1102,
.phy_id_broken = true,
+ .parent_mdiodev = dev,
+ .fwnode = of_fwnode_handle(child),
};
struct phy_device *phy;
@@ -583,30 +585,11 @@ static void tja1102_p1_register(struct work_struct *work)
continue;
}
- phy = phy_device_create(&config);
- if (IS_ERR(phy)) {
- dev_err(dev, "Can't create PHY device for Port 1: %i\n",
- config.phy_addr);
- continue;
- }
-
- /* Overwrite parent device. phy_device_create() set parent to
- * the mii_bus->dev, which is not correct in case.
- */
- phy->mdio.dev.parent = dev;
-
- ret = of_mdiobus_phy_device_register(bus, phy, child,
- config.phy_addr);
- if (ret) {
- /* All resources needed for Port 1 should be already
- * available for Port 0. Both ports use the same
- * interrupt line, so -EPROBE_DEFER would make no sense
- * here.
- */
- dev_err(dev, "Can't register Port 1. Unexpected error: %i\n",
- ret);
- phy_device_free(phy);
- }
+ phy = phy_device_atomic_register(&config);
+ if (IS_ERR(phy))
+ dev_err_probe(dev, PTR_ERR(phy),
+ "Can't create PHY device for Port 1: %i\n",
+ config.phy_addr);
}
}
--
2.39.2
Currently these two public APIs are used to create a 'struct phy_device'
device. In addition to that the device get_phy_device() can adapt the
is_c45 parameter as well if supprted by the mii bus.
Both APIs do have a different set of parameters which can be unified to
upcoming API extension. For this the 'struct phy_device_config' is
introduced which is the only parameter for both functions. This new
mechnism also adds the possiblity to read the configuration back within
the driver for possible validation checks.
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/net/ethernet/adi/adin1110.c | 6 ++-
drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +++-
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +++--
drivers/net/ethernet/socionext/netsec.c | 7 ++-
drivers/net/mdio/fwnode_mdio.c | 13 +++---
drivers/net/mdio/mdio-xgene.c | 6 ++-
drivers/net/phy/fixed_phy.c | 6 ++-
drivers/net/phy/mdio_bus.c | 7 ++-
drivers/net/phy/nxp-tja11xx.c | 23 +++++-----
drivers/net/phy/phy_device.c | 55 ++++++++++++-----------
drivers/net/phy/sfp.c | 7 ++-
include/linux/phy.h | 29 +++++++++---
12 files changed, 121 insertions(+), 57 deletions(-)
diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
index 3f316a0f4158..01b0c2a3a294 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -1565,6 +1565,9 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv)
{
struct device *dev = &priv->spidev->dev;
struct adin1110_port_priv *port_priv;
+ struct phy_device_config config = {
+ .mii_bus = priv->mii_bus,
+ };
struct net_device *netdev;
int ret;
int i;
@@ -1599,7 +1602,8 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv)
netdev->priv_flags |= IFF_UNICAST_FLT;
netdev->features |= NETIF_F_NETNS_LOCAL;
- port_priv->phydev = get_phy_device(priv->mii_bus, i + 1, false);
+ config.phy_addr = i + 1;
+ port_priv->phydev = get_phy_device(&config);
if (IS_ERR(port_priv->phydev)) {
netdev_err(netdev, "Could not find PHY with device address: %d.\n", i);
return PTR_ERR(port_priv->phydev);
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 16e7fb2c0dae..27ba903bbd0a 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -1056,6 +1056,10 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata)
{
struct ethtool_link_ksettings *lks = &pdata->phy.lks;
struct xgbe_phy_data *phy_data = pdata->phy_data;
+ struct phy_device_config config = {
+ .mii_bus = phy_data->mii,
+ .phy_addr = phy_data->mdio_addr,
+ };
struct phy_device *phydev;
int ret;
@@ -1086,8 +1090,8 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata)
}
/* Create and connect to the PHY device */
- phydev = get_phy_device(phy_data->mii, phy_data->mdio_addr,
- (phy_data->phydev_mode == XGBE_MDIO_MODE_CL45));
+ config.is_c45 = phy_data->phydev_mode == XGBE_MDIO_MODE_CL45;
+ phydev = get_phy_device(&config);
if (IS_ERR(phydev)) {
netdev_err(pdata->netdev, "get_phy_device failed\n");
return -ENODEV;
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index 928d934cb21a..3c41c4db5d9b 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -687,9 +687,12 @@ static int
hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
u32 addr)
{
+ struct phy_device_config config = {
+ .mii_bus = mdio,
+ .phy_addr = addr,
+ };
struct phy_device *phy;
const char *phy_type;
- bool is_c45;
int rc;
rc = fwnode_property_read_string(mac_cb->fw_port,
@@ -698,13 +701,13 @@ hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
return rc;
if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_XGMII)))
- is_c45 = true;
+ config.is_c45 = true;
else if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_SGMII)))
- is_c45 = false;
+ config.is_c45 = false;
else
return -ENODATA;
- phy = get_phy_device(mdio, addr, is_c45);
+ phy = get_phy_device(&config);
if (!phy || IS_ERR(phy))
return -EIO;
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 2d7347b71c41..a0786dfb827a 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1948,6 +1948,11 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
return ret;
}
} else {
+ struct phy_device_config config = {
+ .mii_bus = bus,
+ .phy_addr = phy_addr,
+ };
+
/* Mask out all PHYs from auto probing. */
bus->phy_mask = ~0;
ret = mdiobus_register(bus);
@@ -1956,7 +1961,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
return ret;
}
- priv->phydev = get_phy_device(bus, phy_addr, false);
+ priv->phydev = get_phy_device(&config);
if (IS_ERR(priv->phydev)) {
ret = PTR_ERR(priv->phydev);
dev_err(priv->dev, "get_phy_device err(%d)\n", ret);
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..47ef702d4ffd 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -112,10 +112,13 @@ EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register);
int fwnode_mdiobus_register_phy(struct mii_bus *bus,
struct fwnode_handle *child, u32 addr)
{
+ struct phy_device_config config = {
+ .mii_bus = bus,
+ .phy_addr = addr,
+ };
struct mii_timestamper *mii_ts = NULL;
struct pse_control *psec = NULL;
struct phy_device *phy;
- bool is_c45;
u32 phy_id;
int rc;
@@ -129,11 +132,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
goto clean_pse;
}
- is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
- if (is_c45 || fwnode_get_phy_id(child, &phy_id))
- phy = get_phy_device(bus, addr, is_c45);
+ config.is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
+ if (config.is_c45 || fwnode_get_phy_id(child, &config.phy_id))
+ phy = get_phy_device(&config);
else
- phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+ phy = phy_device_create(&config);
if (IS_ERR(phy)) {
rc = PTR_ERR(phy);
goto clean_mii_ts;
diff --git a/drivers/net/mdio/mdio-xgene.c b/drivers/net/mdio/mdio-xgene.c
index 7aafc221b5cf..6754c35aa6c3 100644
--- a/drivers/net/mdio/mdio-xgene.c
+++ b/drivers/net/mdio/mdio-xgene.c
@@ -262,9 +262,13 @@ static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg)
struct phy_device *xgene_enet_phy_register(struct mii_bus *bus, int phy_addr)
{
+ struct phy_device_config config = {
+ .mii_bus = bus,
+ .phy_addr = phy_addr,
+ };
struct phy_device *phy_dev;
- phy_dev = get_phy_device(bus, phy_addr, false);
+ phy_dev = get_phy_device(&config);
if (!phy_dev || IS_ERR(phy_dev))
return NULL;
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index aef739c20ac4..d217301a71d1 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -229,6 +229,9 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
struct gpio_desc *gpiod)
{
struct fixed_mdio_bus *fmb = &platform_fmb;
+ struct phy_device_config config = {
+ .mii_bus = fmb->mii_bus,
+ };
struct phy_device *phy;
int phy_addr;
int ret;
@@ -254,7 +257,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
return ERR_PTR(ret);
}
- phy = get_phy_device(fmb->mii_bus, phy_addr, false);
+ config.phy_addr = phy_addr;
+ phy = get_phy_device(&config);
if (IS_ERR(phy)) {
fixed_phy_del(phy_addr);
return ERR_PTR(-EINVAL);
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 389f33a12534..8390db7ae4bc 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -516,9 +516,14 @@ static int mdiobus_create_device(struct mii_bus *bus,
static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
{
struct phy_device *phydev = ERR_PTR(-ENODEV);
+ struct phy_device_config config = {
+ .mii_bus = bus,
+ .phy_addr = addr,
+ .is_c45 = c45,
+ };
int err;
- phydev = get_phy_device(bus, addr, c45);
+ phydev = get_phy_device(&config);
if (IS_ERR(phydev))
return phydev;
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index ec91e671f8aa..b5e03d66b7f5 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -554,17 +554,20 @@ static void tja1102_p1_register(struct work_struct *work)
struct device *dev = &phydev_phy0->mdio.dev;
struct device_node *np = dev->of_node;
struct device_node *child;
- int ret;
for_each_available_child_of_node(np, child) {
+ struct phy_device_config config = {
+ .mii_bus = bus,
+ /* Real PHY ID of Port 1 is 0 */
+ .phy_id = PHY_ID_TJA1102,
+ };
struct phy_device *phy;
- int addr;
- addr = of_mdio_parse_addr(dev, child);
- if (addr < 0) {
+ config.phy_addr = of_mdio_parse_addr(dev, child);
+ if (config.phy_addr < 0) {
dev_err(dev, "Can't parse addr\n");
continue;
- } else if (addr != phydev_phy0->mdio.addr + 1) {
+ } else if (config.phy_addr != phydev_phy0->mdio.addr + 1) {
/* Currently we care only about double PHY chip TJA1102.
* If some day NXP will decide to bring chips with more
* PHYs, this logic should be reworked.
@@ -574,16 +577,15 @@ static void tja1102_p1_register(struct work_struct *work)
continue;
}
- if (mdiobus_is_registered_device(bus, addr)) {
+ if (mdiobus_is_registered_device(bus, config.phy_addr)) {
dev_err(dev, "device is already registered\n");
continue;
}
- /* Real PHY ID of Port 1 is 0 */
- phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL);
+ phy = phy_device_create(&config);
if (IS_ERR(phy)) {
dev_err(dev, "Can't create PHY device for Port 1: %i\n",
- addr);
+ config.phy_addr);
continue;
}
@@ -592,7 +594,8 @@ static void tja1102_p1_register(struct work_struct *work)
*/
phy->mdio.dev.parent = dev;
- ret = of_mdiobus_phy_device_register(bus, phy, child, addr);
+ ret = of_mdiobus_phy_device_register(bus, phy, child,
+ config.phy_addr);
if (ret) {
/* All resources needed for Port 1 should be already
* available for Port 0. Both ports use the same
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e4d08dcfed70..bb465a324eef 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -629,8 +629,10 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
return 0;
}
-static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr)
+static struct phy_device *phy_device_alloc(struct phy_device_config *config)
{
+ struct mii_bus *bus = config->mii_bus;
+ int addr = config->phy_addr;
struct phy_device *dev;
struct mdio_device *mdiodev;
@@ -656,9 +658,15 @@ static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr)
return dev;
}
-static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45,
- struct phy_c45_device_ids *c45_ids)
+static int phy_device_init(struct phy_device *dev,
+ struct phy_device_config *config)
{
+ struct phy_c45_device_ids *c45_ids = &config->c45_ids;
+ struct mii_bus *bus = config->mii_bus;
+ bool is_c45 = config->is_c45;
+ u32 phy_id = config->phy_id;
+ int addr = config->phy_addr;
+
dev->speed = SPEED_UNKNOWN;
dev->duplex = DUPLEX_UNKNOWN;
dev->pause = 0;
@@ -711,18 +719,16 @@ static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45,
return phy_request_driver_module(dev, phy_id);
}
-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)
+struct phy_device *phy_device_create(struct phy_device_config *config)
{
struct phy_device *dev;
int ret;
- dev = phy_device_alloc(bus, addr);
+ dev = phy_device_alloc(config);
if (IS_ERR(dev))
return dev;
- ret = phy_device_init(dev, phy_id, is_c45, c45_ids);
+ ret = phy_device_init(dev, config);
if (ret) {
put_device(&dev->mdio.dev);
dev = ERR_PTR(ret);
@@ -954,12 +960,16 @@ int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
}
EXPORT_SYMBOL(fwnode_get_phy_id);
-static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
- u32 *phy_id, struct phy_c45_device_ids *c45_ids)
+static int phy_device_detect(struct phy_device_config *config)
{
+ struct phy_c45_device_ids *c45_ids = &config->c45_ids;
+ struct mii_bus *bus = config->mii_bus;
+ u32 *phy_id = &config->phy_id;
+ bool is_c45 = config->is_c45;
+ int addr = config->phy_addr;
int r;
- if (*is_c45)
+ if (is_c45)
r = get_phy_c45_ids(bus, addr, c45_ids);
else
r = get_phy_c22_id(bus, addr, phy_id);
@@ -972,8 +982,8 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
* probe with C45 to see if we're able to get a valid PHY ID in the C45
* space, if successful, create the C45 PHY device.
*/
- if (!*is_c45 && *phy_id == 0 && bus->read_c45) {
- *is_c45 = true;
+ if (!is_c45 && *phy_id == 0 && bus->read_c45) {
+ config->is_c45 = true;
return get_phy_c45_ids(bus, addr, c45_ids);
}
@@ -983,11 +993,9 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
/**
* get_phy_device - reads the specified PHY device and returns its @phy_device
* struct
- * @bus: the target MII bus
- * @addr: PHY address on the MII bus
- * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
+ * @config: The PHY device config
*
- * Probe for a PHY at @addr on @bus.
+ * Use the @config parameters to probe for a PHY.
*
* When probing for a clause 22 PHY, then read the ID registers. If we find
* a valid ID, allocate and return a &struct phy_device.
@@ -999,21 +1007,18 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
* Returns an allocated &struct phy_device on success, %-ENODEV if there is
* no PHY present, or %-EIO on bus access error.
*/
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
+struct phy_device *get_phy_device(struct phy_device_config *config)
{
- struct phy_c45_device_ids c45_ids;
- u32 phy_id = 0;
+ struct phy_c45_device_ids *c45_ids = &config->c45_ids;
int r;
- c45_ids.devices_in_package = 0;
- c45_ids.mmds_present = 0;
- memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
+ memset(c45_ids->device_ids, 0xff, sizeof(c45_ids->device_ids));
- r = phy_device_detect(bus, addr, &is_c45, &phy_id, &c45_ids);
+ r = phy_device_detect(config);
if (r)
return ERR_PTR(r);
- return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
+ return phy_device_create(config);
}
EXPORT_SYMBOL(get_phy_device);
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index f0fcb06fbe82..8fb0a1a49aaf 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1635,10 +1635,15 @@ static void sfp_sm_phy_detach(struct sfp *sfp)
static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
{
+ struct phy_device_config config = {
+ .mii_bus = sfp->i2c_mii,
+ .phy_addr = addr,
+ .is_c45 = is_c45,
+ };
struct phy_device *phy;
int err;
- phy = get_phy_device(sfp->i2c_mii, addr, is_c45);
+ phy = get_phy_device(&config);
if (phy == ERR_PTR(-ENODEV))
return PTR_ERR(phy);
if (IS_ERR(phy)) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c17a98712555..498f4dc7669d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -756,6 +756,27 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
return container_of(to_mdio_device(dev), struct phy_device, mdio);
}
+/**
+ * struct phy_device_config - Configuration of a PHY
+ *
+ * @mii_bus: The target MII bus the PHY is connected to
+ * @phy_addr: PHY address on the MII bus
+ * @phy_id: UID for this device found during discovery
+ * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
+ * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
+ *
+ * The struct contain possible configuration parameters for a PHY device which
+ * are used to setup the struct phy_device.
+ */
+
+struct phy_device_config {
+ struct mii_bus *mii_bus;
+ int phy_addr;
+ u32 phy_id;
+ struct phy_c45_device_ids c45_ids;
+ bool is_c45;
+};
+
/**
* struct phy_tdr_config - Configuration of a TDR raw test
*
@@ -1538,9 +1559,7 @@ int phy_modify_paged_changed(struct phy_device *phydev, int page, u32 regnum,
int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum,
u16 mask, u16 set);
-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);
+struct phy_device *phy_device_create(struct phy_device_config *config);
void phy_device_set_miits(struct phy_device *phydev,
struct mii_timestamper *mii_ts);
#if IS_ENABLED(CONFIG_PHYLIB)
@@ -1549,7 +1568,7 @@ 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(const struct fwnode_handle *fwnode);
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
+struct phy_device *get_phy_device(struct phy_device_config *config);
int phy_device_register(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
#else
@@ -1581,7 +1600,7 @@ struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
}
static inline
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
+struct phy_device *get_phy_device(struct phy_device_config *config)
{
return NULL;
}
--
2.39.2
> To bundle the phy firmware parsing step within phx_device.c the commit
> copies the required code from fwnode_mdio.c. After we converterd all
> callers of fwnode_mdiobus_* to this new API we can remove the support
> from fwnode_mdio.c.
Why bundle the code? Why not call it in fwnode_mdio.c?
The bundling in this patch makes it harder to see the interesting part
of this patch, how the reset is handled. That is what this whole
patchset is about, so you want the review focus to be on that.
Andrew
On Wed, Apr 05, 2023 at 11:26:56AM +0200, Marco Felsch wrote:
> Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port
> report 0 for the 2nd port. To fix this a driver needs to supply the
> phyid instead and tell the phy framework to not try to readout the
> phyid. The latter case is done via the new 'phy_id_broken' flag which
> tells the phy framework to skip phyid readout for the corresponding phy.
In general, we try to avoid work around for broken hardware in the
core. Please try to solve this within nxp-tja11xx.c.
Andrew
On 4/5/2023 5:27 AM, Andrew Lunn wrote:
> On Wed, Apr 05, 2023 at 11:26:56AM +0200, Marco Felsch wrote:
>> Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port
>> report 0 for the 2nd port. To fix this a driver needs to supply the
>> phyid instead and tell the phy framework to not try to readout the
>> phyid. The latter case is done via the new 'phy_id_broken' flag which
>> tells the phy framework to skip phyid readout for the corresponding phy.
>
> In general, we try to avoid work around for broken hardware in the
> core. Please try to solve this within nxp-tja11xx.c.
Agreed, and one way to solve working around broken PHY identification
registers is to provide them through the compatible string via
"ethernet-phyAAAA.BBBB". This forces the PHY library not to read from
those registers yet instantiate the PHY device and force it to bind to a
certain phy_driver.
--
Florian
On Wed, Apr 05, 2023 at 11:26:51AM +0200, Marco Felsch wrote:
> The current phy reset handling is broken in a way that it needs
> pre-running firmware to setup the phy initially. Since the very first
> step is to readout the PHYID1/2 registers before doing anything else.
>
> The whole dection logic will fall apart if the pre-running firmware
> don't setup the phy accordingly or the kernel boot resets GPIOs states
> or disables clocks. In such cases the PHYID1/2 read access will fail and
> so the whole detection will fail.
>
> I fixed this via this series, the fix will include a new kernel API
> called phy_device_atomic_register() which will do all necessary things
> and return a 'struct phy_device' on success. So setting up a phy and the
> phy state machine is more convenient.
Please add a section explaining why the current API is broken beyond
repair. You need to justify adding a new call, rather than fixing the
existing code to just do what is necessary to allow the PHY to be
found.
Andrew
On Wed, Apr 05, 2023 at 11:27:03AM +0200, Marco Felsch wrote:
> There are phy's not mention any assert/deassert delay within their
> datasheets but the real world showed that this is not true. They need at
> least a few us to be accessible and to readout the register values. So
> add a sane default value of 1000us for both assert and deassert to fix
> this in a global matter.
>
> Signed-off-by: Marco Felsch <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
Hi Marco,
On 4/5/2023 2:26 AM, Marco Felsch wrote:
> The current phy reset handling is broken in a way that it needs
> pre-running firmware to setup the phy initially. Since the very first
> step is to readout the PHYID1/2 registers before doing anything else.
>
> The whole dection logic will fall apart if the pre-running firmware
> don't setup the phy accordingly or the kernel boot resets GPIOs states
> or disables clocks. In such cases the PHYID1/2 read access will fail and
> so the whole detection will fail.
PHY reset is a bit too broad and should need some clarifications between:
- external reset to the PHY whereby a hardware pin on the PHY IC may be used
- internal reset to the PHY whereby we call into the PHY driver
soft_reset function to have the PHY software reset itself
You are changing the way the former happens, not the latter, at least
not changing the latter intentionally if at all.
This is important because your cover letter will be in the merge commit
in the networking tree.
Will do a more thorough review on a patch by patch basis. Thanks.
>
> I fixed this via this series, the fix will include a new kernel API
> called phy_device_atomic_register() which will do all necessary things
> and return a 'struct phy_device' on success. So setting up a phy and the
> phy state machine is more convenient.
>
> I tested the series on a i.MX8MP-EVK and a custom board which have a
> TJA1102 dual-port ethernet phy. Other testers are welcome :)
>
> Regards,
> Marco
>
> Signed-off-by: Marco Felsch <[email protected]>
> ---
> Marco Felsch (12):
> net: phy: refactor phy_device_create function
> net: phy: refactor get_phy_device function
> net: phy: add phy_device_set_miits helper
> net: phy: unify get_phy_device and phy_device_create parameter list
> net: phy: add phy_id_broken support
> net: phy: add phy_device_atomic_register helper
> net: mdio: make use of phy_device_atomic_register helper
> net: phy: add possibility to specify mdio device parent
> net: phy: nxp-tja11xx: make use of phy_device_atomic_register()
> of: mdio: remove now unused of_mdiobus_phy_device_register()
> net: mdiobus: remove now unused fwnode helpers
> net: phy: add default gpio assert/deassert delay
>
> Documentation/firmware-guide/acpi/dsd/phy.rst | 2 +-
> MAINTAINERS | 1 -
> drivers/net/ethernet/adi/adin1110.c | 6 +-
> drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +-
> drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +-
> drivers/net/ethernet/socionext/netsec.c | 7 +-
> drivers/net/mdio/Kconfig | 7 -
> drivers/net/mdio/Makefile | 1 -
> drivers/net/mdio/acpi_mdio.c | 20 +-
> drivers/net/mdio/fwnode_mdio.c | 183 ------------
> drivers/net/mdio/mdio-xgene.c | 6 +-
> drivers/net/mdio/of_mdio.c | 23 +-
> drivers/net/phy/bcm-phy-ptp.c | 2 +-
> drivers/net/phy/dp83640.c | 2 +-
> drivers/net/phy/fixed_phy.c | 6 +-
> drivers/net/phy/mdio_bus.c | 7 +-
> drivers/net/phy/micrel.c | 2 +-
> drivers/net/phy/mscc/mscc_ptp.c | 2 +-
> drivers/net/phy/nxp-c45-tja11xx.c | 2 +-
> drivers/net/phy/nxp-tja11xx.c | 47 ++-
> drivers/net/phy/phy_device.c | 348 +++++++++++++++++++---
> drivers/net/phy/sfp.c | 7 +-
> include/linux/fwnode_mdio.h | 35 ---
> include/linux/of_mdio.h | 8 -
> include/linux/phy.h | 46 ++-
> 25 files changed, 442 insertions(+), 347 deletions(-)
> ---
> base-commit: 054fbf7ff8143d35ca7d3bb5414bb44ee1574194
> change-id: 20230405-net-next-topic-net-phy-reset-4f79ff7df4a0
>
> Best regards,
--
Florian
Hi Florian,
On 23-04-05, Florian Fainelli wrote:
> Hi Marco,
>
> On 4/5/2023 2:26 AM, Marco Felsch wrote:
> > The current phy reset handling is broken in a way that it needs
> > pre-running firmware to setup the phy initially. Since the very first
> > step is to readout the PHYID1/2 registers before doing anything else.
> >
> > The whole dection logic will fall apart if the pre-running firmware
> > don't setup the phy accordingly or the kernel boot resets GPIOs states
> > or disables clocks. In such cases the PHYID1/2 read access will fail and
> > so the whole detection will fail.
>
> PHY reset is a bit too broad and should need some clarifications between:
>
> - external reset to the PHY whereby a hardware pin on the PHY IC may be used
>
> - internal reset to the PHY whereby we call into the PHY driver soft_reset
> function to have the PHY software reset itself
>
> You are changing the way the former happens, not the latter, at least not
> changing the latter intentionally if at all.
Yes.
> This is important because your cover letter will be in the merge commit in
> the networking tree.
Ah okay, I didn't know that. I will adapt the cover letter accordingly.
> Will do a more thorough review on a patch by patch basis. Thanks.
Thanks a lot, looking forward to it.
Regards,
Marco
On 23-04-05, Andrew Lunn wrote:
> > To bundle the phy firmware parsing step within phx_device.c the commit
> > copies the required code from fwnode_mdio.c. After we converterd all
> > callers of fwnode_mdiobus_* to this new API we can remove the support
> > from fwnode_mdio.c.
>
> Why bundle the code? Why not call it in fwnode_mdio.c?
The current fwnode_mdio.c don't provide the proper helper functions yet.
Instead the parsing is spread between fwnode_mdiobus_register_phy() and
fwnode_mdiobus_phy_device_register(). Of course these can be extracted
and exported but I don't see the benefit. IMHO it just cause jumping
around files and since fwnode is a proper firmware abstraction we could
use is directly wihin core/lib files.
> The bundling in this patch makes it harder to see the interesting part
> of this patch, how the reset is handled. That is what this whole
> patchset is about, so you want the review focus to be on that.
I know and I thought about adding the firmware parsing helpers first but
than I went this way. I can split this of course to make the patch
smaller.
Regards,
Marco
>
> Andrew
>
On 23-04-05, Florian Fainelli wrote:
> On 4/5/2023 5:27 AM, Andrew Lunn wrote:
> > On Wed, Apr 05, 2023 at 11:26:56AM +0200, Marco Felsch wrote:
> > > Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port
> > > report 0 for the 2nd port. To fix this a driver needs to supply the
> > > phyid instead and tell the phy framework to not try to readout the
> > > phyid. The latter case is done via the new 'phy_id_broken' flag which
> > > tells the phy framework to skip phyid readout for the corresponding phy.
> >
> > In general, we try to avoid work around for broken hardware in the
> > core. Please try to solve this within nxp-tja11xx.c.
>
> Agreed, and one way to solve working around broken PHY identification
> registers is to provide them through the compatible string via
> "ethernet-phyAAAA.BBBB". This forces the PHY library not to read from those
> registers yet instantiate the PHY device and force it to bind to a certain
> phy_driver.
The nxp-tja11xx.c is a bit special in case of two-port devices since the
2nd port registers a 2nd phy device which is correct but don't have a
dedicated compatible and so on. My 2nd idea here was to check if phy_id
is !0 and in this case just use it. I went this way to make it a bit
more explicit.
Regards,
Marco
> --
> Florian
>
Hi Andrew,
On 23-04-05, Andrew Lunn wrote:
> On Wed, Apr 05, 2023 at 11:26:51AM +0200, Marco Felsch wrote:
> > The current phy reset handling is broken in a way that it needs
> > pre-running firmware to setup the phy initially. Since the very first
> > step is to readout the PHYID1/2 registers before doing anything else.
> >
> > The whole dection logic will fall apart if the pre-running firmware
> > don't setup the phy accordingly or the kernel boot resets GPIOs states
> > or disables clocks. In such cases the PHYID1/2 read access will fail and
> > so the whole detection will fail.
> >
> > I fixed this via this series, the fix will include a new kernel API
> > called phy_device_atomic_register() which will do all necessary things
> > and return a 'struct phy_device' on success. So setting up a phy and the
> > phy state machine is more convenient.
>
> Please add a section explaining why the current API is broken beyond
> repair. You need to justify adding a new call, rather than fixing the
> existing code to just do what is necessary to allow the PHY to be
> found.
TIL from Florian that you use the cover-letter information in your merge
commits. I will adapt the cover-letter accordingly and mention why this
PR introduces a new API.
Regards,
Marco
>
> Andrew
>
> The current fwnode_mdio.c don't provide the proper helper functions yet.
> Instead the parsing is spread between fwnode_mdiobus_register_phy() and
> fwnode_mdiobus_phy_device_register(). Of course these can be extracted
> and exported but I don't see the benefit. IMHO it just cause jumping
> around files and since fwnode is a proper firmware abstraction we could
> use is directly wihin core/lib files.
No, assuming fwnode is the proper firmware abstraction is wrong. You
need to be very careful any time you convert of_ to fwnode_ and look
at the history of every property. Look at the number of deprecated OF
properties in Documentation/devicetree/bindings. They should never be
moved to fwnode_ because then you are moving deprecated properties to
ACPI, which never had them in the first place! You cannot assume DT
and ACPI are the same thing, have the same binding. And the same is
true, in theory, in the opposite direction. We don't want the DT
properties polluted with ACPI only properties. Not that anybody takes
ACPI seriously in networking.
> I know and I thought about adding the firmware parsing helpers first but
> than I went this way. I can split this of course to make the patch
> smaller.
Please do. Also, i read your commit message thinking it was a straight
copy of the code, and hence i did not need to review the code. But in
fact it is new code. So i need to take a close look at it.
But what i think is most important for this patchset is the
justification for not fixing the current API. Why is it broken beyond
repair?
Andrew
> The nxp-tja11xx.c is a bit special in case of two-port devices since the
> 2nd port registers a 2nd phy device which is correct but don't have a
> dedicated compatible and so on. My 2nd idea here was to check if phy_id
> is !0 and in this case just use it. I went this way to make it a bit
> more explicit.
What is actually wrong with the current solution? It is nicely hidden
away in the driver, where workarounds for broken hardware should
be. If you have found device 1 of 2, does that not suggest its resets
are already in a good state and nothing needs to be done for the
second PHY? So just register the second PHY with the code from within
the driver.
Andrew
On 23-04-05, Andrew Lunn wrote:
> > The current fwnode_mdio.c don't provide the proper helper functions yet.
> > Instead the parsing is spread between fwnode_mdiobus_register_phy() and
> > fwnode_mdiobus_phy_device_register(). Of course these can be extracted
> > and exported but I don't see the benefit. IMHO it just cause jumping
> > around files and since fwnode is a proper firmware abstraction we could
> > use is directly wihin core/lib files.
>
> No, assuming fwnode is the proper firmware abstraction is wrong. You
> need to be very careful any time you convert of_ to fwnode_ and look
> at the history of every property. Look at the number of deprecated OF
> properties in Documentation/devicetree/bindings. They should never be
> moved to fwnode_ because then you are moving deprecated properties to
> ACPI, which never had them in the first place!
The handling of deprecated properties is always a pain. Drivers handling
deprecated properties correctly for of_ should handle it correctly for
fwnode_ too. IMHO it would be driver bug if not existing deprecated
properties cause an error. Of course there will be properties which
need special attention for ACPI case but I don't see a problem for
deprecated properties since those must be handled correctly for of_ case
too.
> You cannot assume DT and ACPI are the same thing, have the same
> binding. And the same is true, in theory, in the opposite direction.
> We don't want the DT properties polluted with ACPI only properties.
> Not that anybody takes ACPI seriously in networking.
My assumption was that ACPI is becoming more closer to OF and the
fwnode/device abstraction is the abstraction to have one driver
interacting correctly with OF and ACPI. As I said above there will be
some corner-cases which need special attention of course :/
Also while answering this mail, I noticed that there are already some
'small' fwnode/device_ helpers within phy_device.c. So why not bundling
everything within phy_device.c?
> > I know and I thought about adding the firmware parsing helpers first but
> > than I went this way. I can split this of course to make the patch
> > smaller.
>
> Please do. Also, i read your commit message thinking it was a straight
> copy of the code, and hence i did not need to review the code. But in
> fact it is new code. So i need to take a close look at it.
>
> But what i think is most important for this patchset is the
> justification for not fixing the current API. Why is it broken beyond
> repair?
Currently we have one API which creates/allocates the 'struct
phy_device' and intialize the state which is:
- phy_device_create()
This function requests a driver based on the phy_id/c45_ids. The ID have
to come from somewhere if autodection is used. For autodetection case
- get_phy_device()
is called. This function try to access the phy without taken possible
hardware dependencies into account. These dependecies can be reset-lines
(in my case), clocks, supplies, ...
For taking fwnode (and possible dependencies) into account fwnode_mdio.c
was written which provides two helpers:
- fwnode_mdiobus_register_phy()
- fwnode_mdiobus_phy_device_register().
The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around
fwnode_mdiobus_register_phy().
fwnode_mdiobus_register_phy():
1st) calls get_phy_device() in case of autodection or c45. If phy_id
is provided and !c45 case phy_device_create() is called to get a
'struct phy_device'
- The autodection/c45 case try to access the PHYID registers
which is not possible, please see above.
2nd) call fwnode_mdiobus_phy_device_register() or
phy_device_register() directly.
- phy_device_register() is the first time we taking the possible
hardware reset line into account, which is far to late.
fwnode_mdiobus_phy_device_register():
- takes a 'struct phy_device' as parameter, again this have to come
from somewhere.
- calls phy_device_register() which is taken the possibel hardware
reset into account, again to late.
Why do I need the autodection? Because PHYs can be changed due to EOL,
cheaper device, ... I don't wanna have a new devicetree/firmware for the
updated product, just let the magic happen :)
Why do I introduce a new API?
1st) There are working users of get_phy_device() and I don't wanna
break their systems, so I left this logic untouched.
2nd) The fwnode API is replaced by this new one, since it is
broken (please see above).
3rd) IMHO the 'phy request/phy create' handling is far to complex
therefore I introduced a single API which:
- intialize all structures and states
- prepare the device for interaction by using fwnode
- initialize/detect the device and requests the coorect phy
module
- applies the fixups
- add the device to the kernel
- finally return the 'struct phy_device' to the user, so the
driver can do $stuff.
4th) The new 'struct phy_device_config' makes it easier to
adapt/extend the API.
Thanks a lot for your fast response and feedback :)
Regards,
Marco
> Currently we have one API which creates/allocates the 'struct
> phy_device' and intialize the state which is:
> - phy_device_create()
>
> This function requests a driver based on the phy_id/c45_ids. The ID have
> to come from somewhere if autodection is used. For autodetection case
> - get_phy_device()
>
> is called. This function try to access the phy without taken possible
> hardware dependencies into account. These dependecies can be reset-lines
> (in my case), clocks, supplies, ...
>
> For taking fwnode (and possible dependencies) into account fwnode_mdio.c
> was written which provides two helpers:
> - fwnode_mdiobus_register_phy()
> - fwnode_mdiobus_phy_device_register().
>
> The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around
> fwnode_mdiobus_register_phy().
It seems to me that the real problem is that mdio_device_reset() takes
an mdio_device. mdiobus_register_gpiod() and mdiobus_register_reset()
also take an mdio_device. These are the functions you want to call
before calling of_mdiobus_register_phy() in __of_mdiobus_register() to
ensure the PHY is out of reset. But you don't have an mdio_device yet.
So i think a better solution is to refactor this code. Move the
resources into a structure of their own, and make that a member of
mdio_device. You can create a stack version of this resource structure
in __of_mdiobus_register(), parse DT to fill it out by calling
mdiobus_register_gpiod() and mdiobus_register_reset() taking this new
structure, take it out of reset by calling mdio_device_reset(), and
then call of_mdiobus_register_phy(). If a PHY is found, copy the
values in the resulting mdio_device. If not, release the resources.
Doing it like this means there is no API change.
Andrew
On 23-04-05, Andrew Lunn wrote:
> > Currently we have one API which creates/allocates the 'struct
> > phy_device' and intialize the state which is:
> > - phy_device_create()
> >
> > This function requests a driver based on the phy_id/c45_ids. The ID have
> > to come from somewhere if autodection is used. For autodetection case
> > - get_phy_device()
> >
> > is called. This function try to access the phy without taken possible
> > hardware dependencies into account. These dependecies can be reset-lines
> > (in my case), clocks, supplies, ...
> >
> > For taking fwnode (and possible dependencies) into account fwnode_mdio.c
> > was written which provides two helpers:
> > - fwnode_mdiobus_register_phy()
> > - fwnode_mdiobus_phy_device_register().
> >
> > The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around
> > fwnode_mdiobus_register_phy().
>
> It seems to me that the real problem is that mdio_device_reset() takes
> an mdio_device. mdiobus_register_gpiod() and mdiobus_register_reset()
> also take an mdio_device. These are the functions you want to call
> before calling of_mdiobus_register_phy() in __of_mdiobus_register() to
> ensure the PHY is out of reset. But you don't have an mdio_device yet.
Of course, this was the problem as well and therefore I did the split in
the first two patches, to differentiate between allocation and init.
> So i think a better solution is to refactor this code. Move the
> resources into a structure of their own, and make that a member of
> mdio_device.
Sorry I can't follow you here, I did the refactoring already to
differentiate between phy_device_alloc() and phy_device_init(). The
parse and registration happen in between, like you descriped below. I
didn't changed/touched the mdio_device and phy_device structs since
those structs are very open and can be adapted by every driver.
> You can create a stack version of this resource structure
> in __of_mdiobus_register(), parse DT to fill it out by calling
> mdiobus_register_gpiod() and mdiobus_register_reset() taking this new
> structure, take it out of reset by calling mdio_device_reset(), and
> then call of_mdiobus_register_phy(). If a PHY is found, copy the
> values in the resulting mdio_device. If not, release the resources.
It is not just the reset, my approach would be the ground work for
adding support of other resources to, which are not handled yet. e.g.
phy-supply, clocks, pwdn-lines, ... With my approach it is far easier of
adding this
> Doing it like this means there is no API change.
Why is it that important? All users of the current fwnode API are
changed and even better, they are replaced in a 2:1 ratio. The new API
is the repaired version of the fwnode API which is already used by ACPI
and OF to register a phy_device. For all non-ACPI/OF users the new API
provides a way to allocate/identify and register a new phy within a
single call.
Regards,
Marco
+Raju
On 4/5/2023 2:56 PM, Marco Felsch wrote:
> Currently these two public APIs are used to create a 'struct phy_device'
> device. In addition to that the device get_phy_device() can adapt the
> is_c45 parameter as well if supprted by the mii bus.
>
> Both APIs do have a different set of parameters which can be unified to
> upcoming API extension. For this the 'struct phy_device_config' is
> introduced which is the only parameter for both functions. This new
> mechnism also adds the possiblity to read the configuration back within
> the driver for possible validation checks.
>
> Signed-off-by: Marco Felsch <[email protected]>
For the change on AMD XGBE driver.
Acked-by: Shyam Sundar S K <[email protected]>
> ---
> drivers/net/ethernet/adi/adin1110.c | 6 ++-
> drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +++-
> drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +++--
> drivers/net/ethernet/socionext/netsec.c | 7 ++-
> drivers/net/mdio/fwnode_mdio.c | 13 +++---
> drivers/net/mdio/mdio-xgene.c | 6 ++-
> drivers/net/phy/fixed_phy.c | 6 ++-
> drivers/net/phy/mdio_bus.c | 7 ++-
> drivers/net/phy/nxp-tja11xx.c | 23 +++++-----
> drivers/net/phy/phy_device.c | 55 ++++++++++++-----------
> drivers/net/phy/sfp.c | 7 ++-
> include/linux/phy.h | 29 +++++++++---
> 12 files changed, 121 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 3f316a0f4158..01b0c2a3a294 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c
> @@ -1565,6 +1565,9 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv)
> {
> struct device *dev = &priv->spidev->dev;
> struct adin1110_port_priv *port_priv;
> + struct phy_device_config config = {
> + .mii_bus = priv->mii_bus,
> + };
> struct net_device *netdev;
> int ret;
> int i;
> @@ -1599,7 +1602,8 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv)
> netdev->priv_flags |= IFF_UNICAST_FLT;
> netdev->features |= NETIF_F_NETNS_LOCAL;
>
> - port_priv->phydev = get_phy_device(priv->mii_bus, i + 1, false);
> + config.phy_addr = i + 1;
> + port_priv->phydev = get_phy_device(&config);
> if (IS_ERR(port_priv->phydev)) {
> netdev_err(netdev, "Could not find PHY with device address: %d.\n", i);
> return PTR_ERR(port_priv->phydev);
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index 16e7fb2c0dae..27ba903bbd0a 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> @@ -1056,6 +1056,10 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata)
> {
> struct ethtool_link_ksettings *lks = &pdata->phy.lks;
> struct xgbe_phy_data *phy_data = pdata->phy_data;
> + struct phy_device_config config = {
> + .mii_bus = phy_data->mii,
> + .phy_addr = phy_data->mdio_addr,
> + };
> struct phy_device *phydev;
> int ret;
>
> @@ -1086,8 +1090,8 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata)
> }
>
> /* Create and connect to the PHY device */
> - phydev = get_phy_device(phy_data->mii, phy_data->mdio_addr,
> - (phy_data->phydev_mode == XGBE_MDIO_MODE_CL45));
> + config.is_c45 = phy_data->phydev_mode == XGBE_MDIO_MODE_CL45;
> + phydev = get_phy_device(&config);
> if (IS_ERR(phydev)) {
> netdev_err(pdata->netdev, "get_phy_device failed\n");
> return -ENODEV;
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
> index 928d934cb21a..3c41c4db5d9b 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
> @@ -687,9 +687,12 @@ static int
> hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
> u32 addr)
> {
> + struct phy_device_config config = {
> + .mii_bus = mdio,
> + .phy_addr = addr,
> + };
> struct phy_device *phy;
> const char *phy_type;
> - bool is_c45;
> int rc;
>
> rc = fwnode_property_read_string(mac_cb->fw_port,
> @@ -698,13 +701,13 @@ hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
> return rc;
>
> if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_XGMII)))
> - is_c45 = true;
> + config.is_c45 = true;
> else if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_SGMII)))
> - is_c45 = false;
> + config.is_c45 = false;
> else
> return -ENODATA;
>
> - phy = get_phy_device(mdio, addr, is_c45);
> + phy = get_phy_device(&config);
> if (!phy || IS_ERR(phy))
> return -EIO;
>
> diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> index 2d7347b71c41..a0786dfb827a 100644
> --- a/drivers/net/ethernet/socionext/netsec.c
> +++ b/drivers/net/ethernet/socionext/netsec.c
> @@ -1948,6 +1948,11 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
> return ret;
> }
> } else {
> + struct phy_device_config config = {
> + .mii_bus = bus,
> + .phy_addr = phy_addr,
> + };
> +
> /* Mask out all PHYs from auto probing. */
> bus->phy_mask = ~0;
> ret = mdiobus_register(bus);
> @@ -1956,7 +1961,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
> return ret;
> }
>
> - priv->phydev = get_phy_device(bus, phy_addr, false);
> + priv->phydev = get_phy_device(&config);
> if (IS_ERR(priv->phydev)) {
> ret = PTR_ERR(priv->phydev);
> dev_err(priv->dev, "get_phy_device err(%d)\n", ret);
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index 1183ef5e203e..47ef702d4ffd 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -112,10 +112,13 @@ EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register);
> int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> struct fwnode_handle *child, u32 addr)
> {
> + struct phy_device_config config = {
> + .mii_bus = bus,
> + .phy_addr = addr,
> + };
> struct mii_timestamper *mii_ts = NULL;
> struct pse_control *psec = NULL;
> struct phy_device *phy;
> - bool is_c45;
> u32 phy_id;
> int rc;
>
> @@ -129,11 +132,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> goto clean_pse;
> }
>
> - is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
> - if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> - phy = get_phy_device(bus, addr, is_c45);
> + config.is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
> + if (config.is_c45 || fwnode_get_phy_id(child, &config.phy_id))
> + phy = get_phy_device(&config);
> else
> - phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + phy = phy_device_create(&config);
> if (IS_ERR(phy)) {
> rc = PTR_ERR(phy);
> goto clean_mii_ts;
> diff --git a/drivers/net/mdio/mdio-xgene.c b/drivers/net/mdio/mdio-xgene.c
> index 7aafc221b5cf..6754c35aa6c3 100644
> --- a/drivers/net/mdio/mdio-xgene.c
> +++ b/drivers/net/mdio/mdio-xgene.c
> @@ -262,9 +262,13 @@ static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>
> struct phy_device *xgene_enet_phy_register(struct mii_bus *bus, int phy_addr)
> {
> + struct phy_device_config config = {
> + .mii_bus = bus,
> + .phy_addr = phy_addr,
> + };
> struct phy_device *phy_dev;
>
> - phy_dev = get_phy_device(bus, phy_addr, false);
> + phy_dev = get_phy_device(&config);
> if (!phy_dev || IS_ERR(phy_dev))
> return NULL;
>
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index aef739c20ac4..d217301a71d1 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -229,6 +229,9 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
> struct gpio_desc *gpiod)
> {
> struct fixed_mdio_bus *fmb = &platform_fmb;
> + struct phy_device_config config = {
> + .mii_bus = fmb->mii_bus,
> + };
> struct phy_device *phy;
> int phy_addr;
> int ret;
> @@ -254,7 +257,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
> return ERR_PTR(ret);
> }
>
> - phy = get_phy_device(fmb->mii_bus, phy_addr, false);
> + config.phy_addr = phy_addr;
> + phy = get_phy_device(&config);
> if (IS_ERR(phy)) {
> fixed_phy_del(phy_addr);
> return ERR_PTR(-EINVAL);
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 389f33a12534..8390db7ae4bc 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -516,9 +516,14 @@ static int mdiobus_create_device(struct mii_bus *bus,
> static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
> {
> struct phy_device *phydev = ERR_PTR(-ENODEV);
> + struct phy_device_config config = {
> + .mii_bus = bus,
> + .phy_addr = addr,
> + .is_c45 = c45,
> + };
> int err;
>
> - phydev = get_phy_device(bus, addr, c45);
> + phydev = get_phy_device(&config);
> if (IS_ERR(phydev))
> return phydev;
>
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> index ec91e671f8aa..b5e03d66b7f5 100644
> --- a/drivers/net/phy/nxp-tja11xx.c
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -554,17 +554,20 @@ static void tja1102_p1_register(struct work_struct *work)
> struct device *dev = &phydev_phy0->mdio.dev;
> struct device_node *np = dev->of_node;
> struct device_node *child;
> - int ret;
>
> for_each_available_child_of_node(np, child) {
> + struct phy_device_config config = {
> + .mii_bus = bus,
> + /* Real PHY ID of Port 1 is 0 */
> + .phy_id = PHY_ID_TJA1102,
> + };
> struct phy_device *phy;
> - int addr;
>
> - addr = of_mdio_parse_addr(dev, child);
> - if (addr < 0) {
> + config.phy_addr = of_mdio_parse_addr(dev, child);
> + if (config.phy_addr < 0) {
> dev_err(dev, "Can't parse addr\n");
> continue;
> - } else if (addr != phydev_phy0->mdio.addr + 1) {
> + } else if (config.phy_addr != phydev_phy0->mdio.addr + 1) {
> /* Currently we care only about double PHY chip TJA1102.
> * If some day NXP will decide to bring chips with more
> * PHYs, this logic should be reworked.
> @@ -574,16 +577,15 @@ static void tja1102_p1_register(struct work_struct *work)
> continue;
> }
>
> - if (mdiobus_is_registered_device(bus, addr)) {
> + if (mdiobus_is_registered_device(bus, config.phy_addr)) {
> dev_err(dev, "device is already registered\n");
> continue;
> }
>
> - /* Real PHY ID of Port 1 is 0 */
> - phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL);
> + phy = phy_device_create(&config);
> if (IS_ERR(phy)) {
> dev_err(dev, "Can't create PHY device for Port 1: %i\n",
> - addr);
> + config.phy_addr);
> continue;
> }
>
> @@ -592,7 +594,8 @@ static void tja1102_p1_register(struct work_struct *work)
> */
> phy->mdio.dev.parent = dev;
>
> - ret = of_mdiobus_phy_device_register(bus, phy, child, addr);
> + ret = of_mdiobus_phy_device_register(bus, phy, child,
> + config.phy_addr);
> if (ret) {
> /* All resources needed for Port 1 should be already
> * available for Port 0. Both ports use the same
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e4d08dcfed70..bb465a324eef 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -629,8 +629,10 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
> return 0;
> }
>
> -static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr)
> +static struct phy_device *phy_device_alloc(struct phy_device_config *config)
> {
> + struct mii_bus *bus = config->mii_bus;
> + int addr = config->phy_addr;
> struct phy_device *dev;
> struct mdio_device *mdiodev;
>
> @@ -656,9 +658,15 @@ static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr)
> return dev;
> }
>
> -static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45,
> - struct phy_c45_device_ids *c45_ids)
> +static int phy_device_init(struct phy_device *dev,
> + struct phy_device_config *config)
> {
> + struct phy_c45_device_ids *c45_ids = &config->c45_ids;
> + struct mii_bus *bus = config->mii_bus;
> + bool is_c45 = config->is_c45;
> + u32 phy_id = config->phy_id;
> + int addr = config->phy_addr;
> +
> dev->speed = SPEED_UNKNOWN;
> dev->duplex = DUPLEX_UNKNOWN;
> dev->pause = 0;
> @@ -711,18 +719,16 @@ static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45,
> return phy_request_driver_module(dev, phy_id);
> }
>
> -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)
> +struct phy_device *phy_device_create(struct phy_device_config *config)
> {
> struct phy_device *dev;
> int ret;
>
> - dev = phy_device_alloc(bus, addr);
> + dev = phy_device_alloc(config);
> if (IS_ERR(dev))
> return dev;
>
> - ret = phy_device_init(dev, phy_id, is_c45, c45_ids);
> + ret = phy_device_init(dev, config);
> if (ret) {
> put_device(&dev->mdio.dev);
> dev = ERR_PTR(ret);
> @@ -954,12 +960,16 @@ int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
> }
> EXPORT_SYMBOL(fwnode_get_phy_id);
>
> -static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
> - u32 *phy_id, struct phy_c45_device_ids *c45_ids)
> +static int phy_device_detect(struct phy_device_config *config)
> {
> + struct phy_c45_device_ids *c45_ids = &config->c45_ids;
> + struct mii_bus *bus = config->mii_bus;
> + u32 *phy_id = &config->phy_id;
> + bool is_c45 = config->is_c45;
> + int addr = config->phy_addr;
> int r;
>
> - if (*is_c45)
> + if (is_c45)
> r = get_phy_c45_ids(bus, addr, c45_ids);
> else
> r = get_phy_c22_id(bus, addr, phy_id);
> @@ -972,8 +982,8 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
> * probe with C45 to see if we're able to get a valid PHY ID in the C45
> * space, if successful, create the C45 PHY device.
> */
> - if (!*is_c45 && *phy_id == 0 && bus->read_c45) {
> - *is_c45 = true;
> + if (!is_c45 && *phy_id == 0 && bus->read_c45) {
> + config->is_c45 = true;
> return get_phy_c45_ids(bus, addr, c45_ids);
> }
>
> @@ -983,11 +993,9 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
> /**
> * get_phy_device - reads the specified PHY device and returns its @phy_device
> * struct
> - * @bus: the target MII bus
> - * @addr: PHY address on the MII bus
> - * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
> + * @config: The PHY device config
> *
> - * Probe for a PHY at @addr on @bus.
> + * Use the @config parameters to probe for a PHY.
> *
> * When probing for a clause 22 PHY, then read the ID registers. If we find
> * a valid ID, allocate and return a &struct phy_device.
> @@ -999,21 +1007,18 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45,
> * Returns an allocated &struct phy_device on success, %-ENODEV if there is
> * no PHY present, or %-EIO on bus access error.
> */
> -struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> +struct phy_device *get_phy_device(struct phy_device_config *config)
> {
> - struct phy_c45_device_ids c45_ids;
> - u32 phy_id = 0;
> + struct phy_c45_device_ids *c45_ids = &config->c45_ids;
> int r;
>
> - c45_ids.devices_in_package = 0;
> - c45_ids.mmds_present = 0;
> - memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
> + memset(c45_ids->device_ids, 0xff, sizeof(c45_ids->device_ids));
>
> - r = phy_device_detect(bus, addr, &is_c45, &phy_id, &c45_ids);
> + r = phy_device_detect(config);
> if (r)
> return ERR_PTR(r);
>
> - return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
> + return phy_device_create(config);
> }
> EXPORT_SYMBOL(get_phy_device);
>
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index f0fcb06fbe82..8fb0a1a49aaf 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -1635,10 +1635,15 @@ static void sfp_sm_phy_detach(struct sfp *sfp)
>
> static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
> {
> + struct phy_device_config config = {
> + .mii_bus = sfp->i2c_mii,
> + .phy_addr = addr,
> + .is_c45 = is_c45,
> + };
> struct phy_device *phy;
> int err;
>
> - phy = get_phy_device(sfp->i2c_mii, addr, is_c45);
> + phy = get_phy_device(&config);
> if (phy == ERR_PTR(-ENODEV))
> return PTR_ERR(phy);
> if (IS_ERR(phy)) {
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index c17a98712555..498f4dc7669d 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -756,6 +756,27 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
> return container_of(to_mdio_device(dev), struct phy_device, mdio);
> }
>
> +/**
> + * struct phy_device_config - Configuration of a PHY
> + *
> + * @mii_bus: The target MII bus the PHY is connected to
> + * @phy_addr: PHY address on the MII bus
> + * @phy_id: UID for this device found during discovery
> + * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
> + * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
> + *
> + * The struct contain possible configuration parameters for a PHY device which
> + * are used to setup the struct phy_device.
> + */
> +
> +struct phy_device_config {
> + struct mii_bus *mii_bus;
> + int phy_addr;
> + u32 phy_id;
> + struct phy_c45_device_ids c45_ids;
> + bool is_c45;
> +};
> +
> /**
> * struct phy_tdr_config - Configuration of a TDR raw test
> *
> @@ -1538,9 +1559,7 @@ int phy_modify_paged_changed(struct phy_device *phydev, int page, u32 regnum,
> int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum,
> u16 mask, u16 set);
>
> -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);
> +struct phy_device *phy_device_create(struct phy_device_config *config);
> void phy_device_set_miits(struct phy_device *phydev,
> struct mii_timestamper *mii_ts);
> #if IS_ENABLED(CONFIG_PHYLIB)
> @@ -1549,7 +1568,7 @@ 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(const struct fwnode_handle *fwnode);
> -struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
> +struct phy_device *get_phy_device(struct phy_device_config *config);
> int phy_device_register(struct phy_device *phy);
> void phy_device_free(struct phy_device *phydev);
> #else
> @@ -1581,7 +1600,7 @@ struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> }
>
> static inline
> -struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> +struct phy_device *get_phy_device(struct phy_device_config *config)
> {
> return NULL;
> }
>
Lets try again....
There are a number of things i don't like about this patchset.
It does too many different things.
It pulls workarounds into the core.
I don't like the phy_device_config. It would make sense if there were
more than 6 arguments to pass to a function, but not for less.
I don't like the name phy_device_atomic_register(), but that is bike
shedding.
There is no really strong argument to change the API.
There is no really strong argument to move to fwnode.
The problem you are trying to solve is to call phy_device_reset()
earlier, before reading the ID registers. Please produce a patchset
which is only focused on that. Nothing else.
Andrew