RE: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: Add properties for i.MX95 Pinctrl OEM extensions

[Peng Fan <peng.fan@xxxxxxx>]

Hi Rob, Sudeep, Cristian, Linus

Although this is dt-binding stuff, I hope you could also help suggest how to
support NXP SCMI OEM extensions. There is less chance NXP will change
SCMI firmware to make linux happy using generic conf stuff, because
the SCMI firmware will also need to work for FreeRTOS/GHS/QNX and etc.

> Subject: Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: Add properties
> for i.MX95 Pinctrl OEM extensions
> 
> On Thu, Apr 25, 2024 at 09:06:00AM +0000, Peng Fan wrote:
> > Hi Rob,
> >
> > > Subject: Re: [PATCH v2 1/2] dt-bindings: firmware: arm,scmi: Add
> > > properties for i.MX95 Pinctrl OEM extensions
> > >
> > > On Thu, Apr 18, 2024 at 10:53:17AM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > >
> > > > i.MX95 Pinctrl is managed by SCMI firmware using OEM extensions.
> > > > This patch is to add i.MX95 Pinctrl OEM extensions properties.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > ---
> > > >  .../devicetree/bindings/firmware/arm,scmi.yaml     |  9 ++-
> > > >  .../bindings/firmware/nxp,imx95-scmi-pinctrl.yaml  | 66
> > > > ++++++++++++++++++++++
> > > >  2 files changed, 72 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> >
> > ....
> >
> > > > +
> > > > +title: i.MX System Control and Management Interface (SCMI)
> > > > +Pinctrl Protocol
> > > > +
> > > > +maintainers:
> > > > +  - Peng Fan <peng.fan@xxxxxxx>
> > > > +
> > > > +patternProperties:
> > > > +  '[a-f0-9]+$':
> > > > +    type: object
> > > > +    unevaluatedProperties: false
> > > > +
> > > > +    properties:
> > > > +      pins:
> > > > +        $ref: /schemas/types.yaml#/definitions/string
> > > > +        description: name of the pin
> > > > +        items:
> > > > +          enum: [ daptdi, daptmsswdio, daptclkswclk, daptdotraceswo,
> > > > +                  gpioio00, gpioio01, gpioio02, gpioio03,
> > > > + gpioio04,
> > >
> > > ioio?
> >
> > Yes. The name is exported from SCMI firmware.
> >
> > >
> > > > +                  gpioio05, gpioio06, gpioio07, gpioio08, gpioio09,
> > > > +                  gpioio10, gpioio11, gpioio12, gpioio13, gpioio14,
> > > > +                  gpioio15, gpioio16, gpioio17, gpioio18, gpioio19,
> > > > +                  gpioio20, gpioio21, gpioio22, gpioio23, gpioio24,
> > > > +                  gpioio25, gpioio26, gpioio27, gpioio28, gpioio29,
> > > > +                  gpioio30, gpioio31, gpioio32, gpioio33, gpioio34,
> > > > +                  gpioio35, gpioio36, gpioio37, ccmclko1, ccmclko2,
> > > > +                  ccmclko3, ccmclko4, enet1mdc, enet1mdio, enet1td3,
> > > > +                  enet1td2, enet1td1, enet1td0, enet1txctl, enet1txc,
> > > > +                  enet1rxctl, enet1rxc, enet1rd0, enet1rd1, enet1rd2,
> > > > +                  enet1rd3, enet2mdc, enet2mdio, enet2td3, enet2td2,
> > > > +                  enet2td1, enet2td0, enet2txctl, enet2txc, enet2rxctl,
> > > > +                  enet2rxc, enet2rd0, enet2rd1, enet2rd2, enet2rd3,
> > > > +                  sd1clk, sd1cmd, sd1data0, sd1data1, sd1data2, sd1data3,
> > > > +                  sd1data4, sd1data5, sd1data6, sd1data7, sd1strobe,
> > > > +                  sd2vselect, sd3clk, sd3cmd, sd3data0, sd3data1,
> > > > +                  sd3data2, sd3data3, xspi1data0, xspi1data1, xspi1data2,
> > > > +                  xspi1data3, xspi1data4, xspi1data5, xspi1data6,
> > > > +                  xspi1data7, xspi1dqs, xspi1sclk, xspi1ss0b, xspi1ss1b,
> > > > +                  sd2cdb, sd2clk, sd2cmd, sd2data0, sd2data1, sd2data2,
> > > > +                  sd2data3, sd2resetb, i2c1scl, i2c1sda, i2c2scl, i2c2sda,
> > > > +                  uart1rxd, uart1txd, uart2rxd, uart2txd, pdmclk,
> > > > +                  pdmbitstream0, pdmbitstream1, sai1txfs, sai1txc,
> > > > +                  sai1txd0, sai1rxd0, wdogany, fccuerr0 ]
> > > > +
> > > > +      nxp,func-id:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description: Indicates the PAD connections which function
> > > > + module
> > >
> > > The description makes no sense...
> >
> > I will use below description in v3:
> > For each pin, there are up to 8 muxing options (called ALT modes). For
> example:
> > Select 1 of 5 iomux modes to be used for pad: DAP_TCLK_SWCLK.
> > 000b - Select mux mode: ALT0 mux port: JTAG_MUX_TCK of instance:
> > jtag_mux 010b - Select mux mode: ALT2 mux port: CAN4_RX of instance:
> > imx95_wakeupmix_top 100b - Select mux mode: ALT4 mux port:
> > FLEXIO1_FLEXIO30 of instance: imx95_wakeupmix_top 101b - Select mux
> > mode: ALT5 mux port: GPIO3_IO30 of instance: imx95_wakeupmix_top
> 110b
> > - Select mux mode: ALT6 mux port: LPUART5_CTS_B of instance:
> imx95_wakeupmix_top The nxp,func-id property is selecting one ALT mode.
> 
> Looks like constraints on the values. Please add schema for them. I don't
> think you need to list an example.

Will add maxItems: 8, minItems: 1
> 
> Perhaps you should be using 'pinmux' property. That is the standard way to
> specify a numeric mux mode.

Ok let me try. 

> 
> 
> > >
> > > > +
> > > > +      nxp,pin-conf:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description: The PAD configuration
> > >
> > > PAD stands for? Or is it pin configuration as the property name says?
> >
> > It is pin configuration which includes pull up/down, drive strength and etc.
> >
> > >
> > > This is just a raw register values?
> >
> > Yes.
> >
> > Why can't standard pinctrl properties be
> > > used here. What's the point of using SCMI pinctrl if you are just shuttling
> > > register values to SCMI?
> >
> > Standard pinctrl properties will need more memory in our SCMI
> > firmware design. We have limited on-chip memory for SCMI firmware usage.
> 
> Then maybe you shouldn't be using SCMI and just write a pinctrl driver.

I think I have to. pinctrl-scmi-imx.c was not preferred by ARM SCMI maintainer.

If Linus, Cristian, Sudeep are ok, I could add back pinctrl-scmi-imx.c for
some OEM mapping/unmapping stuff.

> Or, Linux has to parse the DT anyways, so use standard properties and
> transform them into your register values and send that to SCMI.

Cristian, Sudeep, Linus,

Is this ok for you? Before sending to SCMI, linux driver transform
the bits into register value, and send to SCMI firmware.

> 
> > > > +
> > > > +      nxp,daisy-id:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description: The select input register ID
> > > > +
> > > > +      nxp,daisy-conf:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description: The select input register value
> > >
> > > I remember some daisy chaining stuff in iomuxc, but otherwise
> > >
> > > What's the relationship between these 2 properties? Is it valid if only
> > > 1 is present? I doubt it.
> >
> > The scmi firmware will convert the daisy-id into register address, and
> > write the daisy-conf value into the register.
> >
> >  Looks like an address and value pair. Either the
> > > schema needs to define allowed combinations
> >
> > The firmware will check whether id and conf is valid, is there a need to
> define
> > allowed combinations? There will be lots of check to define allowed
> > combinations.
> 
> The firmware can't check that the DT contains nxp,daisy-id but not
> nxp,daisy-conf or vice-versa. Of course the kernel could check that, but
> validating the DT is not the kernel's job.
> 
> 
> >  or design the properties in a
> > > way you don't have too. IOW, make a single property that's a tuple of
> register
> > > ID and value.
> >
> > Define tuple will make the property not able to use linux generic pinconf
> > driver. The current parse_dt_cfg for custom_dt_params only support a
> > single u32 value and not easy to support tuple.
> 
> Sounds like a Linux problem. We're talking about bindings. Fix Linux if
> it doesn't meet your needs.
> 
> 
> How many bits of id and conf do you need? This too could just be part of
> 'pinmux' property. It's value is entirely h/w specific and up to you.
> You just need to fit it into 32-bits (though we could perhaps enlarge
> it).

For conf, 4 bits is find. For id, 12 bits is fine I think. 

Using pinmux property with some vendor handing code. Not sure
ARM SCMI maintainer is happy or not.

Thanks,
Peng.

> 
> Rob