[RESEND PATCH net 00/10] net: ethernet: mediatek: a couple of fixes

[RESEND PATCH net 00/10] net: ethernet: mediatek: a couple of fixes

[Sean Wang]

a couple of fixes come out from integrating with linux-4.8 rc1
they all are verified and workable on linux-4.8 rc1

Sean Wang (10):
  net: ethernet: mediatek: fix fails from TX housekeeping due to
    incorrect port setup
  net: ethernet: mediatek: fix incorrect return value of devm_clk_get
    with EPROBE_DEFER
  net: ethernet: mediatek: fix API usage with skb_free_frag
  net: ethernet: mediatek: remove redundant free_irq for
    devm_request_irq allocated irq
  net: ethernet: mediatek: fix logic unbalance between probe and remove
  net: ethernet: mediatek: fix the loss of pin-mux setting for GMAC2
  net: ethernet: mediatek: fix issue of driver removal with interface is
    up
  net: ethernet: mediatek: fix the missing of_node_put() after node is
    used done inside mtk_mdio_init
  net: ethernet: mediatek: use devm_mdiobus_alloc instead of
    mdiobus_alloc inside mtk_mdio_init
  net: ethernet: mediatek: fix error handling inside mtk_mdio_init

 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 74 +++++++++++++++++++----------
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 ++
 2 files changed, 52 insertions(+), 25 deletions(-)

-- 
1.9.1

[RESEND PATCH net 01/10] net: ethernet: mediatek: fix fails from TX housekeeping due to incorrect port setup

[Sean Wang]

which net device the SKB is complete for depends on the forward port
on txd4 on the corresponding TX descriptor, but the information isn't
set up well in case of  SKB fragments that would lead to watchdog timeout
from the upper layer, so fix it up.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 1801fd8..6e4a6ca 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -587,14 +587,15 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
 	dma_addr_t mapped_addr;
 	unsigned int nr_frags;
 	int i, n_desc = 1;
-	u32 txd4 = 0;
+	u32 txd4 = 0, fport;
 
 	itxd = ring->next_free;
 	if (itxd == ring->last_free)
 		return -ENOMEM;
 
 	/* set the forward port */
-	txd4 |= (mac->id + 1) << TX_DMA_FPORT_SHIFT;
+	fport = (mac->id + 1) << TX_DMA_FPORT_SHIFT;
+	txd4 |= fport;
 
 	tx_buf = mtk_desc_to_tx_buf(ring, itxd);
 	memset(tx_buf, 0, sizeof(*tx_buf));
@@ -652,7 +653,7 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
 			WRITE_ONCE(txd->txd3, (TX_DMA_SWC |
 					       TX_DMA_PLEN0(frag_map_size) |
 					       last_frag * TX_DMA_LS0));
-			WRITE_ONCE(txd->txd4, 0);
+			WRITE_ONCE(txd->txd4, fport);
 
 			tx_buf->skb = (struct sk_buff *)MTK_DMA_DUMMY_DESC;
 			tx_buf = mtk_desc_to_tx_buf(ring, txd);
-- 
1.9.1

[RESEND PATCH net 02/10] net: ethernet: mediatek: fix incorrect return value of devm_clk_get with EPROBE_DEFER

[Sean Wang]

If the return value of devm_clk_get is EPROBE_DEFER, we should
defer probing the driver. The change is verified and works based
on 4.8-rc1 staying with the latest clk-next code for MT7623.

Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 6e4a6ca..02b048f 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1851,8 +1851,15 @@ static int mtk_probe(struct platform_device *pdev)
 	eth->clk_gp1 = devm_clk_get(&pdev->dev, "gp1");
 	eth->clk_gp2 = devm_clk_get(&pdev->dev, "gp2");
 	if (IS_ERR(eth->clk_esw) || IS_ERR(eth->clk_gp1) ||
-	    IS_ERR(eth->clk_gp2) || IS_ERR(eth->clk_ethif))
-		return -ENODEV;
+	    IS_ERR(eth->clk_gp2) || IS_ERR(eth->clk_ethif)) {
+		if (PTR_ERR(eth->clk_esw) == -EPROBE_DEFER ||
+		    PTR_ERR(eth->clk_gp1) == -EPROBE_DEFER ||
+		    PTR_ERR(eth->clk_gp1) == -EPROBE_DEFER ||
+		    PTR_ERR(eth->clk_gp2) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		else
+			return -ENODEV;
+	}
 
 	clk_prepare_enable(eth->clk_ethif);
 	clk_prepare_enable(eth->clk_esw);
-- 
1.9.1

[RESEND PATCH net 03/10] net: ethernet: mediatek: fix API usage with skb_free_frag

[Sean Wang]

use skb_free_frag() instead of legacy put_page()

Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 02b048f..1b131a1 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -864,7 +864,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 		/* receive data */
 		skb = build_skb(data, ring->frag_size);
 		if (unlikely(!skb)) {
-			put_page(virt_to_head_page(new_data));
+			skb_free_frag(new_data);
 			netdev->stats.rx_dropped++;
 			goto release_desc;
 		}
-- 
1.9.1

[RESEND PATCH net 04/10] net: ethernet: mediatek: remove redundant free_irq for devm_request_irq allocated irq

[Sean Wang]

these irqs are not used for shared irq and disabled during ethernet stops.
irq requested by devm_request_irq is safe to be freed automatically on
driver detach.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 1b131a1..9883dac 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1506,8 +1506,6 @@ static void mtk_uninit(struct net_device *dev)
 	phy_disconnect(mac->phy_dev);
 	mtk_mdio_cleanup(eth);
 	mtk_irq_disable(eth, ~0);
-	free_irq(eth->irq[1], dev);
-	free_irq(eth->irq[2], dev);
 }
 
 static int mtk_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
