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

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

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