| Message ID | 1538675595-3706-4-git-send-email-martin.fuzzey@flowbird.group |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
| Series | net: dm: fec: Fixes and improvements | expand |
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 > >
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;
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(+)