[PATCH net-next v2 0/8] macb usrio/tsu patches

[PATCH net-next v2 0/8] macb usrio/tsu patches

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey folks,

After doing some debugging of broken tsu/ptp support on mpfs, I've come
up with some very rfc patches that I'd like opinions on - particularly
because they impact a bunch of platforms that I have no access to at all
and have no idea how they work. The at91 platforms I can just ask
Nicolas about (and he already provided some info directly, so I'm not
super worried at least about the usrio portion there) but the others
my gut says are likely incorrect in the driver at the moment.

These patches are fairly opinionated and not necessarily technically
correct or w/e. The only thing I am confident in saying that they are is
more deliberate than what's being done at the moment.

At the very least, it'd be good of the soc vendor folks could check
their platforms and see if their usrio stuff actually lines up with what
the driver currently calls "macb_default_usrio". Ours didn't and it was
a nasty surprise.

I sent this once before, but got no responses, maybe I'll get some this
time! To that end, I've also dropped the rfc, since noone expressed that
level of passing interest and this does fix a problem on my platform.
I've not marked it net because I don't think there's that level of
urgency, as the usrio default on our platform does what we want, and
we've provided a temporary solution of disabling usrio in our match data
to the reporter and in our downstream tree.

Theo, you added eyeq5 recently. Does it genuinely have the same usrio
bits as the at91 devices? That you send patches dealing with phys makes
it seem to me like it doesn't have the usrio stuff about mii modes..

Cheers,
Conor.

CC: Valentina.FernandezAlanis@microchip.com
CC: Andrew Lunn <andrew+netdev@lunn.ch>
CC: David S. Miller <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Paul Walmsley <pjw@kernel.org>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Albert Ou <aou@eecs.berkeley.edu>
CC: Alexandre Ghiti <alex@ghiti.fr>
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Claudiu Beznea <claudiu.beznea@tuxon.dev>
CC: Richard Cochran <richardcochran@gmail.com>
CC: Samuel Holland <samuel.holland@sifive.com>
CC: netdev@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-riscv@lists.infradead.org
CC: Neil Armstrong <narmstrong@baylibre.com>
CC: Dave Stevenson <dave.stevenson@raspberrypi.com>
CC: Sean Anderson <sean.anderson@linux.dev>
CC: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
CC: Abin Joseph <abin.joseph@amd.com>
CC: Théo Lebrun <theo.lebrun@bootlin.com>

Conor Dooley (8):
  riscv: dts: microchip: add tsu clock to macb on mpfs
  net: macb: rename macb_default_usrio to at91_default_usrio as not all
    platforms have mii mode control in usrio
  net: macb: np4 doesn't need a usrio pointer
  dt-bindings: net: macb: add property indicating timer adjust mode
  net: macb: timer adjust mode is not supported
  net: macb: add mpfs specific usrio configuration
  net: macb: warn on pclk use as a tsu_clk fallback
  net: macb: clean up tsu clk rate acquisition

 .../devicetree/bindings/net/cdns,macb.yaml    |  15 ++
 arch/riscv/boot/dts/microchip/Makefile.orig   |  26 ++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |   8 +-
 drivers/net/ethernet/cadence/macb.h           |   3 +
 drivers/net/ethernet/cadence/macb_main.c      | 137 +++++++++++-------
 5 files changed, 131 insertions(+), 58 deletions(-)
 create mode 100644 arch/riscv/boot/dts/microchip/Makefile.orig

-- 
2.51.0

[PATCH net-next v2 1/8] riscv: dts: microchip: add tsu clock to macb on mpfs

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

In increment mode, the tsu clock for the macb is provided separately to
the pck, usually the same clock as the reference to the rtc provided by
an off-chip oscillator. pclk is 150 MHz typically, and the reference is
either 100 MHz or 125 MHz, so having the tsu clock is required for
correct rate selection.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/boot/dts/microchip/Makefile.orig | 26 +++++++++++++++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi     |  8 +++----
 2 files changed, 30 insertions(+), 4 deletions(-)
 create mode 100644 arch/riscv/boot/dts/microchip/Makefile.orig

diff --git a/arch/riscv/boot/dts/microchip/Makefile.orig b/arch/riscv/boot/dts/microchip/Makefile.orig
new file mode 100644
index 0000000000000..e94f4096fd401
--- /dev/null
+++ b/arch/riscv/boot/dts/microchip/Makefile.orig
@@ -0,0 +1,26 @@
+# SPDX-License-Identifier: GPL-2.0
+<<<<<<< HEAD
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-beaglev-fire.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-disco-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-icicle-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-icicle-kit-prod.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-m100pfsevp.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-polarberry.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-sev-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-tysom-m.dtb
+||||||| constructed fake ancestor
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-beaglev-fire.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-icicle-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-m100pfsevp.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-polarberry.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-sev-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP_POLARFIRE) += mpfs-tysom-m.dtb
+=======
+dtb-$(CONFIG_ARCH_MICROCHIP) += mpfs-beaglev-fire.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP) += mpfs-icicle-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP) += mpfs-m100pfsevp.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP) += mpfs-polarberry.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP) += mpfs-sev-kit.dtb
+dtb-$(CONFIG_ARCH_MICROCHIP) += mpfs-tysom-m.dtb
+>>>>>>> riscv: dts: microchip: remove POLARFIRE mention in Makefile
+dtb-$(CONFIG_ARCH_MICROCHIP) += pic64gx-curiosity-kit.dtb
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 5c2963e269b83..ccd7c2e4724f4 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -448,8 +448,8 @@ mac0: ethernet@20110000 {
 			interrupt-parent = <&plic>;
 			interrupts = <64>, <65>, <66>, <67>, <68>, <69>;
 			local-mac-address = [00 00 00 00 00 00];
-			clocks = <&clkcfg CLK_MAC0>, <&clkcfg CLK_AHB>;
-			clock-names = "pclk", "hclk";
+			clocks = <&clkcfg CLK_MAC0>, <&clkcfg CLK_AHB>, <&refclk>;
+			clock-names = "pclk", "hclk", "tsu_clk";
 			resets = <&mss_top_sysreg CLK_MAC0>;
 			status = "disabled";
 		};
@@ -462,8 +462,8 @@ mac1: ethernet@20112000 {
 			interrupt-parent = <&plic>;
 			interrupts = <70>, <71>, <72>, <73>, <74>, <75>;
 			local-mac-address = [00 00 00 00 00 00];
-			clocks = <&clkcfg CLK_MAC1>, <&clkcfg CLK_AHB>;
-			clock-names = "pclk", "hclk";
+			clocks = <&clkcfg CLK_MAC1>, <&clkcfg CLK_AHB>, <&refclk>;
+			clock-names = "pclk", "hclk", "tsu_clk";
 			resets = <&mss_top_sysreg CLK_MAC1>;
 			status = "disabled";
 		};
-- 
2.51.0

[PATCH 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

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Calling this structure macb_default_usrio is misleading, I believe, as
it implies that it should be used if your platform has nothing special
to do in usrio. Since usrio is platform dependant, the default here is
probably for each usrio to do nothing, with the macb documentation I
have access to prescribing no standard behaviour here. We noticed that
this was problematic because on mpfs, a bit that macb_default_usrio
sets to deal with the MII mode actually changes the source for the
tsu_clk to something with how the majority of mpfs devices are actually
configured!

Rename it to at91_default_usrio, since that's where the values actually
come from for these. I have no idea if any of the other platforms that
use the default actually copied at91's usrio configuration or if they
have usrio configurations where what the driver does has no impact.

Gate touching these bits behind a capability, like the clken refclock
usrio knob, so that platforms without the MII mode stuff can avoid
running this code.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/net/ethernet/cadence/macb.h      |   1 +
 drivers/net/ethernet/cadence/macb_main.c | 108 +++++++++++++----------
 2 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 87414a2ddf6e3..8cb0b3778ee9e 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -779,6 +779,7 @@
 #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/ethernet/cadence/macb_main.c
index 5bc35f651ebd2..778d2115f66fc 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4613,13 +4613,15 @@ static int macb_init(struct platform_device *pdev)
 
 	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
 		val = 0;
-		if (phy_interface_mode_is_rgmii(bp->phy_interface))
-			val = bp->usrio->rgmii;
-		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
-			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
-			val = bp->usrio->rmii;
-		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
-			val = bp->usrio->mii;
+		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
+			if (phy_interface_mode_is_rgmii(bp->phy_interface))
+				val = bp->usrio->rgmii;
+			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
+				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
+				val = bp->usrio->rmii;
+			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
+				val = bp->usrio->mii;
+		}
 
 		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
 			val |= bp->usrio->refclk;
@@ -4637,13 +4639,6 @@ static int macb_init(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct macb_usrio_config macb_default_usrio = {
-	.mii = MACB_BIT(MII),
-	.rmii = MACB_BIT(RMII),
-	.rgmii = GEM_BIT(RGMII),
-	.refclk = MACB_BIT(CLKEN),
-};
-
 #if defined(CONFIG_OF)
 /* 1518 rounded up */
 #define AT91ETHER_MAX_RBUFF_SZ	0x600
@@ -5218,6 +5213,13 @@ static int eyeq5_init(struct platform_device *pdev)
 	return ret;
 }
 
