RE: [PATCH v2 0/3] Renesas R9A06G032 PINCTRL Driver

[Phil Edworthy <phil.edworthy@xxxxxxxxxxx>]

Hi Geert,

On 05 September 2018 10:37, Geert Uytterhoeven wrote:
> On Mon, Sep 3, 2018 at 1:03 PM Phil Edworthy wrote:
> > On 03 September 2018 11:34, jacopo mondi wrote:
> > > On Thu, Aug 30, 2018 at 02:12:52PM +0100, Phil Edworthy wrote:
> > > > This implements the pinctrl driver for the RZ/N1 family of
> > > > devices, including the R9A06G032 (RZ/N1D) device.
> > > >
> > > > One area that is likely to be contentious is the use of 'virtual
> > > > pins' for the MDIO pinmuxing. The driver uses two pins (170 and
> > > > 171) that don't exist on
> > > the
> > > > device to configure the MDIO source within the RZ/N1 devices. On
> > > > these
> > > devices,
> > > > there are two Ethernet MACs, a 5-Port Switch, numerous industrial
> > > Ethernet
> > > > peripherals, any of which can be the MDIO source. Configuring the
> > > > MDIO
> > > source
> > > > could be done without the virtual pins, e.g. by extending the
> > > > functions to cover all MDIO variants (a total of 32 additional
> > > > functions), but this would allow users to misconfigure individual
> > > > MDIO pins, rather than assign all
> > > MDIO
> > > > pins to a MDIO source. The choice of how to implement this will
> > > > affect the DT bindings.
> > > >
> > > > This series was originally written by Michel Pollet whilst at
> > > > Renesas, and I have taken over this work.
> > > >
> > > > One point from Michel's v1 series:
> > > > "Note, I used renesas,rzn1-pinmux node to specify the pinmux
> > > > constants, and I also don't use some of the properties documented
> > > > in pinctrl-bindings.txt on purpose, as they are too limited for my
> > > > use (I need to be able to set, clear, ignore or reset level, pull
> > > > up/down and function as the pinmux might be set by another OS/core
> > > > running concurently)."
> > > >
> > >
> > > I start by saying that I don't know this HW pin controller well, so
> > > I might be missing something, but as commented on the original
> > > series from Micheal, I still don't see why you need a custom property
> here...
> > >
> > > My understanding, looking at this comment and the header provided by
> > > patch [1/3] (include/dt-bindings/pinctrl/rzn1-pinctrl.h) is that
> > > basically need to control pull-up/down and the output driver strength.
> > >
> > > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > > reports a set of generic pin configuration properties to be applied
> > > to a pin configuration (and multiplexing) pin controller child node
> > > that fully express all (most?) of your needs.
> > >
> > > Eg. a pin configuration with pull up applied, using examples from
> > > your cover letter should be expressed as
> > >
> > > Your example:
> > >          &pinctrl {
> > >                  pinsuart0: pinsuart0 {
> > >                          renesas,rzn1-pinmux-ids = <
> > >                                  RZN1_MUX(103, UART0_I)  /* UART0_TXD */
> > >                                  RZN1_MUX_PUP(104, UART0_I)      /* UART0_RXD */
> > >                          >;
> > >                  };
> > >          };
> > >
> > > Using standard pinctroller bindings pin configuration properties:
> > >
> > >          &pinctrl {
> > >                  pinsuart0: uart0  {
> > >                         pinsuart_tx0 {
> > >                                 pinmux = <103, UART0_I>;  /* UART0_TXD */
> > >                         };
> > >
> > >                         pinsuart_rx0 {
> > >                                  pinmux = <104, UART0_I>; /* UART0_RXD */
> > >                                  bias-pull-up;
> > >                         };
> > >                  };
> > >          };
> > >
> > > Is there anything I am missing? Maybe from the interaction with
> > > "another OS/core running concurrently" you mentioned? In this case
> > > if you only have to perform pin configuration (because muxing is
> > > handled
> > > already) things are even simpler, just use the pin configuration
> > > bindings, without involving muxing at all:
> > >
> > >         &pinctrl {
> > >                 pinsuart_conf: uart0 {
> > >                         pins = <103, 104>;
> > >                         bias-pull-up;
> > >                  };
> > >          };
> >
> > Sorry I didn’t address your point.
> > The only reason we want to use new properties is so the driver can
> > process dts files that have been generated from an existing PinMux
> > App. That output is used by VxWorks as well as our out-of-tree Linux
> > port. If that is not a good enough reason to add new properties, then
> > I can't see any technical reason not to use the existing bindings.
> > The use with another OS running on a different core should not be a
> > barrier as it must not use the same pins as Linux.
> 
> Have the VxWorks DT bindings been submitted for review to the devicetree
> mailing list?
I'm not involved with the VxWorks port, but I am pretty sure that they have
not been submitted for review.

Thanks
Phil