On 5/13/24 5:11 PM, Christophe ROULLIER wrote:
On 4/26/24 17:37, Marek Vasut wrote:
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 ?
Not agree, there is also "Normal" case MII (MII with quartz/cristal)
(first column in the table above), so need to keep this test to check
clk_rate 25MHz.
What other rate is supported in the MII mode ? Isn't the rate always 25
MHz for MII , no matter whether it is generated by RCC or external Xtal ?
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.
Right
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;
Not for case "Normal" MII :-)
What happens if that external clock source is not an xtal, but an
oscillator with its own driver described in DT, and enabled e.g. using
GPIO (that's compatible "gpio-gate-clock" for that oscillator) . Then
you do need to enable those clock even in the "normal" (with external
clock source instead of RCC clock source) MII case.
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).
You are right, but in STM32MP15/MP13 reference manual it is write that
we need to update SYSCFG (SYSCFG_PMCSETR) register only in "Ethernet
50MHz RMII clock selection":
Bit 17 ETH_REF_CLK_SEL: Ethernet 50MHz RMII clock selection.
Set by software.
0: Writing '0' has no effect, reading '0' means External clock is
used. Need selection of AFMux. Could be used with all PHY
1: Writing '1' set this bit, reading '1' means Internal clock
ETH_CLK1 from RCC is used regardless AFMux. Could be used only with RMII
PHY
Look at this:
"
RM0436 Rev 6 Reset and clock control (RCC)
Clock distribution for Ethernet (ETH)
Figure 83. Peripheral clock distribution for Ethernet
Page 575
"
See the mux at bottom left side in the PKCS group.
I suspect that no matter whether the clock are 25 MHz or 50 MHz, they
enter this mux, the mux selects the clock source from either RCC or from
external oscillator, and therefore the original code was correct.
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?
As explained previously, need to keep check of clock frequency in this
test.
I sent 5 patches which split this code up, you were on CC:
[net-next,RFC,PATCH 1/5] net: stmmac: dwmac-stm32: Separate out external
clock rate validation
Maybe you can apply those, fix them up as needed, and then add this
series on top ? I think it would simplify this code a lot, since that
series splits up the clock rate validation / PMCR configuration /
external-internal clock selection into separate steps. What do you think ?
