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

[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