[PATCH 0/4] s32g: Use a syscon for GPR

[PATCH 0/4] s32g: Use a syscon for GPR

[Dan Carpenter]

*** BLURB HERE ***

Dan Carpenter (4):
  net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII
  dt-bindings: mfd: syscon: Document the GPR syscon for the NXP S32 SoCs
  dt-bindings: net: nxp,s32-dwmac: Use the GPR syscon
  dts: s32g: Add GPR syscon region

 .../devicetree/bindings/mfd/syscon.yaml       |  2 ++
 .../bindings/net/nxp,s32-dwmac.yaml           |  6 +++++
 arch/arm64/boot/dts/freescale/s32g2.dtsi      |  8 +++++++
 arch/arm64/boot/dts/freescale/s32g3.dtsi      |  8 +++++++
 .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
 5 files changed, 42 insertions(+), 5 deletions(-)

-- 
2.51.0

[PATCH 1/4] net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII

[Dan Carpenter]

On the s32 chipset the GMAC_0_CTRL_STS register is in GPR region.
Originally, accessing this register was done in a sort of ad-hoc way,
but we want to use the syscon interface to do it.

This is a little bit uglier because we to maintain backwards compatibility
to the old device trees so we have to support both ways to access this
register.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
index 5a485ee98fa7..20de761b7d28 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
@@ -11,12 +11,14 @@
 #include <linux/device.h>
 #include <linux/ethtool.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.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/regmap.h>
 #include <linux/stmmac.h>
 
 #include "stmmac_platform.h"
