diff mbox series

[net-next,v7,2/8] net: stmmac: dwmac-stm32: Separate out external clock rate validation

Message ID 20240611083606.733453-3-christophe.roullier@foss.st.com (mailing list archive)
State New, archived
Headers show
Series Series to deliver Ethernet for STM32MP13 | expand

Commit Message

Christophe Roullier June 11, 2024, 8:36 a.m. UTC 
From: Marek Vasut <marex@denx.de>

Pull the external clock frequency validation into a separate function,
to avoid conflating it with external clock DT property decoding and
clock mux register configuration. This should make the code easier to
read and understand.

This does change the code behavior slightly. The clock mux PMCR register
setting now depends solely on the DT properties which configure the clock
mux between external clock and internal RCC generated clock. The mux PMCR
register settings no longer depend on the supplied clock frequency, that
supplied clock frequency is now only validated, and if the clock frequency
is invalid for a mode, it is rejected.

Previously, the code would switch the PMCR register clock mux to internal
RCC generated clock if external clock couldn't provide suitable frequency,
without checking whether the RCC generated clock frequency is correct. Such
behavior is risky at best, user should have configured their clock correctly
in the first place, so this behavior is removed here.

Signed-off-by: Marek Vasut <marex@denx.de>
Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 51 +++++++++++++++----
 1 file changed, 41 insertions(+), 10 deletions(-)

Comments

Mark Brown June 26, 2024, 10:38 a.m. UTC | #1 
On Tue, Jun 11, 2024 at 10:36:00AM +0200, Christophe Roullier wrote:
> From: Marek Vasut <marex@denx.de>
> 
> Pull the external clock frequency validation into a separate function,
> to avoid conflating it with external clock DT property decoding and
> clock mux register configuration. This should make the code easier to
> read and understand.

For the past few days networking has been broken on the Avenger 96, a
stm32mp157a based platform.  The stm32-dwmac driver fails to probe:

<6>[    1.894271] stm32-dwmac 5800a000.ethernet: IRQ eth_wake_irq not found
<6>[    1.899694] stm32-dwmac 5800a000.ethernet: IRQ eth_lpi not found
<6>[    1.905849] stm32-dwmac 5800a000.ethernet: IRQ sfty not found
<3>[    1.912304] stm32-dwmac 5800a000.ethernet: Unable to parse OF data
<3>[    1.918393] stm32-dwmac 5800a000.ethernet: probe with driver stm32-dwmac failed with error -75

which looks a bit odd given the commit contents but I didn't look at the
driver code at all.

Full boot log here:

   https://lava.sirena.org.uk/scheduler/job/467150

A working equivalent is here:

   https://lava.sirena.org.uk/scheduler/job/466518

A bisection identified this commit as being responsible, log below:

git bisect start
# status: waiting for both good and bad commits
# bad: [0fc4bfab2cd45f9acb86c4f04b5191e114e901ed] Add linux-next specific files for 20240625
git bisect bad 0fc4bfab2cd45f9acb86c4f04b5191e114e901ed
# status: waiting for good commit(s), bad commit known
# good: [3d9217c41c07b72af3a5c147cb82c75f757f4200] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect good 3d9217c41c07b72af3a5c147cb82c75f757f4200
# bad: [5699faecf4e2347f81eea62db0455feb4d794537] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect bad 5699faecf4e2347f81eea62db0455feb4d794537
# good: [ba73da675606373565868962ad8c615f175662ed] Merge branch 'fs-next' of linux-next
git bisect good ba73da675606373565868962ad8c615f175662ed
# bad: [7e7c714a36a5b10e391168e7e8145060e041ea12] Merge branch 'af_unix-remove-spin_lock_nested-and-convert-to-lock_cmp_fn'
git bisect bad 7e7c714a36a5b10e391168e7e8145060e041ea12
# good: [93d4e8bb3f137e8037a65ea96f175f81c25c50e5] Merge tag 'wireless-next-2024-06-07' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
git bisect good 93d4e8bb3f137e8037a65ea96f175f81c25c50e5
# bad: [4314175af49668ab20c0d60d7d7657986e1d0c7c] Merge branch 'net-smc-IPPROTO_SMC'
git bisect bad 4314175af49668ab20c0d60d7d7657986e1d0c7c
# good: [811efc06e5f30a57030451b2d1998aa81273baf8] net/tcp: Move tcp_inbound_hash() from headers
git bisect good 811efc06e5f30a57030451b2d1998aa81273baf8
# good: [5f703ce5c981ee02c00e210d5b155bbbfbf11263] net: hsr: Send supervisory frames to HSR network with ProxyNodeTable data
git bisect good 5f703ce5c981ee02c00e210d5b155bbbfbf11263
# bad: [6c3282a6b296385bee2c383442c39f507b0d51dd] net: stmmac: add select_pcs() platform method
git bisect bad 6c3282a6b296385bee2c383442c39f507b0d51dd
# bad: [404dbd26322f50c8123bf5bff9a409356889035f] net: qrtr: ns: Ignore ENODEV failures in ns
git bisect bad 404dbd26322f50c8123bf5bff9a409356889035f
# bad: [c60a54b52026bd2c9a88ae00f2aac7a67fed8e38] net: stmmac: dwmac-stm32: Clean up the debug prints
git bisect bad c60a54b52026bd2c9a88ae00f2aac7a67fed8e38
# bad: [582ac134963e2d5cf6c45db027e156fcfb7f7678] net: stmmac: dwmac-stm32: Separate out external clock rate validation
git bisect bad 582ac134963e2d5cf6c45db027e156fcfb7f7678
# good: [8a9044e5169bab7a8edadb4ceb748391657f0d7f] dt-bindings: net: add STM32MP13 compatible in documentation for stm32
git bisect good 8a9044e5169bab7a8edadb4ceb748391657f0d7f
# first bad commit: [582ac134963e2d5cf6c45db027e156fcfb7f7678] net: stmmac: dwmac-stm32: Separate out external clock rate validation
Christophe Roullier June 27, 2024, 6:45 a.m. UTC | #2 
Hi Mark,

