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

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

[Dan Carpenter]

The s32g devices have a GPR register region which holds a number of
miscellaneous registers.  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 */

We still have to maintain backwards compatibility to this format,
of course, but it would be better to access these through a syscon.
First of all, putting all the registers together is more organized
and shows how the hardware actually is implemented.  Secondly, in
some versions of this chipset those registers can only be accessed
via SCMI, if the registers aren't grouped together each driver will
have to create a whole lot of if then statements to access it via
IOMEM or via SCMI, where if we use a syscon interface we can write
a driver to handle that quite transparently without modifying each
individual driver which reads or writes to one of these registers.
That code is out of tree for now, but eventually we'll want to
support this.

Changed since v1:
* Add imx@lists.linux.dev to the CC list.
* Fix forward porting bug.  s/PHY_INTF_SEL_RGMII/S32_PHY_INTF_SEL_RGMII/
* Use the correct SoC names nxp,s32g2-gpr and nxp,s32g3-gpr instead of
  nxp,s32g-gpr which is the SoC family.
* Fix the phandle name by adding the vendor prefix
* Fix the documentation for the phandle
* Remove #address-cells and #size-cells from the syscon block

Here is the whole list of registers in the GPR region

Starting 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)

Starting 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)

Starting from 4007C800

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

Starting from 4007CB00

4 WKUP Pad Pullup/Pulldown Select (WKUP_PUS)

Dan Carpenter (4):
  net: stmmac: s32: use a syscon for S32_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       |  4 ++++
 .../bindings/net/nxp,s32-dwmac.yaml           | 10 ++++++++
 arch/arm64/boot/dts/freescale/s32g2.dtsi      |  6 +++++
 arch/arm64/boot/dts/freescale/s32g3.dtsi      |  6 +++++
 .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
 5 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.51.0

[PATCH v2 1/4] net: stmmac: s32: use a syscon for S32_PHY_INTF_SEL_RGMII

[Dan Carpenter]

On the s32 chipsets 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 ugly because we have 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>
---
Fix forward porting bug.  s/PHY_INTF_SEL_RGMII/S32_PHY_INTF_SEL_RGMII/

 .../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..2e6bb41f49e1 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, S32_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,
+					"nxp,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

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

[Dan Carpenter]

The S32 chipsets have 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 it's cleaner to use a
syscon interface to access these registers.

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>
---
v2: Add the vendor prefix to the phandle
    Fix the documentation

 .../devicetree/bindings/net/nxp,s32-dwmac.yaml         | 10 ++++++++++
 1 file changed, 10 insertions(+)

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

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

[Frank Li]

On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> The s32g devices have a GPR register region which holds a number of
> miscellaneous registers.  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 */
>
> We still have to maintain backwards compatibility to this format,
> of course, but it would be better to access these through a syscon.
> First of all, putting all the registers together is more organized
> and shows how the hardware actually is implemented.  Secondly, in
> some versions of this chipset those registers can only be accessed
> via SCMI, if the registers aren't grouped together each driver will
> have to create a whole lot of if then statements to access it via
> IOMEM or via SCMI,

Does SCMI work as regmap? syscon look likes simple, but missed abstract
in overall.

You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */

regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);

So all code can use regmap function without if-then statements if SCMI work
as regmap.

Frank


