[PATCH v2 0/4] RTL9300 MDIO driver

[PATCH v2 0/4] RTL9300 MDIO driver

[Chris Packham]

This series adds a driver for the MDIO controller on the RTL9300 family
of devices. The controller is a little unique in that we can't access
the SMI interfaces directly. Instead we associate the SMI interface with
a switch port and use the port number to address the SMI bus in
software.

Chris Packham (4):
  dt-bindings: net: Add Realtek MDIO controller
  dt-bindings: mfd: Add MDIO interface to rtl9301-switch
  mips: dts: realtek: Add MDIO controller
  net: mdio: Add RTL9300 MDIO driver

 .../bindings/mfd/realtek,rtl9301-switch.yaml  |  15 +
 .../bindings/net/realtek,rtl9301-mdio.yaml    |  82 +++++
 arch/mips/boot/dts/realtek/rtl930x.dtsi       |   8 +
 drivers/net/mdio/Kconfig                      |   7 +
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/mdio-realtek-rtl.c           | 341 ++++++++++++++++++
 6 files changed, 454 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
 create mode 100644 drivers/net/mdio/mdio-realtek-rtl.c

-- 
2.47.1

[PATCH v2 1/4] dt-bindings: net: Add Realtek MDIO controller

[Chris Packham]

Add dtschema for the MDIO controller found in the RTL9300 SoCs. The
controller is slightly unusual in that direct MDIO communication is not
possible. Instead, the SMI bus and PHY address are associated with a
switch port and the port number is used when talking to the PHY.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - None

 .../bindings/net/realtek,rtl9301-mdio.yaml    | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml

diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
new file mode 100644
index 000000000000..95ed77ff8dcc
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/realtek,rtl9301-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL9300 MDIO Controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+allOf:
+  - $ref: mdio.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - realtek,rtl9302b-mdio
+              - realtek,rtl9302c-mdio
+              - realtek,rtl9303-mdio
+          - const: realtek,rtl9301-mdio
+      - const: realtek,rtl9301-mdio
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  '^ethernet-phy(@[a-f0-9]+)?':
+    type: object
+    $ref: ethernet-phy.yaml#
+
+    properties:
+      reg:
+        description:
+          The MDIO communication on the RTL9300 is abstracted by the switch. At
+          the software level communication uses the switch port to address the
+          PHY with the actual MDIO bus and address having been setup via the
+          realtek,smi-address property.
+
+      realtek,smi-address:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        description: SMI interface and address for the connected PHY
+        items:
+          - description: SMI interface number associated with the port.
+          - description: SMI address of the PHY for the port.
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio@ca00 {
+        compatible = "realtek,rtl9301-mdio";
+        reg = <0xca00 0x200>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+            compatible = "ethernet-phy-ieee802.3-c45";
+            reg = <0>;
+            realtek,smi-address = <0 1>;
+        };
+
+        ethernet-phy@8 {
+            compatible = "ethernet-phy-ieee802.3-c45";
+            reg = <8>;
+            realtek,smi-address = <1 1>;
+        };
+    };
-- 
2.47.1

Re: [PATCH v2 1/4] dt-bindings: net: Add Realtek MDIO controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

On Mon, Dec 16, 2024 at 04:13:43PM +1300, Chris Packham wrote:
> Add dtschema for the MDIO controller found in the RTL9300 SoCs. The
> controller is slightly unusual in that direct MDIO communication is not
> possible. Instead, the SMI bus and PHY address are associated with a
> switch port and the port number is used when talking to the PHY.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> +      realtek,smi-address:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description: SMI interface and address for the connected PHY
> +        items:
> +          - description: SMI interface number associated with the port.
> +          - description: SMI address of the PHY for the port.


I don't really understand this property, but I also don't understand the
MDIO bus, so with that caveat
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/4] dt-bindings: net: Add Realtek MDIO controller

[Chris Packham]

On 17/12/2024 07:52, Conor Dooley wrote:
> On Mon, Dec 16, 2024 at 04:13:43PM +1300, Chris Packham wrote:
>> Add dtschema for the MDIO controller found in the RTL9300 SoCs. The
>> controller is slightly unusual in that direct MDIO communication is not
>> possible. Instead, the SMI bus and PHY address are associated with a
>> switch port and the port number is used when talking to the PHY.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +      realtek,smi-address:
>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>> +        description: SMI interface and address for the connected PHY
>> +        items:
>> +          - description: SMI interface number associated with the port.
>> +          - description: SMI address of the PHY for the port.
>
> I don't really understand this property, but I also don't understand the
> MDIO bus, so with that caveat
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I'll try to clarify here as it may be of relevance to other reviewers, 
if any of this should go in the commit message or the binding let me know.

The MDIO bus is used to manage one or more network PHYs. Sometimes there 
is an MDIO interface as part of a NIC controller but it's become 
increasingly common to have a the MDIO controller separated from the 
Ethernet controller, particularly when there are multiple Ethernet 
controllers in a SoC. In the device trees there is a usually a node for 
the MDIO controller and the attached PHYs are child nodes. The Ethernet 
interface has phandle property which references the attached PHY.

The RTL9300 (and similar Realtek Ethernet switches) don't directly 
expose the MDIO interface to us. There seems to be an internal PHY 
polling mechanism and the user access to the PHYs works in conjunction 
with that. So rather than being able to reference PHYs and MDIO 
interfaces directly we need to work with switch port numbers instead. 
The actual hardware MDIO bus and PHY address is captured in the 
"realtek,smi-address" property.

[PATCH v2 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Chris Packham]

The MDIO controller is part of the switch on the RTL9300 family of
devices. Add a $ref to the mfd binding for these devices.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - None

 .../bindings/mfd/realtek,rtl9301-switch.yaml      | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
