Re: [PATCH v2 05/11] net: stmmac: dwmac-stm32: update config management for phy wo cristal

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

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;
      }




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux