[PATCH 2/3] net: fec: convert to using gpiod framework

[Lothar Waßmann]

Use gpiod_get_optional() instead of checking for a valid GPIO number
and calling devm_gpio_request_one() conditionally.

Signed-off-by: Lothar Waßmann <L...@KARO-electronics.de>
---
drivers/net/ethernet/freescale/fec_main.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index e17d74f..1a983fc 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3233,7 +3233,7 @@ static int fec_enet_init(struct net_device *ndev)
#ifdef CONFIG_OF
static void fec_reset_phy(struct platform_device *pdev)
{
- int err, phy_reset;
+ struct gpio_desc *phy_reset;
int msec = 1;
struct device_node *np = pdev->dev.of_node;

@@ -3245,18 +3245,15 @@ static void fec_reset_phy(struct platform_device *pdev)
if (msec > 1000)
msec = 1;

- phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
- if (!gpio_is_valid(phy_reset))
- return;
-
- err = devm_gpio_request_one(&pdev->dev, phy_reset,
- GPIOF_OUT_INIT_LOW, "phy-reset");
- if (err) {
- dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
+ phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(phy_reset)) {
+ dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
+ PTR_ERR(phy_reset));
return;
}
msleep(msec);
- gpio_set_value_cansleep(phy_reset, 1);
+ gpiod_set_value_cansleep(phy_reset, 1);
}
#else /* CONFIG_OF */
static void fec_reset_phy(struct platform_device *pdev)
--
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Lothar Waßmann]

If a PHY uses ENET_OUT as reference clock, it may need a RESET to get
functional after the clock had been disabled.

Failure to do this results in the link state constantly toggling
between up and down:
fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 02188000.ethernet eth0: Link is Down
fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 02188000.ethernet eth0: Link is Down
[...]

Signed-off-by: Lothar Waßmann <L...@KARO-electronics.de>
---

drivers/net/ethernet/freescale/fec.h | 1 +
drivers/net/ethernet/freescale/fec_main.c | 46 ++++++++++++++++++++++++-------
2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 99d33e2..8ab4f7f 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -519,6 +519,7 @@ struct fec_enet_private {
int pause_flag;
int wol_flag;
u32 quirks;
+ struct gpio_desc *phy_reset;

struct napi_struct napi;
int csum_flags;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1a983fc..7ba2bbb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -66,6 +66,7 @@

static void set_multicast_list(struct net_device *ndev);
static void fec_enet_itr_coal_init(struct net_device *ndev);
+static void fec_reset_phy(struct platform_device *pdev);

#define DRIVER_NAME "fec"

@@ -1861,6 +1862,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
ret = clk_prepare_enable(fep->clk_enet_out);
if (ret)
goto failed_clk_enet_out;
+
+ fec_reset_phy(fep->pdev);
}
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
@@ -3231,13 +3234,32 @@ static int fec_enet_init(struct net_device *ndev)
}

#ifdef CONFIG_OF
-static void fec_reset_phy(struct platform_device *pdev)
+static struct gpio_desc *fec_get_reset_gpio(struct platform_device *pdev)
{
struct gpio_desc *phy_reset;
- int msec = 1;

struct device_node *np = pdev->dev.of_node;

if (!np)
+ return ERR_PTR(-ENODEV);
+

+ phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(phy_reset))
+ dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
+ PTR_ERR(phy_reset));

+ return phy_reset;
+}
+
+static void fec_reset_phy(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct net_device *ndev = platform_get_drvdata(pdev);
+ struct fec_enet_private *fep = netdev_priv(ndev);
+ int msec = 1;
+
+ if (!fep->phy_reset)
+ return;
+ if (!np)
return;

of_property_read_u32(np, "phy-reset-duration", &msec);
@@ -3245,17 +3267,16 @@ static void fec_reset_phy(struct platform_device *pdev)

if (msec > 1000)
msec = 1;

- phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
- GPIOD_OUT_LOW);
- if (IS_ERR(phy_reset)) {
- dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
- PTR_ERR(phy_reset));
- return;
- }
+ gpiod_set_value_cansleep(fep->phy_reset, 0);
msleep(msec);
- gpiod_set_value_cansleep(phy_reset, 1);
+ gpiod_set_value_cansleep(fep->phy_reset, 1);
}
#else /* CONFIG_OF */
+static void fec_get_reset_gpio(struct platform_device *pdev)
+{
+ return -EINVAL;
+}
+

static void fec_reset_phy(struct platform_device *pdev)
{

/*
@@ -3309,8 +3330,12 @@ fec_probe(struct platform_device *pdev)
struct device_node *np = pdev->dev.of_node, *phy_node;
int num_tx_qs;
int num_rx_qs;
+ struct gpio_desc *phy_reset;

fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
+ phy_reset = fec_get_reset_gpio(pdev);
+ if (IS_ERR(phy_reset) && PTR_ERR(phy_reset) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;

/* Init network device */
ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private),
@@ -3331,6 +3356,7 @@ fec_probe(struct platform_device *pdev)
fep->netdev = ndev;
fep->num_rx_queues = num_rx_qs;
fep->num_tx_queues = num_tx_qs;
+ fep->phy_reset = phy_reset;