index f053303ab1e6..eeb08e7435fa 100644
--- a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
+++ b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
@@ -41,6 +41,9 @@ patternProperties:
   'i2c@[0-9a-f]+$':
     $ref: /schemas/i2c/realtek,rtl9301-i2c.yaml#
 
+  'mdio@[0-9a-f]+$':
+    $ref: /schemas/net/realtek,rtl9301-mdio.yaml#
+
 required:
   - compatible
   - reg
@@ -110,5 +113,17 @@ examples:
           };
         };
       };
+
+      mdio0: mdio@ca00 {
+        compatible = "realtek,rtl9301-mdio";
+        reg = <0xca00 0x200>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+          reg = <0>;
+          realtek,smi-address = <0 1>;
+        };
+      };
     };
 
-- 
2.47.1

Re: [PATCH v2 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

On Mon, Dec 16, 2024 at 04:13:44PM +1300, Chris Packham wrote:
> The MDIO controller is part of the switch on the RTL9300 family of
> devices. Add a $ref to the mfd binding for these devices.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - None
> 
>  .../bindings/mfd/realtek,rtl9301-switch.yaml      | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
> index f053303ab1e6..eeb08e7435fa 100644
> --- a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
> +++ b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
> @@ -41,6 +41,9 @@ patternProperties:
>    'i2c@[0-9a-f]+$':
>      $ref: /schemas/i2c/realtek,rtl9301-i2c.yaml#
>  
> +  'mdio@[0-9a-f]+$':
> +    $ref: /schemas/net/realtek,rtl9301-mdio.yaml#
> +
>  required:
>    - compatible
>    - reg
> @@ -110,5 +113,17 @@ examples:
>            };
>          };
>        };
> +
> +      mdio0: mdio@ca00 {

Label here is unused, but that alone isn't worth a respin.
Acked-by: Conor Dooley <conor.dooley@microchip.com>

> +        compatible = "realtek,rtl9301-mdio";
> +        reg = <0xca00 0x200>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@0 {
> +          reg = <0>;
> +          realtek,smi-address = <0 1>;
> +        };
> +      };
>      };
>  
> -- 
> 2.47.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Chris Packham]

On 17/12/2024 07:53, Conor Dooley wrote:
> On Mon, Dec 16, 2024 at 04:13:44PM +1300, Chris Packham wrote:
>> The MDIO controller is part of the switch on the RTL9300 family of
>> devices. Add a $ref to the mfd binding for these devices.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      - None
>>
>>   .../bindings/mfd/realtek,rtl9301-switch.yaml      | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
>> index f053303ab1e6..eeb08e7435fa 100644
>> --- a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
>> @@ -41,6 +41,9 @@ patternProperties:
>>     'i2c@[0-9a-f]+$':
>>       $ref: /schemas/i2c/realtek,rtl9301-i2c.yaml#
>>   
>> +  'mdio@[0-9a-f]+$':
>> +    $ref: /schemas/net/realtek,rtl9301-mdio.yaml#
>> +
>>   required:
>>     - compatible
>>     - reg
>> @@ -110,5 +113,17 @@ examples:
>>             };
>>           };
>>         };
>> +
>> +      mdio0: mdio@ca00 {
> Label here is unused, but that alone isn't worth a respin.
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

I'll be re-spinning the series for other reasons so I'll fix this up and 
add your ack while I'm at it.

>
>> +        compatible = "realtek,rtl9301-mdio";
>> +        reg = <0xca00 0x200>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        ethernet-phy@0 {
>> +          reg = <0>;
>> +          realtek,smi-address = <0 1>;
>> +        };
>> +      };
>>       };
>>   
>> -- 
>> 2.47.1
>>

Re: (subset) [PATCH v2 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Lee Jones]

On Mon, 16 Dec 2024 16:13:44 +1300, Chris Packham wrote:
> The MDIO controller is part of the switch on the RTL9300 family of
> devices. Add a $ref to the mfd binding for these devices.
> 
> 

Applied, thanks!

