-----Original Message-----
From: Claudiu.Beznea@microchip.com <redacted>
Sent: Monday, August 1, 2022 5:06 PM
To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>;
michal.simek@xilinx.com; Nicolas.Ferre@microchip.com;
davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
pabeni@redhat.com; gregkh@linuxfoundation.org
Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org; git (AMD-Xilinx) [off-list ref]; git@xilinx.com
Subject: Re: [PATCH v2 net-next 2/2] net: macb: Add zynqmp SGMII dynamic
configuration support

On 29.07.2022 22:35, Radhey Shyam Pandey wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you know
the content is safe

Add support for the dynamic configuration which takes care of
configuring the GEM secure space configuration registers using EEMI
APIs. High level sequence is to:
- Check for the PM dynamic configuration support, if no error proceed with
  GEM dynamic configurations(next steps) otherwise skip the dynamic
  configuration.
- Configure GEM Fixed configurations.
- Configure GEM_CLK_CTRL (gemX_sgmii_mode).
- Trigger GEM reset.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Conor Dooley <conor.dooley@microchip.com> (for MPFS)
---
Changes for v2:
- Add phy_exit() in error return paths.
---
 drivers/net/ethernet/cadence/macb_main.c | 25
+++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c

b/drivers/net/ethernet/cadence/macb_main.c
index 4cd4f57ca2aa..517b40ff098b 100644

--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -38,6 +38,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/ptp_classify.h>
 #include <linux/reset.h>
+#include <linux/firmware/xlnx-zynqmp.h>
 #include "macb.h"

 /* This structure is only used for MACB on SiFive FU540 devices */ @@
-4621,6 +4622,30 @@ static int init_reset_optional(struct platform_device

*pdev)

quoted

                                             "failed to init SGMII PHY\n");
        }

+       ret = zynqmp_pm_is_function_supported(PM_IOCTL,

IOCTL_SET_GEM_CONFIG);

quoted

+       if (!ret) {
+               u32 pm_info[2];
+
+               ret = of_property_read_u32_array(pdev->dev.of_node, "power-

domains",

quoted

+                                                pm_info, ARRAY_SIZE(pm_info));
+               if (ret < 0) {
+                       phy_exit(bp->sgmii_phy);

Could you move this to a single exit point and jump in there with goto?
Same for the rest of occurencies.

Ok, will make to use of common exit path and send out a new version.

Also, I notice just now that phy_init() is done only if bp->phy_interface ==
PHY_INTERFACE_MODE_SGMII, phy_exit() should be handled only if this is
true, too.

If phy interface is not SGMII bp->sgmii_phy would be NULL and calling 
phy_exit should still be fine as these phy APIs has already a NULL check.

quoted

+                       dev_err(&pdev->dev, "Failed to read power management

information\n");

quoted

+                       return ret;
+               }
+               ret = zynqmp_pm_set_gem_config(pm_info[1],

GEM_CONFIG_FIXED, 0);

quoted

+               if (ret < 0) {
+                       phy_exit(bp->sgmii_phy);
+                       return ret;
+               }
+
+               ret = zynqmp_pm_set_gem_config(pm_info[1],

GEM_CONFIG_SGMII_MODE, 1);

quoted

+               if (ret < 0) {
+                       phy_exit(bp->sgmii_phy);
+                       return ret;
+               }
+       }
+
        /* Fully reset controller at hardware level if mapped in device tree */
        ret = device_reset_optional(&pdev->dev);
        if (ret) {
--
2.1.1