diff mbox series

[v3,02/11] net: stmmac: dwmac-stm32: Separate out external clock rate validation

Message ID 20240603092757.71902-3-christophe.roullier@foss.st.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Series to deliver Ethernet for STM32MP13 | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Christophe Roullier June 3, 2024, 9:27 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>
---
 .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 54 +++++++++++++++----
 1 file changed, 44 insertions(+), 10 deletions(-)

Comments

Russell King (Oracle) June 3, 2024, 9:38 a.m. UTC | #1 
On Mon, Jun 03, 2024 at 11:27:48AM +0200, Christophe Roullier wrote:
> +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) {

Should these be phy_interface? Does this clock depend on the interface
mode used with the PHY?
Marek Vasut June 3, 2024, 2:27 p.m. UTC | #2 
On 6/3/24 11:27 AM, 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.
> 
> 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>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 54 +++++++++++++++----
>   1 file changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> index c92dfc4ecf570..43340a5573c64 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> @@ -157,25 +157,57 @@ 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);

 From Sai in
Re: [net-next,RFC,PATCH 1/5] net: stmmac: dwmac-stm32: Separate out 
external clock rate validation

Please check reverse x-mass tree is followed for these variables, if 
possible.

> +	switch (plat_dat->mac_interface) {
> +	case PHY_INTERFACE_MODE_MII:
> +		if (clk_rate == ETH_CK_F_25M)
> +			return 0;
> +		break;
> +	case PHY_INTERFACE_MODE_GMII:
> +		if (clk_rate == ETH_CK_F_25M)
> +			return 0;
> +		break;

 From Sai in
Re: [net-next,RFC,PATCH 1/5] net: stmmac: dwmac-stm32: Separate out 
external clock rate validation

Please check, whether we can combine the two cases..

> +	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;
> +}

[...]
Christophe Roullier June 4, 2024, 9:13 a.m. UTC | #3 
On 6/3/24 11:38, Russell King (Oracle) wrote:
> On Mon, Jun 03, 2024 at 11:27:48AM +0200, Christophe Roullier wrote:
>> +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) {
> Should these be phy_interface?

Hi,

The code is validating the clock frequency of clock that are INPUT into 
the MAC. These clock can be generated by either the PHY, or Xtal, or 
some other source, but they are still the clock which are INPUT into the 
MAC. Therefore I believe mac_interface is correct here.

> Does this clock depend on the interface
> mode used with the PHY?
>
I don't think the clock depend on the PHY mode. Look at 
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c :
"
458         plat->phy_interface = phy_mode;
459         rc = stmmac_of_get_mac_mode(np);
460         plat->mac_interface = rc < 0 ? plat->phy_interface : rc;
"
and this comment:
"
382 /**
383  * stmmac_of_get_mac_mode - retrieves the interface of the MAC
384  * @np: - device-tree node
385  * Description:
386  * Similar to `of_get_phy_mode()`, this function will retrieve (from
387  * the device-tree) the interface mode on the MAC side. This assumes
388  * that there is mode converter in-between the MAC & PHY
389  * (e.g. GMII-to-RGMII).
390  */
391 static int stmmac_of_get_mac_mode(struct device_node *np)
"
I think in the unlikely case that you would have a mode converter 
between the MAC and PHY, the clock that are validated by this code would 
still be the clock that are INPUT into the MAC, i.e. clock on the MAC 
side of the mode converter and NOT on the PHY side , and those clock 
would not depend on the PHY mode, they would depend on the MAC mode .
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..43340a5573c64 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -157,25 +157,57 @@  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:
+		if (clk_rate == ETH_CK_F_25M)
+			return 0;
+		break;
+	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 +215,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 +226,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 +239,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);