From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2FF71377EAF; Tue, 3 Mar 2026 18:02:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772560926; cv=none; b=aECmk4nssZAqWWefeGj7KsYnn1ka6vchbsq7/LoSYrdwNkyWbrngb33yI+K9lYD5j1qT06r1iYGGY7jlxcppOYz3E2tqj+R3VM5sNSrE19W+9f8Vn13flbVMBsG1IsNTXmk+li5bzaQA/DfrBPAuciRoPg7psEVk49Y1GMomk2s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772560926; c=relaxed/simple; bh=L6o1qFUQTP4oddulMbYBYh/EeWe1SJpi+VjvmKahXdA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=FXhMgW3qqf8Xpw0KPvp5bJyHFT8kHKj1Qf+jLbH0+I8J2SLM6ob1D4VlykBGoTE+Q+cbqVotxl09WYXPvurxDxHSwaR2P1XRK5riCjTUm58E6hWTzSLRGdbNtvHTzDPJQvSe/Fqg89JayFL4uTm+/1u52B8xtWXlhWcmcyoVj4E= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=cnLJ5h2H; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cnLJ5h2H" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B1EEBC116C6; Tue, 3 Mar 2026 18:02:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1772560926; bh=L6o1qFUQTP4oddulMbYBYh/EeWe1SJpi+VjvmKahXdA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=cnLJ5h2HOatPK8cZcKiapmiu6SXhGmiB09791lnqMnVrlb35IVoOsBUa+hdU6LLUy 6oeppaIx56oZROPV+N8sndNjxqe7TA4M57Ihg2q1eUsUBaWhh4tj6fZp0KUmNo0EsH 1UOmbYX4WBywphNw9GwdN+USJEWWmE4QVB0sEbUn4lsZNcgoFlWNGvzT82z44xa8aW fDqAgTJVzyYvd+v4ogVtU/g2z72ER9ct/FW+lknWnNWV+YzSwEOIP6R9SJUViYQxLJ GEl5a5zWY44n9dYQ0T7951GVChmbCsLPlmgvWvkz3tF0GdhntDiLLzOMuOnZSw/9xs 2gyu0IW38VqMQ== Date: Tue, 3 Mar 2026 18:01:58 +0000 From: Conor Dooley To: Ryan Wanner Cc: Jakub Kicinski , andrew+netdev@lunn.ch, richardcochran@gmail.com, abin.joseph@amd.com, robh@kernel.org, edumazet@google.com, netdev@vger.kernel.org, theo.lebrun@bootlin.com, pjw@kernel.org, Valentina.FernandezAlanis@microchip.com, krzk+dt@kernel.org, sean.anderson@linux.dev, aou@eecs.berkeley.edu, linux-kernel@vger.kernel.org, alex@ghiti.fr, devicetree@vger.kernel.org, palmer@dabbelt.com, nicolas.ferre@microchip.com, vineeth.karumanchi@amd.com, claudiu.beznea@tuxon.dev, samuel.holland@sifive.com, daire.mcnamara@microchip.com, conor+dt@kernel.org, dave.stevenson@raspberrypi.com, linux-riscv@lists.infradead.org, davem@davemloft.net, conor.dooley@microchip.com, narmstrong@baylibre.com, pabeni@redhat.com Subject: Re: [net-next,v2,2/8] net: macb: rename macb_default_usrio to at91_default_usrio as not all platforms have mii mode control in usrio Message-ID: <20260303-stardust-shaky-c8a757ec149d@spud> References: <20260226-enjoyer-shock-e17f9dc7cbdb@spud> <20260228232600.4187398-1-kuba@kernel.org> <20260228-shopping-april-a8c4d2481cbe@spud> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="NYgeMk65ZDrXJJA6" Content-Disposition: inline In-Reply-To: --NYgeMk65ZDrXJJA6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 03, 2026 at 10:35:05AM -0700, Ryan Wanner wrote: > On 2/28/26 17:06, Conor Dooley wrote: > > Yo, > >=20 > > On Sat, Feb 28, 2026 at 03:26:00PM -0800, Jakub Kicinski wrote: > >> This is an AI-generated review of your patch. The human sending this > >> email has considered the AI review valid, or at least plausible. > >> > >> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-l= ocal.html > >> --- > >> net: macb: rename macb_default_usrio to at91_default_usrio as not all > >> platforms have mii mode control in usrio > >> > >> This patch renames macb_default_usrio to at91_default_usrio to better > >> reflect that the usrio configuration is platform-specific, and > >> introduces a new capability flag (MACB_CAPS_USRIO_HAS_MII) to gate MII > >> mode configuration code for platforms that actually support it. > >> > >>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethern= et/cadence/macb.h > >>> --- a/drivers/net/ethernet/cadence/macb.h > >>> +++ b/drivers/net/ethernet/cadence/macb.h > >>> @@ -779,6 +779,7 @@ #define MACB_CAPS_DMA_64B BIT(21) > >>> #define MACB_CAPS_DMA_PTP BIT(22) > >>> #define MACB_CAPS_RSC BIT(23) > >>> #define MACB_CAPS_NO_LSO BIT(24) > >>> +#define MACB_CAPS_USRIO_HAS_MII BIT(25) > >>> > >>> /* LSO settings */ > >>> #define MACB_LSO_UFO_ENABLE 0x01 > >>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/e= thernet/cadence/macb_main.c > >>> --- a/drivers/net/ethernet/cadence/macb_main.c > >>> +++ b/drivers/net/ethernet/cadence/macb_main.c > >>> @@ -4614,13 +4614,15 @@ > >>> > >>> if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) { > >>> val =3D 0; > >>> - if (phy_interface_mode_is_rgmii(bp->phy_interface)) > >>> - val =3D bp->usrio->rgmii; > >>> - else if (bp->phy_interface =3D=3D PHY_INTERFACE_MODE_RMII && > >>> - (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII)) > >>> - val =3D bp->usrio->rmii; > >>> - else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII)) > >>> - val =3D bp->usrio->mii; > >>> + if (bp->caps & MACB_CAPS_USRIO_HAS_MII) { > >>> + if (phy_interface_mode_is_rgmii(bp->phy_interface)) > >>> + val =3D bp->usrio->rgmii; > >>> + else if (bp->phy_interface =3D=3D PHY_INTERFACE_MODE_RMII && > >>> + (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII)) > >>> + val =3D bp->usrio->rmii; > >>> + else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII)) > >>> + val =3D bp->usrio->mii; > >>> + } > >>> > >>> if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN) > >>> val |=3D bp->usrio->refclk; > >> > >> Can bp->usrio be NULL here? The new MACB_CAPS_USRIO_HAS_MII capability > >> gates the MII-related dereferences of bp->usrio, but bp->usrio is also > >> dereferenced when MACB_CAPS_USRIO_HAS_CLKEN is set. > >> > >> With the change to default_gem_config later in this patch: > >> > >> static const struct macb_config default_gem_config =3D { > >> ... > >> .usrio =3D NULL, > >> ... > >> }; > >> > >> A device that matches the generic 'cdns,macb' compatible string (witho= ut > >> specific .data) will get bp->usrio =3D NULL during probe. If the device > >> tree then has 'cdns,refclk-ext', macb_configure_caps() adds > >> MACB_CAPS_USRIO_HAS_CLKEN without checking if bp->usrio is valid: > >> > >> macb_configure_caps() { > >> ... > >> if (of_property_read_bool(np, "cdns,refclk-ext")) > >> bp->caps |=3D MACB_CAPS_USRIO_HAS_CLKEN; > >> ... > >> } > >> > >> Then in macb_init(), the code would dereference bp->usrio->refclk with > >> bp->usrio being NULL, causing a crash. > >> > >> The MACB_CAPS_USRIO_HAS_MII gate only protects the MII-related > >> dereferences above, not the CLKEN dereference. > >=20 > > AI makes a good point here, but in one way this isn't even a problem > > with the patchset, it's actually another example of the sort of thing > > that I am trying to get rid of from the driver. As far as I can tell, > > this was added very recently, for the emac on sama7g5. The sama7g5-emac > > sets this cap in its match data, as do several other devices, so this > > code that sets the cap based on the dt property isn't needed. > >=20 > > I would like to get rid of setting the cap based on the dt property, > > because it is inherently tied to how at91 have their USRIO set up. > > Other platforms that want to use this external refclk might use a > > different mechanism for the selection, or only support an external > > source. What USRIO does is very platform specific, so it should be > > something that is opted in to explicitly. I didn't ask for the property > > to be bound to only the at91 devices that use it when I reviewed the > > binding, because I figured other platforms might be able to reuse the > > property and that's also why I didn't ask for a microchip prefix on the > > property. For the driver to reflect that general use, it shouldn't then > > do at91-specific things when the property is present. > >=20 > > Ryan, how does this property actually work now, given your removal > > of the cap from match data was reverted? I feel like it just doesn't do > > what you want it to do anymore? Before the revert, having the property > > meant that the value in sama7g5_usrio.refclk would be written to the > > hardware and not having the property meant that the bit would remain > > unset. You then had to revert to avoid breaking old devices, because > > your property changed the default behaviour for the emac, so now having > > the property means that the value in sama7g5_usrio.refclk is written to > > the hardware, but that also happens without the property because the cap > > is set in the match data. >=20 > So even with the revert the patch still does what it is intended to do, > mainly for the sama7g5_gem and the newer SAM devices, sam9x75 and sama7d6= 5. Oh, the gems actually have this, but with the default the other way to the emacs? I had figured that only the emacs actually had it, given they were all added prior to your change. Obviously the property then cannot be removed if the gems need it. > Currently with the removal of the change to the emac there is no > functional difference. With the newer SAM devices that need this usrio > flexibility this patch still works. >=20 > How this works now is for sama7g5_gmac configs, the default is to use > the internal clock for refclk then adding that dt property will set that > bit to 1 and refclk will be from an external source. For the > sama7g5_emac configs this has no effect due to ABI. > >=20 > > Unless I am missing something, should we not actually revert > > dce32ece3bb8f ("net: cadence: macb: Expose REFCLK as a device tree > > property") and 1b7531c094c88 ("dt-bindings: net: cdns,macb: Add external > > REFCLK property"), and instead add a property like > > "microchip,refclk-internal", because that would actually let you > > override the default behaviour of the driver? >=20 > I think this could be the better way to go as this keeps the existing > behavior for the legacy devices while still allowing the initial goal of > the patch series. Seems it would be more strait forward in the ABI sense > to have a "microchip,refclk-internal" property than an "refclk-ext" > property and having the risk of breaking older devices. By the sounds of things, you actually need both to be functional. The gems default to internal, so need refclk-external and the emacs default to external so need refclk-internal. Maybe it'd be better to have "microchip/cdns,refclk-source" that can be set to a string to deal with both cases? I'd also like to decouple setting the USRIO_HAS_CLKEN cap from what direction it is set in. That way, all devices with the bit in their USRIO would set the cap, but the value would come from either a default in match data or from the dt property if set. I think that's more natural behaviour for the capability, since the bit is in the usrio register regardless of which refclk source is used. Does that seem reasonable? Cheers, Conor. --NYgeMk65ZDrXJJA6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCaaciFgAKCRB4tDGHoIJi 0iS9AQCjnsRz0NWqsRl5qWTu4E7GWND3l2ogy4YgZPpLy+kH3gD9EogI23o/sbVv KHKyLaStf6KjgJurlA7hGFDZkb4AmQY= =MaXN -----END PGP SIGNATURE----- --NYgeMk65ZDrXJJA6--