From:   "Ismail, Mohammad Athari" <mohammad.athari.ismail@intel.com>
To:     Russell King <linux@armlinux.org.uk>
CC:     Andrew Lunn <andrew@lunn.ch>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Oleksij Rempel <linux@rempel-privat.de>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: RE: [PATCH net v2] net: phy: marvell: add Marvell specific PHY
 loopback
Thread-Topic: [PATCH net v2] net: phy: marvell: add Marvell specific PHY
 loopback
Thread-Index: AQHYB5efdv1tXs/AiUepicn/jvMmF6xfXQeAgACh7IA=
Date:   Wed, 12 Jan 2022 22:55:27 +0000
Message-ID: <CO1PR11MB477101305A603B6BC7083360D5529@CO1PR11MB4771.namprd11.prod.outlook.com>
References: <20220112093344.27894-1-mohammad.athari.ismail@intel.com>
 <Yd7T1e/R9jGWMK2B@shell.armlinux.org.uk>
In-Reply-To: <Yd7T1e/R9jGWMK2B@shell.armlinux.org.uk>
Accept-Language: en-US
Content-Language: en-US
dlp-version: 11.6.200.16
dlp-product: dlpe-windows
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?zeMyxcJbm+a96R0NCDxj05JSxeXdRTUhQaH2srC1kWg88BjWRk3pbE7ogpl6?=
 =?us-ascii?Q?G4WCVlUkeZCO/nDDQK/Xz2FX2eaaF+VtfEEuvqlxuKCihG7CRch8GuUhalDR?=
 =?us-ascii?Q?OTmWVmRnTK4I8HqDo1P4WB3dEBt3LV1rqCPMj8nYyfhvBmxdJECQQywfsxCf?=
 =?us-ascii?Q?FKX3JxvxKZvrHBfv3TLvn1/GLeghsmA/oRz3OJagfc5OCUfStm+hbA7ybm19?=
 =?us-ascii?Q?jNo5sbTsxb8TzF2bP8A9DcsfC+1dI7jYDzWfGFC4wWlXC58Pg3hASdXHRui2?=
 =?us-ascii?Q?0McFIXWrGzu5Vky+qJF87WhsiI3sAloVUK7ldiMWP18hJL3kDmptIT4x+UIp?=
 =?us-ascii?Q?2P6vwI3vv86ObS9gN+Jisxzi1j7jgmtCcpA4B0Zryf7sNm8U+mGujoJSKcEa?=
 =?us-ascii?Q?beNM+fJj/fVGKJXg3EXfa0tSThnjPmMY8VyqbM1FZhdMa8OfaAJ+tzwSvKLX?=
 =?us-ascii?Q?EJ8QG4Rrd8GmcxEIjTTZUDqdFXMboiWCDxfxtpbebkqumbxElehVeFBJ4V1q?=
 =?us-ascii?Q?/KjuL6QpAxIUyaCJDcfe6shpgjfT9fwVfci2ecgiHdQSsW1hN6JZ80Dd5VET?=
 =?us-ascii?Q?XIzCzoaJS3IMBXWH4NKFxk7u8oidURqsYnHPBSHXN1G3inYpy2kxAKn7xk2T?=
 =?us-ascii?Q?S3Ymjk2SzcbDYRVbfE0X/TCsP2GpfIj6AeiL2qccQTV9rpMQqBrLsZm/HDXq?=
 =?us-ascii?Q?LOYnrtoQWv2vNuG6BHyf0ZTCWe55Qk74wz9fsWSgwXcb30MxnL041bu9R+sx?=
 =?us-ascii?Q?eoB0tatRjlRrfvk+XGO2q/GT3Uw1Fa70JqMcgMYFKFLzaJDfcu5XCdae7r2p?=
 =?us-ascii?Q?TErCo0lgSwxk1pDbwk3E4dIyWW9KZFw3vQnyaJi4jOHjocgAPj4/S5Xps8wu?=
 =?us-ascii?Q?xPhtJdw+M8R7v/5QC+0ksT92q985il9+8FJy7eu6heZxSsWniDoTlXcKdzMx?=
 =?us-ascii?Q?+LsGKvaIGRomPpKFIsTy8J2kKoXjgxepR03vHHXXWnr0cw7uw3emIEg0dHBs?=
 =?us-ascii?Q?kpbSbrw0n+JBTtCStfsPQiFWHLK7KpvJ63Axysn6uY24cXZ515CoLUxFxq43?=
 =?us-ascii?Q?pxXyXExBb4PHQJPlWXAlIHV83PjEy/glt+0jixJ72YUqruFH/iV5B1iBDh2o?=
 =?us-ascii?Q?1HL/eDmmsxCPAOgrAylUlF3u2Z4CAsZwfXxJ6Jv+9G2aJl852E3mCTuQazYM?=
 =?us-ascii?Q?Y3+3drKQX+44nBOuuW5YzK0N4ZIVtt21cB973uppCQ/EfP2U/b+ujSni4r61?=
 =?us-ascii?Q?7VQbqyZVgS3LvZ/yxMJBvmgrXUti4YqLeKnMx0himMOCzTuiZYV+Ya+mp6/W?=
 =?us-ascii?Q?kzvJo5WOeFNCW422b9SVHQ2fpWeGygpdo6q63yxWzBVOO0SjEAwjBQDcgJuB?=
 =?us-ascii?Q?ZOdrlC/Q7KmcbA5lwcdlbymEtZ1FaSQrgrR6O4VAlamgAlrf+9uMrKscLXdJ?=
 =?us-ascii?Q?tUyQIXCNiO3Ljz6dF5tHE4p6cJ8rP4TyxCziYs/50NlC0fcZNlpCFnKP/Jh/?=
 =?us-ascii?Q?xGjPu5vSJdyaC18+x5KpTK5fr4UoYuVj+We6TqvSvvMigMU2dIRQIaXdpeAO?=
 =?us-ascii?Q?4dKlK9pOzSRponSDo9teN56esC5SOwObnB8pYS8QgoSC2edmNG7bfmM4qYVj?=
 =?us-ascii?Q?chaCaotuxF9oK7Ysdb/yZltfS22/D7TEFYGGkdImjgFhmTcf6NDkap05LKcF?=
 =?us-ascii?Q?rGAHlUUNRVeis9Lsdqr8wRyRdfuPQaIL6v/C9Vyx9a22i+RDzdG7/rDP5Opu?=
 =?us-ascii?Q?IAwrdU2iTQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4771.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ad56442-c6e9-4955-0c2c-08d9d61ea07f
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jan 2022 22:55:27.8585
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: seic0EkxVtwaVBY/fD5Rl7fIOmL52x24rPLPYfRRSlPYLgNkvtCRxcXl8xBT/N5vf0nPbo73HFLucFyN5+EhOkifgDWOd3rRD9AQxZosZ2TKuSluO+B1jaGA7nSMZ3jL
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4092
X-OriginatorOrg: intel.com
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, January 12, 2022 9:13 PM
> To: Ismail, Mohammad Athari <mohammad.athari.ismail@intel.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; David S . Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Oleksij
> Rempel <linux@rempel-privat.de>; Heiner Kallweit
> <hkallweit1@gmail.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH net v2] net: phy: marvell: add Marvell specific PHY
> loopback
>=20
> On Wed, Jan 12, 2022 at 05:33:44PM +0800, Mohammad Athari Bin Ismail
> wrote:
> > +static int marvell_loopback(struct phy_device *phydev, bool enable) {
> > +	if (enable) {
> > +		u16 bmcr_ctl =3D 0, mscr2_ctl =3D 0;
> > +
> > +		if (phydev->speed =3D=3D SPEED_1000)
> > +			bmcr_ctl =3D BMCR_SPEED1000;
> > +		else if (phydev->speed =3D=3D SPEED_100)
> > +			bmcr_ctl =3D BMCR_SPEED100;
> > +
> > +		if (phydev->duplex =3D=3D DUPLEX_FULL)
> > +			bmcr_ctl |=3D BMCR_FULLDPLX;
> > +
> > +		phy_modify(phydev, MII_BMCR, ~0, bmcr_ctl);
>=20
> Is there any point in doing a read-modify-write here if you're just setti=
ng all
> bits in the register? Wouldn't phy_write() be more appropriate? What abou=
t
> error handing?

Yes, you're right. phy_write() is more suitable. And will add error handlin=
g as well.
Will include them in v3 patch.

>=20
> > +
> > +		if (phydev->speed =3D=3D SPEED_1000)
> > +			mscr2_ctl =3D BMCR_SPEED1000;
> > +		else if (phydev->speed =3D=3D SPEED_100)
> > +			mscr2_ctl =3D BMCR_SPEED100;
> > +
> > +		phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
> > +				 MII_88E1510_MSCR_2, BMCR_SPEED1000 |
> > +				 BMCR_SPEED100, mscr2_ctl);
> > +

I believe this also need error handling.

> > +		/* Need soft reset to have speed configuration takes effect
> */
> > +		genphy_soft_reset(phydev);

Ditto.

> > +
> > +		/* FIXME: Based on trial and error test, it seem 1G need to
> have
> > +		 * delay between soft reset and loopback enablement.
> > +		 */
> > +		if (phydev->speed =3D=3D SPEED_1000)
> > +			msleep(1000);
> > +
> > +		return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK,
> > +				  BMCR_LOOPBACK);
> > +	} else {
> > +		phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
>=20
> Error handling?

Will add it in v3 patch.

-Athari-


>=20
> Thanks.
>=20
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!