-- 
1.9.1

[RESEND PATCH net 05/10] net: ethernet: mediatek: fix logic unbalance between probe and remove

[Sean Wang]

original mdio_cleanup is not in the symmetric place against where
mdio_init is, so relocate mdio_cleanup to the right one.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 9883dac..5bd31f8 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1504,7 +1504,6 @@ static void mtk_uninit(struct net_device *dev)
 	struct mtk_eth *eth = mac->hw;
 
 	phy_disconnect(mac->phy_dev);
-	mtk_mdio_cleanup(eth);
 	mtk_irq_disable(eth, ~0);
 }
 
@@ -1915,6 +1914,7 @@ static int mtk_remove(struct platform_device *pdev)
 	netif_napi_del(&eth->tx_napi);
 	netif_napi_del(&eth->rx_napi);
 	mtk_cleanup(eth);
+	mtk_mdio_cleanup(eth);
 
 	return 0;
 }
-- 
1.9.1

[RESEND PATCH net 06/10] net: ethernet: mediatek: fix the loss of pin-mux setting for GMAC2

[Sean Wang]

ommited the setting about pin-mux which results in incorrect signals
being routed on GMAC2.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 14 ++++++++++++++
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 5bd31f8..0a4c782 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1415,6 +1415,7 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
 	usleep_range(10, 20);
 	reset_control_deassert(eth->rstc);
 	usleep_range(10, 20);
+	pinctrl_select_state(eth->pins, eth->ephy_default);
 
 	/* Set GE2 driving and slew rate */
 	regmap_write(eth->pctl, GPIO_DRV_SEL10, 0xa00);
@@ -1858,6 +1859,19 @@ static int mtk_probe(struct platform_device *pdev)
 			return -ENODEV;
 	}
 
+	eth->pins = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(eth->pins)) {
+		dev_err(&pdev->dev, "cannot get pinctrl\n");
+		return PTR_ERR(eth->pins);
+	}
+
+	eth->ephy_default =
+		pinctrl_lookup_state(eth->pins, "default");
+	if (IS_ERR(eth->ephy_default)) {
+		dev_err(&pdev->dev, "cannot get pinctrl state\n");
+		return PTR_ERR(eth->ephy_default);
+	}
+
 	clk_prepare_enable(eth->clk_ethif);
 	clk_prepare_enable(eth->clk_esw);
 	clk_prepare_enable(eth->clk_gp1);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index f82e3ac..13d3f1b 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -404,6 +404,9 @@ struct mtk_eth {
 	struct clk			*clk_esw;
 	struct clk			*clk_gp1;
 	struct clk			*clk_gp2;
+	struct pinctrl			*pins;
+	struct pinctrl_state		*ephy_default;
+
 	struct mii_bus			*mii_bus;
 	struct work_struct		pending_work;
 };
-- 
1.9.1

[RESEND PATCH net 07/10] net: ethernet: mediatek: fix issue of driver removal with interface is up

[Sean Wang]

1) mtk_stop() must be called to stop for freeing DMA resources
acquired and restoring state changed by mtk_open() when module
removal.

2) group clock disabled related function into mtk_hw_deinit which
could be reused with others functionality such as the whole ethernet
reset that would be posted in the later series of patches.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 0a4c782..c573475 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1478,6 +1478,16 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
 	return 0;
 }
 
