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

[jacopo mondi]

Hi Phil,

On Mon, Sep 03, 2018 at 11:03:18AM +0000, Phil Edworthy wrote:
> Hi Jacopo,
>
> 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.

I see. I step back then and let this to be handled by the pinctrl
subsystem people and maintainer :)

Thanks
   j

> 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.
>
> Thanks
> Phil
>
> > Thanks
> >   j
> >
> > > Patch 0003 should really be applied after patch:
> > >  "ARM: dts: r9a06g032: Correct UART and add all other UARTs", see
> > >  https://www.spinics.net/lists/arm-kernel/msg673525.html
> > >
> > > Main changes:
> > > v2:
> > >  - Change to generic rzn1 family driver, instead of device specific.
> > >  - Review comments fixed.
> > >  - Fix error handling during probe
> > >
> > > Phil Edworthy (3):
> > >   dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation
> > >   pinctrl: renesas: Renesas RZ/N1 pinctrl driver
> > >   ARM: dts: r9a06g032: Add pinctrl node
> > >
> > >  .../bindings/pinctrl/renesas,rzn1-pinctrl.txt      |  97 +++
> > >  arch/arm/boot/dts/r9a06g032.dtsi                   |   8 +
> > >  drivers/pinctrl/Kconfig                            |  10 +
> > >  drivers/pinctrl/Makefile                           |   1 +
> > >  drivers/pinctrl/pinctrl-rzn1.c                     | 844 +++++++++++++++++++++
> > >  include/dt-bindings/pinctrl/rzn1-pinctrl.h         | 191 +++++
> > >  6 files changed, 1151 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
> > >  create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
> > >  create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h
> > >
> > > --
> > > 2.7.4
> > >

Attachment: signature.asc
Description: PGP signature