Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Leo Li <leoyang.li@nxp.com>
To:     Russell King <linux@armlinux.org.uk>
CC:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        David Bauer <mail@david-bauer.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Viorel Suman <viorel.suman@nxp.com>,
        Wei Fang <wei.fang@nxp.com>
Subject: RE: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
Thread-Topic: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functions
Thread-Index: AQHZwZ33oD8fGtEVxE+PIFQQ7nRsG6/W0x4AgACIw/A=
Date:   Wed, 2 Aug 2023 18:43:44 +0000
Message-ID: <AM0PR04MB628950F03E423244198EFEF98F0BA@AM0PR04MB6289.eurprd04.prod.outlook.com>
References: <20230728215320.31801-1-leoyang.li@nxp.com>
 <20230728215320.31801-2-leoyang.li@nxp.com>
 <ZMouSluMSC+bIi9x@shell.armlinux.org.uk>
In-Reply-To: <ZMouSluMSC+bIi9x@shell.armlinux.org.uk>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?qrpfrJnG3nLYhjPMtNISGKq1j3lMiAr75Pktu4ElsVtbTbYZyKVBZA0pBVWz?=
 =?us-ascii?Q?XLSvii+dXeBaeG1lNk7hNLQONxnPqCJj4MCAhrFW9HJFELSzo5vt2tSuvyOm?=
 =?us-ascii?Q?lpvHyJKv45iKMaZD8LIZjo55dw3Ya3cBMxNWscNRULOqNFTZq7/tyjJ+okjb?=
 =?us-ascii?Q?MVbJ2xMZEjiASvInTCJFSUgILU3sQwb/ZUmpUsfOGFsPZ0SiY3rV0L3EZiZW?=
 =?us-ascii?Q?SIFBaFwPyngrcxgGeJFQehEpvYtzL00obgARMZ3a6mJkmrJMMRsiTC8FUtud?=
 =?us-ascii?Q?ndD3n65FnedTUkwl+F2fB5Yzyz8I63soFTw7vYXc+rfz8udDePidpLUdPiRw?=
 =?us-ascii?Q?bUTz6+54MZl/YCKXFLrHwCiM82O+OeQcL3bL7owOxpxHHRNSc4mB7ORuz59f?=
 =?us-ascii?Q?FnB1D8DObWk0//fa5xFy3eyO+r+9T2z9fco8+xuk0y63rNCozgfIijTryiHN?=
 =?us-ascii?Q?ZTxeBqRxBxnkFx+GKU/ckwXe8mmvv6YheJl3foa49nNnLg8e9k/90NsFy/BC?=
 =?us-ascii?Q?6NvU+J0x745ur4FoL5OOtowmezpFFxrWC7XqaZ4AnmLYxsKoy6oLtgVfcZ5I?=
 =?us-ascii?Q?8vRiXiQaJH/sfvU5ePDBOyKzjP6NQyv7Z/mNajlcavx+oOT9vfQTTlTNej6q?=
 =?us-ascii?Q?dAc3bCEmo+X/zDxAZbQgubp6+bujFDWRthPv1paKsT32Pp9KiDVMD3L6PWEu?=
 =?us-ascii?Q?VOSIk4r/+yzJLbHaU2/vQZ54f/FJnNM2TuG2mbpGskdCJrEB0EViMCKfp8nA?=
 =?us-ascii?Q?fqgwFx4Ldf0usjEQ3F0Mrs1tYm4ZbvhTzvnmq0VFQGs6XDo7NQgqkVdApqnY?=
 =?us-ascii?Q?19LVtf5Qgml78raVzu/tJyKV4DxZR2Bag0/0VfU/GFMMIkymCoOCaIG5wO2T?=
 =?us-ascii?Q?Jtz7jtSXczbGwapkhCJrDA8y5zkYVVTXbM7RFbRplBD0N0UrWYNjMyG2UJhv?=
 =?us-ascii?Q?qkOXIObVi3Dm38A2Adr8tvM+v5IV9Z2GX4Xw6N/xFzP0VCeJ2EdCRrHhNcPt?=
 =?us-ascii?Q?Z6xVxkabMVTLmitfuSSS4iBTHLt/i8yRyHRBlOzsfmD1iiUFOC7QDPHyPOw1?=
 =?us-ascii?Q?T7jngAzilc9vcJS1IJK/Qu53pXDucGoaY3HyITXQoQqmnZIsCIM3L69LJwUh?=
 =?us-ascii?Q?zZQRNRbd+HebJGrU+unqVS0VsCJz72c+WJXi5BwQzF1NIbuXelr99x3ZPbg8?=
 =?us-ascii?Q?AhUdKzBeGX7Ns01djd6yFb0Vszh5ZQd/76ESQenC4c+t3pS63puPSHzy7ejq?=
 =?us-ascii?Q?V7cukksuu05qYLtHF2ngDjAFNuGPhb/XBY9mrIs01BJpaLodI2SMU6DcWTHv?=
 =?us-ascii?Q?dlh03FUekfuvi7pcYJ8OdpTdUiylencDDEOm2u52ScmdYSClOkaFx3x0HyDV?=
 =?us-ascii?Q?J4Pd+GfE0qk1RYfXkJk1XEsJR37a2hRaF9SU7UXWI2Q1XbnkSmB2uGC5pY2q?=
 =?us-ascii?Q?VFV8RQw5wSnAd8aRNst3AqNRwQiFPjitRKa8HLxevbKjY8l1iZfJJODByShS?=
 =?us-ascii?Q?okaphbV7oZ+00cI7xD5BAs5ayxn3BOQwyEbqNWyPOjdnAmFfhAS/7e730Arr?=
 =?us-ascii?Q?uqCDfrDVCpl0X86f/dU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6289.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: df9def24-e4b4-4edc-3516-08db93886695
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Aug 2023 18:43:44.8816
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: njW8Oz6N1So+3tr3HCYQL3haPbz0yIe4n+RoW3HbM2BO1o5L5DBgq7JNVZa/Fp5V0k0bb0rXkEBeXMBPyFA/OQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB7561
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, August 2, 2023 5:22 AM
> To: Leo Li <leoyang.li@nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; David Bauer <mail@david-bauer.net>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Viorel Suman
> <viorel.suman@nxp.com>; Wei Fang <wei.fang@nxp.com>
> Subject: Re: [PATCH v3 1/2] net: phy: at803x: fix the wol setting functio=
ns
>=20
> On Fri, Jul 28, 2023 at 04:53:19PM -0500, Li Yang wrote:
> > In commit 7beecaf7d507 ("net: phy: at803x: improve the WOL feature"),
> > it seems not correct to use a wol_en bit in a 1588 Control Register
> > which is only available on AR8031/AR8033(share the same phy_id) to
> > determine if WoL is enabled.  Change it back to use
> > AT803X_INTR_ENABLE_WOL for determining the WoL status which is
> > applicable on all chips supporting wol. Also update the
> > at803x_set_wol() function to only update the 1588 register on chips
> > having it.  After this change, disabling wol at probe from commit
> > d7cd5e06c9dd ("net: phy: at803x: disable WOL at probe") is no longer
> needed.  So that part is removed.
>=20
> Okay, having been through the AR8031, AR8033, and AR8035 datasheets that
> I have, this is what I've gathered:
>=20
> AR8031 and AR8033 are identical as far as WoL is concerned:
> 	In terms of hardware, these have a WOL_INT pin that is separate
> 	from the normal interrupt.
>=20
> 	MMD3 0x8012 (1588 register) bit 5 controls whether the WoL
> 	function is enabled or disabled. Defaults to enabled.
>=20
> 	BMCR in copper/fiber can be used to save more power.
>=20
> 	AR8035 details below also apply.
>=20
> AR8035:
> 	No WOL_INT pin.
>=20
> 	No MMD3 0x8012 register.
>=20
> 	WoL interrupt enable in C22 register 0x12 bit 0
> 	WoL interrupt status in C22 register 0x13 bit 0
> 	WoL MAC address programmed in MMD3 registers 0x804a (bits 47:32)
> 	0x804b (bits 31:16) and 0x804c (bits 15:0)
>=20
> So, what this means is that AR8035, the only possibility for WoL is via t=
he INT
> pin and the C22 interrupt enable/status registers.
>=20
> For AR8031 and AR8033, it depends how the hardware is wired.
>=20
> If WOL_INT is used to wake the system, then MMD3 0x8012 has to be used
> to enable or disable that functionality. From my reading of the datasheet=
s,
> WOL_INT is unaffected by the C22 interrupt enable register settings.
>=20
> If INT is used to wake the system, then it behaves the same as AR8035.
> However, the datasheet doesn't make it clear whether MMD3 0x8012 bit 5
> also has an effect - although I would lean more towards it having an effe=
ct.
>=20
> So, given that:
>=20
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index
> > c1f307d90518..1d61f7190367 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -459,21 +459,27 @@ static int at803x_set_wol(struct phy_device
> *phydev,
> >  			phy_write_mmd(phydev, MDIO_MMD_PCS,
> offsets[i],
> >  				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
> >
> > -		/* Enable WOL function */
> > -		ret =3D phy_modify_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL,
> > -				0, AT803X_WOL_EN);
> > -		if (ret)
> > -			return ret;
> > +		/* Enable WOL function for 1588 */
> > +		if (phydev->drv->phy_id =3D=3D ATH8031_PHY_ID) {
> > +			ret =3D phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > +					     AT803X_PHY_MMD3_WOL_CTRL,
> > +					     0, AT803X_WOL_EN);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  		/* Enable WOL interrupt */
> >  		ret =3D phy_modify(phydev, AT803X_INTR_ENABLE, 0,
> AT803X_INTR_ENABLE_WOL);
> >  		if (ret)
> >  			return ret;
> >  	} else {
> > -		/* Disable WoL function */
> > -		ret =3D phy_modify_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL,
> > -				AT803X_WOL_EN, 0);
> > -		if (ret)
> > -			return ret;
> > +		/* Disable WoL function for 1588 */
> > +		if (phydev->drv->phy_id =3D=3D ATH8031_PHY_ID) {
> > +			ret =3D phy_modify_mmd(phydev, MDIO_MMD_PCS,
> > +					     AT803X_PHY_MMD3_WOL_CTRL,
> > +					     AT803X_WOL_EN, 0);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  		/* Disable WOL interrupt */
> >  		ret =3D phy_modify(phydev, AT803X_INTR_ENABLE,
> AT803X_INTR_ENABLE_WOL, 0);
> >  		if (ret)
> > @@ -508,11 +514,11 @@ static void at803x_get_wol(struct phy_device
> *phydev,
> >  	wol->supported =3D WAKE_MAGIC;
> >  	wol->wolopts =3D 0;
> >
> > -	value =3D phy_read_mmd(phydev, MDIO_MMD_PCS,
> AT803X_PHY_MMD3_WOL_CTRL);
> > +	value =3D phy_read(phydev, AT803X_INTR_ENABLE);
> >  	if (value < 0)
> >  		return;
> >
> > -	if (value & AT803X_WOL_EN)
> > +	if (value & AT803X_INTR_ENABLE_WOL)
> >  		wol->wolopts |=3D WAKE_MAGIC;
> >  }
> >
>=20
> The above all looks correct to me.
>=20
> > @@ -858,9 +864,6 @@ static int at803x_probe(struct phy_device *phydev)
> >  	if (phydev->drv->phy_id =3D=3D ATH8031_PHY_ID) {
> >  		int ccr =3D phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> >  		int mode_cfg;
> > -		struct ethtool_wolinfo wol =3D {
> > -			.wolopts =3D 0,
> > -		};
> >
> >  		if (ccr < 0)
> >  			return ccr;
> > @@ -876,13 +879,6 @@ static int at803x_probe(struct phy_device *phydev)
> >  			priv->is_fiber =3D true;
> >  			break;
> >  		}
> > -
> > -		/* Disable WOL by default */
> > -		ret =3D at803x_set_wol(phydev, &wol);
> > -		if (ret < 0) {
> > -			phydev_err(phydev, "failed to disable WOL on
> probe: %d\n", ret);
> > -			return ret;
> > -		}
> >  	}
> >
> >  	return 0;
>=20
> This doesn't look correct to me, because in the case of AR8031 or
> AR8033 using WOL_INT, because MMD3 0x8012 bit 5 defaults on reset to
> being set, if we don't want WoL enabled after the PHY has been probed, we
> need to clear it.

The intent was to remove the code added by d7cd5e06c9d ("net: phy: at803x: =
disable WOL at probe") which claims to be needed after 7beecaf7d507b ("net:=
 phy: at803x: improve the WOL feature").  Since I have changed the behavior=
 back to before 7beecaf7d507b, it is natural that this part of d7cd5e06c9d =
is no longer needed.

But if we want MMD3 0x8012 bit 5 to be aligned with the WOL_INT on probe, w=
e probably just need to clear the bit for AR8031/8033 instead of calling th=
e set_wol for all chips.

Regards,
Leo