Message-ID: <CA+h21hruqt6nGG5ksDSwrGH_w5GtGF4fjAMCWJne7QJrjusERQ@mail.gmail.com>
Date:   Sat, 24 Aug 2019 18:21:33 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arseny Solokha <asolokha@...kras.ru>
Cc:     Claudiu Manoil <claudiu.manoil@....com>,
        Russell King <linux@...linux.org.uk>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [RFC PATCH 1/2] gianfar: convert to phylink

Hi Arseny.

On Tue, 30 Jul 2019 at 17:40, Arseny Solokha <asolokha@...kras.ru> wrote:
>
> > Hi Arseny,
> >
> > Nice project!
>
> Vladimir, Russell, thanks for your review. I'm on vacation now, so won't fully
> address your comments in a few weeks: while I can build the code, I won't have
> access to hardware to test.
>
> So it seems this patch will turn into a series where we'll have some cleanup
> patches preceding the actual conversion (and the latter will also contain a
> documentation change in Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
> which I've overlooked in the first submission). I'll try to post trivial
> cleanups independently while still on vacation.
>

Yes, ideally the cleanup would be separate from the conversion.

>
> >> @@ -891,11 +912,21 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> >>
> >>         err = of_property_read_string(np, "phy-connection-type", &ctype);
> >>
> >> -       /* We only care about rgmii-id.  The rest are autodetected */
> >> -       if (err == 0 && !strcmp(ctype, "rgmii-id"))
> >> -               priv->interface = PHY_INTERFACE_MODE_RGMII_ID;
> >> -       else
> >> +       /* We only care about rgmii-id and sgmii - the former
> >> +        * is indistinguishable from rgmii in hardware, and phylink needs
> >> +        * the latter to be set appropriately for correct phy configuration.
> >> +        * The rest are autodetected
> >> +        */
> >> +       if (err == 0) {
> >> +               if (!strcmp(ctype, "rgmii-id"))
> >> +                       priv->interface = PHY_INTERFACE_MODE_RGMII_ID;
> >> +               else if (!strcmp(ctype, "sgmii"))
> >> +                       priv->interface = PHY_INTERFACE_MODE_SGMII;
> >> +               else
> >> +                       priv->interface = PHY_INTERFACE_MODE_MII;
> >> +       } else {
> >>                 priv->interface = PHY_INTERFACE_MODE_MII;
> >> +       }
> >>
> >
> > No. Don't do this. Just do:
> >
> >     err = of_get_phy_mode(np);
> >     if (err < 0)
> >         goto err_grp_init;
> >
> >     priv->interface = err;
> >
> >>         if (of_find_property(np, "fsl,magic-packet", NULL))
> >>                 priv->device_flags |= FSL_GIANFAR_DEV_HAS_MAGIC_PACKET;
> >> @@ -903,19 +934,21 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
> >>         if (of_get_property(np, "fsl,wake-on-filer", NULL))
> >>                 priv->device_flags |= FSL_GIANFAR_DEV_HAS_WAKE_ON_FILER;
> >>
> >> -       priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
> >> +       priv->device_node = np;
> >> +       priv->speed = SPEED_UNKNOWN;
> >>
> >> -       /* In the case of a fixed PHY, the DT node associated
> >> -        * to the PHY is the Ethernet MAC DT node.
> >> -        */
> >> -       if (!priv->phy_node && of_phy_is_fixed_link(np)) {
> >> -               err = of_phy_register_fixed_link(np);
> >> -               if (err)
> >> -                       goto err_grp_init;
> >> +       priv->phylink_config.dev = &priv->ndev->dev;
> >> +       priv->phylink_config.type = PHYLINK_NETDEV;
> >>
> >> -               priv->phy_node = of_node_get(np);
> >> +       phylink = phylink_create(&priv->phylink_config, of_fwnode_handle(np),
> >> +                                priv->interface, &gfar_phylink_ops);
> >
> > You introduced a bug here.
> > of_phy_connect used to take the PHY interface type (for good or bad)
> > from gfar_get_interface() (which is reconstructing it from the MAC
> > registers).
> > You are now passing the PHY interface type to phylink_create from the
> > "phy-connection-type" DT property.
> > At the very least, you are breaking LS1021A which uses phy-mode
> > instead of phy-connection-type (hence my comment above to use the
> > generic OF helper).
> > Actually I think you just uncovered a latent bug, in that the DT
> > bindings for phy-mode didn't mean much at all to the driver - it would
> > rely on what the bootloader had set up.
> > Actually DT bindings for phy-connection-type were most likely simply
> > bolt on on top of gianfar when they figured they couldn't just
> > auto-detect the various species of required RGMII delays.
> > But gfar_get_interface is a piece of history that was introduced in
> > the same commit as the enum phy_interface_t itself: e8a2b6a42073
> > ("[PATCH] PHY: Add support for configuring the PHY connection
> > interface"). Its time has come.
>
> <…>
>
> >>         }
> >>
> >> +       priv->tbi_phy = NULL;
> >> +       interface = gfar_get_interface(dev);
> >
> > Be consistent and just go for priv->interface. Nobody's changing it anyway.
>
> So if I get you right, I'm supposed to drop gfar_get_interface() and rely on DT
> bindings entirely?
>

Oof, I checked arch/powerpc/boot/dts/fsl and the following boards
using the gianfar driver are guilty of populating phy-handle but not
phy-connection-type:
mpc8540ads
mpc8541cds
mpc8548cds_32b
mpc8548cds_36b
mpc8555cds
mpc8560ads
mpc8568mds
ppa8548

I think a sane logic for populating priv->interface would be:
- Attempt of_get_phy_mode
- If phy-mode or phy-connection-type properties are not found, revert
to gfar_get_interface for the legacy blobs above.

>
> >> @@ -3387,23 +3384,6 @@ static irqreturn_t gfar_interrupt(int irq, void *grp_id)
> >>         return IRQ_HANDLED;
> >>  }
> >>
> >> -/* Called every time the controller might need to be made
> >> - * aware of new link state.  The PHY code conveys this
> >> - * information through variables in the phydev structure, and this
> >> - * function converts those variables into the appropriate
> >> - * register values, and can bring down the device if needed.
> >> - */
> >> -static void adjust_link(struct net_device *dev)
> >> -{
> >> -       struct gfar_private *priv = netdev_priv(dev);
> >> -       struct phy_device *phydev = dev->phydev;
> >> -
> >> -       if (unlikely(phydev->link != priv->oldlink ||
> >> -                    (phydev->link && (phydev->duplex != priv->oldduplex ||
> >> -                                      phydev->speed != priv->oldspeed))))
> >> -               gfar_update_link_state(priv);
> >> -}
> >
> > Getting rid of the cruft from this function deserves its own patch.
>
> How am I supposed to remove it without breaking the PHYLIB-based driver? Or do
> you mean making it call gfar_update_link_state() just before the conversion
> which will then remove adjust_link() altogether?
>

I don't know, if you can't refactor without breaking anything then ok.

>
> >>
> >>         if (unlikely(test_bit(GFAR_RESETTING, &priv->state)))
> >>                 return;
> >>
> >> -       if (phydev->link) {
> >> -               u32 tempval1 = gfar_read(&regs->maccfg1);
> >> -               u32 tempval = gfar_read(&regs->maccfg2);
> >> -               u32 ecntrl = gfar_read(&regs->ecntrl);
> >> -               u32 tx_flow_oldval = (tempval1 & MACCFG1_TX_FLOW);
> >> +       if (unlikely(phylink_autoneg_inband(mode)))
> >> +               return;
> >>
> >> -               if (phydev->duplex != priv->oldduplex) {
> >> -                       if (!(phydev->duplex))
> >> -                               tempval &= ~(MACCFG2_FULL_DUPLEX);
> >> -                       else
> >> -                               tempval |= MACCFG2_FULL_DUPLEX;
> >> +       maccfg1 = gfar_read(&regs->maccfg1);
> >> +       maccfg2 = gfar_read(&regs->maccfg2);
> >> +       ecntrl = gfar_read(&regs->ecntrl);
> >>
> >> -                       priv->oldduplex = phydev->duplex;
> >> -               }
> >> +       new_maccfg2 = maccfg2 & ~(MACCFG2_FULL_DUPLEX | MACCFG2_IF);
> >> +       new_ecntrl = ecntrl & ~ECNTRL_R100;
> >>
> >> -               if (phydev->speed != priv->oldspeed) {
> >> -                       switch (phydev->speed) {
> >> -                       case 1000:
> >> -                               tempval =
> >> -                                   ((tempval & ~(MACCFG2_IF)) | MACCFG2_GMII);
> >> +       if (state->duplex)
> >> +               new_maccfg2 |= MACCFG2_FULL_DUPLEX;
> >>
> >> -                               ecntrl &= ~(ECNTRL_R100);
> >> -                               break;
> >> -                       case 100:
> >> -                       case 10:
> >> -                               tempval =
> >> -                                   ((tempval & ~(MACCFG2_IF)) | MACCFG2_MII);
> >> -
> >> -                               /* Reduced mode distinguishes
> >> -                                * between 10 and 100
> >> -                                */
> >> -                               if (phydev->speed == SPEED_100)
> >> -                                       ecntrl |= ECNTRL_R100;
> >> -                               else
> >> -                                       ecntrl &= ~(ECNTRL_R100);
> >> -                               break;
> >> -                       default:
> >> -                               netif_warn(priv, link, priv->ndev,
> >> -                                          "Ack!  Speed (%d) is not 10/100/1000!\n",
> >> -                                          phydev->speed);
> >
> > Please 1. remove "Ack!" 2. treat SPEED_UNKNOWN here by setting the MAC
> > into a low-power state (e.g. 10 Mbps - the power savings are real).
> > Don't print that Speed -1 is not 10/100/1000, we know that.
>
> In my first conversion attempt I see "Ack!" when changing link speed on when
> shutting it down, so switching to 10 Mbps doesn't seem right for me—hence early
> return in this case. Maybe I'm doing something wrong here.
>

When mac_config calls with SPEED_UNKNOWN, the link is down, and you
can put the MAC in the lowest energy state it can go to (10 Mbps, in
this case). Or so I've been told. Maybe Russell can chime in. Anyway,
you don't need to print anything, there's lots of prints from PHYLINK
already.

>
> >> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> >> index 3433b46b90c1..146b30d07789 100644
> >> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> >> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> >> @@ -35,7 +35,7 @@
> >>  #include <asm/types.h>
> >>  #include <linux/ethtool.h>
> >>  #include <linux/mii.h>
> >> -#include <linux/phy.h>
> >> +#include <linux/phylink.h>
> >>  #include <linux/sort.h>
> >>  #include <linux/if_vlan.h>
> >>  #include <linux/of_platform.h>
> >> @@ -207,12 +207,10 @@ static void gfar_get_regs(struct net_device *dev, struct ethtool_regs *regs,
> >>  static unsigned int gfar_usecs2ticks(struct gfar_private *priv,
> >>                                      unsigned int usecs)
> >>  {
> >> -       struct net_device *ndev = priv->ndev;
> >> -       struct phy_device *phydev = ndev->phydev;
> >
> > Are you sure this still works? You missed a ndev->phydev check from
> > gfar_gcoalesce, where this is called from. Technically you can still
> > check ndev->phydev, it's just that PHYLINK doesn't guarantee you'll
> > have one (e.g. fixed-link interfaces).
>
> It still works for RGMII PHYs, SGMII and 1000Base-X in my testing. I didn't
> check it with fixed-link, though.
>
>
> >> @@ -1519,6 +1472,24 @@ static int gfar_get_ts_info(struct net_device *dev,
> >>         return 0;
> >>  }
> >>
> >> +/* Set link ksettings (phy address, speed) for ethtools */
> >
> > ethtool, not ethtools. Also, I'm not quite sure what you mean by
> > setting the "phy address" with ethtool.
>
> Well, I know where I've copied it from… Thanks for pointing it out.

Regards,
-Vladimir

Powered by blists - more mailing lists