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

[PATCH v2 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.

v1 -> v2:
 - Added Fixed tags to 1/4 and 3/4

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 v2 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.

Fixes: d55ad2967d89 ("powerpc/mpc85xx: Create dts components for the FSL QorIQ DPAA FMan")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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

[PATCH v2 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 v2 net 2/4] dt-bindings: net: Document fsl,erratum-a009885

[Rob Herring]

On Tue, Jan 18, 2022 at 10:50:51PM +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>
> ---
>  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

Adding errata properties doesn't work because then you have to update 
your dtb to fix the issue where as if you use the compatible property 
(specific to the SoC) you can fix the issue with just a (stable) kernel 
update.

Yes, I see we already have some, but doesn't mean we need more of them.

> +		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 v2 net 2/4] dt-bindings: net: Document fsl,erratum-a009885

[Tobias Waldekranz]

On Thu, Jan 20, 2022 at 20:08, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 18, 2022 at 10:50:51PM +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>
>> ---
>>  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
>
> Adding errata properties doesn't work because then you have to update 
> your dtb to fix the issue where as if you use the compatible property 
> (specific to the SoC) you can fix the issue with just a (stable) kernel 
> update.
>
> Yes, I see we already have some, but doesn't mean we need more of them.

I agree. Unfortunately all users of the driver also use the same
compatible string, so there was no way I could think of that would not
involve rebuilding DTBs anyway. Given that situation, I chose to just
extend what was already there.

[PATCH v2 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.

Fixes: d55ad2967d89 ("powerpc/mpc85xx: Create dts components for the FSL QorIQ DPAA FMan")
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

[PATCH v2 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 v2 net 0/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 18 Jan 2022 22:50:49 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v2,net,1/4] net/fsl: xgmac_mdio: Add workaround for erratum A-009885
    https://git.kernel.org/netdev/net/c/6198c7220197
  - [v2,net,2/4] dt-bindings: net: Document fsl,erratum-a009885
    https://git.kernel.org/netdev/net/c/ea11fc509ff2
  - [v2,net,3/4] powerpc/fsl/dts: Enable WA for erratum A-009885 on fman3l MDIO buses
    https://git.kernel.org/netdev/net/c/0d375d610fa9
  - [v2,net,4/4] net/fsl: xgmac_mdio: Fix incorrect iounmap when removing module
    https://git.kernel.org/netdev/net/c/3f7c239c7844

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html