+static const struct macb_usrio_config at91_default_usrio = {
+	.mii = MACB_BIT(MII),
+	.rmii = MACB_BIT(RMII),
+	.rgmii = GEM_BIT(RGMII),
+	.refclk = MACB_BIT(CLKEN),
+};
+
 static const struct macb_usrio_config sama7g5_usrio = {
 	.mii = 0,
 	.rmii = 1,
@@ -5228,104 +5230,114 @@ static const struct macb_usrio_config sama7g5_usrio = {
 
 static const struct macb_config fu540_c000_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP,
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = fu540_c000_clk_init,
 	.init = fu540_c000_init,
 	.jumbo_max_len = 10240,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config at91sam9260_config = {
-	.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+	.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII |
+		MACB_CAPS_USRIO_HAS_MII,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config sama5d3macb_config = {
 	.caps = MACB_CAPS_SG_DISABLED |
-		MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+		MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII |
+		MACB_CAPS_USRIO_HAS_MII,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config pc302gem_config = {
-	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
+	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config sama5d2_config = {
-	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO,
+	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
 	.jumbo_max_len = 10240,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config sama5d29_config = {
-	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_GEM_HAS_PTP,
+	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_GEM_HAS_PTP |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config sama5d3_config = {
 	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE |
-		MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO,
+		MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
 	.jumbo_max_len = 10240,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config sama5d4_config = {
-	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 4,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config emac_config = {
-	.caps = MACB_CAPS_NEEDS_RSTONUBR | MACB_CAPS_MACB_IS_EMAC,
+	.caps = MACB_CAPS_NEEDS_RSTONUBR | MACB_CAPS_MACB_IS_EMAC |
+		MACB_CAPS_USRIO_HAS_MII,
 	.clk_init = at91ether_clk_init,
 	.init = at91ether_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config np4_config = {
 	.caps = MACB_CAPS_USRIO_DISABLED,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config zynqmp_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 		MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
 	.jumbo_max_len = 10240,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config zynq_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF |
-		MACB_CAPS_NEEDS_RSTONUBR,
+		MACB_CAPS_NEEDS_RSTONUBR |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config mpfs_config = {
@@ -5335,7 +5347,7 @@ static const struct macb_config mpfs_config = {
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 	.max_tx_length = 4040, /* Cadence Erratum 1686 */
 	.jumbo_max_len = 4040,
 };
@@ -5343,7 +5355,8 @@ static const struct macb_config mpfs_config = {
 static const struct macb_config sama7g5_gem_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_CLK_HW_CHG |
 		MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII |
-		MACB_CAPS_MIIONRGMII | MACB_CAPS_GEM_HAS_PTP,
+		MACB_CAPS_MIIONRGMII | MACB_CAPS_GEM_HAS_PTP |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5353,7 +5366,8 @@ static const struct macb_config sama7g5_gem_config = {
 static const struct macb_config sama7g5_emac_config = {
 	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII |
 		MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_MIIONRGMII |
-		MACB_CAPS_GEM_HAS_PTP,
+		MACB_CAPS_GEM_HAS_PTP |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5364,12 +5378,13 @@ static const struct macb_config versal_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
 		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
 		MACB_CAPS_NEED_TSUCLK | MACB_CAPS_QUEUE_DISABLE |
-		MACB_CAPS_QBV,
+		MACB_CAPS_QBV |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
 	.jumbo_max_len = 10240,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config eyeq5_config = {
@@ -5380,17 +5395,18 @@ static const struct macb_config eyeq5_config = {
 	.clk_init = macb_clk_init,
 	.init = eyeq5_init,
 	.jumbo_max_len = 10240,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 };
 
 static const struct macb_config raspberrypi_rp1_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_CLK_HW_CHG |
 		MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP,
+		MACB_CAPS_GEM_HAS_PTP |
+		MACB_CAPS_USRIO_HAS_MII,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = &at91_default_usrio,
 	.jumbo_max_len = 10240,
 };
 
@@ -5431,7 +5447,7 @@ static const struct macb_config default_gem_config = {
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &macb_default_usrio,
+	.usrio = NULL,
 	.jumbo_max_len = 10240,
 };
 
-- 
2.51.0

[PATCH net-next v2 3/8] net: macb: np4 doesn't need a usrio pointer

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

USRIO is disabled on this platform, having a pointer to a usrio config
structure doesn't actually do anything other than look weird.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 778d2115f66fc..ddbb0c327b303 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5315,7 +5315,7 @@ static const struct macb_config np4_config = {
 	.caps = MACB_CAPS_USRIO_DISABLED,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
-	.usrio = &at91_default_usrio,
+	.usrio = NULL,
 };
 
 static const struct macb_config zynqmp_config = {
-- 
2.51.0

[PATCH net-next v2 4/8] dt-bindings: net: macb: add property indicating timer adjust mode

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The GEM IP has two methods for modifying the ptp timer. The first of
these, named "increment mode", relies on software controlling the timer
by setting tsu_timer_incr and tsu_timer_incr_sub_nsec and performing
once-off adjustments via the tsu_timer_adjust register. This is what the
macb driver uses. The second mechanism, "timer adjust mode" uses the
gem_tsu_inc_ctrl and gem_tsu_ms signals to control the timer. These
modes are not intended to be used in parallel, but both can be possible
on the same device and which mode is used cannot be determined from the
compatible on all devices, because some users of the GEM IP are SoC
FPGAs that permit configuring how the IP is wired up.

Add a property to indicate that gem_tsu_inc_ctrl and gem_tsu_ms are wired
up for timer adjust mode.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../devicetree/bindings/net/cdns,macb.yaml        | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index cb14c35ba9969..292279499d9e6 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -120,6 +120,12 @@ properties:
   power-domains:
     maxItems: 1
 
+  cdns,timer-adjust:
+    type: boolean
+    description:
+      Set when the hardware is operating in timer-adjust mode, where the timer
+      is controlled by the gem_tsu_inc_ctrl and gem_tsu_ms inputs.
+
   cdns,refclk-ext:
     type: boolean
     description:
@@ -186,6 +192,15 @@ allOf:
       properties:
         reg:
           maxItems: 1
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: microchip,mpfs-macb
+    then:
+      properties:
+        cdns,timer-adjust: false
 
   - if:
       properties:
-- 
2.51.0

[PATCH net-next v2 5/8] net: macb: timer adjust mode is not supported

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The ptp portion of this driver controls the tsu's timer using the
controls for "increment mode", which is not compatible with the hardware
trying to control it via the gem_tsu_inc_ctrl and gem_tsu_ms inputs in
"timer adjust mode". Abort probe if the property signalling that the
relevant signals have been wired up is present.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ddbb0c327b303..fa55e6e7036f2 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5535,6 +5535,12 @@ static int macb_probe(struct platform_device *pdev)
 
 	bp->usrio = macb_config->usrio;
 
+	if (of_property_read_bool(bp->pdev->dev.of_node, "cdns,timer-adjust") &&
+			IS_ENABLED(CONFIG_MACB_USE_HWSTAMP)) {
+		dev_err(&pdev->dev, "Timer adjust mode is not supported\n");
+		goto err_out_free_netdev;
+	}
+
 	/* By default we set to partial store and forward mode for zynqmp.
 	 * Disable if not set in devicetree.
 	 */
-- 
2.51.0

[PATCH net-next v2 6/8] net: macb: add mpfs specific usrio configuration

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

On mpfs the driver needs to make sure the tsu clock source is not the
fabric, as this requires that the hardware is in Timer Adjust mode,
which is not compatible with the linux driver trying to control the
hardware. It is unlikely that this will be set, as the peripheral is
reset during probe, but if the resets are not provided in devicetree
it's probable that this bit is set incorrectly, as U-Boot's macb driver
has the same issue with using usrio settings for at91 platforms as the
default.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/net/ethernet/cadence/macb.h      |  2 ++
 drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 8cb0b3778ee9e..04961658a21c2 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -780,6 +780,7 @@
 #define MACB_CAPS_RSC				BIT(23)
 #define MACB_CAPS_NO_LSO			BIT(24)
 #define MACB_CAPS_USRIO_HAS_MII			BIT(25)
+#define MACB_CAPS_USRIO_HAS_TSUCLK_SOURCE	BIT(25)
 
 /* LSO settings */
 #define MACB_LSO_UFO_ENABLE			0x01
@@ -1212,6 +1213,7 @@ struct macb_usrio_config {
 	u32 rgmii;
 	u32 refclk;
 	u32 hdfctlen;
+	u32 tsu_source;
 };
 
 struct macb_config {
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index fa55e6e7036f2..ca141f8935d48 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4626,6 +4626,9 @@ static int macb_init(struct platform_device *pdev)
 		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
 			val |= bp->usrio->refclk;
 
+		if (bp->caps & MACB_CAPS_USRIO_HAS_TSUCLK_SOURCE)
+			val |= bp->usrio->tsu_source;
+
 		macb_or_gem_writel(bp, USRIO, val);
 	}
 
@@ -5220,6 +5223,10 @@ static const struct macb_usrio_config at91_default_usrio = {
 	.refclk = MACB_BIT(CLKEN),
 };
 
+static const struct macb_usrio_config mpfs_usrio = {
+	.tsu_source = 0,
+};
+
 static const struct macb_usrio_config sama7g5_usrio = {
 	.mii = 0,
 	.rmii = 1,
@@ -5343,11 +5350,12 @@ static const struct macb_config zynq_config = {
 static const struct macb_config mpfs_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 		MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP,
+		MACB_CAPS_GEM_HAS_PTP |
+		MACB_CAPS_USRIO_HAS_TSUCLK_SOURCE,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
-	.usrio = &at91_default_usrio,
+	.usrio = &mpfs_usrio,
 	.max_tx_length = 4040, /* Cadence Erratum 1686 */
 	.jumbo_max_len = 4040,
 };
-- 
2.51.0

[PATCH net-next v2 7/8] net: macb: warn on pclk use as a tsu_clk fallback

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The Candence GEM IP has a configuration parameter which determines the
source of the clock used for the timestamp unit (if it is enabled),
switching it between using the pclk and a dedicated input.

When ptp support was added to the macb driver, a new tsu_clk was added
to represent the dedicated input. While this is understanble, I think
it is bug prone and that the tsu_clk should represent whatever clock is
used for the timerstamper and not just that specific input.

>From a devicetree point of view, it doesn't really matter if the tsu_clk
represents a physical clock or a conceptual clock, because the clock
configuration is determined when the IP is instantiated not at runtime.
>From a driver point of view, the benefit of taking the conceptual
approach is avoiding misconfiguring the driver when the hardware
supports ptp (and it is set as a capability in the relevant per-device
structure) but no tsu_clk is provided in devicetree. At the moment, the
timestamper will be registered and programmed with an increment that
reflects the pclk in these cases, but will malfunction if the pclk and
tsu_clk frequencies do not match.

Out of the devices that claim MACB_CAPS_GEM_HAS_PTP the fu540, mpfs,
sama5d2 and sama7g5-emac (but not sama7g5-gem) are at risk of having
this problem with the in-kernel devicetrees. It may be that these
platforms actually do use the pclk for the timestamper (either by
supplying pclk to the tsu_clk input of the IP, or by having the IP
block configured to use pclk instead of the tsu_clk input), but
at least mpfs is wrong though, it does not use pclk for the
tsu_clk, so the driver is registering the ptp clock incorrectly.

Add a warning if no tsu_clk is provided on a platform that uses the
timerstamper, to encourage people to specifically provide a tsu_clk and
avoid silently registering the timerstamper with the wrong clock

While this changes the meaning of the devicetree property, it is
backwards compatible as there's no functional change for platforms that
didn't provide a tsu_clk and the changed meaning of providing a tsu_clk
in the devicetree does not impact platforms that already provided one as
the decision about the tsu clock source is at IP instantiation time
rather than at runtime.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ca141f8935d48..5c24ea146d63d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3534,6 +3534,7 @@ static unsigned int gem_get_tsu_rate(struct macb *bp)
 	else if (!IS_ERR(bp->pclk)) {
 		tsu_clk = bp->pclk;
 		tsu_rate = clk_get_rate(tsu_clk);
+		dev_warn(&bp->pdev->dev, "devicetree missing tsu_clk, using pclk as fallback\n");
 	} else
 		return -ENOTSUPP;
 	return tsu_rate;
-- 
2.51.0

[PATCH net-next v2 8/8] net: macb: clean up tsu clk rate acquisition

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

tsu_clk is grabbed during probe, so doesn't need to be re-grabbed here.
pclk is mandatory, probe will fail if it is err/NULL, so there's no need
to check it here or have a !pclk 3rd arm. Simplify gem_get_tsu_rate() to
account for these facts.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 5c24ea146d63d..f5ec283ddb497 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3527,16 +3527,14 @@ static unsigned int gem_get_tsu_rate(struct macb *bp)
 	struct clk *tsu_clk;
 	unsigned int tsu_rate;
 
-	tsu_clk = devm_clk_get(&bp->pdev->dev, "tsu_clk");
-	if (!IS_ERR(tsu_clk))
-		tsu_rate = clk_get_rate(tsu_clk);
-	/* try pclk instead */
-	else if (!IS_ERR(bp->pclk)) {
+	if (!IS_ERR_OR_NULL(bp->tsu_clk)) {
+		tsu_rate = clk_get_rate(bp->tsu_clk);
+	} else {
 		tsu_clk = bp->pclk;
 		tsu_rate = clk_get_rate(tsu_clk);
 		dev_warn(&bp->pdev->dev, "devicetree missing tsu_clk, using pclk as fallback\n");
-	} else
-		return -ENOTSUPP;
+	}
+
 	return tsu_rate;
 }
 
-- 
2.51.0

Re: [PATCH net-next v2 1/8] riscv: dts: microchip: add tsu clock to macb on mpfs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Thu, Feb 26, 2026 at 11:03:16AM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> In increment mode, the tsu clock for the macb is provided separately to
> the pck, usually the same clock as the reference to the rtc provided by
> an off-chip oscillator. pclk is 150 MHz typically, and the reference is
> either 100 MHz or 125 MHz, so having the tsu clock is required for
> correct rate selection.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/boot/dts/microchip/Makefile.orig | 26 +++++++++++++++++++++

Of course I noticed this only after sending, but this patch isn't for
net anyway so it's kinda moot and I could fix it myself on application
or in the likely v3.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jakub Kicinski]

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-local.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/ethernet/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/ethernet/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 = 0;
> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
> -			val = bp->usrio->rgmii;
> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> -			val = bp->usrio->rmii;
> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> -			val = bp->usrio->mii;
> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
> +				val = bp->usrio->rgmii;
> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> +				val = bp->usrio->rmii;
> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> +				val = bp->usrio->mii;
> +		}
>
>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
>  			val |= 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 = {
	...
	.usrio = NULL,
	...
};

A device that matches the generic 'cdns,macb' compatible string (without
specific .data) will get bp->usrio = 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 |= 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.

> @@ -4638,13 +4640,6 @@
>  	return 0;
>  }
>
> -static const struct macb_usrio_config macb_default_usrio = {
> -	.mii = MACB_BIT(MII),
> -	.rmii = MACB_BIT(RMII),
> -	.rgmii = GEM_BIT(RGMII),
> -	.refclk = MACB_BIT(CLKEN),
> -};
> -

[ ... ]

> @@ -5219,6 +5214,13 @@ static int eyeq5_init(struct platform_device *pdev)
>  	return ret;
>  }
>
> +static const struct macb_usrio_config at91_default_usrio = {
> +	.mii = MACB_BIT(MII),
> +	.rmii = MACB_BIT(RMII),
> +	.rgmii = GEM_BIT(RGMII),
> +	.refclk = MACB_BIT(CLKEN),
> +};
> +

[ ... ]

> @@ -5432,7 +5448,7 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,
> -	.usrio = &macb_default_usrio,
> +	.usrio = NULL,
>  	.jumbo_max_len = 10240,
>  };
>
-- 
pw-bot: cr

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6569 bytes --]

Yo,

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-local.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/ethernet/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/ethernet/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 = 0;
> > -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
> > -			val = bp->usrio->rgmii;
> > -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> > -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > -			val = bp->usrio->rmii;
> > -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > -			val = bp->usrio->mii;
> > +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
> > +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
> > +				val = bp->usrio->rgmii;
> > +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> > +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > +				val = bp->usrio->rmii;
> > +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > +				val = bp->usrio->mii;
> > +		}
> >
> >  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> >  			val |= 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 = {
> 	...
> 	.usrio = NULL,
> 	...
> };
> 
> A device that matches the generic 'cdns,macb' compatible string (without
> specific .data) will get bp->usrio = 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 |= 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.

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.

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.

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.

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?

Cheers,
Conor.

> 
> > @@ -4638,13 +4640,6 @@
> >  	return 0;
> >  }
> >
> > -static const struct macb_usrio_config macb_default_usrio = {
> > -	.mii = MACB_BIT(MII),
> > -	.rmii = MACB_BIT(RMII),
> > -	.rgmii = GEM_BIT(RGMII),
> > -	.refclk = MACB_BIT(CLKEN),
> > -};
> > -
> 
> [ ... ]
> 
> > @@ -5219,6 +5214,13 @@ static int eyeq5_init(struct platform_device *pdev)
> >  	return ret;
> >  }
> >
> > +static const struct macb_usrio_config at91_default_usrio = {
> > +	.mii = MACB_BIT(MII),
> > +	.rmii = MACB_BIT(RMII),
> > +	.rgmii = GEM_BIT(RGMII),
> > +	.refclk = MACB_BIT(CLKEN),
> > +};
> > +
> 
> [ ... ]
> 
> > @@ -5432,7 +5448,7 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
> >  	.dma_burst_length = 16,
> >  	.clk_init = macb_clk_init,
> >  	.init = macb_init,
> > -	.usrio = &macb_default_usrio,
> > +	.usrio = NULL,
> >  	.jumbo_max_len = 10240,
> >  };
> >
> -- 
> pw-bot: cr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v2 5/8] net: macb: timer adjust mode is not supported

[Simon Horman]

On Thu, Feb 26, 2026 at 11:03:20AM +0000, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The ptp portion of this driver controls the tsu's timer using the
> controls for "increment mode", which is not compatible with the hardware
> trying to control it via the gem_tsu_inc_ctrl and gem_tsu_ms inputs in
> "timer adjust mode". Abort probe if the property signalling that the
> relevant signals have been wired up is present.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ddbb0c327b303..fa55e6e7036f2 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5535,6 +5535,12 @@ static int macb_probe(struct platform_device *pdev)
>  
>  	bp->usrio = macb_config->usrio;
>  
> +	if (of_property_read_bool(bp->pdev->dev.of_node, "cdns,timer-adjust") &&
> +			IS_ENABLED(CONFIG_MACB_USE_HWSTAMP)) {
> +		dev_err(&pdev->dev, "Timer adjust mode is not supported\n");

Hi Conor,

Assuming this is an error condition I think that err should
be set to an negative error value here. Else the function will return 0.

Flagged by Smatch.

> +		goto err_out_free_netdev;
> +	}
> +
>  	/* By default we set to partial store and forward mode for zynqmp.
>  	 * Disable if not set in devicetree.
>  	 */
> -- 
> 2.51.0
> 
> 

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

[Ryan Wanner]

On 2/28/26 17:06, Conor Dooley wrote:
> Yo,
> 
> 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-local.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/ethernet/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/ethernet/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 = 0;
>>> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>> -			val = bp->usrio->rgmii;
>>> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>>> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>> -			val = bp->usrio->rmii;
>>> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>> -			val = bp->usrio->mii;
>>> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
>>> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>> +				val = bp->usrio->rgmii;
>>> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>>> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>> +				val = bp->usrio->rmii;
>>> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>> +				val = bp->usrio->mii;
>>> +		}
>>>
>>>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
>>>  			val |= 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 = {
>> 	...
>> 	.usrio = NULL,
>> 	...
>> };
>>
>> A device that matches the generic 'cdns,macb' compatible string (without
>> specific .data) will get bp->usrio = 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 |= 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.
> 
> 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.
> 
> 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.
> 
> 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.

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 sama7d65.

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.

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.
> 
> 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?

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.

Just my train of thought with this,
Ryan
> 
> Cheers,
> Conor.
> 
>>
>>> @@ -4638,13 +4640,6 @@
>>>  	return 0;
>>>  }
>>>
>>> -static const struct macb_usrio_config macb_default_usrio = {
>>> -	.mii = MACB_BIT(MII),
>>> -	.rmii = MACB_BIT(RMII),
>>> -	.rgmii = GEM_BIT(RGMII),
>>> -	.refclk = MACB_BIT(CLKEN),
>>> -};
>>> -
>>
>> [ ... ]
>>
>>> @@ -5219,6 +5214,13 @@ static int eyeq5_init(struct platform_device *pdev)
>>>  	return ret;
>>>  }
>>>
>>> +static const struct macb_usrio_config at91_default_usrio = {
>>> +	.mii = MACB_BIT(MII),
>>> +	.rmii = MACB_BIT(RMII),
>>> +	.rgmii = GEM_BIT(RGMII),
>>> +	.refclk = MACB_BIT(CLKEN),
>>> +};
>>> +
>>
>> [ ... ]
>>
>>> @@ -5432,7 +5448,7 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
>>>  	.dma_burst_length = 16,
>>>  	.clk_init = macb_clk_init,
>>>  	.init = macb_init,
>>> -	.usrio = &macb_default_usrio,
>>> +	.usrio = NULL,
>>>  	.jumbo_max_len = 10240,
>>>  };
>>>
>> -- 
>> pw-bot: cr

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 8067 bytes --]

On Tue, Mar 03, 2026 at 10:35:05AM -0700, Ryan Wanner wrote:
> On 2/28/26 17:06, Conor Dooley wrote:
> > Yo,
> > 
> > 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-local.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/ethernet/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/ethernet/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 = 0;
> >>> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
> >>> -			val = bp->usrio->rgmii;
> >>> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> >>> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>> -			val = bp->usrio->rmii;
> >>> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>> -			val = bp->usrio->mii;
> >>> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
> >>> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
> >>> +				val = bp->usrio->rgmii;
> >>> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> >>> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>> +				val = bp->usrio->rmii;
> >>> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>> +				val = bp->usrio->mii;
> >>> +		}
> >>>
> >>>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> >>>  			val |= 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 = {
> >> 	...
> >> 	.usrio = NULL,
> >> 	...
> >> };
> >>
> >> A device that matches the generic 'cdns,macb' compatible string (without
> >> specific .data) will get bp->usrio = 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 |= 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.
> > 
> > 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.
> > 
> > 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.
> > 
> > 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.
> 
> 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 sama7d65.

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.
> 
> 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.
> > 
> > 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?
> 
> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ryan Wanner]

On 3/3/26 11:01, Conor Dooley wrote:
> On Tue, Mar 03, 2026 at 10:35:05AM -0700, Ryan Wanner wrote:
>> On 2/28/26 17:06, Conor Dooley wrote:
>>> Yo,
>>>
>>> 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-local.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/ethernet/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/ethernet/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 = 0;
>>>>> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>>>> -			val = bp->usrio->rgmii;
>>>>> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>>>>> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>> -			val = bp->usrio->rmii;
>>>>> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>> -			val = bp->usrio->mii;
>>>>> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
>>>>> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>>>> +				val = bp->usrio->rgmii;
>>>>> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>>>>> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>> +				val = bp->usrio->rmii;
>>>>> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>> +				val = bp->usrio->mii;
>>>>> +		}
>>>>>
>>>>>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
>>>>>  			val |= 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 = {
>>>> 	...
>>>> 	.usrio = NULL,
>>>> 	...
>>>> };
>>>>
>>>> A device that matches the generic 'cdns,macb' compatible string (without
>>>> specific .data) will get bp->usrio = 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 |= 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> 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.
>>
>> 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 sama7d65.
> 
> 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.

Yes that is correct, I am not sure why this was left out in the initial
implementation of the gems maybe others can give some insight. The gems
can, similar to the emacs, take an external or internal ref clock that
is decided by the usrio registers. The difference with the gems and the
emacs is the gems can have 125MHz refclk for RGMII (50Mhz for RMII)
either internal or external where as the emacs can only switch refclk
for RMII.

> 
>> 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.
>>
>> 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.
>>>
>>> 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?
>>
>> 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?

Since a dt string would need to be placed either way could the default
be setting be 1 like how the emacs is set then make a
"microchip,refclk-internal" flag, this seems to have the minimal amount
of conflicts with legacy implementation.

> 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?

So to leave the "USRIO_HAS_CLKEN" caps flag in the existing configs for
emacs, then have this be a toggle for the gem configs, and the
"microchip/cdns,refclk-source" would be parsed to either set this bit to
1 or 0? The USRIO refclk bit will be set in the macb_init() based on if
the dt property is there rather than how it is currently implemented now
if I understand correctly?

Ryan

> 
> Cheers,
> Conor.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 11246 bytes --]

On Tue, Mar 03, 2026 at 03:04:19PM -0700, Ryan Wanner wrote:
> On 3/3/26 11:01, Conor Dooley wrote:
> > On Tue, Mar 03, 2026 at 10:35:05AM -0700, Ryan Wanner wrote:
> >> On 2/28/26 17:06, Conor Dooley wrote:
> >>> Yo,
> >>>
> >>> 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-local.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/ethernet/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/ethernet/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 = 0;
> >>>>> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
> >>>>> -			val = bp->usrio->rgmii;
> >>>>> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> >>>>> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>>>> -			val = bp->usrio->rmii;
> >>>>> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>>>> -			val = bp->usrio->mii;
> >>>>> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
> >>>>> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
> >>>>> +				val = bp->usrio->rgmii;
> >>>>> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> >>>>> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>>>> +				val = bp->usrio->rmii;
> >>>>> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> >>>>> +				val = bp->usrio->mii;
> >>>>> +		}
> >>>>>
> >>>>>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> >>>>>  			val |= 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 = {
> >>>> 	...
> >>>> 	.usrio = NULL,
> >>>> 	...
> >>>> };
> >>>>
> >>>> A device that matches the generic 'cdns,macb' compatible string (without
> >>>> specific .data) will get bp->usrio = 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 |= 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.
> >>>
> >>> 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.
> >>>
> >>> 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.
> >>>
> >>> 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.
> >>
> >> 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 sama7d65.
> > 
> > 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.
> 
> Yes that is correct, I am not sure why this was left out in the initial
> implementation of the gems maybe others can give some insight. The gems
> can, similar to the emacs, take an external or internal ref clock that
> is decided by the usrio registers. The difference with the gems and the
> emacs is the gems can have 125MHz refclk for RGMII (50Mhz for RMII)
> either internal or external where as the emacs can only switch refclk
> for RMII.
> 
> > 
> >> 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.
> >>
> >> 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.
> >>>
> >>> 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?
> >>
> >> 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?
> 
> Since a dt string would need to be placed either way could the default
> be setting be 1 like how the emacs is set then make a
> "microchip,refclk-internal" flag, this seems to have the minimal amount
> of conflicts with legacy implementation.

