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

[Andrei Stefanescu]

Hi Conor,

On 04/10/2024 00:01, Conor Dooley wrote:
> 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.

Just to confirm that I got it right, SIUL2 would end up being a single node,
looking something like:


siul2: siul2@4009c000 {
	compatible = "nxp,s32g2-siul2";
	reg = <0x4009C000 SIUL2_0_SIZE>,
	      <0x44010000 SIUL2_1_SIZE>;
	gpio-controller;
	#gpio-cells = <2>;
	gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
	gpio-reserved-ranges = <102 10>, <123 21>;
	interrupt-controller;
	#interrupt-cells = <2>;
	interrupts = <GIC_SPI ..>;

	/* for nvmem */
	#address-cells = <1>;
	#size-cells = <1>;

	*-nvmem-*@index {
		reg = <index 0x4>;
		[..]	
	};

	*-pins-* {
		pinmux = <...>
		[..]
	};
};

The siul2 node is for a mfd driver which will have two mfd_cells:
- one for the combined pinctrl&GPIO
- one for the nvmem driver

I think I can distinguish between nvmem cells and pinctrl functions
based on the presence of the pinmux property. Otherwise, I could
create two subnodes in the siul2 node for them:

siul2 {
	[..]
	nvmem-cells {
		*-nvmem-*@index {
			reg = <index 0x4>;
			[..]	
		};
	};

	pinctrl-functions {
		*-pins-* {
			pinmux = <...>
			[..]
		};
	};
	[..]
};


The siul2 driver will pass to the mfd_cells:
- access to the multiple regmaps(2 * SIUL2 each with 32bit and 16bit registers)
- the interrupt resource
- nvmem cells

This also implies that I should add a new separate binding for the siul2 node and
deprecate the existing pinctrl one(nxp,s32g2-siul2-pinctrl.yaml).

Would that be right?


Best regards,
Andrei