+static int mtk_hw_deinit(struct mtk_eth *eth)
+{
+	clk_disable_unprepare(eth->clk_esw);
+	clk_disable_unprepare(eth->clk_gp1);
+	clk_disable_unprepare(eth->clk_gp2);
+	clk_disable_unprepare(eth->clk_ethif);
+
+	return 0;
+}
+
 static int __init mtk_init(struct net_device *dev)
 {
 	struct mtk_mac *mac = netdev_priv(dev);
@@ -1919,11 +1929,15 @@ err_free_dev:
 static int mtk_remove(struct platform_device *pdev)
 {
 	struct mtk_eth *eth = platform_get_drvdata(pdev);
+	int i;
 
-	clk_disable_unprepare(eth->clk_ethif);
-	clk_disable_unprepare(eth->clk_esw);
-	clk_disable_unprepare(eth->clk_gp1);
-	clk_disable_unprepare(eth->clk_gp2);
+	/* stop all devices to make sure that dma is properly shut down */
+	for (i = 0; i < MTK_MAC_COUNT; i++) {
+		if (!eth->netdev[i])
+			continue;
+		mtk_stop(eth->netdev[i]);
+	}
+	mtk_hw_deinit(eth);
 
 	netif_napi_del(&eth->tx_napi);
 	netif_napi_del(&eth->rx_napi);
-- 
1.9.1

[RESEND PATCH net 08/10] net: ethernet: mediatek: fix the missing of_node_put() after node is used done inside mtk_mdio_init

[Sean Wang]

This patch adds the missing of_node_put() after finishing the usage
of of_get_child_by_name.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index c573475..e3baa63 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -320,6 +320,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	err = of_mdiobus_register(eth->mii_bus, mii_np);
 	if (err)
 		goto err_free_bus;
+	of_node_put(mii_np);
 
 	return 0;
 
-- 
1.9.1

[RESEND PATCH net 09/10] net: ethernet: mediatek: use devm_mdiobus_alloc instead of mdiobus_alloc inside mtk_mdio_init

[Sean Wang]

a lot of parts in the driver uses devm_* APIs to gain benefits from the
device resource management, so devm_mdiobus_alloc is also used instead
of mdiobus_alloc to have more elegant code flow.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index e3baa63..05d85da 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 		goto err_put_node;
 	}
 
-	eth->mii_bus = mdiobus_alloc();
+	eth->mii_bus = devm_mdiobus_alloc(eth->dev);
 	if (!eth->mii_bus) {
 		err = -ENOMEM;
 		goto err_put_node;
@@ -318,18 +318,9 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 
 	snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name);
 	err = of_mdiobus_register(eth->mii_bus, mii_np);
-	if (err)
-		goto err_free_bus;
-	of_node_put(mii_np);
-
-	return 0;
-
-err_free_bus:
-	mdiobus_free(eth->mii_bus);
 
 err_put_node:
 	of_node_put(mii_np);
-	eth->mii_bus = NULL;
 	return err;
 }
 
@@ -339,8 +330,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth)
 		return;
 
 	mdiobus_unregister(eth->mii_bus);
-	of_node_put(eth->mii_bus->dev.of_node);
-	mdiobus_free(eth->mii_bus);
 }
 
 static inline void mtk_irq_disable(struct mtk_eth *eth, u32 mask)
-- 
1.9.1

[RESEND PATCH net 10/10] net: ethernet: mediatek: fix error handling inside mtk_mdio_init

[Sean Wang]

return -ENODEV if no child is found in MDIO bus.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 05d85da..2d547c2 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -300,7 +300,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
 	}
 
 	if (!of_device_is_available(mii_np)) {
-		err = 0;
+		err = -ENODEV;
 		goto err_put_node;
 	}
 
-- 
1.9.1

Re: [RESEND PATCH net 01/10] net: ethernet: mediatek: fix fails from TX housekeeping due to incorrect port setup

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> which net device the SKB is complete for depends on the forward port
> on txd4 on the corresponding TX descriptor, but the information isn't
> set up well in case of  SKB fragments that would lead to watchdog timeout
> from the upper layer, so fix it up.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>

