diff mbox

[3/6] net: arc_emac: support the phy reset for emac driver

Message ID 1457693731-6966-4-git-send-email-wxt@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Caesar Wang March 11, 2016, 10:55 a.m. UTC
This patch adds to support the emac phy reset.

1) phy-reset-gpios:
The phy-reset-gpios is an optional property for arc emac device tree boot.
Change the binding document to match the driver code.

2) phy-reset-duration:
Different boards may require different phy reset duration. Add property
phy-reset-duration for device tree probe, so that the boards that need
a longer reset duration can specify it in their device tree.

3) phy-reset-active-high:
We need that for a custom hardware that needs the reverse reset sequence.

Of course, this patch will fix the issue on
https://patchwork.kernel.org/patch/8186801/.

In some cases, the emac couldn't work if you don't have reset the phy.
Let's add it to happy work.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 drivers/net/ethernet/arc/emac_main.c | 41 ++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

Sergei Shtylyov March 11, 2016, 1:47 p.m. UTC | #1
Hello.

On 3/11/2016 1:55 PM, Caesar Wang wrote:

> This patch adds to support the emac phy reset.
>
> 1) phy-reset-gpios:
> The phy-reset-gpios is an optional property for arc emac device tree boot.
> Change the binding document to match the driver code.
>
> 2) phy-reset-duration:
> Different boards may require different phy reset duration. Add property
> phy-reset-duration for device tree probe, so that the boards that need
> a longer reset duration can specify it in their device tree.
>
> 3) phy-reset-active-high:
> We need that for a custom hardware that needs the reverse reset sequence.
>
> Of course, this patch will fix the issue on
> https://patchwork.kernel.org/patch/8186801/.
>
> In some cases, the emac couldn't work if you don't have reset the phy.
> Let's add it to happy work.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>   drivers/net/ethernet/arc/emac_main.c | 41 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
>
> diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c
> index 6446af1..42384f6a 100644
> --- a/drivers/net/ethernet/arc/emac_main.c
> +++ b/drivers/net/ethernet/arc/emac_main.c
> @@ -764,6 +764,45 @@ static const struct net_device_ops arc_emac_netdev_ops = {
>   #endif
>   };
>
> +#ifdef CONFIG_OF
> +static void emac_reset_phy(struct net_device *pdev)
> +{
> +	int err, phy_reset;
> +	bool active_high = false;
> +	int msec = 10;
> +	struct device *dev = pdev->dev.parent;
> +	struct device_node *np = dev->of_node;
> +
> +	if (!np)
> +		return;
> +
> +	of_property_read_u32(np, "phy-reset-duration", &msec);
> +	/* A sane reset duration should not be longer than 1s */
> +	if (msec > 1000)
> +		msec = 1;
> +
> +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> +	if (!gpio_is_valid(phy_reset))
> +		return;
> +
> +	active_high = of_property_read_bool(np, "phy-reset-active-high");
> +
> +	err = devm_gpio_request_one(dev, phy_reset, active_high ?
> +				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +				    "phy-reset");
> +	if (err) {
> +		dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);
> +		return;
> +	}
> +	msleep(msec);
> +	gpio_set_value_cansleep(phy_reset, !active_high);
> +}
[...]

    Why not make it the mii_bus::reset() method? It gets called before the 
MDIO bus scan.

MBR, Sergei
Heiko Stübner March 11, 2016, 2:56 p.m. UTC | #2
Hi Caesar,

