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@xxxxxxxxxxx>
---
.../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;
}