[2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch
      commit: 1061081cbe930f97ad54e820ad1996f55d93c57f

--
Lee Jones [李琼斯]

Re: (subset) [PATCH v2 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Chris Packham]

Hi Lee,

On 18/12/2024 04:07, Lee Jones wrote:
> On Mon, 16 Dec 2024 16:13:44 +1300, Chris Packham wrote:
>> The MDIO controller is part of the switch on the RTL9300 family of
>> devices. Add a $ref to the mfd binding for these devices.
>>
>>
> Applied, thanks!
>
> [2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch
>        commit: 1061081cbe930f97ad54e820ad1996f55d93c57f
>
> --
> Lee Jones [李琼斯]
Is it too late to drop this out? I think I'm probably going to change 
the MDIO binding a little which may change how it fits into the overall 
switch mfd.

Re: (subset) [PATCH v2 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Lee Jones]

On Thu, 19 Dec 2024, Chris Packham wrote:

> Hi Lee,
> 
> On 18/12/2024 04:07, Lee Jones wrote:
> > On Mon, 16 Dec 2024 16:13:44 +1300, Chris Packham wrote:
> >> The MDIO controller is part of the switch on the RTL9300 family of
> >> devices. Add a $ref to the mfd binding for these devices.
> >>
> >>
> > Applied, thanks!
> >
> > [2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch
> >        commit: 1061081cbe930f97ad54e820ad1996f55d93c57f
> >
> > --
> > Lee Jones [李琼斯]
> Is it too late to drop this out? I think I'm probably going to change 
> the MDIO binding a little which may change how it fits into the overall 
> switch mfd.

Unapplied, thanks.

-- 
Lee Jones [李琼斯]

[PATCH v2 3/4] mips: dts: realtek: Add MDIO controller

[Chris Packham]

Add a device tree node for the MDIO controller on the RTL9300 chips.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - None

 arch/mips/boot/dts/realtek/rtl930x.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/mips/boot/dts/realtek/rtl930x.dtsi b/arch/mips/boot/dts/realtek/rtl930x.dtsi
index 17577457d159..5f74d121ce84 100644
--- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
+++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
@@ -57,6 +57,14 @@ i2c1: i2c@388 {
 			#size-cells = <0>;
 			status = "disabled";
 		};
+
+		mdio0: mdio@ca00 {
+			compatible = "realtek,rtl9301-mdio";
+			reg = <0xca00 0x200>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
 	};
 };
 
-- 
2.47.1

[PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

Add a driver for the MDIO controller on the RTL9300 family of Ethernet
switches with integrated SoC. There are 4 physical SMI interfaces on the
RTL9300 but access is done using the switch ports so a single MDIO bus
is presented to the rest of the system.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - Add clause 22 support
    - Remove commented out code
    - Formatting cleanup
    - Set MAX_PORTS correctly for MDIO interface
    - Fix off-by-one error in pn check

 drivers/net/mdio/Kconfig            |   7 +
 drivers/net/mdio/Makefile           |   1 +
 drivers/net/mdio/mdio-realtek-rtl.c | 341 ++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-realtek-rtl.c

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 4a7a303be2f7..0c6240c4a7e9 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -185,6 +185,13 @@ config MDIO_IPQ8064
 	  This driver supports the MDIO interface found in the network
 	  interface units of the IPQ8064 SoC
 
+config MDIO_REALTEK_RTL
+	tristate "Realtek RTL9300 MDIO interface support"
+	depends on MACH_REALTEK_RTL || COMPILE_TEST
+	help
+	  This driver supports the MDIO interface found in the Realtek
+	  RTL9300 family of Ethernet switches with integrated SoC.
+
 config MDIO_REGMAP
 	tristate
 	help
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 1015f0db4531..2cd8b491f301 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
 obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
 obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
+obj-$(CONFIG_MDIO_REALTEK_RTL)		+= mdio-realtek-rtl.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
 obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)		+= mdio-thunder.o
diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
new file mode 100644
index 000000000000..a13b84279138
--- /dev/null
+++ b/drivers/net/mdio/mdio-realtek-rtl.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MDIO controller for RTL9300 switches with integrated SoC.
+ *
+ * The MDIO communication is abstracted by the switch. At the software level
+ * communication uses the switch port to address the PHY with the actual MDIO
+ * bus and address having been setup via the realtek,smi-address property.
+ */
+
+#include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define SMI_GLB_CTRL			0x000
+#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
+#define SMI_PORT0_15_POLLING_SEL	0x008
+#define SMI_ACCESS_PHY_CTRL_0		0x170
+#define SMI_ACCESS_PHY_CTRL_1		0x174
+#define   PHY_CTRL_RWOP			BIT(2)
+#define   PHY_CTRL_TYPE			BIT(1)
+#define   PHY_CTRL_CMD			BIT(0)
+#define   PHY_CTRL_FAIL			BIT(25)
+#define SMI_ACCESS_PHY_CTRL_2		0x178
+#define SMI_ACCESS_PHY_CTRL_3		0x17c
+#define SMI_PORT0_5_ADDR_CTRL		0x180
+
+#define MAX_PORTS       28
+#define MAX_SMI_BUSSES  4
+#define MAX_SMI_ADDR	0x1f
+
+struct realtek_mdio_priv {
+	struct regmap *regmap;
+	u8 smi_bus[MAX_PORTS];
+	u8 smi_addr[MAX_PORTS];
+	bool smi_bus_isc45[MAX_SMI_BUSSES];
+	u32 reg_base;
+};
+
+static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
+{
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+
+	return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+					val, !(val & PHY_CTRL_CMD), 10, 500);
+}
+
+static int realtek_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int realtek_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_RWOP | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   PHY_CTRL_TYPE | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int realtek_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
+				  int regnum, u16 value)
+{
+	struct realtek_mdio_priv *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 val;
+	int err;
+
+	err = realtek_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+			   PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
+{
+	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
+	struct regmap *regmap = priv->regmap;
+	u32 reg_base = priv->reg_base;
+	u32 port_addr[5] = { 0 };
+	u32 poll_sel[2] = { 0 };
+	int i, err;
+
+	/* Associate the port with the SMI interface and PHY */
+	for (i = 0; i < MAX_PORTS; i++) {
+		int pos;
+
+		if (priv->smi_bus[i] > 3)
+			continue;
+
+		pos = (i % 6) * 5;
+		port_addr[i / 6] |= priv->smi_addr[i] << pos;
+
+		pos = (i % 16) * 2;
+		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
+	}
+
+	/* Put the interfaces into C45 mode if required */
+	for (i = 0; i < MAX_SMI_BUSSES; i++) {
+		if (priv->smi_bus_isc45[i]) {
+			glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
+			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
+		}
+	}
+
+	err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_5_ADDR_CTRL,
+				port_addr, 5);
+	if (err)
+		return err;
+
+	err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_15_POLLING_SEL,
+				poll_sel, 2);
+	if (err)
+		return err;
+
+	err = regmap_update_bits(regmap, reg_base + SMI_GLB_CTRL,
+				 glb_ctrl_mask, glb_ctrl_val);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int realtek_mdiobus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct realtek_mdio_priv *priv;
+	struct fwnode_handle *child;
+	struct mii_bus *bus;
+	int err;
+
+	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "Reaktek Switch MDIO Bus";
+	bus->read = realtek_mdio_read_c22;
+	bus->write = realtek_mdio_write_c22;
+	bus->read_c45 = realtek_mdio_read_c45;
+	bus->write_c45 =  realtek_mdio_write_c45;
+	bus->parent = dev;
+	priv = bus->priv;
+
+	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	err = device_property_read_u32(dev, "reg", &priv->reg_base);
+	if (err)
+		return err;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
+
+	device_for_each_child_node(dev, child) {
+		u32 pn, smi_addr[2];
+
+		err = fwnode_property_read_u32(child, "reg", &pn);
+		if (err)
+			return err;
+
+		if (pn >= MAX_PORTS)
+			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
+
+		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
+		if (err) {
+			smi_addr[0] = 0;
+			smi_addr[1] = pn;
+		}
+
+		if (smi_addr[0] > MAX_SMI_BUSSES)
+			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
+					     smi_addr[0]);
+
+		if (smi_addr[1] > MAX_SMI_ADDR)
+			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
+
+		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
+			priv->smi_bus_isc45[smi_addr[0]] = true;
+
+		priv->smi_bus[pn] = smi_addr[0];
+		priv->smi_addr[pn] = smi_addr[1];
+	}
+
+	err = realtek_mdiobus_init(priv);
+	if (err)
+		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
+
+	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
+	if (err)
+		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
+
+	return 0;
+}
+
+static const struct of_device_id realtek_mdio_ids[] = {
+	{ .compatible = "realtek,rtl9301-mdio" },
+	{ .compatible = "realtek,rtl9302b-mdio" },
+	{ .compatible = "realtek,rtl9302c-mdio" },
+	{ .compatible = "realtek,rtl9303-mdio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, realtek_mdio_ids);
+
+static struct platform_driver rtl9300_mdio_driver = {
+	.probe = realtek_mdiobus_probe,
+	.driver = {
+		.name = "mdio-rtl9300",
+		.of_match_table = realtek_mdio_ids,
+	},
+};
+
+module_platform_driver(rtl9300_mdio_driver);
+
+MODULE_DESCRIPTION("RTL9300 MDIO driver");
+MODULE_LICENSE("GPL");
-- 
2.47.1

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Simon Horman]

On Mon, Dec 16, 2024 at 04:13:46PM +1300, Chris Packham wrote:
> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> switches with integrated SoC. There are 4 physical SMI interfaces on the
> RTL9300 but access is done using the switch ports so a single MDIO bus
> is presented to the rest of the system.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

...

> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c

...

> +#define MAX_SMI_BUSSES  4
> +#define MAX_SMI_ADDR	0x1f
> +
> +struct realtek_mdio_priv {
> +	struct regmap *regmap;
> +	u8 smi_bus[MAX_PORTS];
> +	u8 smi_addr[MAX_PORTS];
> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
> +	u32 reg_base;
> +};

...

> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct realtek_mdio_priv *priv;
> +	struct fwnode_handle *child;
> +	struct mii_bus *bus;
> +	int err;
> +
> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "Reaktek Switch MDIO Bus";
> +	bus->read = realtek_mdio_read_c22;
> +	bus->write = realtek_mdio_write_c22;
> +	bus->read_c45 = realtek_mdio_read_c45;
> +	bus->write_c45 =  realtek_mdio_write_c45;
> +	bus->parent = dev;
> +	priv = bus->priv;
> +
> +	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	err = device_property_read_u32(dev, "reg", &priv->reg_base);
> +	if (err)
> +		return err;
> +
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> +	device_for_each_child_node(dev, child) {
> +		u32 pn, smi_addr[2];
> +
> +		err = fwnode_property_read_u32(child, "reg", &pn);
> +		if (err)
> +			return err;
> +
> +		if (pn >= MAX_PORTS)
> +			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> +		if (err) {
> +			smi_addr[0] = 0;
> +			smi_addr[1] = pn;
> +		}
> +
> +		if (smi_addr[0] > MAX_SMI_BUSSES)

Hi Chris,

Should this condition be

		if (smi_addr[0] >= MAX_SMI_BUSSES)


> +			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> +					     smi_addr[0]);
> +
> +		if (smi_addr[1] > MAX_SMI_ADDR)
> +			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> +
> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> +			priv->smi_bus_isc45[smi_addr[0]] = true;

Otherwise it seems that smi_bus_isc45 may overflow here.

Flagged by Smatch.

> +
> +		priv->smi_bus[pn] = smi_addr[0];
> +		priv->smi_addr[pn] = smi_addr[1];
> +	}
> +
> +	err = realtek_mdiobus_init(priv);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> +
> +	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> +	return 0;
> +}

