[PATCH net-next 0/2] net: mdio: add amlogic gxl mdio mux support

[PATCH net-next 0/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet]

Add support for the MDIO multiplexer found in the Amlogic GXL SoC family.
This multiplexer allows to choose between the external (SoC pins) MDIO bus,
or the internal one leading to the integrated 10/100M PHY.

This multiplexer has been handled with the mdio-mux-mmioreg generic driver
so far. When it was added, it was thought the logic was handled by a
single register.

It turns out more than a single register need to be properly set.
As long as the device is using the Amlogic vendor bootloader, or upstream
u-boot with net support, it is working fine since the kernel is inheriting
the bootloader settings. Without net support in the bootloader, this glue
comes unset in the kernel and only the external path may operate properly.

With this driver (and the associated DT update), the kernel no longer relies
on the bootloader to set things up, fixing the problem.

This has been tested on the aml-s905x-cc (LePotato) for the internal path
and the aml-s912-pc (Tartiflette) for the external path.

Jerome Brunet (2):
  dt-bindings: net: add amlogic gxl mdio multiplexer
  net: mdio: add amlogic gxl mdio mux support

 .../bindings/net/amlogic,gxl-mdio-mux.yaml    |  64 +++++++
 drivers/net/mdio/Kconfig                      |  11 ++
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/mdio-mux-meson-gxl.c         | 160 ++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml
 create mode 100644 drivers/net/mdio/mdio-mux-meson-gxl.c

-- 
2.39.0

[PATCH net-next 1/2] dt-bindings: net: add amlogic gxl mdio multiplexer

[Jerome Brunet]

Add documentation for the MDIO bus multiplexer found on the Amlogic GXL
SoC family

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../bindings/net/amlogic,gxl-mdio-mux.yaml    | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml

diff --git a/Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml b/Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml
new file mode 100644
index 000000000000..d21bce695fa9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/amlogic,gxl-mdio-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic GXL MDIO bus multiplexer
+
+maintainers:
+  - Jerome Brunet <jbrunet@baylibre.com>
+
+description:
+  This is a special case of a MDIO bus multiplexer. It allows to choose between
+  the internal mdio bus leading to the embedded 10/100 PHY or the external
+  MDIO bus on the Amlogic GXL SoC family.
+
+allOf:
+  - $ref: mdio-mux.yaml#
+
+properties:
+  compatible:
+    const: amlogic,gxl-mdio-mux
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: ref
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    eth_phy_mux: mdio@558 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "amlogic,gxl-mdio-mux";
+      clocks = <&refclk>;
+      clock-names = "ref";
+      reg = <0x558 0xc>;
+      mdio-parent-bus = <&mdio0>;
+
+      external_mdio: mdio@0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <0x0>;
+      };
+
+      internal_mdio: mdio@1 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <0x1>;
+      };
+    };
-- 
2.39.0

[PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet]

Add support for the mdio mux and internal phy glue of the GXL SoC
family

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/net/mdio/Kconfig              |  11 ++
 drivers/net/mdio/Makefile             |   1 +
 drivers/net/mdio/mdio-mux-meson-gxl.c | 160 ++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-mux-meson-gxl.c

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index bfa16826a6e1..80f3e10b1be1 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -215,6 +215,17 @@ config MDIO_BUS_MUX_MESON_G12A
 	  the amlogic g12a SoC. The multiplexers connects either the external
 	  or the internal MDIO bus to the parent bus.
 
+config MDIO_BUS_MUX_MESON_GXL
+	tristate "Amlogic GXL based MDIO bus multiplexer"
+	depends on ARCH_MESON || COMPILE_TEST
+	depends on OF_MDIO && HAS_IOMEM && COMMON_CLK
+	select MDIO_BUS_MUX
+	default m if ARCH_MESON
+	help
+	  This module provides a driver for the MDIO multiplexer/glue of
+	  the amlogic gxl SoC. The multiplexers connects either the external
+	  or the internal MDIO bus to the parent bus.
+
 config MDIO_BUS_MUX_BCM6368
 	tristate "Broadcom BCM6368 MDIO bus multiplexers"
 	depends on OF && OF_MDIO && (BMIPS_GENERIC || COMPILE_TEST)
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 15f8dc4042ce..7d4cb4c11e4e 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_MDIO_BUS_MUX_BCM6368)	+= mdio-mux-bcm6368.o
 obj-$(CONFIG_MDIO_BUS_MUX_BCM_IPROC)	+= mdio-mux-bcm-iproc.o
 obj-$(CONFIG_MDIO_BUS_MUX_GPIO)		+= mdio-mux-gpio.o
 obj-$(CONFIG_MDIO_BUS_MUX_MESON_G12A)	+= mdio-mux-meson-g12a.o