Acked-by: John Crispin <john@phrozen.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 1801fd8..6e4a6ca 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -587,14 +587,15 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
>  	dma_addr_t mapped_addr;
>  	unsigned int nr_frags;
>  	int i, n_desc = 1;
> -	u32 txd4 = 0;
> +	u32 txd4 = 0, fport;
>  
>  	itxd = ring->next_free;
>  	if (itxd == ring->last_free)
>  		return -ENOMEM;
>  
>  	/* set the forward port */
> -	txd4 |= (mac->id + 1) << TX_DMA_FPORT_SHIFT;
> +	fport = (mac->id + 1) << TX_DMA_FPORT_SHIFT;
> +	txd4 |= fport;
>  
>  	tx_buf = mtk_desc_to_tx_buf(ring, itxd);
>  	memset(tx_buf, 0, sizeof(*tx_buf));
> @@ -652,7 +653,7 @@ static int mtk_tx_map(struct sk_buff *skb, struct net_device *dev,
>  			WRITE_ONCE(txd->txd3, (TX_DMA_SWC |
>  					       TX_DMA_PLEN0(frag_map_size) |
>  					       last_frag * TX_DMA_LS0));
> -			WRITE_ONCE(txd->txd4, 0);
> +			WRITE_ONCE(txd->txd4, fport);
>  
>  			tx_buf->skb = (struct sk_buff *)MTK_DMA_DUMMY_DESC;
>  			tx_buf = mtk_desc_to_tx_buf(ring, txd);
> 

Re: [RESEND PATCH net 04/10] net: ethernet: mediatek: remove redundant free_irq for devm_request_irq allocated irq

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> these irqs are not used for shared irq and disabled during ethernet stops.
> irq requested by devm_request_irq is safe to be freed automatically on
> driver detach.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>

Acked-by: John Crispin <john@phrozen.org>

> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 1b131a1..9883dac 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -1506,8 +1506,6 @@ static void mtk_uninit(struct net_device *dev)
>  	phy_disconnect(mac->phy_dev);
>  	mtk_mdio_cleanup(eth);
>  	mtk_irq_disable(eth, ~0);
> -	free_irq(eth->irq[1], dev);
> -	free_irq(eth->irq[2], dev);
>  }
>  
>  static int mtk_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> 

Re: [RESEND PATCH net 03/10] net: ethernet: mediatek: fix API usage with skb_free_frag

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> use skb_free_frag() instead of legacy put_page()
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>

Acked-by: John Crispin <john@phrozen.org>

> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 02b048f..1b131a1 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -864,7 +864,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
>  		/* receive data */
>  		skb = build_skb(data, ring->frag_size);
>  		if (unlikely(!skb)) {
> -			put_page(virt_to_head_page(new_data));
> +			skb_free_frag(new_data);
>  			netdev->stats.rx_dropped++;
>  			goto release_desc;
>  		}
> 

Re: [RESEND PATCH net 05/10] net: ethernet: mediatek: fix logic unbalance between probe and remove

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> original mdio_cleanup is not in the symmetric place against where
> mdio_init is, so relocate mdio_cleanup to the right one.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>

Acked-by: John Crispin <john@phrozen.org>

> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 9883dac..5bd31f8 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -1504,7 +1504,6 @@ static void mtk_uninit(struct net_device *dev)
>  	struct mtk_eth *eth = mac->hw;
>  
>  	phy_disconnect(mac->phy_dev);
> -	mtk_mdio_cleanup(eth);
>  	mtk_irq_disable(eth, ~0);
>  }
>  
> @@ -1915,6 +1914,7 @@ static int mtk_remove(struct platform_device *pdev)
>  	netif_napi_del(&eth->tx_napi);
>  	netif_napi_del(&eth->rx_napi);
>  	mtk_cleanup(eth);
> +	mtk_mdio_cleanup(eth);
>  
>  	return 0;
>  }
> 

Re: [RESEND PATCH net 06/10] net: ethernet: mediatek: fix the loss of pin-mux setting for GMAC2

[Andrew Lunn]

On Thu, Aug 25, 2016 at 06:44:57PM +0800, Sean Wang wrote:
> ommited the setting about pin-mux which results in incorrect signals
> being routed on GMAC2.

Hi Sean

Please could you explain this some more. I don't know too much about
pinctrl, but i've never seen a driver have to do anything with it. The
core driver code handles it all, selecting the default state. See
seeing this here makes me wonder if it is correct.

Thanks
	Andrew

> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 14 ++++++++++++++
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 5bd31f8..0a4c782 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -1415,6 +1415,7 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
>  	usleep_range(10, 20);
>  	reset_control_deassert(eth->rstc);
>  	usleep_range(10, 20);
> +	pinctrl_select_state(eth->pins, eth->ephy_default);
>  
>  	/* Set GE2 driving and slew rate */
>  	regmap_write(eth->pctl, GPIO_DRV_SEL10, 0xa00);
> @@ -1858,6 +1859,19 @@ static int mtk_probe(struct platform_device *pdev)
>  			return -ENODEV;
>  	}
>  
> +	eth->pins = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(eth->pins)) {
> +		dev_err(&pdev->dev, "cannot get pinctrl\n");
> +		return PTR_ERR(eth->pins);
> +	}
> +
> +	eth->ephy_default =
> +		pinctrl_lookup_state(eth->pins, "default");
> +	if (IS_ERR(eth->ephy_default)) {
> +		dev_err(&pdev->dev, "cannot get pinctrl state\n");
> +		return PTR_ERR(eth->ephy_default);
> +	}
> +
>  	clk_prepare_enable(eth->clk_ethif);
>  	clk_prepare_enable(eth->clk_esw);
>  	clk_prepare_enable(eth->clk_gp1);
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index f82e3ac..13d3f1b 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -404,6 +404,9 @@ struct mtk_eth {
>  	struct clk			*clk_esw;
>  	struct clk			*clk_gp1;
>  	struct clk			*clk_gp2;
> +	struct pinctrl			*pins;
> +	struct pinctrl_state		*ephy_default;
> +
>  	struct mii_bus			*mii_bus;
>  	struct work_struct		pending_work;
>  };
> -- 
> 1.9.1
> 

