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

From: Pandey, Radhey Shyam
Date: Mon Aug 01 2022 - 14:50:56 EST

Next message: Marijn Suijten: "Re: [PATCH v2 2/3] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: move common nodes to a common dtsi"
Previous message: Siddh Raman Pant: "Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue"
In reply to: Claudiu.Beznea: "Re: [PATCH v2 net-next 1/2] firmware: xilinx: add support for sd/gem config"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Claudiu.Beznea@xxxxxxxxxxxxx <Claudiu.Beznea@xxxxxxxxxxxxx>
> Sent: Monday, August 1, 2022 8:36 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; git@xxxxxxxxxx;
> ronak.jain@xxxxxxxxxx; Nicolas.Ferre@xxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; michal.simek@xxxxxxxxxx
> 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@xxxxxxxxxxxxx <Claudiu.Beznea@xxxxxxxxxxxxx>
> >> Sent: Monday, August 1, 2022 3:27 PM
> >> To: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>;
> >> michal.simek@xxxxxxxxxx; Nicolas.Ferre@xxxxxxxxxxxxx;
> >> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> >> pabeni@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx
> >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; git
> >> (AMD-Xilinx) <git@xxxxxxx>; git@xxxxxxxxxx; ronak.jain@xxxxxxxxxx
> >> 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@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >>> Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@xxxxxxx>
> >>> ---
> >>> 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
> >>>
> >

Next message: Marijn Suijten: "Re: [PATCH v2 2/3] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: move common nodes to a common dtsi"
Previous message: Siddh Raman Pant: "Re: [PATCH v3] kernel/watch_queue: Make pipe NULL while clearing watch_queue"
In reply to: Claudiu.Beznea: "Re: [PATCH v2 net-next 1/2] firmware: xilinx: add support for sd/gem config"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]