+obj-$(CONFIG_MDIO_BUS_MUX_MESON_GXL)	+= mdio-mux-meson-gxl.o
 obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) 	+= mdio-mux-mmioreg.o
 obj-$(CONFIG_MDIO_BUS_MUX_MULTIPLEXER) 	+= mdio-mux-multiplexer.o
diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
new file mode 100644
index 000000000000..205095d845ea
--- /dev/null
+++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Baylibre, SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mdio-mux.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define ETH_REG2		0x0
+#define  REG2_PHYID		GENMASK(21, 0)
+#define   EPHY_GXL_ID		0x110181
+#define  REG2_LEDACT		GENMASK(23, 22)
+#define  REG2_LEDLINK		GENMASK(25, 24)
+#define  REG2_DIV4SEL		BIT(27)
+#define  REG2_ADCBYPASS		BIT(30)
+#define  REG2_CLKINSEL		BIT(31)
+#define ETH_REG3		0x4
+#define  REG3_ENH		BIT(3)
+#define  REG3_CFGMODE		GENMASK(6, 4)
+#define  REG3_AUTOMDIX		BIT(7)
+#define  REG3_PHYADDR		GENMASK(12, 8)
+#define  REG3_PWRUPRST		BIT(21)
+#define  REG3_PWRDOWN		BIT(22)
+#define  REG3_LEDPOL		BIT(23)
+#define  REG3_PHYMDI		BIT(26)
+#define  REG3_CLKINEN		BIT(29)
+#define  REG3_PHYIP		BIT(30)
+#define  REG3_PHYEN		BIT(31)
+#define ETH_REG4		0x8
+#define  REG4_PWRUPRSTSIG	BIT(0)
+
+#define MESON_GXL_MDIO_EXTERNAL_ID 0
+#define MESON_GXL_MDIO_INTERNAL_ID 1
+
+struct gxl_mdio_mux {
+	void __iomem *regs;
+	void *mux_handle;
+};
+
+static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
+{
+	u32 val;
+
+ 	/* Setup the internal phy */
+	val = (REG3_ENH |
+	       FIELD_PREP(REG3_CFGMODE, 0x7) |
+	       REG3_AUTOMDIX |
+	       FIELD_PREP(REG3_PHYADDR, 8) |
+	       REG3_LEDPOL |
+	       REG3_PHYMDI |
+	       REG3_CLKINEN |
+	       REG3_PHYIP);
+
+	writel_relaxed(REG4_PWRUPRSTSIG, priv->regs + ETH_REG4);
+	writel_relaxed(val, priv->regs + ETH_REG3);
+	mdelay(10);
+
+	/* Set the internal phy id */
+	writel_relaxed(FIELD_PREP(REG2_PHYID, 0x110181),
+		       priv->regs + ETH_REG2);
+
+	/* Enable the internal phy */
+	val |= REG3_PHYEN;
+	writel_relaxed(val, priv->regs + ETH_REG3);
+	writel_relaxed(0, priv->regs + ETH_REG4);
+
+	/* The phy needs a bit of time to come up */
+	mdelay(10);
+
+	return 0;
+}
+
+static int gxl_enable_external_mdio(struct gxl_mdio_mux *priv)
+{
+ 	/* Reset the mdio bus mux to the external phy */
+	writel_relaxed(0, priv->regs + ETH_REG3);
+
+	return 0;
+}
+
+static int gxl_mdio_switch_fn(int current_child, int desired_child,
+			       void *data)
+{
+	struct gxl_mdio_mux *priv = dev_get_drvdata(data);
+
+	if (current_child == desired_child)
+		return 0;
+
+	switch (desired_child) {
+	case MESON_GXL_MDIO_EXTERNAL_ID:
+		return gxl_enable_external_mdio(priv);
+	case MESON_GXL_MDIO_INTERNAL_ID:
+		return gxl_enable_internal_mdio(priv);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct of_device_id gxl_mdio_mux_match[] = {
+	{ .compatible = "amlogic,gxl-mdio-mux", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gxl_mdio_mux_match);
+
+
+static int gxl_mdio_mux_probe(struct platform_device *pdev){
+	struct device *dev = &pdev->dev;
+	struct clk *rclk;
+	struct gxl_mdio_mux *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, priv);
+
+	priv->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	rclk = devm_clk_get_enabled(dev, "ref");
+	if (IS_ERR(rclk))
+		return dev_err_probe(dev, PTR_ERR(rclk),
+				     "failed to get reference clock\n");
+
+	ret = mdio_mux_init(dev, dev->of_node, gxl_mdio_switch_fn,
+			    &priv->mux_handle, dev, NULL);
+	if (ret)
+		dev_err_probe(dev, ret, "mdio multiplexer init failed\n");
+
+	return ret;
+}
+
+static int gxl_mdio_mux_remove(struct platform_device *pdev)
+{
+	struct gxl_mdio_mux *priv = platform_get_drvdata(pdev);
+
+	mdio_mux_uninit(priv->mux_handle);
+
+	return 0;
+}
+
+static struct platform_driver gxl_mdio_mux_driver = {
+	.probe		= gxl_mdio_mux_probe,
+	.remove		= gxl_mdio_mux_remove,
+	.driver		= {
+		.name	= "gxl-mdio-mux",
+		.of_match_table = gxl_mdio_mux_match,
+	},
+};
+module_platform_driver(gxl_mdio_mux_driver);
+
+MODULE_DESCRIPTION("Amlogic GXL MDIO multiplexer driver");
+MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.39.0

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Simon Horman]

On Mon, Jan 16, 2023 at 10:16:36AM +0100, Jerome Brunet wrote:
> Add support for the mdio mux and internal phy glue of the GXL SoC
> family
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/net/mdio/Kconfig              |  11 ++
>  drivers/net/mdio/Makefile             |   1 +
>  drivers/net/mdio/mdio-mux-meson-gxl.c | 160 ++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+)
>  create mode 100644 drivers/net/mdio/mdio-mux-meson-gxl.c

