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
