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

[Cristian Marussi <cristian.marussi@xxxxxxx>]

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@xxxxxxxxxxxxxx/

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.

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.

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@xxxxxxxx> 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