Hi All,
The main intention of this mail is to trigger discussion to find a proper
solution. All code is hackish and based on v5.9.
Problem statement:
There is an issue observed with MDIO OF PHYs discover/reset sequence in
case PHY has reset line with default state is (1).
In this case, when Linux boots PHY is in reset and following code fails:
of_mdiobus_register()
|- for_each_available_child_of_node(np, child)
|- of_mdiobus_register_phy
|- get_phy_device
|- get_phy_c22_id ---- > *fails as PHY is in reset*
...
|- of_mdiobus_phy_device_register() --> can't be reached
The current PHY allows to specify PHY reset line for PHY:
&mdio {
phy0: ethernet-phy@0 {
reg = <0>;
ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
+ reset-gpios = <&pca9555 4 GPIO_ACTIVE_LOW>;
ti,dp83867-rxctrl-strap-quirk;
};
};
But it doesn't help in this case, as PHY's reset code is initialized when
PHY's mdio_device is registered, which, in turn, happens after
get_phy_device() call (and get_phy_device() is failed).
of_mdiobus_phy_device_register()
|-phy_device_register
|-mdiobus_register_device()
|-mdiobus_register_gpiod(mdiodev);
|-mdiobus_register_reset(mdiodev);
There is also possibility to add GPIO reset line to MDIO node itself, but
It also doesn't help when there are >1 PHY and every PHY has it own reset
line.
Only one possible W/A now is to use GPIO HOG, with drawback that PHY will
stay active always.
Some history:
- commit 69226896ad63 ("mdio_bus: Issue GPIO RESET to PHYs") from Roger
Quadros <rog...@ti.com> originally added possibility to specify >1 GPIO
reset line in MDIO node, which allowed to solve such issues.
- commit 4c5e7a2c0501 ("dt-bindings: mdio: Clarify binding document") and
follow up commit d396e84c5604 ("mdio_bus: handle only single PHY reset
GPIO") rolled back original solution to only one GPIO reset line, which
causes problems now.
Possible solutions I come up with:
1) Try to add PHY reset code around get_phy_device() call in
of_mdiobus_register_phy()
cons:
- need to extract/share mdio_device reset code as PHY may have not only GPIO,
but also reset_control object assigned.
And all current mdio_device rest code expected to have mdio_device already
initialized.
- There 12 calls to get_phy_device() in v5.9 Kernel
2) Try to consolidate OF mdio_device/PHY initialization in one place, as
illustrated by of_phy_device_create() function (marked by "// option 2" in
code).
3) Return back possibility to use >1 GPIO reset line in MDIO node. Even if
It seems right thing to do by itself (Devices attached to MDIO bus may have
any combination of shared reset lines - not always "one for all"), there
are more things to consider:
- PHY reset_control objects handling
- the fact that MDIO reset will put PHY out of reset and not allow to
reset it again (more like gpio-hog)
I'd be appreciated for any comments to help resolve it. May be there is
better way to handle this?
Cc: Florian Fainelli <f.faine...@gmail.com>
Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
---
drivers/net/phy/phy_device.c | 152 +++++++++++++++++++++++++++++++++++
drivers/of/of_mdio.c | 35 +++++++-
include/linux/phy.h | 3 +
3 files changed, 186 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..9b22e7c1c4a1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -23,6 +23,8 @@
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/of_irq.h>
+#include <linux/of_mdio.h>
#include <linux/phy.h>
#include <linux/phy_led_triggers.h>
#include <linux/property.h>
@@ -3019,6 +3021,156 @@ void phy_drivers_unregister(struct phy_driver *drv, int
n)
}
EXPORT_SYMBOL(phy_drivers_unregister);
+struct phy_device *
+of_phy_device_create(struct mii_bus *bus, struct device_node *child, int addr,
+ u32 phy_id, bool is_c45)
+{
+ struct phy_c45_device_ids c45_ids;
+ struct phy_device *dev;
+ struct mdio_device *mdiodev;
+ int ret = 0;
+ int rc;
+
+ /* We allocate the device, and initialize the default values */
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return ERR_PTR(-ENOMEM);
+
+ mdiodev = &dev->mdio;
+ mdiodev->dev.parent = &bus->dev;
+ mdiodev->dev.bus = &mdio_bus_type;
+ mdiodev->dev.type = &mdio_bus_phy_type;
+ mdiodev->bus = bus;
+ mdiodev->bus_match = phy_bus_match;
+ mdiodev->addr = addr;
+ mdiodev->flags = MDIO_DEVICE_FLAG_PHY;
+ mdiodev->device_free = phy_mdio_device_free;
+ mdiodev->device_remove = phy_mdio_device_remove;
+
+ /* Associate the OF node with the device structure so it
+ * can be looked up later
+ */
+ of_node_get(child);
+ mdiodev->dev.of_node = child;
+ mdiodev->dev.fwnode = of_fwnode_handle(child);
+
+ rc = of_irq_get(child, 0);
+ if (rc == -EPROBE_DEFER)
+ return ERR_PTR(rc);
+
+ if (rc > 0) {
+ dev->irq = rc;
+ bus->irq[addr] = rc;
+ } else {
+ dev->irq = bus->irq[addr];
+ }
+
+ if (of_property_read_bool(child, "broken-turn-around"))
+ bus->phy_ignore_ta_mask |= 1 << addr;
+ of_property_read_u32(child, "reset-assert-us",
+ &mdiodev->reset_assert_delay);
+ of_property_read_u32(child, "reset-deassert-us",
+ &mdiodev->reset_deassert_delay);
+
+ dev->speed = SPEED_UNKNOWN;
+ dev->duplex = DUPLEX_UNKNOWN;
+ dev->pause = 0;
+ dev->asym_pause = 0;
+ dev->link = 0;
+ dev->interface = PHY_INTERFACE_MODE_GMII;
+ dev->autoneg = AUTONEG_ENABLE;
+ dev->is_c45 = is_c45;
+
+ dev_set_name(&mdiodev->dev, PHY_ID_FMT, bus->id, addr);
+ device_initialize(&mdiodev->dev);
+
+ dev->state = PHY_DOWN;
+
+ mutex_init(&dev->lock);
+ INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
+
+ ret = mdiobus_register_device(mdiodev);
+ if (ret)
+ return ERR_PTR(ret);
+
+ /* Deassert the reset signal */
+ phy_device_reset(dev, 0);
+
+ if (phy_id == U32_MAX) {
+ phy_id = 0;
+
+ c45_ids.devices_in_package = 0;
+ c45_ids.mmds_present = 0;
+ memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
+
+ if (is_c45)
+ ret = get_phy_c45_ids(bus, addr, &c45_ids);
+ else
+ ret = get_phy_c22_id(bus, addr, &phy_id);
+
+ if (ret)
+ goto err_out;
+
+ dev->phy_id = phy_id;
+ dev->c45_ids = c45_ids;
+ } else {
+ dev->phy_id = phy_id;
+ }
+
+ /* Run all of the fixups for this PHY */
+ ret = phy_scan_fixups(dev);
+ if (ret) {
+ phydev_err(dev, "failed to initialize\n");
+ goto err_out;
+ }
+
+ /* Request the appropriate module unconditionally; don't
+ * bother trying to do so only if it isn't already loaded,
+ * because that gets complicated. A hotplug event would have
+ * done an unconditional modprobe anyway.
+ * We don't do normal hotplug because it won't work for MDIO
+ * -- because it relies on the device staying around for long
+ * enough for the driver to get loaded. With MDIO, the NIC
+ * driver will get bored and give up as soon as it finds that
+ * there's no driver _already_ loaded.
+ */
+ if (is_c45) {
+ const int num_ids = ARRAY_SIZE(c45_ids.device_ids);
+ int i;
+
+ for (i = 1; i < num_ids; i++) {
+ if (c45_ids.device_ids[i] == 0xffffffff)
+ continue;
+
+ ret = phy_request_driver_module(dev,
+ c45_ids.device_ids[i]);
+ if (ret)
+ break;
+ }
+ } else {
+ ret = phy_request_driver_module(dev, phy_id);
+ }
+
+ if (ret)
+ goto err_out;
+
+ ret = device_add(&mdiodev->dev);
+ if (ret) {
+ phydev_err(dev, "failed to add\n");
+ goto err_out;
+ }
+
+ return dev;
+
+err_out:
+ /* Assert the reset signal */
+ phy_device_reset(dev, 1);
+
+ mdiobus_unregister_device(mdiodev);
+ put_device(&mdiodev->dev);
+ return ERR_PTR(ret);
+}
+
static struct phy_driver genphy_driver = {
.phy_id = 0xffffffff,
.phy_id_mask = 0xffffffff,
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 4daf94bb56a5..26783a9193da 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -10,6 +10,7 @@
#include <linux/kernel.h>
#include <linux/device.h>
+#include <linux/gpio/consumer.h>
#include <linux/netdevice.h>
#include <linux/err.h>
#include <linux/phy.h>
@@ -105,7 +106,7 @@ int of_mdiobus_phy_device_register(struct mii_bus *mdio,
struct phy_device *phy,
EXPORT_SYMBOL(of_mdiobus_phy_device_register);
static int of_mdiobus_register_phy(struct mii_bus *mdio,
- struct device_node *child, u32 addr)
+ struct device_node *child, u32 addr)
{
struct mii_timestamper *mii_ts;
struct phy_device *phy;
@@ -120,16 +121,42 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
is_c45 = of_device_is_compatible(child,
"ethernet-phy-ieee802.3-c45");
- if (!is_c45 && !of_get_phy_id(child, &phy_id))
- phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
- else
+ if (!is_c45 && !of_get_phy_id(child, &phy_id)) {
+ phy = of_phy_device_create(mdio, child, addr, phy_id, 0);
+ } else {
+ struct gpio_desc *reset_gpio;
+ unsigned int reset_assert_delay = DEFAULT_GPIO_RESET_DELAY;
+ unsigned int reset_deassert_delay = DEFAULT_GPIO_RESET_DELAY;
+
+ reset_gpio = gpiod_get_from_of_node(child, "reset-gpios", 0,
+ GPIOD_OUT_LOW, "PHY
reset");
+ if (IS_ERR(reset_gpio)) {
+ if (PTR_ERR(reset_gpio) == -ENOENT)
+ reset_gpio = NULL;
+ else
+ return PTR_ERR(reset_gpio);
+ }
+
+ if (reset_gpio) {
+ gpiod_set_value_cansleep(reset_gpio, 1);
+ fsleep(reset_assert_delay);
+ gpiod_set_value_cansleep(reset_gpio, 0);
+ fsleep(reset_deassert_delay);
+ pr_err("========== reset PHY\n");
+ }
+// option 2
+// phy = of_phy_device_create(mdio, child, addr, U32_MAX, is_c45);
phy = get_phy_device(mdio, addr, is_c45);
+
+ gpiod_put(reset_gpio);
+ }
if (IS_ERR(phy)) {
if (mii_ts)
unregister_mii_timestamper(mii_ts);
return PTR_ERR(phy);
}
+// option 2 - remove
rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
if (rc) {
if (mii_ts)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index eb3cb1a98b45..3e5efb120cad 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1753,4 +1753,7 @@ module_exit(phy_module_exit)
bool phy_driver_is_genphy(struct phy_device *phydev);
bool phy_driver_is_genphy_10g(struct phy_device *phydev);
+struct phy_device *of_phy_device_create(struct mii_bus *bus, struct
device_node *child,
+ int addr, u32 phy_id, bool is_c45);
+
#endif /* __PHY_H */
--
2.17.1
