| Message ID | 20240121-pinctrl-scmi-v3-0-8d94ba79dca8@nxp.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support | expand |
Hi Sudeep, Cristian Would you pick up patch 1-4? And for i.MX95 OEM extenstion, do you have any suggestions? I have two points: 1. use vendor compatible. This would also benefit when supporting vendor protocol. 2. Introduce a property saying supporting-generic-pinconf How do you think? Thanks, Peng. 在 1/21/2024 6:21 PM, Peng Fan (OSS) 写道: > This patchset is a rework from Oleksii's RFC v5 patchset > https://lore.kernel.org/all/cover.1698353854.git.oleksii_moisieiev@epam.com/ > > This patchset introduces some changes based on RFC v5: > - introduce helper get_max_msg_size > - support compatible string > - iterate the id_table > - Support multiple configs in one command > - Added i.MX support > - Patch 5 firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support > is almost same as RFCv5 expect multiple configs support. > - Patch 4 the dt-bindings includes compatible string to support i.MX > - Rebased on 2023-12-15 linux-next/master > > If any comments from RFC v5 are missed, I am sorry in advance. > > This PINCTRL Protocol is following Version 3.2 SCMI Spec Beta release. > > On ARM-based systems, a separate Cortex-M based System Control Processor > (SCP) provides control on pins, as well as with power, clocks, reset > controllers. So implement the driver to support such cases. > > The i.MX95 Example as below: > > Configuration: > The scmi-pinctrl driver can be configured using DT bindings. > For example: > / { > sram0: sram@445b1000 { > compatible = "mmio-sram"; > reg = <0x0 0x445b1000 0x0 0x400>; > > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0x0 0x445b1000 0x400>; > > scmi_buf0: scmi-sram-section@0 { > compatible = "arm,scmi-shmem"; > reg = <0x0 0x80>; > }; > > scmi_buf1: scmi-sram-section@80 { > compatible = "arm,scmi-shmem"; > reg = <0x80 0x80>; > }; > }; > > firmware { > scmi { > compatible = "arm,scmi"; > mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>; > shmem = <&scmi_buf0>, <&scmi_buf1>; > #address-cells = <1>; > #size-cells = <0>; > > scmi_iomuxc: protocol@19 { > compatible = "fsl,imx95-scmi-pinctrl"; > reg = <0x19>; > }; > }; > }; > }; > > &scmi_iomuxc { > pinctrl_tpm3: tpm3grp { > fsl,pins = < > IMX95_PAD_GPIO_IO12__TPM3_CH2 0x51e > >; > }; > }; > > This patchset has been tested on i.MX95-19x19-EVK board. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > Changes in v3: > - Add R-b for dt-binding patch > - Use 80 chars per line to align with other scmi drivers > - Add pinctrl_scmi_alloc_configs pinctrl_scmi_free_configs to replace > driver global config_value and config_type array to avoid in parrell > access issue. When num_configs is larger than 4, use alloc, else use > stack. > - Drop the separate MAITAINERS entry for firmware scmi pinctrl > - Use enum type, not u8 when referring the scmi or generic pin conf type > - Drop scmi_pinctrl_config_get_all which is not used at all for now. > - Update copyright year to 2024 > - Move the enum scmi_pinctrl_conf_type above pinctrl_proto_ops for consistency > - Link to v2: https://lore.kernel.org/r/20240104-pinctrl-scmi-v2-0-a9bd86ab5a84@nxp.com > > Changes in v2: > Added comments, and added R-b for Patch 1 > Moved the compatile string and i.MX patch to the end, marked NOT APPLY > Patchset based on lore.kernel.org/all/20231221151129.325749-1-cristian.marussi@arm.com/ > Addressed the binding doc issue, dropped i.MX content. > For the firmware pinctrl scmi driver, addressed the comments from Cristian > For the pinctrl scmi driver, addressed comments from Cristian > > For the i.MX95 OEM stuff, I not have good idea, expect using compatbile > string. Maybe the firmware public an protocol attribute to indicate it is > VENDOR stuff or NXP use a new protocol id, not 0x19. But I think > current pinctrl-scmi.c not able to support OEM config, should we extend > it with some method? Anyway if patch 1-4 is good enough, they could > be picked up first. > > Since I am only able to test the patch on i.MX95 which not support > geneirc pinconf, only OEM configs are tested in my side. > > --- > Oleksii Moisieiev (1): > firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support > > Peng Fan (5): > firmware: arm_scmi: introduce helper get_max_msg_size > dt-bindings: firmware: arm,scmi: support pinctrl protocol > pinctrl: Implementation of the generic scmi-pinctrl driver > [NOT APPLY]firmware: scmi: support compatible string > [NOT APPLY] pinctrl: scmi: implement pinctrl_scmi_imx_dt_node_to_map > > .../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++ > MAINTAINERS | 1 + > drivers/firmware/arm_scmi/Makefile | 1 + > drivers/firmware/arm_scmi/bus.c | 39 +- > drivers/firmware/arm_scmi/common.h | 2 +- > drivers/firmware/arm_scmi/driver.c | 32 +- > drivers/firmware/arm_scmi/pinctrl.c | 908 +++++++++++++++++++++ > drivers/firmware/arm_scmi/protocols.h | 3 + > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-scmi-imx.c | 117 +++ > drivers/pinctrl/pinctrl-scmi.c | 609 ++++++++++++++ > drivers/pinctrl/pinctrl-scmi.h | 12 + > include/linux/scmi_protocol.h | 77 ++ > 14 files changed, 1849 insertions(+), 14 deletions(-) > --- > base-commit: 5389a88b06eb19c3fb08200cc1519406e299b7b0 > change-id: 20231215-pinctrl-scmi-4c5b0374f4c6 > > Best regards,
On Mon, Jan 29, 2024 at 08:36:50PM +0800, Peng Fan wrote: > Hi Sudeep, Cristian > > Would you pick up patch 1-4? I will for v6.9 sometime. > And for i.MX95 OEM extenstion, do you have any suggestions? > I have two points: > 1. use vendor compatible. This would also benefit when supporting vendor > protocol. May be, but that was never on plate for standard protocols. So I don't like that approach either. > 2. Introduce a property saying supporting-generic-pinconf > I am not sure what you mean by that. But that doesn't sound right especial in context of SCMI. So I would say no. > How do you think? > I don't have any other suggestions than fix your driver to use the pinmux properly with features in the upstream pinmux subsystem.
> Subject: Re: [PATCH v3 0/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol > protocol basic support > > On Thu, Feb 01, 2024 at 07:14:17AM +0000, Peng Fan wrote: > > > Subject: Re: [PATCH v3 0/6] firmware: arm_scmi: Add SCMI v3.2 > > > pincontrol protocol basic support > > > > > Hi Peng, > > > > On Mon, Jan 29, 2024 at 1:37 PM Peng Fan <peng.fan@oss.nxp.com> > wrote: > > > > > > > And for i.MX95 OEM extenstion, do you have any suggestions? > > > > I have two points: > > > > 1. use vendor compatible. This would also benefit when supporting > > > > vendor protocol. > > > > 2. Introduce a property saying supporting-generic-pinconf > > > > > > > > How do you think? > > > > > > While I don't know how OEM extensions to SCMI were designed, the pin > > > control subsystem has the philosophy that extensions are for minor > > > fringe stuff, such as a pin config option that no other silicon is > > > using and thus have no use for anyone else. Well that is actually > > > all the custom extensions we have. > > > (This notion is even carried over to SCMI pinctrl.) > > > > > > The i.MX95 OEM extension is really odd to me, it looks like a > > > reimplementation of the core aspects of SCMI pin control, and looks > > > much more like the old i.MX drivers than like the SCMI driver. > > > > i.MX SCMI pin protocol conf settings follows non-SCMI pin conf settings. > > > > It is not just a matter of using custom SCMI OEM types, it is the whole > layout/definitions of the i.MX pin/groups/funcs DT bindings that deviates > from the generic DT bindings layout as handled and expected by the Linux > Pinctrl subsystem (AFAIU), while the SCMI Pinctrl driver as it stands in this > series, was conceived, designed and implemented originally by Oleksii to just > use the generic existing Pinctrl DT bindings; as a consequence, in your i.MX > extensions, you had to add a dedicated i.MX DT parser to interpret the > protocol@19 DT snippet in a completely different way, to try to stick your > custom solution on top of the generic one. The two links shows the drop of i.MX generic pinconf https://patchwork.ozlabs.org/project/linux-gpio/patch/1541149669-10857-7-git-send-email-aisheng.dong@nxp.com/ https://lore.kernel.org/all/20230302072132.1051590-1-linux@rasmusvillemoes.dk/ For non-scmi platforms, the generic pinconf was supported for i.MX7ULP for a while, and then dropped in the end per i.MX maintainers and agreed by Linus. For i.MX95 SCMI platforms, the firmware design is simple and use similar programming model to simplify firmware design. Using generic pinconf means the firmware needs exporting groups/functions/pins and etc, the firmware design will be complicated and code size enlarged. I have no better ideas without introducing a compatible for dt map hook. Build exclusive is not acceptable for distro support. So the last options is i.MX95 switch back to VENDOR protocol ID saying 0x82. But this means to exports functions of pinctrl-scmi.c and reused by pinctrl-scmi-imx.c. If you agree, I will ask firmware developer switch back to a new SCMI ID, and I will use new ID for i.mx pinctrl driver. But in the end I would think when more SCMI vendor protocols comes in, saying vendor A and vendor B both use ID 0x81, both use 0x81 as RTC functions, same issue will come back. Thanks, Peng. > > Thanks, > Cristian > > > > > > > But I sure cannot speak of what is allowed in SCMI OEM extensions or not. > > > > + SPEC owner, Souvik. Any comments? > > > > Thanks, > > Peng. > > > > > > > > Yours, > > > Linus Walleij
On Sun, Feb 4, 2024 at 10:29 AM Peng Fan <peng.fan@nxp.com> wrote: > Using generic pinconf means the firmware needs exporting groups/functions/pins > and etc, the firmware design will be complicated and code size enlarged. This is very much to the core of the problem isn't it? So the argument is to save code effort and size in the firmware. This reflects some of the reasoning behind the device tree bindings that encode "magic numbers" in the DT nodes to mux and configure pins. Often the argument is that it saves space and effort. When the i.MX driver was first discussed it used the standard scheme actually. Look at i.MX 53 for example: https://lore.kernel.org/linux-kernel/1322999384-7886-2-git-send-email-b29396@freescale.com/ Groups and functions! As strings! Then the DT bindings were discussed back and forth between Dong Aisheng (the original driver author), Sasha Hauer and Shawn Guo before arriving at the fsl,pins scheme. Back in the day I was pretty much clueless about device tree and relied on others to review the bindings, which ended up like this: Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt This was in 2011/2012 so many things were not considered. It is clear that this scheme with a number of integers that get poked into registers is convenient for some DT authors, also pinctrl-single uses this as well as I think Mediatek and maybe a few others. Over the years I have come to regret it a bit, I think insisting on groups and functions as strings is better for abstraction. And the point of firmware is to abstract things so they work the same on all systems. Yours, Linus Walleij
On Sun, Feb 04, 2024 at 09:29:25AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH v3 0/6] firmware: arm_scmi: Add SCMI v3.2 pincontrol > > protocol basic support > > > > On Thu, Feb 01, 2024 at 07:14:17AM +0000, Peng Fan wrote: > > > > Subject: Re: [PATCH v3 0/6] firmware: arm_scmi: Add SCMI v3.2 > > > > pincontrol protocol basic support > > > > > > > > Hi Peng, > > > > > > On Mon, Jan 29, 2024 at 1:37 PM Peng Fan <peng.fan@oss.nxp.com> > > wrote: > > > > > > > > > And for i.MX95 OEM extenstion, do you have any suggestions? > > > > > I have two points: > > > > > 1. use vendor compatible. This would also benefit when supporting > > > > > vendor protocol. > > > > > 2. Introduce a property saying supporting-generic-pinconf > > > > > > > > > > How do you think? > > > > > > > > While I don't know how OEM extensions to SCMI were designed, the pin > > > > control subsystem has the philosophy that extensions are for minor > > > > fringe stuff, such as a pin config option that no other silicon is > > > > using and thus have no use for anyone else. Well that is actually > > > > all the custom extensions we have. > > > > (This notion is even carried over to SCMI pinctrl.) > > > > > > > > The i.MX95 OEM extension is really odd to me, it looks like a > > > > reimplementation of the core aspects of SCMI pin control, and looks > > > > much more like the old i.MX drivers than like the SCMI driver. > > > > > > i.MX SCMI pin protocol conf settings follows non-SCMI pin conf settings. > > > > > > > It is not just a matter of using custom SCMI OEM types, it is the whole > > layout/definitions of the i.MX pin/groups/funcs DT bindings that deviates > > from the generic DT bindings layout as handled and expected by the Linux > > Pinctrl subsystem (AFAIU), while the SCMI Pinctrl driver as it stands in this > > series, was conceived, designed and implemented originally by Oleksii to just > > use the generic existing Pinctrl DT bindings; as a consequence, in your i.MX > > extensions, you had to add a dedicated i.MX DT parser to interpret the > > protocol@19 DT snippet in a completely different way, to try to stick your > > custom solution on top of the generic one. > > The two links shows the drop of i.MX generic pinconf > https://patchwork.ozlabs.org/project/linux-gpio/patch/1541149669-10857-7-git-send-email-aisheng.dong@nxp.com/ > https://lore.kernel.org/all/20230302072132.1051590-1-linux@rasmusvillemoes.dk/ > > For non-scmi platforms, the generic pinconf was supported > for i.MX7ULP for a while, and then dropped in the end per i.MX maintainers > and agreed by Linus. > > For i.MX95 SCMI platforms, the firmware design is simple and use similar > programming model to simplify firmware design. > > Using generic pinconf means the firmware needs exporting groups/functions/pins > and etc, the firmware design will be complicated and code size enlarged. > Understood. > I have no better ideas without introducing a compatible for dt map hook. > > Build exclusive is not acceptable for distro support. > Indeed, and I understand that, but in any case it wont be required for both the generic pinconf and the IMX way to coexist and live together on the same system/platform at runtime, right ? So, I was thinking as an alternative, while still building all (generic+IMX) in defconfig, wouldn't be possible to detect at runtime in the pinctrl-scmi probe() which is the type of binding used in the DT (i.e. generic VS imx), before registering with the core Pinctrl subsystem, and so act accordingly parsing and providing different callbacks based on that ? I mean, without any compatible addition, just looking up the protocol@19 node content @probe and switch to iMX callbacks if the fsl,pins binding is found ? Not sure is this is totally viable, nor clean or that it is not less horrific from the Pinctrl_subsystem/DT point of view for Linus ... just an idea to think about or discard. > So the last options is i.MX95 switch back to VENDOR protocol ID saying > 0x82. But this means to exports functions of pinctrl-scmi.c and reused > by pinctrl-scmi-imx.c. If you agree, I will ask firmware developer > switch back to a new SCMI ID, and I will use new ID for i.mx pinctrl > driver. > I dont think there is really any chance to upstream a vendor protocol that is a bare copy of a standard protocol just to workaround this...and if this is going to be the end-result you can just keep your current 2 small 'IMX custom-compatible' patches downstream. > But in the end I would think when more SCMI vendor protocols > comes in, saying vendor A and vendor B both use ID 0x81, > both use 0x81 as RTC functions, same issue will come back. > This wont be a problem once the RFC[1] I posted last week is in...unless someone objects, you will have to identify your vendor protocol module at build time ALSO with the vendor/sub_vendor ID string exposed by your fw... ...so each vendor will effectively have the full vendor protocol space number at their disposal. Indeed this week, I hope to review your series about custom protocols and the qualcomm ones and ask both for feedback on [1] and then to rework those series on some non-RFC similar to 1 that I will post afterwards. Thanks, Cristian [1]: https://lore.kernel.org/linux-arm-kernel/20240122122437.546621-1-cristian.marussi@arm.com/
On Mon, Feb 5, 2024 at 10:11 AM Peng Fan <peng.fan@nxp.com> wrote: > [Me] > > This is very much to the core of the problem isn't it? > > Yes. Code size should be as small as possible. This is a valid argument, I don't know how big your SCMI FW is, or if it has to fit into some on-chip memory or so. > And using SCMI generic pinconf, there will be multiple > SCMI calls(not MMIO access), such as setting mux(ops->set_mux) > needs an SCMI call, pad settings(ops->pin_config_set) needs an SCMI call. > And maybe ops->get_function_name needs an extra SCMI call. > > With current i.MX design, only one SCMI call is used, which > use less time. I think this could be fixed in the spec, let's see what the spec author says. In any case: pin control is pretty much never on a hot path, a few microseconds more or less doesn't matter. It's usually just used and probe, suspend/resume and maybe shutdown. All usecases on the slowpath, so I think it's a premature optimization. > > Over the years I have come to regret it a bit, I think insisting on groups and > > functions as strings is better for abstraction. And the point of firmware is to > > abstract things so they work the same on all systems. > > With current: > pinctrl_uart1: uart1grp { > fsl,pins = < > IMX95_PAD_UART1_RXD__AONMIX_TOP_LPUART1_RX 0x31e > IMX95_PAD_UART1_TXD__AONMIX_TOP_LPUART1_TX 0x31e > >; > }; > > It is very easy for people to know the meaning from reading reference manual. Yes If the defines are provided it's quite readable. And I think Freescale people really get used to it. But from a maintenance and standardization point of view, it's nice if everything works the same way and looks the same. > If using generic pinconf, the dt node will be like > Uartgrp { > pins = "uart1txd", "uart1rxd"; > functions = "uart1"; > bias-xx > drive-strength = > }; Well that looks good to me :) It looks the same as e.g. Qualcomm or nVidia pins. So it's easy for me to read and understand, and that's my perspective as a maintainer. > The firmware needs add more code to export functions, pack the conf settings, > each pins needs a function name per current i.MX HW logic. Indeed, but I think any standard requires a bit of code to implement. How much is "too much" code is a matter of taste and physical contraints. (A PC UEFI BIOS isn't exactly small either.) I understand your point of view, and I think it is pretty much the same stance that the Freescale maintainers put forward for the DT bindings for Freescale. Yours, Linus Walleij
This patchset is a rework from Oleksii's RFC v5 patchset https://lore.kernel.org/all/cover.1698353854.git.oleksii_moisieiev@epam.com/ This patchset introduces some changes based on RFC v5: - introduce helper get_max_msg_size - support compatible string - iterate the id_table - Support multiple configs in one command - Added i.MX support - Patch 5 firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support is almost same as RFCv5 expect multiple configs support. - Patch 4 the dt-bindings includes compatible string to support i.MX - Rebased on 2023-12-15 linux-next/master If any comments from RFC v5 are missed, I am sorry in advance. This PINCTRL Protocol is following Version 3.2 SCMI Spec Beta release. On ARM-based systems, a separate Cortex-M based System Control Processor (SCP) provides control on pins, as well as with power, clocks, reset controllers. So implement the driver to support such cases. The i.MX95 Example as below: Configuration: The scmi-pinctrl driver can be configured using DT bindings. For example: / { sram0: sram@445b1000 { compatible = "mmio-sram"; reg = <0x0 0x445b1000 0x0 0x400>; #address-cells = <1>; #size-cells = <1>; ranges = <0x0 0x0 0x445b1000 0x400>; scmi_buf0: scmi-sram-section@0 { compatible = "arm,scmi-shmem"; reg = <0x0 0x80>; }; scmi_buf1: scmi-sram-section@80 { compatible = "arm,scmi-shmem"; reg = <0x80 0x80>; }; }; firmware { scmi { compatible = "arm,scmi"; mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>; shmem = <&scmi_buf0>, <&scmi_buf1>; #address-cells = <1>; #size-cells = <0>; scmi_iomuxc: protocol@19 { compatible = "fsl,imx95-scmi-pinctrl"; reg = <0x19>; }; }; }; }; &scmi_iomuxc { pinctrl_tpm3: tpm3grp { fsl,pins = < IMX95_PAD_GPIO_IO12__TPM3_CH2 0x51e >; }; }; This patchset has been tested on i.MX95-19x19-EVK board. Signed-off-by: Peng Fan <peng.fan@nxp.com> --- Changes in v3: - Add R-b for dt-binding patch - Use 80 chars per line to align with other scmi drivers - Add pinctrl_scmi_alloc_configs pinctrl_scmi_free_configs to replace driver global config_value and config_type array to avoid in parrell access issue. When num_configs is larger than 4, use alloc, else use stack. - Drop the separate MAITAINERS entry for firmware scmi pinctrl - Use enum type, not u8 when referring the scmi or generic pin conf type - Drop scmi_pinctrl_config_get_all which is not used at all for now. - Update copyright year to 2024 - Move the enum scmi_pinctrl_conf_type above pinctrl_proto_ops for consistency - Link to v2: https://lore.kernel.org/r/20240104-pinctrl-scmi-v2-0-a9bd86ab5a84@nxp.com Changes in v2: Added comments, and added R-b for Patch 1 Moved the compatile string and i.MX patch to the end, marked NOT APPLY Patchset based on lore.kernel.org/all/20231221151129.325749-1-cristian.marussi@arm.com/ Addressed the binding doc issue, dropped i.MX content. For the firmware pinctrl scmi driver, addressed the comments from Cristian For the pinctrl scmi driver, addressed comments from Cristian For the i.MX95 OEM stuff, I not have good idea, expect using compatbile string. Maybe the firmware public an protocol attribute to indicate it is VENDOR stuff or NXP use a new protocol id, not 0x19. But I think current pinctrl-scmi.c not able to support OEM config, should we extend it with some method? Anyway if patch 1-4 is good enough, they could be picked up first. Since I am only able to test the patch on i.MX95 which not support geneirc pinconf, only OEM configs are tested in my side. --- Oleksii Moisieiev (1): firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Peng Fan (5): firmware: arm_scmi: introduce helper get_max_msg_size dt-bindings: firmware: arm,scmi: support pinctrl protocol pinctrl: Implementation of the generic scmi-pinctrl driver [NOT APPLY]firmware: scmi: support compatible string [NOT APPLY] pinctrl: scmi: implement pinctrl_scmi_imx_dt_node_to_map .../devicetree/bindings/firmware/arm,scmi.yaml | 50 ++ MAINTAINERS | 1 + drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/bus.c | 39 +- drivers/firmware/arm_scmi/common.h | 2 +- drivers/firmware/arm_scmi/driver.c | 32 +- drivers/firmware/arm_scmi/pinctrl.c | 908 +++++++++++++++++++++ drivers/firmware/arm_scmi/protocols.h | 3 + drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-scmi-imx.c | 117 +++ drivers/pinctrl/pinctrl-scmi.c | 609 ++++++++++++++ drivers/pinctrl/pinctrl-scmi.h | 12 + include/linux/scmi_protocol.h | 77 ++ 14 files changed, 1849 insertions(+), 14 deletions(-) --- base-commit: 5389a88b06eb19c3fb08200cc1519406e299b7b0 change-id: 20231215-pinctrl-scmi-4c5b0374f4c6 Best regards,