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

quoted

-----Original Message-----
From: Claudiu.Beznea@microchip.com <redacted>
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) [off-list ref]; 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:

quoted

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

From: Ronak Jain <redacted>

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 <redacted>
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.

quoted

---
 drivers/firmware/xilinx/zynqmp.c     | 31

+++++++++++++++++++++++++++++++

quoted

 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,

quoted

+u32 value) {
+       return zynqmp_pm_invoke_fn(PM_IOCTL, node,

IOCTL_SET_SD_CONFIG,

quoted

+                                  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

quoted

+ * @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,

quoted

+                            u32 value) {
+       return zynqmp_pm_invoke_fn(PM_IOCTL, node,

IOCTL_SET_GEM_CONFIG,

quoted

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

quoted

+       SD_CONFIG_BASECLK = 2, /* To set SD_BASECLK in SD_CONFIG_REG1

*/

quoted

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

quoted

+       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

 */

quoted

quoted

+
 /**
  * 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);

quoted

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,

quoted

+u32 value); int zynqmp_pm_set_gem_config(u32 node, enum

pm_gem_config_type config,

quoted

+                            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