Message-ID: <20240930-shortness-unedited-650f7996e912@spud>
Date: Mon, 30 Sep 2024 16:00:57 +0100
From: Conor Dooley <conor@...nel.org>
To: Andrei Stefanescu <andrei.stefanescu@....nxp.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Chester Lin <chester62515@...il.com>,
	Matthias Brugger <mbrugger@...e.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>, linux-gpio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	NXP S32 Linux Team <s32@....com>,
	Christophe Lizzi <clizzi@...hat.com>,
	Alberto Ruiz <aruizrui@...hat.com>,
	Enric Balletbo <eballetb@...hat.com>
Subject: Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP
 S32G2/S32G3 SoCs

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@....com>
> >> Signed-off-by: Larisa Grigore <larisa.grigore@....com>
> >> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> >> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@....nxp.com>
> > 
> > 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@....com>
> Signed-off-by: Phu Luu An <phu.luuan@....com>
> Co-developed-by: Larisa Grigore <larisa.grigore@....com>
> Signed-off-by: Larisa Grigore <larisa.grigore@....com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@....com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@....nxp.com>
> 
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    #include <dt-bindings/interrupt-controller/irq.h>
> >> +
> >> +    gpio@...9d700 {
> >> +        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 ^^^

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists