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
