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

[Andrei Stefanescu]

Hi Conor,

On 30/09/2024 18:07, Conor Dooley wrote:
> On Mon, Sep 30, 2024 at 04:00:57PM +0100, Conor Dooley wrote:
>> On Fri, Sep 27, 2024 at 10:13:54AM +0300, Andrei Stefanescu wrote:
>>> Hi Conor,
>>>
>>> Thank you very much for the prompt review!
>>>
>>> On 26/09/2024 18:38, Conor Dooley wrote:
>>>> On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:
>>>>> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
>>>>>
>>>>> Signed-off-by: Phu Luu An <phu.luuan@xxxxxxx>
>>>>> Signed-off-by: Larisa Grigore <larisa.grigore@xxxxxxx>
>>>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
>>>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxxxxxx>
>>>>
>>>> What's up with this SoB chain? You're the author what did
>>>> the other 3 people do? Are they missing co-developed-by tags?
>>>
>>> Yes, thank you for suggesting it! I will also add Co-developed-by tags
>>> for them. In the end it should look like this:
>>>
>>> Co-developed-by: Phu Luu An <phu.luuan@xxxxxxx>
>>> Signed-off-by: Phu Luu An <phu.luuan@xxxxxxx>
>>> Co-developed-by: Larisa Grigore <larisa.grigore@xxxxxxx>
>>> Signed-off-by: Larisa Grigore <larisa.grigore@xxxxxxx>
>>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxxxxxx>
>>>
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +
>>>>> +    gpio@4009d700 {
>>>>> +        compatible = "nxp,s32g2-siul2-gpio";
>>>>> +        reg = <0x4009d700 0x10>,
>>>>> +              <0x44011700 0x18>,
>>>>> +              <0x4009d740 0x10>,
>>>>> +              <0x44011740 0x18>,
>>>>> +              <0x44010010 0xb4>,
>>>>> +              <0x44011078 0x80>;
>>>>
>>>> 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.

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
>>>
> 
>