[PATCH net-next v2 0/3] Cadence MACB/GEM support for ZynqMP SGMII

[PATCH net-next v2 0/3] Cadence MACB/GEM support for ZynqMP SGMII

[Robert Hancock]

Changes to allow SGMII mode to work properly in the GEM driver on the
Xilinx ZynqMP platform.

Changes since v1:
-changed order of controller reset and PHY init as per suggestion
-switched device reset to be optional
-updated bindings doc patch for switch to YAML

Robert Hancock (3):
  dt-bindings: net: cdns,macb: added generic PHY and reset mappings for
    ZynqMP
  net: macb: Added ZynqMP-specific initialization
  arm64: dts: zynqmp: Added GEM reset definitions

 .../devicetree/bindings/net/cdns,macb.yaml    | 46 ++++++++++++++++++
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        |  8 ++++
 drivers/net/ethernet/cadence/macb_main.c      | 48 ++++++++++++++++++-
 3 files changed, 101 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH net-next v2 1/3] dt-bindings: net: cdns,macb: added generic PHY and reset mappings for ZynqMP

[Robert Hancock]

Updated macb DT binding documentation to reflect the phy-names, phys,
resets, reset-names properties which are now used with ZynqMP GEM
devices, and added a ZynqMP-specific DT example.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 .../devicetree/bindings/net/cdns,macb.yaml    | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index 8dd06db34169..efc759e052c4 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -81,6 +81,25 @@ properties:
 
   phy-handle: true
 
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: sgmii-phy
+    description:
+      Required with ZynqMP SoC when in SGMII mode.
+      Should reference PS-GTR generic PHY device for this controller
+      instance. See ZynqMP example.
+
+  resets:
+    maxItems: 1
+    description:
+      Recommended with ZynqMP, specify reset control for this
+      controller instance with zynqmp-reset driver.
+
+  reset-names:
+    maxItems: 1
+
   fixed-link: true
 
   iommus:
@@ -157,3 +176,30 @@ examples:
                     reset-gpios = <&pioE 6 1>;
             };
     };
+
+    gem1: ethernet@ff0c0000 {
+            compatible = "cdns,zynqmp-gem", "cdns,gem";
+            interrupt-parent = <&gic>;
+            interrupts = <0 59 4>, <0 59 4>;
+            reg = <0x0 0xff0c0000 0x0 0x1000>;
+            clocks = <&zynqmp_clk LPD_LSBUS>, <&zynqmp_clk GEM1_REF>,
+                     <&zynqmp_clk GEM1_TX>, <&zynqmp_clk GEM1_RX>,
+                     <&zynqmp_clk GEM_TSU>;
+            clock-names = "pclk", "hclk", "tx_clk", "rx_clk", "tsu_clk";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            #stream-id-cells = <1>;
+            iommus = <&smmu 0x875>;
+            power-domains = <&zynqmp_firmware PD_ETH_1>;
+            resets = <&zynqmp_reset ZYNQMP_RESET_GEM1>;
+            reset-names = "gem1_rst";
+            status = "okay";
+            phy-mode = "sgmii";
+            phy-names = "sgmii-phy";
+            phys = <&psgtr 1 PHY_TYPE_SGMII 1 1>;
+            fixed-link {
+                    speed = <1000>;
+                    full-duplex;
+                    pause;
+            };
+    };
-- 
2.31.1

[PATCH net-next v2 2/3] net: macb: Added ZynqMP-specific initialization

[Robert Hancock]

The GEM controllers on ZynqMP were missing some initialization steps which
are required in some cases when using SGMII mode, which uses the PS-GTR
transceivers managed by the phy-zynqmp driver.

The GEM core appears to need a hardware-level reset in order to work
properly in SGMII mode in cases where the GT reference clock was not
present at initial power-on. This can be done using a reset mapped to
the zynqmp-reset driver in the device tree.

Also, when in SGMII mode, the GEM driver needs to ensure the PHY is
initialized and powered on when it is initializing.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 48 +++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a363da928e8b..80882908a68f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -34,7 +34,9 @@
 #include <linux/udp.h>
 #include <linux/tcp.h>
 #include <linux/iopoll.h>
+#include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */
@@ -4455,6 +4457,50 @@ static int fu540_c000_init(struct platform_device *pdev)
 	return macb_init(pdev);
 }
 
