> Hi Arseny.
>
> On Tue, 30 Jul 2019 at 17:40, Arseny Solokha <asolo...@kb.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.
I've just sent a cleanup series. It won't make the conversion easier to digest,
though.
>> >> @@ -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.
I can, of course, effectively revert 6ce29b0e2a04 ("gianfar: Avoid unnecessary
reg accesses in adjust_link()") and 2a4eebf0c485 ("gianfar: Restore link state
settings after MAC reset") just before the conversion, but I fail to see a point
in that. It would only make subsequent bisection harder.
>> >> if (unlikely(test_bit(GFAR_RESETTING, &priv->state)))
>> >> return;
>> >>
>> >> - if (phydev->link) {
>> >> - u32 tempval1 = gfar_read(®s->maccfg1);
>> >> - u32 tempval = gfar_read(®s->maccfg2);
>> >> - u32 ecntrl = gfar_read(®s->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(®s->maccfg1);
>> >> + maccfg2 = gfar_read(®s->maccfg2);
>> >> + ecntrl = gfar_read(®s->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.
OK. I believe this comment only applies to a PHYLINK-based driver and I should
omit this change from a cleanup series? Because in the PHYLIB-based driver this
code only gets executed when the link is up, and I can't immediately see how it
could be called with phydev->speed set to SPEED_UNKNOWN.
>> >> 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
