[PATCH net 0/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[PATCH net 0/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Tobias Waldekranz]

The individual messages mostly speak for themselves.

It is very possible that there are more chips out there that are
impacted by this, but I only have access to the errata document for
the T1024 family, so I've limited the DT changes to the exact FMan
version used in that device. Hopefully someone from NXP can supply a
follow-up if need be.

The final commit is an unrelated fix that was brought to my attention
by sparse.

Tobias Waldekranz (4):
  net/fsl: xgmac_mdio: Add workaround for erratum A-009885
  dt-bindings: net: Document fsl,erratum-a009885
  powerpc/fsl/dts: Enable WA for erratum A-009885 on fman3l MDIO buses
  net/fsl: xgmac_mdio: Fix incorrect iounmap when removing module

 .../devicetree/bindings/net/fsl-fman.txt      |  9 ++++++
 arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi |  2 ++
 drivers/net/ethernet/freescale/xgmac_mdio.c   | 28 ++++++++++++++-----
 3 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Tobias Waldekranz]

Once an MDIO read transaction is initiated, we must read back the data
register within 16 MDC cycles after the transaction completes. Outside
of this window, reads may return corrupt data.

Therefore, disable local interrupts in the critical section, to
maximize the probability that we can satisfy this requirement.

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

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index 5b8b9bcf41a2..bf566ac3195b 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -51,6 +51,7 @@ struct tgec_mdio_controller {
 struct mdio_fsl_priv {
 	struct	tgec_mdio_controller __iomem *mdio_base;
 	bool	is_little_endian;
+	bool	has_a009885;
 	bool	has_a011043;
 };
 
@@ -186,10 +187,10 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 {
 	struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv;
 	struct tgec_mdio_controller __iomem *regs = priv->mdio_base;
+	unsigned long flags;
 	uint16_t dev_addr;
 	uint32_t mdio_stat;
 	uint32_t mdio_ctl;
-	uint16_t value;
 	int ret;
 	bool endian = priv->is_little_endian;
 
@@ -221,12 +222,18 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 			return ret;
 	}
 
+	if (priv->has_a009885)
+		/* Once the operation completes, i.e. MDIO_STAT_BSY clears, we
+		 * must read back the data register within 16 MDC cycles.
+		 */
+		local_irq_save(flags);
+
 	/* Initiate the read */
 	xgmac_write32(mdio_ctl | MDIO_CTL_READ, &regs->mdio_ctl, endian);
 
 	ret = xgmac_wait_until_done(&bus->dev, regs, endian);
 	if (ret)
-		return ret;
+		goto irq_restore;
 
 	/* Return all Fs if nothing was there */
 	if ((xgmac_read32(&regs->mdio_stat, endian) & MDIO_STAT_RD_ER) &&
@@ -234,13 +241,17 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)
 		dev_dbg(&bus->dev,
 			"Error while reading PHY%d reg at %d.%hhu\n",
 			phy_id, dev_addr, regnum);
-		return 0xffff;
+		ret = 0xffff;
+	} else {
+		ret = xgmac_read32(&regs->mdio_data, endian) & 0xffff;
+		dev_dbg(&bus->dev, "read %04x\n", ret);
 	}
 
-	value = xgmac_read32(&regs->mdio_data, endian) & 0xffff;
-	dev_dbg(&bus->dev, "read %04x\n", value);
+irq_restore:
+	if (priv->has_a009885)
+		local_irq_restore(flags);
 
-	return value;
+	return ret;
 }
 
 static int xgmac_mdio_probe(struct platform_device *pdev)
@@ -287,6 +298,8 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 	priv->is_little_endian = device_property_read_bool(&pdev->dev,
 							   "little-endian");
 
+	priv->has_a009885 = device_property_read_bool(&pdev->dev,
+						      "fsl,erratum-a009885");
 	priv->has_a011043 = device_property_read_bool(&pdev->dev,
 						      "fsl,erratum-a011043");
 
-- 
2.25.1

Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Andrew Lunn]

On Sun, Jan 16, 2022 at 10:15:26PM +0100, Tobias Waldekranz wrote:
> Once an MDIO read transaction is initiated, we must read back the data
> register within 16 MDC cycles after the transaction completes. Outside
> of this window, reads may return corrupt data.
> 
> Therefore, disable local interrupts in the critical section, to
> maximize the probability that we can satisfy this requirement.

