| Message ID | 20181019010843.3605-2-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Bugfix for the netsec driver | expand |
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
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
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)