Hi Jerome,

please run this patch through checkpatch.

...

> diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> new file mode 100644
> index 000000000000..205095d845ea
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c

...

> +static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
> +{

nit: I think void would be a more appropriate return type for this
     function. Likewise gxl_enable_external_mdio()

...

> +static int gxl_mdio_mux_probe(struct platform_device *pdev){

nit: '{' should be at the beginning of a new line

> +	struct device *dev = &pdev->dev;
> +	struct clk *rclk;
> +	struct gxl_mdio_mux *priv;

nit: reverse xmas tree for local variable declarations.

> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;

nit: may be it is nicer to use dev_err_probe() here for consistency.

> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regs))
> +		return PTR_ERR(priv->regs);

And here.

...

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet]

On Mon 16 Jan 2023 at 13:11, Simon Horman <simon.horman@corigine.com> wrote:

> On Mon, Jan 16, 2023 at 10:16:36AM +0100, Jerome Brunet wrote:
>> Add support for the mdio mux and internal phy glue of the GXL SoC
>> family
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/net/mdio/Kconfig              |  11 ++
>>  drivers/net/mdio/Makefile             |   1 +
>>  drivers/net/mdio/mdio-mux-meson-gxl.c | 160 ++++++++++++++++++++++++++
>>  3 files changed, 172 insertions(+)
>>  create mode 100644 drivers/net/mdio/mdio-mux-meson-gxl.c
>
> Hi Jerome,
>
> please run this patch through checkpatch.

Shame ... I really thought I did, but I forgot indeed.
I am really sorry for this. I'll fix everything.

>
> ...
>
>> diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
>> new file mode 100644
>> index 000000000000..205095d845ea
>> --- /dev/null
>> +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
>
> ...
>
>> +static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
>> +{
>
> nit: I think void would be a more appropriate return type for this
>      function. Likewise gxl_enable_external_mdio()
>
> ...
>
>> +static int gxl_mdio_mux_probe(struct platform_device *pdev){
>
> nit: '{' should be at the beginning of a new line
>
>> +	struct device *dev = &pdev->dev;
>> +	struct clk *rclk;
>> +	struct gxl_mdio_mux *priv;
>
> nit: reverse xmas tree for local variable declarations.
>
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>
> nit: may be it is nicer to use dev_err_probe() here for consistency.

That was on purpose. I only use the `dev_err_probe()` when the probe may
defer, which I don't expect here.

I don't mind changing if you prefer it this way.

>
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(priv->regs))
>> +		return PTR_ERR(priv->regs);
>
> And here.
>
> ...

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Simon Horman]