...

-- 
pw-bot: changes-requested

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 17/12/2024 05:48, Simon Horman wrote:
> On Mon, Dec 16, 2024 at 04:13:46PM +1300, Chris Packham wrote:
>> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
>> switches with integrated SoC. There are 4 physical SMI interfaces on the
>> RTL9300 but access is done using the switch ports so a single MDIO bus
>> is presented to the rest of the system.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ...
>
>> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
> ...
>
>> +#define MAX_SMI_BUSSES  4
>> +#define MAX_SMI_ADDR	0x1f
>> +
>> +struct realtek_mdio_priv {
>> +	struct regmap *regmap;
>> +	u8 smi_bus[MAX_PORTS];
>> +	u8 smi_addr[MAX_PORTS];
>> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
>> +	u32 reg_base;
>> +};
> ...
>
>> +static int realtek_mdiobus_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct realtek_mdio_priv *priv;
>> +	struct fwnode_handle *child;
>> +	struct mii_bus *bus;
>> +	int err;
>> +
>> +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
>> +	if (!bus)
>> +		return -ENOMEM;
>> +
>> +	bus->name = "Reaktek Switch MDIO Bus";
>> +	bus->read = realtek_mdio_read_c22;
>> +	bus->write = realtek_mdio_write_c22;
>> +	bus->read_c45 = realtek_mdio_read_c45;
>> +	bus->write_c45 =  realtek_mdio_write_c45;
>> +	bus->parent = dev;
>> +	priv = bus->priv;
>> +
>> +	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
>> +	if (IS_ERR(priv->regmap))
>> +		return PTR_ERR(priv->regmap);
>> +
>> +	err = device_property_read_u32(dev, "reg", &priv->reg_base);
>> +	if (err)
>> +		return err;
>> +
>> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
>> +
>> +	device_for_each_child_node(dev, child) {
>> +		u32 pn, smi_addr[2];
>> +
>> +		err = fwnode_property_read_u32(child, "reg", &pn);
>> +		if (err)
>> +			return err;
>> +
>> +		if (pn >= MAX_PORTS)
>> +			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
>> +
>> +		err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
>> +		if (err) {
>> +			smi_addr[0] = 0;
>> +			smi_addr[1] = pn;
>> +		}
>> +
>> +		if (smi_addr[0] > MAX_SMI_BUSSES)
> Hi Chris,
>
> Should this condition be
>
> 		if (smi_addr[0] >= MAX_SMI_BUSSES)
Yes. You are correct.
>> +			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
>> +					     smi_addr[0]);
>> +
>> +		if (smi_addr[1] > MAX_SMI_ADDR)
>> +			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
>> +
>> +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
>> +			priv->smi_bus_isc45[smi_addr[0]] = true;
> Otherwise it seems that smi_bus_isc45 may overflow here.
>
> Flagged by Smatch.