Since this is for net, a Fixes: tag would be nice. Maybe that would be
for the commit which added this driver, or maybe when the DTSI files
for the SOCs which have this errata we added?

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

    Andrew

Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Tobias Waldekranz]

On Sun, Jan 16, 2022 at 23:02, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Jan 16, 2022 at 10:15:26PM +0100, Tobias Waldekranz wrote:
>> Once an MDIO read transaction is initiated, we must read back the data
>> register within 16 MDC cycles after the transaction completes. Outside
>> of this window, reads may return corrupt data.
>> 
>> Therefore, disable local interrupts in the critical section, to
>> maximize the probability that we can satisfy this requirement.
>
> Since this is for net, a Fixes: tag would be nice. Maybe that would be
> for the commit which added this driver, or maybe when the DTSI files
> for the SOCs which have this errata we added?

Alright, I wasn't sure how to tag WAs for errata since it is more about
the hardware than the driver. Should I send a v2 even if nothing else
pops up, or is this more of a if-you're-sending-a-v2-anyway type of
comment?

Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Andrew Lunn]

On Mon, Jan 17, 2022 at 08:24:22AM +0100, Tobias Waldekranz wrote:
> On Sun, Jan 16, 2022 at 23:02, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Sun, Jan 16, 2022 at 10:15:26PM +0100, Tobias Waldekranz wrote:
> >> Once an MDIO read transaction is initiated, we must read back the data
> >> register within 16 MDC cycles after the transaction completes. Outside
> >> of this window, reads may return corrupt data.
> >> 
> >> Therefore, disable local interrupts in the critical section, to
> >> maximize the probability that we can satisfy this requirement.
> >
> > Since this is for net, a Fixes: tag would be nice. Maybe that would be
> > for the commit which added this driver, or maybe when the DTSI files
> > for the SOCs which have this errata we added?
> 
> Alright, I wasn't sure how to tag WAs for errata since it is more about
> the hardware than the driver.

The tag gives the backporter an idea how far back to go. If support
for this SoC has only recently been added, there is no need to
backport a long way. If it is an old SoC, then maybe more effort
should be put into the backport?

> Should I send a v2 even if nothing else
> pops up, or is this more of a if-you're-sending-a-v2-anyway type of
> comment?

If you reply with a Fixes: patchwork will automagically append it like
it does Reviewed-by, Tested-by etc.

   Andrew

Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Jakub Kicinski]

On Mon, 17 Jan 2022 15:00:41 +0100 Andrew Lunn wrote:
> > Should I send a v2 even if nothing else
> > pops up, or is this more of a if-you're-sending-a-v2-anyway type of
> > comment?  
> 
> If you reply with a Fixes: patchwork will automagically append it like
> it does Reviewed-by, Tested-by etc.

