netsec: ignore 'phy-mode' device property on ACPI systems

PATCH Applied Ard Biesheuvel 4 participants 9 messages Oct 18, 2020 – Oct 20, 2020

Subsystems (4)

Networking Network drivers Device tree bindings Socionext (SNI) netsec network driver

lore

Share & tools

lore

Ard Biesheuvel

author

2020-10-18 16:39 UTC #0 lore raw

PATCH 1/1 netsec: ignore 'phy-mode' device property on ACPI systems

Since commit bbc4d71d63549bc ("net: phy: realtek: fix rtl8211e rx/tx delay config"), the Realtek PHY driver will override any TX/RX delay set by hardware straps if the phy-mode device property does not match.

This is causing problems on SynQuacer based platforms (the only SoC that incorporates the netsec hardware), since many were built with this Realtek PHY, and shipped with firmware that defines the phy-mode as 'rgmii', even though the PHY is configured for TX and RX delay using pull-ups.

From the driver's perspective, we should not make any assumptions in the general case that the PHY hardware does not require any initial configuration. However, the situation is slightly different for ACPI boot, since it implies rich firmware with AML abstractions to handle hardware details that are not exposed to the OS. So in the ACPI case, it is reasonable to assume that the PHY comes up in the right mode, regardless of whether the mode is set by straps, by boot time firmware or by AML executed by the ACPI interpreter.

So let's ignore the 'phy-mode' device property when probing the netsec driver in ACPI mode, and hardcode the mode to PHY_INTERFACE_MODE_NA, which should work with any PHY provided that it is configured by the time the driver attaches to it. While at it, document that omitting the mode is permitted for DT probing as well, by setting the phy-mode DT property to the empty string.

Cc Jassi Brar <[email protected]>

Cc Ilias Apalodimas <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Andrew Lunn <[email protected]>

Cc Willy Liu <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Masahisa Kojima <[email protected]>

Cc Serge Semin <[email protected]>

Fixes 533dd11a12f6 ("net: socionext: Add Synquacer NetSec driver")

Signed-off-by Ard Biesheuvel <[email protected]>

 Documentation/devicetree/bindings/net/socionext-netsec.txt |  4 +++-
 drivers/net/ethernet/socionext/netsec.c                    | 24 ++++++++++++++------
 2 files changed, 20 insertions(+), 8 deletions(-)

Documentation/devicetree/bindings/net/socionext-netsec.txt +3 -1

index 9d6c9feb12ff..a3c1dffaa4bb 100644
@@ -30,7 +30,9 @@ Optional properties: (See ethernet.txt file in the same directory)
 - max-frame-size: See ethernet.txt in the same directory.
 
 The MAC address will be determined using the optional properties