Sorry, issue found, I will push fix this morning.

Regards.

Christophe.

On 6/26/24 12:38, Mark Brown wrote:
> On Tue, Jun 11, 2024 at 10:36:00AM +0200, Christophe Roullier wrote:
>> From: Marek Vasut <marex@denx.de>
>>
>> Pull the external clock frequency validation into a separate function,
>> to avoid conflating it with external clock DT property decoding and
>> clock mux register configuration. This should make the code easier to
>> read and understand.
> For the past few days networking has been broken on the Avenger 96, a
> stm32mp157a based platform.  The stm32-dwmac driver fails to probe:
>
> <6>[    1.894271] stm32-dwmac 5800a000.ethernet: IRQ eth_wake_irq not found
> <6>[    1.899694] stm32-dwmac 5800a000.ethernet: IRQ eth_lpi not found
> <6>[    1.905849] stm32-dwmac 5800a000.ethernet: IRQ sfty not found
> <3>[    1.912304] stm32-dwmac 5800a000.ethernet: Unable to parse OF data
> <3>[    1.918393] stm32-dwmac 5800a000.ethernet: probe with driver stm32-dwmac failed with error -75
>
> which looks a bit odd given the commit contents but I didn't look at the
> driver code at all.
>
> Full boot log here:
>
>     https://lava.sirena.org.uk/scheduler/job/467150
>
> A working equivalent is here:
>
>     https://lava.sirena.org.uk/scheduler/job/466518
>
> A bisection identified this commit as being responsible, log below:
>
> git bisect start
> # status: waiting for both good and bad commits
> # bad: [0fc4bfab2cd45f9acb86c4f04b5191e114e901ed] Add linux-next specific files for 20240625
> git bisect bad 0fc4bfab2cd45f9acb86c4f04b5191e114e901ed
> # status: waiting for good commit(s), bad commit known
> # good: [3d9217c41c07b72af3a5c147cb82c75f757f4200] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
> git bisect good 3d9217c41c07b72af3a5c147cb82c75f757f4200
> # bad: [5699faecf4e2347f81eea62db0455feb4d794537] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> git bisect bad 5699faecf4e2347f81eea62db0455feb4d794537
> # good: [ba73da675606373565868962ad8c615f175662ed] Merge branch 'fs-next' of linux-next
> git bisect good ba73da675606373565868962ad8c615f175662ed
> # bad: [7e7c714a36a5b10e391168e7e8145060e041ea12] Merge branch 'af_unix-remove-spin_lock_nested-and-convert-to-lock_cmp_fn'
> git bisect bad 7e7c714a36a5b10e391168e7e8145060e041ea12
> # good: [93d4e8bb3f137e8037a65ea96f175f81c25c50e5] Merge tag 'wireless-next-2024-06-07' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
> git bisect good 93d4e8bb3f137e8037a65ea96f175f81c25c50e5
> # bad: [4314175af49668ab20c0d60d7d7657986e1d0c7c] Merge branch 'net-smc-IPPROTO_SMC'
> git bisect bad 4314175af49668ab20c0d60d7d7657986e1d0c7c
> # good: [811efc06e5f30a57030451b2d1998aa81273baf8] net/tcp: Move tcp_inbound_hash() from headers
> git bisect good 811efc06e5f30a57030451b2d1998aa81273baf8
> # good: [5f703ce5c981ee02c00e210d5b155bbbfbf11263] net: hsr: Send supervisory frames to HSR network with ProxyNodeTable data
> git bisect good 5f703ce5c981ee02c00e210d5b155bbbfbf11263
> # bad: [6c3282a6b296385bee2c383442c39f507b0d51dd] net: stmmac: add select_pcs() platform method
> git bisect bad 6c3282a6b296385bee2c383442c39f507b0d51dd
> # bad: [404dbd26322f50c8123bf5bff9a409356889035f] net: qrtr: ns: Ignore ENODEV failures in ns
> git bisect bad 404dbd26322f50c8123bf5bff9a409356889035f
> # bad: [c60a54b52026bd2c9a88ae00f2aac7a67fed8e38] net: stmmac: dwmac-stm32: Clean up the debug prints
> git bisect bad c60a54b52026bd2c9a88ae00f2aac7a67fed8e38
> # bad: [582ac134963e2d5cf6c45db027e156fcfb7f7678] net: stmmac: dwmac-stm32: Separate out external clock rate validation
> git bisect bad 582ac134963e2d5cf6c45db027e156fcfb7f7678
> # good: [8a9044e5169bab7a8edadb4ceb748391657f0d7f] dt-bindings: net: add STM32MP13 compatible in documentation for stm32
> git bisect good 8a9044e5169bab7a8edadb4ceb748391657f0d7f
> # first bad commit: [582ac134963e2d5cf6c45db027e156fcfb7f7678] net: stmmac: dwmac-stm32: Separate out external clock rate validation
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index c92dfc4ecf570..2fd2620ebed69 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -157,25 +157,54 @@  static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat, bool resume)
 	return stm32_dwmac_clk_enable(dwmac, resume);
 }
 
