Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Conor Dooley]

On Thu, Oct 03, 2024 at 01:22:35PM +0300, Andrei Stefanescu wrote:
> Hi Conor,
> 
> >>>>>
> >>>>> Huh, I only noticed this now. Are you sure that this is a correct
> >>>>> representation of this device, and it is not really part of some syscon?
> >>>>> The "random" nature of the addresses  and the tiny sizes of the
> >>>>> reservations make it seem that way. What other devices are in these
> >>>>> regions?
> >>>
> >>> Thanks for your answer to my second question, but I think you missed this
> >>> part here ^^^
> >>
> >> Reading it again, I think you might have answered my first question,
> >> though not explicitly. The regions in question do both pinctrl and gpio,
> >> but you have chosen to represent it has lots of mini register regions,
> >> rather than as a simple-mfd type device - which I think would be the
> >> correct representation. .
> > 
> > Yes, SIUL2 is mostly used for pinctrl and GPIO. The only other uses case is
> > to register a nvmem device for the first two registers in the SIUL2 MIDR1/MIDR2
> > (MCU ID Register) which tell us information about the SoC (revision,
> > SRAM size and so on).
> > 
> > I will convert the SIUL2 node into a simple-mfd device and switch the
> > GPIO and pinctrl drivers to use the syscon regmap in v5.
> 
> I replied in the other patch series
> https://lore.kernel.org/all/a924bbb6-96ec-40be-9d82-a76b2ab73afd@xxxxxxxxxxx/
> that I actually decided to unify the pinctrl&GPIO drivers instead of making
> them mfd_cells.

Yeah, I'm sorry I didn't reply to that sooner. I was being a lazy shit,
and read a book instead of replying yesterday. Almost did it again today
too...

To answer the question there, about simple-mfd/syscon not being quite
right:
I guess you aren't a simple-mfd, but this region does seem to be an mfd
to me, given it has 3 features. I wouldn't object to this being a single
node/device with two reg regions, given you're saying that the SIUL2_0
and SIUL2_1 registers both are required for the SIUL2 device to work but
are in different regions of the memory map.

> I have a question regarding the NVMEM driver that I mentioned earlier. I haven't
> yet created a patch series to upstream it but I wanted to discuss about it
> here since it relates to SIUL2 and, in the future, we would like to upstream it
> as well.
> 
> We register a NVMEM driver for the first two registers of SIUL2 which can
> then be read by other drivers to get information about the SoC. I think
> there are two options for integrating it:
> 
> - Separate it from the pinctrl&GPIO driver as if it were part of a different
> IP. This would look something like this in the device tree
> 
> /* SIUL2_0 base address is 0x4009c000 */
> /* SIUL2_1 base address is 0x44010000 */
> 
> nvmem1@4009c000 {
> 	/* The registers are 32bit wide but start at offset 0x4 */
> 	reg = <0x4009c000 0xc>;
> 	[..]
> };
> 
> pinctrl-gpio@4009c010 {
> 	reg = <0x4009c010 0xb84>,  /* SIUL2_0 32bit registers */
> 	      <0x4009d700 0x50>,   /* SIUL2_0 16bit registers */
> 	      <0x44010010 0x11f0>, /* SIUL2_1 32bit registers */
> 	      <0x4401170c 0x4c>,   /* SIUL2_1 16bit registers */  
> 	[..]
> };
> 
> nvmem2@0x44010000 {
> 	reg = <0x44010000 0xc>;
> 	[..]
> }
> 
> - have the nvmem as an mfd cell and the pinctrl&GPIO as another mfd cell
> 
> The first option keeps the nvmem completely separated from pinctrl&GPIO
> but it makes the pinctrl&GPIO node start at an "odd" address. The second one
> more accurately represents the hardware (since the functionality is part of
> the same hardware block) but I am not sure if adding the mfd layer would add
> any benefit since the two functionalities don't have any shared resources in
> common.

That's kinda what mfd is for innit, multiple (disparate) functions. I'm
not sure that you need an nvmem child node though, you may be able to
"just" ref nvmem.yaml, but I am not 100% sure how that interacts with
the pinctrl child node you will probably want to house pinctrl
properties in. The mfd driver would be capable of registering drivers
for each of the functions, you don't need to have a child node and a
compatible to register them. Cos of that, you shouldn't really require
a child node for GPIO, the gpio controller properties could go in the
mfd node itself.

Attachment: signature.asc
Description: PGP signature