netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Bugfix for the netsec driver
@ 2018-10-19  1:08 masahisa.kojima
  2018-10-19  1:08 ` [PATCH 1/3] net: socionext: Stop PHY before resetting netsec masahisa.kojima
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: masahisa.kojima @ 2018-10-19  1:08 UTC (permalink / raw)
  To: netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo, Masahisa Kojima

From: Masahisa Kojima <masahisa.kojima@linaro.org>

This patch series include bugfix for the netsec ethernet
controller driver, fix the problem in interface down/up.

Masahisa Kojima (3):
  net: socionext: stop PHY before resetting netsec
  net: socionext: Add dummy PHY register read in phy_write()
  net: socionext: reset tx queue in ndo_stop

 drivers/net/ethernet/socionext/netsec.c | 36 +++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

-- 
2.14.2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] net: socionext: Stop PHY before resetting netsec
  2018-10-19  1:08 [PATCH 0/3] Bugfix for the netsec driver masahisa.kojima
@ 2018-10-19  1:08 ` masahisa.kojima
  2018-10-19  2:59   ` Florian Fainelli
  2018-10-19  1:08 ` [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write() masahisa.kojima
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: masahisa.kojima @ 2018-10-19  1:08 UTC (permalink / raw)
  To: netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo, Masahisa Kojima

From: Masahisa Kojima <masahisa.kojima@linaro.org>

After resetting netsec IP, driver have to wait until
netsec mode turns to NRM mode.
But sometimes mode transition to NRM will not complete
if the PHY is in normal operation state.
To avoid this situation, stop PHY before resetting netsec.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
---
 drivers/net/ethernet/socionext/netsec.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 4289ccb26e4e..273cc5fc07e0 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1343,11 +1343,11 @@ static int netsec_netdev_stop(struct net_device *ndev)
 	netsec_uninit_pkt_dring(priv, NETSEC_RING_TX);
 	netsec_uninit_pkt_dring(priv, NETSEC_RING_RX);
 
-	ret = netsec_reset_hardware(priv, false);
-
 	phy_stop(ndev->phydev);
 	phy_disconnect(ndev->phydev);
 
+	ret = netsec_reset_hardware(priv, false);
+
 	pm_runtime_put_sync(priv->dev);
 
 	return ret;
@@ -1415,7 +1415,7 @@ static const struct net_device_ops netsec_netdev_ops = {
 };
 
 static int netsec_of_probe(struct platform_device *pdev,
-			   struct netsec_priv *priv)
+			   struct netsec_priv *priv, u32 *phy_addr)
 {
 	priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
 	if (!priv->phy_np) {
@@ -1423,6 +1423,8 @@ static int netsec_of_probe(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	*phy_addr = of_mdio_parse_addr(&pdev->dev, priv->phy_np);
+
 	priv->clk = devm_clk_get(&pdev->dev, NULL); /* get by 'phy_ref_clk' */
 	if (IS_ERR(priv->clk)) {
 		dev_err(&pdev->dev, "phy_ref_clk not found\n");
@@ -1473,6 +1475,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
 {
 	struct mii_bus *bus;
 	int ret;
+	u16 data;
 
 	bus = devm_mdiobus_alloc(priv->dev);
 	if (!bus)
@@ -1486,6 +1489,10 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
 	bus->parent = priv->dev;
 	priv->mii_bus = bus;
 
+	/* set phy power down */
+	data = netsec_phy_read(bus, phy_addr, 0) | 0x800;
+	netsec_phy_write(bus, phy_addr, 0, data);
+
 	if (dev_of_node(priv->dev)) {
 		struct device_node *mdio_node, *parent = dev_of_node(priv->dev);
 
@@ -1623,7 +1630,7 @@ static int netsec_probe(struct platform_device *pdev)
 	}
 
 	if (dev_of_node(&pdev->dev))
-		ret = netsec_of_probe(pdev, priv);
+		ret = netsec_of_probe(pdev, priv, &phy_addr);
 	else
 		ret = netsec_acpi_probe(pdev, priv, &phy_addr);
 	if (ret)
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write()
  2018-10-19  1:08 [PATCH 0/3] Bugfix for the netsec driver masahisa.kojima
  2018-10-19  1:08 ` [PATCH 1/3] net: socionext: Stop PHY before resetting netsec masahisa.kojima
@ 2018-10-19  1:08 ` masahisa.kojima
  2018-10-19  3:01   ` Florian Fainelli
  2018-10-19  1:08 ` [PATCH 3/3] net: socionext: Reset tx queue in ndo_stop masahisa.kojima
  2018-10-19  2:56 ` [PATCH 0/3] Bugfix for the netsec driver Florian Fainelli
  3 siblings, 1 reply; 9+ messages in thread
From: masahisa.kojima @ 2018-10-19  1:08 UTC (permalink / raw)
  To: netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo, Masahisa Kojima

From: Masahisa Kojima <masahisa.kojima@linaro.org>

There is a compatibility issue between RTL8211E implemented
in Developerbox and netsec network controller IP(F_GMAC4).

RTL8211E expects MDC clock must be kept toggling for several
clock cycle with MDIO high before entering the IDLE state.
To meet this requirement, netsec driver needs to issue dummy
read(e.g. read PHYID1(offset 0x2) register) right after write.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
---
 drivers/net/ethernet/socionext/netsec.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 273cc5fc07e0..e7faaf8be99e 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -431,9 +431,12 @@ static int netsec_mac_update_to_phy_state(struct netsec_priv *priv)
 	return 0;
 }
 
+static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr);
+
 static int netsec_phy_write(struct mii_bus *bus,
 			    int phy_addr, int reg, u16 val)
 {
+	int status;
 	struct netsec_priv *priv = bus->priv;
 
 	if (netsec_mac_write(priv, GMAC_REG_GDR, val))
@@ -446,8 +449,19 @@ static int netsec_phy_write(struct mii_bus *bus,
 			      GMAC_REG_SHIFT_CR_GAR)))
 		return -ETIMEDOUT;
 
-	return netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
-					  NETSEC_GMAC_GAR_REG_GB);
+	status = netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
+					    NETSEC_GMAC_GAR_REG_GB);
+
+	/* Developerbox implements RTL8211E PHY and there is
+	 * a compatibility problem with F_GMAC4.
+	 * RTL8211E expects MDC clock must be kept toggling for several
+	 * clock cycle with MDIO high before entering the IDLE state.
+	 * To meet this requirement, netsec driver needs to issue dummy
+	 * read(e.g. read PHYID1(offset 0x2) register) right after write.
+	 */
+	netsec_phy_read(bus, phy_addr, 2);
+
+	return status;
 }
 
 static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr)
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] net: socionext: Reset tx queue in ndo_stop
  2018-10-19  1:08 [PATCH 0/3] Bugfix for the netsec driver masahisa.kojima
  2018-10-19  1:08 ` [PATCH 1/3] net: socionext: Stop PHY before resetting netsec masahisa.kojima
  2018-10-19  1:08 ` [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write() masahisa.kojima
@ 2018-10-19  1:08 ` masahisa.kojima
  2018-10-19  2:56 ` [PATCH 0/3] Bugfix for the netsec driver Florian Fainelli
  3 siblings, 0 replies; 9+ messages in thread
From: masahisa.kojima @ 2018-10-19  1:08 UTC (permalink / raw)
  To: netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo, Masahisa Kojima

From: Masahisa Kojima <masahisa.kojima@linaro.org>

Without resetting tx queue in ndo_stop, packets and bytes count
are not reset when the interface is down.
Eventually, tx queue is exhausted and packets will not be
sent out.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
---
 drivers/net/ethernet/socionext/netsec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index e7faaf8be99e..4b32da76d577 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -954,6 +954,9 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id)
 	dring->head = 0;
 	dring->tail = 0;
 	dring->pkt_cnt = 0;
+
+	if (id == NETSEC_RING_TX)
+		netdev_reset_queue(priv->ndev);
 }
 
 static void netsec_free_dring(struct netsec_priv *priv, int id)
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/3] Bugfix for the netsec driver
  2018-10-19  1:08 [PATCH 0/3] Bugfix for the netsec driver masahisa.kojima
                   ` (2 preceding siblings ...)
  2018-10-19  1:08 ` [PATCH 3/3] net: socionext: Reset tx queue in ndo_stop masahisa.kojima
@ 2018-10-19  2:56 ` Florian Fainelli
  3 siblings, 0 replies; 9+ messages in thread
From: Florian Fainelli @ 2018-10-19  2:56 UTC (permalink / raw)
  To: masahisa.kojima, netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo



On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:
> From: Masahisa Kojima <masahisa.kojima@linaro.org>
> 
> This patch series include bugfix for the netsec ethernet
> controller driver, fix the problem in interface down/up.

Since all of these are bugfixes you would want to provide a Fixes: tag
for the offending commits, that way you can get automated backporting to
stable trees, also patches should be targeted at the "net" tree, which
is indicated with a subject start with [PATCH net], more comments in the
patches.

> 
> Masahisa Kojima (3):
>   net: socionext: stop PHY before resetting netsec
>   net: socionext: Add dummy PHY register read in phy_write()
>   net: socionext: reset tx queue in ndo_stop
> 
>  drivers/net/ethernet/socionext/netsec.c | 36 +++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] net: socionext: Stop PHY before resetting netsec
  2018-10-19  1:08 ` [PATCH 1/3] net: socionext: Stop PHY before resetting netsec masahisa.kojima
@ 2018-10-19  2:59   ` Florian Fainelli
  2018-10-19  6:24     ` Masahisa Kojima
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2018-10-19  2:59 UTC (permalink / raw)
  To: masahisa.kojima, netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo



On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:
> From: Masahisa Kojima <masahisa.kojima@linaro.org>
> 
> After resetting netsec IP, driver have to wait until
> netsec mode turns to NRM mode.
> But sometimes mode transition to NRM will not complete
> if the PHY is in normal operation state.
> To avoid this situation, stop PHY before resetting netsec.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
> ---
>  drivers/net/ethernet/socionext/netsec.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> index 4289ccb26e4e..273cc5fc07e0 100644
> --- a/drivers/net/ethernet/socionext/netsec.c
> +++ b/drivers/net/ethernet/socionext/netsec.c
> @@ -1343,11 +1343,11 @@ static int netsec_netdev_stop(struct net_device *ndev)
>  	netsec_uninit_pkt_dring(priv, NETSEC_RING_TX);
>  	netsec_uninit_pkt_dring(priv, NETSEC_RING_RX);
>  
> -	ret = netsec_reset_hardware(priv, false);
> -
>  	phy_stop(ndev->phydev);
>  	phy_disconnect(ndev->phydev);
>  
> +	ret = netsec_reset_hardware(priv, false);
> +
>  	pm_runtime_put_sync(priv->dev);
>  
>  	return ret;
> @@ -1415,7 +1415,7 @@ static const struct net_device_ops netsec_netdev_ops = {
>  };
>  
>  static int netsec_of_probe(struct platform_device *pdev,
> -			   struct netsec_priv *priv)
> +			   struct netsec_priv *priv, u32 *phy_addr)
>  {
>  	priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
>  	if (!priv->phy_np) {
> @@ -1423,6 +1423,8 @@ static int netsec_of_probe(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> +	*phy_addr = of_mdio_parse_addr(&pdev->dev, priv->phy_np);
> +
>  	priv->clk = devm_clk_get(&pdev->dev, NULL); /* get by 'phy_ref_clk' */
>  	if (IS_ERR(priv->clk)) {
>  		dev_err(&pdev->dev, "phy_ref_clk not found\n");
> @@ -1473,6 +1475,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
>  {
>  	struct mii_bus *bus;
>  	int ret;
> +	u16 data;
>  
>  	bus = devm_mdiobus_alloc(priv->dev);
>  	if (!bus)
> @@ -1486,6 +1489,10 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
>  	bus->parent = priv->dev;
>  	priv->mii_bus = bus;
>  
> +	/* set phy power down */
> +	data = netsec_phy_read(bus, phy_addr, 0) | 0x800;
> +	netsec_phy_write(bus, phy_addr, 0, data);

This is not explained in your commit message, and this is using open
coded values instead of the symbolic names from include/uapi/linux/mii.h.

Note that depending on the type of PHY you are interfaced with if the
PHY is in power down, the only register it is allowed to accept writes
for is MII_BMCR (per 802.3 clause 22 spec), any other read or write to a
different register can be discarded by its MDIO snooping logic. Since
probing the MDIO bus involves reading MII_PHYSID1/ID2, what is this
trying to achieve?

> +
>  	if (dev_of_node(priv->dev)) {
>  		struct device_node *mdio_node, *parent = dev_of_node(priv->dev);
>  
> @@ -1623,7 +1630,7 @@ static int netsec_probe(struct platform_device *pdev)
>  	}
>  
>  	if (dev_of_node(&pdev->dev))
> -		ret = netsec_of_probe(pdev, priv);
> +		ret = netsec_of_probe(pdev, priv, &phy_addr);
>  	else
>  		ret = netsec_acpi_probe(pdev, priv, &phy_addr);
>  	if (ret)
> 

-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write()
  2018-10-19  1:08 ` [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write() masahisa.kojima
@ 2018-10-19  3:01   ` Florian Fainelli
  2018-10-19  6:44     ` Masahisa Kojima
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2018-10-19  3:01 UTC (permalink / raw)
  To: masahisa.kojima, netdev
  Cc: ilias.apalodimas, jaswinder.singh, ard.biesheuvel,
	osaki.yoshitoyo



On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:
> From: Masahisa Kojima <masahisa.kojima@linaro.org>
> 
> There is a compatibility issue between RTL8211E implemented
> in Developerbox and netsec network controller IP(F_GMAC4).
> 
> RTL8211E expects MDC clock must be kept toggling for several
> clock cycle with MDIO high before entering the IDLE state.
> To meet this requirement, netsec driver needs to issue dummy
> read(e.g. read PHYID1(offset 0x2) register) right after write.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
> ---
>  drivers/net/ethernet/socionext/netsec.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> index 273cc5fc07e0..e7faaf8be99e 100644
> --- a/drivers/net/ethernet/socionext/netsec.c
> +++ b/drivers/net/ethernet/socionext/netsec.c
> @@ -431,9 +431,12 @@ static int netsec_mac_update_to_phy_state(struct netsec_priv *priv)
>  	return 0;
>  }
>  
> +static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr);
> +
>  static int netsec_phy_write(struct mii_bus *bus,
>  			    int phy_addr, int reg, u16 val)
>  {
> +	int status;
>  	struct netsec_priv *priv = bus->priv;
>  
>  	if (netsec_mac_write(priv, GMAC_REG_GDR, val))
> @@ -446,8 +449,19 @@ static int netsec_phy_write(struct mii_bus *bus,
>  			      GMAC_REG_SHIFT_CR_GAR)))
>  		return -ETIMEDOUT;
>  
> -	return netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
> -					  NETSEC_GMAC_GAR_REG_GB);
> +	status = netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
> +					    NETSEC_GMAC_GAR_REG_GB);
> +
> +	/* Developerbox implements RTL8211E PHY and there is
> +	 * a compatibility problem with F_GMAC4.
> +	 * RTL8211E expects MDC clock must be kept toggling for several
> +	 * clock cycle with MDIO high before entering the IDLE state.
> +	 * To meet this requirement, netsec driver needs to issue dummy
> +	 * read(e.g. read PHYID1(offset 0x2) register) right after write.
> +	 */
> +	netsec_phy_read(bus, phy_addr, 2);

