Date:	Tue, 19 Aug 2008 11:26:28 +0200
From:	Laurent Pinchart <laurentp@...-semaphore.com>
To:	avorontsov@...mvista.com
Cc:	linuxppc-dev@...abs.org, "Greg Kroah-Hartman" <greg@...ah.com>,
	linux-usb@...r.kernel.org,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Li Yang <leoli@...escale.com>, linux-kernel@...r.kernel.org,
	Timur Tabi <timur@...escale.com>
Subject: Re: [PATCH 1/3] gpiolib: make gpio_to_chip() public

On Monday 18 August 2008, Anton Vorontsov wrote:
> On Mon, Aug 18, 2008 at 04:44:36PM +0200, Laurent Pinchart wrote:
> > On Monday 18 August 2008, Anton Vorontsov wrote:
> > > On Mon, Aug 18, 2008 at 03:58:46PM +0200, Laurent Pinchart wrote:
> > > [...]
> > > > > Not exactly. But you can do this way, if you need to preserve
> > > > > a direction. What I did is a bit different though.
> > > > > 
> > > > > qe_gpio_set_dedicated() actually just restores a mode that
> > > > > firmware had set up, including direction (since direction could
> > > > > be a part of dedicated configuration).
> > > > > 
> > > > > That is, upon GPIO controller registration, we save all registers,
> > > > > then driver can set up a pin to a GPIO mode via standard API, and
> > > > > then it can _revert_ a pin to a dedicated function via
> > > > > qe_gpio_set_dedicated() call. Dedicated function is specified by
> > > > > the firmware (or board file), we're just restoring it.
> > > > 
> > > > The semantic of the set_dedicated() operation needs to be clearly
> > > > defined then.
> > > 
> > > It is. We set up a dedicated function that firmware (or board file)
> > > has configured.
> > 
> > A comment in the source would help.
> > 
> > > > I can live with this behaviour, but it might not be
> > > > acceptable for everybody.
> > > 
> > > For example?
> > > 
> > > > Your patch requires the firmware to set a pin in dedicated mode at
> > > > bootup in order to be used later in dedicated mode.
> > > 
> > > Yes. On a PowerPC this is always true: firmware should set up PIO
> > > config. Linux' board file could fixup the firmware though.
> > 
> > That's not what I meant. What if the hardware requires to pin to be
> > configured in GPIO mode with a fixed value until the SOC-specific
> > driver that will drive the GPIO is loaded ? That's not possible
> > with your API.
> 
> Yes, this isn't possible with this API. Because you can do this
> with standard GPIO API! ;-)
> 
> Just call gpio_direction_*() in the board file, before probing the
> hardware.

You'd have to do that after the GPIO drivers saved the pin registers.

> > Until a SOC peripheral is initialized by its associated Linux driver,
> > the behaviour of a GPIO pin in dedicated mode will be undefined.
> 
> Huh?! Then all current software is simply broken: we're setting pinmux
> config _prior_ to controller initialization.

Undefined behaviour doesn't mean broken behaviour :-) Signals coming from SOC peripherals are mostly undefined until the peripheral is initialized. For most hardware that doesn't matter much, but it might in some cases. For instance the state of the RTS signal on a serial port probably doesn't matter before we start serial communication, but some boards might require that RTS is deasserted before the controller is initialized. We can just ignore the issue for now and wait until it bites us.

> > The firmware/board code will probably want to set the pin as a GPIO
> > output with a fixed value until the driver initializes the hardware.
> 
> Probably? Do you have any such hardware?

Nope. I was referring to the hardware such as in the above example.

> > > Another option would be to add some argument to the set_dedicated
> > > call, thus the software could specify arbitrary dedicated
> > > function (thus no need to save/restore anything). But this would
> > > be SOC-model specific, thus no driver can use this argument anyway.
> > 
> > Drivers that require dedicated mode are SOC-specific anyway.
> 
> I didn't say "SOC-specific". I said "SOC-model specific", which
> means that the driver would be not portable even across QE chips
> (i.e. MPC8323 vs. MPC8360, you can assume that the "CLK12" function
> is having same PAR/ODR/DAT/DIR bits).

If I'm not mistaken, the PAR bit is used to select between GPIO and dedicated mode by definition. It should be safe to assume that setting a GPIO in dedicated mode requires the PAR bit to be set. But as I stated above, we can ignore that for now until a hardware use case comes up.

> > > > If, for some
> > > > reason (driver not loaded, ...), no GPIO user shows up for that
> > > > pin, it will stay configured in dedicated mode.
> > > 
> > > Yes.
> > > 
> > > > It might be better to set the PAR bit unconditionally in
> > > 
> > > Why it might be better?
> > 
> > Because, as explained a few lines down, the board initialization code
> > will be able to configure a pin in a known state (PAR not set) at boot
> > time until a driver requests the pin to be switched to dedicated mode.
> 
> You can do this as I described above. But prior to this, yes, you have
> to configure the pins and let Linux save these values. There is no other
> way to pass this information, unfortunately.

Ok.

I started implementing CPM2 dedicated mode support to be used in the CPM UART driver for RTS/CTS flow control, and soon realized there was a show stopper. The CPM UART driver is mostly CPM1/CPM2 agnostic. I can't use a function such as cpm2_gpio32_set_dedicated, as that wouldn't work on a CPM1 (at least not on all its ports). I could call the right function depending on which port the GPIO is on, but that's really hackish and would defeat the purpose of using GPIOs. What we really need there is a set_dedicated/set_option/set_whatever function in the GPIO API :-/

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

Download attachment "signature.asc " of type "application/pgp-signature" (198 bytes)

Powered by blists - more mailing lists