>where if we use a syscon interface we can write
> a driver to handle that quite transparently without modifying each
> individual driver which reads or writes to one of these registers.
> That code is out of tree for now, but eventually we'll want to
> support this.
>
> Changed since v1:
> * Add imx@lists.linux.dev to the CC list.
> * Fix forward porting bug.  s/PHY_INTF_SEL_RGMII/S32_PHY_INTF_SEL_RGMII/
> * Use the correct SoC names nxp,s32g2-gpr and nxp,s32g3-gpr instead of
>   nxp,s32g-gpr which is the SoC family.
> * Fix the phandle name by adding the vendor prefix
> * Fix the documentation for the phandle
> * Remove #address-cells and #size-cells from the syscon block
>
> Here is the whole list of registers in the GPR region
>
> Starting 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)
>
> Starting 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)
>
> Starting from 4007C800
>
> 4  GPR01 Register (GPR01)
> 30 GPR12 Register (GPR12)
> 58 GPR22 Register (GPR22)
> 70 GPR28 Register (GPR28)
> 74 GPR29 Register (GPR29)
>
> Starting from 4007CB00
>
> 4 WKUP Pad Pullup/Pulldown Select (WKUP_PUS)
>
> Dan Carpenter (4):
>   net: stmmac: s32: use a syscon for S32_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       |  4 ++++
>  .../bindings/net/nxp,s32-dwmac.yaml           | 10 ++++++++
>  arch/arm64/boot/dts/freescale/s32g2.dtsi      |  6 +++++
>  arch/arm64/boot/dts/freescale/s32g3.dtsi      |  6 +++++
>  .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
>  5 files changed, 44 insertions(+), 5 deletions(-)
>
> --
> 2.51.0
>

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

[Dan Carpenter]

On Mon, Dec 15, 2025 at 10:56:49AM -0500, Frank Li wrote:
> On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> > The s32g devices have a GPR register region which holds a number of
> > miscellaneous registers.  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 */
> >
> > We still have to maintain backwards compatibility to this format,
> > of course, but it would be better to access these through a syscon.
> > First of all, putting all the registers together is more organized
> > and shows how the hardware actually is implemented.  Secondly, in
> > some versions of this chipset those registers can only be accessed
> > via SCMI, if the registers aren't grouped together each driver will
> > have to create a whole lot of if then statements to access it via
> > IOMEM or via SCMI,
> 
> Does SCMI work as regmap? syscon look likes simple, but missed abstract
> in overall.
> 

The SCMI part of this is pretty complicated and needs discussion.  It
might be that it requires a vendor extension.  Right now, the out of
tree code uses a nvmem vendor extension but that probably won't get
merged upstream.

But in theory, it's fairly simple, you can write a regmap driver and
register it as a syscon and everything that was accessing nxp,phy-sel
accesses the same register but over SCMI.

> You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */
> 
> regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);
> 

You can use have an MMIO syscon, or you can create a custom driver
and register it as a syscon using of_syscon_register_regmap().

> So all code can use regmap function without if-then statements if SCMI work
> as regmap.
> 

regards,
dan carpenter

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

[Frank Li]

On Mon, Dec 15, 2025 at 09:33:54PM +0300, Dan Carpenter wrote:
> On Mon, Dec 15, 2025 at 10:56:49AM -0500, Frank Li wrote:
> > On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> > > The s32g devices have a GPR register region which holds a number of
> > > miscellaneous registers.  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 */
> > >
> > > We still have to maintain backwards compatibility to this format,
> > > of course, but it would be better to access these through a syscon.
> > > First of all, putting all the registers together is more organized
> > > and shows how the hardware actually is implemented.  Secondly, in
> > > some versions of this chipset those registers can only be accessed
> > > via SCMI, if the registers aren't grouped together each driver will
> > > have to create a whole lot of if then statements to access it via
> > > IOMEM or via SCMI,
> >
> > Does SCMI work as regmap? syscon look likes simple, but missed abstract
> > in overall.
> >
>
> The SCMI part of this is pretty complicated and needs discussion.  It
> might be that it requires a vendor extension.  Right now, the out of
> tree code uses a nvmem vendor extension but that probably won't get
> merged upstream.
>
> But in theory, it's fairly simple, you can write a regmap driver and
> register it as a syscon and everything that was accessing nxp,phy-sel
> accesses the same register but over SCMI.

nxp,phy-sel is not standard API. Driver access raw register value. such
as write 1 to offset 0x100.

After change to SCMI, which may mapped to difference command. Even change
to other SOC, value and offset also need be changed. It is not standilzed
as what you expected.

>
> > You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */
> >
> > regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);
> >
>
> You can use have an MMIO syscon, or you can create a custom driver
> and register it as a syscon using of_syscon_register_regmap().

My means is that it is not necessary to create nxp,phy-sel, especially
there already have <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */

Frank

>
> > So all code can use regmap function without if-then statements if SCMI work
> > as regmap.
> >
>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

On Mon, Dec 15, 2025 at 02:28:43PM -0500, Frank Li wrote:
> On Mon, Dec 15, 2025 at 09:33:54PM +0300, Dan Carpenter wrote:
> > On Mon, Dec 15, 2025 at 10:56:49AM -0500, Frank Li wrote:
> > > On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> > > > The s32g devices have a GPR register region which holds a number of
> > > > miscellaneous registers.  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 */
> > > >
> > > > We still have to maintain backwards compatibility to this format,
> > > > of course, but it would be better to access these through a syscon.
> > > > First of all, putting all the registers together is more organized
> > > > and shows how the hardware actually is implemented.  Secondly, in
> > > > some versions of this chipset those registers can only be accessed
> > > > via SCMI, if the registers aren't grouped together each driver will
> > > > have to create a whole lot of if then statements to access it via
> > > > IOMEM or via SCMI,
> > >
> > > Does SCMI work as regmap? syscon look likes simple, but missed abstract
> > > in overall.
> > >
> >
> > The SCMI part of this is pretty complicated and needs discussion.  It
> > might be that it requires a vendor extension.  Right now, the out of
> > tree code uses a nvmem vendor extension but that probably won't get
> > merged upstream.
> >
> > But in theory, it's fairly simple, you can write a regmap driver and
> > register it as a syscon and everything that was accessing nxp,phy-sel
> > accesses the same register but over SCMI.
> 
> nxp,phy-sel is not standard API. Driver access raw register value. such
> as write 1 to offset 0x100.
> 
> After change to SCMI, which may mapped to difference command. Even change
> to other SOC, value and offset also need be changed. It is not standilzed
> as what you expected.

We're writing to an offset in a syscon.  Right now the device tree
says that the syscon is an MMIO syscon.  But for SCMI devices we
would point the phandle to a custom syscon.  The phandle and the offset
would stay the same, but how the syscon is implemented would change.

> 
> >
> > > You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */
> > >
> > > regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);
> > >
> >
> > You can use have an MMIO syscon, or you can create a custom driver
> > and register it as a syscon using of_syscon_register_regmap().
> 
> My means is that it is not necessary to create nxp,phy-sel, especially
> there already have <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
> 

Right now the out of tree dwmac-s32cc.c driver does something like
this:

    89          if (gmac->use_nvmem) {
    90                  ret = write_nvmem_cell(gmac->dev, "gmac_phy_intf_sel", intf_sel);
    91                  if (ret)
    92                          return ret;
    93          } else {
    94                  writel(intf_sel, gmac->ctrl_sts);
    95          }

Which is quite complicated, but with a syscon, then it's just:

	regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII);

Even without SCMI, the hardware has all these registers grouped together
it just feels cleaner to group them together in the device tree as well.

regards,
dan carpenter

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

[Frank Li]

