From: Marek Vasut <marex@denx.de>
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
Subject: Re: [PATCH v2 05/11] net: stmmac: dwmac-stm32: update config management for phy wo cristal
Date: Fri, 26 Apr 2024 17:37:51 +0200 [thread overview]
Message-ID: <b790f34e-8bfb-44f6-869d-798508008483@denx.de> (raw)
In-Reply-To: <20240426125707.585269-6-christophe.roullier@foss.st.com>
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;
> }
>
next prev parent reply other threads:[~2024-04-26 16:00 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 12:56 [PATCH v2 00/11] Series to deliver Ethernets for STM32MP13 Christophe Roullier
2024-04-26 12:56 ` [PATCH v2 01/11] dt-bindings: net: add STM32MP13 compatible in documentation for stm32 Christophe Roullier
2024-04-26 14:46 ` Marek Vasut
2024-04-26 19:16 ` Rob Herring
2024-04-26 12:56 ` [PATCH v2 02/11] dt-bindings: net: add phy-supply property " Christophe Roullier
2024-04-26 14:47 ` Marek Vasut
2024-05-13 11:45 ` Christophe ROULLIER
2024-05-13 14:16 ` Marek Vasut
2024-04-26 15:30 ` Rob Herring
2024-05-13 14:06 ` Christophe ROULLIER
2024-05-13 14:20 ` Marek Vasut
2024-04-26 12:56 ` [PATCH v2 03/11] net: stmmac: dwmac-stm32: rework glue to simplify management Christophe Roullier
2024-04-26 14:53 ` Marek Vasut
2024-05-13 12:48 ` Christophe ROULLIER
2024-05-13 14:23 ` Marek Vasut
2024-04-26 12:57 ` [PATCH v2 04/11] net: stmmac: dwmac-stm32: add management of stm32mp13 Christophe Roullier
2024-04-26 12:57 ` [PATCH v2 05/11] net: stmmac: dwmac-stm32: update config management for phy wo cristal Christophe Roullier
2024-04-26 15:37 ` Marek Vasut [this message]
2024-05-13 15:11 ` Christophe ROULLIER
2024-05-13 22:33 ` Marek Vasut
2024-04-26 12:57 ` [PATCH v2 06/11] net: stmmac: dwmac-stm32: clean the way to manage wol irqwake Christophe Roullier
2024-04-26 15:40 ` Marek Vasut
2024-05-13 15:14 ` Christophe ROULLIER
2024-04-26 12:57 ` [PATCH v2 07/11] net: stmmac: dwmac-stm32: support the phy-supply regulator binding Christophe Roullier
2024-04-26 15:48 ` Marek Vasut
2024-04-26 12:57 ` [PATCH v2 08/11] ARM: dts: stm32: add ethernet1 and ethernet2 support on stm32mp13 Christophe Roullier
2024-04-26 12:57 ` [PATCH v2 09/11] ARM: dts: stm32: add ethernet1/2 RMII pins for STM32MP13F-DK board Christophe Roullier
2024-04-26 12:57 ` [PATCH v2 10/11] ARM: dts: stm32: add ethernet1 and ethernet2 for STM32MP135F-DK board Christophe Roullier
2024-04-26 15:44 ` Marek Vasut
2024-05-13 16:01 ` Alexandre TORGUE
2024-05-16 0:23 ` Marek Vasut
2024-05-16 7:58 ` Alexandre TORGUE
2024-05-16 12:22 ` Andrew Lunn
2024-05-17 7:30 ` Alexandre TORGUE
2024-04-26 12:57 ` [PATCH v2 11/11] ARM: multi_v7_defconfig: Add MCP23S08 pinctrl support Christophe Roullier
2024-04-26 14:22 ` [PATCH v2 00/11] Series to deliver Ethernets for STM32MP13 Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b790f34e-8bfb-44f6-869d-798508008483@denx.de \
--to=marex@denx.de \
--cc=alexandre.torgue@foss.st.com \
--cc=broonie@kernel.org \
--cc=christophe.roullier@foss.st.com \
--cc=conor+dt@kernel.org \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=joabreu@synopsys.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kuba@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mcoquelin.stm32@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=richardcochran@gmail.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).