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

[Andrei Stefanescu]

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.

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.

What do you think?

Best regards,
Andrei


> 
> Best regards,
> Andrei
> 
>  
>> Cheers,
>> Conor.
>>
>>>
>>>>>
>>>>> Additionally, it looks like "opads0" and "ipads0" are in a different
>>>>> region to their "1" equivalents. Should this really be represented as
>>>>> two disctint GPIO controllers?
>>>>
>>>> I will add a bit more context regarding the hardware.
>>>>
>>>> The hardware module which implements pinctrl & GPIO is called SIUL2.
>>>> For both S32G2 and S32G3 we have the same version of the module and 
>>>> it is integrated in the same way. Each SoC has two SIUL2 instances which
>>>> mostly have the same register types and only differ in the number
>>>> of pads associated to them:
>>>>
>>>> - SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
>>>> - SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190
>>>>
>>>> There are multiple registers for the SIUL2 modules which are important
>>>> for pinctrl & GPIO:
>>>>
>>>> - MSCR (Multiplexed Signal Configuration Register)
>>>>   It configures the function of a pin and some
>>>>   pinconf properties:
>>>>     - input buffer
>>>>     - output buffer
>>>>     - open-drain
>>>>     - pull-up/pull-down
>>>>     - slew rate
>>>>   Function 0 means the pin is to be used as a GPIO.
>>>>
>>>> - IMCR (Input Multiplexed Signal and Configuration Register)
>>>>   If the signal on this pad is to be read by another hardware
>>>>   module, this register is similar to a multiplexer, its value
>>>>   configures which pad the hardware will link to the module.
>>>>   As an example let's consider the I2C0 SDA line. It has one
>>>>   IMCR associated to it. Below are its possible pins and
>>>>   corresponding IMCR values:
>>>>     pin 16 <- 2
>>>>     pin 24 <- 7
>>>>     pin 31 <- 3
>>>>     pin 122 <- 4 
>>>>       (Note that MSCR122 is part of SIUL2_1 but the IMCR for
>>>>        I2C0_SDA is part of SIUL2_0)
>>>>     pin 153 <- 5
>>>>     pin 161 <- 6
>>>>   The IMCR values should be aligned with the function bits in the
>>>>   MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
>>>>   the function bits in MSCR122 and write the value 4 to the I2C0_SDA
>>>>   IMCR. 
>>>>
>>>> - PGPDO/PGPDI Parallel GPIO Pad Data Out/In
>>>>   16 bit registers where each bit(besides some gaps) represents
>>>>   a GPIO's output/input value
>>>>
>>>> - DISR0, DIRER0, IREER0, IFEER0
>>>>   These registers are used for: status, enable, rising/falling edge
>>>>   configuration for interrupts. We have 32 interrupts called EIRQ and
>>>>   each interrupt has one or more pads associated with it (controlled
>>>>   by an IMCR register per EIRQ).
>>>>
>>>>   However, one important thing to note is that even though there are
>>>>   EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
>>>>   are only present in SIUL2_1.
>>>>
>>>> Because of mixed pins (I2C0_SDA in the example above with the MSCR
>>>> in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
>>>> configuration registers in SIUL2_1 we decided to have a single
>>>> driver instance.
>>>>
>>>>>
>>>>>
>>>>> Cheers,
>>>>> Conor.
>>>>>
>>>>
>>>> Best regards,
>>>> Andrei
>>>>
>>
>>
>  
>