On Mon, Dec 15, 2025 at 11:11:03PM +0300, Dan Carpenter wrote:
> On Mon, Dec 15, 2025 at 02:28:43PM -0500, Frank Li wrote:
> > On Mon, Dec 15, 2025 at 09:33:54PM +0300, Dan Carpenter wrote:
> > > On Mon, Dec 15, 2025 at 10:56:49AM -0500, Frank Li wrote:
> > > > On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> > > > > The s32g devices have a GPR register region which holds a number of
> > > > > miscellaneous registers.  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 */
> > > > >
> > > > > We still have to maintain backwards compatibility to this format,
> > > > > of course, but it would be better to access these through a syscon.
> > > > > First of all, putting all the registers together is more organized
> > > > > and shows how the hardware actually is implemented.  Secondly, in
> > > > > some versions of this chipset those registers can only be accessed
> > > > > via SCMI, if the registers aren't grouped together each driver will
> > > > > have to create a whole lot of if then statements to access it via
> > > > > IOMEM or via SCMI,
> > > >
> > > > Does SCMI work as regmap? syscon look likes simple, but missed abstract
> > > > in overall.
> > > >
> > >
> > > The SCMI part of this is pretty complicated and needs discussion.  It
> > > might be that it requires a vendor extension.  Right now, the out of
> > > tree code uses a nvmem vendor extension but that probably won't get
> > > merged upstream.
> > >
> > > But in theory, it's fairly simple, you can write a regmap driver and
> > > register it as a syscon and everything that was accessing nxp,phy-sel
> > > accesses the same register but over SCMI.
> >
> > nxp,phy-sel is not standard API. Driver access raw register value. such
> > as write 1 to offset 0x100.
> >
> > After change to SCMI, which may mapped to difference command. Even change
> > to other SOC, value and offset also need be changed. It is not standilzed
> > as what you expected.
>
> We're writing to an offset in a syscon.  Right now the device tree
> says that the syscon is an MMIO syscon.  But for SCMI devices we
> would point the phandle to a custom syscon.  The phandle and the offset
> would stay the same, but how the syscon is implemented would change.

Your SCMI syscon driver will convert some private hard code to some
function, such previous example's '1' as SEL_RGMII. It is hard maintained
in long term.

>
> >
> > >
> > > > You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */
> > > >
> > > > regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);
> > > >
> > >
> > > You can use have an MMIO syscon, or you can create a custom driver
> > > and register it as a syscon using of_syscon_register_regmap().
> >
> > My means is that it is not necessary to create nxp,phy-sel, especially
> > there already have <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
> >
>
> Right now the out of tree dwmac-s32cc.c driver does something like
> this:
>
>     89          if (gmac->use_nvmem) {
>     90                  ret = write_nvmem_cell(gmac->dev, "gmac_phy_intf_sel", intf_sel);
>     91                  if (ret)
>     92                          return ret;
>     93          } else {
>     94                  writel(intf_sel, gmac->ctrl_sts);
>     95          }
>
> Which is quite complicated, but with a syscon, then it's just:
>
> 	regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII);
>
> Even without SCMI, the hardware has all these registers grouped together
> it just feels cleaner to group them together in the device tree as well.

Why not implement standard phy interface,
phy_set_mode_ext(PHY_MODE_ETHERNET, RGMII);

For example:  drivers/pci/controller/dwc/pci-imx6.c

In legency platform, it use syscon to set some registers. It becomes mess
when more platform added.  And it becomes hard to convert because avoid
break compatibltiy now.

It doesn't become worse since new platforms switched to use standard
inteface, (phy, reset ...).

Frank

>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

On Mon, Dec 15, 2025 at 04:07:48PM -0500, Frank Li wrote:
> On Mon, Dec 15, 2025 at 11:11:03PM +0300, Dan Carpenter wrote:
> > On Mon, Dec 15, 2025 at 02:28:43PM -0500, Frank Li wrote:
> > > On Mon, Dec 15, 2025 at 09:33:54PM +0300, Dan Carpenter wrote:
> > > > On Mon, Dec 15, 2025 at 10:56:49AM -0500, Frank Li wrote:
> > > > > On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> > > > > > The s32g devices have a GPR register region which holds a number of
> > > > > > miscellaneous registers.  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 */
> > > > > >
> > > > > > We still have to maintain backwards compatibility to this format,
> > > > > > of course, but it would be better to access these through a syscon.
> > > > > > First of all, putting all the registers together is more organized
> > > > > > and shows how the hardware actually is implemented.  Secondly, in
> > > > > > some versions of this chipset those registers can only be accessed
> > > > > > via SCMI, if the registers aren't grouped together each driver will
> > > > > > have to create a whole lot of if then statements to access it via
> > > > > > IOMEM or via SCMI,
> > > > >
> > > > > Does SCMI work as regmap? syscon look likes simple, but missed abstract
> > > > > in overall.
> > > > >
> > > >
> > > > The SCMI part of this is pretty complicated and needs discussion.  It
> > > > might be that it requires a vendor extension.  Right now, the out of
> > > > tree code uses a nvmem vendor extension but that probably won't get
> > > > merged upstream.
> > > >
> > > > But in theory, it's fairly simple, you can write a regmap driver and
> > > > register it as a syscon and everything that was accessing nxp,phy-sel
> > > > accesses the same register but over SCMI.
> > >
> > > nxp,phy-sel is not standard API. Driver access raw register value. such
> > > as write 1 to offset 0x100.
> > >
> > > After change to SCMI, which may mapped to difference command. Even change
> > > to other SOC, value and offset also need be changed. It is not standilzed
> > > as what you expected.
> >
> > We're writing to an offset in a syscon.  Right now the device tree
> > says that the syscon is an MMIO syscon.  But for SCMI devices we
> > would point the phandle to a custom syscon.  The phandle and the offset
> > would stay the same, but how the syscon is implemented would change.
> 
> Your SCMI syscon driver will convert some private hard code to some
> function, such previous example's '1' as SEL_RGMII. It is hard maintained
> in long term.
> 

