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

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


Changes for v2:
- Add phy_exit() in error return paths.
- Use tab indent for zynqmp_pm_set_sd/gem_config return documentation.

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 | 25 ++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h     | 33 ++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)

-- 
2.1.1

[PATCH v2 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>
---
Changes for v2:
- Use tab indent for zynqmp_pm_set_sd/gem_config return documentation.
---
 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..44c44077dfc5 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -1298,6 +1298,37 @@ int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
 }
 
 /**
+ * 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
  * @name:	Matching string for scope argument
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.1.1

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

[Claudiu.Beznea]

On 29.07.2022 22:35, 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>
> ---
> Changes for v2:
> - Use tab indent for zynqmp_pm_set_sd/gem_config return documentation.
> ---
>  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..44c44077dfc5 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -1298,6 +1298,37 @@ int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
>  }
> 
>  /**
> + * 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
>   * @name:      Matching string for scope argument
> 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 */
> +};

As you adapted kernel style documentation for the rest of code added in
this patch you can follow this rules for enums, too.

> +
>  /**
>   * 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.1.1
> 

RE: [PATCH v2 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: Monday, August 1, 2022 3:27 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) <git@amd.com>; git@xilinx.com;
> ronak.jain@xilinx.com
> Subject: Re: [PATCH v2 net-next 1/2] firmware: xilinx: add support for
> sd/gem config
> 
> On 29.07.2022 22:35, 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>
> > ---
> > Changes for v2:
> > - Use tab indent for zynqmp_pm_set_sd/gem_config return
> documentation.
> > ---
> >  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..44c44077dfc5 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -1298,6 +1298,37 @@ int zynqmp_pm_get_feature_config(enum
> > pm_feature_config_id id,  }
> >
> >  /**
> > + * 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
> >   * @name:      Matching string for scope argument
> > 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 */ };
> 
> As you adapted kernel style documentation for the rest of code added in this
> patch you can follow this rules for enums, too.

Which particular style issue you are mentioning here? There is a tab 
before GEM_CONFIG_* enum member and also checkpatch  --strict 
report no issues.

> 
> > +
> >  /**
> >   * 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.1.1
> >

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

[Claudiu.Beznea]

On 01.08.2022 15:52, Pandey, Radhey Shyam wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> -----Original Message-----
>> From: Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com>
>> Sent: Monday, August 1, 2022 3:27 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) <git@amd.com>; git@xilinx.com;
>> ronak.jain@xilinx.com
>> Subject: Re: [PATCH v2 net-next 1/2] firmware: xilinx: add support for
>> sd/gem config
>>
>> On 29.07.2022 22:35, 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>
>>> ---
>>> Changes for v2:
>>> - Use tab indent for zynqmp_pm_set_sd/gem_config return
>> documentation.
>>> ---
>>>  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..44c44077dfc5 100644
>>> --- a/drivers/firmware/xilinx/zynqmp.c
>>> +++ b/drivers/firmware/xilinx/zynqmp.c
>>> @@ -1298,6 +1298,37 @@ int zynqmp_pm_get_feature_config(enum
>>> pm_feature_config_id id,  }
>>>
>>>  /**
>>> + * 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
>>>   * @name:      Matching string for scope argument
>>> 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 */ };
>>
>> As you adapted kernel style documentation for the rest of code added in this
>> patch you can follow this rules for enums, too.
> 
> Which particular style issue you are mentioning here?

I'm talking about:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst#n169

 There is a tab
> before GEM_CONFIG_* enum member and also checkpatch  --strict
> report no issues.

You have this for functions:
+/**
+ * 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.
+ */

And some structures in the file are using it, e.g.:

/**

 * struct zynqmp_pm_query_data - PM query data

 * @qid:        query ID

 * @arg1:       Argument 1 of query data

 * @arg2:       Argument 2 of query data

 * @arg3:       Argument 3 of query data

 */


> 
>>
>>> +
>>>  /**
>>>   * 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.1.1
>>>
> 

RE: [PATCH v2 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: Monday, August 1, 2022 8:36 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; git@xilinx.com;
> ronak.jain@xilinx.com; Nicolas.Ferre@microchip.com;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; gregkh@linuxfoundation.org; michal.simek@xilinx.com
> Subject: Re: [PATCH v2 net-next 1/2] firmware: xilinx: add support for
> sd/gem config
> 
> On 01.08.2022 15:52, Pandey, Radhey Shyam wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> >> -----Original Message-----
> >> From: Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com>
> >> Sent: Monday, August 1, 2022 3:27 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) <git@amd.com>; git@xilinx.com; ronak.jain@xilinx.com
> >> Subject: Re: [PATCH v2 net-next 1/2] firmware: xilinx: add support
> >> for sd/gem config
> >>
> >> On 29.07.2022 22:35, 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>
> >>> ---
> >>> Changes for v2:
> >>> - Use tab indent for zynqmp_pm_set_sd/gem_config return
> >> documentation.
> >>> ---
> >>>  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..44c44077dfc5 100644
> >>> --- a/drivers/firmware/xilinx/zynqmp.c
> >>> +++ b/drivers/firmware/xilinx/zynqmp.c
> >>> @@ -1298,6 +1298,37 @@ int zynqmp_pm_get_feature_config(enum
> >>> pm_feature_config_id id,  }
> >>>
> >>>  /**
> >>> + * 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
> >>>   * @name:      Matching string for scope argument
> >>> 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 */ };
> >>
> >> As you adapted kernel style documentation for the rest of code added
> >> in this patch you can follow this rules for enums, too.
> >
> > Which particular style issue you are mentioning here?
> 
> I'm talking about:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu
> mentation/doc-guide/kernel-doc.rst#n169
> 
>  There is a tab
> > before GEM_CONFIG_* enum member and also checkpatch  --strict report
> > no issues.
> 
> You have this for functions:
> +/**
> + * 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.
> + */
> 
> And some structures in the file are using it, e.g.:
> 
> /**
> 
>  * struct zynqmp_pm_query_data - PM query data
> 
>  * @qid:        query ID
> 
>  * @arg1:       Argument 1 of query data
> 
>  * @arg2:       Argument 2 of query data
> 
>  * @arg3:       Argument 3 of query data
> 
>  */

Thanks, I see . will modify enum documentation for pm_sd_config_type and
enum pm_gem_config_type as these are new enum defined in this patch.
Something like:
/**
 * enum pm_sd_config_type - PM SD configuration.
 * @SD_CONFIG_EMMC_SEL: To set SD_EMMC_SEL in CTRL_REG_SD and SD_SLOTTYPE
 * @SD_CONFIG_BASECLK: To set SD_BASECLK in SD_CONFIG_REG1
 * @SD_CONFIG_8BIT: To set SD_8BIT in SD_CONFIG_REG2
 * @SD_CONFIG_FIXED: To set fixed config registers
 */
enum pm_sd_config_type {
        SD_CONFIG_EMMC_SEL = 1,
        SD_CONFIG_BASECLK = 2,
        SD_CONFIG_8BIT = 3,
        SD_CONFIG_FIXED = 4,
};

For pm_ioctl_id as its extension I think we should keep enum 
documentation as is and complete enum documentation can 
be converted later in a follow-up patch. Hope that's fine.

> 
> 
> >
> >>
> >>> +
> >>>  /**
> >>>   * 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.1.1
> >>>
> >

[PATCH v2 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>
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)
 					     "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) {
+			phy_exit(bp->sgmii_phy);
+			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) {
+			phy_exit(bp->sgmii_phy);
+			return ret;
+		}
+
+		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
+		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

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

[Conor.Dooley]

On 29/07/2022 20:35, 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.
> - 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)

Do you have cc suppression turned on or did this not get picked up
b/c of the (for MPFS) you added? In the future, please CC me on
later revisions if I provided a tested-by :)
Thanks,
Conor.

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

[Claudiu.Beznea]

On 29.07.2022 22:35, 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>
> 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)
>                                              "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) {
> +                       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.

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.

> +                       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) {
> +                       phy_exit(bp->sgmii_phy);
> +                       return ret;
> +               }
> +
> +               ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
> +               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
> 

RE: [PATCH v2 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: 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) <git@amd.com>; 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:
> > 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)
> >                                              "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) {
> > +                       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.

> 
> > +                       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) {
> > +                       phy_exit(bp->sgmii_phy);
> > +                       return ret;
> > +               }
> > +
> > +               ret = zynqmp_pm_set_gem_config(pm_info[1],
> GEM_CONFIG_SGMII_MODE, 1);
> > +               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
> >