[v4,2/2] dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

Add device tree binding documentation and header file for Renesas R7S9210
(RZ/A2) SoCs.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v3:
 - Added Reviewed-by
v2:
 * Moved gpio-controller to required
 * Wrote a better description of what the sub-nodes are for
 * Added pinmux property description
 * Changed macro RZA2_PIN_ID to RZA2_PIN
---
 .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 88 ++++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 ++++++++++++
 2 files changed, 135 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
 create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h

Hi Chris,

On Wed, Nov 7, 2018 at 7:27 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add device tree binding documentation and header file for Renesas R7S9210
> (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

> new file mode 100644
> index 000000000000..622d37a7225b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> @@ -0,0 +1,88 @@
> +Renesas RZ/A2 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> +Each port features up to 8 pins, each of them configurable for GPIO
> +function (port mode) or in alternate function mode.
> +Up to 8 different alternate function modes exist for each single pin.
> +
> +Pin controller node
> +-------------------
> +
> +Required properties:
> +  - compatible: should be:
> +    - "renesas,r7s9210-pinctrl": for RZ/A2M

On RZ/A1, the datasheet called this "Ports", and the corresponding compatible
value is "renesas,r7s72100-ports".
On RZ/A2, the datasheet calls this "GPIO", so perhaps "renesas,r7s9210-gpio"?
Hmm, then you may want to call the single node gpio-controller instead of
pin-controller (and all references to it)? It's really both.

On RZ/A1, it's different, as you have a single pin-controller node, with GPIO
subnodes.

> +  - reg
> +    Address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +  - gpio-controller
> +    This pin controller also controls pins as GPIO
> +  - #gpio-cells
> +    Must be 2
> +  - gpio-ranges
> +    Expresses the total number GPIO ports/pins in this SoC

number of

> +
> +
> +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> +
> +       pinctrl: pin-controller@fcffe000 {
> +               compatible = "renesas,r7s9210-pinctrl";
> +               reg = <0xfcffe000 0x9D1>;

0x9d1

BTW, that's a real odd number. What about rounding up to the hardware
granularity, i.e. 0x1000?

> +
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +               gpio-ranges = <&pinctrl 0 0 176>;
> +       };
> +
> +Sub-nodes
> +---------
> +
> +The child nodes of the pin controller designate pins to be used for
> +specific peripheral functions or as GPIO.
> +
> +- Pin multiplexing sub-nodes:
> +  A pin multiplexing sub-node describes how to configure a set of
> +  (or a single) pin in some desired alternate function mode.
> +  The values for the pinmux properties are a combination of port name, pin
> +  number and the desired function index. Use the RZA2_PINMUX macro located
> +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h

RZA2_PIN

> +  to express the desired port pin.
> +
> +  Required properties:
> +    - pinmux:
> +      integer array representing pin number and pin multiplexing configuration.
> +      When a pin has to be configured in alternate function mode, use this
> +      property to identify the pin by its global index, and provide its
> +      alternate function configuration number along with it.
> +      When multiple pins are required to be configured as part of the same
> +      alternate function they shall be specified as members of the same
> +      argument list of a single "pinmux" property.
> +      Helper macros to ease assembling the pin index from its position
> +      (port where it sits on and pin number) and alternate function identifier
> +      are provided by the pin controller header file at:
> +      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>

include/dt-bindings/pinctrl/r7s9210-pinctrl.h (or
<dt-bindings/pinctrl/r7s9210-pinctrl.h>)

> +      Integers values in "pinmux" argument list are assembled as:
> +      ((PORT * 8 + PIN) | MUX_FUNC << 16)

> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Defines macros and constants for Renesas RZ/A2 pin controller pin
> + * muxing functions.
> + */
> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +
> +#define RZA2_PINS_PER_PORT     8
> +
> +/* Port names as labeled in the Hardware Manual */
> +#define P0 0
> +#define P1 1
> +#define P2 2
> +#define P3 3
> +#define P4 4
> +#define P5 5
> +#define P6 6
> +#define P7 7
> +#define P8 8
> +#define P9 9
> +#define PA 10
> +#define PB 11
> +#define PC 12

This may conflict with MIPS again ;-)
Oh, you don't include the bindings header in the driver source file, and doing
so would cause issues with (previous version of) the enum...