Am Freitag, 11. März 2016, 16:47:59 schrieb Sergei Shtylyov:
> On 3/11/2016 1:55 PM, Caesar Wang wrote:
> > This patch adds to support the emac phy reset.
> > 
> > 1) phy-reset-gpios:
> > The phy-reset-gpios is an optional property for arc emac device tree boot.
> > Change the binding document to match the driver code.
> > 
> > 2) phy-reset-duration:
> > Different boards may require different phy reset duration. Add property
> > phy-reset-duration for device tree probe, so that the boards that need
> > a longer reset duration can specify it in their device tree.
> > 
> > 3) phy-reset-active-high:
> > We need that for a custom hardware that needs the reverse reset sequence.
> > 
> > Of course, this patch will fix the issue on
> > https://patchwork.kernel.org/patch/8186801/.
> > 
> > In some cases, the emac couldn't work if you don't have reset the phy.
> > Let's add it to happy work.
> > 
> > Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> > ---
> > 
> >   drivers/net/ethernet/arc/emac_main.c | 41
> >   ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/arc/emac_main.c
> > b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644
> > --- a/drivers/net/ethernet/arc/emac_main.c
> > +++ b/drivers/net/ethernet/arc/emac_main.c
> > @@ -764,6 +764,45 @@ static const struct net_device_ops
> > arc_emac_netdev_ops = {> 
> >   #endif
> >   };
> > 
> > +#ifdef CONFIG_OF
> > +static void emac_reset_phy(struct net_device *pdev)
> > +{
> > +	int err, phy_reset;
> > +	bool active_high = false;
> > +	int msec = 10;
> > +	struct device *dev = pdev->dev.parent;
> > +	struct device_node *np = dev->of_node;
> > +
> > +	if (!np)
> > +		return;
> > +
> > +	of_property_read_u32(np, "phy-reset-duration", &msec);
> > +	/* A sane reset duration should not be longer than 1s */
> > +	if (msec > 1000)
> > +		msec = 1;
> > +
> > +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> > +	if (!gpio_is_valid(phy_reset))
> > +		return;
> > +
> > +	active_high = of_property_read_bool(np, "phy-reset-active-high");
> > +
> > +	err = devm_gpio_request_one(dev, phy_reset, active_high ?
> > +				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > +				    "phy-reset");
> > +	if (err) {
> > +		dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);
> > +		return;
> > +	}
> > +	msleep(msec);
> > +	gpio_set_value_cansleep(phy_reset, !active_high);
> > +}
> 
> [...]
> 
>     Why not make it the mii_bus::reset() method? It gets called before the
> MDIO bus scan.

while meddling with the emac, I've built some sort of preliminary variant of
Sergei's suggestion at [0] - maybe you could take a look for some sort of
inspiration ;-) 

The code is lifted from the designware gmac driver, so the binding also is
similar.


Heiko


[0] https://github.com/mmind/linux-rockchip/commit/a943c588783438ff1c508dfa8c79f1709aa5775e
Heiko Stübner March 11, 2016, 2:59 p.m. UTC | #3
Am Freitag, 11. März 2016, 15:56:04 schrieb Heiko Stübner:
> Hi Caesar,
> 
> Am Freitag, 11. März 2016, 16:47:59 schrieb Sergei Shtylyov:
> > On 3/11/2016 1:55 PM, Caesar Wang wrote:
> > > This patch adds to support the emac phy reset.
> > > 
> > > 1) phy-reset-gpios:
> > > The phy-reset-gpios is an optional property for arc emac device tree
> > > boot.
> > > Change the binding document to match the driver code.
> > > 
> > > 2) phy-reset-duration:
> > > Different boards may require different phy reset duration. Add property
> > > phy-reset-duration for device tree probe, so that the boards that need
> > > a longer reset duration can specify it in their device tree.
> > > 
> > > 3) phy-reset-active-high:
> > > We need that for a custom hardware that needs the reverse reset
> > > sequence.
> > > 
> > > Of course, this patch will fix the issue on
> > > https://patchwork.kernel.org/patch/8186801/.
> > > 
> > > In some cases, the emac couldn't work if you don't have reset the phy.
> > > Let's add it to happy work.
> > > 
> > > Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> > > ---
> > > 
> > >   drivers/net/ethernet/arc/emac_main.c | 41
> > >   ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/arc/emac_main.c
> > > b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644
> > > --- a/drivers/net/ethernet/arc/emac_main.c
> > > +++ b/drivers/net/ethernet/arc/emac_main.c
> > > @@ -764,6 +764,45 @@ static const struct net_device_ops
> > > arc_emac_netdev_ops = {>
> > > 
> > >   #endif
> > >   };
> > > 
> > > +#ifdef CONFIG_OF
> > > +static void emac_reset_phy(struct net_device *pdev)
> > > +{
> > > +	int err, phy_reset;
> > > +	bool active_high = false;
> > > +	int msec = 10;
> > > +	struct device *dev = pdev->dev.parent;
> > > +	struct device_node *np = dev->of_node;
> > > +
> > > +	if (!np)
> > > +		return;
> > > +
> > > +	of_property_read_u32(np, "phy-reset-duration", &msec);
> > > +	/* A sane reset duration should not be longer than 1s */
> > > +	if (msec > 1000)
> > > +		msec = 1;
> > > +
> > > +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> > > +	if (!gpio_is_valid(phy_reset))
> > > +		return;
> > > +
> > > +	active_high = of_property_read_bool(np, "phy-reset-active-high");
> > > +
> > > +	err = devm_gpio_request_one(dev, phy_reset, active_high ?
> > > +				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > +				    "phy-reset");
> > > +	if (err) {
> > > +		dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);
> > > +		return;
> > > +	}
> > > +	msleep(msec);
> > > +	gpio_set_value_cansleep(phy_reset, !active_high);
> > > +}
> > 
> > [...]
> > 
> >     Why not make it the mii_bus::reset() method? It gets called before the
> > 
> > MDIO bus scan.
> 
> while meddling with the emac, I've built some sort of preliminary variant of
> Sergei's suggestion at [0] - maybe you could take a look for some sort of
> inspiration ;-)

