Message ID | 20240415-pinctrl-scmi-v10-0-59c6e7a586ee@nxp.com (mailing list archive) |
---|---|
Headers | show |
Series | firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support | expand |
I'm trying to re-base AKASHI Takahiro's gpio driver on top of your scmi pinctrl driver. https://lore.kernel.org/all/20231005025843.508689-1-takahiro.akashi@linaro.org/ I need to do something like this below to save the gpio information. So now, great, I have the information but I'm not sure how to export it from the scmi pinctrl driver to the gpio driver... (This is a probably a stupid question but I am real newbie with regards to gpio). The other thing is that the SCMI spec says: 4.11.2.7 PINCTRL_SETTINGS_GET This command can be used by an agent to get the pin or group configuration, and the function selected to be enabled. It can also be used to read the value of a pin when it is set to GPIO mode. What does that mean? Is that right, or is it something left over from a previous revision of the spec. regards, dan carpenter diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c index a2a7f880d6a3..f803be8a223f 100644 --- a/drivers/firmware/arm_scmi/pinctrl.c +++ b/drivers/firmware/arm_scmi/pinctrl.c @@ -26,6 +26,7 @@ #define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0)) #define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0)) +#define IS_GPIO_FUNC(x) le32_get_bits((x), BIT(17)) #define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31)) #define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0)) @@ -107,6 +108,7 @@ struct scmi_group_info { struct scmi_function_info { char name[SCMI_MAX_STR_SIZE]; bool present; + bool gpio; u32 *groups; u32 nr_groups; }; @@ -189,7 +191,7 @@ static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph, static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph, enum scmi_pinctrl_selector_type type, - u32 selector, char *name, + u32 selector, char *name, bool *gpio, u32 *n_elems) { int ret; @@ -216,17 +218,20 @@ static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph, tx->flags = cpu_to_le32(type); ret = ph->xops->do_xfer(ph, t); - if (!ret) { - if (n_elems) - *n_elems = NUM_ELEMS(rx->attributes); + if (ret) + goto xfer_put; - strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); + if (n_elems) + *n_elems = NUM_ELEMS(rx->attributes); - ext_name_flag = !!EXT_NAME_FLAG(rx->attributes); - } + if (type == FUNCTION_TYPE && gpio) + *gpio = !!IS_GPIO_FUNC(rx->attributes); - ph->xops->xfer_put(ph, t); + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); + ext_name_flag = !!EXT_NAME_FLAG(rx->attributes); +xfer_put: + ph->xops->xfer_put(ph, t); if (ret) return ret; /* @@ -602,7 +607,7 @@ static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph, int ret; ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, group->name, - &group->nr_pins); + NULL, &group->nr_pins); if (ret) return ret; @@ -687,7 +692,7 @@ static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph, int ret; ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, func->name, - &func->nr_groups); + &func->gpio, &func->nr_groups); if (ret) return ret; @@ -778,7 +783,8 @@ static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph, if (!pin) return -EINVAL; - ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name, NULL); + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name, NULL, + NULL); if (ret) return ret;
On Tue, Apr 16, 2024 at 09:20:11PM +0300, Dan Carpenter wrote: > I'm trying to re-base AKASHI Takahiro's gpio driver on top of your scmi > pinctrl driver. > https://lore.kernel.org/all/20231005025843.508689-1-takahiro.akashi@linaro.org/ > I need to do something like this below to save the gpio information. > > So now, great, I have the information but I'm not sure how to export it > from the scmi pinctrl driver to the gpio driver... (This is a probably > a stupid question but I am real newbie with regards to gpio). > Hi Dan, I dont think it is a stupid question, I'll try to answer your questions as much as possible, regarding the SCMI side, since I am definitely not so much familiar with the GPIO/Pinctrl subsystem either. First of all, to put things in perspective, drivers/firmware/arm_scmi/pinctrl.c is just the SCMI protocol layer, which as part of the core SCMI driver is in charge of implementing the specific protocol (i.e. building and sending appropriate messages via the SCMI core) and which, in turn, exposes a set of protocol-specific operations in scmi_protocol.h. On top of this there are the SCMI drivers (like drivers/pinctrl/pinctrl-scmi.c) that, on one side, plug into the SCMI stack, and as such can use the specific protocol_ops, and on the other side register into some custom existing Linux susbsystem like Pinctrl, so that it can relay generic Pinctrl related requests, via the above SCMI pinctrl_ops, to the platform SCMI fw (finally translated into SCMI messages at the protocol layer...) In all of this, note that the various protocol_ops in scmi_protocol.h are NOT exported symbols (would have been dozens): that is the reason why a driver willing to use an SCMI protocol via its specific ops, has to be, first, an SCMI driver so that can grab the related protocol_ops and an handle to the SCMI instance, during its probe phase. NOW, as far as I can remember (and have understood) AKASHI gpio-pinctrl driver was INSTEAD meant to be a generic GPIO driver on top of Pinctrl subsystem, so something that could work on top of any pinctrl controller, NOT necessarily an SCMI one, so, as a consequence it is NOT an SCMI driver and it cannot access directly any of the pinctrl_ops (existing or future), BUT it will have, instead, to be based on the Pinctrl subsystem API to achieve its functionalities in a generic manner. So at the end, AFAICU: - you collect any additional gpio info you need (and can get from the spec) at the SCMI Pinctrl protocol layer in drivers/firmware/arm_scmi/pinctrl.c (as you are doing) - you expose such info via pinctrl_ops: in these regards many OTHER protocols usually exposes some .get_info() ops to get a generic info descriptor including all info about a specific resource, BUT this is not the case for pinctrl_ops, which just exposes a few custom ops to get only the bits that are strictly needed (like resource names via .name_get()). Here is up to you which kind of interface to expose really, depending on the SCMI Pinctrl driver usage pattern. (is_gpio() ? .get_gpios() ? just a new out-param in an existing ops ?) - in the SCMI pinctrl-scmi driver you can finally make use of your new protocol_ops to provide to the Pinctrl subsystem the funcs needed by the Pinctrl API calls as issued by the gpio-pinctrl driver....and in these regards I really dont know what are the missing bits...I suppose something that has to work as the SCMI backend for the pinctrl_gpio_* calls inside gpio-pinctrl. > The other thing is that the SCMI spec says: > > 4.11.2.7 > PINCTRL_SETTINGS_GET > > This command can be used by an agent to get the pin or group > configuration, and the function selected to be enabled. It can also > be used to read the value of a pin when it is set to GPIO mode. > > What does that mean? Is that right, or is it something left over from a > previous revision of the spec. > My guess is that, this is a (certainly obscure) way for the spec to express the fact that using this message you can get the pin/group selected funcs AND pin/group configs, configs, that, include the settings for any OEM Config type as specified in SCMI spec Table 24, which, in turn, contains Input-mode/Output-mode/Output-value types that I suppose pertain to the GPIO world. As a consequence, I guess you neeed somehow to connect the above pinctrl_gpio_set/get_config and pinctrl_gpio_get_direction into the pinctrl_ops .setting_get_one() and .settings_conf() by using the proper GPIO-related OEM types, not sure of the details (as said I am ignorant) BUT it could be that this is already handled somehow by the current pinctrl-scmi driver if the GPIO ranges are handled correctly throughout all the chain of susbsystem involved... (looking at the internals of https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/core.c#L912) ...but I really not familiar on how GPIO ranges are supposed to work so it is better that now I shut up :D ...apologies for the long email, especially if I said something already obvious. Thanks, Cristian
Hi Dan, > Subject: Re: [PATCH v10 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > I'm trying to re-base AKASHI Takahiro's gpio driver on top of your scmi pinctrl > driver. > https://lore.ke/ > rnel.org%2Fall%2F20231005025843.508689-1- > takahiro.akashi%40linaro.org%2F&data=05%7C02%7Cpeng.fan%40nxp.com% > 7C342dd6eb0463456d0d6608dc5e41de1c%7C686ea1d3bc2b4c6fa92cd99c5 > c301635%7C0%7C0%7C638488884186606528%7CUnknown%7CTWFpbGZs > b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn > 0%3D%7C0%7C%7C%7C&sdata=DMJZ2uwuJigkEnEcY7JdBw6DMPjHxcUvvh7 > 2fsaep50%3D&reserved=0 > I need to do something like this below to save the gpio information. > > So now, great, I have the information but I'm not sure how to export it from > the scmi pinctrl driver to the gpio driver... (This is a probably a stupid > question but I am real newbie with regards to gpio). > > The other thing is that the SCMI spec says: > > 4.11.2.7 > PINCTRL_SETTINGS_GET > > This command can be used by an agent to get the pin or group > configuration, and the function selected to be enabled. It can also > be used to read the value of a pin when it is set to GPIO mode. > > What does that mean? Is that right, or is it something left over from a > previous revision of the spec. > > regards, > dan carpenter > > diff --git a/drivers/firmware/arm_scmi/pinctrl.c > b/drivers/firmware/arm_scmi/pinctrl.c Just a short question, you will make this a standalone patch part of your gpio pinctrl patchset, right? Or you wanna include this change in my v11 patch? I hope v11 + imx oem patches could land in 6.10, so I would not expect big changes to v11. Thanks, Peng. > index a2a7f880d6a3..f803be8a223f 100644 > --- a/drivers/firmware/arm_scmi/pinctrl.c > +++ b/drivers/firmware/arm_scmi/pinctrl.c > @@ -26,6 +26,7 @@ > #define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > #define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0)) > > +#define IS_GPIO_FUNC(x) le32_get_bits((x), BIT(17)) > #define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31)) > #define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0)) > > @@ -107,6 +108,7 @@ struct scmi_group_info { struct scmi_function_info { > char name[SCMI_MAX_STR_SIZE]; > bool present; > + bool gpio; > u32 *groups; > u32 nr_groups; > }; > @@ -189,7 +191,7 @@ static int scmi_pinctrl_validate_id(const struct > scmi_protocol_handle *ph, > > static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph, > enum scmi_pinctrl_selector_type type, > - u32 selector, char *name, > + u32 selector, char *name, bool *gpio, > u32 *n_elems) > { > int ret; > @@ -216,17 +218,20 @@ static int scmi_pinctrl_attributes(const struct > scmi_protocol_handle *ph, > tx->flags = cpu_to_le32(type); > > ret = ph->xops->do_xfer(ph, t); > - if (!ret) { > - if (n_elems) > - *n_elems = NUM_ELEMS(rx->attributes); > + if (ret) > + goto xfer_put; > > - strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); > + if (n_elems) > + *n_elems = NUM_ELEMS(rx->attributes); > > - ext_name_flag = !!EXT_NAME_FLAG(rx->attributes); > - } > + if (type == FUNCTION_TYPE && gpio) > + *gpio = !!IS_GPIO_FUNC(rx->attributes); > > - ph->xops->xfer_put(ph, t); > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE); > + ext_name_flag = !!EXT_NAME_FLAG(rx->attributes); > > +xfer_put: > + ph->xops->xfer_put(ph, t); > if (ret) > return ret; > /* > @@ -602,7 +607,7 @@ static int scmi_pinctrl_get_group_info(const struct > scmi_protocol_handle *ph, > int ret; > > ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector, group- > >name, > - &group->nr_pins); > + NULL, &group->nr_pins); > if (ret) > return ret; > > @@ -687,7 +692,7 @@ static int scmi_pinctrl_get_function_info(const struct > scmi_protocol_handle *ph, > int ret; > > ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector, func- > >name, > - &func->nr_groups); > + &func->gpio, &func->nr_groups); > if (ret) > return ret; > > @@ -778,7 +783,8 @@ static int scmi_pinctrl_get_pin_info(const struct > scmi_protocol_handle *ph, > if (!pin) > return -EINVAL; > > - ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name, > NULL); > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector, pin->name, > NULL, > + NULL); > if (ret) > return ret; >
On Wed, Apr 17, 2024 at 12:15:57PM +0000, Peng Fan wrote: > Hi Dan, > > > Subject: Re: [PATCH v10 0/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol > > protocol basic support > > > > I'm trying to re-base AKASHI Takahiro's gpio driver on top of your scmi pinctrl > > driver. > > https://lore.ke/ > > rnel.org%2Fall%2F20231005025843.508689-1- > > takahiro.akashi%40linaro.org%2F&data=05%7C02%7Cpeng.fan%40nxp.com% > > 7C342dd6eb0463456d0d6608dc5e41de1c%7C686ea1d3bc2b4c6fa92cd99c5 > > c301635%7C0%7C0%7C638488884186606528%7CUnknown%7CTWFpbGZs > > b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn > > 0%3D%7C0%7C%7C%7C&sdata=DMJZ2uwuJigkEnEcY7JdBw6DMPjHxcUvvh7 > > 2fsaep50%3D&reserved=0 > > I need to do something like this below to save the gpio information. > > > > So now, great, I have the information but I'm not sure how to export it from > > the scmi pinctrl driver to the gpio driver... (This is a probably a stupid > > question but I am real newbie with regards to gpio). > > > > The other thing is that the SCMI spec says: > > > > 4.11.2.7 > > PINCTRL_SETTINGS_GET > > > > This command can be used by an agent to get the pin or group > > configuration, and the function selected to be enabled. It can also > > be used to read the value of a pin when it is set to GPIO mode. > > > > What does that mean? Is that right, or is it something left over from a > > previous revision of the spec. > > > > regards, > > dan carpenter > > > > diff --git a/drivers/firmware/arm_scmi/pinctrl.c > > b/drivers/firmware/arm_scmi/pinctrl.c > > Just a short question, you will make this a standalone > patch part of your gpio pinctrl patchset, right? > > Or you wanna include this change in my v11 patch? > > I hope v11 + imx oem patches could land in 6.10, > so I would not expect big changes to v11. Yeah. Let's get your patches merged. I don't think there is anything in mine which conflict with yours. It's just addon. regards, dan carpenter
On Wed, Apr 17, 2024 at 12:15:57PM +0000, Peng Fan wrote: > Hi Dan, > > Just a short question, you will make this a standalone > patch part of your gpio pinctrl patchset, right? > > Or you wanna include this change in my v11 patch? > > I hope v11 + imx oem patches could land in 6.10, I haven't looked at i.MX OEM patches even once so far. IIRC it is all in the pinctrl driver and you may not need my review/ack. But let us get this series in first. This series looks good overall. Since it has pinctrl driver, I need Linus to ack/agree to pick the whole series up or I can ack them so that Linus can take the whole series. Either way it is fine for me. Thanks for all your efforts in pursuing this. -- Regards, Sudeep