+static int zynqmp_init(struct platform_device *pdev)
+{
+	struct net_device *dev = platform_get_drvdata(pdev);
+	struct macb *bp = netdev_priv(dev);
+	int ret;
+
+	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+		/* Ensure PS-GTR PHY device used in SGMII mode is ready */
+		struct phy *sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy");
+
+		if (IS_ERR(sgmii_phy)) {
+			ret = PTR_ERR(sgmii_phy);
+			dev_err_probe(&pdev->dev, ret,
+				      "failed to get PS-GTR PHY\n");
+			return ret;
+		}
+
+		ret = phy_init(sgmii_phy);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = phy_power_on(sgmii_phy);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to power on PS-GTR PHY: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	/* Fully reset GEM controller at hardware level using zynqmp-reset driver,
+	 * if mapped in device tree.
+	 */
+	ret = device_reset_optional(&pdev->dev);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "failed to reset controller");
+		return ret;
+	}
+
+	return macb_init(pdev);
+}
+
 static const struct macb_usrio_config sama7g5_usrio = {
 	.mii = 0,
 	.rmii = 1,
@@ -4550,7 +4596,7 @@ static const struct macb_config zynqmp_config = {
 			MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
-	.init = macb_init,
+	.init = zynqmp_init,
 	.jumbo_max_len = 10240,
 	.usrio = &macb_default_usrio,
 };
-- 
2.31.1

[PATCH net-next v2 3/3] arm64: dts: zynqmp: Added GEM reset definitions

[Robert Hancock]

The Cadence GEM/MACB driver now utilizes the platform-level reset on the
ZynqMP platform. Add reset definitions to the ZynqMP platform device
tree to allow this to be used.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 74e66443e4ce..9bec3ba20c69 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -512,6 +512,8 @@ gem0: ethernet@ff0b0000 {
 			#stream-id-cells = <1>;
 			iommus = <&smmu 0x874>;
 			power-domains = <&zynqmp_firmware PD_ETH_0>;
+			resets = <&zynqmp_reset ZYNQMP_RESET_GEM0>;
+			reset-names = "gem0_rst";
 		};
 
 		gem1: ethernet@ff0c0000 {
@@ -526,6 +528,8 @@ gem1: ethernet@ff0c0000 {
 			#stream-id-cells = <1>;
 			iommus = <&smmu 0x875>;
 			power-domains = <&zynqmp_firmware PD_ETH_1>;
+			resets = <&zynqmp_reset ZYNQMP_RESET_GEM1>;
+			reset-names = "gem1_rst";
 		};
 
 		gem2: ethernet@ff0d0000 {
@@ -540,6 +544,8 @@ gem2: ethernet@ff0d0000 {
 			#stream-id-cells = <1>;
 			iommus = <&smmu 0x876>;
 			power-domains = <&zynqmp_firmware PD_ETH_2>;
+			resets = <&zynqmp_reset ZYNQMP_RESET_GEM2>;
+			reset-names = "gem2_rst";
 		};
 
 		gem3: ethernet@ff0e0000 {
@@ -554,6 +560,8 @@ gem3: ethernet@ff0e0000 {
 			#stream-id-cells = <1>;
 			iommus = <&smmu 0x877>;
 			power-domains = <&zynqmp_firmware PD_ETH_3>;
+			resets = <&zynqmp_reset ZYNQMP_RESET_GEM3>;
+			reset-names = "gem3_rst";
 		};
 
 		gpio: gpio@ff0a0000 {
-- 
2.31.1

Re: [PATCH net-next v2 1/3] dt-bindings: net: cdns,macb: added generic PHY and reset mappings for ZynqMP

[Rob Herring]

On Tue, 25 Jan 2022 11:05:31 -0600, Robert Hancock wrote:
> Updated macb DT binding documentation to reflect the phy-names, phys,
> resets, reset-names properties which are now used with ZynqMP GEM
> devices, and added a ZynqMP-specific DT example.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  .../devicetree/bindings/net/cdns,macb.yaml    | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/net/cdns,macb.example.dts:41.39-40 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:378: Documentation/devicetree/bindings/net/cdns,macb.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1398: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1584186

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH net-next v2 2/3] net: macb: Added ZynqMP-specific initialization

[Michal Simek]

On 1/25/22 18:05, Robert Hancock wrote:
> The GEM controllers on ZynqMP were missing some initialization steps which
> are required in some cases when using SGMII mode, which uses the PS-GTR
> transceivers managed by the phy-zynqmp driver.
> 
> The GEM core appears to need a hardware-level reset in order to work
> properly in SGMII mode in cases where the GT reference clock was not
> present at initial power-on. This can be done using a reset mapped to
> the zynqmp-reset driver in the device tree.
> 
> Also, when in SGMII mode, the GEM driver needs to ensure the PHY is
> initialized and powered on when it is initializing.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>   drivers/net/ethernet/cadence/macb_main.c | 48 +++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a363da928e8b..80882908a68f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -34,7 +34,9 @@
>   #include <linux/udp.h>
>   #include <linux/tcp.h>
>   #include <linux/iopoll.h>
> +#include <linux/phy/phy.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>   #include "macb.h"
>   
>   /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -4455,6 +4457,50 @@ static int fu540_c000_init(struct platform_device *pdev)
>   	return macb_init(pdev);
>   }
>   
> +static int zynqmp_init(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct macb *bp = netdev_priv(dev);
> +	int ret;
> +
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +		/* Ensure PS-GTR PHY device used in SGMII mode is ready */
> +		struct phy *sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy");
> +
> +		if (IS_ERR(sgmii_phy)) {
> +			ret = PTR_ERR(sgmii_phy);
> +			dev_err_probe(&pdev->dev, ret,
> +				      "failed to get PS-GTR PHY\n");
> +			return ret;
> +		}
> +
> +		ret = phy_init(sgmii_phy);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n",
> +				ret);
> +			return ret;
> +		}

I think reset below should be here to follow correct startup sequence.

Thanks,
Michal


> +
> +		ret = phy_power_on(sgmii_phy);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to power on PS-GTR PHY: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* Fully reset GEM controller at hardware level using zynqmp-reset driver,
> +	 * if mapped in device tree.
> +	 */
> +	ret = device_reset_optional(&pdev->dev);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "failed to reset controller");
> +		return ret;
> +	}
> +
> +	return macb_init(pdev);
> +}
> +
>   static const struct macb_usrio_config sama7g5_usrio = {
>   	.mii = 0,
>   	.rmii = 1,
> @@ -4550,7 +4596,7 @@ static const struct macb_config zynqmp_config = {
>   			MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
>   	.dma_burst_length = 16,
>   	.clk_init = macb_clk_init,
> -	.init = macb_init,
> +	.init = zynqmp_init,
>   	.jumbo_max_len = 10240,
>   	.usrio = &macb_default_usrio,
>   };

Re: [PATCH net-next v2 2/3] net: macb: Added ZynqMP-specific initialization

[Claudiu.Beznea]

On 26.01.2022 09:30, Michal Simek wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 1/25/22 18:05, Robert Hancock wrote:
>> The GEM controllers on ZynqMP were missing some initialization steps which
>> are required in some cases when using SGMII mode, which uses the PS-GTR
>> transceivers managed by the phy-zynqmp driver.
>>
>> The GEM core appears to need a hardware-level reset in order to work
>> properly in SGMII mode in cases where the GT reference clock was not
>> present at initial power-on. This can be done using a reset mapped to
>> the zynqmp-reset driver in the device tree.
>>
>> Also, when in SGMII mode, the GEM driver needs to ensure the PHY is
>> initialized and powered on when it is initializing.
>>
>> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
>> ---
>>   drivers/net/ethernet/cadence/macb_main.c | 48 +++++++++++++++++++++++-
>>   1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index a363da928e8b..80882908a68f 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -34,7 +34,9 @@
>>   #include <linux/udp.h>
>>   #include <linux/tcp.h>
>>   #include <linux/iopoll.h>
>> +#include <linux/phy/phy.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>>   #include "macb.h"
>>
>>   /* This structure is only used for MACB on SiFive FU540 devices */
>> @@ -4455,6 +4457,50 @@ static int fu540_c000_init(struct platform_device
>> *pdev)
>>       return macb_init(pdev);
>>   }
>>
>> +static int zynqmp_init(struct platform_device *pdev)
>> +{
>> +     struct net_device *dev = platform_get_drvdata(pdev);
>> +     struct macb *bp = netdev_priv(dev);
>> +     int ret;
>> +
>> +     if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>> +             /* Ensure PS-GTR PHY device used in SGMII mode is ready */
>> +             struct phy *sgmii_phy = devm_phy_get(&pdev->dev, "sgmii-phy");
>> +
>> +             if (IS_ERR(sgmii_phy)) {
>> +                     ret = PTR_ERR(sgmii_phy);
>> +                     dev_err_probe(&pdev->dev, ret,
>> +                                   "failed to get PS-GTR PHY\n");
>> +                     return ret;
>> +             }
>> +
>> +             ret = phy_init(sgmii_phy);
>> +             if (ret) {
>> +                     dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n",
>> +                             ret);
>> +                     return ret;
>> +             }
> 
> I think reset below should be here to follow correct startup sequence.

If that's the case is the functionality still kept if moving phy_power_on()
in macb_open() and the correspondent phy_power_off() in macb_close() ?

Also, Robert, please handle the error path in this function (with calls to
phy_exit(), phy_power_off()) and PHY handling in macb_remove().

Thank you,
Claudiu Beznea

> 
> Thanks,
> Michal
> 
> 
>> +
>> +             ret = phy_power_on(sgmii_phy);
>> +             if (ret) {
>> +                     dev_err(&pdev->dev, "failed to power on PS-GTR PHY:
>> %d\n",
>> +                             ret);
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     /* Fully reset GEM controller at hardware level using zynqmp-reset
>> driver,
>> +      * if mapped in device tree.
>> +      */
>> +     ret = device_reset_optional(&pdev->dev);
>> +     if (ret) {
>> +             dev_err_probe(&pdev->dev, ret, "failed to reset controller");
>> +             return ret;
>> +     }
>> +
>> +     return macb_init(pdev);
>> +}
>> +
>>   static const struct macb_usrio_config sama7g5_usrio = {
>>       .mii = 0,
>>       .rmii = 1,
>> @@ -4550,7 +4596,7 @@ static const struct macb_config zynqmp_config = {
>>                       MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
>>       .dma_burst_length = 16,
>>       .clk_init = macb_clk_init,
>> -     .init = macb_init,
>> +     .init = zynqmp_init,
>>       .jumbo_max_len = 10240,
>>       .usrio = &macb_default_usrio,
>>   };