No, there isn't any conversion needed.  It's exactly the same as writing
to the register except it goes through SCMI.

> >
> > >
> > > >
> > > > > You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */
> > > > >
> > > > > regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);
> > > > >
> > > >
> > > > You can use have an MMIO syscon, or you can create a custom driver
> > > > and register it as a syscon using of_syscon_register_regmap().
> > >
> > > My means is that it is not necessary to create nxp,phy-sel, especially
> > > there already have <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
> > >
> >
> > Right now the out of tree dwmac-s32cc.c driver does something like
> > this:
> >
> >     89          if (gmac->use_nvmem) {
> >     90                  ret = write_nvmem_cell(gmac->dev, "gmac_phy_intf_sel", intf_sel);
> >     91                  if (ret)
> >     92                          return ret;
> >     93          } else {
> >     94                  writel(intf_sel, gmac->ctrl_sts);
> >     95          }
> >
> > Which is quite complicated, but with a syscon, then it's just:
> >
> > 	regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII);
> >
> > Even without SCMI, the hardware has all these registers grouped together
> > it just feels cleaner to group them together in the device tree as well.
> 
> Why not implement standard phy interface,
> phy_set_mode_ext(PHY_MODE_ETHERNET, RGMII);
> 
> For example:  drivers/pci/controller/dwc/pci-imx6.c
> 
> In legency platform, it use syscon to set some registers. It becomes mess
> when more platform added.  And it becomes hard to convert because avoid
> break compatibltiy now.
> 
> It doesn't become worse since new platforms switched to use standard
> inteface, (phy, reset ...).
> 

This happens below that layer, this is just saying where the registers
are found.  The GMAC_0_CTRL_STS is just one register in the GPR region,
most of the others are unrelated to PHY.

regards,
dan carpenter

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

[Frank Li]