On Mon, Jan 16, 2023 at 02:27:57PM +0100, Jerome Brunet wrote:
> 
> On Mon 16 Jan 2023 at 13:11, Simon Horman <simon.horman@corigine.com> wrote:
> 
> > On Mon, Jan 16, 2023 at 10:16:36AM +0100, Jerome Brunet wrote:
> >> Add support for the mdio mux and internal phy glue of the GXL SoC
> >> family
> >> 
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  drivers/net/mdio/Kconfig              |  11 ++
> >>  drivers/net/mdio/Makefile             |   1 +
> >>  drivers/net/mdio/mdio-mux-meson-gxl.c | 160 ++++++++++++++++++++++++++
> >>  3 files changed, 172 insertions(+)
> >>  create mode 100644 drivers/net/mdio/mdio-mux-meson-gxl.c
> >
> > Hi Jerome,
> >
> > please run this patch through checkpatch.
> 
> Shame ... I really thought I did, but I forgot indeed.
> I am really sorry for this. I'll fix everything.

No problem, it happens.

> > ...
> >
> >> diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> >> new file mode 100644
> >> index 000000000000..205095d845ea
> >> --- /dev/null
> >> +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> >
> > ...
> >
> >> +static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
> >> +{
> >
> > nit: I think void would be a more appropriate return type for this
> >      function. Likewise gxl_enable_external_mdio()
> >
> > ...
> >
> >> +static int gxl_mdio_mux_probe(struct platform_device *pdev){
> >
> > nit: '{' should be at the beginning of a new line
> >
> >> +	struct device *dev = &pdev->dev;
> >> +	struct clk *rclk;
> >> +	struct gxl_mdio_mux *priv;
> >
> > nit: reverse xmas tree for local variable declarations.
> >
> >> +	int ret;
> >> +
> >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >
> > nit: may be it is nicer to use dev_err_probe() here for consistency.
> 
> That was on purpose. I only use the `dev_err_probe()` when the probe may
> defer, which I don't expect here.
> 
> I don't mind changing if you prefer it this way.

I have no strong opinion on this :)

> >> +	platform_set_drvdata(pdev, priv);
> >> +
> >> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(priv->regs))
> >> +		return PTR_ERR(priv->regs);
> >
> > And here.
> >
> > ...
> 

Re: [PATCH net-next 1/2] dt-bindings: net: add amlogic gxl mdio multiplexer

[Krzysztof Kozlowski]

On 16/01/2023 10:16, Jerome Brunet wrote:
> Add documentation for the MDIO bus multiplexer found on the Amlogic GXL
> SoC family

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  .../bindings/net/amlogic,gxl-mdio-mux.yaml    | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml b/Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml
> new file mode 100644
> index 000000000000..d21bce695fa9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/amlogic,gxl-mdio-mux.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/amlogic,gxl-mdio-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic GXL MDIO bus multiplexer
> +
> +maintainers:
> +  - Jerome Brunet <jbrunet@baylibre.com>
> +
> +description:
> +  This is a special case of a MDIO bus multiplexer. It allows to choose between
> +  the internal mdio bus leading to the embedded 10/100 PHY or the external
> +  MDIO bus on the Amlogic GXL SoC family.
> +
> +allOf:
> +  - $ref: mdio-mux.yaml#
> +
> +properties:
> +  compatible:
> +    const: amlogic,gxl-mdio-mux
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: ref
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    eth_phy_mux: mdio@558 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      compatible = "amlogic,gxl-mdio-mux";

compatible, then reg then the rest.

> +      clocks = <&refclk>;
> +      clock-names = "ref";
> +      reg = <0x558 0xc>;
> +      mdio-parent-bus = <&mdio0>;
> +
> +      external_mdio: mdio@0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        reg = <0x0>;

reg is before other properties.

> +      };
> +
> +      internal_mdio: mdio@1 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        reg = <0x1>;

Ditto. If you resend, keep my tag and finally use get_maintainers.pl

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH net-next 1/2] dt-bindings: net: add amlogic gxl mdio multiplexer

[Jerome Brunet]

On Tue 17 Jan 2023 at 09:31, Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 16/01/2023 10:16, Jerome Brunet wrote:
>> Add documentation for the MDIO bus multiplexer found on the Amlogic GXL
>> SoC family
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
>

Hi Krzysztof,