Re: [RESEND PATCH net 07/10] net: ethernet: mediatek: fix issue of driver removal with interface is up

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> 1) mtk_stop() must be called to stop for freeing DMA resources
> acquired and restoring state changed by mtk_open() when module
> removal.
> 
> 2) group clock disabled related function into mtk_hw_deinit which
> could be reused with others functionality such as the whole ethernet
> reset that would be posted in the later series of patches.
> 

Hi Sean,

these are 2 unrelated changes so they really need to go into two
separate patches. i also think that change 1) would better fit into the
future series making use of that functionality.

	John

> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 0a4c782..c573475 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -1478,6 +1478,16 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
>  	return 0;
>  }
>  
> +static int mtk_hw_deinit(struct mtk_eth *eth)
> +{
> +	clk_disable_unprepare(eth->clk_esw);
> +	clk_disable_unprepare(eth->clk_gp1);
> +	clk_disable_unprepare(eth->clk_gp2);
> +	clk_disable_unprepare(eth->clk_ethif);
> +
> +	return 0;
> +}
> +
>  static int __init mtk_init(struct net_device *dev)
>  {
>  	struct mtk_mac *mac = netdev_priv(dev);
> @@ -1919,11 +1929,15 @@ err_free_dev:
>  static int mtk_remove(struct platform_device *pdev)
>  {
>  	struct mtk_eth *eth = platform_get_drvdata(pdev);
> +	int i;
>  
> -	clk_disable_unprepare(eth->clk_ethif);
> -	clk_disable_unprepare(eth->clk_esw);
> -	clk_disable_unprepare(eth->clk_gp1);
> -	clk_disable_unprepare(eth->clk_gp2);
> +	/* stop all devices to make sure that dma is properly shut down */
> +	for (i = 0; i < MTK_MAC_COUNT; i++) {
> +		if (!eth->netdev[i])
> +			continue;
> +		mtk_stop(eth->netdev[i]);
> +	}
> +	mtk_hw_deinit(eth);
>  
>  	netif_napi_del(&eth->tx_napi);
>  	netif_napi_del(&eth->rx_napi);
> 

Re: [RESEND PATCH net 08/10] net: ethernet: mediatek: fix the missing of_node_put() after node is used done inside mtk_mdio_init

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> This patch adds the missing of_node_put() after finishing the usage
> of of_get_child_by_name.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Acked-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>

> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index c573475..e3baa63 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -320,6 +320,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
>  	err = of_mdiobus_register(eth->mii_bus, mii_np);
>  	if (err)
>  		goto err_free_bus;
> +	of_node_put(mii_np);
>  
>  	return 0;
>  
> 

Re: [RESEND PATCH net 09/10] net: ethernet: mediatek: use devm_mdiobus_alloc instead of mdiobus_alloc inside mtk_mdio_init

[John Crispin]

Hi Sean,

small nitpick inline

On 25/08/2016 12:45, Sean Wang wrote:
> a lot of parts in the driver uses devm_* APIs to gain benefits from the
> device resource management, so devm_mdiobus_alloc is also used instead
> of mdiobus_alloc to have more elegant code flow.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index e3baa63..05d85da 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
>  		goto err_put_node;
>  	}
>  
> -	eth->mii_bus = mdiobus_alloc();
> +	eth->mii_bus = devm_mdiobus_alloc(eth->dev);
>  	if (!eth->mii_bus) {
>  		err = -ENOMEM;
>  		goto err_put_node;
> @@ -318,18 +318,9 @@ static int mtk_mdio_init(struct mtk_eth *eth)
>  
>  	snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name);
>  	err = of_mdiobus_register(eth->mii_bus, mii_np);
> -	if (err)
> -		goto err_free_bus;
> -	of_node_put(mii_np);
> -
> -	return 0;
> -
> -err_free_bus:
> -	mdiobus_free(eth->mii_bus);
>  
>  err_put_node:
>  	of_node_put(mii_np);
> -	eth->mii_bus = NULL;
>  	return err;

you might want to rename err to ret. that would make it more readable.
right now it looks like the code always flows through the error path.

	John

>  }
>  
> @@ -339,8 +330,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth)
>  		return;
>  
>  	mdiobus_unregister(eth->mii_bus);
> -	of_node_put(eth->mii_bus->dev.of_node);
> -	mdiobus_free(eth->mii_bus);
>  }
>  
>  static inline void mtk_irq_disable(struct mtk_eth *eth, u32 mask)
> 