MII_PHYSID1 instead of 0x2 would be preferable. It is not clear to me
from your commit message if this is a problem specific to your MDIO
controller implementation and the RTL8211E PHY or if this is a general
problem of the PHY irrespective of the MDIO controller it is interface
with. If the latter, then we should seek a solution at a different layer
such that other systems don't have that same problem.

Thank you!
-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] net: socionext: Stop PHY before resetting netsec
  2018-10-19  2:59   ` Florian Fainelli
@ 2018-10-19  6:24     ` Masahisa Kojima
  0 siblings, 0 replies; 9+ messages in thread
From: Masahisa Kojima @ 2018-10-19  6:24 UTC (permalink / raw)
  To: f.fainelli
  Cc: netdev, Ilias Apalodimas, Jassi Brar, Ard Biesheuvel,
	Yoshitoyo Osaki

Hi,

Thank you very much for your comment.
I have not realized that reading MII_PHYSID1/ID2 are ignored
if the PHY is in power down, this patch has a side effect
in ACPI case.

My intention is that netsec_reset_hardware() should be called
in PHY power down state, but current place to add PHY power down
is not proper.
I will consider the right place to do.

On Fri, 19 Oct 2018 at 11:59, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:
> > From: Masahisa Kojima <masahisa.kojima@linaro.org>
> >
> > After resetting netsec IP, driver have to wait until
> > netsec mode turns to NRM mode.
> > But sometimes mode transition to NRM will not complete
> > if the PHY is in normal operation state.
> > To avoid this situation, stop PHY before resetting netsec.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
> > ---
> >  drivers/net/ethernet/socionext/netsec.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> > index 4289ccb26e4e..273cc5fc07e0 100644
> > --- a/drivers/net/ethernet/socionext/netsec.c
> > +++ b/drivers/net/ethernet/socionext/netsec.c
> > @@ -1343,11 +1343,11 @@ static int netsec_netdev_stop(struct net_device *ndev)
> >       netsec_uninit_pkt_dring(priv, NETSEC_RING_TX);
> >       netsec_uninit_pkt_dring(priv, NETSEC_RING_RX);
> >
> > -     ret = netsec_reset_hardware(priv, false);
> > -
> >       phy_stop(ndev->phydev);
> >       phy_disconnect(ndev->phydev);
> >
> > +     ret = netsec_reset_hardware(priv, false);
> > +
> >       pm_runtime_put_sync(priv->dev);
> >
> >       return ret;
> > @@ -1415,7 +1415,7 @@ static const struct net_device_ops netsec_netdev_ops = {
> >  };
> >
> >  static int netsec_of_probe(struct platform_device *pdev,
> > -                        struct netsec_priv *priv)
> > +                        struct netsec_priv *priv, u32 *phy_addr)
> >  {
> >       priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> >       if (!priv->phy_np) {
> > @@ -1423,6 +1423,8 @@ static int netsec_of_probe(struct platform_device *pdev,
> >               return -EINVAL;
> >       }
> >
> > +     *phy_addr = of_mdio_parse_addr(&pdev->dev, priv->phy_np);
> > +
> >       priv->clk = devm_clk_get(&pdev->dev, NULL); /* get by 'phy_ref_clk' */
> >       if (IS_ERR(priv->clk)) {
> >               dev_err(&pdev->dev, "phy_ref_clk not found\n");
> > @@ -1473,6 +1475,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
> >  {
> >       struct mii_bus *bus;
> >       int ret;
> > +     u16 data;
> >
> >       bus = devm_mdiobus_alloc(priv->dev);
> >       if (!bus)
> > @@ -1486,6 +1489,10 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
> >       bus->parent = priv->dev;
> >       priv->mii_bus = bus;
> >
> > +     /* set phy power down */
> > +     data = netsec_phy_read(bus, phy_addr, 0) | 0x800;
> > +     netsec_phy_write(bus, phy_addr, 0, data);
>
> This is not explained in your commit message, and this is using open
> coded values instead of the symbolic names from include/uapi/linux/mii.h.
>
> Note that depending on the type of PHY you are interfaced with if the
> PHY is in power down, the only register it is allowed to accept writes
> for is MII_BMCR (per 802.3 clause 22 spec), any other read or write to a
> different register can be discarded by its MDIO snooping logic. Since
> probing the MDIO bus involves reading MII_PHYSID1/ID2, what is this
> trying to achieve?
>
> > +
> >       if (dev_of_node(priv->dev)) {
> >               struct device_node *mdio_node, *parent = dev_of_node(priv->dev);
> >
> > @@ -1623,7 +1630,7 @@ static int netsec_probe(struct platform_device *pdev)
> >       }
> >
> >       if (dev_of_node(&pdev->dev))
> > -             ret = netsec_of_probe(pdev, priv);
> > +             ret = netsec_of_probe(pdev, priv, &phy_addr);
> >       else
> >               ret = netsec_acpi_probe(pdev, priv, &phy_addr);
> >       if (ret)
> >
>
> --
> Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write()
  2018-10-19  3:01   ` Florian Fainelli
@ 2018-10-19  6:44     ` Masahisa Kojima
  0 siblings, 0 replies; 9+ messages in thread
From: Masahisa Kojima @ 2018-10-19  6:44 UTC (permalink / raw)
  To: f.fainelli
  Cc: netdev, Ilias Apalodimas, Jassi Brar, Ard Biesheuvel,
	Yoshitoyo Osaki

On Fri, 19 Oct 2018 at 12:01, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 10/18/2018 6:08 PM, masahisa.kojima@linaro.org wrote:
> > From: Masahisa Kojima <masahisa.kojima@linaro.org>
> >
> > There is a compatibility issue between RTL8211E implemented
> > in Developerbox and netsec network controller IP(F_GMAC4).
> >
> > RTL8211E expects MDC clock must be kept toggling for several
> > clock cycle with MDIO high before entering the IDLE state.
> > To meet this requirement, netsec driver needs to issue dummy
> > read(e.g. read PHYID1(offset 0x2) register) right after write.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com>
> > ---
> >  drivers/net/ethernet/socionext/netsec.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> > index 273cc5fc07e0..e7faaf8be99e 100644
> > --- a/drivers/net/ethernet/socionext/netsec.c
> > +++ b/drivers/net/ethernet/socionext/netsec.c
> > @@ -431,9 +431,12 @@ static int netsec_mac_update_to_phy_state(struct netsec_priv *priv)
> >       return 0;
> >  }
> >
> > +static int netsec_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr);
> > +
> >  static int netsec_phy_write(struct mii_bus *bus,
> >                           int phy_addr, int reg, u16 val)
> >  {
> > +     int status;
> >       struct netsec_priv *priv = bus->priv;
> >
> >       if (netsec_mac_write(priv, GMAC_REG_GDR, val))
> > @@ -446,8 +449,19 @@ static int netsec_phy_write(struct mii_bus *bus,
> >                             GMAC_REG_SHIFT_CR_GAR)))
> >               return -ETIMEDOUT;
> >
> > -     return netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
> > -                                       NETSEC_GMAC_GAR_REG_GB);
> > +     status = netsec_mac_wait_while_busy(priv, GMAC_REG_GAR,
> > +                                         NETSEC_GMAC_GAR_REG_GB);
> > +
> > +     /* Developerbox implements RTL8211E PHY and there is
> > +      * a compatibility problem with F_GMAC4.
> > +      * RTL8211E expects MDC clock must be kept toggling for several
> > +      * clock cycle with MDIO high before entering the IDLE state.
> > +      * To meet this requirement, netsec driver needs to issue dummy
> > +      * read(e.g. read PHYID1(offset 0x2) register) right after write.
> > +      */
> > +     netsec_phy_read(bus, phy_addr, 2);
>
> MII_PHYSID1 instead of 0x2 would be preferable. It is not clear to me
> from your commit message if this is a problem specific to your MDIO
> controller implementation and the RTL8211E PHY or if this is a general
> problem of the PHY irrespective of the MDIO controller it is interface
> with. If the latter, then we should seek a solution at a different layer
> such that other systems don't have that same problem.
>
> Thank you!
> --
> Florian

I will replace direct value with MACROs, also for other instances.

Our MDIO controller stops MDC clock right after the write access,
but RTL8211E PHY expects to keep several clock cycle.
So it is depending on our MDIO controller implementation.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-19 14:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-19  1:08 [PATCH 0/3] Bugfix for the netsec driver masahisa.kojima
2018-10-19  1:08 ` [PATCH 1/3] net: socionext: Stop PHY before resetting netsec masahisa.kojima
2018-10-19  2:59   ` Florian Fainelli
2018-10-19  6:24     ` Masahisa Kojima
2018-10-19  1:08 ` [PATCH 2/3] net: socionext: Add dummy PHY register read in phy_write() masahisa.kojima
2018-10-19  3:01   ` Florian Fainelli
2018-10-19  6:44     ` Masahisa Kojima
2018-10-19  1:08 ` [PATCH 3/3] net: socionext: Reset tx queue in ndo_stop masahisa.kojima
2018-10-19  2:56 ` [PATCH 0/3] Bugfix for the netsec driver Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).