I do use get_maintainers.pl but I also filter based on past experience
to avoid spamming to much. It seems I stayed on the pre-2015
requirement to send only to devicetree list (I was actually making an
exception specifically for DT) ... and there was no complain so far ;)

I've read documentation again and it is explicit. This will be fixed for
v2.

Thanks for pointing this out.

Re: [PATCH net-next 1/2] dt-bindings: net: add amlogic gxl mdio multiplexer

[Krzysztof Kozlowski]

On 17/01/2023 10:05, Jerome Brunet wrote:
> 
> On Tue 17 Jan 2023 at 09:31, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On 16/01/2023 10:16, Jerome Brunet wrote:
>>> Add documentation for the MDIO bus multiplexer found on the Amlogic GXL
>>> SoC family
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC.  It might happen, that command when run on an older
>> kernel, gives you outdated entries.  Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
> 
> Hi Krzysztof,
> 
> I do use get_maintainers.pl but I also filter based on past experience
> to avoid spamming to much. It seems I stayed on the pre-2015
> requirement to send only to devicetree list (I was actually making an
> exception specifically for DT) ... and there was no complain so far ;)
> 
> I've read documentation again and it is explicit. This will be fixed for
> v2.
> 
> Thanks for pointing this out.

For regular patchsets not spanning over 10 different subsystems (so
total number of CCs should be 5-10), please Cc all
maintainers/reviewers/supporters/lists pointed by maintainers.pl. Skip
git fallback. How your patch should appear in my mailbox if you skip me?
Not everyone are using Patchwork.

Best regards,
Krzysztof

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Andrew Lunn]

> > >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >> +	if (!priv)
> > >> +		return -ENOMEM;
> > >
> > > nit: may be it is nicer to use dev_err_probe() here for consistency.
> > 
> > That was on purpose. I only use the `dev_err_probe()` when the probe may
> > defer, which I don't expect here.
> > 
> > I don't mind changing if you prefer it this way.
> 
> I have no strong opinion on this :)

dev_err_probe() does not apply here, because devm_kzalloc does not
return an error code. Hence it cannot be EPROBE_DEFFER, which is what
dev_err_probe() is looking for.

       Andrew

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Andrew Lunn]

> +static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
> +{
> +	u32 val;
> +
> + 	/* Setup the internal phy */
> +	val = (REG3_ENH |
> +	       FIELD_PREP(REG3_CFGMODE, 0x7) |
> +	       REG3_AUTOMDIX |
> +	       FIELD_PREP(REG3_PHYADDR, 8) |
> +	       REG3_LEDPOL |
> +	       REG3_PHYMDI |
> +	       REG3_CLKINEN |
> +	       REG3_PHYIP);
> +
> +	writel_relaxed(REG4_PWRUPRSTSIG, priv->regs + ETH_REG4);
> +	writel_relaxed(val, priv->regs + ETH_REG3);
> +	mdelay(10);

Probably the second _relaxed() should not be. You want it guaranteed
to be written out before you do the mdelay().

> +
> +	/* Set the internal phy id */
> +	writel_relaxed(FIELD_PREP(REG2_PHYID, 0x110181),
> +		       priv->regs + ETH_REG2);

So how does this play with what Heiner has been reporting recently?
What is the reset default? Who determined this value?

> +	/* Enable the internal phy */
> +	val |= REG3_PHYEN;
> +	writel_relaxed(val, priv->regs + ETH_REG3);
> +	writel_relaxed(0, priv->regs + ETH_REG4);
> +
> +	/* The phy needs a bit of time to come up */
> +	mdelay(10);

What do you mean by 'come up'? Not link up i assume. But maybe it will
not respond to MDIO requests?

    Andrew

Re: [PATCH net-next 0/2] net: mdio: add amlogic gxl mdio mux support

[Andrew Lunn]

On Mon, Jan 16, 2023 at 10:16:34AM +0100, Jerome Brunet wrote:
> Add support for the MDIO multiplexer found in the Amlogic GXL SoC family.
> This multiplexer allows to choose between the external (SoC pins) MDIO bus,
> or the internal one leading to the integrated 10/100M PHY.
> 
> This multiplexer has been handled with the mdio-mux-mmioreg generic driver
> so far. When it was added, it was thought the logic was handled by a
> single register.
> 
> It turns out more than a single register need to be properly set.
> As long as the device is using the Amlogic vendor bootloader, or upstream
> u-boot with net support, it is working fine since the kernel is inheriting
> the bootloader settings. Without net support in the bootloader, this glue
> comes unset in the kernel and only the external path may operate properly.
> 
> With this driver (and the associated DT update), the kernel no longer relies
> on the bootloader to set things up, fixing the problem.

Ideally, you should also post an actual user of this driver, i.e. the
DT updates.

> This has been tested on the aml-s905x-cc (LePotato) for the internal path
> and the aml-s912-pc (Tartiflette) for the external path.

So these exist in mainline, which is enough for me.

   Andrew

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Simon Horman]