Re: [RESEND PATCH net 10/10] net: ethernet: mediatek: fix error handling inside mtk_mdio_init

[John Crispin]

On 25/08/2016 12:45, Sean Wang wrote:
> return -ENODEV if no child is found in MDIO bus.
> 
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

Acked-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>

> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 05d85da..2d547c2 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -300,7 +300,7 @@ static int mtk_mdio_init(struct mtk_eth *eth)
>  	}
>  
>  	if (!of_device_is_available(mii_np)) {
> -		err = 0;
> +		err = -ENODEV;
>  		goto err_put_node;
>  	}
>  
> 

Re: [RESEND PATCH net 02/10] net: ethernet: mediatek: fix incorrect return value of devm_clk_get with EPROBE_DEFER

[John Crispin]

On 25/08/2016 12:44, Sean Wang wrote:
> If the return value of devm_clk_get is EPROBE_DEFER, we should
> defer probing the driver. The change is verified and works based
> on 4.8-rc1 staying with the latest clk-next code for MT7623.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 6e4a6ca..02b048f 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -1851,8 +1851,15 @@ static int mtk_probe(struct platform_device *pdev)
>  	eth->clk_gp1 = devm_clk_get(&pdev->dev, "gp1");
>  	eth->clk_gp2 = devm_clk_get(&pdev->dev, "gp2");
>  	if (IS_ERR(eth->clk_esw) || IS_ERR(eth->clk_gp1) ||
> -	    IS_ERR(eth->clk_gp2) || IS_ERR(eth->clk_ethif))
> -		return -ENODEV;
> +	    IS_ERR(eth->clk_gp2) || IS_ERR(eth->clk_ethif)) {
> +		if (PTR_ERR(eth->clk_esw) == -EPROBE_DEFER ||
> +		    PTR_ERR(eth->clk_gp1) == -EPROBE_DEFER ||
> +		    PTR_ERR(eth->clk_gp1) == -EPROBE_DEFER ||
> +		    PTR_ERR(eth->clk_gp2) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		else
> +			return -ENODEV;
> +	}

Hi Sean,

this looks a bit tedious. maybe a better solution would be to add an
array to struct mtk_eth for the clocks and an enum for the index
mapping. that would allow the usage of loops to work out if all clocks
are fine. the following code calling clk_prepare_enable() could then
also be turned into a loop

	John

>  
>  	clk_prepare_enable(eth->clk_ethif);
>  	clk_prepare_enable(eth->clk_esw);
> 

Re: [RESEND PATCH net 06/10] net: ethernet: mediatek: fix the loss

[Sean Wang]

On Thu, 25 Aug 2016 15:30:34 +0200, Andrew Lunn wrote:
>On Thu, Aug 25, 2016 at 06:44:57PM +0800, Sean Wang wrote:
>> ommited the setting about pin-mux which results in incorrect signals
>> being routed on GMAC2.
>
>Hi Sean
>
>Please could you explain this some more. I don't know too much about
>pinctrl, but i've never seen a driver have to do anything with it. The
>core driver code handles it all, selecting the default state. See
>seeing this here makes me wonder if it is correct.
>
>Thanks
>	Andrew
>
>>

Hi Andrew,

Here pinctrl is used to setup what function the group of the pins is for.

The group of the pins could be configured for the function provided 
by the SoC, such as general purpose I/O or specific function such as
ethernet depending on what products or boards you design for various 
customers or vendors. Thanks for device tree introducing, it is easy 
to find what resources the board needs including the pins usage is 
also defined here.
 
Pins are limited resource that is also meant for the cost 
so that it is common way seen on embedded system.

thanks,
Sean

>> +	eth->pins = devm_pinctrl_get(&pdev->dev);
>> +	if (IS_ERR(eth->pins)) {
>> +		dev_err(&pdev->dev, "cannot get pinctrl\n");
>> +		return PTR_ERR(eth->pins);
>> +	}
>> +
>> +	eth->ephy_default =
>> +		pinctrl_lookup_state(eth->pins, "default");
>> +	if (IS_ERR(eth->ephy_default)) {
>> +		dev_err(&pdev->dev, "cannot get pinctrl state\n");
>> +		return PTR_ERR(eth->ephy_default);
>> +	}
>> +
>>  	clk_prepare_enable(eth->clk_ethif);
>>  	clk_prepare_enable(eth->clk_esw);
>>  	clk_prepare_enable(eth->clk_gp1);
>> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
>> index f82e3ac..13d3f1b 100644
>> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
>> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
>> @@ -404,6 +404,9 @@ struct mtk_eth {
>>  	struct clk			*clk_esw;
>>  	struct clk			*clk_gp1;
>>  	struct clk			*clk_gp2;
>> +	struct pinctrl			*pins;
>> +	struct pinctrl_state		*ephy_default;
>> +
>>  	struct mii_bus			*mii_bus;
>>  	struct work_struct		pending_work;
>>  };
>> -- 
>> 1.9.1

Re: [RESEND PATCH net 02/10] net: ethernet: mediatek: fix incorrect

[Sean Wang]

On Date: Thu, 25 Aug 2016 15:49:10 +0200, John Crispin wrote:
>On 25/08/2016 12:44, Sean Wang wrote:
>> If the return value of devm_clk_get is EPROBE_DEFER, we should
>> defer probing the driver. The change is verified and works based
>> on 4.8-rc1 staying with the latest clk-next code for MT7623.
>> 
>> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> ---
>>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
...
>> +		    PTR_ERR(eth->clk_gp2) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
>> +		else
>> +			return -ENODEV;
>> +	}
>
>Hi Sean,
>
>this looks a bit tedious. maybe a better solution would be to add an
>array to struct mtk_eth for the clocks and an enum for the index
>mapping. that would allow the usage of loops to work out if all clocks
>are fine. the following code calling clk_prepare_enable() could then
>also be turned into a loop
>
>	John

