Received-SPF: pass (google.com: domain of linux-kernel+bounces-160348-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <b790f34e-8bfb-44f6-869d-798508008483@denx.de>
Date: Fri, 26 Apr 2024 17:37:51 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 05/11] net: stmmac: dwmac-stm32: update config
 management for phy wo cristal
To: Christophe Roullier <christophe.roullier@foss.st.com>,
 "David S . Miller" <davem@davemloft.net>, Eric Dumazet
 <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
 Paolo Abeni <pabeni@redhat.com>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Maxime Coquelin <mcoquelin.stm32@gmail.com>,
 Alexandre Torgue <alexandre.torgue@foss.st.com>,
 Richard Cochran <richardcochran@gmail.com>, Jose Abreu
 <joabreu@synopsys.com>, Liam Girdwood <lgirdwood@gmail.com>,
 Mark Brown <broonie@kernel.org>
Cc: netdev@vger.kernel.org, devicetree@vger.kernel.org,
 linux-stm32@st-md-mailman.stormreply.com,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20240426125707.585269-1-christophe.roullier@foss.st.com>
 <20240426125707.585269-6-christophe.roullier@foss.st.com>
Content-Language: en-US
From: Marek Vasut <marex@denx.de>
In-Reply-To: <20240426125707.585269-6-christophe.roullier@foss.st.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 4/26/24 2:57 PM, Christophe Roullier wrote:
> Some cleaning because some Ethernet PHY configs do not need to add
> st,ext-phyclk property.
> Change print info message "No phy clock provided" only when debug.
> 
> Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 27 ++++++++++---------
>   1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> index 7529a8d15492..e648c4e790a7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> @@ -55,17 +55,17 @@
>    *|         |        |      25MHz    |        50MHz       |                  |
>    * ---------------------------------------------------------------------------
>    *|  MII    |	 -   |     eth-ck    |	      n/a	  |	  n/a        |
> - *|         |        | st,ext-phyclk |                    |		     |
> + *|         |        |	             |                    |		     |
>    * ---------------------------------------------------------------------------
>    *|  GMII   |	 -   |     eth-ck    |	      n/a	  |	  n/a        |
> - *|         |        | st,ext-phyclk |                    |		     |
> + *|         |        |               |                    |		     |
>    * ---------------------------------------------------------------------------
>    *| RGMII   |	 -   |     eth-ck    |	      n/a	  |      eth-ck      |
> - *|         |        | st,ext-phyclk |                    | st,eth-clk-sel or|
> + *|         |        |               |                    | st,eth-clk-sel or|
>    *|         |        |               |                    | st,ext-phyclk    |
>    * ---------------------------------------------------------------------------
>    *| RMII    |	 -   |     eth-ck    |	    eth-ck        |	  n/a        |
> - *|         |        | st,ext-phyclk | st,eth-ref-clk-sel |		     |
> + *|         |        |               | st,eth-ref-clk-sel |		     |
>    *|         |        |               | or st,ext-phyclk   |		     |
>    * ---------------------------------------------------------------------------
>    *
> @@ -174,23 +174,22 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
>   	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 (clk_rate == ETH_CK_F_25M)

I see two problems here.

First, according to the table above, in MII mode, clk_rate cannot be 
anything else but 25 MHz, so the (clk_rate == ETH_CK_F_25M) condition is 
always true. Why not drop that condition ?

The "dwmac->ext_phyclk" means "Ethernet PHY have no crystal", which 
means the clock are provided by the STM32 RCC clock IP instead, which 
means if the dwmac->ext_phyclk is true, dwmac->enable_eth_ck should be 
set to true, because dwmac->enable_eth_ck controls the enablement of 
these STM32 clock IP generated clock.

Second, as far as I understand it, there is no way to operate this IP 
with external clock in MII mode, so this section should always be only:

dwmac->enable_eth_ck = true;

>   			dwmac->enable_eth_ck = true;
>   		val = dwmac->ops->pmcsetr.eth1_selmii;
>   		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 (clk_rate == ETH_CK_F_25M)
>   			dwmac->enable_eth_ck = true;
> -			val |= dwmac->ops->pmcsetr.eth1_clk_sel;
> -		}
>   		pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n");
>   		break;
>   	case PHY_INTERFACE_MODE_RMII:
>   		val = dwmac->ops->pmcsetr.eth1_sel_rmii | dwmac->ops->pmcsetr.eth2_sel_rmii;
> -		if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
> +		if (clk_rate == ETH_CK_F_25M)
> +			dwmac->enable_eth_ck = true;
> +		if (clk_rate == ETH_CK_F_50M &&
>   		    (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {

This doesn't seem to be equivalent change to the previous code . Here, 
if the clock frequency is 25 MHz, the clock are unconditionally enabled. 
Before, the code enabled the clock only if clock frequency was 25 MHz 
AND one of the "dwmac->eth_ref_clk_sel_reg" or "dwmac->ext_phyclk" was 
set (i.e. clock provided by SoC RCC clock IP).

I think it might make this code easier if you drop all of the frequency 
test conditionals, which aren't really all that useful, and only enable 
the clock if either dwmac->ext_phyclk / dwmac->eth_clk_sel_reg / 
dwmac->eth_ref_clk_sel_reg is set , because effectively what this entire 
convoluted code is implementing is "if (clock supplied by clock IP i.e. 
RCC) enable the clock()" *, right ?

* And it is also toggling the right clock mux bit in PMCSETR.

So, for MII this would be plain:
dwmac->enable_eth_ck = true;

For GMII/RGMII this would be:
if (dwmac->ext_phyclk || dwmac->eth_clk_sel_reg)
   dwmac->enable_eth_ck = true;

For RMII this would be:
if (dwmac->ext_phyclk || dwmac->eth_ref_clk_sel_reg)
   dwmac->enable_eth_ck = true;

Maybe the clock frequency validation can be retained, but done separately?

>   			dwmac->enable_eth_ck = true;
>   			val |= dwmac->ops->pmcsetr.eth1_ref_clk_sel;
> @@ -203,7 +202,9 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
>   	case PHY_INTERFACE_MODE_RGMII_RXID:
>   	case PHY_INTERFACE_MODE_RGMII_TXID:
>   		val = dwmac->ops->pmcsetr.eth1_sel_rgmii | dwmac->ops->pmcsetr.eth2_sel_rgmii;
> -		if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) &&
> +		if (clk_rate == ETH_CK_F_25M)
> +			dwmac->enable_eth_ck = true;
> +		if (clk_rate == ETH_CK_F_125M &&
>   		    (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
>   			dwmac->enable_eth_ck = true;
>   			val |= dwmac->ops->pmcsetr.eth1_clk_sel;
> @@ -219,7 +220,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
>   	}
>   
>   	/* Need to update PMCCLRR (clear register) */
> -	regmap_write(dwmac->regmap, reg + dwmac->ops->syscfg_clr_off,
> +	regmap_write(dwmac->regmap, dwmac->ops->syscfg_clr_off,
>   		     dwmac->mode_mask);
>   
>   	/* Update PMCSETR (set register) */
> @@ -328,7 +329,7 @@ static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
>   	/*  Get ETH_CLK clocks */
>   	dwmac->clk_eth_ck = devm_clk_get(dev, "eth-ck");
>   	if (IS_ERR(dwmac->clk_eth_ck)) {
> -		dev_info(dev, "No phy clock provided...\n");
> +		dev_dbg(dev, "No phy clock provided...\n");
>   		dwmac->clk_eth_ck = NULL;
>   	}
>