#if !defined(CONFIG_M5272)
/* default enable pause frame auto negotiation */

[Fabio Estevam]

On Mon, Nov 30, 2015 at 9:32 AM, Lothar Waßmann <L...@karo-electronics.de> wrote:

> - gpio_set_value_cansleep(phy_reset, 1);
> + gpiod_set_value_cansleep(phy_reset, 1);

This will break some existing DTBs.

Currently the fec driver does not care whether 'phy-reset-gpios' is
active low or active high. (Yes, it was a mistake to assume in the
driver that it is always active low.)

After this change the polarity will be taken into account and may
break some dtb's.

I suggest you to send a single patch with the minimum changes to fix
the regression caused by 8fff755e9f8d ("net: fec: Ensure clocks are
enabled while using mdio bus").

[Duan Andy]

From: Lothar Waßmann <L...@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> König
> Subject: [PATCH 2/3] net: fec: convert to using gpiod framework

>
> Use gpiod_get_optional() instead of checking for a valid GPIO number and
> calling devm_gpio_request_one() conditionally.
>

> Signed-off-by: Lothar Waßmann <L...@KARO-electronics.de>

This API will judge the GPIO active polarity, there many imx boards in dts files don't care the polarity.
So pls drop the patch.

Or use gpiod_set_raw_value_cansleep() instead of gpiod_set_value_cansleep().

> }
> #else /* CONFIG_OF */
> static void fec_reset_phy(struct platform_device *pdev)
> --
> 2.1.4

¢éì¹» ®&Þ~º&¶ ¬–+-±éݶ ¥Šw®žË›±Êâmébžìdz¹Þ–)í…æèw* jg¬±¨ ¶‰šŽŠÝ¢j/ êäz¹Þ–Šà2ŠÞ™¨è­Ú&¢)ß¡«a¶Ú þø ®G« éh® æj:+v‰¨Šwè†Ù¥>Wš±êÞiÛaxP jØm¶ŸÿÃ-» +ƒùdš_

[Duan Andy]

From: Lothar Waßmann <L...@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> König

> Subject: [PATCH 3/3] net: fec: Reset ethernet PHY whenever the enet_out
> clock is being enabled

>
> If a PHY uses ENET_OUT as reference clock, it may need a RESET to get
> functional after the clock had been disabled.
>
> Failure to do this results in the link state constantly toggling between
> up and down:
> fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
> rx/tx fec 02188000.ethernet eth0: Link is Down fec 02188000.ethernet eth0:
> Link is Up - 100Mbps/Full - flow control rx/tx fec 02188000.ethernet eth0:
> Link is Down [...]
>

> Signed-off-by: Lothar Waßmann <L...@KARO-electronics.de>

> +*pdev)

> {
> struct gpio_desc *phy_reset;
> - int msec = 1;
> struct device_node *np = pdev->dev.of_node;
>
> if (!np)
> + return ERR_PTR(-ENODEV);

No need to check device node.

> +
> + phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(phy_reset))
> + dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
> + PTR_ERR(phy_reset));
> + return phy_reset;
> +}
> +
> +static void fec_reset_phy(struct platform_device *pdev) {

> + struct device_node *np = pdev->dev.of_node;
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct fec_enet_private *fep = netdev_priv(ndev);
> + int msec = 1;
> +
> + if (!fep->phy_reset)
> + return;
> + if (!np)
> return;

No need to check device node.

>
> of_property_read_u32(np, "phy-reset-duration", &msec); @@ -3245,17
> +3267,16 @@ static void fec_reset_phy(struct platform_device *pdev)
> if (msec > 1000)
> msec = 1;
>
> - phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
> - GPIOD_OUT_LOW);
> - if (IS_ERR(phy_reset)) {
> - dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
> - PTR_ERR(phy_reset));
> - return;
> - }
> + gpiod_set_value_cansleep(fep->phy_reset, 0);
> msleep(msec);
> - gpiod_set_value_cansleep(phy_reset, 1);
> + gpiod_set_value_cansleep(fep->phy_reset, 1);
> }
> #else /* CONFIG_OF */
> +static void fec_get_reset_gpio(struct platform_device *pdev) {

èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…éb 맲æìr¸›zX§» ®w¥Š{ayº ʇڙë,j ­¢f£¢·hš‹àz¹ ®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾ «‘êçzZ+ƒùšŽŠÝ¢j" ú!¶iO•æ¬z·švØ^ ¶ m§ÿðà nÆŠàþY&—

[Lothar Waßmann]

Hi,

> From: Lothar Waßmann <L...@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> > Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> > Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> > König
> > Subject: [PATCH 2/3] net: fec: convert to using gpiod framework
> >
> > Use gpiod_get_optional() instead of checking for a valid GPIO number and
> > calling devm_gpio_request_one() conditionally.
> >

> > Signed-off-by: Lothar Waßmann <L...@KARO-electronics.de>

I could prepare a patch that temporarily uses the raw functions and add
a comment to convert these to the proper functions once all DTB files
have been corrected and some time has passed to account for boards using
old dtbs with newer kernels.

I could also send a patch to convert all in-tree dts files to use
GPIO_ACTIVE_LOW if that is desired.


Lothar Waßmann