Hi Conor,

On 04/10/2024 00:01, Conor Dooley wrote:

On Thu, Oct 03, 2024 at 01:22:35PM +0300, Andrei Stefanescu wrote:

quoted

Hi Conor,

quoted

quoted

quoted

quoted

quoted

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@oss.nxp.com/ (local)
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.

quoted

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