From: Chris Brandt <Chris.Brandt@renesas.com>
To: Linus Walleij <linus.walleij@linaro.org>
CC: Jacopo Mondi <jacopo+renesas@jmondi.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
        Russell King <linux@armlinux.org.uk>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl
 header
Thread-Topic: [PATCH v3 3/7] arm: dts: dt-bindings: Add Renesas RZ pinctrl
 header
Thread-Index: AQHSpLJ6VNE1pFUaX0a1Hx3QR4RDhaGr1ZuAgAAPSICAAS0xgIAARUCA
Date: Thu, 30 Mar 2017 13:27:31 +0000
Message-ID: <SG2PR06MB116501FDC518BB49630520588A340@SG2PR06MB1165.apcprd06.prod.outlook.com>
References: <1490368934-12494-1-git-send-email-jacopo+renesas@jmondi.org>
 <1490368934-12494-4-git-send-email-jacopo+renesas@jmondi.org>
 <CACRpkdZ4Z6c2YDu-e6kZ8-K7Dv_tRGxZj0vAqgPBx7rWwmHU-g@mail.gmail.com>
 <SG2PR06MB11659AB956F83E8DCE2C9AA18A350@SG2PR06MB1165.apcprd06.prod.outlook.com>
 <CACRpkda_Gw9vhLqpWw8oOZyZ59nj5j9Q-T5RXVsf0vsxWcjyHg@mail.gmail.com>
In-Reply-To: <CACRpkda_Gw9vhLqpWw8oOZyZ59nj5j9Q-T5RXVsf0vsxWcjyHg@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Mar 2017 13:27:31.5298
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 53d82571-da19-47e4-9cb4-625a166a4a2a
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SG2PR06MB1168
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v2UDRnAc003385
Content-Length: 4289
Lines: 94

On Thursday, March 30, 2017, Linus Walleij wrote:
> >> > +/*
> >> > + * Pin is bi-directional.
> >> > + * An alternate function that needs both input/output
> >> > +functionalities shall
> >> > + * be configured as bidirectional.
> >> > + * Eg. SDA/SCL pins of an I2c interface.
> >> > + */
> >> > +#define BI_DIR                 (1 << 3)
> >>
> >> Any specific reason why this should not simply be added to
> >> include/linux/pinctrl/pinconf-generic.h
> >> as PIN_CONFIG_BIDIRECTIONAL and parsed in drivers/pinctrl/pinconf-
> >> generic.c from the (new) DT property "bidirectional" simply?
> >
> > I see your point. It would cut down from every driver out there
> > inventing some new property or config instead of everyone just sharing
> > a fixed set.
> > Maybe someone else out there will end up having a need for a
> > "bidirectional" option.
> 
> I was thinking about that one. It is a bit weird electrically, like what
> kind of electronics is really bidirectional?
> 
> It seems like a fancy name for open drain/open source, what we call
> "single ended" configuration. (See docs in
> Documentation/gpio/driver.txt)
> 
> It would be great if you could check if that is what they mean, actually.

Here is the definition of the register in the hardware manual:

---
54.3.13 Port Bidirection Control Register (PBDCn)

This register enables or disables the input buffer while the output buffer is enabled. When the input buffer is enabled
while the output buffer is enabled, the bidirectional mode is entered, allowing the level of the Pn_m pin to always be read
via the PPRn.PPRnm bit.
---


But...what they don't really tell you is that any peripheral that needs to TX and RX data over a pin needs this set.


In the hardware manual, there is a pretty picture (Figure 54.1 Logical Diagram of Port Control) that shows a detailed logic diagram of the interface between the peripheral bus and the pin pad.
As far as I can tell, the "function mux" portion of this controller only knows how to set a pin as direction=in or direction=out.
So, in the case of I2C where each the SCL and SDA pins needs to be driven and read...it can't do that.
Therefore, there is an additional register to manually enable the input buffer and let the mux enable the output buffer.

So while yes, I2C is open-drain, this is also the case for the data pins for the SDHI. And the MDIO pin from Ethernet. It really has to do with the fact that this pin controller wasn't designed to enable both the input and output buffers at the same time.

The situation is similar for our SWIO_IN and SWIO_OUT flags. For example, to use the SSI sound interface, you have to set the TX signals to "input" (SWIO_IN). Makes sense, right??

I could argue that all of these "FLAGS" settings should have been incorporated in the HW logic of the pin controller...but for whatever reason, the they had to make them separate registers and make SW do it.
So, I could argue that these registers settings are really part of pin muxing, not really user config....and hence belong in the "pinmux" property.


How about this compromise: Instead of BI_DIR, we use "TYPE_I2C", "TYPE_SDDATA", "TYPE_MDIO", "TYPE_LVDS", etc... to describe the 'special' ones. That way it can go back under "pinmux". Like Jacopo said, these register settings are really supposed to be set when you are selecting the pin-mux option.


Of course behind the scenes, it's really:
  #define TYPE_I2C    BI_DIR 
  #define TYPE_SDDATA BI_DIR
  #define TYPE_SDCMD  BI_DIR
  #define TYPE_LVDS   SWIO_OUT

Examples:

	i2c3_pins: i2c3 {
		pinmux = <PIN(1, 6) | FUNC_1 | TYPE_I2C>,
		         <PIN(1, 7) | FUNC_1 | TYPE_I2C>;
	};

	/* SHDI ch1 on CN1 */
	sdhi1_pins: sdhi1 {
		/* SHDI ch1 on Port 3 */
		pinmux = <PIN(3, 8) | FUNC_7>,			/* SDHI1 CD */
				<PIN(3, 9) FUNC_7)>,			/* SDHI1 WP */
				<PIN(3, 10) FUNC_7 | TYPE_SDDATA)>,	/* SDHI1 DAT1 */
				<PIN(3, 11) FUNC_7 | TYPE_SDDATA)>,	/* SDHI1 DAT0 */
				<PIN(3, 12) FUNC_7)>,			/* SDHI1 CLK */
				<PIN(3, 13) FUNC_7 | TYPE_SDCMD)>,	/* SDHI1 CMD */
				<PIN(3, 14) FUNC_7 | TYPE_SDDATA)>,	/* SDHI1 DAT3 */
				<PIN(3, 15) FUNC_7 | TYPE_SDDATA)>;	/* SDHI1 DAT2 */
	};


# Honestly, I have no idea where this pin controller came from. I have not seen it in any other Renesas part (Mitsubishi/Hitachi/NEC).


Chris