forgot to add ... of course the newer gpiod_* infrastructure should be used 
instead of the old integer-based gpio numbers

> 
> The code is lifted from the designware gmac driver, so the binding also is
> similar.
> 
> 
> Heiko
> 
> 
> [0]
> https://github.com/mmind/linux-rockchip/commit/a943c588783438ff1c508dfa8c79
> f1709aa5775e
Sergei Shtylyov March 11, 2016, 6:35 p.m. UTC | #4
On 03/11/2016 01:55 PM, Caesar Wang wrote:

> This patch adds to support the emac phy reset.
>
> 1) phy-reset-gpios:
> The phy-reset-gpios is an optional property for arc emac device tree boot.
> Change the binding document to match the driver code.

    The binding document is apparently changed in another patch. Not sure what 
you wanted to say here...

> 2) phy-reset-duration:
> Different boards may require different phy reset duration. Add property
> phy-reset-duration for device tree probe, so that the boards that need
> a longer reset duration can specify it in their device tree.
>
> 3) phy-reset-active-high:
> We need that for a custom hardware that needs the reverse reset sequence.

    No, we don't really need that, "phy-reset-gpio" prop can contain this data.

> Of course, this patch will fix the issue on
> https://patchwork.kernel.org/patch/8186801/.
>
> In some cases, the emac couldn't work if you don't have reset the phy.
> Let's add it to happy work.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>   drivers/net/ethernet/arc/emac_main.c | 41 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
>
> diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c
> index 6446af1..42384f6a 100644
> --- a/drivers/net/ethernet/arc/emac_main.c
> +++ b/drivers/net/ethernet/arc/emac_main.c
> @@ -764,6 +764,45 @@ static const struct net_device_ops arc_emac_netdev_ops = {
>   #endif
>   };
>
> +#ifdef CONFIG_OF
> +static void emac_reset_phy(struct net_device *pdev)
> +{
> +	int err, phy_reset;
> +	bool active_high = false;
> +	int msec = 10;
> +	struct device *dev = pdev->dev.parent;
> +	struct device_node *np = dev->of_node;
> +
> +	if (!np)
> +		return;
> +
> +	of_property_read_u32(np, "phy-reset-duration", &msec);
> +	/* A sane reset duration should not be longer than 1s */
> +	if (msec > 1000)
> +		msec = 1;
> +
> +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);

    Forgot to say that these old integer-base GPIO APIs shouldn't be used by 
the new code, there's new 'struct gpio_desc' based APIs, like devm_gpiod_get() 
etc.

> +	if (!gpio_is_valid(phy_reset))
> +		return;
> +
> +	active_high = of_property_read_bool(np, "phy-reset-active-high");

    Well, I still don't understand why this prop is needed, while the GPIO 
sense is transparently handled by the gpiolib (at least when using DT).

[...]

MBR, Sergei
Sergei Shtylyov March 11, 2016, 7:22 p.m. UTC | #5
On 03/11/2016 01:55 PM, Caesar Wang wrote:

> This patch adds to support the emac phy reset.
>
> 1) phy-reset-gpios:
> The phy-reset-gpios is an optional property for arc emac device tree boot.
> Change the binding document to match the driver code.
>
> 2) phy-reset-duration:
> Different boards may require different phy reset duration. Add property
> phy-reset-duration for device tree probe, so that the boards that need
> a longer reset duration can specify it in their device tree.
>
> 3) phy-reset-active-high:
> We need that for a custom hardware that needs the reverse reset sequence.

    What concerns me the most about the existing (and suggested) PHY reset 
related props is that they are located in the MAC device node while not having 
*anything* to do with the MAC at all! These props actually belong to the PHY 
nodes, and I'm currently looking into how to handle them there, where they 
belong...

> Of course, this patch will fix the issue on
> https://patchwork.kernel.org/patch/8186801/.
>
> In some cases, the emac couldn't work if you don't have reset the phy.
> Let's add it to happy work.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>

[...]

MBR, Sergei
Caesar Wang March 13, 2016, 3:57 a.m. UTC | #6
Hi Sergei,