On Wed, Jan 18, 2023 at 03:56:32AM +0100, Andrew Lunn wrote:
> > > >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > >> +	if (!priv)
> > > >> +		return -ENOMEM;
> > > >
> > > > nit: may be it is nicer to use dev_err_probe() here for consistency.
> > > 
> > > That was on purpose. I only use the `dev_err_probe()` when the probe may
> > > defer, which I don't expect here.
> > > 
> > > I don't mind changing if you prefer it this way.
> > 
> > I have no strong opinion on this :)
> 
> dev_err_probe() does not apply here, because devm_kzalloc does not
> return an error code. Hence it cannot be EPROBE_DEFFER, which is what
> dev_err_probe() is looking for.

Sure, there is no EPROBE_DEFFER.

But, FWIIW, my reading of the documentation for dev_err_probe()
is that it's use in such cases is acceptable.

Anyway, let's pass on my suggestion.

Re: [PATCH net-next 0/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet]

On Wed 18 Jan 2023 at 04:08, Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Jan 16, 2023 at 10:16:34AM +0100, Jerome Brunet wrote:
>> Add support for the MDIO multiplexer found in the Amlogic GXL SoC family.
>> This multiplexer allows to choose between the external (SoC pins) MDIO bus,
>> or the internal one leading to the integrated 10/100M PHY.
>> 
>> This multiplexer has been handled with the mdio-mux-mmioreg generic driver
>> so far. When it was added, it was thought the logic was handled by a
>> single register.
>> 
>> It turns out more than a single register need to be properly set.
>> As long as the device is using the Amlogic vendor bootloader, or upstream
>> u-boot with net support, it is working fine since the kernel is inheriting
>> the bootloader settings. Without net support in the bootloader, this glue
>> comes unset in the kernel and only the external path may operate properly.
>> 
>> With this driver (and the associated DT update), the kernel no longer relies
>> on the bootloader to set things up, fixing the problem.
>
> Ideally, you should also post an actual user of this driver, i.e. the
> DT updates.

I usually avoid doing this since the DT part is intended for another
maintainer. The idea is make life easy for them and let them pick the
entire series (or not). I don't mind sending the DT update along if it
is the perferred way with netdev.

FYI, the DT update would look like this :
https://gitlab.com/jbrunet/linux/-/commit/1d38ccf1b9f264111b1c56f18cfb4804227d3894.patch

>
>> This has been tested on the aml-s905x-cc (LePotato) for the internal path
>> and the aml-s912-pc (Tartiflette) for the external path.
>
> So these exist in mainline, which is enough for me.

Yes the boards exists in mainline, there are still using the mdio-mux-mmioreg driver
ATM

>
>    Andrew

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet]

On Wed 18 Jan 2023 at 04:02, Andrew Lunn <andrew@lunn.ch> wrote:

>> +static int gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
>> +{
>> +	u32 val;
>> +
>> + 	/* Setup the internal phy */
>> +	val = (REG3_ENH |
>> +	       FIELD_PREP(REG3_CFGMODE, 0x7) |
>> +	       REG3_AUTOMDIX |
>> +	       FIELD_PREP(REG3_PHYADDR, 8) |
>> +	       REG3_LEDPOL |
>> +	       REG3_PHYMDI |
>> +	       REG3_CLKINEN |
>> +	       REG3_PHYIP);
>> +
>> +	writel_relaxed(REG4_PWRUPRSTSIG, priv->regs + ETH_REG4);
>> +	writel_relaxed(val, priv->regs + ETH_REG3);
>> +	mdelay(10);
>
> Probably the second _relaxed() should not be. You want it guaranteed
> to be written out before you do the mdelay().

Good point, I'll have a look

>
>> +
>> +	/* Set the internal phy id */
>> +	writel_relaxed(FIELD_PREP(REG2_PHYID, 0x110181),
>> +		       priv->regs + ETH_REG2);
>
> So how does this play with what Heiner has been reporting recently?

What Heiner reported recently is related to the g12 family, not the gxl
which this driver address.

That being said, the g12 does things in a similar way - the glue
is just a bit different:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/mdio/mdio-mux-meson-g12a.c?h=v6.2-rc4#n165

> What is the reset default? Who determined this value?

It's the problem, the reset value is 0. That is why GXL does work with the
internal PHY if the bootloader has not initialized it before the kernel
comes up ... and there is no guarantee that it will.

The phy id value is arbitrary, same as the address. They match what AML
is using internally.

They have been kept to avoid making a mess if a vendor bootloader is
used with the mainline kernel, I guess.

I suppose any value could be used here, as long as it matches the value
in the PHY driver:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/meson-gxl.c?h=v6.2-rc4#n253

>
>> +	/* Enable the internal phy */
>> +	val |= REG3_PHYEN;
>> +	writel_relaxed(val, priv->regs + ETH_REG3);
>> +	writel_relaxed(0, priv->regs + ETH_REG4);
>> +
>> +	/* The phy needs a bit of time to come up */
>> +	mdelay(10);
>
> What do you mean by 'come up'? Not link up i assume. But maybe it will
> not respond to MDIO requests?

Yes this MDIO multiplexer is also the glue that provides power and
clocks to the internal PHY. Once the internal PHY is selected, it needs
a bit a of time before it is usuable. 

>
>     Andrew

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Andrew Lunn]

> >> +
> >> +	/* Set the internal phy id */
> >> +	writel_relaxed(FIELD_PREP(REG2_PHYID, 0x110181),
> >> +		       priv->regs + ETH_REG2);
> >
> > So how does this play with what Heiner has been reporting recently?
> 
> What Heiner reported recently is related to the g12 family, not the gxl
> which this driver address.
> 
> That being said, the g12 does things in a similar way - the glue
> is just a bit different:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/mdio/mdio-mux-meson-g12a.c?h=v6.2-rc4#n165
> 
> > What is the reset default? Who determined this value?
> 
> It's the problem, the reset value is 0. That is why GXL does work with the
> internal PHY if the bootloader has not initialized it before the kernel
> comes up ... and there is no guarantee that it will.
> 
> The phy id value is arbitrary, same as the address. They match what AML
> is using internally.

Please document where these values have come from. In the future we
might need to point a finger when it all goes horribly wrong.

> They have been kept to avoid making a mess if a vendor bootloader is
> used with the mainline kernel, I guess.
> 
> I suppose any value could be used here, as long as it matches the value
> in the PHY driver:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/meson-gxl.c?h=v6.2-rc4#n253

Some Marvell Ethernet switches with integrated PHYs have IDs with the
vendor part set to Marvell, but the lower part is 0. The date sheet
even says this is deliberate, you need to look at some other register
in the switches address space to determine what the part is. That
works O.K in the vendor crap monolithic driver, but not for Linux
which separates the drivers up. So we have to intercept the reads and
fill in the lower part. And we have no real knowledge if the PHYs are
all the same, or there are differences. So we put in the switch ID,
and the PHY driver then has an entry per switch. That gives us some
future wiggle room if we find the PHYs are actually different.

Is there any indication in the datasheets that the PHY is the exact
same one as in the g12? Are we really safe to reuse this value between
different SoCs?

I actually find it an odd feature. Does the datasheet say anything
about Why you can set the ID in software? The ID describes the
hardware, and software configuration should not be able to change the
hardware in any meaningful way.

> >> +	/* Enable the internal phy */
> >> +	val |= REG3_PHYEN;
> >> +	writel_relaxed(val, priv->regs + ETH_REG3);
> >> +	writel_relaxed(0, priv->regs + ETH_REG4);
> >> +
> >> +	/* The phy needs a bit of time to come up */
> >> +	mdelay(10);
> >
> > What do you mean by 'come up'? Not link up i assume. But maybe it will
> > not respond to MDIO requests?
> 
> Yes this MDIO multiplexer is also the glue that provides power and
> clocks to the internal PHY. Once the internal PHY is selected, it needs
> a bit a of time before it is usuable. 

O.K, please reword it to indicate power up, not link up.

     Andrew

Re: [PATCH net-next 0/2] net: mdio: add amlogic gxl mdio mux support

[Andrew Lunn]

