[PATCH net-next 0/2] macb: add zynqmp SGMII dynamic configuration support

[PATCH net-next 0/2] macb: add zynqmp SGMII dynamic configuration support

[Radhey Shyam Pandey]

This patchset add firmware and driver support to do SD/GEM dynamic
configuration. In traditional flow GEM secure space configuration 
is done by FSBL. However in specific usescases like dynamic designs
where GEM is not enabled in base vivado design, FSBL skips GEM 
initialization and we need a mechanism to configure GEM secure space
in linux space at runtime.

mmc: sdhci-of-arasan: Add support for dynamic configuration will 
be send post this series.

Radhey Shyam Pandey (1):
  net: macb: Add zynqmp SGMII dynamic configuration support

Ronak Jain (1):
  firmware: xilinx: add support for sd/gem config

 drivers/firmware/xilinx/zynqmp.c         | 31 ++++++++++++++++++++++
 drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h     | 33 ++++++++++++++++++++++++
 3 files changed, 84 insertions(+)

-- 
2.25.1

[PATCH net-next 1/2] firmware: xilinx: add support for sd/gem config

[Radhey Shyam Pandey]

From: Ronak Jain <ronak.jain@xilinx.com>

Add new APIs in firmware to configure SD/GEM registers. Internally
it calls PM IOCTL for below SD/GEM register configuration:
- SD/EMMC select
- SD slot type
- SD base clock
- SD 8 bit support
- SD fixed config
- GEM SGMII Mode
- GEM fixed config

Signed-off-by: Ronak Jain <ronak.jain@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 31 ++++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h | 33 ++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 7977a494a651..32a35bafb65e 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -1297,6 +1297,37 @@ int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
 				   id, 0, payload);
 }
 
+/**
+ * zynqmp_pm_set_sd_config - PM call to set value of SD config registers
+ * @node:	SD node ID
+ * @config:	The config type of SD registers
+ * @value:	Value to be set
+ *
+ * Return:      Returns 0 on success or error value on failure.
+ */
+int zynqmp_pm_set_sd_config(u32 node, enum pm_sd_config_type config, u32 value)
+{
+	return zynqmp_pm_invoke_fn(PM_IOCTL, node, IOCTL_SET_SD_CONFIG,
+				   config, value, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_config);
+
+/**
+ * zynqmp_pm_set_gem_config - PM call to set value of GEM config registers
+ * @node:	GEM node ID
+ * @config:	The config type of GEM registers
+ * @value:	Value to be set
+ *
+ * Return:      Returns 0 on success or error value on failure.
+ */
+int zynqmp_pm_set_gem_config(u32 node, enum pm_gem_config_type config,
+			     u32 value)
+{
+	return zynqmp_pm_invoke_fn(PM_IOCTL, node, IOCTL_SET_GEM_CONFIG,
+				   config, value, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_set_gem_config);
+
 /**
  * struct zynqmp_pm_shutdown_scope - Struct for shutdown scope
  * @subtype:	Shutdown subtype
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 1ec73d5352c3..063a93c133f1 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -152,6 +152,9 @@ enum pm_ioctl_id {
 	/* Runtime feature configuration */
 	IOCTL_SET_FEATURE_CONFIG = 26,
 	IOCTL_GET_FEATURE_CONFIG = 27,
+	/* Dynamic SD/GEM configuration */
+	IOCTL_SET_SD_CONFIG = 30,
+	IOCTL_SET_GEM_CONFIG = 31,
 };
 
 enum pm_query_id {
@@ -393,6 +396,18 @@ enum pm_feature_config_id {
 	PM_FEATURE_EXTWDT_VALUE = 4,
 };
 
+enum pm_sd_config_type {
+	SD_CONFIG_EMMC_SEL = 1, /* To set SD_EMMC_SEL in CTRL_REG_SD and SD_SLOTTYPE */
+	SD_CONFIG_BASECLK = 2, /* To set SD_BASECLK in SD_CONFIG_REG1 */
+	SD_CONFIG_8BIT = 3, /* To set SD_8BIT in SD_CONFIG_REG2 */
+	SD_CONFIG_FIXED = 4, /* To set fixed config registers */
+};
+
+enum pm_gem_config_type {
+	GEM_CONFIG_SGMII_MODE = 1, /* To set GEM_SGMII_MODE in GEM_CLK_CTRL register */
+	GEM_CONFIG_FIXED = 2, /* To set fixed config registers */
+};
+
 /**
  * struct zynqmp_pm_query_data - PM query data
  * @qid:	query ID
@@ -468,6 +483,9 @@ int zynqmp_pm_feature(const u32 api_id);
 int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id);
 int zynqmp_pm_set_feature_config(enum pm_feature_config_id id, u32 value);
 int zynqmp_pm_get_feature_config(enum pm_feature_config_id id, u32 *payload);
+int zynqmp_pm_set_sd_config(u32 node, enum pm_sd_config_type config, u32 value);
+int zynqmp_pm_set_gem_config(u32 node, enum pm_gem_config_type config,
+			     u32 value);
 #else
 static inline int zynqmp_pm_get_api_version(u32 *version)
 {
@@ -733,6 +751,21 @@ static inline int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
 {
 	return -ENODEV;
 }
+
+static inline int zynqmp_pm_set_sd_config(u32 node,
+					  enum pm_sd_config_type config,
+					  u32 value)
+{
+	return -ENODEV;
+}
+
+static inline int zynqmp_pm_set_gem_config(u32 node,
+					   enum pm_gem_config_type config,
+					   u32 value)
+{
+	return -ENODEV;
+}
+
 #endif
 
 #endif /* __FIRMWARE_ZYNQMP_H__ */
-- 
2.25.1

[PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Radhey Shyam Pandey]

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>
---
 drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 7eb7822cd184..97f77fa9e165 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,25 @@ static int init_reset_optional(struct platform_device *pdev)
 					     "failed to init SGMII PHY\n");
 	}
 