? 2016?03?12? 02:35, Sergei Shtylyov ??:
> On 03/11/2016 01:55 PM, Caesar Wang wrote:
>
>> This patch adds to support the emac phy reset.
>>
>> 1) phy-reset-gpios:
>> The phy-reset-gpios is an optional property for arc emac device tree 
>> boot.
>> Change the binding document to match the driver code.
>
>    The binding document is apparently changed in another patch. Not 
> sure what you wanted to say here...
>
>> 2) phy-reset-duration:
>> Different boards may require different phy reset duration. Add property
>> phy-reset-duration for device tree probe, so that the boards that need
>> a longer reset duration can specify it in their device tree.
>>
>> 3) phy-reset-active-high:
>> We need that for a custom hardware that needs the reverse reset 
>> sequence.
>
>    No, we don't really need that, "phy-reset-gpio" prop can contain 
> this data.
>
>> Of course, this patch will fix the issue on
>> https://patchwork.kernel.org/patch/8186801/.
>>
>> In some cases, the emac couldn't work if you don't have reset the phy.
>> Let's add it to happy work.
>>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
>>
>>   drivers/net/ethernet/arc/emac_main.c | 41 
>> ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/arc/emac_main.c 
>> b/drivers/net/ethernet/arc/emac_main.c
>> index 6446af1..42384f6a 100644
>> --- a/drivers/net/ethernet/arc/emac_main.c
>> +++ b/drivers/net/ethernet/arc/emac_main.c
>> @@ -764,6 +764,45 @@ static const struct net_device_ops 
>> arc_emac_netdev_ops = {
>>   #endif
>>   };
>>
>> +#ifdef CONFIG_OF
>> +static void emac_reset_phy(struct net_device *pdev)
>> +{
>> +    int err, phy_reset;
>> +    bool active_high = false;
>> +    int msec = 10;
>> +    struct device *dev = pdev->dev.parent;
>> +    struct device_node *np = dev->of_node;
>> +
>> +    if (!np)
>> +        return;
>> +
>> +    of_property_read_u32(np, "phy-reset-duration", &msec);
>> +    /* A sane reset duration should not be longer than 1s */
>> +    if (msec > 1000)
>> +        msec = 1;
>> +
>> +    phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>
>    Forgot to say that these old integer-base GPIO APIs shouldn't be 
> used by the new code, there's new 'struct gpio_desc' based APIs, like 
> devm_gpiod_get() etc.
>
>> +    if (!gpio_is_valid(phy_reset))
>> +        return;
>> +
>> +    active_high = of_property_read_bool(np, "phy-reset-active-high");
>
>    Well, I still don't understand why this prop is needed, while the 
> GPIO sense is transparently handled by the gpiolib (at least when 
> using DT).

Okay, I have to admit that reference the exist way to do...


wxt@nb:~/kernel/drivers/net/ethernet$ ag reset-gpios
micrel/ks8851.c
1427:    gpio = of_get_named_gpio_flags(spi->dev.of_node, "reset-gpios",

arc/emac_main.c
787:    phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
797:        dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);

arc/emac_main.c~
784:    phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
794:        dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);

davicom/dm9000.c
1451:    reset_gpios = of_get_named_gpio_flags(dev->of_node, 
"reset-gpios", 0,

freescale/fec_main.c
3206:    phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
3216:        dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", 
err);

cadence/macb.c
2958:        int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);

...

Yep, the gpiolib that can include the prop to work with the same ways.



>
> [...]
>
> MBR, Sergei
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff mbox

Patch

diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c
index 6446af1..42384f6a 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -764,6 +764,45 @@  static const struct net_device_ops arc_emac_netdev_ops = {
 #endif
 };
 
+#ifdef CONFIG_OF
+static void emac_reset_phy(struct net_device *pdev)
+{
+	int err, phy_reset;
+	bool active_high = false;
+	int msec = 10;
+	struct device *dev = pdev->dev.parent;
+	struct device_node *np = dev->of_node;
+
+	if (!np)
+		return;
+
+	of_property_read_u32(np, "phy-reset-duration", &msec);
+	/* A sane reset duration should not be longer than 1s */
+	if (msec > 1000)
+		msec = 1;
+
+	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (!gpio_is_valid(phy_reset))
+		return;
+
+	active_high = of_property_read_bool(np, "phy-reset-active-high");
+
+	err = devm_gpio_request_one(dev, phy_reset, active_high ?
+				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				    "phy-reset");
+	if (err) {
+		dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);
+		return;
+	}
+	msleep(msec);
+	gpio_set_value_cansleep(phy_reset, !active_high);
+}
+#else
+static void emac_reset_phy(struct platform_device *pdev)
+{
+}
+#endif /* CONFIG_OF */
+
 int arc_emac_probe(struct net_device *ndev, int interface)
 {
 	struct device *dev = ndev->dev.parent;
@@ -803,6 +842,8 @@  int arc_emac_probe(struct net_device *ndev, int interface)
 	/* FIXME :: no multicast support yet */
 	ndev->flags &= ~IFF_MULTICAST;
 
+	emac_reset_phy(ndev);
+
 	priv = netdev_priv(ndev);
 	priv->dev = dev;