[PATCH v4 00/16] Add support for Synopsis DWMAC IP on NXP Automotive SoCs S32G2xx/S32G3xx/S32R45

[PATCH v4 00/16] Add support for Synopsis DWMAC IP on NXP Automotive SoCs S32G2xx/S32G3xx/S32R45

[Jan Petrous via B4 Relay]

The SoC series S32G2xx and S32G3xx feature one DWMAC instance,
the SoC S32R45 has two instances. The devices can use RGMII/RMII/MII
interface over Pinctrl device or the output can be routed
to the embedded SerDes for SGMII connectivity.

The provided stmmac glue code implements only basic functionality,
interface support is restricted to RGMII only. More, including
SGMII/SerDes support will come later.

This patchset adds stmmac glue driver based on downstream NXP git [0].

[0] https://github.com/nxp-auto-linux/linux

v4:
- fixed empty commit messages for rgmi_clock() helper patches
- fixed yaml path in MAINTAINERS
- switched to platform_driver::remove() as suggested Uwe
- yaml: returned back all compatibility sting values
- added better commit description for rgmii_clock() helper
- Link to v3: https://lore.kernel.org/r/20241013-upstream_s32cc_gmac-v3-0-d84b5a67b930@oss.nxp.com

v3:
- switched to b4 WoW to overcome threading issue with b4
- extracted the hunk with the typo fix from v2 patch#1 to separate patch
  as Jacob suggested
- removed dead code for RMII/MII support, which will be added alter
- used new rgmii_clock() helper in other stmmac:dwmac glue drivers
- yaml: compatible strings compressed to simple one "nxp,s32-dwmac",
  removed duplicated required properties, already defined in snps,dwmac,
  fixed example

v2:
- send to wider audience as first version missed many maintainers
- created rgmi_clk() helper as Russell suggested (see patch#4)
- address Andrew's, Russell's, Serge's and Simon's comments

Message-ID: <AM9PR04MB85066576AD6848E2402DA354E2832@AM9PR04MB8506.eurprd04.prod.outlook.com>

Cc: 

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
Jan Petrous (OSS) (16):
      net: driver: stmmac: Fix CSR divider comment
      net: driver: stmmac: Extend CSR calc support
      net: stmmac: Fix clock rate variables size
      net: phy: Add helper for mapping RGMII link speed to clock rate
      net: dwmac-dwc-qos-eth: Use helper rgmii_clock
      net: dwmac-imx: Use helper rgmii_clock
      net: dwmac-intel-plat: Use helper rgmii_clock
      net: dwmac-rk: Use helper rgmii_clock
      net: dwmac-starfive: Use helper rgmii_clock
      net: macb: Use helper rgmii_clock
      net: xgene_enet: Use helper rgmii_clock
      net: dwmac-sti: Use helper rgmii_clock
      dt-bindings: net: Add DT bindings for DWMAC on NXP S32G/R SoCs
      net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver
      MAINTAINERS: Add Jan Petrous as the NXP S32G/R DWMAC driver maintainer
      net: stmmac: dwmac-s32: Read PTP clock rate when ready

 .../devicetree/bindings/net/nxp,s32-dwmac.yaml     |  98 +++++++++
 .../devicetree/bindings/net/snps,dwmac.yaml        |   3 +
 MAINTAINERS                                        |   7 +
 drivers/net/ethernet/apm/xgene/xgene_enet_hw.c     |  16 +-
 drivers/net/ethernet/cadence/macb_main.c           |  14 +-
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |  12 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
 drivers/net/ethernet/stmicro/stmmac/common.h       |   2 +
 .../ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c    |  11 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c    |  15 +-
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c |  20 +-
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |   2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c     |  30 +--
 drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c    | 239 +++++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/dwmac-starfive.c   |  19 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c    |  18 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |   6 +-
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   2 +-
 include/linux/phy.h                                |  23 ++
 include/linux/stmmac.h                             |  10 +-
 21 files changed, 431 insertions(+), 119 deletions(-)
---
base-commit: 2f87d0916ce0d2925cedbc9e8f5d6291ba2ac7b2
change-id: 20240923-upstream_s32cc_gmac-6891eb75b126

Best regards,
-- 
Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

[PATCH v4 01/16] net: driver: stmmac: Fix CSR divider comment

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

The comment in declaration of STMMAC_CSR_250_300M
incorrectly describes the constant as '/* MDC = clk_scr_i/122 */'
but the DWC Ether QOS Handbook version 5.20a says it is
CSR clock/124.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
 include/linux/stmmac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index d79ff252cfdc..75cbfb576358 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -33,7 +33,7 @@
 #define	STMMAC_CSR_20_35M	0x2	/* MDC = clk_scr_i/16 */
 #define	STMMAC_CSR_35_60M	0x3	/* MDC = clk_scr_i/26 */
 #define	STMMAC_CSR_150_250M	0x4	/* MDC = clk_scr_i/102 */
-#define	STMMAC_CSR_250_300M	0x5	/* MDC = clk_scr_i/122 */
+#define	STMMAC_CSR_250_300M	0x5	/* MDC = clk_scr_i/124 */
 
 /* MTL algorithms identifiers */
 #define MTL_TX_ALGORITHM_WRR	0x0

-- 
2.46.0

[PATCH v4 02/16] net: driver: stmmac: Extend CSR calc support

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Add support for CSR clock range up to 800 MHz.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h      | 2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
 include/linux/stmmac.h                            | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 684489156dce..e364cf99d1ff 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -257,6 +257,8 @@ struct stmmac_safety_stats {
 #define CSR_F_150M	150000000
 #define CSR_F_250M	250000000
 #define CSR_F_300M	300000000
+#define CSR_F_500M	500000000
+#define CSR_F_800M	800000000
 
 #define	MAC_CSR_H_FRQ_MASK	0x20
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d3895d7eecfc..f9cab62cfde9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -324,6 +324,10 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
 			priv->clk_csr = STMMAC_CSR_150_250M;
 		else if ((clk_rate >= CSR_F_250M) && (clk_rate <= CSR_F_300M))
 			priv->clk_csr = STMMAC_CSR_250_300M;
+		else if ((clk_rate >= CSR_F_300M) && (clk_rate < CSR_F_500M))
+			priv->clk_csr = STMMAC_CSR_300_500M;
+		else if ((clk_rate >= CSR_F_500M) && (clk_rate < CSR_F_800M))
+			priv->clk_csr = STMMAC_CSR_500_800M;
 	}
 
 	if (priv->plat->flags & STMMAC_FLAG_HAS_SUN8I) {
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 75cbfb576358..865d0fe26f98 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -34,6 +34,8 @@
 #define	STMMAC_CSR_35_60M	0x3	/* MDC = clk_scr_i/26 */
 #define	STMMAC_CSR_150_250M	0x4	/* MDC = clk_scr_i/102 */
 #define	STMMAC_CSR_250_300M	0x5	/* MDC = clk_scr_i/124 */
+#define	STMMAC_CSR_300_500M	0x6	/* MDC = clk_scr_i/204 */
+#define	STMMAC_CSR_500_800M	0x7	/* MDC = clk_scr_i/324 */
 
 /* MTL algorithms identifiers */
 #define MTL_TX_ALGORITHM_WRR	0x0

-- 
2.46.0

[PATCH v4 03/16] net: stmmac: Fix clock rate variables size

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

The clock API clk_get_rate() returns unsigned long value.
Expand affected members of stmmac platform data and
convert the stmmac_clk_csr_set() and dwmac4_core_init() methods
to defining the unsigned long clk_rate local variables.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 2 +-
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c       | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c       | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c   | 2 +-
 include/linux/stmmac.h                                  | 6 +++---
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 901a3c1959fa..2a5b38723635 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -777,7 +777,7 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
 		netdev_err(priv->dev, "Failed to max out clk_ptp_ref: %d\n", err);
 	plat_dat->clk_ptp_rate = clk_get_rate(plat_dat->clk_ptp_ref);
 
-	netdev_dbg(priv->dev, "PTP rate %d\n", plat_dat->clk_ptp_rate);
+	netdev_dbg(priv->dev, "PTP rate %lu\n", plat_dat->clk_ptp_rate);
 }
 
 static int qcom_ethqos_probe(struct platform_device *pdev)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index e65a65666cc1..ac0b02783889 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -26,7 +26,7 @@ static void dwmac4_core_init(struct mac_device_info *hw,
 	struct stmmac_priv *priv = netdev_priv(dev);
 	void __iomem *ioaddr = hw->pcsr;
 	u32 value = readl(ioaddr + GMAC_CONFIG);
-	u32 clk_rate;
+	unsigned long clk_rate;
 
 	value |= GMAC_CORE_INIT;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f9cab62cfde9..dd155dd4903b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -300,7 +300,7 @@ static void stmmac_global_err(struct stmmac_priv *priv)
  */
 static void stmmac_clk_csr_set(struct stmmac_priv *priv)
 {
-	u32 clk_rate;
+	unsigned long clk_rate;
 
 	clk_rate = clk_get_rate(priv->plat->stmmac_clk);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index ad868e8d195d..b1e4df1a86a0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -639,7 +639,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 		dev_info(&pdev->dev, "PTP uses main clock\n");
 	} else {
 		plat->clk_ptp_rate = clk_get_rate(plat->clk_ptp_ref);
-		dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate);
+		dev_dbg(&pdev->dev, "PTP rate %lu\n", plat->clk_ptp_rate);
 	}
 
 	plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev,
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 865d0fe26f98..c9878a612e53 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -252,8 +252,8 @@ struct plat_stmmacenet_data {
 	struct clk *stmmac_clk;
 	struct clk *pclk;
 	struct clk *clk_ptp_ref;
-	unsigned int clk_ptp_rate;
-	unsigned int clk_ref_rate;
+	unsigned long clk_ptp_rate;
+	unsigned long clk_ref_rate;
 	unsigned int mult_fact_100ns;
 	s32 ptp_max_adj;
 	u32 cdc_error_adj;
@@ -265,7 +265,7 @@ struct plat_stmmacenet_data {
 	int mac_port_sel_speed;
 	int has_xgmac;
 	u8 vlan_fail_q;
-	unsigned int eee_usecs_rate;
+	unsigned long eee_usecs_rate;
 	struct pci_dev *pdev;
 	int int_snapshot_num;
 	int msi_mac_vec;

-- 
2.46.0

[PATCH v4 04/16] net: phy: Add helper for mapping RGMII link speed to clock rate

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

The RGMII interface supports three data rates: 10/100 Mbps
and 1 Gbps. These speeds correspond to clock frequencies
of 2.5/25 MHz and 125 MHz, respectively.

Many Ethernet drivers, including glues in stmmac, follow
a similar pattern of converting RGMII speed to clock frequency.

To simplify code, define the helper rgmii_clock(speed)
to convert connection speed to clock frequency.

Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 include/linux/phy.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index a98bc91a0cde..e28c971d7616 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -298,6 +298,29 @@ static inline const char *phy_modes(phy_interface_t interface)
 	}
 }
 
+/**
+ * rgmii_clock - map link speed to the clock rate
+ * @speed: link speed value
+ *
+ * Description: maps RGMII supported link speeds
+ * into the clock rates.
+ *
+ * Returns: clock rate or negative errno
+ */
+static inline long rgmii_clock(int speed)
+{
+	switch (speed) {
+	case SPEED_10:
+		return 2500000;
+	case SPEED_100:
+		return 25000000;
+	case SPEED_1000:
+		return 125000000;
+	default:
+		return -EINVAL;
+	}
+}
+
 #define PHY_INIT_TIMEOUT	100000
 #define PHY_FORCE_TIMEOUT	10
 

-- 
2.46.0

Re: [PATCH v4 04/16] net: phy: Add helper for mapping RGMII link speed to clock rate

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:46PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> The RGMII interface supports three data rates: 10/100 Mbps
> and 1 Gbps. These speeds correspond to clock frequencies
> of 2.5/25 MHz and 125 MHz, respectively.
> 
> Many Ethernet drivers, including glues in stmmac, follow
> a similar pattern of converting RGMII speed to clock frequency.
> 
> To simplify code, define the helper rgmii_clock(speed)
> to convert connection speed to clock frequency.
> 
> Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 05/16] net: dwmac-dwc-qos-eth: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index ec924c6c76c6..5080891c33e0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -181,24 +181,19 @@ static void dwc_qos_remove(struct platform_device *pdev)
 static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct tegra_eqos *eqos = priv;
