Re: [RFC v5 5/5] dt-bindings: firmware: arm,scmi: Add support for pinctrl protocol

[Souvik Chakravarty <souvik.chakravarty@arm.com>]

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