Sounds like something I should start looking at for myself. Have you got 
a link to share?

>> +
>> +		priv->smi_bus[pn] = smi_addr[0];
>> +		priv->smi_addr[pn] = smi_addr[1];
>> +	}
>> +
>> +	err = realtek_mdiobus_init(priv);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
>> +
>> +	err = devm_of_mdiobus_register(dev, bus, dev->of_node);
>> +	if (err)
>> +		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
>> +
>> +	return 0;
>> +}
> ...
>

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Simon Horman]

+ Dan Carpenter

On Tue, Dec 17, 2024 at 10:47:10AM +1300, Chris Packham wrote:
> 
> On 17/12/2024 05:48, Simon Horman wrote:
> > On Mon, Dec 16, 2024 at 04:13:46PM +1300, Chris Packham wrote:
> > > Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> > > switches with integrated SoC. There are 4 physical SMI interfaces on the
> > > RTL9300 but access is done using the switch ports so a single MDIO bus
> > > is presented to the rest of the system.
> > > 
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

...

> > > +		if (smi_addr[0] > MAX_SMI_BUSSES)
> > Hi Chris,
> > 
> > Should this condition be
> > 
> > 		if (smi_addr[0] >= MAX_SMI_BUSSES)
> Yes. You are correct.
> > > +			return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> > > +					     smi_addr[0]);
> > > +
> > > +		if (smi_addr[1] > MAX_SMI_ADDR)
> > > +			return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> > > +
> > > +		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> > > +			priv->smi_bus_isc45[smi_addr[0]] = true;
> > Otherwise it seems that smi_bus_isc45 may overflow here.
> > 
> > Flagged by Smatch.
> 
> Sounds like something I should start looking at for myself. Have you got a
> link to share?

Hi Chris,

Smatch is here: https://github.com/error27/smatch
And my usage of it is informed by
https://blogs.oracle.com/linux/post/smatch-static-analysis-tool-overview-by-dan-carpenter

FWIIW, I run it usking kchecker on individual source files.

I've also CCed the author, Dan Carpenter, for good measure.

...

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Luiz Angelo Daros de Luca]

Hello Chris,

> +++ b/drivers/net/mdio/mdio-realtek-rtl.c

I wonder if the name might be dubious in the future with other realtek
products with MDIO. Realtek is quite a large company with many
products. Would a version/model/family/usage in that name help a far
future reader to identify what this file is about?

How would this realtek MDIO driver interact with the
drivers/net/dsa/realtek drivers? I guess it might not be too much as
this is the SoC MDIO bus and not the user MDIO bus (also something
called "realtek MDIO driver"). Also, the code logic there just rhymes
with some register access implemented there.

> @@ -0,0 +1,341 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MDIO controller for RTL9300 switches with integrated SoC.
> + *
> + * The MDIO communication is abstracted by the switch. At the software level
> + * communication uses the switch port to address the PHY with the actual MDIO
> + * bus and address having been setup via the realtek,smi-address property.
> + */
> +
> +#include <linux/mdio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define SMI_GLB_CTRL                   0x000
> +#define   GLB_CTRL_INTF_SEL(intf)      BIT(16 + (intf))
> +#define SMI_PORT0_15_POLLING_SEL       0x008
> +#define SMI_ACCESS_PHY_CTRL_0          0x170
> +#define SMI_ACCESS_PHY_CTRL_1          0x174
> +#define   PHY_CTRL_RWOP                        BIT(2)
> +#define   PHY_CTRL_TYPE                        BIT(1)
> +#define   PHY_CTRL_CMD                 BIT(0)
> +#define   PHY_CTRL_FAIL                        BIT(25)
> +#define SMI_ACCESS_PHY_CTRL_2          0x178
> +#define SMI_ACCESS_PHY_CTRL_3          0x17c
> +#define SMI_PORT0_5_ADDR_CTRL          0x180
> +
> +#define MAX_PORTS       28
> +#define MAX_SMI_BUSSES  4
> +#define MAX_SMI_ADDR   0x1f
> +
> +struct realtek_mdio_priv {
> +       struct regmap *regmap;
> +       u8 smi_bus[MAX_PORTS];
> +       u8 smi_addr[MAX_PORTS];
> +       bool smi_bus_isc45[MAX_SMI_BUSSES];
> +       u32 reg_base;
> +};
> +
> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)

All those realtek_mdio_* prefix might collide with realtek_mdio_* from
drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
is about the interface (MDIO or SMI) between (the other vendor) SoC
and the switch. I don't know if the maintainers are OK with it but
listing those symbols in alphabetic order from both sources might be
confusing.