Still, would it make sense to call these PORTx instead of Px?
The register descriptions use PORTx.<reg>.

> +#define PD 13
> +#define PE 14
> +#define PF 15
> +#define PG 16
> +#define PH 17
> +/* No I */
> +#define PJ 18
> +#define PK 19
> +#define PL 20
> +#define PM 21

There's no PM in my datasheet. Should that be JP0?
Oh, the register descriptions do use PORTM.

> +
> +/*
> + * Create the pin index from its bank and position numbers and store in
> + * the upper 8 bits the alternate function identifier

These are not really the upper 8 bits, unless your wordsize is 24-bit ;-)

"upper 16 bits", like on RZ/A1?

> + */
> +#define RZA2_PINMUX(b, p, f)   ((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))

Gr{oetje,eeting}s,

                        Geert

Hi Geert,

On Monday, November 12, 2018 1, Geert Uytterhoeven wrote:
> > +Required properties:
> > +  - compatible: should be:
> > +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> 
> On RZ/A1, the datasheet called this "Ports", and the corresponding
> compatible
> value is "renesas,r7s72100-ports".
> On RZ/A2, the datasheet calls this "GPIO", so perhaps "renesas,r7s9210-
> gpio"?
> Hmm, then you may want to call the single node gpio-controller instead of
> pin-controller (and all references to it)? It's really both.
> 
> On RZ/A1, it's different, as you have a single pin-controller node, with
> GPIO
> subnodes.

So here's where we get into the interesting discussions.

You're going off the title of the chapters in the hardware manual. But, 
I'm looking at what the IP is (and where it was uses in other SoCs).

For RZ/A1, the pin controller/GPIO is a horrible piece of HW I've never 
seen before and hope to never see again.

For RZ/A2, the controllers came from the RZ/T1. In that manual, the 
chapter was call "Multi-Function Pin Controller (MPC)" (Chapter 18). The 
GPIO was in Chapter 17 called "I/O Ports".
Then for RZ/A2, they simply combined the two chapters into one since the
hardware was also 'combined' and just picked a name "GPIO". (they 
basically copy/pasted the text from the two chapters)

So, what's the rule of naming? Does it really have to match exactly what
it says in the hardware manual?
I'm assuming an RZ/A3 would use this same pin controller.


> > +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> > +
> > +       pinctrl: pin-controller@fcffe000 {
> > +               compatible = "renesas,r7s9210-pinctrl";
> > +               reg = <0xfcffe000 0x9D1>;
> 
> 0x9d1
> 
> BTW, that's a real odd number. What about rounding up to the hardware
> granularity, i.e. 0x1000?

I remember us getting into trouble once because numbers were rounded up 
or down and then conflicted with other nodes/register addresses. So, I 
was putting number exactly as they are. But if you want, I can round it 
up.


> > +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-
> pinctrl.h
> 
> RZA2_PIN

Good catch! Thank you.



> > +      are provided by the pin controller header file at:
> > +      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>
> 
> include/dt-bindings/pinctrl/r7s9210-pinctrl.h (or
> <dt-bindings/pinctrl/r7s9210-pinctrl.h>)

Thanks.

> > +/* Port names as labeled in the Hardware Manual */
> > +#define P0 0
> > +#define P1 1
> > +#define P2 2
> > +#define P3 3
> > +#define P4 4
> > +#define P5 5
> > +#define P6 6
> > +#define P7 7
> > +#define P8 8
> > +#define P9 9
> > +#define PA 10
> > +#define PB 11
> > +#define PC 12
> 
> This may conflict with MIPS again ;-)

Damn MIPS!


> Oh, you don't include the bindings header in the driver source file, and
> doing
> so would cause issues with (previous version of) the enum...
> 
> Still, would it make sense to call these PORTx instead of Px?
> The register descriptions use PORTx.<reg>.

I liked Px because it made my lines in the device tree shorter.
But, I won't argue if you think it would be better to use PORTx (that's 
only 3 more characters).


> > +#define PM 21
> 
> There's no PM in my datasheet. Should that be JP0?
> Oh, the register descriptions do use PORTM.

Like you mentioned, they made it confusing because instead of calling 
the on pin on Port M "PM0" they called it "JP0" for 'JTAG PORT'.
From a hardware IP standpoint, it's a "Port M", so I wanted to call it 
that. I wanted to make it generic because if another SoC uses this same
controller, it might have more ports, so port M will really be a port M.


> > + * Create the pin index from its bank and position numbers and store in
> > + * the upper 8 bits the alternate function identifier
> 
> These are not really the upper 8 bits, unless your wordsize is 24-bit ;-)
> 
> "upper 16 bits", like on RZ/A1?

Opps. You're correct. Thanks!


Chris

Hi Chris,

On Tue, Nov 13, 2018 at 5:38 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Monday, November 12, 2018 1, Geert Uytterhoeven wrote:
> > > +Required properties:
> > > +  - compatible: should be:
> > > +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> >
> > On RZ/A1, the datasheet called this "Ports", and the corresponding
> > compatible
> > value is "renesas,r7s72100-ports".
> > On RZ/A2, the datasheet calls this "GPIO", so perhaps "renesas,r7s9210-
> > gpio"?
> > Hmm, then you may want to call the single node gpio-controller instead of
> > pin-controller (and all references to it)? It's really both.
> >
> > On RZ/A1, it's different, as you have a single pin-controller node, with
> > GPIO
> > subnodes.
>
> So here's where we get into the interesting discussions.
>
> You're going off the title of the chapters in the hardware manual. But,
> I'm looking at what the IP is (and where it was uses in other SoCs).
>
> For RZ/A1, the pin controller/GPIO is a horrible piece of HW I've never
> seen before and hope to never see again.
>
> For RZ/A2, the controllers came from the RZ/T1. In that manual, the
> chapter was call "Multi-Function Pin Controller (MPC)" (Chapter 18). The
> GPIO was in Chapter 17 called "I/O Ports".
> Then for RZ/A2, they simply combined the two chapters into one since the
> hardware was also 'combined' and just picked a name "GPIO". (they
> basically copy/pasted the text from the two chapters)
>
> So, what's the rule of naming? Does it really have to match exactly what
> it says in the hardware manual?
> I'm assuming an RZ/A3 would use this same pin controller.

Thanks for the explanation!

I'm fine with "renesas,r7s9210-pinctrl".

> > > +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> > > +
> > > +       pinctrl: pin-controller@fcffe000 {
> > > +               compatible = "renesas,r7s9210-pinctrl";
> > > +               reg = <0xfcffe000 0x9D1>;
> >
> > 0x9d1
> >
> > BTW, that's a real odd number. What about rounding up to the hardware
> > granularity, i.e. 0x1000?
>
> I remember us getting into trouble once because numbers were rounded up
> or down and then conflicted with other nodes/register addresses. So, I
> was putting number exactly as they are. But if you want, I can round it
> up.

If you're worried for an overlap with another node in DT, keep on using
(lower case) 0x9d1.

The mapping will be rounded up to PAGE_SIZE anyway :-)

> > > +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-
> > pinctrl.h
> >
> > RZA2_PIN
>
> Good catch! Thank you.

Note that you still do have RZA2_PIN_ID_TO_PORT() and
RZA2_PIN_ID_TO_PIN() macros in the driver.
But RZA2_PIN_TO_PIN() sounds really lame...

> > > +/* Port names as labeled in the Hardware Manual */
> > > +#define P0 0
> > > +#define P1 1
> > > +#define P2 2
> > > +#define P3 3
> > > +#define P4 4
> > > +#define P5 5
> > > +#define P6 6
> > > +#define P7 7
> > > +#define P8 8
> > > +#define P9 9
> > > +#define PA 10
> > > +#define PB 11
> > > +#define PC 12
> >
> > This may conflict with MIPS again ;-)
>
> Damn MIPS!
>
>
> > Oh, you don't include the bindings header in the driver source file, and
> > doing
> > so would cause issues with (previous version of) the enum...
> >
> > Still, would it make sense to call these PORTx instead of Px?
> > The register descriptions use PORTx.<reg>.
>
> I liked Px because it made my lines in the device tree shorter.
> But, I won't argue if you think it would be better to use PORTx (that's
> only 3 more characters).

Any preference from the DT people?

> > > +#define PM 21
> >
> > There's no PM in my datasheet. Should that be JP0?
> > Oh, the register descriptions do use PORTM.
>
> Like you mentioned, they made it confusing because instead of calling
> the on pin on Port M "PM0" they called it "JP0" for 'JTAG PORT'.
> From a hardware IP standpoint, it's a "Port M", so I wanted to call it
> that. I wanted to make it generic because if another SoC uses this same
> controller, it might have more ports, so port M will really be a port M.

OK.

Perhaps adding a convenience definition

    #define JP PM

may be a good idea? Or just a comment?

    #define PM 21        /* JP */


Anyway, we're getting closer to bikeshedding, so with the real issues fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

Gr{oetje,eeting}s,

                        Geert

Hi Geert,

On Tuesday, November 13, 2018 1, Geert Uytterhoeven wrote:
> Perhaps adding a convenience definition
> 
>     #define JP PM
> 
> may be a good idea? Or just a comment?
> 
>     #define PM 21        /* JP */

Either one is OK I guess. I'll see which one makes more sense as I 
reworked the driver.


> Anyway, we're getting closer to bikeshedding,

:)


Chris

Hi Chris,
    thanks for the patch

Just two minor things, so please add my
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>


On Wed, Nov 07, 2018 at 01:27:33PM -0500, Chris Brandt wrote:
> Add device tree binding documentation and header file for Renesas R7S9210
> (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>  - Added Reviewed-by
> v2:
>  * Moved gpio-controller to required
>  * Wrote a better description of what the sub-nodes are for
>  * Added pinmux property description
>  * Changed macro RZA2_PIN_ID to RZA2_PIN
> ---
>  .../bindings/pinctrl/renesas,rza2-pinctrl.txt      | 88 ++++++++++++++++++++++
>  include/dt-bindings/pinctrl/r7s9210-pinctrl.h      | 47 ++++++++++++
>  2 files changed, 135 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
>  create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> new file mode 100644
> index 000000000000..622d37a7225b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
> @@ -0,0 +1,88 @@
> +Renesas RZ/A2 combined Pin and GPIO controller
> +
> +The Renesas SoCs of the RZ/A2 series feature a combined Pin and GPIO controller.
> +Pin multiplexing and GPIO configuration is performed on a per-pin basis.
> +Each port features up to 8 pins, each of them configurable for GPIO
> +function (port mode) or in alternate function mode.
> +Up to 8 different alternate function modes exist for each single pin.
> +
> +Pin controller node
> +-------------------
> +
> +Required properties:
> +  - compatible: should be:

s/should/shall ?

> +    - "renesas,r7s9210-pinctrl": for RZ/A2M
> +  - reg
> +    Address base and length of the memory area where the pin controller
> +    hardware is mapped to.
> +  - gpio-controller
> +    This pin controller also controls pins as GPIO
> +  - #gpio-cells
> +    Must be 2
> +  - gpio-ranges
> +    Expresses the total number GPIO ports/pins in this SoC
> +
> +

Two empty lines.

Thanks
   j

> +Example: Pin controller node for RZ/A2M SoC (r7s9210)
> +
> +	pinctrl: pin-controller@fcffe000 {
> +		compatible = "renesas,r7s9210-pinctrl";
> +		reg = <0xfcffe000 0x9D1>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-ranges = <&pinctrl 0 0 176>;
> +	};
> +
> +Sub-nodes
> +---------
> +
> +The child nodes of the pin controller designate pins to be used for
> +specific peripheral functions or as GPIO.
> +
> +- Pin multiplexing sub-nodes:
> +  A pin multiplexing sub-node describes how to configure a set of
> +  (or a single) pin in some desired alternate function mode.
> +  The values for the pinmux properties are a combination of port name, pin
> +  number and the desired function index. Use the RZA2_PINMUX macro located
> +  in include/dt-bindings/pinctrl/r7s9210-pinctrl.h to easily define these.
> +  For assigning GPIO pins, use the macro RZA2_PIN_ID also in r7s9210-pinctrl.h
> +  to express the desired port pin.
> +
> +  Required properties:
> +    - pinmux:
> +      integer array representing pin number and pin multiplexing configuration.
> +      When a pin has to be configured in alternate function mode, use this
> +      property to identify the pin by its global index, and provide its
> +      alternate function configuration number along with it.
> +      When multiple pins are required to be configured as part of the same
> +      alternate function they shall be specified as members of the same
> +      argument list of a single "pinmux" property.
> +      Helper macros to ease assembling the pin index from its position
> +      (port where it sits on and pin number) and alternate function identifier
> +      are provided by the pin controller header file at:
> +      <include/dt-bindings/pinctrl/r7s9210-pinctrl.h>
> +      Integers values in "pinmux" argument list are assembled as:
> +      ((PORT * 8 + PIN) | MUX_FUNC << 16)
> +
> +  Example: Board specific pins configuration
> +
> +	&pinctrl {
> +		/* Serial Console */
> +		scif4_pins: serial4 {
> +			pinmux = <RZA2_PINMUX(P9, 0, 4)>,	/* TxD4 */
> +				 <RZA2_PINMUX(P9, 1, 4)>;	/* RxD4 */
> +		};
> +	};
> +
> +  Example: Assigning a GPIO:
> +
> +	leds {
> +		status = "okay";
> +		compatible = "gpio-leds";
> +
> +		led0 {
> +			/* P6_0 */
> +			gpios = <&pinctrl RZA2_PIN(P6, 0) GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> diff --git a/include/dt-bindings/pinctrl/r7s9210-pinctrl.h b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> new file mode 100644
> index 000000000000..1e2671b61c0a
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s9210-pinctrl.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Defines macros and constants for Renesas RZ/A2 pin controller pin
> + * muxing functions.
> + */
> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H
> +
> +#define RZA2_PINS_PER_PORT	8
> +
> +/* Port names as labeled in the Hardware Manual */
> +#define P0 0
> +#define P1 1
> +#define P2 2
> +#define P3 3
> +#define P4 4
> +#define P5 5
> +#define P6 6
> +#define P7 7
> +#define P8 8
> +#define P9 9
> +#define PA 10
> +#define PB 11
> +#define PC 12
> +#define PD 13
> +#define PE 14
> +#define PF 15
> +#define PG 16
> +#define PH 17
> +/* No I */
> +#define PJ 18
> +#define PK 19
> +#define PL 20
> +#define PM 21
> +
> +/*
> + * Create the pin index from its bank and position numbers and store in
> + * the upper 8 bits the alternate function identifier
> + */
> +#define RZA2_PINMUX(b, p, f)	((b) * RZA2_PINS_PER_PORT + (p) | (f << 16))
> +
> +/*
> + * Convert a port and pin label to its global pin index
> + */
> + #define RZA2_PIN(port, pin)	((port) * RZA2_PINS_PER_PORT + (pin))
> +
> +#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA2_H */
> --
> 2.16.1
>

On Tuesday, November 13, 2018, jacopo mondi wrote:
> Just two minor things, so please add my
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks Jacopo!

Chris

On Tue, Nov 13, 2018 at 6:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Anyway, we're getting closer to bikeshedding, so with the real issues fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I assume you are queueing the RZ/A2 stuff as well Geert?
Acked-by: Linus Walleij <linus.walleij@linaro.org>
FWIW, I trust Geert to have the final say on Renesas code.

Yours,
Linus Walleij

Hi Linus,

On Mon, Nov 19, 2018 at 2:22 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Nov 13, 2018 at 6:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Anyway, we're getting closer to bikeshedding, so with the real issues fixed:
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I assume you are queueing the RZ/A2 stuff as well Geert?

Yes, that's the plan...

> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> FWIW, I trust Geert to have the final say on Renesas code.

Thanks!

Gr{oetje,eeting}s,

                        Geert