Messages in this thread | | | Date | Mon, 13 Nov 2023 12:56:06 +0000 | Subject | Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol | From | Souvik Chakravarty <> |
| |
Hi,
On 10/11/2023 15:24, Cristian Marussi wrote: > On Fri, Nov 10, 2023 at 09:58:39AM +0900, Takahiro Akashi wrote: >> Hi Arm folks, >> > >> Do you have any comment? >> I expect that you have had some assumption when you defined >> SCMI pinctrl protocol specification. >> > > [CC Souvik] > > @Souvik for context see: > https://lore.kernel.org/all/CACRpkdZ4GborirSpa3GK_PwMgCvY0ePEmZO+CwnLcP6nAdieow@mail.gmail.com/ > > Hi, > > I am not sure what is the full story here, BUT the spec was mainly aimed > at supporting PINCTRL in SCMI with the idea to then, later on, base GPIO > on top of it, "easily" building on the PINCTRL spec features in the future > with a separate series from the one Oleksii is working on...but it like > seems the future is already here and maybe we have discovered something > to be clarified... > > Souvik/Oleksii can tell you better what were (if any) further assumptions > related to GPIO on top on SCMI/PINCTRL, but the aim of this series was > always to be just the basic Generic Pinctrl support when dealing with an > SCMI server backend.
The initial assumption always was that GPIOs can be considered as a specific function. Note that the spec does not define the types of function and leaves it to the DT binding (or driver) to figure out the function descriptions/names.
> > Regarding the current Pinctrl series by Oleksii, I would also notice that, > indeed, some "non-spec-dictated" naming assumptions are ALREADY present > somehow, because, currently, the spec and the pinctrl SCMI protocol layer > speak/refer about pins/groups/functions, as usual, only in terms of numeric > identifiers/IDs (with an associated name of course), while the pinctrl > driver (thanks to the Linux pictrl subsystem layer) describes and refers > anything in the DT in terms of names: so all of this really works only > because the names used in the DT happen to match the names reported by > the backend server. > > My test DT uses just what Oleksii exemplified in the cover letter: > > pinctrl_i2c2: i2c2 { > groups = "i2c2_a", "i2c2_b"; > function = "i2c2"; > }; > > pinctrl_mdio: pins_mdio { > groups = "avb_mdio"; > drive-strength = <24>; > }; > > keys_pins: keys { > pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1"; > bias-pull-up; > }; > > > with a dummmy test driver referring to it, so as to trigger the drivers > core to initialize the pinctrl stuff. > > But all of this works just because, in the example of my emulated setup, > my fake server exposes resources that are exactly named just as how the > above DT expects pins/functions/pins to be named, because this is how > the Generic Pinctrl subsystem in Linux is supposed to work, right ? > > The difference is that the names, in the case of pinctrl-scmi, are not > hardcoded in the specific pin-controller driver BUT are provided dynamically > by the SCMI server at runtime. > > And this is just a naming convention, between the Linux picntrl subsys AND > the SCMI server, that allows the Linux Pinctrl subsys to map, under-hood, > names to type/IDs as expected by the SCMI protocol layer (and by the spec): > so when you will define and describe a real platform with a DT, you will > will have to provide your name references, knowing that the shipped platform > SCMI fw will advertise exactly the same (or a superset of them) > > As such, personally, I would find reasonable to use, equally, some > conventional function name like 'gpio' to advertise and configure groups > of pins as being used as GPIOs.
As a general principle, we dont try to put naming conventions in the spec if it can be easily resolved via DT. If this is proving to be a hassle then we can "recommend" in the spec that pins which can only be GPIOs are named starting "GPIO". Similar for functions.
However looking at Linus' comments below, I am not sure we are at that stage yet?
Regards, Souvik
> > Maybe, though, both of these expected naming comventions should be > explicitly stated in the spec: indeed if you look at some Sensor protocol > extensions added in v3.0, in 4.7.2.5.1 "Sensor Axis Descriptors" > regarding naming we say: > > "It is recommended that the name ends with ‘_’ > followed by the axis of the sensor in uppercase. For > example, the name for the x-axis of a triaxial > accelerometer could be “acc_X” or “_X”." > > ...so maybe some similar remarks could be added here. > > Souvik is really the one who can have a say about the opportunity (or > not) of these kind of explicit advised naming conventions on the spec, > so I have CCed him. > >> On Mon, Nov 06, 2023 at 02:12:36PM +0100, Linus Walleij wrote: >>> On Fri, Oct 27, 2023 at 8:28???AM Oleksii Moisieiev >>> <Oleksii_Moisieiev@epam.com> wrote: >>> >>>> + keys_pins: keys-pins { >>>> + pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1"; >>>> + bias-pull-up; >>>> + }; >>> >>> This is kind of interesting and relates to my question about naming groups and >>> functions of GPIO pins. >>> >>> Here we see four pins suspiciously named "GP_*" which I read as >>> "generic purpose" >>> and they are not muxed to *any* function, yes pulled up. >>> >>> I would have expected something like: >>> >>> keys_pins: keys-pins { >>> groups = "GP_5_17_grp", "GP_5_20_grp", "GP_5_22_grp", "GP_2_1_grp"; >>> function = "gpio"; >>> pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1"; >>> bias-pull-up; >>> }; >>> >>> I hope this illustrates what I see as a problem in not designing in >>> GPIO as an explicit >>> function, I get the impression that these pins are GPIO because it is hardware >>> default. >> >> If you want to stick to "explicit", we may rather introduce a pre-defined >> sub-node name, "gpio", in a device tree binding, i.e. >> >> protocol@19 { // pinctrl protocol >> ... // other pinmux nodes >> >> scmi_gpio: gpio { // "gpio" is a fixed name >> keys-pins { >> pins = "GP_5_17", "GP_5_20", "GP_5_22", "GP_2_1"; >> bias-pull-up; >> // possibly input or output >> }; >> input-pins { >> groups = "some group"; // any name >> input-mode; >> } >> output-pins { >> pins = "foo1", "foo2"; // any name >> output-mode; >> } >> } >> } >> > > I suppose your proposal of a specially named "gpio" node would be > another way, BUT it would also mean describing something in the DT that > could be discoverable dynamically querying the server (while making the > above assumptions about conventions). > > Thanks, > Cristian
| |