Unless I misunderstand what you're saying, you're suggesting changing
the default for the non-emacs? I don't think this can be done, for the
same reason that your emac patch got reverted - it breaks existing
devices. With my suggestion, the driver support for refclk-ext would
remain, so as not to break any existing setups, but it would be removed
from the binding or marked deprecated.

> > 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?
> 
> So to leave the "USRIO_HAS_CLKEN" caps flag in the existing configs for
> emacs, then have this be a toggle for the gem configs, and the
> "microchip/cdns,refclk-source" would be parsed to either set this bit to
> 1 or 0? The USRIO refclk bit will be set in the macb_init() based on if
> the dt property is there rather than how it is currently implemented now
> if I understand correctly?

No, I don't think this is what I am suggesting. I am suggesting adding
USRIO_HAS_CLKEN to the gem configs, and then modifying the usrio struct
to have a member called something like "refclk_default_enable" and setting
it to true for emacs and false for gems. Then in init code somewhere, we
would do:

	if (property_present(refclk-source))
		if (property_read_string(refclk-source) == "external"
			refclk_enable = true
		else
			refclk_enable = false
	else if (property_present(refclk-ext))
		refclk_enable = true
	else
		refclk_enable = usrio.refclk_default_enable

Then later on in init, when configuring the caps, we would do:

	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {

		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
			val |= refclk_enable;

		macb_or_gem_writel(bp, USRIO, val);
	}

Or maybe we just roll all that up together inside the !USRIO_DISABLED
code. 

The code currently in macb_configure_caps() about refclks would be
deleted.

This would produce one property that works for both emacs and gems, not
change any defaults for existing devices and retain support for whatever
devicetrees made it into the wild with your property.

Does that make sense?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ryan Wanner]

