[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <36215390-77ec-4959-9bbc-65af32734d31@oss.nxp.com>
Date: Fri, 4 Oct 2024 14:10:07 +0300
From: Andrei Stefanescu <andrei.stefanescu@....nxp.com>
To: Conor Dooley <conor@...nel.org>
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
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@oss.nxp.com/
>> 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@...9c000 {
>> /* The registers are 32bit wide but start at offset 0x4 */
>> reg = <0x4009c000 0xc>;
>> [..]
>> };
>>
>> pinctrl-gpio@...9c010 {
>> 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@...4010000 {
>> 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@...9c000 {
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-*@...ex {
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-*@...ex {
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
Powered by blists - more mailing lists