| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3137049f-eac8-4522-ad2e-b2b0d3537239@foss.st.com>
Date: Mon, 13 May 2024 17:11:52 +0200
From: Christophe ROULLIER <christophe.roullier@...s.st.com>
To: Marek Vasut <marex@...x.de>, "David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo
Abeni <pabeni@...hat.com>, Rob Herring <robh+dt@...nel.org>,
Krzysztof
Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley
<conor+dt@...nel.org>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre
Torgue <alexandre.torgue@...s.st.com>,
Richard Cochran
<richardcochran@...il.com>,
Jose Abreu <joabreu@...opsys.com>,
Liam Girdwood
<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>
CC: <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 05/11] net: stmmac: dwmac-stm32: update config
management for phy wo cristal
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@...s.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 ?
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.
>
> 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 :-)
>
>> 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
>
> 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.
>
>> 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;
>> }
Powered by blists - more mailing lists