On 3/3/26 15:44, Conor Dooley wrote:
> On Tue, Mar 03, 2026 at 03:04:19PM -0700, Ryan Wanner wrote:
>> On 3/3/26 11:01, Conor Dooley wrote:
>>> On Tue, Mar 03, 2026 at 10:35:05AM -0700, Ryan Wanner wrote:
>>>> On 2/28/26 17:06, Conor Dooley wrote:
>>>>> Yo,
>>>>>
>>>>> 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-local.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/ethernet/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/ethernet/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 = 0;
>>>>>>> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>>>>>> -			val = bp->usrio->rgmii;
>>>>>>> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>>>>>>> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>>>> -			val = bp->usrio->rmii;
>>>>>>> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>>>> -			val = bp->usrio->mii;
>>>>>>> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
>>>>>>> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>>>>>> +				val = bp->usrio->rgmii;
>>>>>>> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
>>>>>>> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>>>> +				val = bp->usrio->rmii;
>>>>>>> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
>>>>>>> +				val = bp->usrio->mii;
>>>>>>> +		}
>>>>>>>
>>>>>>>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
>>>>>>>  			val |= 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 = {
>>>>>> 	...
>>>>>> 	.usrio = NULL,
>>>>>> 	...
>>>>>> };
>>>>>>
>>>>>> A device that matches the generic 'cdns,macb' compatible string (without
>>>>>> specific .data) will get bp->usrio = 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 |= 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> 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 sama7d65.
>>>
>>> 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.
>>
>> Yes that is correct, I am not sure why this was left out in the initial
>> implementation of the gems maybe others can give some insight. The gems
>> can, similar to the emacs, take an external or internal ref clock that
>> is decided by the usrio registers. The difference with the gems and the
>> emacs is the gems can have 125MHz refclk for RGMII (50Mhz for RMII)
>> either internal or external where as the emacs can only switch refclk
>> for RMII.
>>
>>>
>>>> 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.
>>>>
>>>> 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.
>>>>>
>>>>> 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?
>>>>
>>>> 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?
>>
>> Since a dt string would need to be placed either way could the default
>> be setting be 1 like how the emacs is set then make a
>> "microchip,refclk-internal" flag, this seems to have the minimal amount
>> of conflicts with legacy implementation.
> 
> Unless I misunderstand what you're saying, you're suggesting changing
> the default for the non-emacs? I don't think this can be done, for the
> same reason that your emac patch got reverted - it breaks existing
> devices. With my suggestion, the driver support for refclk-ext would
> remain, so as not to break any existing setups, but it would be removed
> from the binding or marked deprecated.

I understand what you are saying now about the gem and emcas being flipped.

> 
>>> 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?
>>
>> So to leave the "USRIO_HAS_CLKEN" caps flag in the existing configs for
>> emacs, then have this be a toggle for the gem configs, and the
>> "microchip/cdns,refclk-source" would be parsed to either set this bit to
>> 1 or 0? The USRIO refclk bit will be set in the macb_init() based on if
>> the dt property is there rather than how it is currently implemented now
>> if I understand correctly?
> 
> No, I don't think this is what I am suggesting. I am suggesting adding
> USRIO_HAS_CLKEN to the gem configs, and then modifying the usrio struct
> to have a member called something like "refclk_default_enable" and setting
> it to true for emacs and false for gems. Then in init code somewhere, we
> would do:
> 
> 	if (property_present(refclk-source))
> 		if (property_read_string(refclk-source) == "external"
> 			refclk_enable = true
> 		else
> 			refclk_enable = false
> 	else if (property_present(refclk-ext))
> 		refclk_enable = true
> 	else
> 		refclk_enable = usrio.refclk_default_enable
> 
> Then later on in init, when configuring the caps, we would do:
> 
> 	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> 
> 		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> 			val |= refclk_enable;
> 
> 		macb_or_gem_writel(bp, USRIO, val);
> 	}
> 
> Or maybe we just roll all that up together inside the !USRIO_DISABLED
> code. 
> 
> The code currently in macb_configure_caps() about refclks would be
> deleted.
> 
> This would produce one property that works for both emacs and gems, not
> change any defaults for existing devices and retain support for whatever
> devicetrees made it into the wild with your property.
> 
> Does that make sense?

