| Message ID | 20221031100843.14579-1-clin@suse.com |
|---|---|
| Headers | show |
| Series | Add pinctrl support for S32 SoC family | expand |
Hi Rob, Thank you for reviewing this patch. On Wed, Nov 02, 2022 at 10:49:03AM -0500, Rob Herring wrote: > On Mon, Oct 31, 2022 at 06:08:42PM +0800, Chester Lin wrote: > > Add DT schema for the pinctrl driver of NXP S32 SoC family. > > > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> > > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml | 91 +++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml > > new file mode 100644 > > index 000000000000..eafb9091cbf7 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml > > @@ -0,0 +1,91 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (c) 2022 NXP > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: NXP S32 Common Chassis SIUL2 iomux controller > > + > > +maintainers: > > + - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> > > + - Chester Lin <clin@suse.com> > > + > > +description: | > > + Core driver for the pin controller found on S32 Common Chassis SoC. > > + > > +properties: > > + compatible: > > + enum: > > + - nxp,s32g-siul2-pinctrl > > + > > + reg: > > + minItems: 5 > > + maxItems: 6 > > + description: A list of register regions to be reserved. > > Need to be explicit about what each entry is. > They are - MSCR registers group 0 managed by the SIUL2 controller #0 - MSCR registers group 1 managed by the SIUL2 controller #1 - MSCR registers group 2 managed by the SIUL2 controller #1 - IMCR registers group 0 managed by the SIUL2 controller #0 - IMCR registers group 1 managed by the SIUL2 controller #1 - IMCR registers group 2 managed by the SIUL2 controller #1 - MSCR: Multiplexed Signal Configuration Register An MSCR register can configure the associated pin as either a GPIO pin or a function output pin depends on the selected signal source. - IMCR: Input Multiplexed Signal Configuration Register An IMCR register can configure the associated pin as function input pin depends on the selected signal source. I will add descriptions for each entry in v2, thanks. > > + > > + nxp,pins: > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > + description: > > + A list of [start, end] pin ID boundaries that correspond to each of > > + the register regions reserved. > > Looks like a matrix rather than an array. > > > + > > +required: > > + - compatible > > + - reg > > + - nxp,pins > > + > > +patternProperties: > > + '_pins$': > > s/_/-/ > > > + type: object > > additionalProperties: false > > (and a blank line after) > > > + patternProperties: > > + '_grp[0-9]$': > > s/_/-/ > > > + type: object > > + allOf: > > + - $ref: pinmux-node.yaml# > > + - $ref: pincfg-node.yaml# > > unevaluatedProperties: false > > > + description: > > + Pinctrl node's client devices specify pin muxes using subnodes, > > + which in turn use the standard properties below. > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + > > + pinctrl: siul2-pinctrl@4009c240 { > > pinctrl@... > > > + compatible = "nxp,s32g-siul2-pinctrl"; > > + > > + /* MSCR range */ > > + reg = <0x4009c240 0x198>, > > + <0x44010400 0x2c>, > > + <0x44010480 0xbc>, > > + /* MSCR range */ It's IMCR range but not the 2nd MSCR range, will fix it in v2. > > + <0x4009ca40 0x150>, > > + <0x44010c1c 0x45c>, > > + <0x440110f8 0x108>; > > What is in these holes in the memory map? Is this part of some larger > block? If so, that block needs to be described. > In this case S32G contains two SIUL2 controllers, which are located in two different memory regions, siul2_0@4009C000 and siul2_1@44010000. The siul2_0 contains the MSCR0-MSCR101 [starts at 0x4009c240] and IMCR0-IMCR83 [starts at 0x4009ca40] registers, and there are some other reserved regions and registers which are not related to pin muxing in siul2 memory ranges. Not sure what the original reason it could be, but the expanded MSCR112-MSCR122, MSCR144 -MSCR190 and IMCR119-IMCR397, IMCR430-IMCR495 registers are present in siul2_1's memory space. Besides, in the S32G pin map, some indexes are hidden since they're not used as pinouts, such as MSCR indexes 102-111, 123-143, and IMCR indexes 84-118, 398-429. Anyway, that's why they look so fragmented. I will add more descriptions for each entry in v2. Thanks, Chester > > + > > + /* MSCR range */ > > + nxp,pins = <0 101>, > > + <112 122>, > > + <144 190>, > > + /* IMCR range */ > > + <512 595>, > > + <631 909>, > > + <942 1007>; > > + > > + llce_can0_pins { > > + llce_can0_grp0 { > > + pinmux = <0x2b0>; > > + input-enable; > > + slew-rate = <0x00>; > > + }; > > + > > + llce_can0_grp1 { > > + pinmux = <0x2c2>; > > + output-enable; > > + slew-rate = <0x00>; > > + }; > > + }; > > + }; > > +... > > -- > > 2.37.3 > > > >
Hi Linus, On Tue, Nov 08, 2022 at 01:31:16PM +0100, Linus Walleij wrote: > Hi Chester, > > thanks for your patch! > Thanks for reviewing this patch series as well! > On Mon, Oct 31, 2022 at 11:09 AM Chester Lin <clin@suse.com> wrote: > > > Add DT schema for the pinctrl driver of NXP S32 SoC family. > > > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> > > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> > > Signed-off-by: Chester Lin <clin@suse.com> > (...) > > + nxp,pins: > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > + description: > > + A list of [start, end] pin ID boundaries that correspond to each of > > + the register regions reserved. > > That's an interesting approach. > > But what about just hardcoding this into the driver instead? > > If you have the compatible, surely you know these indexes from > that compatible string? > The nxp,pins property is more like a common language that both kernel and u-boot can share with. Of course hardcoding indexes can be achieved as well but that also means the index matrices in both kernel driver and u-boot driver must be revised and synchronized if new boards may have different reg and nxp,pins. IMHO, having these ID ranges in DT could be more intuitive, developers can easily compare every pair with the corresponding reg entry in the same dts file since reg maps are fragmented as I mentioned in Rob's code review[1]. Based on this approach, both kernel driver and u-boot driver can reuse the same parser to acquire pin ID ranges from different boards, which can also simplify codes and have less patches in the future. Regards, Chester [1] https://lkml.org/lkml/2022/11/9/722 > Yours, > Linus Walleij
On Wed, Nov 9, 2022 at 5:45 PM Chester Lin <clin@suse.com> wrote: > > If you have the compatible, surely you know these indexes from > > that compatible string? > > The nxp,pins property is more like a common language that both kernel and > u-boot can share with. Of course hardcoding indexes can be achieved as well > but that also means the index matrices in both kernel driver and u-boot driver > must be revised and synchronized if new boards may have different reg and > nxp,pins. Is it new *BOARDS* that need a new table really or new *SOC:s*? I was under the impression that this is per-soc and then it can be derived from the compatible string and should be in the driver. If it is something that vary from board to board then it should be in the device tree because it can not be derived from the version of the hardware i.e the compatible string. So which one is it? Yours, Linus Walleij
Hi Chester and Linus, > > > If you have the compatible, surely you know these indexes from that > > > compatible string? > > > > The nxp,pins property is more like a common language that both kernel > > and u-boot can share with. Of course hardcoding indexes can be > > achieved as well but that also means the index matrices in both kernel > > driver and u-boot driver must be revised and synchronized if new > > boards may have different reg and nxp,pins. > > Is it new *BOARDS* that need a new table really or new *SOC:s*? > > I was under the impression that this is per-soc and then it can be derived from the compatible string and should be in the driver. Yes, the "nxp,pins" property only depends on the SoC. It will not change from board to board(boards using the same SoC). It can be derived from the compatible string. Best regards, Andrei