Message-ID: <20240930-bamboo-curliness-eb4787b81ea3@spud>
Date: Mon, 30 Sep 2024 16:07:08 +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 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@....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 ^^^

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

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



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

Powered by blists - more mailing lists