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

["Pandey, Radhey Shyam" <radhey.shyam.pandey@amd.com>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel