On Mon, Oct 08, 2018 at 02:12:59PM +0000, Igor Russkikh wrote:
> Hi Andrew,
>
> >> + if (aqc111_data->dpa) {
> >> + aqc111_set_phy_speed(dev, AUTONEG_DISABLE, SPEED_100);
> >
> > I don't think that works. You should leave AUTONEG on, but only
> > advertise SPEED_100 and trigger auto-neg. If you force it to 100,
> > there is no guarantee the peer will figure out what the new link speed
> > is. I've often seen failed auto-net result in 10/Half. So you will
> > loose the link, making WoL pointless.
>
> Phy does not support 10M, low power mode explicitly uses 100M
> for power safety reasons.
>
> It is meaningless here to add Autoneg to 100M because thats the only
> speedmask bit anyway.
If you have AUTONEG_DISABLE, i would assume you PHY is not even trying
to auto_neg. So the speedmask is irrelevent, it is not sent to the
peer. And since the peer is not receiving any auto-neg information, it
will fail to auto-neg, and most likely default to 10/Half.
To do this right, please take a look at this commit
commit 2b9672ddb6f347467d7b33b86c5dfc4d5c0501a8
Author: Heiner Kallweit <hkallweit1@xxxxxxxxx>
Date: Thu Jul 12 21:32:53 2018 +0200
net: phy: add phy_speed_down and phy_speed_up
Some network drivers include functionality to speed down the PHY when
suspending and just waiting for a WoL packet because this saves energy.
This functionality is quite generic, therefore let's factor it out to
phylib.
>
> >> + aqc111_set_phy_speed(dev, aqc111_data->autoneg,
> >> + aqc111_data->advertised_speed);
> >> +
> >
> > Should that be conditional on aqc111_data->dpa?
>
> Actually no, because set_phy_speed internally checks this flag.
So you should probably remove the check above when forcing the speed
to 100. Make the code symmetrical.
>
> >> + u8 rsvd[283];
> >> +};
> >
> > Do you really need these 283 bytes??
>
> >> struct aqc111_phy_options phy_ops;
> >> + struct aqc111_wol_cfg wol_cfg;
> >
> > Those 283 bytes make this whole structure bigger...
>
> FW interface expects the WOL config request WOL_CFG_SIZE bytes.
> These reserved fields are just not used now by linux driver.
> They configure extra wol features like a sleep proxy.
> Thus, we anyway have to allocate this somewhere.
Well, i think your low level function for actually sending a command
does a dup before sending. You don't actually send this, you send a
copy. Maybe you can pad it out then?
Andrew