-defined in ethernet.txt.
+defined in ethernet.txt. The 'phy-mode' property is required, but may
+be set to the empty string if the PHY configuration is programmed by
+the firmware or set by hardware straps, and needs to be preserved.
 
 Example:
 	eth0: ethernet@522d0000 {

drivers/net/ethernet/socionext/netsec.c +17 -7

index 806eb651cea3..1503cc9ec6e2 100644
@@ -6,6 +6,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/acpi.h>
 #include <linux/of_mdio.h>
+#include <linux/of_net.h>
 #include <linux/etherdevice.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>

@@ -1833,6 +1834,14 @@ static const struct net_device_ops netsec_netdev_ops = {
 static int netsec_of_probe(struct platform_device *pdev,
 			   struct netsec_priv *priv, u32 *phy_addr)
 {
+	int err;
+
+	err = of_get_phy_mode(pdev->dev.of_node, &priv->phy_interface);
+	if (err) {
+		dev_err(&pdev->dev, "missing required property 'phy-mode'\n");
+		return err;
+	}
+
 	priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
 	if (!priv->phy_np) {
 		dev_err(&pdev->dev, "missing required property 'phy-handle'\n");

@@ -1859,6 +1868,14 @@ static int netsec_acpi_probe(struct platform_device *pdev,
 	if (!IS_ENABLED(CONFIG_ACPI))
 		return -ENODEV;
 
+	/* ACPI systems are assumed to configure the PHY in firmware, so
+	 * there is really no need to discover the PHY mode from the DSDT.
+	 * Since firmware is known to exist in the field that configures the
+	 * PHY correctly but passes the wrong mode string in the phy-mode
+	 * device property, we have no choice but to ignore it.
+	 */
+	priv->phy_interface = PHY_INTERFACE_MODE_NA;
+
 	ret = device_property_read_u32(&pdev->dev, "phy-channel", phy_addr);
 	if (ret) {
 		dev_err(&pdev->dev,

@@ -1995,13 +2012,6 @@ static int netsec_probe(struct platform_device *pdev)
 	priv->msg_enable = NETIF_MSG_TX_ERR | NETIF_MSG_HW | NETIF_MSG_DRV |
 			   NETIF_MSG_LINK | NETIF_MSG_PROBE;
 
-	priv->phy_interface = device_get_phy_mode(&pdev->dev);
-	if ((int)priv->phy_interface < 0) {
-		dev_err(&pdev->dev, "missing required property 'phy-mode'\n");
-		ret = -ENODEV;
-		goto free_ndev;
-	}
-
 	priv->ioaddr = devm_ioremap(&pdev->dev, mmio_res->start,
 				    resource_size(mmio_res));
 	if (!priv->ioaddr) {

Signature · 1 line

2.17.1

Andrew Lunn

2020-10-18 17:52 UTC #1 lore raw

Documentation/devicetree/bindings/net/socionext-netsec.txt Optional properties: (See ethernet.txt file in the same directory) · line 35

Quoted 10 lines

Documentation/devicetree/bindings/net/socionext-netsec.txt +3 -1

@@ -30,7 +30,9 @@ Optional properties: (See ethernet.txt file in the same directory)
 - max-frame-size: See ethernet.txt in the same directory.
 
 The MAC address will be determined using the optional properties
-defined in ethernet.txt.
+defined in ethernet.txt. The 'phy-mode' property is required, but may
+be set to the empty string if the PHY configuration is programmed by
+the firmware or set by hardware straps, and needs to be preserved.

In general, phy-mode is not mandatory. of_get_phy_mode() does the right thing if it is not found, it sets &priv->phy_interface to PHY_INTERFACE_MODE_NA, but returns -ENODEV. Also, it does not break backwards compatibility to convert a mandatory property to optional. So you could just do

	of_get_phy_mode(pdev->dev.of_node, &priv->phy_interface);

skip all the error checking, and document it as optional.

Andrew

Ilias Apalodimas

maintainer

2020-10-18 20:32 UTC #2 lore raw

Quoted 17 lines · Andrew Lunn

Documentation/devicetree/bindings/net/socionext-netsec.txt Optional properties: (See ethernet.txt file in the same directory) · line 35
Quoted 10 lines
Documentation/devicetree/bindings/net/socionext-netsec.txt +3 -1
@@ -30,7 +30,9 @@ Optional properties: (See ethernet.txt file in the same directory)
 - max-frame-size: See ethernet.txt in the same directory.
 
 The MAC address will be determined using the optional properties
-defined in ethernet.txt.
+defined in ethernet.txt. The 'phy-mode' property is required, but may
+be set to the empty string if the PHY configuration is programmed by
+the firmware or set by hardware straps, and needs to be preserved.
In general, phy-mode is not mandatory. of_get_phy_mode() does the right thing if it is not found, it sets &priv->phy_interface to PHY_INTERFACE_MODE_NA, but returns -ENODEV. Also, it does not break backwards compatibility to convert a mandatory property to optional. So you could just do

Quoted 3 lines

	of_get_phy_mode(pdev->dev.of_node, &priv->phy_interface);
skip all the error checking, and document it as optional.

Why ?
The patch as is will not affect systems built on any firmware implementations that use ACPI and somehow configure the hardware.
Although the only firmware implementations I am aware of on upsteream are based on EDK2, I prefer the explicit error as is now, in case a firmware does on initialize the PHY properly (and is using a DT).

Cheers
/Ilias

Quoted 2 lines

Andrew

Ard Biesheuvel

author

2020-10-19 06:30 UTC #3 lore raw

Quoted 21 lines · Ilias Apalodimas

Quoted 19 lines · Andrew Lunn
Documentation/devicetree/bindings/net/socionext-netsec.txt Optional properties: (See ethernet.txt file in the same directory) · line 35
Quoted 10 lines
Documentation/devicetree/bindings/net/socionext-netsec.txt +3 -1
@@ -30,7 +30,9 @@ Optional properties: (See ethernet.txt file in the same directory)
 - max-frame-size: See ethernet.txt in the same directory.
 
 The MAC address will be determined using the optional properties
-defined in ethernet.txt.
+defined in ethernet.txt. The 'phy-mode' property is required, but may
+be set to the empty string if the PHY configuration is programmed by
+the firmware or set by hardware straps, and needs to be preserved.
In general, phy-mode is not mandatory. of_get_phy_mode() does the right thing if it is not found, it sets &priv->phy_interface to PHY_INTERFACE_MODE_NA, but returns -ENODEV. Also, it does not break backwards compatibility to convert a mandatory property to optional. So you could just do
      of_get_phy_mode(pdev->dev.of_node, &priv->phy_interface);

Quoted 9 lines

Quoted 1 line
skip all the error checking, and document it as optional.
Why ?
The patch as is will not affect systems built on any firmware implementations that use ACPI and somehow configure the hardware.
Although the only firmware implementations I am aware of on upsteream are based on EDK2, I prefer the explicit error as is now, in case a firmware does on initialize the PHY properly (and is using a DT).

We will also lose the ability to report bogus values for phy-mode this way, so I think we should stick with the check.

Ilias Apalodimas

maintainer Reviewed-by

2020-10-20 08:48 UTC #4 lore raw

Hi Ard,

Quoted 33 lines · Ard Biesheuvel

Quoted 21 lines · Ilias Apalodimas
Quoted 19 lines · Andrew Lunn
Documentation/devicetree/bindings/net/socionext-netsec.txt Optional properties: (See ethernet.txt file in the same directory) · line 35
Quoted 10 lines
Documentation/devicetree/bindings/net/socionext-netsec.txt +3 -1
@@ -30,7 +30,9 @@ Optional properties: (See ethernet.txt file in the same directory)
 - max-frame-size: See ethernet.txt in the same directory.
 
 The MAC address will be determined using the optional properties
-defined in ethernet.txt.
+defined in ethernet.txt. The 'phy-mode' property is required, but may
+be set to the empty string if the PHY configuration is programmed by
+the firmware or set by hardware straps, and needs to be preserved.
In general, phy-mode is not mandatory. of_get_phy_mode() does the right thing if it is not found, it sets &priv->phy_interface to PHY_INTERFACE_MODE_NA, but returns -ENODEV. Also, it does not break backwards compatibility to convert a mandatory property to optional. So you could just do
      of_get_phy_mode(pdev->dev.of_node, &priv->phy_interface);
Quoted 9 lines
Quoted 1 line
skip all the error checking, and document it as optional.
Why ?
The patch as is will not affect systems built on any firmware implementations that use ACPI and somehow configure the hardware.
Although the only firmware implementations I am aware of on upsteream are based on EDK2, I prefer the explicit error as is now, in case a firmware does on initialize the PHY properly (and is using a DT).

Quoted 2 lines

We will also lose the ability to report bogus values for phy-mode this way, so I think we should stick with the check.

I hope Andrew is fine with the current changes

Reviewed-by Ilias Apalodimas <[email protected]>

Andrew Lunn

2020-10-20 12:49 UTC #5 lore raw

Quoted 1 line

I hope Andrew is fine with the current changes

Yes, i'm O.K. with it. Making phy-mode optional would just make the driver more uniform with others.

Andrew

Ard Biesheuvel

author

2020-10-20 13:23 UTC #6 lore raw

Quoted 4 lines · Andrew Lunn

Quoted 2 lines
I hope Andrew is fine with the current changes
Yes, i'm O.K. with it.

Thanks

Quoted 3 lines

Making phy-mode optional would just make the
driver more uniform with others.

Making phy-mode optional is fine with me, but I think it would belong in a separate patch in any case. But I'd still prefer having the possibility to spot bogus phy-mode values rather than ignoring them.

Andrew Lunn

Reviewed-by on v1 1/1

2020-10-20 12:50 UTC #7 lore raw

In reply to Ard Biesheuvel

Quoted 26 lines · Ard Biesheuvel

Since commit bbc4d71d63549bc ("net: phy: realtek: fix rtl8211e rx/tx delay config"), the Realtek PHY driver will override any TX/RX delay set by hardware straps if the phy-mode device property does not match.
This is causing problems on SynQuacer based platforms (the only SoC that incorporates the netsec hardware), since many were built with this Realtek PHY, and shipped with firmware that defines the phy-mode as 'rgmii', even though the PHY is configured for TX and RX delay using pull-ups.
Quoted 1 line
From the driver's perspective, we should not make any assumptions in
the general case that the PHY hardware does not require any initial configuration. However, the situation is slightly different for ACPI boot, since it implies rich firmware with AML abstractions to handle hardware details that are not exposed to the OS. So in the ACPI case, it is reasonable to assume that the PHY comes up in the right mode, regardless of whether the mode is set by straps, by boot time firmware or by AML executed by the ACPI interpreter.
So let's ignore the 'phy-mode' device property when probing the netsec driver in ACPI mode, and hardcode the mode to PHY_INTERFACE_MODE_NA, which should work with any PHY provided that it is configured by the time the driver attaches to it. While at it, document that omitting the mode is permitted for DT probing as well, by setting the phy-mode DT property to the empty string.

Quoted 11 lines

Cc Jassi Brar <[email protected]>
Cc Ilias Apalodimas <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Andrew Lunn <[email protected]>
Cc Willy Liu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Masahisa Kojima <[email protected]>
Cc Serge Semin <[email protected]>
Fixes 533dd11a12f6 ("net: socionext: Add Synquacer NetSec driver")
Signed-off-by Ard Biesheuvel <[email protected]>

Reviewed-by Andrew Lunn <[email protected]>

Andrew

Jakub Kicinski

maintainer Applied

2020-10-20 23:12 UTC #8 lore raw

Quoted 27 lines · On Tue, 20 Oct 2020 14:50:01 +0200 Andrew Lunn wrote:

Quoted 26 lines · Ard Biesheuvel
Since commit bbc4d71d63549bc ("net: phy: realtek: fix rtl8211e rx/tx delay config"), the Realtek PHY driver will override any TX/RX delay set by hardware straps if the phy-mode device property does not match.
This is causing problems on SynQuacer based platforms (the only SoC that incorporates the netsec hardware), since many were built with this Realtek PHY, and shipped with firmware that defines the phy-mode as 'rgmii', even though the PHY is configured for TX and RX delay using pull-ups.
Quoted 1 line
From the driver's perspective, we should not make any assumptions in
the general case that the PHY hardware does not require any initial configuration. However, the situation is slightly different for ACPI boot, since it implies rich firmware with AML abstractions to handle hardware details that are not exposed to the OS. So in the ACPI case, it is reasonable to assume that the PHY comes up in the right mode, regardless of whether the mode is set by straps, by boot time firmware or by AML executed by the ACPI interpreter.
So let's ignore the 'phy-mode' device property when probing the netsec driver in ACPI mode, and hardcode the mode to PHY_INTERFACE_MODE_NA, which should work with any PHY provided that it is configured by the time the driver attaches to it. While at it, document that omitting the mode is permitted for DT probing as well, by setting the phy-mode DT property to the empty string.

Quoted 4 lines

Quoted 2 lines
Fixes 533dd11a12f6 ("net: socionext: Add Synquacer NetSec driver")
Signed-off-by Ard Biesheuvel <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>

Thanks, applied.

Just to be on the safe side please make sure to CC Rob & DT list if your patch touches anything device tree.

Quoted 13 lines

Documentation/devicetree/bindings/net/socionext-netsec.txt +3 -1

@@ -30,7 +30,9 @@ Optional properties: (See ethernet.txt file in the same directory)
 - max-frame-size: See ethernet.txt in the same directory.
 
 The MAC address will be determined using the optional properties
-defined in ethernet.txt.
+defined in ethernet.txt. The 'phy-mode' property is required, but may
+be set to the empty string if the PHY configuration is programmed by
+the firmware or set by hardware straps, and needs to be preserved.
 
 Example:
 	eth0: ethernet@522d0000 {