On Tue, Dec 16, 2025 at 10:56:02AM +0300, Dan Carpenter wrote:
> On Mon, Dec 15, 2025 at 04:07:48PM -0500, Frank Li wrote:
> > On Mon, Dec 15, 2025 at 11:11:03PM +0300, Dan Carpenter wrote:
> > > On Mon, Dec 15, 2025 at 02:28:43PM -0500, Frank Li wrote:
> > > > On Mon, Dec 15, 2025 at 09:33:54PM +0300, Dan Carpenter wrote:
> > > > > On Mon, Dec 15, 2025 at 10:56:49AM -0500, Frank Li wrote:
> > > > > > On Mon, Dec 15, 2025 at 05:41:43PM +0300, Dan Carpenter wrote:
> > > > > > > The s32g devices have a GPR register region which holds a number of
> > > > > > > miscellaneous registers.  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 */
> > > > > > >
> > > > > > > We still have to maintain backwards compatibility to this format,
> > > > > > > of course, but it would be better to access these through a syscon.
> > > > > > > First of all, putting all the registers together is more organized
> > > > > > > and shows how the hardware actually is implemented.  Secondly, in
> > > > > > > some versions of this chipset those registers can only be accessed
> > > > > > > via SCMI, if the registers aren't grouped together each driver will
> > > > > > > have to create a whole lot of if then statements to access it via
> > > > > > > IOMEM or via SCMI,
> > > > > >
> > > > > > Does SCMI work as regmap? syscon look likes simple, but missed abstract
> > > > > > in overall.
> > > > > >
> > > > >
> > > > > The SCMI part of this is pretty complicated and needs discussion.  It
> > > > > might be that it requires a vendor extension.  Right now, the out of
> > > > > tree code uses a nvmem vendor extension but that probably won't get
> > > > > merged upstream.
> > > > >
> > > > > But in theory, it's fairly simple, you can write a regmap driver and
> > > > > register it as a syscon and everything that was accessing nxp,phy-sel
> > > > > accesses the same register but over SCMI.
> > > >
> > > > nxp,phy-sel is not standard API. Driver access raw register value. such
> > > > as write 1 to offset 0x100.
> > > >
> > > > After change to SCMI, which may mapped to difference command. Even change
> > > > to other SOC, value and offset also need be changed. It is not standilzed
> > > > as what you expected.
> > >
> > > We're writing to an offset in a syscon.  Right now the device tree
> > > says that the syscon is an MMIO syscon.  But for SCMI devices we
> > > would point the phandle to a custom syscon.  The phandle and the offset
> > > would stay the same, but how the syscon is implemented would change.
> >
> > Your SCMI syscon driver will convert some private hard code to some
> > function, such previous example's '1' as SEL_RGMII. It is hard maintained
> > in long term.
> >
>
> No, there isn't any conversion needed.  It's exactly the same as writing
> to the register except it goes through SCMI.
>
> > >
> > > >
> > > > >
> > > > > > You still use regmap by use MMIO. /* GMAC_0_CTRL_STS */
> > > > > >
> > > > > > regmap = devm_regmap_init_mmio(dev, sts_offset, &regmap_config);
> > > > > >
> > > > >
> > > > > You can use have an MMIO syscon, or you can create a custom driver
> > > > > and register it as a syscon using of_syscon_register_regmap().
> > > >
> > > > My means is that it is not necessary to create nxp,phy-sel, especially
> > > > there already have <0x4007c004 0x4>;    /* GMAC_0_CTRL_STS */
> > > >
> > >
> > > Right now the out of tree dwmac-s32cc.c driver does something like
> > > this:
> > >
> > >     89          if (gmac->use_nvmem) {
> > >     90                  ret = write_nvmem_cell(gmac->dev, "gmac_phy_intf_sel", intf_sel);
> > >     91                  if (ret)
> > >     92                          return ret;
> > >     93          } else {
> > >     94                  writel(intf_sel, gmac->ctrl_sts);
> > >     95          }
> > >
> > > Which is quite complicated, but with a syscon, then it's just:
> > >
> > > 	regmap_write(gmac->sts_regmap, gmac->sts_offset, S32_PHY_INTF_SEL_RGMII);
> > >
> > > Even without SCMI, the hardware has all these registers grouped together
> > > it just feels cleaner to group them together in the device tree as well.
> >
> > Why not implement standard phy interface,
> > phy_set_mode_ext(PHY_MODE_ETHERNET, RGMII);
> >
> > For example:  drivers/pci/controller/dwc/pci-imx6.c
> >
> > In legency platform, it use syscon to set some registers. It becomes mess
> > when more platform added.  And it becomes hard to convert because avoid
> > break compatibltiy now.
> >
> > It doesn't become worse since new platforms switched to use standard
> > inteface, (phy, reset ...).
> >
>
> This happens below that layer, this is just saying where the registers
> are found.  The GMAC_0_CTRL_STS is just one register in the GPR region,
> most of the others are unrelated to PHY.