The suggestion is better, so I will use your suggested way to 
to implement the logic in the next version.

>
>>  
>>  	clk_prepare_enable(eth->clk_ethif);
>>  	clk_prepare_enable(eth->clk_esw);
>> 

Re: [RESEND PATCH net 07/10] net: ethernet: mediatek: fix issue of driver removal with interface is up

[Sean Wang]

Date: Thu, 25 Aug 2016 15:35:34 +0200, John Crispin wrote:
>On 25/08/2016 12:44, Sean Wang wrote:
>> 1) mtk_stop() must be called to stop for freeing DMA resources
>> acquired and restoring state changed by mtk_open() when module
>> removal.
>> 
>> 2) group clock disabled related function into mtk_hw_deinit which
>> could be reused with others functionality such as the whole ethernet
>> reset that would be posted in the later series of patches.
>> 
>
>Hi Sean,
>
>these are 2 unrelated changes so they really need to go into two
>separate patches. i also think that change 1) would better fit into the
>future series making use of that functionality.
>
>	John
>

okay. splitting makes sense more

I will leave 1) here 
and move 2) into the better place that is one part of the future series 
about whole ethernet reset, I will post them after the current series is done

thanks for your effort on reviewing

>> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> ---
>>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> index 0a4c782..c573475 100644
>> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
>> @@ -1478,6 +1478,16 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
>>  	return 0;
>>  }
>>  
>> +static int mtk_hw_deinit(struct mtk_eth *eth)
>> +{
>> +	clk_disable_unprepare(eth->clk_esw);
>> +	clk_disable_unprepare(eth->clk_gp1);
>> +	clk_disable_unprepare(eth->clk_gp2);
>> +	clk_disable_unprepare(eth->clk_ethif);
>> +
>> +	return 0;
>> +}
>> +
>>  static int __init mtk_init(struct net_device *dev)
>>  {
>>  	struct mtk_mac *mac = netdev_priv(dev);
>> @@ -1919,11 +1929,15 @@ err_free_dev:
>>  static int mtk_remove(struct platform_device *pdev)
>>  {
>>  	struct mtk_eth *eth = platform_get_drvdata(pdev);
>> +	int i;
>>  
>> -	clk_disable_unprepare(eth->clk_ethif);
>> -	clk_disable_unprepare(eth->clk_esw);
>> -	clk_disable_unprepare(eth->clk_gp1);
>> -	clk_disable_unprepare(eth->clk_gp2);
>> +	/* stop all devices to make sure that dma is properly shut down */
>> +	for (i = 0; i < MTK_MAC_COUNT; i++) {
>> +		if (!eth->netdev[i])
>> +			continue;
>> +		mtk_stop(eth->netdev[i]);
>> +	}
>> +	mtk_hw_deinit(eth);
>>  
>>  	netif_napi_del(&eth->tx_napi);
>>  	netif_napi_del(&eth->rx_napi);
>> 

Re: [RESEND PATCH net 09/10] net: ethernet: mediatek: use devm_mdiobus_alloc instead of mdiobus_alloc inside mtk_mdio_init

[sean.wang]

On Date: Thu, 25 Aug 2016 15:39:08 +0200, John Crispin wrote:
>Hi Sean, >
>small nitpick inline
>
>On 25/08/2016 12:45, Sean Wang wrote:
>> a lot of parts in the driver uses devm_* APIs to gain benefits from the
....
  
>> -	eth->mii_bus = mdiobus_alloc();
>> +	eth->mii_bus = devm_mdiobus_alloc(eth->dev);
>>  	if (!eth->mii_bus) {
>>  		err = -ENOMEM;
>>  		goto err_put_node;
>> @@ -318,18 +318,9 @@ static int mtk_mdio_init(struct mtk_eth *eth)
>>  
>>  	snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name);
>>  	err = of_mdiobus_register(eth->mii_bus, mii_np);
>> -	if (err)
>> -		goto err_free_bus;
>> -	of_node_put(mii_np);
>> -
>> -	return 0;
>> -
>> -err_free_bus:
>> -	mdiobus_free(eth->mii_bus);
>>  
>>  err_put_node:
>>  	of_node_put(mii_np);
>> -	eth->mii_bus = NULL;
>>  	return err;
>
>you might want to rename err to ret. that would make it more readable.
>right now it looks like the code always flows through the error path.
>
>	John
>

okay, i will change this from err to ret for readable code
and my thought is using common code provided by the framework helps
to 
1) have simplified the code flow as [1] says 
2) decrease the risk of incorrect error handling by human
3) only a few drivers used it since it ware proposed on linux 3.16,
so just hope to promote for this.

