| Message ID | 20240427215113.57548-1-marex@denx.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RFC,1/5] net: stmmac: dwmac-stm32: Separate out external clock rate validation | expand |
> -----Original Message----- > From: Marek Vasut <marex@denx.de> > Sent: Sunday, April 28, 2024 3:21 AM > To: netdev@vger.kernel.org > Cc: Marek Vasut <marex@denx.de>; David S. Miller <davem@davemloft.net>; > Alexandre Torgue <alexandre.torgue@foss.st.com>; Christophe Roullier > <christophe.roullier@foss.st.com>; Eric Dumazet <edumazet@google.com>; > Jakub Kicinski <kuba@kernel.org>; Jose Abreu <joabreu@synopsys.com>; > Maxime Coquelin <mcoquelin.stm32@gmail.com>; Paolo Abeni > <pabeni@redhat.com>; linux-arm-kernel@lists.infradead.org; linux- > stm32@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@denx.de> > --- > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Christophe Roullier <christophe.roullier@foss.st.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Jose Abreu <joabreu@synopsys.com> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > Cc: netdev@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 >
On 4/29/24 9:19 AM, Sai Krishna Gajula wrote: > >> -----Original Message----- >> From: Marek Vasut <marex@denx.de> >> Sent: Sunday, April 28, 2024 3:21 AM >> To: netdev@vger.kernel.org >> Cc: Marek Vasut <marex@denx.de>; David S. Miller <davem@davemloft.net>; >> Alexandre Torgue <alexandre.torgue@foss.st.com>; Christophe Roullier >> <christophe.roullier@foss.st.com>; Eric Dumazet <edumazet@google.com>; >> Jakub Kicinski <kuba@kernel.org>; Jose Abreu <joabreu@synopsys.com>; >> Maxime Coquelin <mcoquelin.stm32@gmail.com>; Paolo Abeni >> <pabeni@redhat.com>; linux-arm-kernel@lists.infradead.org; linux- >> stm32@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@denx.de> >> --- >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >> Cc: Christophe Roullier <christophe.roullier@foss.st.com> >> Cc: Eric Dumazet <edumazet@google.com> >> Cc: Jakub Kicinski <kuba@kernel.org> >> Cc: Jose Abreu <joabreu@synopsys.com> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Paolo Abeni <pabeni@redhat.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> Cc: netdev@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.. I hope those would be addressed in v4 of: [PATCH v3 02/11] net: stmmac: dwmac-stm32: Separate out external clock rate validation Thanks !
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);
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> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Christophe Roullier <christophe.roullier@foss.st.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Jose Abreu <joabreu@synopsys.com> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Paolo Abeni <pabeni@redhat.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com Cc: netdev@vger.kernel.org --- .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 54 +++++++++++++++---- 1 file changed, 44 insertions(+), 10 deletions(-)