That part is pretty finicky, it's supposed to work but when I apply
these I only get review tags from Andrew and a Fixes tag already
present on the last patch :(

A v2 with Fixes tags included in the posting would be best after all.
Thanks!

Re: [PATCH net 1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[Tobias Waldekranz]

On Sun, Jan 16, 2022 at 22:15, Tobias Waldekranz <tobias@waldekranz.com> wrote:
> Once an MDIO read transaction is initiated, we must read back the data
> register within 16 MDC cycles after the transaction completes. Outside
> of this window, reads may return corrupt data.
>
> Therefore, disable local interrupts in the critical section, to
> maximize the probability that we can satisfy this requirement.
>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Fixes: d55ad2967d89 ("powerpc/mpc85xx: Create dts components for the FSL QorIQ DPAA FMan")

[PATCH net 2/4] dt-bindings: net: Document fsl,erratum-a009885

[Tobias Waldekranz]

Update FMan binding documentation with the newly added workaround for
erratum A-009885.

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

diff --git a/Documentation/devicetree/bindings/net/fsl-fman.txt b/Documentation/devicetree/bindings/net/fsl-fman.txt
index c00fb0d22c7b..020337f3c05f 100644
--- a/Documentation/devicetree/bindings/net/fsl-fman.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fman.txt
@@ -410,6 +410,15 @@ PROPERTIES
 		The settings and programming routines for internal/external
 		MDIO are different. Must be included for internal MDIO.
 
+- fsl,erratum-a009885
+		Usage: optional
+		Value type: <boolean>
+		Definition: Indicates the presence of the A009885
+		erratum describing that the contents of MDIO_DATA may
+		become corrupt unless it is read within 16 MDC cycles
+		of MDIO_CFG[BSY] being cleared, when performing an
+		MDIO read operation.
+
 - fsl,erratum-a011043
 		Usage: optional
 		Value type: <boolean>
-- 
2.25.1

Re: [PATCH net 2/4] dt-bindings: net: Document fsl,erratum-a009885

[Andrew Lunn]

On Sun, Jan 16, 2022 at 10:15:27PM +0100, Tobias Waldekranz wrote:
> Update FMan binding documentation with the newly added workaround for
> erratum A-009885.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

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

    Andrew

[PATCH net 3/4] powerpc/fsl/dts: Enable WA for erratum A-009885 on fman3l MDIO buses

[Tobias Waldekranz]

This block is used in (at least) T1024 and T1040, including their
variants like T1023 etc.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi
index c90702b04a53..48e5cd61599c 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi
@@ -79,6 +79,7 @@ mdio0: mdio@fc000 {
 		#size-cells = <0>;
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xfc000 0x1000>;
+		fsl,erratum-a009885;
 	};
 
 	xmdio0: mdio@fd000 {
@@ -86,6 +87,7 @@ xmdio0: mdio@fd000 {
 		#size-cells = <0>;
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xfd000 0x1000>;
+		fsl,erratum-a009885;
 	};
 };
 
-- 
2.25.1

Re: [PATCH net 3/4] powerpc/fsl/dts: Enable WA for erratum A-009885 on fman3l MDIO buses

[Tobias Waldekranz]

On Sun, Jan 16, 2022 at 22:15, Tobias Waldekranz <tobias@waldekranz.com> wrote:
> This block is used in (at least) T1024 and T1040, including their
> variants like T1023 etc.
>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Fixes: d55ad2967d89 ("powerpc/mpc85xx: Create dts components for the FSL QorIQ DPAA FMan")

[PATCH net 4/4] net/fsl: xgmac_mdio: Fix incorrect iounmap when removing module

[Tobias Waldekranz]

As reported by sparse: In the remove path, the driver would attempt to
unmap its own priv pointer - instead of the io memory that it mapped
in probe.

Fixes: 9f35a7342cff ("net/fsl: introduce Freescale 10G MDIO driver")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/ethernet/freescale/xgmac_mdio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c
index bf566ac3195b..266e562bd67a 100644
--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -331,9 +331,10 @@ static int xgmac_mdio_probe(struct platform_device *pdev)
 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(bus->priv);
+	iounmap(priv->mdio_base);
 	mdiobus_free(bus);
 
 	return 0;
-- 
2.25.1

Re: [PATCH net 4/4] net/fsl: xgmac_mdio: Fix incorrect iounmap when removing module

[Andrew Lunn]

On Sun, Jan 16, 2022 at 10:15:29PM +0100, Tobias Waldekranz wrote:
> As reported by sparse: In the remove path, the driver would attempt to
> unmap its own priv pointer - instead of the io memory that it mapped
> in probe.

Hi Tobias

The change itself is O.K.

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

But you could also change to devm_ so the core will handle the unmap,
it is very unlikely to unmap the wrong thing.

     Andrew

Re: [PATCH net 4/4] net/fsl: xgmac_mdio: Fix incorrect iounmap when removing module

[Tobias Waldekranz]

On Sun, Jan 16, 2022 at 22:54, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Jan 16, 2022 at 10:15:29PM +0100, Tobias Waldekranz wrote:
>> As reported by sparse: In the remove path, the driver would attempt to
>> unmap its own priv pointer - instead of the io memory that it mapped
>> in probe.
>
> Hi Tobias
>
> The change itself is O.K.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
> But you could also change to devm_ so the core will handle the unmap,
> it is very unlikely to unmap the wrong thing.

Good idea. I have some more changes I want to include in this driver,
but they are more net-next material. Will try to work in this change as
well.