[1] https://patchwork.ozlabs.org/patch/344093/

>>  }
>>  
>> @@ -339,8 +330,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth)
>>  		return;
>>  
>>  	mdiobus_unregister(eth->mii_bus);
>> -	of_node_put(eth->mii_bus->dev.of_node);
>> -	mdiobus_free(eth->mii_bus);
>>  }

Re: [RESEND PATCH net 06/10] net: ethernet: mediatek: fix the loss

[Andrew Lunn]

> Hi Andrew,
> 
> Here pinctrl is used to setup what function the group of the pins is
> for.

Agreed.
 
> The group of the pins could be configured for the function provided 
> by the SoC, such as general purpose I/O or specific function such as
> ethernet depending on what products or boards you design for various 
> customers or vendors. Thanks for device tree introducing, it is easy 
> to find what resources the board needs including the pins usage is 
> also defined here.

All clear. However, if the ethernet driver has loaded, it means the
device tree says the ethernet should be loaded, unless it happens to
be on some discoverable bus. And so the device tree node for the
ethernet should also contain the needed pinctrl properties.  The core
driver code should of seen these properties and already enabled the
correct pinctrl state before the driver probes.

This is how every other driver works. Like i said, i don't think i've
seen any other driver do its own pinctrl. So i just need a simple
description, what is different here, why does this driver need to do
it, when no other does?

    Andrew

Re: [RESEND PATCH net 06/10] net: ethernet: mediatek: fix the loss

[Sean Wang]

Date: Fri, 26 Aug 2016 16:17:59 +0200, Andrew Lunn wrote:
>> Hi Andrew,
>> 
>> Here pinctrl is used to setup what function the group of the pins is
>> for.
>
>Agreed.
> 
>> The group of the pins could be configured for the function provided 
>> by the SoC, such as general purpose I/O or specific function such as
>> ethernet depending on what products or boards you design for various 
>> customers or vendors. Thanks for device tree introducing, it is easy 
>> to find what resources the board needs including the pins usage is 
>> also defined here.
>
>All clear. However, if the ethernet driver has loaded, it means the
>device tree says the ethernet should be loaded, unless it happens to
>be on some discoverable bus. And so the device tree node for the
>ethernet should also contain the needed pinctrl properties.  The core
>driver code should of seen these properties and already enabled the
>correct pinctrl state before the driver probes.
>
>This is how every other driver works. Like i said, i don't think i've
>seen any other driver do its own pinctrl. So i just need a simple
>description, what is different here, why does this driver need to do
>it, when no other does?
>
>    Andrew
>

You are right
all that I need about pinctrl are all being done with core driver 
as you said, so the patch I did seems the redundant work and i will remove 
it from the patch set.

thanks for your patient and careful reviewing and that also helps me getting 
familiar with based driver with pinctrl more :)

Sean