> I usually avoid doing this since the DT part is intended for another
> maintainer. The idea is make life easy for them and let them pick the
> entire series (or not). I don't mind sending the DT update along if it
> is the perferred way with netdev.
> 
> FYI, the DT update would look like this :
> https://gitlab.com/jbrunet/linux/-/commit/1d38ccf1b9f264111b1c56f18cfb4804227d3894.patch
> 
> >
> >> This has been tested on the aml-s905x-cc (LePotato) for the internal path
> >> and the aml-s912-pc (Tartiflette) for the external path.
> >
> > So these exist in mainline, which is enough for me.
> 
> Yes the boards exists in mainline, there are still using the mdio-mux-mmioreg driver
> ATM

The point of posting the actual users is sometimes we get vendor crap
with no actual in tree users. We want to avoid that. It can be enough
to mention in the cover letter than a future patchset will change the
DT files X, Y and Z, making it clear there are in tree users.

   Andrew

Re: [PATCH net-next 2/2] net: mdio: add amlogic gxl mdio mux support

[Jerome Brunet]

On Thu 19 Jan 2023 at 18:17, Andrew Lunn <andrew@lunn.ch> wrote:

>> >> +
>> >> +	/* Set the internal phy id */
>> >> +	writel_relaxed(FIELD_PREP(REG2_PHYID, 0x110181),
>> >> +		       priv->regs + ETH_REG2);
>> >
>> > So how does this play with what Heiner has been reporting recently?
>> 
>> What Heiner reported recently is related to the g12 family, not the gxl
>> which this driver address.
>> 
>> That being said, the g12 does things in a similar way - the glue
>> is just a bit different:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/mdio/mdio-mux-meson-g12a.c?h=v6.2-rc4#n165
>> 
>> > What is the reset default? Who determined this value?
>> 
>> It's the problem, the reset value is 0. That is why GXL does work with the
>> internal PHY if the bootloader has not initialized it before the kernel
>> comes up ... and there is no guarantee that it will.
>> 
>> The phy id value is arbitrary, same as the address. They match what AML
>> is using internally.
>
> Please document where these values have come from. In the future we
> might need to point a finger when it all goes horribly wrong.
>

OK

>> They have been kept to avoid making a mess if a vendor bootloader is
>> used with the mainline kernel, I guess.
>> 
>> I suppose any value could be used here, as long as it matches the value
>> in the PHY driver:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/meson-gxl.c?h=v6.2-rc4#n253
>
> Some Marvell Ethernet switches with integrated PHYs have IDs with the
> vendor part set to Marvell, but the lower part is 0. The date sheet
> even says this is deliberate, you need to look at some other register
> in the switches address space to determine what the part is. That
> works O.K in the vendor crap monolithic driver, but not for Linux
> which separates the drivers up. So we have to intercept the reads and
> fill in the lower part. And we have no real knowledge if the PHYs are
> all the same, or there are differences. So we put in the switch ID,
> and the PHY driver then has an entry per switch. That gives us some
> future wiggle room if we find the PHYs are actually different.
>
> Is there any indication in the datasheets that the PHY is the exact
> same one as in the g12? Are we really safe to reuse this value between
> different SoCs?

There is zero information about the PHY in the datasheet.
The gxl and g12 don't use the same ID values.
The PHY ip is very similar but slightly different between the 2.
(see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/meson-gxl.c)

My guess is the g12 as another version of the IP, with some bug fixed.
The integration (clocking scheme mostly) is also different, which is why
the mux/glue is different.

>
> I actually find it an odd feature. Does the datasheet say anything
> about Why you can set the ID in software? The ID describes the
> hardware, and software configuration should not be able to change the
> hardware in any meaningful way.

Again, zero information. 
It is a bought IP (similar to the Rockchip judging by the PHY driver).
I'm not surprised the provider of the IP would make the ID
easy to configure. AML chose to keep that configurable through the glue,
instead of fixing it. This is how it is.

>
>> >> +	/* Enable the internal phy */
>> >> +	val |= REG3_PHYEN;
>> >> +	writel_relaxed(val, priv->regs + ETH_REG3);
>> >> +	writel_relaxed(0, priv->regs + ETH_REG4);
>> >> +
>> >> +	/* The phy needs a bit of time to come up */
>> >> +	mdelay(10);
>> >
>> > What do you mean by 'come up'? Not link up i assume. But maybe it will
>> > not respond to MDIO requests?
>> 
>> Yes this MDIO multiplexer is also the glue that provides power and
>> clocks to the internal PHY. Once the internal PHY is selected, it needs
>> a bit a of time before it is usuable. 
>
> O.K, please reword it to indicate power up, not link up.
>

Sure

>      Andrew