[U-Boot] [PATCH 0/4] net: dm: fec: Fixes and improvements
A few fixes and improvements for the FEC driver.
They are functionally independent but included in a series to avoid conflicts if merged out of order as they touch the same files.
Martin Fuzzey (4): net: dm: fec: Fix time unit error in phy-reset-duration net: dm: fec: Fix phy-reset-duration clamping and defaults net: dm: fec: Support the phy-supply binding net: dm: fec: Obtain the transceiver type from the DT
drivers/net/fec_mxc.c | 60 ++++++++++++++++++++++++++++++++++++++++----------- drivers/net/fec_mxc.h | 3 +++ 2 files changed, 51 insertions(+), 12 deletions(-)
The DT binding says that phy-reset-duration is in ms, but the driver currently uses udelay().
Switch to mdelay() to fix this.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group --- drivers/net/fec_mxc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index dac07b6..a1295fc 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -1254,7 +1254,7 @@ static void fec_gpio_reset(struct fec_priv *priv) debug("fec_gpio_reset: fec_gpio_reset(dev)\n"); if (dm_gpio_is_valid(&priv->phy_reset_gpio)) { dm_gpio_set_value(&priv->phy_reset_gpio, 1); - udelay(priv->reset_delay); + mdelay(priv->reset_delay); dm_gpio_set_value(&priv->phy_reset_gpio, 0); } }
The DT binding says: - phy-reset-duration : Reset duration in milliseconds. Should present only if property "phy-reset-gpios" is available. Missing the property will have the duration be 1 millisecond. Numbers greater than 1000 are invalid and 1 millisecond will be used instead.
However the current code: - clamps values greater than 1000ms to 1000ms rather than 1. - does not initialize the delay if the property does not exist (else clause mismatch) - returns an error if phy-reset-gpios is not defined
Fix all this and simplify by using dev_read_u32_default()
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group --- drivers/net/fec_mxc.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index a1295fc..163ae4c 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -1354,20 +1354,17 @@ static int fecmxc_ofdata_to_platdata(struct udevice *dev) ret = gpio_request_by_name(dev, "phy-reset-gpios", 0, &priv->phy_reset_gpio, GPIOD_IS_OUT); if (ret == 0) { - ret = dev_read_u32_array(dev, "phy-reset-duration", - &priv->reset_delay, 1); - } else if (ret == -ENOENT) { - priv->reset_delay = 1000; - ret = 0; - } - - if (priv->reset_delay > 1000) { - printf("FEX MXC: gpio reset timeout should be less the 1000\n"); - priv->reset_delay = 1000; + priv->reset_delay = dev_read_u32_default(dev, + "phy-reset-duration", + 1); + if (priv->reset_delay > 1000) { + printf("FEC MXC: phy reset duration should be <= 1000ms\n"); + priv->reset_delay = 1; + } } #endif
- return ret; + return 0; }
static const struct udevice_id fecmxc_ids[] = {
Configure the phy regulator if defined by the "phy-supply" DT phandle.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group --- drivers/net/fec_mxc.c | 20 ++++++++++++++++++++ drivers/net/fec_mxc.h | 3 +++ 2 files changed, 23 insertions(+)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 163ae4c..4a5555e 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -15,6 +15,7 @@ #include <miiphy.h> #include <net.h> #include <netdev.h> +#include <power/regulator.h>
#include <asm/io.h> #include <linux/errno.h> @@ -1272,6 +1273,16 @@ static int fecmxc_probe(struct udevice *dev) if (ret) return ret;
+#ifdef CONFIG_DM_REGULATOR + if (priv->phy_supply) { + ret = regulator_autoset(priv->phy_supply); + if (ret) { + printf("%s: Error enabling phy supply\n", dev->name); + return ret; + } + } +#endif + #ifdef CONFIG_DM_GPIO fec_gpio_reset(priv); #endif @@ -1327,6 +1338,11 @@ static int fecmxc_remove(struct udevice *dev) mdio_unregister(priv->bus); mdio_free(priv->bus);
+#ifdef CONFIG_DM_REGULATOR + if (priv->phy_supply) + regulator_set_enable(priv->phy_supply, false); +#endif + return 0; }
@@ -1364,6 +1380,10 @@ static int fecmxc_ofdata_to_platdata(struct udevice *dev) } #endif
+#ifdef CONFIG_DM_REGULATOR + device_get_supply_regulator(dev, "phy-supply", &priv->phy_supply); +#endif + return 0; }
diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h index fd89443..848cd7c 100644 --- a/drivers/net/fec_mxc.h +++ b/drivers/net/fec_mxc.h @@ -250,6 +250,9 @@ struct fec_priv { int phy_id; int (*mii_postcall)(int); #endif +#ifdef CONFIG_DM_REGULATOR + struct udevice *phy_supply; +#endif #ifdef CONFIG_DM_GPIO struct gpio_desc phy_reset_gpio; uint32_t reset_delay;
On Thu, Oct 4, 2018 at 12:55 PM Martin Fuzzey martin.fuzzey@flowbird.group wrote:
Configure the phy regulator if defined by the "phy-supply" DT phandle.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group
This patch seems to break the Ethernet on my board, but I think I have a possible solution (see below)
drivers/net/fec_mxc.c | 20 ++++++++++++++++++++ drivers/net/fec_mxc.h | 3 +++ 2 files changed, 23 insertions(+)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 163ae4c..4a5555e 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -15,6 +15,7 @@ #include <miiphy.h> #include <net.h> #include <netdev.h> +#include <power/regulator.h>
#include <asm/io.h> #include <linux/errno.h> @@ -1272,6 +1273,16 @@ static int fecmxc_probe(struct udevice *dev) if (ret) return ret;
+#ifdef CONFIG_DM_REGULATOR
if (priv->phy_supply) {ret = regulator_autoset(priv->phy_supply);
I have a board that uses a fixed regulator driven by a GPIO. It's neither always-on, nor it is enabled on boot and it doesn't have a specified current setting. With DM_REGULATOR set, regulator_autoset fails and FEC doesn't come up. Looking at a bunch of other drivers, and how they enable their respective regulators, they're using regulator_set_enable instead of autoset.
Is there a reason we couldn't use
ret = regulator_set_enable(priv->phy_supply, true);
adam
if (ret) {printf("%s: Error enabling phy supply\n", dev->name);return ret;}}+#endif
#ifdef CONFIG_DM_GPIO fec_gpio_reset(priv); #endif @@ -1327,6 +1338,11 @@ static int fecmxc_remove(struct udevice *dev) mdio_unregister(priv->bus); mdio_free(priv->bus);
+#ifdef CONFIG_DM_REGULATOR
if (priv->phy_supply)regulator_set_enable(priv->phy_supply, false);+#endif
return 0;}
@@ -1364,6 +1380,10 @@ static int fecmxc_ofdata_to_platdata(struct udevice *dev) } #endif
+#ifdef CONFIG_DM_REGULATOR
device_get_supply_regulator(dev, "phy-supply", &priv->phy_supply);+#endif
return 0;}
diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h index fd89443..848cd7c 100644 --- a/drivers/net/fec_mxc.h +++ b/drivers/net/fec_mxc.h @@ -250,6 +250,9 @@ struct fec_priv { int phy_id; int (*mii_postcall)(int); #endif +#ifdef CONFIG_DM_REGULATOR
struct udevice *phy_supply;+#endif #ifdef CONFIG_DM_GPIO struct gpio_desc phy_reset_gpio; uint32_t reset_delay; -- 1.9.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Adam,
On 13/01/2019 15:00, Adam Ford wrote:
On Thu, Oct 4, 2018 at 12:55 PM Martin Fuzzey martin.fuzzey@flowbird.group wrote:
Configure the phy regulator if defined by the "phy-supply" DT phandle.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group
This patch seems to break the Ethernet on my board, but I think I have a possible solution (see below)
Ah you are right my bad.
I guess this means that you already had the phy-supply property in your DT but unused up till now.
I have a board that uses a fixed regulator driven by a GPIO. It's neither always-on, nor it is enabled on boot and it doesn't have a specified current setting. With DM_REGULATOR set, regulator_autoset fails and FEC doesn't come up. Looking at a bunch of other drivers, and how they enable their respective regulators, they're using regulator_set_enable instead of autoset.
Indeed, on my board I am setting the voltage and do have regulator-boot-on;,
which is why I didn't run into this issue.
Incidentally it looks like if you need to set the voltage you *need* regulator-boot-on, which means the regulator will always be enabled too.
There seems to be no way to say "please set this voltage but only switch the regulator on if a driver requests it".
Is there a reason we couldn't use
ret = regulator_set_enable(priv->phy_supply, true);
That looks good and tested ok on my board too.
Since my patch has now been merged it needs a follow up patch to replace regulator_autoset() with regulator_set_enable().
I'll send a fix patch soon unless you want to do it as the finder of the problem.
Regards,
Martin
On Tue, Jan 15, 2019 at 11:08 AM Martin Fuzzey martin.fuzzey@flowbird.group wrote:
Hi Adam,
On 13/01/2019 15:00, Adam Ford wrote:
On Thu, Oct 4, 2018 at 12:55 PM Martin Fuzzey martin.fuzzey@flowbird.group wrote:
Configure the phy regulator if defined by the "phy-supply" DT phandle.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group
This patch seems to break the Ethernet on my board, but I think I have a possible solution (see below)
Ah you are right my bad.
I guess this means that you already had the phy-supply property in your DT but unused up till now.
I haven't had DM_REGULATOR turned on until recently which is how I found this bug.
I have a board that uses a fixed regulator driven by a GPIO. It's neither always-on, nor it is enabled on boot and it doesn't have a specified current setting. With DM_REGULATOR set, regulator_autoset fails and FEC doesn't come up. Looking at a bunch of other drivers, and how they enable their respective regulators, they're using regulator_set_enable instead of autoset.
Indeed, on my board I am setting the voltage and do have regulator-boot-on;,
which is why I didn't run into this issue.
Incidentally it looks like if you need to set the voltage you *need* regulator-boot-on, which means the regulator will always be enabled too.
There seems to be no way to say "please set this voltage but only switch the regulator on if a driver requests it".
Is there a reason we couldn't use
ret = regulator_set_enable(priv->phy_supply, true);That looks good and tested ok on my board too.
good.
Since my patch has now been merged it needs a follow up patch to replace regulator_autoset() with regulator_set_enable().
I agree.
I'll send a fix patch soon unless you want to do it as the finder of the problem.
I have a patch ready to go. I can just push it. I'll CC you on the e-mail and you can reply as tested-by or reviewed-by or whatever, if you're open to that.
adam
Regards,
Martin
The DT property "phy-mode" already provides the transceiver type. Use it so that we do not have to also set CONFIG_FEC_XCV_TYPE
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group --- drivers/net/fec_mxc.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 4a5555e..e3fc595 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -1312,8 +1312,27 @@ static int fecmxc_probe(struct udevice *dev) }
priv->bus = bus; - priv->xcv_type = CONFIG_FEC_XCV_TYPE; priv->interface = pdata->phy_interface; + switch (priv->interface) { + case PHY_INTERFACE_MODE_MII: + priv->xcv_type = MII100; + break; + case PHY_INTERFACE_MODE_RMII: + priv->xcv_type = RMII; + break; + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_RGMII_ID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_TXID: + priv->xcv_type = RGMII; + break; + default: + priv->xcv_type = CONFIG_FEC_XCV_TYPE; + printf("Unsupported interface type %d defaulting to %d\n", + priv->interface, priv->xcv_type); + break; + } + ret = fec_phy_init(priv, dev); if (ret) goto err_phy;
participants (2)
-
Adam Ford -
Martin Fuzzey