+	ret = zynqmp_pm_is_function_supported(PM_IOCTL, IOCTL_SET_GEM_CONFIG);
+	if (!ret) {
+		u32 pm_info[2];
+
+		ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains",
+						 pm_info, ARRAY_SIZE(pm_info));
+		if (ret < 0) {
+			dev_err(&pdev->dev, "Failed to read power management information\n");
+			return ret;
+		}
+		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0);
+		if (ret < 0)
+			return ret;
+
+		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Fully reset controller at hardware level if mapped in device tree */
 	ret = device_reset_optional(&pdev->dev);
 	if (ret) {
-- 
2.25.1

Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Claudiu.Beznea]

On 22.07.2022 11:12, Radhey Shyam Pandey wrote:
> 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>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 7eb7822cd184..97f77fa9e165 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,25 @@ static int init_reset_optional(struct platform_device *pdev)
>                                              "failed to init SGMII PHY\n");
>         }
> 
> +       ret = zynqmp_pm_is_function_supported(PM_IOCTL, IOCTL_SET_GEM_CONFIG);
> +       if (!ret) {
> +               u32 pm_info[2];
> +
> +               ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains",
> +                                                pm_info, ARRAY_SIZE(pm_info));
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "Failed to read power management information\n");

You have to undo phy_init() above (not listed in this diff).

> +                       return ret;
> +               }
> +               ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0);
> +               if (ret < 0)

Same here.

> +                       return ret;
> +
> +               ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
> +               if (ret < 0)

And here.

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

Re: [PATCH net-next 1/2] firmware: xilinx: add support for sd/gem config

[Claudiu.Beznea]