-	unsigned long rate = 125000000;
+	long rate = 125000000;
 	bool needs_calibration = false;
 	u32 value;
 	int err;
 
 	switch (speed) {
 	case SPEED_1000:
-		needs_calibration = true;
-		rate = 125000000;
-		break;
-
 	case SPEED_100:
 		needs_calibration = true;
-		rate = 25000000;
-		break;
+		fallthrough;
 
 	case SPEED_10:
-		rate = 2500000;
+		rate = rgmii_clock(speed);
 		break;
 
 	default:

-- 
2.46.0

Re: [PATCH v4 05/16] net: dwmac-dwc-qos-eth: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:47PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v4 05/16] net: dwmac-dwc-qos-eth: Use helper rgmii_clock

[Simon Horman]

On Mon, Oct 28, 2024 at 09:24:47PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> index ec924c6c76c6..5080891c33e0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> @@ -181,24 +181,19 @@ static void dwc_qos_remove(struct platform_device *pdev)
>  static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mode)
>  {
>  	struct tegra_eqos *eqos = priv;
> -	unsigned long rate = 125000000;
> +	long rate = 125000000;
>  	bool needs_calibration = false;
>  	u32 value;
>  	int err;

Hi Jan,

As it seems that there will be another revision anyway,
please update the above so that the local variable declarations
are in reverse xmas tree order - longest line to shortest.

Likewise in s32_dwmac_probe() in the patch
"net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver".

...

Re: [PATCH v4 05/16] net: dwmac-dwc-qos-eth: Use helper rgmii_clock

[Jan Petrous]

On Tue, Nov 05, 2024 at 01:42:06PM +0000, Simon Horman wrote:
> On Mon, Oct 28, 2024 at 09:24:47PM +0100, Jan Petrous via B4 Relay wrote:
> > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> > 
> > Utilize a new helper function rgmii_clock().
> > 
> > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > index ec924c6c76c6..5080891c33e0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > @@ -181,24 +181,19 @@ static void dwc_qos_remove(struct platform_device *pdev)
> >  static void tegra_eqos_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> >  {
> >  	struct tegra_eqos *eqos = priv;
> > -	unsigned long rate = 125000000;
> > +	long rate = 125000000;
> >  	bool needs_calibration = false;
> >  	u32 value;
> >  	int err;
> 
> Hi Jan,
> 
> As it seems that there will be another revision anyway,
> please update the above so that the local variable declarations
> are in reverse xmas tree order - longest line to shortest.
> 
> Likewise in s32_dwmac_probe() in the patch
> "net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver".
> 

Hi Simon,
thanks for review, I will add those formating fixes in v5.

BR.
/Jan

[PATCH v4 06/16] net: dwmac-imx: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
index 6b65420e11b5..4147b56d40e1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
@@ -186,7 +186,7 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
 {
 	struct plat_stmmacenet_data *plat_dat;
 	struct imx_priv_data *dwmac = priv;
-	unsigned long rate;
+	long rate;
 	int err;
 
 	plat_dat = dwmac->plat_dat;
@@ -196,17 +196,8 @@ static void imx_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mod
 	    (plat_dat->mac_interface == PHY_INTERFACE_MODE_MII))
 		return;
 
-	switch (speed) {
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-	case SPEED_100:
-		rate = 25000000;
-		break;
-	case SPEED_10:
-		rate = 2500000;
-		break;
-	default:
+	rate = rgmii_clock(speed);
+	if (rate < 0) {
 		dev_err(dwmac->dev, "invalid speed %u\n", speed);
 		return;
 	}

-- 
2.46.0

Re: [PATCH v4 06/16] net: dwmac-imx: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:48PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 07/16] net: dwmac-intel-plat: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel-plat.c   | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
index d68f0c4e7835..ec62ed90a47e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c
@@ -31,27 +31,15 @@ struct intel_dwmac_data {
 static void kmb_eth_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct intel_dwmac *dwmac = priv;
-	unsigned long rate;
+	long rate;
 	int ret;
 
 	rate = clk_get_rate(dwmac->tx_clk);
 
-	switch (speed) {
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-
-	case SPEED_100:
-		rate = 25000000;
-		break;
-
-	case SPEED_10:
-		rate = 2500000;
-		break;
-
-	default:
+	rate = rgmii_clock(speed);
+	if (rate < 0) {
 		dev_err(dwmac->dev, "Invalid speed\n");
-		break;
+		return;
 	}
 
 	ret = clk_set_rate(dwmac->tx_clk, rate);

-- 
2.46.0

Re: [PATCH v4 07/16] net: dwmac-intel-plat: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:49PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 08/16] net: dwmac-rk: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 30 ++++++--------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 50073bdade46..2fd650b3bc5c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1079,20 +1079,11 @@ static void rk3568_set_gmac_speed(struct rk_priv_data *bsp_priv, int speed)
 {
 	struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk;
 	struct device *dev = &bsp_priv->pdev->dev;
-	unsigned long rate;
+	long rate;
 	int ret;
 
-	switch (speed) {
-	case 10:
-		rate = 2500000;
-		break;
-	case 100:
-		rate = 25000000;
-		break;
-	case 1000:
-		rate = 125000000;
-		break;
-	default:
+	rate = rgmii_clock(speed);
+	if (rate < 0) {
 		dev_err(dev, "unknown speed value for GMAC speed=%d", speed);
 		return;
 	}
@@ -1540,20 +1531,11 @@ static void rv1126_set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
 {
 	struct clk *clk_mac_speed = bsp_priv->clks[RK_CLK_MAC_SPEED].clk;
 	struct device *dev = &bsp_priv->pdev->dev;
-	unsigned long rate;
+	long rate;
 	int ret;
 
-	switch (speed) {
-	case 10:
-		rate = 2500000;
-		break;
-	case 100:
-		rate = 25000000;
-		break;
-	case 1000:
-		rate = 125000000;
-		break;
-	default:
+	rate = rgmii_clock(speed);
+	if (rate < 0) {
 		dev_err(dev, "unknown speed value for RGMII speed=%d", speed);
 		return;
 	}

-- 
2.46.0

Re: [PATCH v4 08/16] net: dwmac-rk: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:50PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 09/16] net: dwmac-starfive: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index 4e1076faee0c..d80461a721c1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -34,24 +34,13 @@ struct starfive_dwmac {
 static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct starfive_dwmac *dwmac = priv;
-	unsigned long rate;
+	long rate;
 	int err;
 
-	rate = clk_get_rate(dwmac->clk_tx);
-
-	switch (speed) {
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-	case SPEED_100:
-		rate = 25000000;
-		break;
-	case SPEED_10:
-		rate = 2500000;
-		break;
-	default:
+	rate = rgmii_clock(speed);
+	if (rate < 0) {
 		dev_err(dwmac->dev, "invalid speed %u\n", speed);
-		break;
+		return;
 	}
 
 	err = clk_set_rate(dwmac->clk_tx, rate);

-- 
2.46.0

Re: [PATCH v4 09/16] net: dwmac-starfive: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:51PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 10/16] net: macb: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index f06babec04a0..f1e481264511 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -530,19 +530,9 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
 	if (bp->phy_interface == PHY_INTERFACE_MODE_MII)
 		return;
 
-	switch (speed) {
-	case SPEED_10:
-		rate = 2500000;
-		break;
-	case SPEED_100:
-		rate = 25000000;
-		break;
-	case SPEED_1000:
-		rate = 125000000;
-		break;
-	default:
+	rate = rgmii_clock(speed);
+	if (rate < 0)
 		return;
-	}
 
 	rate_rounded = clk_round_rate(bp->tx_clk, rate);
 	if (rate_rounded < 0)

-- 
2.46.0

Re: [PATCH v4 10/16] net: macb: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:52PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 11/16] net: xgene_enet: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
index e641dbbea1e2..b854b6b42d77 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
@@ -421,18 +421,12 @@ static void xgene_enet_configure_clock(struct xgene_enet_pdata *pdata)
 
 	if (dev->of_node) {
 		struct clk *parent = clk_get_parent(pdata->clk);
+		long rate = rgmii_clock(pdata->phy_speed);
 
-		switch (pdata->phy_speed) {
-		case SPEED_10:
-			clk_set_rate(parent, 2500000);
-			break;
-		case SPEED_100:
-			clk_set_rate(parent, 25000000);
-			break;
-		default:
-			clk_set_rate(parent, 125000000);
-			break;
-		}
+		if (rate < 0)
+			rate = 125000000;
+
+		clk_set_rate(parent, rate);
 	}
 #ifdef CONFIG_ACPI
 	else {

-- 
2.46.0

Re: [PATCH v4 11/16] net: xgene_enet: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:53PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 12/16] net: dwmac-sti: Use helper rgmii_clock

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Utilize a new helper function rgmii_clock().

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index 4445cddc4cbe..0b02d079360d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
@@ -21,10 +21,7 @@
 
 #include "stmmac_platform.h"
 
-#define DWMAC_125MHZ	125000000
 #define DWMAC_50MHZ	50000000
-#define DWMAC_25MHZ	25000000
-#define DWMAC_2_5MHZ	2500000
 
 #define IS_PHY_IF_MODE_RGMII(iface)	(iface == PHY_INTERFACE_MODE_RGMII || \
 			iface == PHY_INTERFACE_MODE_RGMII_ID || \
@@ -140,7 +137,7 @@ static void stih4xx_fix_retime_src(void *priv, u32 spd, unsigned int mode)
 	struct sti_dwmac *dwmac = priv;
 	u32 src = dwmac->tx_retime_src;
 	u32 reg = dwmac->ctrl_reg;
-	u32 freq = 0;
+	long freq = 0;
 
 	if (dwmac->interface == PHY_INTERFACE_MODE_MII) {
 		src = TX_RETIME_SRC_TXCLK;
@@ -153,19 +150,14 @@ static void stih4xx_fix_retime_src(void *priv, u32 spd, unsigned int mode)
 		}
 	} else if (IS_PHY_IF_MODE_RGMII(dwmac->interface)) {
 		/* On GiGa clk source can be either ext or from clkgen */
-		if (spd == SPEED_1000) {
-			freq = DWMAC_125MHZ;
-		} else {
+		freq = rgmii_clock(spd);
+
+		if (spd != SPEED_1000 && freq > 0)
 			/* Switch to clkgen for these speeds */
 			src = TX_RETIME_SRC_CLKGEN;
-			if (spd == SPEED_100)
-				freq = DWMAC_25MHZ;
-			else if (spd == SPEED_10)
-				freq = DWMAC_2_5MHZ;
-		}
 	}
 
-	if (src == TX_RETIME_SRC_CLKGEN && freq)
+	if (src == TX_RETIME_SRC_CLKGEN && freq > 0)
 		clk_set_rate(dwmac->clk, freq);
 
 	regmap_update_bits(dwmac->regmap, reg, STIH4XX_RETIME_SRC_MASK,

-- 
2.46.0

Re: [PATCH v4 12/16] net: dwmac-sti: Use helper rgmii_clock

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:54PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Utilize a new helper function rgmii_clock().
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 13/16] dt-bindings: net: Add DT bindings for DWMAC on NXP S32G/R SoCs

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Add basic description for DWMAC ethernet IP on NXP S32G2xx, S32G3xx
and S32R45 automotive series SoCs.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 .../devicetree/bindings/net/nxp,s32-dwmac.yaml     | 98 ++++++++++++++++++++++
 .../devicetree/bindings/net/snps,dwmac.yaml        |  3 +
 2 files changed, 101 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
new file mode 100644
index 000000000000..b11ba3bc4c52
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2021-2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nxp,s32-dwmac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP S32G2xx/S32G3xx/S32R45 GMAC ethernet controller
+
+maintainers:
+  - Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
+
+description:
+  This device is a Synopsys DWC IP, integrated on NXP S32G/R SoCs.
+
+properties:
+  compatible:
+    enum:
+      - nxp,s32g2-dwmac
+      - nxp,s32g3-dwmac
+      - nxp,s32r-dwmac
+
+  reg:
+    items:
+      - description: Main GMAC registers
+      - description: GMAC PHY mode control register
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    const: macirq
+
+  clocks:
+    items:
+      - description: Main GMAC clock
+      - description: Transmit clock
+      - description: Receive clock
+      - description: PTP reference clock
+
+  clock-names:
+    items:
+      - const: stmmaceth
+      - const: tx
+      - const: rx
+      - const: ptp_ref
+
+required:
+  - clocks
+  - clock-names
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/phy/phy.h>
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      ethernet@4033c000 {
+        compatible = "nxp,s32g2-dwmac";
+        reg = <0x0 0x4033c000 0x0 0x2000>, /* gmac IP */
+              <0x0 0x4007c004 0x0 0x4>;    /* GMAC_0_CTRL_STS */
+        interrupt-parent = <&gic>;
+        interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "macirq";
+        snps,mtl-rx-config = <&mtl_rx_setup>;
+        snps,mtl-tx-config = <&mtl_tx_setup>;
+        clocks = <&clks 24>, <&clks 17>, <&clks 16>, <&clks 15>;
+        clock-names = "stmmaceth", "tx", "rx", "ptp_ref";
+        phy-mode = "rgmii-id";
+        phy-handle = <&phy0>;
+
+        mtl_rx_setup: rx-queues-config {
+          snps,rx-queues-to-use = <5>;
+        };
+
+        mtl_tx_setup: tx-queues-config {
+          snps,tx-queues-to-use = <5>;
+        };
+
+        mdio {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          compatible = "snps,dwmac-mdio";
+
+          phy0: ethernet-phy@0 {
+              reg = <0>;
+          };
+        };
+      };
+    };
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 4e2ba1bf788c..a88d1c236eaf 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -66,6 +66,9 @@ properties:
         - ingenic,x2000-mac
         - loongson,ls2k-dwmac
         - loongson,ls7a-dwmac
+        - nxp,s32g2-dwmac
+        - nxp,s32g3-dwmac
+        - nxp,s32r-dwmac
         - qcom,qcs404-ethqos
         - qcom,sa8775p-ethqos
         - qcom,sc8280xp-ethqos

-- 
2.46.0

Re: [PATCH v4 13/16] dt-bindings: net: Add DT bindings for DWMAC on NXP S32G/R SoCs

[Krzysztof Kozlowski]

On Mon, Oct 28, 2024 at 09:24:55PM +0100, Jan Petrous (OSS) wrote:
> Add basic description for DWMAC ethernet IP on NXP S32G2xx, S32G3xx
> and S32R45 automotive series SoCs.
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> ---
>  .../devicetree/bindings/net/nxp,s32-dwmac.yaml     | 98 ++++++++++++++++++++++
>  .../devicetree/bindings/net/snps,dwmac.yaml        |  3 +
>  2 files changed, 101 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> new file mode 100644
> index 000000000000..b11ba3bc4c52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2021-2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,s32-dwmac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32G2xx/S32G3xx/S32R45 GMAC ethernet controller
> +
> +maintainers:
> +  - Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> +
> +description:
> +  This device is a Synopsys DWC IP, integrated on NXP S32G/R SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g2-dwmac
> +      - nxp,s32g3-dwmac
> +      - nxp,s32r-dwmac

Your driver says these are fully compatible, why this is not expressed
here?

> +
> +  reg:
> +    items:
> +      - description: Main GMAC registers
> +      - description: GMAC PHY mode control register
>

...

> +        mdio {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          compatible = "snps,dwmac-mdio";
> +
> +          phy0: ethernet-phy@0 {
> +              reg = <0>;

Messed indentation. Keep it consistent.

Best regards,
Krzysztof

Re: [PATCH v4 13/16] dt-bindings: net: Add DT bindings for DWMAC on NXP S32G/R SoCs

[Jan Petrous]

On Tue, Oct 29, 2024 at 08:12:37AM +0100, Krzysztof Kozlowski wrote:
> On Mon, Oct 28, 2024 at 09:24:55PM +0100, Jan Petrous (OSS) wrote:
> > Add basic description for DWMAC ethernet IP on NXP S32G2xx, S32G3xx
> > and S32R45 automotive series SoCs.
> > 
> > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> > ---
> >  .../devicetree/bindings/net/nxp,s32-dwmac.yaml     | 98 ++++++++++++++++++++++
> >  .../devicetree/bindings/net/snps,dwmac.yaml        |  3 +
> >  2 files changed, 101 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > new file mode 100644
> > index 000000000000..b11ba3bc4c52
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > @@ -0,0 +1,98 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2021-2024 NXP
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,s32-dwmac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP S32G2xx/S32G3xx/S32R45 GMAC ethernet controller
> > +
> > +maintainers:
> > +  - Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> > +
> > +description:
> > +  This device is a Synopsys DWC IP, integrated on NXP S32G/R SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nxp,s32g2-dwmac
> > +      - nxp,s32g3-dwmac
> > +      - nxp,s32r-dwmac
> 
> Your driver says these are fully compatible, why this is not expressed
> here?
> 

They are compatible on current stage of driver implementation, the
RGMII interface has no any difference. But later there shall be
added SGMII and this provides some level of difference, at least
from max-speed POV.

The S32R allows higher speed (2G5) on SGMII, but S32G2/S32G3 has
1G as maximum.

> > +
> > +  reg:
> > +    items:
> > +      - description: Main GMAC registers
> > +      - description: GMAC PHY mode control register
> >
> 
> ...
> 
> > +        mdio {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +          compatible = "snps,dwmac-mdio";
> > +
> > +          phy0: ethernet-phy@0 {
> > +              reg = <0>;
> 
> Messed indentation. Keep it consistent.
> 

Thanks. I will fix it in v5.

> Best regards,
> Krzysztof
> 

Re: [PATCH v4 13/16] dt-bindings: net: Add DT bindings for DWMAC on NXP S32G/R SoCs

[Andrew Lunn]

> They are compatible on current stage of driver implementation, the
> RGMII interface has no any difference. But later there shall be
> added SGMII and this provides some level of difference, at least
> from max-speed POV.
> 
> The S32R allows higher speed (2G5) on SGMII, but S32G2/S32G3 has
> 1G as maximum.

Please be careful with your naming. SGMII is defined by CISCO to only
support 10/100/1000Mbps. If you support more than 1G, it is not SGMII,
it is likely to be 2500BaseX, etc.

	Andrew

Re: [PATCH v4 13/16] dt-bindings: net: Add DT bindings for DWMAC on NXP S32G/R SoCs

[Krzysztof Kozlowski]

On 31/10/2024 15:29, Jan Petrous wrote:
> On Tue, Oct 29, 2024 at 08:12:37AM +0100, Krzysztof Kozlowski wrote:
>> On Mon, Oct 28, 2024 at 09:24:55PM +0100, Jan Petrous (OSS) wrote:
>>> Add basic description for DWMAC ethernet IP on NXP S32G2xx, S32G3xx
>>> and S32R45 automotive series SoCs.
>>>
>>> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
>>> ---
>>>  .../devicetree/bindings/net/nxp,s32-dwmac.yaml     | 98 ++++++++++++++++++++++
>>>  .../devicetree/bindings/net/snps,dwmac.yaml        |  3 +
>>>  2 files changed, 101 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
>>> new file mode 100644
>>> index 000000000000..b11ba3bc4c52
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
>>> @@ -0,0 +1,98 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Copyright 2021-2024 NXP
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/nxp,s32-dwmac.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: NXP S32G2xx/S32G3xx/S32R45 GMAC ethernet controller
>>> +
>>> +maintainers:
>>> +  - Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
>>> +
>>> +description:
>>> +  This device is a Synopsys DWC IP, integrated on NXP S32G/R SoCs.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - nxp,s32g2-dwmac
>>> +      - nxp,s32g3-dwmac
>>> +      - nxp,s32r-dwmac
>>
>> Your driver says these are fully compatible, why this is not expressed
>> here?
>>
> 
> They are compatible on current stage of driver implementation, the
> RGMII interface has no any difference. But later there shall be
> added SGMII and this provides some level of difference, at least
> from max-speed POV.
> 
> The S32R allows higher speed (2G5) on SGMII, but S32G2/S32G3 has
> 1G as maximum.

So G2/G3 will work just fine but with lower speeds? That's the meaning
of compatibility.

Best regards,
Krzysztof

[PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

NXP S32G2xx/S32G3xx and S32R45 are automotive grade SoCs
that integrate one or two Synopsys DWMAC 5.10/5.20 IPs.

The basic driver supports only RGMII interface.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig     |  12 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile    |   1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c | 226 ++++++++++++++++++++++++
 3 files changed, 239 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 05cc07b8f48c..a6579377bedb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -154,6 +154,18 @@ config DWMAC_RZN1
 	  the stmmac device driver. This support can make use of a custom MII
 	  converter PCS device.
 
+config DWMAC_S32
+	tristate "NXP S32G/S32R GMAC support"
+	default ARCH_S32
+	depends on OF && (ARCH_S32 || COMPILE_TEST)
+	help
+	  Support for ethernet controller on NXP S32CC SOCs.
+
+	  This selects NXP SoC glue layer support for the stmmac
+	  device driver. This driver is used for the S32CC series
+	  SOCs GMAC ethernet controller, ie. S32G2xx, S32G3xx and
+	  S32R45.
+
 config DWMAC_SOCFPGA
 	tristate "SOCFPGA dwmac support"
 	default ARCH_INTEL_SOCFPGA
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index c2f0e91f6bf8..1e87e2652c82 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_DWMAC_MESON)	+= dwmac-meson.o dwmac-meson8b.o
 obj-$(CONFIG_DWMAC_QCOM_ETHQOS)	+= dwmac-qcom-ethqos.o
 obj-$(CONFIG_DWMAC_ROCKCHIP)	+= dwmac-rk.o
 obj-$(CONFIG_DWMAC_RZN1)	+= dwmac-rzn1.o
+obj-$(CONFIG_DWMAC_S32)		+= dwmac-s32.o
 obj-$(CONFIG_DWMAC_SOCFPGA)	+= dwmac-altr-socfpga.o
 obj-$(CONFIG_DWMAC_STARFIVE)	+= dwmac-starfive.o
 obj-$(CONFIG_DWMAC_STI)		+= dwmac-sti.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
new file mode 100644
index 000000000000..fba221c37594
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NXP S32G/R GMAC glue layer
+ *
+ * Copyright 2019-2024 NXP
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/ethtool.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/of_address.h>
+#include <linux/phy.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define GMAC_TX_RATE_125M	125000000	/* 125MHz */
+#define GMAC_TX_RATE_25M	25000000	/* 25MHz */
+#define GMAC_TX_RATE_2M5	2500000		/* 2.5MHz */
+
+/* SoC PHY interface control register */
+#define PHY_INTF_SEL_MII        0x00
+#define PHY_INTF_SEL_SGMII      0x01
+#define PHY_INTF_SEL_RGMII      0x02
+#define PHY_INTF_SEL_RMII       0x08
+
+struct s32_priv_data {
+	void __iomem *ioaddr;
+	void __iomem *ctrl_sts;
+	struct device *dev;
+	phy_interface_t intf_mode;
+	struct clk *tx_clk;
+	struct clk *rx_clk;
+	bool rx_clk_enabled;
+};
+
+static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac)
+{
+	u32 intf_sel;
+
+	switch (gmac->intf_mode) {
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		intf_sel = PHY_INTF_SEL_RGMII;
+		break;
+	default:
+		dev_err(gmac->dev, "Unsupported PHY interface: %s\n",
+			phy_modes(gmac->intf_mode));
+		return -EINVAL;
+	}
+
+	writel(intf_sel, gmac->ctrl_sts);
+
+	dev_dbg(gmac->dev, "PHY mode set to %s\n", phy_modes(gmac->intf_mode));
+
+	return 0;
+}
+
+static int s32_gmac_init(struct platform_device *pdev, void *priv)
+{
+	struct s32_priv_data *gmac = priv;
+	int ret;
+
+	ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
+	if (!ret)
+		ret = clk_prepare_enable(gmac->tx_clk);
+
+	if (ret) {
+		dev_err(&pdev->dev, "Can't set tx clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(gmac->rx_clk);
+	if (ret)
+		dev_dbg(&pdev->dev, "Can't set rx, clock source is disabled.\n");
+	else
+		gmac->rx_clk_enabled = true;
+
+	ret = s32_gmac_write_phy_intf_select(gmac);
+	if (ret) {
+		clk_disable_unprepare(gmac->tx_clk);
+		if (gmac->rx_clk_enabled) {
+			clk_disable_unprepare(gmac->rx_clk);
+			gmac->rx_clk_enabled = false;
+		}
+
+		dev_err(&pdev->dev, "Can't set PHY interface mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void s32_gmac_exit(struct platform_device *pdev, void *priv)
+{
+	struct s32_priv_data *gmac = priv;
+
+	clk_disable_unprepare(gmac->tx_clk);
+
+	if (gmac->rx_clk_enabled) {
+		clk_disable_unprepare(gmac->rx_clk);
+		gmac->rx_clk_enabled = false;
+	}
+}
+
+static void s32_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
+{
+	struct s32_priv_data *gmac = priv;
+	long tx_clk_rate;
+	int ret;
+
+	if (!gmac->rx_clk_enabled) {
+		ret = clk_prepare_enable(gmac->rx_clk);
+		if (ret) {
+			dev_err(gmac->dev, "Can't set rx clock\n");
+			return;
+		}
+		dev_dbg(gmac->dev, "rx clock enabled\n");
+		gmac->rx_clk_enabled = true;
+	}
+
+	tx_clk_rate = rgmii_clock(speed);
+	if (tx_clk_rate < 0) {
+		dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n", speed);
+		return;
+	}
+
+	dev_dbg(gmac->dev, "Set tx clock to %ld Hz\n", tx_clk_rate);
+	ret = clk_set_rate(gmac->tx_clk, tx_clk_rate);
+	if (ret)
+		dev_err(gmac->dev, "Can't set tx clock\n");
+}
+
+static int s32_dwmac_probe(struct platform_device *pdev)
+{
+	struct plat_stmmacenet_data *plat;
+	struct device *dev = &pdev->dev;
+	struct s32_priv_data *gmac;
+	struct stmmac_resources res;
+	int ret;
+
+	gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
+	if (!gmac)
+		return -ENOMEM;
+
+	gmac->dev = &pdev->dev;
+
+	ret = stmmac_get_platform_resources(pdev, &res);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get platform resources\n");
+
+	plat = devm_stmmac_probe_config_dt(pdev, res.mac);
+	if (IS_ERR(plat))
+		return dev_err_probe(dev, PTR_ERR(plat),
+				     "dt configuration failed\n");
+
+	/* PHY interface mode control reg */
+	gmac->ctrl_sts = devm_platform_get_and_ioremap_resource(pdev, 1, NULL);
+	if (IS_ERR(gmac->ctrl_sts))
+		return dev_err_probe(dev, PTR_ERR(gmac->ctrl_sts),
+				     "S32CC config region is missing\n");
+
+	/* tx clock */
+	gmac->tx_clk = devm_clk_get(&pdev->dev, "tx");
+	if (IS_ERR(gmac->tx_clk))
+		return dev_err_probe(dev, PTR_ERR(gmac->tx_clk),
+				     "tx clock not found\n");
+
+	/* rx clock */
+	gmac->rx_clk = devm_clk_get(&pdev->dev, "rx");
+	if (IS_ERR(gmac->rx_clk))
+		return dev_err_probe(dev, PTR_ERR(gmac->rx_clk),
+				     "rx clock not found\n");
+
+	gmac->intf_mode = plat->phy_interface;
+	gmac->ioaddr = res.addr;
+
+	/* S32CC core feature set */
+	plat->has_gmac4 = true;
+	plat->pmt = 1;
+	plat->flags |= STMMAC_FLAG_SPH_DISABLE;
+	plat->rx_fifo_size = 20480;
+	plat->tx_fifo_size = 20480;
+
+	plat->init = s32_gmac_init;
+	plat->exit = s32_gmac_exit;
+	plat->fix_mac_speed = s32_fix_mac_speed;
+
+	plat->bsp_priv = gmac;
+
+	return stmmac_pltfr_probe(pdev, plat, &res);
+}
+
+static const struct of_device_id s32_dwmac_match[] = {
+	{ .compatible = "nxp,s32g2-dwmac" },
+	{ .compatible = "nxp,s32g3-dwmac" },
+	{ .compatible = "nxp,s32r-dwmac" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, s32_dwmac_match);
+
+static struct platform_driver s32_dwmac_driver = {
+	.probe		= s32_dwmac_probe,
+	.remove		= stmmac_pltfr_remove,
+	.driver		= {
+			    .name		= "s32-dwmac",
+			    .pm		= &stmmac_pltfr_pm_ops,
+			    .of_match_table = s32_dwmac_match,
+	},
+};
+module_platform_driver(s32_dwmac_driver);
+
+MODULE_AUTHOR("Jan Petrous (OSS) <jan.petrous@oss.nxp.com>");
+MODULE_DESCRIPTION("NXP S32G/R common chassis GMAC driver");
+MODULE_LICENSE("GPL");
+

-- 
2.46.0

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Krzysztof Kozlowski]

On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
> +	plat->init = s32_gmac_init;
> +	plat->exit = s32_gmac_exit;
> +	plat->fix_mac_speed = s32_fix_mac_speed;
> +
> +	plat->bsp_priv = gmac;
> +
> +	return stmmac_pltfr_probe(pdev, plat, &res);
> +}
> +
> +static const struct of_device_id s32_dwmac_match[] = {
> +	{ .compatible = "nxp,s32g2-dwmac" },
> +	{ .compatible = "nxp,s32g3-dwmac" },
> +	{ .compatible = "nxp,s32r-dwmac" },

Why do you need three same entries?

Best regards,
Krzysztof

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Jan Petrous]

On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
> > +	plat->init = s32_gmac_init;
> > +	plat->exit = s32_gmac_exit;
> > +	plat->fix_mac_speed = s32_fix_mac_speed;
> > +
> > +	plat->bsp_priv = gmac;
> > +
> > +	return stmmac_pltfr_probe(pdev, plat, &res);
> > +}
> > +
> > +static const struct of_device_id s32_dwmac_match[] = {
> > +	{ .compatible = "nxp,s32g2-dwmac" },
> > +	{ .compatible = "nxp,s32g3-dwmac" },
> > +	{ .compatible = "nxp,s32r-dwmac" },
> 
> Why do you need three same entries?
> 

We have three different SoCs and in v3 review you told me
to return all back:
https://patchwork.kernel.org/comment/26067257/

I'm not sure if we need s32g3 variant, it should depend on
changes between s32g2 and s32g3, but s32r will definitely
carry the interface max-speed value when SGMII support will
be added.

BR.
/Jan

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Krzysztof Kozlowski]

On 31/10/2024 15:43, Jan Petrous wrote:
> On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
>> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
>>> +	plat->init = s32_gmac_init;
>>> +	plat->exit = s32_gmac_exit;
>>> +	plat->fix_mac_speed = s32_fix_mac_speed;
>>> +
>>> +	plat->bsp_priv = gmac;
>>> +
>>> +	return stmmac_pltfr_probe(pdev, plat, &res);
>>> +}
>>> +
>>> +static const struct of_device_id s32_dwmac_match[] = {
>>> +	{ .compatible = "nxp,s32g2-dwmac" },
>>> +	{ .compatible = "nxp,s32g3-dwmac" },
>>> +	{ .compatible = "nxp,s32r-dwmac" },
>>
>> Why do you need three same entries?
>>
> 
> We have three different SoCs and in v3 review you told me
> to return all back:
> https://patchwork.kernel.org/comment/26067257/

It was about binding, not driver.

I also asked there: use proper fallback and compatibility. Both comments
of course affect your driver, but why choosing only first part?

> 
> I'm not sure if we need s32g3 variant, it should depend on
> changes between s32g2 and s32g3, but s32r will definitely
> carry the interface max-speed value when SGMII support will
> be added.
> 
> BR.
> /Jan

Best regards,
Krzysztof

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Jan Petrous]

On Thu, Oct 31, 2024 at 04:44:45PM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2024 15:43, Jan Petrous wrote:
> > On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
> >> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
> >>> +	plat->init = s32_gmac_init;
> >>> +	plat->exit = s32_gmac_exit;
> >>> +	plat->fix_mac_speed = s32_fix_mac_speed;
> >>> +
> >>> +	plat->bsp_priv = gmac;
> >>> +
> >>> +	return stmmac_pltfr_probe(pdev, plat, &res);
> >>> +}
> >>> +
> >>> +static const struct of_device_id s32_dwmac_match[] = {
> >>> +	{ .compatible = "nxp,s32g2-dwmac" },
> >>> +	{ .compatible = "nxp,s32g3-dwmac" },
> >>> +	{ .compatible = "nxp,s32r-dwmac" },
> >>
> >> Why do you need three same entries?
> >>
> > 
> > We have three different SoCs and in v3 review you told me
> > to return all back:
> > https://patchwork.kernel.org/comment/26067257/
> 
> It was about binding, not driver.
> 
> I also asked there: use proper fallback and compatibility. Both comments
> of course affect your driver, but why choosing only first part?
> 

Does it mean I should remove first two (G2/G3) members from match array
and use "nxp,s32r-dwmac" as fallback for G2/G3? And similarly change
the bindings to:

  compatible:
    oneOf:
      - const: nxp,s32r-dwmac
      - items:
	  - enum:
	      - nxp,s32g2-dwmac
	      - nxp,s32g3-dwmac
          - const: nxp,s32r-dwmac

And add here, into the driver, those members back when some device
specific feature will be needed? Am I understand your hints right?

Thanks.
/Jan

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Jan Petrous]

On Thu, Oct 31, 2024 at 06:16:46PM +0100, Jan Petrous wrote:
> On Thu, Oct 31, 2024 at 04:44:45PM +0100, Krzysztof Kozlowski wrote:
> > On 31/10/2024 15:43, Jan Petrous wrote:
> > > On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
> > >> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
> > >>> +	plat->init = s32_gmac_init;
> > >>> +	plat->exit = s32_gmac_exit;
> > >>> +	plat->fix_mac_speed = s32_fix_mac_speed;
> > >>> +
> > >>> +	plat->bsp_priv = gmac;
> > >>> +
> > >>> +	return stmmac_pltfr_probe(pdev, plat, &res);
> > >>> +}
> > >>> +
> > >>> +static const struct of_device_id s32_dwmac_match[] = {
> > >>> +	{ .compatible = "nxp,s32g2-dwmac" },
> > >>> +	{ .compatible = "nxp,s32g3-dwmac" },
> > >>> +	{ .compatible = "nxp,s32r-dwmac" },
> > >>
> > >> Why do you need three same entries?
> > >>
> > > 
> > > We have three different SoCs and in v3 review you told me
> > > to return all back:
> > > https://patchwork.kernel.org/comment/26067257/
> > 
> > It was about binding, not driver.
> > 
> > I also asked there: use proper fallback and compatibility. Both comments
> > of course affect your driver, but why choosing only first part?
> > 
> 
> Does it mean I should remove first two (G2/G3) members from match array
> and use "nxp,s32r-dwmac" as fallback for G2/G3? And similarly change
> the bindings to:
> 
>   compatible:
>     oneOf:
>       - const: nxp,s32r-dwmac
>       - items:
> 	  - enum:
> 	      - nxp,s32g2-dwmac
> 	      - nxp,s32g3-dwmac
>           - const: nxp,s32r-dwmac
> 
> And add here, into the driver, those members back when some device
> specific feature will be needed? Am I understand your hints right?
> 

Sorry, it's not correct. This way I'm not able to detect S32R which is
the only one with higher speed.

Then I could use the G2 as fallback I think, Ie.:

  compatible:
    oneOf:
      - const: nxp,s32g2-dwmac
      - items:
	  - enum:
              - nxp,s32g3-dwmac
              - nxp,s32r-dwmac
           - const: nxp,s32g2-dwmac
 
BR.
/Jan

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Krzysztof Kozlowski]

On 31/10/2024 18:24, Jan Petrous wrote:
> On Thu, Oct 31, 2024 at 06:16:46PM +0100, Jan Petrous wrote:
>> On Thu, Oct 31, 2024 at 04:44:45PM +0100, Krzysztof Kozlowski wrote:
>>> On 31/10/2024 15:43, Jan Petrous wrote:
>>>> On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
>>>>> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
>>>>>> +	plat->init = s32_gmac_init;
>>>>>> +	plat->exit = s32_gmac_exit;
>>>>>> +	plat->fix_mac_speed = s32_fix_mac_speed;
>>>>>> +
>>>>>> +	plat->bsp_priv = gmac;
>>>>>> +
>>>>>> +	return stmmac_pltfr_probe(pdev, plat, &res);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct of_device_id s32_dwmac_match[] = {
>>>>>> +	{ .compatible = "nxp,s32g2-dwmac" },
>>>>>> +	{ .compatible = "nxp,s32g3-dwmac" },
>>>>>> +	{ .compatible = "nxp,s32r-dwmac" },
>>>>>
>>>>> Why do you need three same entries?
>>>>>
>>>>
>>>> We have three different SoCs and in v3 review you told me
>>>> to return all back:
>>>> https://patchwork.kernel.org/comment/26067257/
>>>
>>> It was about binding, not driver.
>>>
>>> I also asked there: use proper fallback and compatibility. Both comments
>>> of course affect your driver, but why choosing only first part?
>>>
>>
>> Does it mean I should remove first two (G2/G3) members from match array
>> and use "nxp,s32r-dwmac" as fallback for G2/G3? And similarly change
>> the bindings to:
>>
>>   compatible:
>>     oneOf:
>>       - const: nxp,s32r-dwmac
>>       - items:
>> 	  - enum:
>> 	      - nxp,s32g2-dwmac
>> 	      - nxp,s32g3-dwmac
>>           - const: nxp,s32r-dwmac
>>
>> And add here, into the driver, those members back when some device
>> specific feature will be needed? Am I understand your hints right?
>>
> 
> Sorry, it's not correct. This way I'm not able to detect S32R which is
> the only one with higher speed.
> 
> Then I could use the G2 as fallback I think, Ie.:
> 
>   compatible:
>     oneOf:
>       - const: nxp,s32g2-dwmac
>       - items:
> 	  - enum:
>               - nxp,s32g3-dwmac
>               - nxp,s32r-dwmac
>            - const: nxp,s32g2-dwmac

I don't understand. In both cases you can 'detect r', if by this you
meant match and bind. I don't care which one is the fallback, but if one
does not work it points to different issues with your code.

Best regards,
Krzysztof

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Jan Petrous]

On Fri, Nov 01, 2024 at 04:40:50PM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2024 18:24, Jan Petrous wrote:
> > On Thu, Oct 31, 2024 at 06:16:46PM +0100, Jan Petrous wrote:
> >> On Thu, Oct 31, 2024 at 04:44:45PM +0100, Krzysztof Kozlowski wrote:
> >>> On 31/10/2024 15:43, Jan Petrous wrote:
> >>>> On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
> >>>>> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
> >>>>>> +	plat->init = s32_gmac_init;
> >>>>>> +	plat->exit = s32_gmac_exit;
> >>>>>> +	plat->fix_mac_speed = s32_fix_mac_speed;
> >>>>>> +
> >>>>>> +	plat->bsp_priv = gmac;
> >>>>>> +
> >>>>>> +	return stmmac_pltfr_probe(pdev, plat, &res);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static const struct of_device_id s32_dwmac_match[] = {
> >>>>>> +	{ .compatible = "nxp,s32g2-dwmac" },
> >>>>>> +	{ .compatible = "nxp,s32g3-dwmac" },
> >>>>>> +	{ .compatible = "nxp,s32r-dwmac" },
> >>>>>
> >>>>> Why do you need three same entries?
> >>>>>
> >>>>
> >>>> We have three different SoCs and in v3 review you told me
> >>>> to return all back:
> >>>> https://patchwork.kernel.org/comment/26067257/
> >>>
> >>> It was about binding, not driver.
> >>>
> >>> I also asked there: use proper fallback and compatibility. Both comments
> >>> of course affect your driver, but why choosing only first part?
> >>>
> >>
> >> Does it mean I should remove first two (G2/G3) members from match array
> >> and use "nxp,s32r-dwmac" as fallback for G2/G3? And similarly change
> >> the bindings to:
> >>
> >>   compatible:
> >>     oneOf:
> >>       - const: nxp,s32r-dwmac
> >>       - items:
> >> 	  - enum:
> >> 	      - nxp,s32g2-dwmac
> >> 	      - nxp,s32g3-dwmac
> >>           - const: nxp,s32r-dwmac
> >>
> >> And add here, into the driver, those members back when some device
> >> specific feature will be needed? Am I understand your hints right?
> >>
> > 
> > Sorry, it's not correct. This way I'm not able to detect S32R which is
> > the only one with higher speed.
> > 
> > Then I could use the G2 as fallback I think, Ie.:
> > 
> >   compatible:
> >     oneOf:
> >       - const: nxp,s32g2-dwmac
> >       - items:
> > 	  - enum:
> >               - nxp,s32g3-dwmac
> >               - nxp,s32r-dwmac
> >            - const: nxp,s32g2-dwmac
> 
> I don't understand. In both cases you can 'detect r', if by this you
> meant match and bind. I don't care which one is the fallback, but if one
> does not work it points to different issues with your code.
> 

I see. I will use the last variant (with s32g2 as fallback) in v5.

BR.
/Jan

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Krzysztof Kozlowski]

On 31/10/2024 18:16, Jan Petrous wrote:
> On Thu, Oct 31, 2024 at 04:44:45PM +0100, Krzysztof Kozlowski wrote:
>> On 31/10/2024 15:43, Jan Petrous wrote:
>>> On Tue, Oct 29, 2024 at 08:13:40AM +0100, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 28, 2024 at 09:24:56PM +0100, Jan Petrous (OSS) wrote:
>>>>> +	plat->init = s32_gmac_init;
>>>>> +	plat->exit = s32_gmac_exit;
>>>>> +	plat->fix_mac_speed = s32_fix_mac_speed;
>>>>> +
>>>>> +	plat->bsp_priv = gmac;
>>>>> +
>>>>> +	return stmmac_pltfr_probe(pdev, plat, &res);
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id s32_dwmac_match[] = {
>>>>> +	{ .compatible = "nxp,s32g2-dwmac" },
>>>>> +	{ .compatible = "nxp,s32g3-dwmac" },
>>>>> +	{ .compatible = "nxp,s32r-dwmac" },
>>>>
>>>> Why do you need three same entries?
>>>>
>>>
>>> We have three different SoCs and in v3 review you told me
>>> to return all back:
>>> https://patchwork.kernel.org/comment/26067257/
>>
>> It was about binding, not driver.
>>
>> I also asked there: use proper fallback and compatibility. Both comments
>> of course affect your driver, but why choosing only first part?
>>
> 
> Does it mean I should remove first two (G2/G3) members from match array
> and use "nxp,s32r-dwmac" as fallback for G2/G3? And similarly change
> the bindings to:
> 
>   compatible:
>     oneOf:
>       - const: nxp,s32r-dwmac
>       - items:
> 	  - enum:
> 	      - nxp,s32g2-dwmac
> 	      - nxp,s32g3-dwmac
>           - const: nxp,s32r-dwmac
> 
> And add here, into the driver, those members back when some device
> specific feature will be needed? Am I understand your hints right?

Yes, assuming devices are compatible, but so far driver suggested they are.

Best regards,
Krzysztof

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Andrew Lunn]

> +#define GMAC_TX_RATE_125M	125000000	/* 125MHz */
> +#define GMAC_TX_RATE_25M	25000000	/* 25MHz */
> +#define GMAC_TX_RATE_2M5	2500000		/* 2.5MHz */

With the swap to the new helper, i think 25M and 2M5 are no longer
needed.

> +static int s32_gmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct s32_priv_data *gmac = priv;
> +	int ret;
> +
> +	ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> +	if (!ret)
> +		ret = clk_prepare_enable(gmac->tx_clk);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't set tx clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(gmac->rx_clk);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "Can't set rx, clock source is disabled.\n");
> +	else
> +		gmac->rx_clk_enabled = true;

Why would this fail? And if it does fail, why is it not fatal? Maybe a
comment here.

> +static void s32_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> +{
> +	struct s32_priv_data *gmac = priv;
> +	long tx_clk_rate;
> +	int ret;
> +
> +	if (!gmac->rx_clk_enabled) {
> +		ret = clk_prepare_enable(gmac->rx_clk);
> +		if (ret) {
> +			dev_err(gmac->dev, "Can't set rx clock\n");

dev_err(), so is failing now fatal, but since this is a void function,
you cannot report the error up the call stack?

	Andrew

Re: [PATCH v4 14/16] net: stmmac: dwmac-s32: add basic NXP S32G/S32R glue driver

[Jan Petrous]

On Tue, Oct 29, 2024 at 01:15:52PM +0100, Andrew Lunn wrote:
> > +#define GMAC_TX_RATE_125M	125000000	/* 125MHz */
> > +#define GMAC_TX_RATE_25M	25000000	/* 25MHz */
> > +#define GMAC_TX_RATE_2M5	2500000		/* 2.5MHz */
> 
> With the swap to the new helper, i think 25M and 2M5 are no longer
> needed.
> 

Sure, I will fix it in v5.

> > +static int s32_gmac_init(struct platform_device *pdev, void *priv)
> > +{
> > +	struct s32_priv_data *gmac = priv;
> > +	int ret;
> > +
> > +	ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> > +	if (!ret)
> > +		ret = clk_prepare_enable(gmac->tx_clk);
> > +
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't set tx clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(gmac->rx_clk);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "Can't set rx, clock source is disabled.\n");
> > +	else
> > +		gmac->rx_clk_enabled = true;
> 
> Why would this fail? And if it does fail, why is it not fatal? Maybe a
> comment here.
> 
> > +static void s32_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> > +{
> > +	struct s32_priv_data *gmac = priv;
> > +	long tx_clk_rate;
> > +	int ret;
> > +
> > +	if (!gmac->rx_clk_enabled) {
> > +		ret = clk_prepare_enable(gmac->rx_clk);
> > +		if (ret) {
> > +			dev_err(gmac->dev, "Can't set rx clock\n");
> 
> dev_err(), so is failing now fatal, but since this is a void function,
> you cannot report the error up the call stack?
> 

I did a homework and checked the issue which was fixed by that 'lazy' rx
clock enable procedure and got conslusion it is not needed anymore, as I
was not able to reproduce the issue on the same board but with newer
kernel version (6.6.32 versus 5.15.73).

So I will simplify rx clock management in v5.

BR.
/Jan

[PATCH v4 15/16] MAINTAINERS: Add Jan Petrous as the NXP S32G/R DWMAC driver maintainer

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

Add myself as NXP S32G/R DWMAC Ethernet driver maintainer.

Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7ad507f49324..9cc074f7ca7d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2803,6 +2803,13 @@ S:	Maintained
 F:	arch/arm64/boot/dts/freescale/s32g*.dts*
 F:	drivers/pinctrl/nxp/
 
+ARM/NXP S32G/S32R DWMAC ETHERNET DRIVER
+M:	Jan Petrous <jan.petrous@oss.nxp.com>
+L:	NXP S32 Linux Team <s32@nxp.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
+F:	drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
+
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
 M:	Alexander Clouter <alex@digriz.org.uk>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)

-- 
2.46.0

Re: [PATCH v4 15/16] MAINTAINERS: Add Jan Petrous as the NXP S32G/R DWMAC driver maintainer

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:57PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> Add myself as NXP S32G/R DWMAC Ethernet driver maintainer.
> 
> Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v4 16/16] net: stmmac: dwmac-s32: Read PTP clock rate when ready

[Jan Petrous via B4 Relay]

From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>

The PTP clock is read by stmmac_platform during DT parse.
On S32G/R the clock is not ready and returns 0. Postpone
reading of the clock on PTP init.

Co-developed-by: Andrei Botila <andrei.botila@nxp.org>
Signed-off-by: Andrei Botila <andrei.botila@nxp.org>
Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
index fba221c37594..da2cdcfd0529 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
@@ -140,6 +140,18 @@ static void s32_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
 		dev_err(gmac->dev, "Can't set tx clock\n");
 }
 
+static void s32_dwmac_ptp_clk_freq_config(struct stmmac_priv *priv)
+{
+	struct plat_stmmacenet_data *plat = priv->plat;
+
+	if (!plat->clk_ptp_ref)
+		return;
+
+	plat->clk_ptp_rate = clk_get_rate(plat->clk_ptp_ref);
+
+	netdev_dbg(priv->dev, "PTP rate %lu\n", plat->clk_ptp_rate);
+}
+
 static int s32_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat;
@@ -195,6 +207,7 @@ static int s32_dwmac_probe(struct platform_device *pdev)
 	plat->init = s32_gmac_init;
 	plat->exit = s32_gmac_exit;
 	plat->fix_mac_speed = s32_fix_mac_speed;
+	plat->ptp_clk_freq_config = s32_dwmac_ptp_clk_freq_config;
 
 	plat->bsp_priv = gmac;
 

-- 
2.46.0

Re: [PATCH v4 16/16] net: stmmac: dwmac-s32: Read PTP clock rate when ready

[Andrew Lunn]

On Mon, Oct 28, 2024 at 09:24:58PM +0100, Jan Petrous via B4 Relay wrote:
> From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> 
> The PTP clock is read by stmmac_platform during DT parse.
> On S32G/R the clock is not ready and returns 0. Postpone
> reading of the clock on PTP init.

This needs more explanation as to why this is a feature, not a bug,
for the PTP clock.

	Andrew

Re: [PATCH v4 16/16] net: stmmac: dwmac-s32: Read PTP clock rate when ready

[Jan Petrous]

On Tue, Oct 29, 2024 at 01:18:43PM +0100, Andrew Lunn wrote:
> On Mon, Oct 28, 2024 at 09:24:58PM +0100, Jan Petrous via B4 Relay wrote:
> > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> > 
> > The PTP clock is read by stmmac_platform during DT parse.
> > On S32G/R the clock is not ready and returns 0. Postpone
> > reading of the clock on PTP init.
> 
> This needs more explanation as to why this is a feature, not a bug,
> for the PTP clock.
> 

Thanks for comment. I did a homework and found out the root cause is
using PTP clocks before they are properly enabled. As I understand,
the clocks, especially the composite variant, require preparation and/or
enabling them, what is not managed correctly for PTP clocks when
stmmac_platform is used. In this case, the PTP clock value is read this way:

   stmmac_probe_config_dt:
   // https://github.com/torvalds/linux/blob/4a5df37964673effcd9f84041f7423206a5ae5f2/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#L634

	/* Fall-back to main clock in case of no PTP ref is passed */
	plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref");
	if (IS_ERR(plat->clk_ptp_ref)) {
		plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
		plat->clk_ptp_ref = NULL;
		dev_info(&pdev->dev, "PTP uses main clock\n");
	} else {
		plat->clk_ptp_rate = clk_get_rate(plat->clk_ptp_ref);
		dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate);
	}

If I change getter to enabled getter:
	plat->clk_ptp_ref = devm_clk_get_enabled(&pdev->dev, "ptp_ref");

The driver got valid rate and the patch is not needed anymore.

So, if I didn't miss something, it seems like I have to replace the current
patch with one fixing clk getter.

BR.
/Jan