| Message ID | 20230824070611.3335107-1-peng.fan@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] scmi: pinctrl: support i.MX9 | expand |
On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > This patch is just to introduce i.MX support to see whether people have > comments for the design. Very interesting! > The binding format: > <mux_reg conf_reg input_reg mux_mode input_val> > dts: > pinctrl_uart1: uart1grp { > fsl,pins = < > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e > >; > }; > > i.MX pinctrl not use generic pinconf, this has been agreeed by > maintainers before. Yes, it has historical reasons. > So after moving to SCMI, we will still > keep the same binding format, and i.MX SCMI firmware also use same > format when configure registers. So we need to use i.MX specific > dt_node_to_map function. I thought the idea with SCMI was to abstract and hide the characteristics of the underlying hardware. I.e. the firmware is to present groups and functions and generic config options and then the driver will use these. This patch, it seems, creates a hybrid between the old freescale driver and the SCMI firmware communication link where the SCMI is just a transport mechanism to something inside SCMI that poke the same registers that userspace could poke, if it could only access these registers. I.e using SCMI on this platform isn't creating any abstraction of the pin control hardware, it is merely making things more complex and also slower bymaking the registers only accessible from this SCMI link. But I could have misunderstood it, so please correct me! Yours, Linus Walleij
Hi Linus, Thanks a lot for the quick response. On Thu, Aug 24, 2023 at 10:43:20AM +0200, Linus Walleij wrote: > On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > > This patch is just to introduce i.MX support to see whether people have > > comments for the design. > > Very interesting! > > > The binding format: > > <mux_reg conf_reg input_reg mux_mode input_val> > > dts: > > pinctrl_uart1: uart1grp { > > fsl,pins = < > > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e > > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e > > >; > > }; > > > > i.MX pinctrl not use generic pinconf, this has been agreeed by > > maintainers before. > > Yes, it has historical reasons. > Good to know. > > So after moving to SCMI, we will still > > keep the same binding format, and i.MX SCMI firmware also use same > > format when configure registers. So we need to use i.MX specific > > dt_node_to_map function. > > I thought the idea with SCMI was to abstract and hide the characteristics of > the underlying hardware. I.e. the firmware is to present groups and > functions and generic config options and then the driver will use these. > Correct. > This patch, it seems, creates a hybrid between the old freescale driver > and the SCMI firmware communication link where the SCMI is just a > transport mechanism to something inside SCMI that poke the same > registers that userspace could poke, if it could only access these > registers. > > I.e using SCMI on this platform isn't creating any abstraction of the > pin control hardware, it is merely making things more complex and > also slower bymaking the registers only accessible from this SCMI link. > Agreed. I don't have much knowledge on generic pinmux conf and suggested Peng to post the RFC to start the discussion instead of getting blocked by me during some internal/private discussions as the main intention for him was upstreaming the changes. I am against the idea of mixing platform specific changes the way it is done here but since I didn't have much knowledge on pinmux conf to suggest/provide any useful feedback I suggested to trigger this discussion. > But I could have misunderstood it, so please correct me! +1
Hi Linus, Thanks for quick reply. > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> > wrote: > > > This patch is just to introduce i.MX support to see whether people > > have comments for the design. > > Very interesting! > > > The binding format: > > <mux_reg conf_reg input_reg mux_mode input_val> > > dts: > > pinctrl_uart1: uart1grp { > > fsl,pins = < > > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e > > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e > > >; > > }; > > > > i.MX pinctrl not use generic pinconf, this has been agreeed by > > maintainers before. > > Yes, it has historical reasons. > > > So after moving to SCMI, we will still keep the same binding format, > > and i.MX SCMI firmware also use same format when configure registers. > > So we need to use i.MX specific dt_node_to_map function. > > I thought the idea with SCMI was to abstract and hide the characteristics of > the underlying hardware. I.e. the firmware is to present groups and > functions and generic config options and then the driver will use these. > > This patch, it seems, creates a hybrid between the old freescale driver and > the SCMI firmware communication link where the SCMI is just a transport > mechanism to something inside SCMI that poke the same registers that > userspace could poke, if it could only access these registers. > > I.e using SCMI on this platform isn't creating any abstraction of the pin > control hardware, Right. it is merely making things more complex and also slower > bymaking the registers only accessible from this SCMI link. This is for safety reason, the pinctrl hardware must be handled by a system manager entity. So mmio direct access not allowed from Cortex-A side. The SCMI firmware is very straightforward, there is no group or function. It just accepts the format as this: MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID, DAISY_CFG, DAISY_VALUE. Similar as linux MMIO format. Our i.MX95 platform will support two settings, one with SCMI firmware, one without SCMI. These two settings will share the same pinctrl header file. And to simplify the scmi firmware design(anyway I am not owner of the firmware), to make pinctrl header shared w/o scmi, we take the current in-upstream freescale imx binding format. Thanks, Peng. > > But I could have misunderstood it, so please correct me! > > Yours, > Linus Walleij
Hi Sudeep, > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > Hi Linus, > > Thanks a lot for the quick response. > > On Thu, Aug 24, 2023 at 10:43:20AM +0200, Linus Walleij wrote: > > On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> > wrote: > > > > > This patch is just to introduce i.MX support to see whether people > > > have comments for the design. > > > > Very interesting! > > > > > The binding format: > > > <mux_reg conf_reg input_reg mux_mode input_val> > > > dts: > > > pinctrl_uart1: uart1grp { > > > fsl,pins = < > > > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e > > > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e > > > >; > > > }; > > > > > > i.MX pinctrl not use generic pinconf, this has been agreeed by > > > maintainers before. > > > > Yes, it has historical reasons. > > > > Good to know. > > > > So after moving to SCMI, we will still keep the same binding format, > > > and i.MX SCMI firmware also use same format when configure > > > registers. So we need to use i.MX specific dt_node_to_map function. > > > > I thought the idea with SCMI was to abstract and hide the > > characteristics of the underlying hardware. I.e. the firmware is to > > present groups and functions and generic config options and then the > driver will use these. > > > > Correct. > > > This patch, it seems, creates a hybrid between the old freescale > > driver and the SCMI firmware communication link where the SCMI is just > > a transport mechanism to something inside SCMI that poke the same > > registers that userspace could poke, if it could only access these > > registers. > > > > I.e using SCMI on this platform isn't creating any abstraction of the > > pin control hardware, it is merely making things more complex and also > > slower bymaking the registers only accessible from this SCMI link. > > > > Agreed. > > I don't have much knowledge on generic pinmux conf and suggested Peng to > post the RFC to start the discussion instead of getting blocked by me during > some internal/private discussions as the main intention for him was > upstreaming the changes. I am against the idea of mixing platform specific > changes the way it is done here but since I didn't have much knowledge on > pinmux conf to suggest/provide any useful feedback I suggested to trigger > this discussion. To use generic pinconf, we still need to extend to use OEM type, our scmi firmware will not support saying bias/pull-up and etc config type, so just as below, we add: diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index 365c4b0ca465..a71721cd321d 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -186,6 +186,16 @@ static const struct pinconf_generic_params dt_params[] = { { "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 }, { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, { "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 }, + { "nxp-mux", PIN_CONFIG_NXP_MUX, 0 }, + { "nxp-conf", PIN_CONFIG_NXP_CONF, 0 }, + { "nxp-daisy-id", PIN_CONFIG_NXP_DAISY_ID, 0 }, + { "nxp-daisy-val", PIN_CONFIG_NXP_DAISY_VAL, 0 }, }; And in dts: + pinctrl_uart1: uart1grp { + txd { + pins = "uart1txd"; + nxp-mux = <0x0>; + nxp-conf = <0x31e>; + }; + + rxd { + pins = "uart1rxd"; + nxp-mux = <0x0>; + nxp-conf = <0x31e>; + }; + }; But the upper will make device tree diverge w/o scmi, because we need support both for i.MX95. I still prefer to use freescale current binding format which would make dts reusable w/o scmi. Thanks, Peng. > > > But I could have misunderstood it, so please correct me! > > +1 > > -- > Regards, > Sudeep
On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > Me: >> it is merely making things more complex and also slower > > bymaking the registers only accessible from this SCMI link. > > This is for safety reason, the pinctrl hardware must be handled > by a system manager entity. So mmio direct access not allowed > from Cortex-A side. Yeah I understood as much. But I don't think that the firmware is really filtering any of the access, it will just poke into any pinctrl register as instructed anyway so what's the point. Just looks like a layer of indirection. But I'm not your system manager, so it's not my decision. > The SCMI firmware is very straightforward, there is no group or > function. > > It just accepts the format as this: > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, > DAISY ID, DAISY_CFG, DAISY_VALUE. > > Similar as linux MMIO format. > > Our i.MX95 platform will support two settings, one with SCMI > firmware, one without SCMI. These two settings will share > the same pinctrl header file. > > And to simplify the scmi firmware design(anyway I am not owner > of the firmware), to make pinctrl header shared w/o scmi, > we take the current in-upstream freescale imx binding format. The SCMI people will have to state their position on this. Like what they consider conformance and what extensions are allowed. This is more a standardization question than an implementation question so it's not really my turf. I was under the impression that the ambition with SCMI firmware was to abstract away and hide aspects of the hardware behind a consistent API. This approach drives a truck through that idea. Yours, Linus Walleij
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > Me: > > >> it is merely making things more complex and also slower > > > bymaking the registers only accessible from this SCMI link. > > > > This is for safety reason, the pinctrl hardware must be handled by a > > system manager entity. So mmio direct access not allowed from Cortex-A > > side. > > Yeah I understood as much. But I don't think that the firmware is really > filtering any of the access, it will just poke into any pinctrl register as > instructed anyway so what's the point. Just looks like a layer of indirection. No, the firmware has a check on whether a pin is allowed to be configured by the agent that wanna to configure the pin. > But I'm not your system manager, so it's not my decision. > > > The SCMI firmware is very straightforward, there is no group or > > function. > > > > It just accepts the format as this: > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID, > > DAISY_CFG, DAISY_VALUE. > > > > Similar as linux MMIO format. > > > > Our i.MX95 platform will support two settings, one with SCMI firmware, > > one without SCMI. These two settings will share the same pinctrl > > header file. > > > > And to simplify the scmi firmware design(anyway I am not owner of the > > firmware), to make pinctrl header shared w/o scmi, we take the current > > in-upstream freescale imx binding format. > > The SCMI people will have to state their position on this. > Like what they consider conformance and what extensions are allowed. This > is more a standardization question than an implementation question so it's > not really my turf. The i.MX95 SCMI firmware uses OEM extension type. So I just follow what the firmware did and support it in linux. Anyway let's wait Sudeep's reply. Thanks, Peng. > > I was under the impression that the ambition with SCMI firmware was to > abstract away and hide aspects of the hardware behind a consistent API. > This approach drives a truck through that idea. > > Yours, > Linus Walleij
On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote: > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > > Me: Hi Peng, > > > > >> it is merely making things more complex and also slower > > > > bymaking the registers only accessible from this SCMI link. > > > > > > This is for safety reason, the pinctrl hardware must be handled by a > > > system manager entity. So mmio direct access not allowed from Cortex-A > > > side. > > > > Yeah I understood as much. But I don't think that the firmware is really > > filtering any of the access, it will just poke into any pinctrl register as > > instructed anyway so what's the point. Just looks like a layer of indirection. > > No, the firmware has a check on whether a pin is allowed to be configured > by the agent that wanna to configure the pin. > > > But I'm not your system manager, so it's not my decision. > > > > > The SCMI firmware is very straightforward, there is no group or > > > function. > > > > > > It just accepts the format as this: > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID, > > > DAISY_CFG, DAISY_VALUE. > > > > > > Similar as linux MMIO format. > > > > > > Our i.MX95 platform will support two settings, one with SCMI firmware, > > > one without SCMI. These two settings will share the same pinctrl > > > header file. > > > > > > And to simplify the scmi firmware design(anyway I am not owner of the > > > firmware), to make pinctrl header shared w/o scmi, we take the current > > > in-upstream freescale imx binding format. > > > > The SCMI people will have to state their position on this. > > Like what they consider conformance and what extensions are allowed. This > > is more a standardization question than an implementation question so it's > > not really my turf. > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow > what the firmware did and support it in linux. Anyway let's > wait Sudeep's reply. > So my unsderstanding on this matter as of now is that: 1. the current SCMI Pinctrl specification can support your usecase by using OEM Types and multiple pins/values CONFIG_GET/SET commands 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c) is equally fine and can support your usecase, AFTER Oleksii fixes it to align it to the latest v3.2-BETA2 specification changes. IOW, this means that, using the SCMI Pinctrl protocol operations exposed in scmi_protocol.h, from somewhere, you are able to properly configure multiple pins/values with your specific OEM types. 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol operations is instead NOT suitable for your usecase since it uses the Linux Generic Pinconf and IMX does not make use of it, and instead IMX has its own bindings and related parsing logic. Am I right ? If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing, also because you'll end-up NOT using most of the generic SCMI Pinctrl driver but just reusing a bit of the probe (customized with your own DT maps parsing) Instead, given that the spec[1.] and the protocol layer[2.] are fine for your use case and you indeed have already a custom way to parse your DT mappings, I would say that you could just write your own custom SCMI driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic one, that does its own IMX DT parsing and calls just the SCMI protocol operations that it needs in the way that your platform expects: so basically another Pinctrl SCMI driver that does not use the generic pinconf DT configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its exposed protocol operations...) Not sure what Sudeep thinks about supporting multiple SCMI driver for the same protocol (we did it already for Sensors hwmon && iio), and if this approach won't need some sort of mutual exclusion mechanism in Kconfig to avoid loading both the generic and the custom IMX (even though they should be able to co-exist from the SCMI kernel/fw stack pint of view, as long as you dont mess-up the DTs and mixup generic pins with custom IMX pins...) Instead, adding an IMX-custom extension to what it was supposed to be a generic driver (as you propose) seems to me like a stretch of the generic Pinctrl driver that is not really worth, since you'll end up polluting the generic driver with some highly custom and specific IMX bits. while really NOT reusing so much of the generic driver at all. Thanks, Cristian
Hi Cristian, > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote: > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > > > Me: > > Hi Peng, > > > > > > > >> it is merely making things more complex and also slower > > > > > bymaking the registers only accessible from this SCMI link. > > > > > > > > This is for safety reason, the pinctrl hardware must be handled by > > > > a system manager entity. So mmio direct access not allowed from > > > > Cortex-A side. > > > > > > Yeah I understood as much. But I don't think that the firmware is > > > really filtering any of the access, it will just poke into any > > > pinctrl register as instructed anyway so what's the point. Just looks like a > layer of indirection. > > > > No, the firmware has a check on whether a pin is allowed to be > > configured by the agent that wanna to configure the pin. > > > > > But I'm not your system manager, so it's not my decision. > > > > > > > The SCMI firmware is very straightforward, there is no group or > > > > function. > > > > > > > > It just accepts the format as this: > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY > ID, > > > > DAISY_CFG, DAISY_VALUE. > > > > > > > > Similar as linux MMIO format. > > > > > > > > Our i.MX95 platform will support two settings, one with SCMI > > > > firmware, one without SCMI. These two settings will share the same > > > > pinctrl header file. > > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of > > > > the firmware), to make pinctrl header shared w/o scmi, we take the > > > > current in-upstream freescale imx binding format. > > > > > > The SCMI people will have to state their position on this. > > > Like what they consider conformance and what extensions are allowed. > > > This is more a standardization question than an implementation > > > question so it's not really my turf. > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow > > what the firmware did and support it in linux. Anyway let's wait > > Sudeep's reply. > > > > So my unsderstanding on this matter as of now is that: > > 1. the current SCMI Pinctrl specification can support your usecase by using > OEM Types and multiple pins/values CONFIG_GET/SET commands Yes, based on the Oleksii patchset with my local multiple configs support. > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c) > is equally fine and can support your usecase, AFTER Oleksii fixes it to > align it to the latest v3.2-BETA2 specification changes. > IOW, this means that, using the SCMI Pinctrl protocol operations > exposed in scmi_protocol.h, from somewhere, you are able to properly > configure multiple pins/values with your specific OEM types. Yes. > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol > operations is instead NOT suitable for your usecase since it uses the Linux > Generic Pinconf and IMX does not make use of it, and instead IMX has > its own bindings and related parsing logic. Yes. > > Am I right ? You are right. > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing, > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver > but just reusing a bit of the probe (customized with your own DT maps > parsing) Only DT map to parse the dts and map to config array. Others are same, so need to export some symbols for pinctrl-scmi-imx.c driver if build imx scmi driver. > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your > use case and you indeed have already a custom way to parse your DT > mappings, I would say that you could just write your own custom SCMI > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic > one, that does its own IMX DT parsing and calls just the SCMI protocol > operations that it needs in the way that your platform expects: so basically > another Pinctrl SCMI driver that does not use the generic pinconf DT > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its > exposed protocol operations...) I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19, because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both driver are built in kernel image. > > Not sure what Sudeep thinks about supporting multiple SCMI driver for the > same protocol (we did it already for Sensors hwmon && iio), and if this > approach won't need some sort of mutual exclusion mechanism in Kconfig > to avoid loading both the generic and the custom IMX (even though they > should be able to co-exist from the SCMI kernel/fw stack pint of view, as > long as you dont mess-up the DTs and mixup generic pins with custom IMX > pins...) > > Instead, adding an IMX-custom extension to what it was supposed to be a > generic driver (as you propose) seems to me like a stretch of the generic > Pinctrl driver that is not really worth, since you'll end up polluting the > generic driver with some highly custom and specific IMX bits. while really > NOT reusing so much of the generic driver at all. If Sudeep agree, I will follow your suggestion and post a patch for review, later. Thanks, Peng. > > Thanks, > Cristian
On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote: > Hi Cristian, > Hi, > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote: > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > > > > Me: > > > > Hi Peng, > > > > > > > > > > >> it is merely making things more complex and also slower > > > > > > bymaking the registers only accessible from this SCMI link. > > > > > > > > > > This is for safety reason, the pinctrl hardware must be handled by > > > > > a system manager entity. So mmio direct access not allowed from > > > > > Cortex-A side. > > > > > > > > Yeah I understood as much. But I don't think that the firmware is > > > > really filtering any of the access, it will just poke into any > > > > pinctrl register as instructed anyway so what's the point. Just looks like a > > layer of indirection. > > > > > > No, the firmware has a check on whether a pin is allowed to be > > > configured by the agent that wanna to configure the pin. > > > > > > > But I'm not your system manager, so it's not my decision. > > > > > > > > > The SCMI firmware is very straightforward, there is no group or > > > > > function. > > > > > > > > > > It just accepts the format as this: > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY > > ID, > > > > > DAISY_CFG, DAISY_VALUE. > > > > > > > > > > Similar as linux MMIO format. > > > > > > > > > > Our i.MX95 platform will support two settings, one with SCMI > > > > > firmware, one without SCMI. These two settings will share the same > > > > > pinctrl header file. > > > > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the > > > > > current in-upstream freescale imx binding format. > > > > > > > > The SCMI people will have to state their position on this. > > > > Like what they consider conformance and what extensions are allowed. > > > > This is more a standardization question than an implementation > > > > question so it's not really my turf. > > > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow > > > what the firmware did and support it in linux. Anyway let's wait > > > Sudeep's reply. > > > > > > > So my unsderstanding on this matter as of now is that: > > > > 1. the current SCMI Pinctrl specification can support your usecase by using > > OEM Types and multiple pins/values CONFIG_GET/SET commands > > Yes, based on the Oleksii patchset with my local multiple configs support. > Yes, I know, I pointed out on his series that the protocol has still to be fixed to be aligned with the latest BETA2 spec (we changed the spec on the fly while he was already posting indeed..) > > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c) > > is equally fine and can support your usecase, AFTER Oleksii fixes it to > > align it to the latest v3.2-BETA2 specification changes. > > IOW, this means that, using the SCMI Pinctrl protocol operations > > exposed in scmi_protocol.h, from somewhere, you are able to properly > > configure multiple pins/values with your specific OEM types. > > Yes. Good. > > > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol > > operations is instead NOT suitable for your usecase since it uses the Linux > > Generic Pinconf and IMX does not make use of it, and instead IMX has > > its own bindings and related parsing logic. > > Yes. > > > > > Am I right ? > > You are right. > > > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing, > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver > > but just reusing a bit of the probe (customized with your own DT maps > > parsing) > > Only DT map to parse the dts and map to config array. Others are same, > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx > scmi driver. > Yes, but you are basically using some exported symbol to parse the DT in your way and then you do not use anything of the various functions/groups stuff...you just leverage some of the probing stuff and then issue you OEM Type configs....I mean most of the picntrl-scmi driver would be unused anyway in this scenario. > > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your > > use case and you indeed have already a custom way to parse your DT > > mappings, I would say that you could just write your own custom SCMI > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic > > one, that does its own IMX DT parsing and calls just the SCMI protocol > > operations that it needs in the way that your platform expects: so basically > > another Pinctrl SCMI driver that does not use the generic pinconf DT > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its > > exposed protocol operations...) > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19, > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both > driver are built in kernel image. > Ok here I lost you. The protocol ID 0x19 is bound to the protocol layer and identifies the standard Pinctrl protocol: usually you use a 0x99 to define and describe you own specific NEW vendor protocol, BUT here you are saying you are fine to use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so I dont see why you should use a new vendor protocol_id to basically expose the same operations. (and I also dont see how you can do that without hacks in the current codebase) You CAN have multiple SCMI drivers using the same protocol at the same time (even more than one protocol at the same time), even though we try to avoid it if there are no good reason to have more than one driver, there is nothing in the spec or in the current SCMI platform or agent stacks that inhibits such scenario (and I use iot heavily for my offline testing indeed.) Look at: - drivers/hwmon/scmi-hwmon - drivers/iio/common/scmi_sensors/scmi_iio.c and you'll see that these 2 drivers uses the same SENSOR protocol, just for different sensor types so they do not interfere one with each other. What happens is that the first driver using a protocol causes its protocol_init to be called once for all. This should work flawlessly like this, if this is not the case for some reason, this will have to be fixed in the protocol implementation: you are supposed to be able to grab the same protocol from different drivers without any issue. I agree that you have to be careful not to share the same pins across 2 different drivers using the same Pinctrl driver, but even if both driver are compiled in, nothing is really happening until the related DT binding are parsed, and so unless you mismatch your DT and assign same pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont see how they can interfere. You could indeed, have a set of pins managed by your custom IMX driver and one distinct other set of pins handled by the SCMI Generic driver by Oleksii, both magically handled by the same SCMI Server backend :P ! BUT to be on the safe side you could anyway force a conflict in Kconfig to mutually exclude one driver when the other is built and vice-versa. Am I missing something ? Why would you need a new vendor ID to define a new protocol without not really having any new protocol ? Thanks, Cristian
On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote: > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote: > > Hi Cristian, > > > > Hi, > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote: > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > > > > > Me: > > > > > > Hi Peng, > > > > > > > > > > > > > >> it is merely making things more complex and also slower > > > > > > > bymaking the registers only accessible from this SCMI link. > > > > > > > > > > > > This is for safety reason, the pinctrl hardware must be handled by > > > > > > a system manager entity. So mmio direct access not allowed from > > > > > > Cortex-A side. > > > > > > > > > > Yeah I understood as much. But I don't think that the firmware is > > > > > really filtering any of the access, it will just poke into any > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a > > > layer of indirection. > > > > > > > > No, the firmware has a check on whether a pin is allowed to be > > > > configured by the agent that wanna to configure the pin. > > > > > > > > > But I'm not your system manager, so it's not my decision. > > > > > > > > > > > The SCMI firmware is very straightforward, there is no group or > > > > > > function. > > > > > > > > > > > > It just accepts the format as this: > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY > > > ID, > > > > > > DAISY_CFG, DAISY_VALUE. > > > > > > > > > > > > Similar as linux MMIO format. > > > > > > > > > > > > Our i.MX95 platform will support two settings, one with SCMI > > > > > > firmware, one without SCMI. These two settings will share the same > > > > > > pinctrl header file. > > > > > > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the > > > > > > current in-upstream freescale imx binding format. > > > > > > > > > > The SCMI people will have to state their position on this. > > > > > Like what they consider conformance and what extensions are allowed. > > > > > This is more a standardization question than an implementation > > > > > question so it's not really my turf. > > > > > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow > > > > what the firmware did and support it in linux. Anyway let's wait > > > > Sudeep's reply. > > > > > > > > > > So my unsderstanding on this matter as of now is that: > > > > > > 1. the current SCMI Pinctrl specification can support your usecase by using > > > OEM Types and multiple pins/values CONFIG_GET/SET commands > > > > Yes, based on the Oleksii patchset with my local multiple configs support. > > > > Yes, I know, I pointed out on his series that the protocol has still to > be fixed to be aligned with the latest BETA2 spec (we changed the spec > on the fly while he was already posting indeed..) > > > > > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c) > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to > > > align it to the latest v3.2-BETA2 specification changes. > > > IOW, this means that, using the SCMI Pinctrl protocol operations > > > exposed in scmi_protocol.h, from somewhere, you are able to properly > > > configure multiple pins/values with your specific OEM types. > > > > Yes. > > Good. > > > > > > > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol > > > operations is instead NOT suitable for your usecase since it uses the Linux > > > Generic Pinconf and IMX does not make use of it, and instead IMX has > > > its own bindings and related parsing logic. > > > > Yes. > > > > > > > > Am I right ? > > > > You are right. > > > > > > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing, > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver > > > but just reusing a bit of the probe (customized with your own DT maps > > > parsing) > > > > Only DT map to parse the dts and map to config array. Others are same, > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx > > scmi driver. > > > > Yes, but you are basically using some exported symbol to parse the DT in > your way and then you do not use anything of the various > functions/groups stuff...you just leverage some of the probing stuff and > then issue you OEM Type configs....I mean most of the picntrl-scmi > driver would be unused anyway in this scenario. > > > > > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your > > > use case and you indeed have already a custom way to parse your DT > > > mappings, I would say that you could just write your own custom SCMI > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic > > > one, that does its own IMX DT parsing and calls just the SCMI protocol > > > operations that it needs in the way that your platform expects: so basically > > > another Pinctrl SCMI driver that does not use the generic pinconf DT > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its > > > exposed protocol operations...) > > > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19, > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both > > driver are built in kernel image. > > > > Ok here I lost you. > > The protocol ID 0x19 is bound to the protocol layer and identifies the > standard Pinctrl protocol: usually you use a 0x99 to define and describe > you own specific NEW vendor protocol, BUT here you are saying you are fine to > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so > I dont see why you should use a new vendor protocol_id to basically > expose the same operations. (and I also dont see how you can do that > without hacks in the current codebase) > > You CAN have multiple SCMI drivers using the same protocol at the same > time (even more than one protocol at the same time), even though we try > to avoid it if there are no good reason to have more than one driver, there > is nothing in the spec or in the current SCMI platform or agent stacks that > inhibits such scenario (and I use iot heavily for my offline testing > indeed.) > > Look at: > > - drivers/hwmon/scmi-hwmon > - drivers/iio/common/scmi_sensors/scmi_iio.c > > and you'll see that these 2 drivers uses the same SENSOR protocol, just for > different sensor types so they do not interfere one with each other. Then, how are those two devices identified in a device tree? That is the point in Peng's case and why he wants to have a dedicated protocol id (I don't agree to this, though.) If we follow Cristian's idea, we may want to have two dt nodes, say pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device nodes to refer to pins, respectively. I think there is currently no mechanism (or binding?) to allow this except adding a protocol id. -Takahiro Akashi > What happens is that the first driver using a protocol causes its > protocol_init to be called once for all. > > This should work flawlessly like this, if this is not the case for some > reason, this will have to be fixed in the protocol implementation: you > are supposed to be able to grab the same protocol from different > drivers without any issue. > > I agree that you have to be careful not to share the same pins across 2 > different drivers using the same Pinctrl driver, but even if both driver > are compiled in, nothing is really happening until the related DT > binding are parsed, and so unless you mismatch your DT and assign same > pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont > see how they can interfere. You could indeed, have a set of pins managed > by your custom IMX driver and one distinct other set of pins handled by > the SCMI Generic driver by Oleksii, both magically handled by the same > SCMI Server backend :P ! > > BUT to be on the safe side you could anyway force a conflict in Kconfig > to mutually exclude one driver when the other is built and vice-versa. > > Am I missing something ? Why would you need a new vendor ID to define a > new protocol without not really having any new protocol ? > > Thanks, > Cristian
On Wed, Sep 06, 2023 at 02:43:46PM +0900, AKASHI Takahiro wrote: > On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote: > > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote: > > > Hi Cristian, > > > > > > > Hi, > > Hi, > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote: > > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > > > > > > Me: > > > > > > > > Hi Peng, > > > > > > > > > > > > > > > > >> it is merely making things more complex and also slower > > > > > > > > bymaking the registers only accessible from this SCMI link. > > > > > > > > > > > > > > This is for safety reason, the pinctrl hardware must be handled by > > > > > > > a system manager entity. So mmio direct access not allowed from > > > > > > > Cortex-A side. > > > > > > > > > > > > Yeah I understood as much. But I don't think that the firmware is > > > > > > really filtering any of the access, it will just poke into any > > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a > > > > layer of indirection. > > > > > > > > > > No, the firmware has a check on whether a pin is allowed to be > > > > > configured by the agent that wanna to configure the pin. > > > > > > > > > > > But I'm not your system manager, so it's not my decision. > > > > > > > > > > > > > The SCMI firmware is very straightforward, there is no group or > > > > > > > function. > > > > > > > > > > > > > > It just accepts the format as this: > > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY > > > > ID, > > > > > > > DAISY_CFG, DAISY_VALUE. > > > > > > > > > > > > > > Similar as linux MMIO format. > > > > > > > > > > > > > > Our i.MX95 platform will support two settings, one with SCMI > > > > > > > firmware, one without SCMI. These two settings will share the same > > > > > > > pinctrl header file. > > > > > > > > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of > > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the > > > > > > > current in-upstream freescale imx binding format. > > > > > > > > > > > > The SCMI people will have to state their position on this. > > > > > > Like what they consider conformance and what extensions are allowed. > > > > > > This is more a standardization question than an implementation > > > > > > question so it's not really my turf. > > > > > > > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow > > > > > what the firmware did and support it in linux. Anyway let's wait > > > > > Sudeep's reply. > > > > > > > > > > > > > So my unsderstanding on this matter as of now is that: > > > > > > > > 1. the current SCMI Pinctrl specification can support your usecase by using > > > > OEM Types and multiple pins/values CONFIG_GET/SET commands > > > > > > Yes, based on the Oleksii patchset with my local multiple configs support. > > > > > > > Yes, I know, I pointed out on his series that the protocol has still to > > be fixed to be aligned with the latest BETA2 spec (we changed the spec > > on the fly while he was already posting indeed..) > > > > > > > > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c) > > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to > > > > align it to the latest v3.2-BETA2 specification changes. > > > > IOW, this means that, using the SCMI Pinctrl protocol operations > > > > exposed in scmi_protocol.h, from somewhere, you are able to properly > > > > configure multiple pins/values with your specific OEM types. > > > > > > Yes. > > > > Good. > > > > > > > > > > > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol > > > > operations is instead NOT suitable for your usecase since it uses the Linux > > > > Generic Pinconf and IMX does not make use of it, and instead IMX has > > > > its own bindings and related parsing logic. > > > > > > Yes. > > > > > > > > > > > Am I right ? > > > > > > You are right. > > > > > > > > > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic > > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing, > > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver > > > > but just reusing a bit of the probe (customized with your own DT maps > > > > parsing) > > > > > > Only DT map to parse the dts and map to config array. Others are same, > > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx > > > scmi driver. > > > > > > > Yes, but you are basically using some exported symbol to parse the DT in > > your way and then you do not use anything of the various > > functions/groups stuff...you just leverage some of the probing stuff and > > then issue you OEM Type configs....I mean most of the picntrl-scmi > > driver would be unused anyway in this scenario. > > > > > > > > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your > > > > use case and you indeed have already a custom way to parse your DT > > > > mappings, I would say that you could just write your own custom SCMI > > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic > > > > one, that does its own IMX DT parsing and calls just the SCMI protocol > > > > operations that it needs in the way that your platform expects: so basically > > > > another Pinctrl SCMI driver that does not use the generic pinconf DT > > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its > > > > exposed protocol operations...) > > > > > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19, > > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse > > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both > > > driver are built in kernel image. > > > > > > > Ok here I lost you. > > > > The protocol ID 0x19 is bound to the protocol layer and identifies the > > standard Pinctrl protocol: usually you use a 0x99 to define and describe > > you own specific NEW vendor protocol, BUT here you are saying you are fine to > > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so > > I dont see why you should use a new vendor protocol_id to basically > > expose the same operations. (and I also dont see how you can do that > > without hacks in the current codebase) > > > > You CAN have multiple SCMI drivers using the same protocol at the same > > time (even more than one protocol at the same time), even though we try > > to avoid it if there are no good reason to have more than one driver, there > > is nothing in the spec or in the current SCMI platform or agent stacks that > > inhibits such scenario (and I use iot heavily for my offline testing > > indeed.) > > > > Look at: > > > > - drivers/hwmon/scmi-hwmon > > - drivers/iio/common/scmi_sensors/scmi_iio.c > > > > and you'll see that these 2 drivers uses the same SENSOR protocol, just for > > different sensor types so they do not interfere one with each other. > > Then, how are those two devices identified in a device tree? At SCMI probe time the SCMI core stack creates one device for each 'protocol_id/name' as provided by the registered SCMI drivers in their respective scmi_device_id/id_tableS, as long as the requested protocol is active in the DT (protocol@XX defined) and the 'name' is not duplicated; each of these devices, sharing the same SCMI protocol, are created sharing also the same DT node protocol@XX. Their respective SCMI drivers are subsequently separately matched on protocol_id/name and then probed as usual: it is up to the drivers not to step on each other feet by competing for the same SCMI resources... ...like issuing comflicting request for the same reosurce domain on the same protocol. I suppose, though, a lot depends on how the respective drivers interact with their subsystems, like IIO SCMI does not really need any info from DT, and the hwmon uses just the phandle to pick the sensor_domain_id to associate, as an example, to thermal sensors. > That is the point in Peng's case and why he wants to have a dedicated > protocol id (I don't agree to this, though.) > If we follow Cristian's idea, we may want to have two dt nodes, say > pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device > nodes to refer to pins, respectively. > I think there is currently no mechanism (or binding?) to allow this > except adding a protocol id. > Beside the uglyness of having 2 different DT bindings schemas for 2 different drivers coexisting in the same parent protocol node, it is still not clear to me why this cant be done technically without the need of a new dummy 0x99 protocol_node, given that each driver can use its own parsing logic in its probe, which is what Peng is doing in dt_node_to_map indeed ...even though, maybe, the result would be so ugly and/or not accepatble from bindings point of view that we will not want to do it at the end anyway....I mean maybe it is just the attempt itself to combine the Generic Pinctrl approach with the highly specific IMX approach that inevitably leads to these clashes... ...I maybe missing something, though, so I'll made some experiments on my side about this before I'll keep on blabbing :P Thanks, Cristian
On Thu, Sep 07, 2023 at 11:05:52AM +0100, Cristian Marussi wrote: > On Wed, Sep 06, 2023 at 02:43:46PM +0900, AKASHI Takahiro wrote: > > On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote: > > > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote: > > > > Hi Cristian, > > > > > > > > > > Hi, > > > > > Hi, > > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > > > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote: > > > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9 > > > > > > > > > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote: > > > > > > > > Me: > > > > > > > > > > Hi Peng, > > > > > > > > > > > > > > > > > > > >> it is merely making things more complex and also slower > > > > > > > > > bymaking the registers only accessible from this SCMI link. > > > > > > > > > > > > > > > > This is for safety reason, the pinctrl hardware must be handled by > > > > > > > > a system manager entity. So mmio direct access not allowed from > > > > > > > > Cortex-A side. > > > > > > > > > > > > > > Yeah I understood as much. But I don't think that the firmware is > > > > > > > really filtering any of the access, it will just poke into any > > > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a > > > > > layer of indirection. > > > > > > > > > > > > No, the firmware has a check on whether a pin is allowed to be > > > > > > configured by the agent that wanna to configure the pin. > > > > > > > > > > > > > But I'm not your system manager, so it's not my decision. > > > > > > > > > > > > > > > The SCMI firmware is very straightforward, there is no group or > > > > > > > > function. > > > > > > > > > > > > > > > > It just accepts the format as this: > > > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY > > > > > ID, > > > > > > > > DAISY_CFG, DAISY_VALUE. > > > > > > > > > > > > > > > > Similar as linux MMIO format. > > > > > > > > > > > > > > > > Our i.MX95 platform will support two settings, one with SCMI > > > > > > > > firmware, one without SCMI. These two settings will share the same > > > > > > > > pinctrl header file. > > > > > > > > > > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of > > > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the > > > > > > > > current in-upstream freescale imx binding format. > > > > > > > > > > > > > > The SCMI people will have to state their position on this. > > > > > > > Like what they consider conformance and what extensions are allowed. > > > > > > > This is more a standardization question than an implementation > > > > > > > question so it's not really my turf. > > > > > > > > > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow > > > > > > what the firmware did and support it in linux. Anyway let's wait > > > > > > Sudeep's reply. > > > > > > > > > > > > > > > > So my unsderstanding on this matter as of now is that: > > > > > > > > > > 1. the current SCMI Pinctrl specification can support your usecase by using > > > > > OEM Types and multiple pins/values CONFIG_GET/SET commands > > > > > > > > Yes, based on the Oleksii patchset with my local multiple configs support. > > > > > > > > > > Yes, I know, I pointed out on his series that the protocol has still to > > > be fixed to be aligned with the latest BETA2 spec (we changed the spec > > > on the fly while he was already posting indeed..) > > > > > > > > > > > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c) > > > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to > > > > > align it to the latest v3.2-BETA2 specification changes. > > > > > IOW, this means that, using the SCMI Pinctrl protocol operations > > > > > exposed in scmi_protocol.h, from somewhere, you are able to properly > > > > > configure multiple pins/values with your specific OEM types. > > > > > > > > Yes. > > > > > > Good. > > > > > > > > > > > > > > > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol > > > > > operations is instead NOT suitable for your usecase since it uses the Linux > > > > > Generic Pinconf and IMX does not make use of it, and instead IMX has > > > > > its own bindings and related parsing logic. > > > > > > > > Yes. > > > > > > > > > > > > > > Am I right ? > > > > > > > > You are right. > > > > > > > > > > > > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic > > > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing, > > > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver > > > > > but just reusing a bit of the probe (customized with your own DT maps > > > > > parsing) > > > > > > > > Only DT map to parse the dts and map to config array. Others are same, > > > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx > > > > scmi driver. > > > > > > > > > > Yes, but you are basically using some exported symbol to parse the DT in > > > your way and then you do not use anything of the various > > > functions/groups stuff...you just leverage some of the probing stuff and > > > then issue you OEM Type configs....I mean most of the picntrl-scmi > > > driver would be unused anyway in this scenario. > > > > > > > > > > > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your > > > > > use case and you indeed have already a custom way to parse your DT > > > > > mappings, I would say that you could just write your own custom SCMI > > > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic > > > > > one, that does its own IMX DT parsing and calls just the SCMI protocol > > > > > operations that it needs in the way that your platform expects: so basically > > > > > another Pinctrl SCMI driver that does not use the generic pinconf DT > > > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its > > > > > exposed protocol operations...) > > > > > > > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19, > > > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse > > > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both > > > > driver are built in kernel image. > > > > > > > > > > Ok here I lost you. > > > > > > The protocol ID 0x19 is bound to the protocol layer and identifies the > > > standard Pinctrl protocol: usually you use a 0x99 to define and describe > > > you own specific NEW vendor protocol, BUT here you are saying you are fine to > > > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so > > > I dont see why you should use a new vendor protocol_id to basically > > > expose the same operations. (and I also dont see how you can do that > > > without hacks in the current codebase) > > > > > > You CAN have multiple SCMI drivers using the same protocol at the same > > > time (even more than one protocol at the same time), even though we try > > > to avoid it if there are no good reason to have more than one driver, there > > > is nothing in the spec or in the current SCMI platform or agent stacks that > > > inhibits such scenario (and I use iot heavily for my offline testing > > > indeed.) > > > > > > Look at: > > > > > > - drivers/hwmon/scmi-hwmon > > > - drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > and you'll see that these 2 drivers uses the same SENSOR protocol, just for > > > different sensor types so they do not interfere one with each other. > > > > Then, how are those two devices identified in a device tree? > > At SCMI probe time the SCMI core stack creates one device for each > 'protocol_id/name' as provided by the registered SCMI drivers in > their respective scmi_device_id/id_tableS, as long as the requested > protocol is active in the DT (protocol@XX defined) and the 'name' > is not duplicated; each of these devices, sharing the same SCMI > protocol, are created sharing also the same DT node protocol@XX. That's fine. > Their respective SCMI drivers are subsequently separately matched on > protocol_id/name and then probed as usual: it is up to the drivers not > to step on each other feet by competing for the same SCMI resources... > ...like issuing comflicting request for the same reosurce domain > on the same protocol. That's fine. My question/idea is simple; how two pinctrl drivers, if possible, be represented in a single device tree. To allow the case of Peng Fan, for example, I suppose that we need some DT binding like: scmi { scmi_pinctrl: protocol@19 { compatible = "arm, scmi-pinctrl-generic"; // not sure if needed pinmux ... // standard binding ... } } scmi_pinctrl_fsl { compatible = "fsl,scmi-pinctrl-imx9"; fsl,pinmux = <&scmi_pinctrl, ...>, <&scmi_pinctrl, ...>; ... } "scmi_pinctrl_fsl" driver may reuse generic SCMI pinctrl helper functions, except dt_node_to_map, in pictrl_ops. -Takahiro Akashi > I suppose, though, a lot depends on how the respective drivers interact > with their subsystems, like IIO SCMI does not really need any info > from DT, and the hwmon uses just the phandle to pick the sensor_domain_id > to associate, as an example, to thermal sensors. > > > That is the point in Peng's case and why he wants to have a dedicated > > protocol id (I don't agree to this, though.) > > If we follow Cristian's idea, we may want to have two dt nodes, say > > pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device > > nodes to refer to pins, respectively. > > I think there is currently no mechanism (or binding?) to allow this > > except adding a protocol id. > > > > Beside the uglyness of having 2 different DT bindings schemas for 2 > different drivers coexisting in the same parent protocol node, it is > still not clear to me why this cant be done technically without the need > of a new dummy 0x99 protocol_node, given that each driver can use its > own parsing logic in its probe, which is what Peng is doing in > dt_node_to_map indeed ...even though, maybe, the result would be so > ugly and/or not accepatble from bindings point of view that we will > not want to do it at the end anyway....I mean maybe it is just the > attempt itself to combine the Generic Pinctrl approach with the highly > specific IMX approach that inevitably leads to these clashes... > > ...I maybe missing something, though, so I'll made some experiments on > my side about this before I'll keep on blabbing :P > > Thanks, > Cristian
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c index 868a2f9821be..93109997614d 100644 --- a/drivers/firmware/arm_scmi/pinctrl.c +++ b/drivers/firmware/arm_scmi/pinctrl.c @@ -6,13 +6,14 @@ */ #include <linux/module.h> +#include <linux/pinctrl/pinconf-generic.h> #include <linux/scmi_protocol.h> #include <linux/slab.h> #include "protocols.h" -#define REG_TYPE_BITS GENMASK(9, 8) -#define REG_CONFIG GENMASK(7, 0) +#define REG_TYPE_CONFIG GENMASK(1, 0) +#define REG_NUM_CONFIG GENMASK(9, 2) #define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16)) #define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0)) @@ -36,10 +37,11 @@ enum scmi_pinctrl_protocol_cmd { PINCTRL_SET_PERMISSIONS = 0xb }; +#define MAX_CONFIG_ENTRY 10 struct scmi_msg_conf_set { __le32 identifier; __le32 attributes; - __le32 config_value; + __le32 config_value[MAX_CONFIG_ENTRY * 2]; }; struct scmi_msg_conf_get { @@ -320,9 +322,11 @@ static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph, tx = t->tx.buf; tx->identifier = cpu_to_le32(selector); + /* attributes = FIELD_PREP(REG_TYPE_BITS, type) | FIELD_PREP(REG_CONFIG, config_type); tx->attributes = cpu_to_le32(attributes); + */ ret = ph->xops->do_xfer(ph, t); if (!ret) @@ -335,7 +339,7 @@ static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph, static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph, u32 selector, enum scmi_pinctrl_selector_type type, - u8 config_type, unsigned long config_value) + unsigned long *configs, unsigned int num_configs) { struct scmi_xfer *t; struct scmi_msg_conf_set *tx; @@ -355,10 +359,14 @@ static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph, tx = t->tx.buf; tx->identifier = cpu_to_le32(selector); - attributes = FIELD_PREP(REG_TYPE_BITS, type) | - FIELD_PREP(REG_CONFIG, config_type); + for (int i = 0; i < num_configs; i++) { + tx->config_value[i * 2] = cpu_to_le32(pinconf_to_config_param(configs[i])); + tx->config_value[i * 2 + 1] = cpu_to_le32(pinconf_to_config_argument(configs[i])); + } + + attributes = FIELD_PREP(REG_TYPE_CONFIG, type) | + FIELD_PREP(REG_NUM_CONFIG, num_configs); tx->attributes = cpu_to_le32(attributes); - tx->config_value = cpu_to_le32(config_value); ret = ph->xops->do_xfer(ph, t); diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 9bc16943014f..dd0a66a17184 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -14,6 +14,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/pinctrl/pinconf-generic.h> #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/seq_file.h> @@ -33,6 +34,29 @@ #define IMX_NO_PAD_CTL 0x80000000 /* no pin config need */ #define IMX_PAD_SION 0x40000000 /* set SION */ +/* + * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID + * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin. + * For generic_pinconf case, there's no extra u32 CONFIG. + * + * PIN_FUNC_ID format: + * Default: + * <mux_reg conf_reg input_reg mux_mode input_val> + * SHARE_MUX_CONF_REG: + * <mux_conf_reg input_reg mux_mode input_val> + * IMX_USE_SCU: + * <pin_id mux_mode> + */ +#define FSL_PIN_SIZE 24 +#define FSL_PIN_SHARE_SIZE 20 +#define FSL_SCU_PIN_SIZE 12 + +/* SCMI pin control types, aligned with SCMI firmware */ +#define IMX_PIN_TYPE_MUX 192 +#define IMX_PIN_TYPE_CONFIG 193 +#define IMX_PIN_TYPE_DAISY_ID 194 +#define IMX_PIN_TYPE_DAISY_CFG 195 + static inline const struct group_desc *imx_pinctrl_find_group_by_name( struct pinctrl_dev *pctldev, const char *name) @@ -55,6 +79,96 @@ static void imx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s, seq_printf(s, "%s", dev_name(pctldev->dev)); } +#define IMX_SCMI_NUM_CFG 4 +int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np, + struct pinctrl_map **map, unsigned *num_maps) +{ + struct pinctrl_map *new_map; + const __be32 *list; + unsigned long *configs = NULL; + unsigned long cfg[IMX_SCMI_NUM_CFG]; + int map_num, size, pin_size, pin_id, num_pins; + int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val; + int i, j; + uint32_t ncfg; + static uint32_t daisy_off; + + if (!daisy_off) { + if (of_machine_is_compatible("fsl,imx93")) + daisy_off = 0x360; + else if (of_machine_is_compatible("fsl,imx95")) + daisy_off = 0x408; + else + dev_err(pctldev->dev, "platform not support scmi pinctrl\n"); + } + + list = of_get_property(np, "fsl,pins", &size); + if (!list) { + dev_err(pctldev->dev, "no fsl,pins property in node %pOF\n", np); + return -EINVAL; + } + + pin_size = FSL_PIN_SIZE; + + if (!size || size % pin_size) { + dev_err(pctldev->dev, "Invalid fsl,pins or pins property in node %pOF\n", np); + return -EINVAL; + } + + num_pins = size / pin_size; + map_num = num_pins; + + new_map = kmalloc_array(map_num, sizeof(struct pinctrl_map), + GFP_KERNEL); + if (!new_map) + return -ENOMEM; + + *map = new_map; + *num_maps = map_num; + + /* create config map */ + for (i = 0; i < num_pins; i++) { + j = 0; + ncfg = IMX_SCMI_NUM_CFG; + mux_reg = be32_to_cpu(*list++); + conf_reg = be32_to_cpu(*list++); + input_reg = be32_to_cpu(*list++); + mux_val = be32_to_cpu(*list++); + input_val = be32_to_cpu(*list++); + conf_val = be32_to_cpu(*list++); + if (conf_val & IMX_PAD_SION) + mux_val |= IOMUXC_CONFIG_SION; + + pin_id = mux_reg / 4; + + cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_MUX, mux_val); + + if (!conf_reg || (conf_val & IMX_NO_PAD_CTL)) { + ncfg--; + } else { + cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_CONFIG, conf_val); + } + + if (!input_reg) { + ncfg -= 2; + } else { + cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_ID, + (input_reg - daisy_off) / 4); + cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_CFG, input_val); + } + + configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL); + + new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN; + new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_id); + new_map[i].data.configs.configs = configs; + new_map[i].data.configs.num_configs = ncfg; + } + + return 0; +} +EXPORT_SYMBOL_GPL(imx_scmi_dt_node_to_map); + static int imx_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np, struct pinctrl_map **map, unsigned *num_maps) @@ -441,22 +555,6 @@ static const struct pinconf_ops imx_pinconf_ops = { .pin_config_group_dbg_show = imx_pinconf_group_dbg_show, }; -/* - * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID - * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin. - * - * PIN_FUNC_ID format: - * Default: - * <mux_reg conf_reg input_reg mux_mode input_val> - * SHARE_MUX_CONF_REG: - * <mux_conf_reg input_reg mux_mode input_val> - * IMX_USE_SCU: - * <pin_id mux_mode> - */ -#define FSL_PIN_SIZE 24 -#define FSL_PIN_SHARE_SIZE 20 -#define FSL_SCU_PIN_SIZE 12 - static void imx_pinctrl_parse_pin_mmio(struct imx_pinctrl *ipctl, unsigned int *pin_id, struct imx_pin *pin, const __be32 **list_p, diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c index a9304402ddf1..92cf10241aa8 100644 --- a/drivers/pinctrl/pinctrl-scmi.c +++ b/drivers/pinctrl/pinctrl-scmi.c @@ -8,6 +8,7 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/seq_file.h> #include <linux/scmi_protocol.h> #include <linux/slab.h> @@ -77,13 +78,28 @@ static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev, } #ifdef CONFIG_OF +#ifdef CONFIG_PINCTRL_IMX +extern int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np, + struct pinctrl_map **map, unsigned *num_maps); +#else +static inline int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, + struct device_node *np, + struct pinctrl_map **map, unsigned *num_maps) +{ + return -ENOTSUPP; +} +#endif static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config, struct pinctrl_map **map, u32 *num_maps) { + if ((of_machine_is_compatible("fsl,imx93") || of_machine_is_compatible("fsl,imx95"))) + return imx_scmi_dt_node_to_map(pctldev, np_config, map, num_maps); + return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps, PIN_MAP_TYPE_INVALID); + } static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map, @@ -242,25 +258,15 @@ static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev, unsigned long *configs, unsigned int num_configs) { - int i, ret; + int ret; struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); - enum pin_config_param config_type; - unsigned long config_value; if (!configs || num_configs == 0) return -EINVAL; - for (i = 0; i < num_configs; i++) { - config_type = pinconf_to_config_param(configs[i]); - config_value = pinconf_to_config_argument(configs[i]); - - ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, config_type, config_value); - if (ret) { - dev_err(pmx->dev, "Error parsing config %ld\n", - configs[i]); - break; - } - } + ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, configs, num_configs); + if (ret) + dev_err(pmx->dev, "Error parsing config %d\n", ret); return ret; } @@ -270,26 +276,15 @@ static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev, unsigned long *configs, unsigned int num_configs) { - int i, ret; + int ret; struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); - enum pin_config_param config_type; - unsigned long config_value; if (!configs || num_configs == 0) return -EINVAL; - for (i = 0; i < num_configs; i++) { - config_type = pinconf_to_config_param(configs[i]); - config_value = pinconf_to_config_argument(configs[i]); - - ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, config_type, - config_value); - if (ret) { - dev_err(pmx->dev, "Error parsing config = %ld", - configs[i]); - break; - } - } + ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, configs, num_configs); + if (ret) + dev_err(pmx->dev, "Error parsing config %d", ret); return ret; }; diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index 99c1405decd7..e58d207b56b6 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h @@ -730,7 +730,7 @@ struct scmi_pinctrl_proto_ops { u8 config_type, unsigned long *config_value); int (*config_set)(const struct scmi_protocol_handle *ph, u32 selector, enum scmi_pinctrl_selector_type type, - u8 config_type, unsigned long config_value); + unsigned long *configs, unsigned int num_configs); int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin); int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin); };