On 22.07.2022 11:11, Radhey Shyam Pandey wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Ronak Jain <ronak.jain@xilinx.com>
> 
> Add new APIs in firmware to configure SD/GEM registers. Internally
> it calls PM IOCTL for below SD/GEM register configuration:
> - SD/EMMC select
> - SD slot type
> - SD base clock
> - SD 8 bit support
> - SD fixed config
> - GEM SGMII Mode
> - GEM fixed config
> 
> Signed-off-by: Ronak Jain <ronak.jain@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
>  drivers/firmware/xilinx/zynqmp.c     | 31 ++++++++++++++++++++++++++
>  include/linux/firmware/xlnx-zynqmp.h | 33 ++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 7977a494a651..32a35bafb65e 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -1297,6 +1297,37 @@ int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
>                                    id, 0, payload);
>  }
> 
> +/**
> + * zynqmp_pm_set_sd_config - PM call to set value of SD config registers
> + * @node:      SD node ID
> + * @config:    The config type of SD registers
> + * @value:     Value to be set
> + *
> + * Return:      Returns 0 on success or error value on failure.

You have spaces after "Return:", doesn't match with tabs that you have
after variable documentation.

> + */
> +int zynqmp_pm_set_sd_config(u32 node, enum pm_sd_config_type config, u32 value)
> +{
> +       return zynqmp_pm_invoke_fn(PM_IOCTL, node, IOCTL_SET_SD_CONFIG,
> +                                  config, value, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_config);
> +
> +/**
> + * zynqmp_pm_set_gem_config - PM call to set value of GEM config registers
> + * @node:      GEM node ID
> + * @config:    The config type of GEM registers
> + * @value:     Value to be set
> + *
> + * Return:      Returns 0 on success or error value on failure.

Same here.

> + */
> +int zynqmp_pm_set_gem_config(u32 node, enum pm_gem_config_type config,
> +                            u32 value)
> +{
> +       return zynqmp_pm_invoke_fn(PM_IOCTL, node, IOCTL_SET_GEM_CONFIG,
> +                                  config, value, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_set_gem_config);
> +
>  /**
>   * struct zynqmp_pm_shutdown_scope - Struct for shutdown scope
>   * @subtype:   Shutdown subtype
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index 1ec73d5352c3..063a93c133f1 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -152,6 +152,9 @@ enum pm_ioctl_id {
>         /* Runtime feature configuration */
>         IOCTL_SET_FEATURE_CONFIG = 26,
>         IOCTL_GET_FEATURE_CONFIG = 27,
> +       /* Dynamic SD/GEM configuration */
> +       IOCTL_SET_SD_CONFIG = 30,
> +       IOCTL_SET_GEM_CONFIG = 31,
>  };
> 
>  enum pm_query_id {
> @@ -393,6 +396,18 @@ enum pm_feature_config_id {
>         PM_FEATURE_EXTWDT_VALUE = 4,
>  };
> 
> +enum pm_sd_config_type {
> +       SD_CONFIG_EMMC_SEL = 1, /* To set SD_EMMC_SEL in CTRL_REG_SD and SD_SLOTTYPE */
> +       SD_CONFIG_BASECLK = 2, /* To set SD_BASECLK in SD_CONFIG_REG1 */
> +       SD_CONFIG_8BIT = 3, /* To set SD_8BIT in SD_CONFIG_REG2 */
> +       SD_CONFIG_FIXED = 4, /* To set fixed config registers */
> +};
> +
> +enum pm_gem_config_type {
> +       GEM_CONFIG_SGMII_MODE = 1, /* To set GEM_SGMII_MODE in GEM_CLK_CTRL register */
> +       GEM_CONFIG_FIXED = 2, /* To set fixed config registers */
> +};
> +
>  /**
>   * struct zynqmp_pm_query_data - PM query data
>   * @qid:       query ID
> @@ -468,6 +483,9 @@ int zynqmp_pm_feature(const u32 api_id);
>  int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id);
>  int zynqmp_pm_set_feature_config(enum pm_feature_config_id id, u32 value);
>  int zynqmp_pm_get_feature_config(enum pm_feature_config_id id, u32 *payload);
> +int zynqmp_pm_set_sd_config(u32 node, enum pm_sd_config_type config, u32 value);
> +int zynqmp_pm_set_gem_config(u32 node, enum pm_gem_config_type config,
> +                            u32 value);
>  #else
>  static inline int zynqmp_pm_get_api_version(u32 *version)
>  {
> @@ -733,6 +751,21 @@ static inline int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
>  {
>         return -ENODEV;
>  }
> +
> +static inline int zynqmp_pm_set_sd_config(u32 node,
> +                                         enum pm_sd_config_type config,
> +                                         u32 value)
> +{
> +       return -ENODEV;
> +}
> +
> +static inline int zynqmp_pm_set_gem_config(u32 node,
> +                                          enum pm_gem_config_type config,
> +                                          u32 value)
> +{
> +       return -ENODEV;
> +}
> +
>  #endif
> 
>  #endif /* __FIRMWARE_ZYNQMP_H__ */
> --
> 2.25.1
> 

Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Conor.Dooley]

On 22/07/2022 09:12, Radhey Shyam Pandey wrote:
> 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.

For mpfs:
Tested-by: Conor Dooley <conor.dooley@microchip.com>

> - 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>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 7eb7822cd184..97f77fa9e165 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,25 @@ static int init_reset_optional(struct platform_device *pdev)
>  					     "failed to init SGMII PHY\n");
>  	}
>  
> +	ret = zynqmp_pm_is_function_supported(PM_IOCTL, IOCTL_SET_GEM_CONFIG);
> +	if (!ret) {
> +		u32 pm_info[2];
> +
> +		ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains",
> +						 pm_info, ARRAY_SIZE(pm_info));
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Failed to read power management information\n");
> +			return ret;
> +		}
> +		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/* Fully reset controller at hardware level if mapped in device tree */
>  	ret = device_reset_optional(&pdev->dev);
>  	if (ret) {

Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Andrew Lunn]

> +		ret = of_property_read_u32_array(pdev->dev.of_node, "power-domains",
> +						 pm_info, ARRAY_SIZE(pm_info));
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Failed to read power management information\n");
> +			return ret;
> +		}
> +		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0);
> +		if (ret < 0)
> +			return ret;
> +

Documentation/devicetree/bindings/net/cdns,macb.yaml says:

  power-domains:
    maxItems: 1

Yet you are using pm_info[1]?

    Andrew

RE: [PATCH net-next 1/2] firmware: xilinx: add support for sd/gem config

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com>
> Sent: Friday, July 22, 2022 2:22 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; ronak.jain@xilinx.com
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next 1/2] firmware: xilinx: add support for sd/gem
> config
> 
> On 22.07.2022 11:11, Radhey Shyam Pandey wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > From: Ronak Jain <ronak.jain@xilinx.com>
> >
> > Add new APIs in firmware to configure SD/GEM registers. Internally it
> > calls PM IOCTL for below SD/GEM register configuration:
> > - SD/EMMC select
> > - SD slot type
> > - SD base clock
> > - SD 8 bit support
> > - SD fixed config
> > - GEM SGMII Mode
> > - GEM fixed config
> >
> > Signed-off-by: Ronak Jain <ronak.jain@xilinx.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > ---
> >  drivers/firmware/xilinx/zynqmp.c     | 31 ++++++++++++++++++++++++++
> >  include/linux/firmware/xlnx-zynqmp.h | 33
> > ++++++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > index 7977a494a651..32a35bafb65e 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -1297,6 +1297,37 @@ int zynqmp_pm_get_feature_config(enum
> pm_feature_config_id id,
> >                                    id, 0, payload);  }
> >
> > +/**
> > + * zynqmp_pm_set_sd_config - PM call to set value of SD config registers
> > + * @node:      SD node ID
> > + * @config:    The config type of SD registers
> > + * @value:     Value to be set
> > + *
> > + * Return:      Returns 0 on success or error value on failure.
> 
> You have spaces after "Return:", doesn't match with tabs that you have after
> variable documentation.

Ok, will fix the return indentation to tab to match with variable documentation.
> 
> > + */
> > +int zynqmp_pm_set_sd_config(u32 node, enum pm_sd_config_type
> config,
> > +u32 value) {
> > +       return zynqmp_pm_invoke_fn(PM_IOCTL, node,
> IOCTL_SET_SD_CONFIG,
> > +                                  config, value, NULL); }
> > +EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_config);
> > +
> > +/**
> > + * zynqmp_pm_set_gem_config - PM call to set value of GEM config
> registers
> > + * @node:      GEM node ID
> > + * @config:    The config type of GEM registers
> > + * @value:     Value to be set
> > + *
> > + * Return:      Returns 0 on success or error value on failure.
> 
> Same here.

Will fix it in v2.
> 
> > + */
> > +int zynqmp_pm_set_gem_config(u32 node, enum pm_gem_config_type
> config,
> > +                            u32 value) {
> > +       return zynqmp_pm_invoke_fn(PM_IOCTL, node,
> IOCTL_SET_GEM_CONFIG,
> > +                                  config, value, NULL); }
> > +EXPORT_SYMBOL_GPL(zynqmp_pm_set_gem_config);
> > +
> >  /**
> >   * struct zynqmp_pm_shutdown_scope - Struct for shutdown scope
> >   * @subtype:   Shutdown subtype
> > diff --git a/include/linux/firmware/xlnx-zynqmp.h
> > b/include/linux/firmware/xlnx-zynqmp.h
> > index 1ec73d5352c3..063a93c133f1 100644
> > --- a/include/linux/firmware/xlnx-zynqmp.h
> > +++ b/include/linux/firmware/xlnx-zynqmp.h
> > @@ -152,6 +152,9 @@ enum pm_ioctl_id {
> >         /* Runtime feature configuration */
> >         IOCTL_SET_FEATURE_CONFIG = 26,
> >         IOCTL_GET_FEATURE_CONFIG = 27,
> > +       /* Dynamic SD/GEM configuration */
> > +       IOCTL_SET_SD_CONFIG = 30,
> > +       IOCTL_SET_GEM_CONFIG = 31,
> >  };
> >
> >  enum pm_query_id {
> > @@ -393,6 +396,18 @@ enum pm_feature_config_id {
> >         PM_FEATURE_EXTWDT_VALUE = 4,
> >  };
> >
> > +enum pm_sd_config_type {
> > +       SD_CONFIG_EMMC_SEL = 1, /* To set SD_EMMC_SEL in CTRL_REG_SD
> and SD_SLOTTYPE */
> > +       SD_CONFIG_BASECLK = 2, /* To set SD_BASECLK in SD_CONFIG_REG1
> */
> > +       SD_CONFIG_8BIT = 3, /* To set SD_8BIT in SD_CONFIG_REG2 */
> > +       SD_CONFIG_FIXED = 4, /* To set fixed config registers */ };
> > +
> > +enum pm_gem_config_type {
> > +       GEM_CONFIG_SGMII_MODE = 1, /* To set GEM_SGMII_MODE in
> GEM_CLK_CTRL register */
> > +       GEM_CONFIG_FIXED = 2, /* To set fixed config registers */ };
> > +
> >  /**
> >   * struct zynqmp_pm_query_data - PM query data
> >   * @qid:       query ID
> > @@ -468,6 +483,9 @@ int zynqmp_pm_feature(const u32 api_id);  int
> > zynqmp_pm_is_function_supported(const u32 api_id, const u32 id);  int
> > zynqmp_pm_set_feature_config(enum pm_feature_config_id id, u32
> value);
> > int zynqmp_pm_get_feature_config(enum pm_feature_config_id id, u32
> > *payload);
> > +int zynqmp_pm_set_sd_config(u32 node, enum pm_sd_config_type
> config,
> > +u32 value); int zynqmp_pm_set_gem_config(u32 node, enum
> pm_gem_config_type config,
> > +                            u32 value);
> >  #else
> >  static inline int zynqmp_pm_get_api_version(u32 *version)  { @@
> > -733,6 +751,21 @@ static inline int zynqmp_pm_get_feature_config(enum
> > pm_feature_config_id id,  {
> >         return -ENODEV;
> >  }
> > +
> > +static inline int zynqmp_pm_set_sd_config(u32 node,
> > +                                         enum pm_sd_config_type config,
> > +                                         u32 value) {
> > +       return -ENODEV;
> > +}
> > +
> > +static inline int zynqmp_pm_set_gem_config(u32 node,
> > +                                          enum pm_gem_config_type config,
> > +                                          u32 value) {
> > +       return -ENODEV;
> > +}
> > +
> >  #endif
> >
> >  #endif /* __FIRMWARE_ZYNQMP_H__ */
> > --
> > 2.25.1
> >

RE: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com>
> Sent: Friday, July 22, 2022 2:22 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; ronak.jain@xilinx.com
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic
> configuration support
> 
> On 22.07.2022 11:12, Radhey Shyam Pandey wrote:
> > 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>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > b/drivers/net/ethernet/cadence/macb_main.c
> > index 7eb7822cd184..97f77fa9e165 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,25 @@ static int init_reset_optional(struct platform_device
> *pdev)
> >                                              "failed to init SGMII PHY\n");
> >         }
> >
> > +       ret = zynqmp_pm_is_function_supported(PM_IOCTL,
> IOCTL_SET_GEM_CONFIG);
> > +       if (!ret) {
> > +               u32 pm_info[2];
> > +
> > +               ret = of_property_read_u32_array(pdev->dev.of_node, "power-
> domains",
> > +                                                pm_info, ARRAY_SIZE(pm_info));
> > +               if (ret < 0) {
> > +                       dev_err(&pdev->dev, "Failed to read power
> > + management information\n");
> 
> You have to undo phy_init() above (not listed in this diff).

Thanks for the review.  I see , will add phy_exit() in this return path
and for below error path as well.
> 
> > +                       return ret;
> > +               }
> > +               ret = zynqmp_pm_set_gem_config(pm_info[1],
> GEM_CONFIG_FIXED, 0);
> > +               if (ret < 0)
> 
> Same here.
> 
> > +                       return ret;
> > +
> > +               ret = zynqmp_pm_set_gem_config(pm_info[1],
> GEM_CONFIG_SGMII_MODE, 1);
> > +               if (ret < 0)
> 
> And here.
> 
> > +                       return ret;
> > +       }
> > +>         /* Fully reset controller at hardware level if mapped in
> > +> device
> tree */
> >         ret = device_reset_optional(&pdev->dev);
> >         if (ret) {
> > --
> > 2.25.1
> >

RE: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Sunday, July 24, 2022 10:24 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> claudiu.beznea@microchip.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic
> configuration support
> 
> > +		ret = of_property_read_u32_array(pdev->dev.of_node,
> "power-domains",
> > +						 pm_info,
> ARRAY_SIZE(pm_info));
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "Failed to read power
> management information\n");
> > +			return ret;
> > +		}
> > +		ret = zynqmp_pm_set_gem_config(pm_info[1],
> GEM_CONFIG_FIXED, 0);
> > +		if (ret < 0)
> > +			return ret;
> > +
> 
> Documentation/devicetree/bindings/net/cdns,macb.yaml says:
> 
>   power-domains:
>     maxItems: 1
> 
> Yet you are using pm_info[1]?

From power-domain description - It's a phandle and PM domain 
specifier as defined by bindings of the power controller specified
by phandle.

I assume the numbers of cells is specified by "#power-domain-cells":
Power-domain-cell is set to 1 in this case.

arch/arm64/boot/dts/xilinx/zynqmp.dtsi
#power-domain-cells = <1>;
power-domains = <&zynqmp_firmware PD_ETH_0>;

Please let me know your thoughts.

> 
>     Andrew

Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Andrew Lunn]

On Mon, Jul 25, 2022 at 02:34:51PM +0000, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Sunday, July 24, 2022 10:24 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> > claudiu.beznea@microchip.com; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic
> > configuration support
> > 
> > > +		ret = of_property_read_u32_array(pdev->dev.of_node,
> > "power-domains",
> > > +						 pm_info,
> > ARRAY_SIZE(pm_info));
> > > +		if (ret < 0) {
> > > +			dev_err(&pdev->dev, "Failed to read power
> > management information\n");
> > > +			return ret;
> > > +		}
> > > +		ret = zynqmp_pm_set_gem_config(pm_info[1],
> > GEM_CONFIG_FIXED, 0);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > 
> > Documentation/devicetree/bindings/net/cdns,macb.yaml says:
> > 
> >   power-domains:
> >     maxItems: 1
> > 
> > Yet you are using pm_info[1]?
> 
> >From power-domain description - It's a phandle and PM domain 
> specifier as defined by bindings of the power controller specified
> by phandle.
> 
> I assume the numbers of cells is specified by "#power-domain-cells":
> Power-domain-cell is set to 1 in this case.
> 
> arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> #power-domain-cells = <1>;
> power-domains = <&zynqmp_firmware PD_ETH_0>;
> 
> Please let me know your thoughts.

Ah, so you ignore the phandle value, and just use the PD_ETH_0?

How robust is this? What if somebody specified a different power
domain?

	Andrew

RE: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, July 25, 2022 10:41 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> claudiu.beznea@microchip.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic
> configuration support
> 
> On Mon, Jul 25, 2022 at 02:34:51PM +0000, Pandey, Radhey Shyam wrote:
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Sunday, July 24, 2022 10:24 PM
> > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> > > claudiu.beznea@microchip.com; davem@davemloft.net;
> > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx)
> > > <git@amd.com>
> > > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII
> > > dynamic configuration support
> > >
> > > > +		ret = of_property_read_u32_array(pdev->dev.of_node,
> > > "power-domains",
> > > > +						 pm_info,
> > > ARRAY_SIZE(pm_info));
> > > > +		if (ret < 0) {
> > > > +			dev_err(&pdev->dev, "Failed to read power
> > > management information\n");
> > > > +			return ret;
> > > > +		}
> > > > +		ret = zynqmp_pm_set_gem_config(pm_info[1],
> > > GEM_CONFIG_FIXED, 0);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +
> > >
> > > Documentation/devicetree/bindings/net/cdns,macb.yaml says:
> > >
> > >   power-domains:
> > >     maxItems: 1
> > >
> > > Yet you are using pm_info[1]?
> >
> > >From power-domain description - It's a phandle and PM domain
> > specifier as defined by bindings of the power controller specified by
> > phandle.
> >
> > I assume the numbers of cells is specified by "#power-domain-cells":
> > Power-domain-cell is set to 1 in this case.
> >
> > arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> > #power-domain-cells = <1>;
> > power-domains = <&zynqmp_firmware PD_ETH_0>;
> >
> > Please let me know your thoughts.
> 
> Ah, so you ignore the phandle value, and just use the PD_ETH_0?
> 
> How robust is this? What if somebody specified a different power domain?

Some background - init_reset_optional() fn is implemented
for three platforms i.e., zynqmp, versal, MPFS.

zynqmp_pm_set_gem_config API expect first argument as GEM node id
so, power-domain DT property is passed to get node ID.

However, power-domain property is read only if underlying firmware 
supports configuration of GEM secure space. It's only true for 
zynqmp SGMII case and for zynqmp power domain is fixed.
In addition to it there is an error handling in power-domain 
property parsing. Hope this answers the question.

> 
> 	Andrew

RE: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Sent: Wednesday, July 27, 2022 12:18 AM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> claudiu.beznea@microchip.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> Subject: RE: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic
> configuration support
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, July 25, 2022 10:41 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> > claudiu.beznea@microchip.com; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic
> > configuration support
> >
> > On Mon, Jul 25, 2022 at 02:34:51PM +0000, Pandey, Radhey Shyam wrote:
> > > > -----Original Message-----
> > > > From: Andrew Lunn <andrew@lunn.ch>
> > > > Sent: Sunday, July 24, 2022 10:24 PM
> > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > > Cc: michal.simek@xilinx.com; nicolas.ferre@microchip.com;
> > > > claudiu.beznea@microchip.com; davem@davemloft.net;
> > > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > > > gregkh@linuxfoundation.org; ronak.jain@xilinx.com; linux-arm-
> > > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > > netdev@vger.kernel.org; git@xilinx.com; git (AMD-Xilinx)
> > > > <git@amd.com>
> > > > Subject: Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII
> > > > dynamic configuration support
> > > >
> > > > > +		ret = of_property_read_u32_array(pdev-
> >dev.of_node,
> > > > "power-domains",
> > > > > +						 pm_info,
> > > > ARRAY_SIZE(pm_info));
> > > > > +		if (ret < 0) {
> > > > > +			dev_err(&pdev->dev, "Failed to read power
> > > > management information\n");
> > > > > +			return ret;
> > > > > +		}
> > > > > +		ret = zynqmp_pm_set_gem_config(pm_info[1],
> > > > GEM_CONFIG_FIXED, 0);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +
> > > >
> > > > Documentation/devicetree/bindings/net/cdns,macb.yaml says:
> > > >
> > > >   power-domains:
> > > >     maxItems: 1
> > > >
> > > > Yet you are using pm_info[1]?
> > >
> > > >From power-domain description - It's a phandle and PM domain
> > > specifier as defined by bindings of the power controller specified
> > > by phandle.
> > >
> > > I assume the numbers of cells is specified by "#power-domain-cells":
> > > Power-domain-cell is set to 1 in this case.
> > >
> > > arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> > > #power-domain-cells = <1>;
> > > power-domains = <&zynqmp_firmware PD_ETH_0>;
> > >
> > > Please let me know your thoughts.
> >
> > Ah, so you ignore the phandle value, and just use the PD_ETH_0?
> >
> > How robust is this? What if somebody specified a different power domain?
> 
> Some background - init_reset_optional() fn is implemented for three
> platforms i.e., zynqmp, versal, MPFS.
> 
> zynqmp_pm_set_gem_config API expect first argument as GEM node id so,
> power-domain DT property is passed to get node ID.
> 
> However, power-domain property is read only if underlying firmware
> supports configuration of GEM secure space. It's only true for zynqmp SGMII
> case and for zynqmp power domain is fixed.
> In addition to it there is an error handling in power-domain property parsing.
> Hope this answers the question.

Please let me know the implementation looks fine or needs any modification?

Re: [PATCH net-next 2/2] net: macb: Add zynqmp SGMII dynamic configuration support

[Andrew Lunn]

> > > How robust is this? What if somebody specified a different power domain?
> > 
> > Some background - init_reset_optional() fn is implemented for three
> > platforms i.e., zynqmp, versal, MPFS.
> > 
> > zynqmp_pm_set_gem_config API expect first argument as GEM node id so,
> > power-domain DT property is passed to get node ID.
> > 
> > However, power-domain property is read only if underlying firmware
> > supports configuration of GEM secure space. It's only true for zynqmp SGMII
> > case and for zynqmp power domain is fixed.
> > In addition to it there is an error handling in power-domain property parsing.
> > Hope this answers the question.
> 
> Please let me know the implementation looks fine or needs any modification?

Given that explanation, it looks fine.

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

    Andrew