[PATCH net-next 0/5] net/fsl: xgmac_mdio: Preamble suppression and custom MDC frequencies

[PATCH net-next 0/5] net/fsl: xgmac_mdio: Preamble suppression and custom MDC frequencies

[Tobias Waldekranz]

The first patch removes the docs for a binding that has never been
supported by the driver as far as I can see. This is a bit of a
mystery to me, maybe Freescale/NXP had/has support for it in an
internal version?

We then start working on the xgmac_mdio driver, converting the driver
to exclusively use managed resources, thereby simplifying the error
paths. Suggested by Andrew.

Preamble suppression is then added, followed by MDC frequency
customization. Neither code will change any bits if the corresponding
dt properties are not specified, so as to not trample on any setup
done by the bootloader, which boards might have relied on up to now.

Finally, we document the new bindings.

Tested on a T1023 based board.

Tobias Waldekranz (5):
  dt-bindings: net: xgmac_mdio: Remove unsupported "bus-frequency"
  net/fsl: xgmac_mdio: Use managed device resources
  net/fsl: xgmac_mdio: Support preamble suppression
  net/fsl: xgmac_mdio: Support setting the MDC frequency
  dt-bindings: net: xgmac_mdio: Add "clock-frequency" and
    "suppress-preamble"

 .../devicetree/bindings/net/fsl-fman.txt      | 22 +++--
 drivers/net/ethernet/freescale/xgmac_mdio.c   | 88 ++++++++++++-------
 2 files changed, 74 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH net-next 1/5] dt-bindings: net: xgmac_mdio: Remove unsupported "bus-frequency"

[Tobias Waldekranz]

This property has never been supported by the driver. The kernel has
settled on "clock-frequency" as the standard name for this binding, so
once that is supported we will document that instead.

Fixes: 7f93c9d90f4d ("power/fsl: add MDIO dt binding for FMan")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 Documentation/devicetree/bindings/net/fsl-fman.txt | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fsl-fman.txt b/Documentation/devicetree/bindings/net/fsl-fman.txt
index 020337f3c05f..cd5288fb4318 100644
--- a/Documentation/devicetree/bindings/net/fsl-fman.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fman.txt
@@ -388,15 +388,6 @@ PROPERTIES
 		Value type: <prop-encoded-array>
 		Definition: A standard property.
 
-- bus-frequency
-		Usage: optional
-		Value type: <u32>
-		Definition: Specifies the external MDIO bus clock speed to
-		be used, if different from the standard 2.5 MHz.
-		This may be due to the standard speed being unsupported (e.g.
-		due to a hardware problem), or to advertise that all relevant
-		components in the system support a faster speed.
-
 - interrupts
 		Usage: required for external MDIO
 		Value type: <prop-encoded-array>
-- 
2.25.1

Re: [PATCH net-next 1/5] dt-bindings: net: xgmac_mdio: Remove unsupported "bus-frequency"

[Andrew Lunn]

On Wed, Jan 26, 2022 at 11:14:28AM +0100, Tobias Waldekranz wrote:
> This property has never been supported by the driver. The kernel has
> settled on "clock-frequency" as the standard name for this binding, so
> once that is supported we will document that instead.
> 
> Fixes: 7f93c9d90f4d ("power/fsl: add MDIO dt binding for FMan")
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

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

    Andrew

[PATCH net-next 2/5] net/fsl: xgmac_mdio: Use managed device resources

[Tobias Waldekranz]

All of the resources used by this driver has managed interfaces, so
use them. Heed the warning in the comment before platform_get_resource
and use a bare devm_ioremap to allow for non-exclusive access to the
IO memory.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/ethernet/freescale/xgmac_mdio.c | 35 ++++-----------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 266e562bd67a..40442d64a247 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -273,7 +273,7 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	bus = mdiobus_alloc_size(sizeof(struct mdio_fsl_priv));
+	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(struct mdio_fsl_priv));
 	if (!bus)
 		return -ENOMEM;
 
@@ -284,13 +284,11 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	bus->probe_capabilities = MDIOBUS_C22_C45;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%pa", &res->start);
 
-	/* Set the PHY base address */
 	priv = bus->priv;
-	priv->mdio_base = ioremap(res->start, resource_size(res));
-	if (!priv->mdio_base) {
-		ret = -ENOMEM;
-		goto err_ioremap;
-	}
+	priv->mdio_base = devm_ioremap(&pdev->dev, res->start,
+				       resource_size(res));
+	if (IS_ERR(priv->mdio_base))
+		return PTR_ERR(priv->mdio_base);
 
 	/* For both ACPI and DT cases, endianness of MDIO controller
 	 * needs to be specified using "little-endian" property.
@@ -312,31 +310,11 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 		ret = -EINVAL;
 	if (ret) {
 		dev_err(&pdev->dev, "cannot register MDIO bus\n");
-		goto err_registration;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, bus);
 
-	return 0;
-
-err_registration:
-	iounmap(priv->mdio_base);
-
-err_ioremap:
-	mdiobus_free(bus);
-
-	return ret;
-}
-
-static int xgmac_mdio_remove(struct platform_device *pdev)
-{
-	struct mii_bus *bus = platform_get_drvdata(pdev);
-	struct mdio_fsl_priv *priv = bus->priv;
-
-	mdiobus_unregister(bus);
-	iounmap(priv->mdio_base);
-	mdiobus_free(bus);
-
 	return 0;
 }
 
@@ -364,7 +342,6 @@ static struct platform_driver xgmac_mdio_driver = {
 		.acpi_match_table = xgmac_acpi_match,
 	},
 	.probe = xgmac_mdio_probe,
-	.remove = xgmac_mdio_remove,
 };
 
 module_platform_driver(xgmac_mdio_driver);
-- 
2.25.1

Re: [PATCH net-next 2/5] net/fsl: xgmac_mdio: Use managed device resources

[Andrew Lunn]

On Wed, Jan 26, 2022 at 11:14:29AM +0100, Tobias Waldekranz wrote:
> All of the resources used by this driver has managed interfaces, so
> use them. Heed the warning in the comment before platform_get_resource
> and use a bare devm_ioremap to allow for non-exclusive access to the
> IO memory.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

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

    Andrew

[PATCH net-next 3/5] net/fsl: xgmac_mdio: Support preamble suppression

[Tobias Waldekranz]

Support the standard "suppress-preamble" attribute to disable preamble
generation.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/ethernet/freescale/xgmac_mdio.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 40442d64a247..18bf2370d45a 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -39,6 +39,7 @@ struct tgec_mdio_controller {
 #define MDIO_STAT_CLKDIV(x)	(((x>>1) & 0xff) << 8)
 #define MDIO_STAT_BSY		BIT(0)
 #define MDIO_STAT_RD_ER		BIT(1)
+#define MDIO_STAT_PRE_DIS	BIT(5)
 #define MDIO_CTL_DEV_ADDR(x) 	(x & 0x1f)
 #define MDIO_CTL_PORT_ADDR(x)	((x & 0x1f) << 5)
 #define MDIO_CTL_PRE_DIS	BIT(10)
@@ -254,6 +255,21 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 	return ret;
 }
 
+static void xgmac_mdio_set_suppress_preamble(struct mii_bus *bus)
+{
+	struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
+	struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
+	struct device *dev = bus->parent;
+	u32 mdio_stat;
+
+	if (!device_property_read_bool(dev, "suppress-preamble"))
+		return;
+
+	mdio_stat = xgmac_read32(&regs->mdio_stat, priv->is_little_endian);
+	mdio_stat |= MDIO_STAT_PRE_DIS;
+	xgmac_write32(mdio_stat, &regs->mdio_stat, priv->is_little_endian);
+}
+
 static int xgmac_mdio_probe(struct platform_device *pdev)
 {
 	struct fwnode_handle *fwnode;
@@ -301,6 +317,8 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
 
+	xgmac_mdio_set_suppress_preamble(bus);
+
 	fwnode = pdev->dev.fwnode;
 	if (is_of_node(fwnode))
 		ret = of_mdiobus_register(bus, to_of_node(fwnode));
-- 
2.25.1

Re: [PATCH net-next 3/5] net/fsl: xgmac_mdio: Support preamble suppression

[Andrew Lunn]

On Wed, Jan 26, 2022 at 11:14:30AM +0100, Tobias Waldekranz wrote:
> Support the standard "suppress-preamble" attribute to disable preamble
> generation.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

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

    Andrew

[PATCH net-next 4/5] net/fsl: xgmac_mdio: Support setting the MDC frequency

[Tobias Waldekranz]

Support the standard "clock-frequency" attribute to set the generated
MDC frequency. If not specified, the driver will leave the divisor
bits untouched.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/ethernet/freescale/xgmac_mdio.c | 35 ++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 18bf2370d45a..2199f8f4ff68 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -14,6 +14,7 @@
 
 #include <linux/acpi.h>
 #include <linux/acpi_mdio.h>
+#include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mdio.h>
@@ -36,7 +37,7 @@ struct tgec_mdio_controller {
 } __packed;
 
 #define MDIO_STAT_ENC		BIT(6)
-#define MDIO_STAT_CLKDIV(x)	(((x>>1) & 0xff) << 8)
+#define MDIO_STAT_CLKDIV(x)	(((x) & 0x1ff) << 7)
 #define MDIO_STAT_BSY		BIT(0)
 #define MDIO_STAT_RD_ER		BIT(1)
 #define MDIO_STAT_PRE_DIS	BIT(5)
@@ -51,6 +52,8 @@ struct tgec_mdio_controller {
 
 struct mdio_fsl_priv {
 	struct	tgec_mdio_controller __iomem *mdio_base;
+	struct clk *enet_clk;
+	u32	mdc_freq;
 	bool	is_little_endian;
 	bool	has_a009885;
 	bool	has_a011043;
@@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 	return ret;
 }
 
+static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus)
+{
+	struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
+	struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
+	struct device *dev = bus->parent;
+	u32 mdio_stat, div;
+
+	if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq))
+		return;
+
+	priv->enet_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->enet_clk)) {
+		dev_err(dev, "Input clock unknown, not changing MDC frequency");
+		return;
+	}
+
+	div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2;
+	if (div < 5 || div > 0x1ff) {
+		dev_err(dev, "Requested MDC frequecy is out of range, ignoring");
+		return;
+	}
+
+	mdio_stat = xgmac_read32(&regs->mdio_stat, priv->is_little_endian);
+	mdio_stat &= ~MDIO_STAT_CLKDIV(0x1ff);
+	mdio_stat |= MDIO_STAT_CLKDIV(div);
+	xgmac_write32(mdio_stat, &regs->mdio_stat, priv->is_little_endian);
+}
+
 static void xgmac_mdio_set_suppress_preamble(struct mii_bus *bus)
 {
 	struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
@@ -319,6 +350,8 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 
 	xgmac_mdio_set_suppress_preamble(bus);
 
+	xgmac_mdio_set_mdc_freq(bus);
+
 	fwnode = pdev->dev.fwnode;
 	if (is_of_node(fwnode))
 		ret = of_mdiobus_register(bus, to_of_node(fwnode));
-- 
2.25.1

Re: [PATCH net-next 4/5] net/fsl: xgmac_mdio: Support setting the MDC frequency

[Andrew Lunn]

Hi Tobias

>  struct mdio_fsl_priv {
>  	struct	tgec_mdio_controller __iomem *mdio_base;
> +	struct clk *enet_clk;

It looks like there is a whitespace issue here?

> +	u32	mdc_freq;
>  	bool	is_little_endian;
>  	bool	has_a009885;
>  	bool	has_a011043;
> @@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
>  	return ret;
>  }
>  
> +static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus)
> +{
> +	struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
> +	struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
> +	struct device *dev = bus->parent;
> +	u32 mdio_stat, div;
> +
> +	if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq))
> +		return;
> +
> +	priv->enet_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->enet_clk)) {
> +		dev_err(dev, "Input clock unknown, not changing MDC frequency");

Is the clock optional or mandatory?

If mandatory, then i would prefer -ENODEV and fail the probe.

> +		return;
> +	}
> +
> +	div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2;
> +	if (div < 5 || div > 0x1ff) {
> +		dev_err(dev, "Requested MDC frequecy is out of range, ignoring");

and here return -EINVAL.

I always think it is best to make it obvious something is broken. One
additional line on the console can be missed for a while. All the
Ethernet devices missing because the PHYs are missing, because the
MDIO bus is missing is going to get noticed very quickly.

     Andrew

Re: [PATCH net-next 4/5] net/fsl: xgmac_mdio: Support setting the MDC frequency

[Tobias Waldekranz]

On Wed, Jan 26, 2022 at 14:07, Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Tobias
>
>>  struct mdio_fsl_priv {
>>  	struct	tgec_mdio_controller __iomem *mdio_base;
>> +	struct clk *enet_clk;
>
> It looks like there is a whitespace issue here?
>

Ahh, sorry about that!

>> +	u32	mdc_freq;
>>  	bool	is_little_endian;
>>  	bool	has_a009885;
>>  	bool	has_a011043;
>> @@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
>>  	return ret;
>>  }
>>  
>> +static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus)
>> +{
>> +	struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
>> +	struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
>> +	struct device *dev = bus->parent;
>> +	u32 mdio_stat, div;
>> +
>> +	if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq))
>> +		return;
>> +
>> +	priv->enet_clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->enet_clk)) {
>> +		dev_err(dev, "Input clock unknown, not changing MDC frequency");
>
> Is the clock optional or mandatory?

As the code is now, it is mandatory. I could add some default frequency,
but I fear that could do more harm than good?

> If mandatory, then i would prefer -ENODEV and fail the probe.
>
>> +		return;
>> +	}
>> +
>> +	div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2;
>> +	if (div < 5 || div > 0x1ff) {
>> +		dev_err(dev, "Requested MDC frequecy is out of range, ignoring");
>
> and here return -EINVAL.
>
> I always think it is best to make it obvious something is broken. One
> additional line on the console can be missed for a while. All the
> Ethernet devices missing because the PHYs are missing, because the
> MDIO bus is missing is going to get noticed very quickly.

Darn, the last thing I did was to change this from a hard to a soft
error :)

Ok, no worries about regressions for deployed stuff already out there?

Re: [PATCH net-next 4/5] net/fsl: xgmac_mdio: Support setting the MDC frequency

[Andrew Lunn]

> >> +	if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq))
> >> +		return;
> >> +
> >> +	priv->enet_clk = devm_clk_get(dev, NULL);
> >> +	if (IS_ERR(priv->enet_clk)) {
> >> +		dev_err(dev, "Input clock unknown, not changing MDC frequency");
> >
> > Is the clock optional or mandatory?
> 
> As the code is now, it is mandatory. I could add some default frequency,
> but I fear that could do more harm than good?

As i said in the reply to the DT patch, it is mandatory if the
"clock-frequency" parameter is present.

> Ok, no worries about regressions for deployed stuff already out there?

It would only cause a regression if a DT blob happened to have a
'clock-frequency' parameter and not clock, which it should not.

        Andrew

[PATCH net-next 5/5] dt-bindings: net: xgmac_mdio: Add "clock-frequency" and "suppress-preamble"

[Tobias Waldekranz]

The driver now supports the standard "clock-frequency" and
"suppress-preamble" properties, do document them in the binding
description.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 .../devicetree/bindings/net/fsl-fman.txt      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl-fman.txt b/Documentation/devicetree/bindings/net/fsl-fman.txt
index cd5288fb4318..801efc7d6818 100644
--- a/Documentation/devicetree/bindings/net/fsl-fman.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fman.txt
@@ -388,6 +388,25 @@ PROPERTIES
 		Value type: <prop-encoded-array>
 		Definition: A standard property.
 
+- clocks
+		Usage: optional
+		Value type: <phandle>
+		Definition: A reference to the input clock of the controller
+		from which the MDC frequency is derived.
+
+- clock-frequency
+		Usage: optional
+		Value type: <u32>
+		Definition: Specifies the external MDC frequency, in Hertz, to
+		be used. Requires that the input clock is specified in the
+		"clocks" property. See also: mdio.yaml.
+
+- suppress-preamble
+		Usage: optional
+		Value type: <boolean>
+		Definition: Disable generation of preamble bits. See also:
+		mdio.yaml.
+
 - interrupts
 		Usage: required for external MDIO
 		Value type: <prop-encoded-array>
-- 
2.25.1

Re: [PATCH net-next 5/5] dt-bindings: net: xgmac_mdio: Add "clock-frequency" and "suppress-preamble"

[Andrew Lunn]

On Wed, Jan 26, 2022 at 11:14:32AM +0100, Tobias Waldekranz wrote:
> The driver now supports the standard "clock-frequency" and
> "suppress-preamble" properties, do document them in the binding
> description.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

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

However, you could convert to yaml as well, if you wanted.

> +- clocks
> +		Usage: optional
> +		Value type: <phandle>
> +		Definition: A reference to the input clock of the controller
> +		from which the MDC frequency is derived.

That answers my question. However, in the presence of a
'clock-frequency' property it cannot be optional, so -ENODEV seems
reasonable if it is missing. Potentially you also need to handle
-EPROBE_DEFER, although it seems quiet unlikely for an internal SoC
clock.

	Andrew