[U-Boot] [PATCH 0/4] net: dm: fec: Fixes and improvements

[U-Boot] [PATCH 0/4] net: dm: fec: Fixes and improvements

[Martin Fuzzey]

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(-)

-- 
1.9.1

[U-Boot] [PATCH 1/4] net: dm: fec: Fix time unit error in phy-reset-duration

[Martin Fuzzey]

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);
 	}
 }
-- 
1.9.1

[U-Boot] [PATCH 2/4] net: dm: fec: Fix phy-reset-duration clamping and defaults

[Martin Fuzzey]

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[] = {
-- 
1.9.1

[U-Boot] [PATCH 3/4] net: dm: fec: Support the phy-supply binding

[Martin Fuzzey]

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;
-- 
1.9.1

[U-Boot] [PATCH 4/4] net: dm: fec: Obtain the transceiver type from the DT

[Martin Fuzzey]

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;
-- 
1.9.1

[U-Boot] [PATCH 3/4] net: dm: fec: Support the phy-supply binding

[Adam Ford]

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 3/4] net: dm: fec: Support the phy-supply binding

[Martin Fuzzey]

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

[U-Boot] [PATCH 3/4] net: dm: fec: Support the phy-supply binding

[Adam Ford]

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
>
>

[U-Boot] [PATCH 3/4] net: dm: fec: Support the phy-supply binding

[Joe Hershberger]

On Thu, Oct 4, 2018 at 1:03 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>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 3/4] net: dm: fec: Support the phy-supply binding

[Martin Fuzzey]

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;
-- 
1.9.1