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

[Sai Krishna Gajula]

> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Sunday, April 28, 2024 3:21 AM
> To: netdev at vger.kernel.org
> Cc: Marek Vasut <marex at denx.de>; David S. Miller <davem at davemloft.net>;
> Alexandre Torgue <alexandre.torgue at foss.st.com>; Christophe Roullier
> <christophe.roullier at foss.st.com>; Eric Dumazet <edumazet at google.com>;
> Jakub Kicinski <kuba at kernel.org>; Jose Abreu <joabreu at synopsys.com>;
> Maxime Coquelin <mcoquelin.stm32 at gmail.com>; Paolo Abeni
> <pabeni at redhat.com>; linux-arm-kernel at lists.infradead.org; linux-
> stm32 at st-md-mailman.stormreply.com
> Subject: [net-next,RFC,PATCH 1/5] net: stmmac: dwmac-stm32:
> Separate out external clock rate validation
> 
> 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 at denx.de>
> ---
> Cc: "David S. Miller" <davem at davemloft.net>
> Cc: Alexandre Torgue <alexandre.torgue at foss.st.com>
> Cc: Christophe Roullier <christophe.roullier at foss.st.com>
> Cc: Eric Dumazet <edumazet at google.com>
> Cc: Jakub Kicinski <kuba at kernel.org>
> Cc: Jose Abreu <joabreu at synopsys.com>
> Cc: Maxime Coquelin <mcoquelin.stm32 at gmail.com>
> Cc: Paolo Abeni <pabeni at redhat.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-stm32 at st-md-mailman.stormreply.com
> Cc: netdev at vger.kernel.org
> ---
>  .../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);

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;

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