The other register should work as other function's providor with mfd.

Frank

>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

On Tue, Dec 16, 2025 at 09:42:06AM -0500, Frank Li wrote:
> > >
> > > Why not implement standard phy interface,
> > > phy_set_mode_ext(PHY_MODE_ETHERNET, RGMII);
> > >
> > > For example:  drivers/pci/controller/dwc/pci-imx6.c
> > >
> > > In legency platform, it use syscon to set some registers. It becomes mess
> > > when more platform added.  And it becomes hard to convert because avoid
> > > break compatibltiy now.
> > >
> > > It doesn't become worse since new platforms switched to use standard
> > > inteface, (phy, reset ...).
> > >
> >
> > This happens below that layer, this is just saying where the registers
> > are found.  The GMAC_0_CTRL_STS is just one register in the GPR region,
> > most of the others are unrelated to PHY.
> 
> The other register should work as other function's providor with mfd.
> 

Syscons are a really standard way to do register accesses.  The
pci-imx6.c driver you mentioned earlier does it that way...  The only
thing which my code does differently is I put the offset into the
phandle, but that's not so unusual and it's arguably a cleaner way
because now both the base address and offset are in the same file.

regards,
dan carpenter

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

[Krzysztof Kozlowski]

On Mon, Dec 15, 2025 at 05:41:57PM +0300, Dan Carpenter wrote:
> The S32 chipsets have 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 it's cleaner to use a
> syscon interface to access these registers.
> 
> 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>
> ---
> v2: Add the vendor prefix to the phandle
>     Fix the documentation
> 
>  .../devicetree/bindings/net/nxp,s32-dwmac.yaml         | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> index 2b8b74c5feec..a65036806d60 100644
> --- a/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> @@ -32,6 +32,15 @@ properties:
>        - description: Main GMAC registers
>        - description: GMAC PHY mode control register
>  
> +  nxp,phy-sel:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - description: phandle to the GPR syscon node
> +      - description: offset of PHY selection register
> +    description:
> +      This is a phandle/offset pair.  The phandle points to the
> +      GPR region and the offset is the GMAC_0_CTRL_STS register.

Do not repeat description twice. The GMAC_0_CTRL_STS should be explained
in description of individual item. This description should only say what
is the purpose of it, why the hardware needs to poke in other devices.

Best regards,
Krzysztof

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

[Frank Li]

On Tue, Dec 16, 2025 at 09:30:08PM +0300, Dan Carpenter wrote:
> On Tue, Dec 16, 2025 at 09:42:06AM -0500, Frank Li wrote:
> > > >
> > > > Why not implement standard phy interface,
> > > > phy_set_mode_ext(PHY_MODE_ETHERNET, RGMII);
> > > >
> > > > For example:  drivers/pci/controller/dwc/pci-imx6.c
> > > >
> > > > In legency platform, it use syscon to set some registers. It becomes mess
> > > > when more platform added.  And it becomes hard to convert because avoid
> > > > break compatibltiy now.
> > > >
> > > > It doesn't become worse since new platforms switched to use standard
> > > > inteface, (phy, reset ...).
> > > >
> > >
> > > This happens below that layer, this is just saying where the registers
> > > are found.  The GMAC_0_CTRL_STS is just one register in the GPR region,
> > > most of the others are unrelated to PHY.
> >
> > The other register should work as other function's providor with mfd.
> >
>
> Syscons are a really standard way to do register accesses.

It is quite like back door. Many clock/reset also use phandle to node to
controller by raw register read/write.

> The
> pci-imx6.c driver you mentioned earlier does it that way...

It is not preferred when we tried to add new one. Give me some time to look
for original threads.

> The only
> thing which my code does differently is I put the offset into the
> phandle, but that's not so unusual and it's arguably a cleaner way
> because now both the base address and offset are in the same file.

It is not big deal about offset. The key is if use phande to direct access
other module's register.

Frank
>
> regards,
> dan carpenter
>