+static int stm32mp1_validate_ethck_rate(struct plat_stmmacenet_data *plat_dat)
+{
+	struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
+	const u32 clk_rate = clk_get_rate(dwmac->clk_eth_ck);
+
+	switch (plat_dat->mac_interface) {
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_GMII:
+		if (clk_rate == ETH_CK_F_25M)
+			return 0;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		if (clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M)
+			return 0;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		if (clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M)
+			return 0;
+		break;
+	default:
+		break;
+	}
+
+	dev_err(dwmac->dev, "Mode %s does not match eth-ck frequency %d Hz",
+		phy_modes(plat_dat->mac_interface), clk_rate);
+	return -EINVAL;
+}
+
 static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
 {
 	struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
-	u32 reg = dwmac->mode_reg, clk_rate;
-	int val;
+	u32 reg = dwmac->mode_reg;
+	int val, ret;
 
-	clk_rate = clk_get_rate(dwmac->clk_eth_ck);
 	dwmac->enable_eth_ck = false;
 	switch (plat_dat->mac_interface) {
 	case PHY_INTERFACE_MODE_MII:
-		if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk)
+		if (dwmac->ext_phyclk)
 			dwmac->enable_eth_ck = true;
 		val = SYSCFG_PMCR_ETH_SEL_MII;
 		pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n");
 		break;
 	case PHY_INTERFACE_MODE_GMII:
 		val = SYSCFG_PMCR_ETH_SEL_GMII;
-		if (clk_rate == ETH_CK_F_25M &&
-		    (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
+		if (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk) {
 			dwmac->enable_eth_ck = true;
 			val |= SYSCFG_PMCR_ETH_CLK_SEL;
 		}
@@ -183,8 +212,7 @@  static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
 		break;
 	case PHY_INTERFACE_MODE_RMII:
 		val = SYSCFG_PMCR_ETH_SEL_RMII;
-		if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
-		    (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {
+		if (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk) {
 			dwmac->enable_eth_ck = true;
 			val |= SYSCFG_PMCR_ETH_REF_CLK_SEL;
 		}
@@ -195,8 +223,7 @@  static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		val = SYSCFG_PMCR_ETH_SEL_RGMII;
-		if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) &&
-		    (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
+		if (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk) {
 			dwmac->enable_eth_ck = true;
 			val |= SYSCFG_PMCR_ETH_CLK_SEL;
 		}
@@ -209,6 +236,10 @@  static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
 		return -EINVAL;
 	}
 
+	ret = stm32mp1_validate_ethck_rate(plat_dat);
+	if (ret)
+		return ret;
+
 	/* Need to update PMCCLRR (clear register) */
 	regmap_write(dwmac->regmap, reg + SYSCFG_PMCCLRR_OFFSET,
 		     dwmac->ops->syscfg_eth_mask);