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.faine...@gmail.com> wrote:
>
>
>
> On 10/18/2018 6:08 PM, masahisa.koj...@linaro.org wrote:
> > From: Masahisa Kojima <masahisa.koj...@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.koj...@linaro.org>
> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshit...@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