If we are set to have an external and internal clock dt property than I
think we should just keep those if statements inside the !USRIO_DISABLED
code block just to keep it more organized and clean and keeping the
current HAS_CLKEN if statement be the fallback default case to set the
register bit value. Some thing like this:

if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
		if (property_present(refclk-source)) {
			if (property_read_string == "external")
				val |= true;
			else
				val |= false;
		}
		else if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
			val |= bp->usrio->refclk;

		macb_or_gem_writel(bp, USRIO, val);
}

Would this be a good middle ground?

Ryan

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2732 bytes --]

On Wed, Mar 04, 2026 at 09:13:39AM -0700, Ryan Wanner wrote:
> > No, I don't think this is what I am suggesting. I am suggesting adding
> > USRIO_HAS_CLKEN to the gem configs, and then modifying the usrio struct
> > to have a member called something like "refclk_default_enable" and setting
> > it to true for emacs and false for gems. Then in init code somewhere, we
> > would do:
> > 
> > 	if (property_present(refclk-source))
> > 		if (property_read_string(refclk-source) == "external"
> > 			refclk_enable = true
> > 		else
> > 			refclk_enable = false
> > 	else if (property_present(refclk-ext))
> > 		refclk_enable = true
> > 	else
> > 		refclk_enable = usrio.refclk_default_enable
> > 
> > Then later on in init, when configuring the caps, we would do:
> > 
> > 	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> > 
> > 		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> > 			val |= refclk_enable;
> > 
> > 		macb_or_gem_writel(bp, USRIO, val);
> > 	}
> > 
> > Or maybe we just roll all that up together inside the !USRIO_DISABLED
> > code. 
> > 
> > The code currently in macb_configure_caps() about refclks would be
> > deleted.
> > 
> > This would produce one property that works for both emacs and gems, not
> > change any defaults for existing devices and retain support for whatever
> > devicetrees made it into the wild with your property.
> > 
> > Does that make sense?
> 
> If we are set to have an external and internal clock dt property than I
> think we should just keep those if statements inside the !USRIO_DISABLED
> code block just to keep it more organized and clean and keeping the
> current HAS_CLKEN if statement be the fallback default case to set the
> register bit value. Some thing like this:
> 
> if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> 		if (property_present(refclk-source)) {
> 			if (property_read_string == "external")
> 				val |= true;
> 			else
> 				val |= false;
> 		}
> 		else if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> 			val |= bp->usrio->refclk;
> 
> 		macb_or_gem_writel(bp, USRIO, val);
> }
> 
> Would this be a good middle ground?

I explicitly would like to make HAS_CLKEN represent the USRIO having the
bit, and not ascribe behaviour to it, so that it is actually a
capability. This is why I want to make the default a member of the usrio
struct in match data.
Also, the bit position depends on the usrio struct, which we both
omitted from our pseudo code, which means that even knowing how to
modify val depends on having HAS_CLKEN/usrio.refclk being set, so your
fallback doesn't sit right with me - I am trying to avoid things writing
to USRIO without specifically being told what it is writing to is valid.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3364 bytes --]

On Wed, Mar 04, 2026 at 06:52:58PM +0000, Conor Dooley wrote:
> On Wed, Mar 04, 2026 at 09:13:39AM -0700, Ryan Wanner wrote:
> > > No, I don't think this is what I am suggesting. I am suggesting adding
> > > USRIO_HAS_CLKEN to the gem configs, and then modifying the usrio struct
> > > to have a member called something like "refclk_default_enable" and setting
> > > it to true for emacs and false for gems. Then in init code somewhere, we
> > > would do:
> > > 
> > > 	if (property_present(refclk-source))
> > > 		if (property_read_string(refclk-source) == "external"
> > > 			refclk_enable = true
> > > 		else
> > > 			refclk_enable = false
> > > 	else if (property_present(refclk-ext))
> > > 		refclk_enable = true
> > > 	else
> > > 		refclk_enable = usrio.refclk_default_enable
> > > 
> > > Then later on in init, when configuring the caps, we would do:
> > > 
> > > 	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> > > 
> > > 		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> > > 			val |= refclk_enable;
> > > 
> > > 		macb_or_gem_writel(bp, USRIO, val);
> > > 	}
> > > 
> > > Or maybe we just roll all that up together inside the !USRIO_DISABLED
> > > code. 
> > > 
> > > The code currently in macb_configure_caps() about refclks would be
> > > deleted.
> > > 
> > > This would produce one property that works for both emacs and gems, not
> > > change any defaults for existing devices and retain support for whatever
> > > devicetrees made it into the wild with your property.
> > > 
> > > Does that make sense?
> > 
> > If we are set to have an external and internal clock dt property than I
> > think we should just keep those if statements inside the !USRIO_DISABLED
> > code block just to keep it more organized and clean and keeping the
> > current HAS_CLKEN if statement be the fallback default case to set the
> > register bit value. Some thing like this:
> > 
> > if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> > 		if (property_present(refclk-source)) {
> > 			if (property_read_string == "external")
> > 				val |= true;
> > 			else
> > 				val |= false;
> > 		}
> > 		else if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> > 			val |= bp->usrio->refclk;
> > 
> > 		macb_or_gem_writel(bp, USRIO, val);
> > }
> > 
> > Would this be a good middle ground?
> 
> I explicitly would like to make HAS_CLKEN represent the USRIO having the
> bit, and not ascribe behaviour to it, so that it is actually a
> capability. This is why I want to make the default a member of the usrio
> struct in match data.
> Also, the bit position depends on the usrio struct, which we both
> omitted from our pseudo code, which means that even knowing how to
> modify val depends on having HAS_CLKEN/usrio.refclk being set, so your
> fallback doesn't sit right with me - I am trying to avoid things writing
> to USRIO without specifically being told what it is writing to is valid.

Ryan and I have discussed this a bit more, and it seems like the
USRIO_HAS_CLKEN capability flag is also being used to represent two
different "features" entirely (turning on a xceiver clock and choosing
the refclk source), depending on which device the driver has been bound
to. The next version of this series will also split this into two
different capability flags, so that each actually does what it says on
the tin.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]