@@ -32,6 +34,8 @@
 struct s32_priv_data {
 	void __iomem *ioaddr;
 	void __iomem *ctrl_sts;
+	struct regmap *sts_regmap;
+	unsigned int sts_offset;
 	struct device *dev;
 	phy_interface_t *intf_mode;
 	struct clk *tx_clk;
@@ -40,7 +44,10 @@ struct s32_priv_data {
 
 static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac)
 {
-	writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
+	if (gmac->ctrl_sts)
+		writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
+	else
+		regmap_write(gmac->sts_regmap, gmac->sts_offset, PHY_INTF_SEL_RGMII);
 
 	dev_dbg(gmac->dev, "PHY mode set to %s\n", phy_modes(*gmac->intf_mode));
 
@@ -125,10 +132,16 @@ static int s32_dwmac_probe(struct platform_device *pdev)
 				     "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");
+	gmac->sts_regmap = syscon_regmap_lookup_by_phandle_args(dev->of_node,
+					"phy-sel", 1, &gmac->sts_offset);
+	if (gmac->sts_regmap == ERR_PTR(-EPROBE_DEFER))
+		return PTR_ERR(gmac->sts_regmap);
+	if (IS_ERR(gmac->sts_regmap)) {
+		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");
-- 
2.51.0

Re: [PATCH 1/4] net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII

[Russell King (Oracle)]

On Mon, Dec 01, 2025 at 04:08:20PM +0300, Dan Carpenter wrote:
> On the s32 chipset the GMAC_0_CTRL_STS register is in GPR region.
> Originally, accessing this register was done in a sort of ad-hoc way,
> but we want to use the syscon interface to do it.
> 
> This is a little bit uglier because we to maintain backwards compatibility
> to the old device trees so we have to support both ways to access this
> register.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> index 5a485ee98fa7..20de761b7d28 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> @@ -11,12 +11,14 @@
>  #include <linux/device.h>
>  #include <linux/ethtool.h>
>  #include <linux/io.h>
> +#include <linux/mfd/syscon.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/regmap.h>
>  #include <linux/stmmac.h>
>  
>  #include "stmmac_platform.h"
> @@ -32,6 +34,8 @@
>  struct s32_priv_data {
>  	void __iomem *ioaddr;
>  	void __iomem *ctrl_sts;
> +	struct regmap *sts_regmap;
> +	unsigned int sts_offset;
>  	struct device *dev;
>  	phy_interface_t *intf_mode;
>  	struct clk *tx_clk;
> @@ -40,7 +44,10 @@ struct s32_priv_data {
>  
>  static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac)
>  {
> -	writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> +	if (gmac->ctrl_sts)
> +		writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> +	else
> +		regmap_write(gmac->sts_regmap, gmac->sts_offset, PHY_INTF_SEL_RGMII);

Sorry, but even if that regmap_write() is targetting the exact same
register, these are not identical.

S32_PHY_INTF_SEL_RGMII, which is a S32-specific value, takes the value 2.
PHY_INTF_SEL_RGMII is the dwmac specific value, and takes the value 1.

If this targets the same register, then by writing PHY_INTF_SEL_RGMII,
you are in effect writing the equivalent of S32_PHY_INTF_SEL_SGMII to
it. This seems like a bug.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/4] net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII

[Dan Carpenter]

On Mon, Dec 01, 2025 at 04:48:12PM +0000, Russell King (Oracle) wrote:
> On Mon, Dec 01, 2025 at 04:08:20PM +0300, Dan Carpenter wrote:
> > On the s32 chipset the GMAC_0_CTRL_STS register is in GPR region.
> > Originally, accessing this register was done in a sort of ad-hoc way,
> > but we want to use the syscon interface to do it.
> > 
> > This is a little bit uglier because we to maintain backwards compatibility
> > to the old device trees so we have to support both ways to access this
> > register.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > index 5a485ee98fa7..20de761b7d28 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > @@ -11,12 +11,14 @@
> >  #include <linux/device.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/io.h>
> > +#include <linux/mfd/syscon.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/regmap.h>
> >  #include <linux/stmmac.h>
> >  
> >  #include "stmmac_platform.h"
> > @@ -32,6 +34,8 @@
> >  struct s32_priv_data {
> >  	void __iomem *ioaddr;
> >  	void __iomem *ctrl_sts;
> > +	struct regmap *sts_regmap;
> > +	unsigned int sts_offset;
> >  	struct device *dev;
> >  	phy_interface_t *intf_mode;
> >  	struct clk *tx_clk;
> > @@ -40,7 +44,10 @@ struct s32_priv_data {
> >  
> >  static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac)
> >  {
> > -	writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> > +	if (gmac->ctrl_sts)
> > +		writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> > +	else
> > +		regmap_write(gmac->sts_regmap, gmac->sts_offset, PHY_INTF_SEL_RGMII);
> 
> Sorry, but even if that regmap_write() is targetting the exact same
> register, these are not identical.
> 
> S32_PHY_INTF_SEL_RGMII, which is a S32-specific value, takes the value 2.
> PHY_INTF_SEL_RGMII is the dwmac specific value, and takes the value 1.
> 
> If this targets the same register, then by writing PHY_INTF_SEL_RGMII,
> you are in effect writing the equivalent of S32_PHY_INTF_SEL_SGMII to
> it. This seems like a bug.
> 

Yeah.  Sorry, I forward ported this, then back ported it, then forward
ported this again and I messed up.  :(

regards,
dan carpenter

Re: [PATCH 1/4] net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII

[Frank Li]

On Mon, Dec 01, 2025 at 04:08:20PM +0300, Dan Carpenter wrote:
> On the s32 chipset the GMAC_0_CTRL_STS register is in GPR region.
> Originally, accessing this register was done in a sort of ad-hoc way,
> but we want to use the syscon interface to do it.

What's benefit by use syscon interface here? syscon have not much
well consided funcitonal abstraction.

Frank

>
> This is a little bit uglier because we to maintain backwards compatibility
> to the old device trees so we have to support both ways to access this
> register.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> index 5a485ee98fa7..20de761b7d28 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> @@ -11,12 +11,14 @@
>  #include <linux/device.h>
>  #include <linux/ethtool.h>
>  #include <linux/io.h>
> +#include <linux/mfd/syscon.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/regmap.h>
>  #include <linux/stmmac.h>
>
>  #include "stmmac_platform.h"
> @@ -32,6 +34,8 @@
>  struct s32_priv_data {
>  	void __iomem *ioaddr;
>  	void __iomem *ctrl_sts;
> +	struct regmap *sts_regmap;
> +	unsigned int sts_offset;
>  	struct device *dev;
>  	phy_interface_t *intf_mode;
>  	struct clk *tx_clk;
> @@ -40,7 +44,10 @@ struct s32_priv_data {
>
>  static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac)
>  {
> -	writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> +	if (gmac->ctrl_sts)
> +		writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> +	else
> +		regmap_write(gmac->sts_regmap, gmac->sts_offset, PHY_INTF_SEL_RGMII);
>
>  	dev_dbg(gmac->dev, "PHY mode set to %s\n", phy_modes(*gmac->intf_mode));
>
> @@ -125,10 +132,16 @@ static int s32_dwmac_probe(struct platform_device *pdev)
>  				     "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");
> +	gmac->sts_regmap = syscon_regmap_lookup_by_phandle_args(dev->of_node,
> +					"phy-sel", 1, &gmac->sts_offset);
> +	if (gmac->sts_regmap == ERR_PTR(-EPROBE_DEFER))
> +		return PTR_ERR(gmac->sts_regmap);
> +	if (IS_ERR(gmac->sts_regmap)) {
> +		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");
> --
> 2.51.0
>

Re: [PATCH 1/4] net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII

[Dan Carpenter]

On Mon, Dec 01, 2025 at 05:29:36PM -0500, Frank Li wrote:
> On Mon, Dec 01, 2025 at 04:08:20PM +0300, Dan Carpenter wrote:
> > On the s32 chipset the GMAC_0_CTRL_STS register is in GPR region.
> > Originally, accessing this register was done in a sort of ad-hoc way,
> > but we want to use the syscon interface to do it.
> 
> What's benefit by use syscon interface here? syscon have not much
> well consided funcitonal abstraction.
> 

The GPR has a bunch of random registers that aren't really related.
On these chips they're just regular MMIO registers, but in other
configurations you can only access them using SCMI.

It's better to group them together that's how they are in the hardware.
Otherwise we'd end up randomly adding a register address to the
ethernet device tree entry, but it's nicer to use a phandle to
reference the GPR.

The only register we're using now is the GMAC_0_CTRL_STS but here
is the list of registers in the GPR.

From 0x4007C000

0  Software-Triggered Faults (SW_NCF)
4  GMAC Control (GMAC_0_CTRL_STS)
28 CMU Status 1 (CMU_STATUS_REG1)
2C CMUs Status 2 (CMU_STATUS_REG2)
30 FCCU EOUT Override Clear (FCCU_EOUT_OVERRIDE_CLEAR_REG)
38 SRC POR Control (SRC_POR_CTRL_REG)
54 GPR21 (GPR21)
5C GPR23 (GPR23)
60 GPR24 Register (GPR24)
CC Debug Control (DEBUG_CONTROL)
F0 Timestamp Control (TIMESTAMP_CONTROL_REGISTER)
F4 FlexRay OS Tick Input Select (FLEXRAY_OS_TICK_INPUT_SELECT_REG)
FC GPR63 Register (GPR63)

Then from 0x4007CA00

0  Coherency Enable for PFE Ports (PFE_COH_EN)
4  PFE EMAC Interface Mode (PFE_EMACX_INTF_SEL)
20 PFE EMACX Power Control (PFE_PWR_CTRL)
28 Error Injection on Cortex-M7 AHB and AXI Pipe (CM7_TCM_AHB_SLICE)
2C Error Injection AHBP Gasket Cortex-M7 (ERROR_INJECTION_AHBP_GASKET_CM7)
40 LLCE Subsystem Status (LLCE_STAT)
44 LLCE Power Control (LLCE_CTRL)
48 DDR Urgent Control (DDR_URGENT_CTRL)
4C FTM Global Load Control (FLXTIM_CTRL)
50 FTM LDOK Status (FLXTIM_STAT)
54 Top CMU Status (CMU_STAT)
58 Accelerator NoC No Pending Trans Status (NOC_NOPEND_TRANS)
90 SerDes RD/WD Toggle Control (PCIE_TOGGLE)
94 SerDes Toggle Done Status (PCIE_TOGGLEDONE_STAT)
E0 Generic Control 0 (GENCTRL0)
E4 Generic Control 1 (GENCTRL1)
F0 Generic Status 0 (GENSTAT0)
FC Cortex-M7 AXI Parity Error and AHBP Gasket Error Alarm (CM7_AXI_AHBP_GASKET_ERROR_ALARM)

From 4007C800

4  GPR01 Register (GPR01)
30 GPR12 Register (GPR12)
58 GPR22 Register (GPR22)
70 GPR28 Register (GPR28)
74 GPR29 Register (GPR29)

From 4007CB00

4 WKUP Pad Pullup/Pulldown Select (WKUP_PUS)

regards,
dan carpenter

[PATCH 2/4] dt-bindings: mfd: syscon: Document the GPR syscon for the NXP S32 SoCs

[Dan Carpenter]

The NXP S32 SoCs have a GPR region which is used by a variety of
drivers.  Some examples of the registers in this region are:
  * DDR_PMU_IRQ
  * GMAC0_PHY_INTF_SEL
  * GMAC1_PHY_INTF_SEL
  * PFE_EMACS_INTF_SEL
  * PFE_COH_EN
  * PFE_PWR_CTRL
  * PFE_EMACS_GENCTRL1
  * PFE_GENCTRL3

Use the syscon interface to access these registers.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 55efb83b1495..6e6b92227092 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -102,6 +102,7 @@ select:
           - mstar,msc313-pmsleep
           - nuvoton,ma35d1-sys
           - nuvoton,wpcm450-shm
+          - nxp,s32-gpr
           - qcom,apq8064-mmss-sfpb
           - qcom,apq8064-sps-sic
           - rockchip,px30-qos
@@ -212,6 +213,7 @@ properties:
               - mstar,msc313-pmsleep
               - nuvoton,ma35d1-sys
               - nuvoton,wpcm450-shm
+              - nxp,s32-gpr
               - qcom,apq8064-mmss-sfpb
               - qcom,apq8064-sps-sic
               - rockchip,px30-qos
-- 
2.51.0

Re: [PATCH 2/4] dt-bindings: mfd: syscon: Document the GPR syscon for the NXP S32 SoCs

[Krzysztof Kozlowski]

On 01/12/2025 14:08, Dan Carpenter wrote:
> The NXP S32 SoCs have a GPR region which is used by a variety of
> drivers.  Some examples of the registers in this region are:
>   * DDR_PMU_IRQ
>   * GMAC0_PHY_INTF_SEL
>   * GMAC1_PHY_INTF_SEL
>   * PFE_EMACS_INTF_SEL
>   * PFE_COH_EN
>   * PFE_PWR_CTRL
>   * PFE_EMACS_GENCTRL1
>   * PFE_GENCTRL3
> 
> Use the syscon interface to access these registers.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> index 55efb83b1495..6e6b92227092 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> @@ -102,6 +102,7 @@ select:
>            - mstar,msc313-pmsleep
>            - nuvoton,ma35d1-sys
>            - nuvoton,wpcm450-shm
> +          - nxp,s32-gpr
>            - qcom,apq8064-mmss-sfpb
>            - qcom,apq8064-sps-sic
>            - rockchip,px30-qos
> @@ -212,6 +213,7 @@ properties:
>                - mstar,msc313-pmsleep
>                - nuvoton,ma35d1-sys
>                - nuvoton,wpcm450-shm
> +              - nxp,s32-gpr
>                - qcom,apq8064-mmss-sf

You should have Soc specific compatible, not family one, so s32g2 and
probably separate s32g3.


>                - qcom,apq8064-sps-sic
>                - rockchip,px30-qos


Best regards,
Krzysztof

[PATCH 3/4] dt-bindings: net: nxp,s32-dwmac: Use the GPR syscon

[Dan Carpenter]

The S32 chipset has a GPR region which has a miscellaneous registers
including the GMAC_0_CTRL_STS register.  Originally this code accessed
that register in a sort of ad-hoc way, but we want to access it using
the syscon interface.

We still need to maintain the old method of accessing the GMAC register
but using a syscon will let us access other registers more cleanly.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
index 2b8b74c5feec..17f6c50dca03 100644
--- a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
@@ -32,6 +32,11 @@ properties:
       - description: Main GMAC registers
       - description: GMAC PHY mode control register
 
+  phy-sel:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - description: The offset into the s32 GPR syscon
+
   interrupts:
     maxItems: 1
 
@@ -74,6 +79,7 @@ examples:
         compatible = "nxp,s32g2-dwmac";
         reg = <0x0 0x4033c000 0x0 0x2000>, /* gmac IP */
               <0x0 0x4007c004 0x0 0x4>;    /* GMAC_0_CTRL_STS */
+        phy-sel = <&gpr 0x4>;
         interrupt-parent = <&gic>;
         interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
         interrupt-names = "macirq";
-- 
2.51.0

Re: [PATCH 3/4] dt-bindings: net: nxp,s32-dwmac: Use the GPR syscon

[Krzysztof Kozlowski]

On 01/12/2025 14:08, Dan Carpenter wrote:
> The S32 chipset has a GPR region which has a miscellaneous registers
> including the GMAC_0_CTRL_STS register.  Originally this code accessed
> that register in a sort of ad-hoc way, but we want to access it using
> the syscon interface.
> 
> We still need to maintain the old method of accessing the GMAC register
> but using a syscon will let us access other registers more cleanly.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> index 2b8b74c5feec..17f6c50dca03 100644
> --- a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> @@ -32,6 +32,11 @@ properties:
>        - description: Main GMAC registers
>        - description: GMAC PHY mode control register
>  
> +  phy-sel:

Missing vendor prefix.

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - description: The offset into the s32 GPR syscon

No, first item is not the offset but the phandle. You need syntax like here:

https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42

The description of the first item (unlike in example above) should say
what is the purpose, how this device is using GPR region, what is it
needed for.

Best regards,
Krzysztof

Re: [PATCH 3/4] dt-bindings: net: nxp,s32-dwmac: Use the GPR syscon

[Dan Carpenter]

On Mon, Dec 01, 2025 at 06:33:07PM +0100, Krzysztof Kozlowski wrote:
> On 01/12/2025 14:08, Dan Carpenter wrote:
> > The S32 chipset has a GPR region which has a miscellaneous registers
> > including the GMAC_0_CTRL_STS register.  Originally this code accessed
> > that register in a sort of ad-hoc way, but we want to access it using
> > the syscon interface.
> > 
> > We still need to maintain the old method of accessing the GMAC register
> > but using a syscon will let us access other registers more cleanly.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > index 2b8b74c5feec..17f6c50dca03 100644
> > --- a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > @@ -32,6 +32,11 @@ properties:
> >        - description: Main GMAC registers
> >        - description: GMAC PHY mode control register
> >  
> > +  phy-sel:
> 
> Missing vendor prefix.
> 
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - description: The offset into the s32 GPR syscon
> 
> No, first item is not the offset but the phandle. You need syntax like here:
> 
> https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
> 
> The description of the first item (unlike in example above) should say
> what is the purpose, how this device is using GPR region, what is it
> needed for.

I had to do it a bit differently from the exynos-usi.yaml code.  When I
tried it that way I got the following warning that the "phy-sel" wasn't
a common suffix and it doesn't have a description.

$ make dt_binding_check DT_SCHEMA_FILES=net/nxp,s32-dwmac.yaml
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  CHKDT   ./Documentation/devicetree/bindings
/home/dcarpenter/progs/kernel/nxp_gpr/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml: properties:nxp,phy-sel: 'anyOf' conditional failed, one must be fixed:
        'description' is a dependency of '$ref'
        '/schemas/types.yaml#/definitions/phandle-array' does not match '^#/(definitions|\\$defs)/'
                hint: A vendor property can have a $ref to a a $defs schema
        hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
        from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
  LINT    ./Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/net/nxp,s32-dwmac.example.dts
  DTC [C] Documentation/devicetree/bindings/net/nxp,s32-dwmac.example.dtb

The exynos-usi.yaml file doesn't generate a warning like that and I wasn't
able to figure out why that is.  But what worked for me was adding the
phandle description like this:

  nxp,phy-sel:
    description: phandle to the GPR syscon node
    $ref: /schemas/types.yaml#/definitions/phandle-array
    items:
      - description: offset of PHY selection register

regards,
dan carpenter

Re: [PATCH 3/4] dt-bindings: net: nxp,s32-dwmac: Use the GPR syscon

[Dan Carpenter]

On Mon, Dec 15, 2025 at 01:59:09PM +0300, Dan Carpenter wrote:
> On Mon, Dec 01, 2025 at 06:33:07PM +0100, Krzysztof Kozlowski wrote:
> > On 01/12/2025 14:08, Dan Carpenter wrote:
> > > The S32 chipset has a GPR region which has a miscellaneous registers
> > > including the GMAC_0_CTRL_STS register.  Originally this code accessed
> > > that register in a sort of ad-hoc way, but we want to access it using
> > > the syscon interface.
> > > 
> > > We still need to maintain the old method of accessing the GMAC register
> > > but using a syscon will let us access other registers more cleanly.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > > index 2b8b74c5feec..17f6c50dca03 100644
> > > --- a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > > +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> > > @@ -32,6 +32,11 @@ properties:
> > >        - description: Main GMAC registers
> > >        - description: GMAC PHY mode control register
> > >  
> > > +  phy-sel:
> > 
> > Missing vendor prefix.
> > 
> > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > +    items:
> > > +      - description: The offset into the s32 GPR syscon
> > 
> > No, first item is not the offset but the phandle. You need syntax like here:
> > 
> > https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42
> > 
> > The description of the first item (unlike in example above) should say
> > what is the purpose, how this device is using GPR region, what is it
> > needed for.
> 
> I had to do it a bit differently from the exynos-usi.yaml code.  When I
> tried it that way I got the following warning that the "phy-sel" wasn't
> a common suffix and it doesn't have a description.
> 
> $ make dt_binding_check DT_SCHEMA_FILES=net/nxp,s32-dwmac.yaml
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   CHKDT   ./Documentation/devicetree/bindings
> /home/dcarpenter/progs/kernel/nxp_gpr/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml: properties:nxp,phy-sel: 'anyOf' conditional failed, one must be fixed:
>         'description' is a dependency of '$ref'
>         '/schemas/types.yaml#/definitions/phandle-array' does not match '^#/(definitions|\\$defs)/'
>                 hint: A vendor property can have a $ref to a a $defs schema
>         hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>         from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>   LINT    ./Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/net/nxp,s32-dwmac.example.dts
>   DTC [C] Documentation/devicetree/bindings/net/nxp,s32-dwmac.example.dtb
> 
> The exynos-usi.yaml file doesn't generate a warning like that and I wasn't
> able to figure out why that is.

Oh, crap.  I'm an idiot.  I've been staring at this for an embarrassing
long time, and I didn't see that the exynos-usi.yaml has a description
after the - items description.  And then I send this and I immediately
see it.  :/

regards,
dan carpenter

[PATCH 4/4] dts: s32g: Add GPR syscon region

[Dan Carpenter]

Add the GPR syscon region for the s32 chipset.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 arch/arm64/boot/dts/freescale/s32g2.dtsi | 8 ++++++++
 arch/arm64/boot/dts/freescale/s32g3.dtsi | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
index 51d00dac12de..3c9472f6c174 100644
--- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
+++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
@@ -325,6 +325,13 @@ usdhc0-200mhz-grp4 {
 			};
 		};
 
+		gpr: syscon@4007c000 {
+			compatible = "nxp,s32-gpr", "syscon";
+			reg = <0x4007c000 0x3000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		ocotp: nvmem@400a4000 {
 			compatible = "nxp,s32g2-ocotp";
 			reg = <0x400a4000 0x400>;
@@ -731,6 +738,7 @@ gmac0: ethernet@4033c000 {
 			compatible = "nxp,s32g2-dwmac";
 			reg = <0x4033c000 0x2000>, /* gmac IP */
 			      <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
+			phy-sel = <&gpr 0x4>;
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "macirq";
diff --git a/arch/arm64/boot/dts/freescale/s32g3.dtsi b/arch/arm64/boot/dts/freescale/s32g3.dtsi
index eff7673e7f34..0ceca3caf133 100644
--- a/arch/arm64/boot/dts/freescale/s32g3.dtsi
+++ b/arch/arm64/boot/dts/freescale/s32g3.dtsi
@@ -383,6 +383,13 @@ usdhc0-200mhz-grp4 {
 			};
 		};
 
+		gpr: syscon@4007c000 {
+			compatible = "nxp,s32-gpr", "syscon";
+			reg = <0x4007c000 0x3000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		ocotp: nvmem@400a4000 {
 			compatible = "nxp,s32g3-ocotp", "nxp,s32g2-ocotp";
 			reg = <0x400a4000 0x400>;
@@ -808,6 +815,7 @@ gmac0: ethernet@4033c000 {
 			compatible = "nxp,s32g2-dwmac";
 			reg = <0x4033c000 0x2000>, /* gmac IP */
 			      <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
+			phy-sel = <&gpr 0x4>;
 			interrupt-parent = <&gic>;
 			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "macirq";
-- 
2.51.0

Re: [PATCH 4/4] dts: s32g: Add GPR syscon region

[Frank Li]

On Mon, Dec 01, 2025 at 04:08:33PM +0300, Dan Carpenter wrote:
> Add the GPR syscon region for the s32 chipset.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  arch/arm64/boot/dts/freescale/s32g2.dtsi | 8 ++++++++
>  arch/arm64/boot/dts/freescale/s32g3.dtsi | 8 ++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> index 51d00dac12de..3c9472f6c174 100644
> --- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> @@ -325,6 +325,13 @@ usdhc0-200mhz-grp4 {
>  			};
>  		};
>
> +		gpr: syscon@4007c000 {
> +			compatible = "nxp,s32-gpr", "syscon";
> +			reg = <0x4007c000 0x3000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +		};
> +

Please cc whole thread to imx@lists.linux.dev.

I think it is not good method by using syscon here.

Suppose using standard phy interface or mux controller interface.

Frank

>  		ocotp: nvmem@400a4000 {
>  			compatible = "nxp,s32g2-ocotp";
>  			reg = <0x400a4000 0x400>;
> @@ -731,6 +738,7 @@ gmac0: ethernet@4033c000 {
>  			compatible = "nxp,s32g2-dwmac";
>  			reg = <0x4033c000 0x2000>, /* gmac IP */
>  			      <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
> +			phy-sel = <&gpr 0x4>;
>  			interrupt-parent = <&gic>;
>  			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
>  			interrupt-names = "macirq";
> diff --git a/arch/arm64/boot/dts/freescale/s32g3.dtsi b/arch/arm64/boot/dts/freescale/s32g3.dtsi
> index eff7673e7f34..0ceca3caf133 100644
> --- a/arch/arm64/boot/dts/freescale/s32g3.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32g3.dtsi
> @@ -383,6 +383,13 @@ usdhc0-200mhz-grp4 {
>  			};
>  		};
>
> +		gpr: syscon@4007c000 {
> +			compatible = "nxp,s32-gpr", "syscon";
> +			reg = <0x4007c000 0x3000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +		};
> +
>  		ocotp: nvmem@400a4000 {
>  			compatible = "nxp,s32g3-ocotp", "nxp,s32g2-ocotp";
>  			reg = <0x400a4000 0x400>;
> @@ -808,6 +815,7 @@ gmac0: ethernet@4033c000 {
>  			compatible = "nxp,s32g2-dwmac";
>  			reg = <0x4033c000 0x2000>, /* gmac IP */
>  			      <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
> +			phy-sel = <&gpr 0x4>;
>  			interrupt-parent = <&gic>;
>  			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
>  			interrupt-names = "macirq";
> --
> 2.51.0
>

Re: [PATCH 4/4] dts: s32g: Add GPR syscon region

[Russell King (Oracle)]

On Mon, Dec 01, 2025 at 11:42:00AM -0500, Frank Li wrote:
> On Mon, Dec 01, 2025 at 04:08:33PM +0300, Dan Carpenter wrote:
> > Add the GPR syscon region for the s32 chipset.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  arch/arm64/boot/dts/freescale/s32g2.dtsi | 8 ++++++++
> >  arch/arm64/boot/dts/freescale/s32g3.dtsi | 8 ++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > index 51d00dac12de..3c9472f6c174 100644
> > --- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > @@ -325,6 +325,13 @@ usdhc0-200mhz-grp4 {
> >  			};
> >  		};
> >
> > +		gpr: syscon@4007c000 {
> > +			compatible = "nxp,s32-gpr", "syscon";
> > +			reg = <0x4007c000 0x3000>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +		};
> > +
> 
> Please cc whole thread to imx@lists.linux.dev.
> 
> I think it is not good method by using syscon here.
> 
> Suppose using standard phy interface or mux controller interface.

I rather disagree, but I would like to see the definition of the
"ctrl_sts" register. The driver defines:

/* SoC PHY interface control register */
#define S32_PHY_INTF_SEL_MII    0x00
#define S32_PHY_INTF_SEL_SGMII  0x01
#define S32_PHY_INTF_SEL_RGMII  0x02
#define S32_PHY_INTF_SEL_RMII   0x08

This is mostly equivalent to the phy_intf_sel_i[2:0] input to the GMAC
block, who's bit combinations are defined by the PHY_INTF_SEL_xxx
constants. These seem to correspond to register bits 3:1, but with
the GMAC being configured for MII mode with an external SGMII PCS
when bit 0 is set.

If this is true, then no, there is no "phy" as such, and if we go
down the route of modelling the GMAC's phy_intf_sel_i[2:0] inputs
as a "phy" then we're going to end up with something that's a
drivers/phy PHY before a real seperate PHY for providing the
SGMII/1000BASE-X signalling. This falls into the category of needless
over-complication with no benefit.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 0/4] s32g: Use a syscon for GPR

[Dan Carpenter]

On Mon, Dec 01, 2025 at 04:08:14PM +0300, Dan Carpenter wrote:
> *** BLURB HERE ***
> 

Sorry, I obviously meant to write a message here.

The s32g devices have a GPR register region which could be accessed
via a syscon.  Currently only the stmmac/dwmac-s32.c uses anything
from there and we just add a line to the device tree to access
that GMAC_0_CTRL_STS register:

			reg = <0x4033c000 0x2000>, /* gmac IP */
			      <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */

But it would be better to have a syscon instead of adding each
register to the device tree like this.

We still have to maintain backwards compatibility to this format,
of course.

regards,
dan carpenter