> +{
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +
> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,

All regmap funcs are adding reg_base to the register address. Isn't a
remap job to do that sum? It just looks odd but I never worked with
MFD. It looks like it is missing a subregmap-like variant.

> +                                       val, !(val & PHY_CTRL_CMD), 10, 500);
> +}
> +
> +static int realtek_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
> +       if (err)
> +               return err;
> +
> +       return val & 0xffff;
> +}
> +
> +static int realtek_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_RWOP | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                                      val, !(val & PHY_CTRL_CMD), 10, 100);
> +       if (err)
> +               return err;
> +
> +       if (val & PHY_CTRL_FAIL)
> +               return -ENXIO;
> +
> +       return 0;
> +}
> +
> +static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
> +                          dev_addr << 16 | (regnum & 0xffff));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          PHY_CTRL_TYPE | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, &val);
> +       if (err)
> +               return err;
> +
> +       return val & 0xffff;
> +}
> +
> +static int realtek_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
> +                                 int regnum, u16 value)
> +{
> +       struct realtek_mdio_priv *priv = bus->priv;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 val;
> +       int err;
> +
> +       err = realtek_mdio_wait_ready(priv);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_0, BIT(phy_id));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_2, value << 16);
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_3,
> +                          dev_addr << 16 | (regnum & 0xffff));
> +       if (err)
> +               return err;
> +
> +       err = regmap_write(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                          PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
> +       if (err)
> +               return err;
> +
> +       err = regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> +                                      val, !(val & PHY_CTRL_CMD), 10, 100);
> +       if (err)
> +               return err;
> +
> +       if (val & PHY_CTRL_FAIL)
> +               return -ENXIO;
> +
> +       return 0;
> +}
> +
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> +       u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> +       struct regmap *regmap = priv->regmap;
> +       u32 reg_base = priv->reg_base;
> +       u32 port_addr[5] = { 0 };
> +       u32 poll_sel[2] = { 0 };
> +       int i, err;
> +
> +       /* Associate the port with the SMI interface and PHY */
> +       for (i = 0; i < MAX_PORTS; i++) {
> +               int pos;
> +
> +               if (priv->smi_bus[i] > 3)
> +                       continue;
> +
> +               pos = (i % 6) * 5;
> +               port_addr[i / 6] |= priv->smi_addr[i] << pos;
> +
> +               pos = (i % 16) * 2;
> +               poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> +       }
> +
> +       /* Put the interfaces into C45 mode if required */
> +       for (i = 0; i < MAX_SMI_BUSSES; i++) {
> +               if (priv->smi_bus_isc45[i]) {
> +                       glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
> +                       glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> +               }
> +       }
> +
> +       err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_5_ADDR_CTRL,
> +                               port_addr, 5);
> +       if (err)
> +               return err;
> +
> +       err = regmap_bulk_write(regmap, reg_base + SMI_PORT0_15_POLLING_SEL,
> +                               poll_sel, 2);
> +       if (err)
> +               return err;
> +
> +       err = regmap_update_bits(regmap, reg_base + SMI_GLB_CTRL,
> +                                glb_ctrl_mask, glb_ctrl_val);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct realtek_mdio_priv *priv;
> +       struct fwnode_handle *child;
> +       struct mii_bus *bus;
> +       int err;
> +
> +       bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> +       if (!bus)
> +               return -ENOMEM;
> +
> +       bus->name = "Reaktek Switch MDIO Bus";
> +       bus->read = realtek_mdio_read_c22;
> +       bus->write = realtek_mdio_write_c22;
> +       bus->read_c45 = realtek_mdio_read_c45;
> +       bus->write_c45 =  realtek_mdio_write_c45;
> +       bus->parent = dev;
> +       priv = bus->priv;
> +
> +       priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> +       if (IS_ERR(priv->regmap))
> +               return PTR_ERR(priv->regmap);
> +
> +       err = device_property_read_u32(dev, "reg", &priv->reg_base);
> +       if (err)
> +               return err;
> +
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> +       device_for_each_child_node(dev, child) {
> +               u32 pn, smi_addr[2];
> +
> +               err = fwnode_property_read_u32(child, "reg", &pn);
> +               if (err)
> +                       return err;
> +
> +               if (pn >= MAX_PORTS)
> +                       return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> +               err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> +               if (err) {
> +                       smi_addr[0] = 0;
> +                       smi_addr[1] = pn;
> +               }
> +
> +               if (smi_addr[0] > MAX_SMI_BUSSES)
> +                       return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n",
> +                                            smi_addr[0]);
> +
> +               if (smi_addr[1] > MAX_SMI_ADDR)
> +                       return dev_err_probe(dev, -EINVAL, "illegal smi addr %d\n", smi_addr[1]);
> +
> +               if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> +                       priv->smi_bus_isc45[smi_addr[0]] = true;
> +
> +               priv->smi_bus[pn] = smi_addr[0];
> +               priv->smi_addr[pn] = smi_addr[1];
> +       }
> +
> +       err = realtek_mdiobus_init(priv);
> +       if (err)
> +               return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> +
> +       err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> +       if (err)
> +               return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id realtek_mdio_ids[] = {
> +       { .compatible = "realtek,rtl9301-mdio" },
> +       { .compatible = "realtek,rtl9302b-mdio" },
> +       { .compatible = "realtek,rtl9302c-mdio" },
> +       { .compatible = "realtek,rtl9303-mdio" },

Do these different compatible strings really matter? AFAIK, compatible
are not for listing all supported models/variants but to describe
devices that have a different behavior and indicating that (with
different strings) is needed to decide how the driver will work. If
the driver does not use which compatible was set, it might indicate
that we don't really need 4 compatible but 1.

> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, realtek_mdio_ids);
> +
> +static struct platform_driver rtl9300_mdio_driver = {
> +       .probe = realtek_mdiobus_probe,
> +       .driver = {
> +               .name = "mdio-rtl9300",
> +               .of_match_table = realtek_mdio_ids,
> +       },
> +};
> +
> +module_platform_driver(rtl9300_mdio_driver);
> +
> +MODULE_DESCRIPTION("RTL9300 MDIO driver");
> +MODULE_LICENSE("GPL");
> --
> 2.47.1
>
>

Regards,

Luiz

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
> Hello Chris,
> 
> > +++ b/drivers/net/mdio/mdio-realtek-rtl.c
> 
> I wonder if the name might be dubious in the future with other realtek
> products with MDIO. Realtek is quite a large company with many
> products. Would a version/model/family/usage in that name help a far
> future reader to identify what this file is about?

Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?

> > +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> 
> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
> is about the interface (MDIO or SMI) between (the other vendor) SoC
> and the switch. I don't know if the maintainers are OK with it but
> listing those symbols in alphabetic order from both sources might be
> confusing.

rtl9300_ as a prefix?

> > +static const struct of_device_id realtek_mdio_ids[] = {
> > +       { .compatible = "realtek,rtl9301-mdio" },
> > +       { .compatible = "realtek,rtl9302b-mdio" },
> > +       { .compatible = "realtek,rtl9302c-mdio" },
> > +       { .compatible = "realtek,rtl9303-mdio" },
> 
> Do these different compatible strings really matter? AFAIK, compatible
> are not for listing all supported models/variants but to describe
> devices that have a different behavior and indicating that (with
> different strings) is needed to decide how the driver will work. If
> the driver does not use which compatible was set, it might indicate
> that we don't really need 4 compatible but 1.

It can be useful when we initially think they are compatible, but
later find out they are not, and we need different behaviour.

FYI: Please trim the text when replying.

	Andrew

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 19/12/2024 22:40, Andrew Lunn wrote:
> On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
>> Hello Chris,
>>
>>> +++ b/drivers/net/mdio/mdio-realtek-rtl.c
>> I wonder if the name might be dubious in the future with other realtek
>> products with MDIO. Realtek is quite a large company with many
>> products. Would a version/model/family/usage in that name help a far
>> future reader to identify what this file is about?
> Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?

Yes my intention was that "rtl" was the family name. I'm happy to change 
to rtl9300.

I suspect this probably will be compatible with the rtl9310. I've just 
received a RTL9313 based board so will probably start looking at that in 
the new year.

>>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
>> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
>> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
>> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
>> is about the interface (MDIO or SMI) between (the other vendor) SoC
>> and the switch. I don't know if the maintainers are OK with it but
>> listing those symbols in alphabetic order from both sources might be
>> confusing.
> rtl9300_ as a prefix?

I'd happily  change to rtl_ or rtl9300_

>>> +static const struct of_device_id realtek_mdio_ids[] = {
>>> +       { .compatible = "realtek,rtl9301-mdio" },
>>> +       { .compatible = "realtek,rtl9302b-mdio" },
>>> +       { .compatible = "realtek,rtl9302c-mdio" },
>>> +       { .compatible = "realtek,rtl9303-mdio" },
>> Do these different compatible strings really matter? AFAIK, compatible
>> are not for listing all supported models/variants but to describe
>> devices that have a different behavior and indicating that (with
>> different strings) is needed to decide how the driver will work. If
>> the driver does not use which compatible was set, it might indicate
>> that we don't really need 4 compatible but 1.
> It can be useful when we initially think they are compatible, but
> later find out they are not, and we need different behaviour.

The way I've written the dt-binding any board should include 
"realtek,rtl9301-mdio" and may also include one of 
"realtek,rtl9302b-mdio", "realtek,rtl9302c-mdio", 
"realtek,rtl9303-mdio". For the MDIO driver the specific chip could 
possibly tell us the maximum SMI bus number. Unfortunately I've only got 
a block diagram of the RTL9302C, I know that does have 4 SMI interfaces, 
the others may have fewer. Things would probably work fine for now with 
just "realtek,rtl9301-mdio" but is there any harm in including the others?

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Luiz Angelo Daros de Luca]

> On 19/12/2024 22:40, Andrew Lunn wrote:
> > On Thu, Dec 19, 2024 at 01:46:41AM -0300, Luiz Angelo Daros de Luca wrote:
> >> Hello Chris,
> >>
> >>> +++ b/drivers/net/mdio/mdio-realtek-rtl.c
> >> I wonder if the name might be dubious in the future with other realtek
> >> products with MDIO. Realtek is quite a large company with many
> >> products. Would a version/model/family/usage in that name help a far
> >> future reader to identify what this file is about?
> > Isnt rtl the family name? Or would you prefer mdio-realtek-rtl9300.c?
>
> Yes my intention was that "rtl" was the family name.

rtl just means realtek. It is the same prefix for a wide range of
Realtek products (audio, usb, storage, network).

> I'm happy to change to rtl9300.

Even the product number might be confusing. For example, the switch
rtl8365mb is newer than (and incompatible with) the rtl8366rb. Realtek
has a library/driver name that could indicate the family (rtl8367b,
rtl8367c, rtl8367d), but it is not tightly related to model numbers.
In the DSA case, we simply adopted the first device that was supported
as the filename, even after that file was expanded to include other
models. I hope rtl93xx model numbers will be less confusing but it is
a Realtek decision. With the realtek past cases, if you want to be
sure it will not be confused with another model in the future, just
use the model "most significant" to your driver, like
modio-realtek-rtl9301. Do not expect that a future rtl93xx model will
be compatible with rtl9300. If rtl9300 does not really mean rtl93??,
rtl9301 would be, at least, more informative.

mdio-realtek-rtl9300.c or even mdio-rtl9300.c are just fine, but I
prefer the former as the common prefix will group different future
models, even if realtek abandons the rtl prefix.

> I suspect this probably will be compatible with the rtl9310. I've just
> received a RTL9313 based board so will probably start looking at that in
> the new year.

At least for the realtek switch-only products, if you have access to
the Realtek Programming Guide or the vendor library/driver, it would
be easy to spot compatible models as they share the same driver
generation. From what I saw from the difference between vendor switch
drivers rtl8367b, rtl8367c and rtl8367d, Relatek tends to use an
incremental model, but breaking the support to older models on every
new family release. They share a lot of code but they differ on
registers (normally constants on each generation). If you intend to
also upstream a DSA driver, you'll probably need to share some code
with the existing drivers as duplicating that much code is normally
rejected in the upstream kernel.

> >>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> >> All those realtek_mdio_* prefix might collide with realtek_mdio_* from
> >> drivers/net/dsa/realtek/realtek-mdio.c. This realtek_mdio_* is about a
> >> Realtek SoC MDIO interface with the switch. The other realtek_mdio_*
> >> is about the interface (MDIO or SMI) between (the other vendor) SoC
> >> and the switch. I don't know if the maintainers are OK with it but
> >> listing those symbols in alphabetic order from both sources might be
> >> confusing.
> > rtl9300_ as a prefix?
>
> I'd happily  change to rtl_ or rtl9300_

"rtl9300_", please (or "rtl9301_") a "rtl" just means realtek.

> >>> +static const struct of_device_id realtek_mdio_ids[] = {
> >>> +       { .compatible = "realtek,rtl9301-mdio" },
> >>> +       { .compatible = "realtek,rtl9302b-mdio" },
> >>> +       { .compatible = "realtek,rtl9302c-mdio" },
> >>> +       { .compatible = "realtek,rtl9303-mdio" },
> >> Do these different compatible strings really matter? AFAIK, compatible
> >> are not for listing all supported models/variants but to describe
> >> devices that have a different behavior and indicating that (with
> >> different strings) is needed to decide how the driver will work. If
> >> the driver does not use which compatible was set, it might indicate
> >> that we don't really need 4 compatible but 1.
> > It can be useful when we initially think they are compatible, but
> > later find out they are not, and we need different behaviour.
>
> The way I've written the dt-binding any board should include
> "realtek,rtl9301-mdio" and may also include one of
> "realtek,rtl9302b-mdio", "realtek,rtl9302c-mdio",
> "realtek,rtl9303-mdio". For the MDIO driver the specific chip could
> possibly tell us the maximum SMI bus number. Unfortunately I've only got
> a block diagram of the RTL9302C, I know that does have 4 SMI interfaces,
> the others may have fewer. Things would probably work fine for now with
> just "realtek,rtl9301-mdio" but is there any harm in including the others?

If "realtek,rtl9301-mdio" is mandatory, until you need to
differentiate each model, the extra compatible strings are not useful.
It would increase the compatible table a little bit. My concern was
not about the extra data but that a similar approach was rejected in
the past. If maintainers are OK with it, I have nothing else to say.

>>> +{
>>> +       struct regmap *regmap = priv->regmap;
>>> +       u32 reg_base = priv->reg_base;
>>> +       u32 val;
>>> +
>>> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
>> All regmap funcs are adding reg_base to the register address. Isn't a
>> remap job to do that sum? It just looks odd but I never worked with
>> MFD. It looks like it is missing a subregmap-like variant.
>
> I'm thinking about dropping the base and just using the full 16-bit
> address. I've already confused myself between this code and the datasheet.

That's what I thought when I saw the sum. I would definitely miss it
at some point.
If their positions are fixed related to syscon base, I would use the
full 16-bit. You could use the base+relative_reg_addr in the macro
that defines the register address without increasing the complexity.

Regards,

Luiz

Re: [PATCH v2 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 19/12/2024 17:46, Luiz Angelo Daros de Luca wrote:
>> +#define SMI_GLB_CTRL                   0x000
>> +#define   GLB_CTRL_INTF_SEL(intf)      BIT(16 + (intf))
>> +#define SMI_PORT0_15_POLLING_SEL       0x008
>> +#define SMI_ACCESS_PHY_CTRL_0          0x170
>> +#define SMI_ACCESS_PHY_CTRL_1          0x174
>> +#define   PHY_CTRL_RWOP                        BIT(2)
>> +#define   PHY_CTRL_TYPE                        BIT(1)
>> +#define   PHY_CTRL_CMD                 BIT(0)
>> +#define   PHY_CTRL_FAIL                        BIT(25)
>> +#define SMI_ACCESS_PHY_CTRL_2          0x178
>> +#define SMI_ACCESS_PHY_CTRL_3          0x17c
>> +#define SMI_PORT0_5_ADDR_CTRL          0x180
>> +
>> +#define MAX_PORTS       28
>> +#define MAX_SMI_BUSSES  4
>> +#define MAX_SMI_ADDR   0x1f
>> +
>> +struct realtek_mdio_priv {
>> +       struct regmap *regmap;
>> +       u8 smi_bus[MAX_PORTS];
>> +       u8 smi_addr[MAX_PORTS];
>> +       bool smi_bus_isc45[MAX_SMI_BUSSES];
>> +       u32 reg_base;
>> +};
>> +
>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
>> +{
>> +       struct regmap *regmap = priv->regmap;
>> +       u32 reg_base = priv->reg_base;
>> +       u32 val;
>> +
>> +       return regmap_read_poll_timeout(regmap, reg_base + SMI_ACCESS_PHY_CTRL_1,
> All regmap funcs are adding reg_base to the register address. Isn't a
> remap job to do that sum? It just looks odd but I never worked with
> MFD. It looks like it is missing a subregmap-like variant.
I'm thinking about dropping the base and just using the full 16-bit 
address. I've already confused myself between this code and the datasheet.