diff mbox

[1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

Message ID 1329719263-18971-1-git-send-email-swarren@nvidia.com
State Superseded, archived
Headers show

Commit Message

Stephen Warren Feb. 20, 2012, 6:27 a.m. UTC
Update gpio.txt based on recent discussions regarding interaction with the
pinctrl subsystem.

Previously, gpio_request() was described as explicitly not performing any
required mux setup operations etc.

Now, gpio_request() is explicitly as explicitly performing any required mux
setup operations where possible. In the case it isn't, platform code is
required to have set up any required muxing or other configuration prior to
gpio_request() being called, in order to maintain the same semantics.

This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
their .request() operation.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 Documentation/gpio.txt |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

Comments

Russell King - ARM Linux Feb. 20, 2012, 7:39 a.m. UTC | #1
On Sun, Feb 19, 2012 at 11:27:42PM -0700, Stephen Warren wrote:
> Update gpio.txt based on recent discussions regarding interaction with the
> pinctrl subsystem.
> 
> Previously, gpio_request() was described as explicitly not performing any
> required mux setup operations etc.
> 
> Now, gpio_request() is explicitly as explicitly performing any required mux
> setup operations where possible. In the case it isn't, platform code is
> required to have set up any required muxing or other configuration prior to
> gpio_request() being called, in order to maintain the same semantics.

So what if you need to have the pin as a GPIO, manipulate it as a GPIO,
and then hand it off to a special function, and then take it back as
a GPIO before you shut the special function down ?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 21, 2012, 10:41 a.m. UTC | #2
On Mon, Feb 20, 2012 at 8:39 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Feb 19, 2012 at 11:27:42PM -0700, Stephen Warren wrote:
>> Update gpio.txt based on recent discussions regarding interaction with the
>> pinctrl subsystem.
>>
>> Previously, gpio_request() was described as explicitly not performing any
>> required mux setup operations etc.
>>
>> Now, gpio_request() is explicitly as explicitly performing any required mux
>> setup operations where possible. In the case it isn't, platform code is
>> required to have set up any required muxing or other configuration prior to
>> gpio_request() being called, in order to maintain the same semantics.
>
> So what if you need to have the pin as a GPIO, manipulate it as a GPIO,
> and then hand it off to a special function, and then take it back as
> a GPIO before you shut the special function down ?

I remember this case very well and we designed for it, so it should be handled
by pin control and GPIO thusly:

Example: use pins 1,2 as I2C, then convert them to GPIO for a while
then back again:

// This call looks up a map containing pins 1,2 and reserve them
p = pinctrl_get(dev, "i2c");
if (IS_ERR(p))
  ...
pinctrl_enable(p);
pinctrl_disable(p);
// This will free up the pins again
pinctrl_put(p);
// So now we can do this...
// NB: the GPIO driver calls pinctr_request_gpio() to check
// that it can take these pins
gpio_request(1, "gpio1"):
gpio_request(2, "gpio2");
// This will trigger a reset or something
gpio_direction_output(1, 1);
gpio_direction_output(2, 1);
// Release pins again
gpio_free(1);
gpio_free(2);
// Take them back for this function
p = pinctrl_get(dev, "i2c");

It's a bit kludgy but works and makes sure the pins are only used
for one thing at a time.

BTW: Russell, which specific platform and driver was it that had this
usecase? I'd like to have a look at the code to educate myself.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 21, 2012, 10:46 a.m. UTC | #3
On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:

> Update gpio.txt based on recent discussions regarding interaction with the
> pinctrl subsystem.
>
> Previously, gpio_request() was described as explicitly not performing any
> required mux setup operations etc.
>
> Now, gpio_request() is explicitly as explicitly performing any required mux
> setup operations where possible. In the case it isn't, platform code is
> required to have set up any required muxing or other configuration prior to
> gpio_request() being called, in order to maintain the same semantics.
>
> This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
> their .request() operation.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Grant can you take this one? I'd prefer for you to have a look at
it as well.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Feb. 21, 2012, 11:06 a.m. UTC | #4
On Tue, Feb 21, 2012 at 11:41:31AM +0100, Linus Walleij wrote:
> I remember this case very well and we designed for it, so it should
> be handled by pin control and GPIO thusly:
> 
> Example: use pins 1,2 as I2C, then convert them to GPIO for a while
> then back again:
> 
> // This call looks up a map containing pins 1,2 and reserve them
> p = pinctrl_get(dev, "i2c");
> if (IS_ERR(p))
>   ...
> pinctrl_enable(p);
> pinctrl_disable(p);
> // This will free up the pins again
> pinctrl_put(p);
> // So now we can do this...
> // NB: the GPIO driver calls pinctr_request_gpio() to check
> // that it can take these pins
> gpio_request(1, "gpio1"):
> gpio_request(2, "gpio2");
> // This will trigger a reset or something
> gpio_direction_output(1, 1);
> gpio_direction_output(2, 1);
> // Release pins again
> gpio_free(1);
> gpio_free(2);
> // Take them back for this function
> p = pinctrl_get(dev, "i2c");
> 
> It's a bit kludgy but works and makes sure the pins are only used
> for one thing at a time.

No.  The case which I'm thinking of is for the Assabet audio, where
we have the following situation.

We have the SSP bus [TXD,RXD,SFRM,SCLK] which is handled by the SSP core.
This is connected through a FPGA to a UDA1341 codec.  The UDA1341 codec
has DI,DO,WS,BCLK signals.  WS (which is effectively the LRCK and therefore
identifies the left and right channels) is connected through a flip-flop
in the FPGA which toggles at every SFRM pulse.

WS is held high for the left sample.  This is the default setting.
Unfortunately, some FPGA builds out there do not tristate the WS output
when the codec is powered off.

This leads to a high leakage current from the FPGA, into the UDA1341 codec
and into other devices.

The solution is to wait for SSP to finish, and while SSP is still enabled,
switch the pins from the SSP block to the GPIO block and toggle the SFRM
signal to flip the WS output before removing power.  When starting up,
with the pins under control of GPIO, power up and them toggle hte SFRM
signal to restore the WS state before giving it back to the SSP block.

However, here's the thing: SSP must be enabled at the point in time when
the pins are given or taken away from it, otherwise SFRM is indeterminant.
We can't allow gpio_request() to fail at the points where we toggle this
pin - failure is not really an option where causing hardware stress is
involved.  We can't allow the GPIO block to have the state of these pins
change state while the pins are given back to the GPIO block before the
GPIOs have been requested (glitches on the SFRM line will cause the state
of the FIFO to change.)

Another case where this kind of run-time switching needs to occur is the
PXA IrDA driver, where the pins need to be switched between their FIR and
SIR modes.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 21, 2012, 12:40 p.m. UTC | #5
On Tue, Feb 21, 2012 at 12:06 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> It's a bit kludgy but works and makes sure the pins are only used
>> for one thing at a time.
>
> No.  The case which I'm thinking of is for the Assabet audio, where
> we have the following situation.

OK... (thanks for the explanation!)

> The solution is to wait for SSP to finish, and while SSP is still enabled,
> switch the pins from the SSP block to the GPIO block and toggle the SFRM
> signal to flip the WS output before removing power.  When starting up,
> with the pins under control of GPIO, power up and them toggle hte SFRM
> signal to restore the WS state before giving it back to the SSP block.
>
> However, here's the thing: SSP must be enabled at the point in time when
> the pins are given or taken away from it, otherwise SFRM is indeterminant.
> We can't allow gpio_request() to fail at the points where we toggle this
> pin - failure is not really an option where causing hardware stress is
> involved.  We can't allow the GPIO block to have the state of these pins
> change state while the pins are given back to the GPIO block before the
> GPIOs have been requested (glitches on the SFRM line will cause the state
> of the FIFO to change.)

This can probably be done, mainly I'd say that control over the pins
need not mean that they are decoupled from some specific hardware, or
that the hardware is disabled or so, the semantics of doing say
pinctrl_get/enable/disable/put aren't set in stone, it's just callbacks into
the device driver so it could avoid doing anything in this case, keep
the pins muxed for the SSP if the transition goes from SSP->GPIO
and back.

However I guess it would be conceptually closer if say the pins could
be used for a device and GPIO at the *same time*.

I had this similar example where a pin on U300 could be in GPIO mode
and "spy" on the peripheral using that pin, by just sampling the value
or triggering on edge transitions.

Since each pin has a pin descriptor, we could solve this by marking
pins as "gpio dualmode", i.e. add some bool dualmode to the
descriptor and augment the core to allow pins with this property to be in
GPIO and "for device X" mode at the same time. So in
pin_request() in pinmux.c we check for this property and allow
dual-mode on a case-by-case basis (assuming this will be pretty rare,
so the default is not to use it).

Of course it assumes the SA1100 being converted to use pin control,
I looked at it a bit and it seems simple enough since the GAFR
register is a single "GPIO or something else"-switch for the GPIOs.
(It'd probably need the SA1100 to be a bit more strict in using
gpiolib in place for the direct assignments though, else the
abstractions get a bit pointless anyway.)

> Another case where this kind of run-time switching needs to occur is the
> PXA IrDA driver, where the pins need to be switched between their FIR and
> SIR modes.

This sounds more mutually exclusive and should be possible to support
with a get/enable/disable/put sequence AFAICT.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Feb. 21, 2012, 12:44 p.m. UTC | #6
On Tue, Feb 21, 2012 at 01:40:05PM +0100, Linus Walleij wrote:
> Of course it assumes the SA1100 being converted to use pin control,
> I looked at it a bit and it seems simple enough since the GAFR
> register is a single "GPIO or something else"-switch for the GPIOs.
> (It'd probably need the SA1100 to be a bit more strict in using
> gpiolib in place for the direct assignments though, else the
> abstractions get a bit pointless anyway.)

That's mostly happened through my recent set of 100 or so patches.
There's a few areas where that's not quite as easy as it should be,
but on the whole, it's mostly complete.

The other thing I forgot to mention, and I suspect it's particular to
SA11x0, is that the GPDR must be set correctly according to the special
function as well as GAFR.  So, if a special function involves driving
a pin, the pin must be set as an output in GPDR.  Conversely, if the
special function involves input only, the pin must be set as an input
in GPDR.

So, on SA11x0, gpio and pin configuration are intimately linked.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Feb. 21, 2012, 1:02 p.m. UTC | #7
On Tue, Feb 21, 2012 at 12:44:09PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2012 at 01:40:05PM +0100, Linus Walleij wrote:
> > Of course it assumes the SA1100 being converted to use pin control,
> > I looked at it a bit and it seems simple enough since the GAFR
> > register is a single "GPIO or something else"-switch for the GPIOs.
> > (It'd probably need the SA1100 to be a bit more strict in using
> > gpiolib in place for the direct assignments though, else the
> > abstractions get a bit pointless anyway.)
> 
> That's mostly happened through my recent set of 100 or so patches.
> There's a few areas where that's not quite as easy as it should be,
> but on the whole, it's mostly complete.
> 
> The other thing I forgot to mention, and I suspect it's particular to
> SA11x0, is that the GPDR must be set correctly according to the special
> function as well as GAFR.  So, if a special function involves driving
> a pin, the pin must be set as an output in GPDR.  Conversely, if the
> special function involves input only, the pin must be set as an input
> in GPDR.
> 
> So, on SA11x0, gpio and pin configuration are intimately linked.

I should have added - the only places which directly accesses one of the
GPDR/GPSR/GPCR registers are:

drivers/pcmcia/sa1100_shannon.c:        unsigned long levels = GPLR;
drivers/video/sa1100fb.c:           GPDR |= mask;
drivers/input/touchscreen/jornada720_ts.c:      if (GPLR & GPIO_GPIO(9)) {
drivers/input/touchscreen/h3600_ts_input.c:     int down = (GPLR & GPIO_BITSY_ACTION_BUTTON) ? 0 : 1;
drivers/input/touchscreen/h3600_ts_input.c:     int down = (GPLR & GPIO_BITSY_NPOWER_BUTTON) ? 0 : 1;

The shannon thing looks like a bug in my PCMCIA patch series - as soc_common
now deals with GPIOs itself (which I've now fixed.)

The sa1100fb thing is a case of what I described above (correctly
configuring the direction for the pins for the special function in use.)

The touchscreen stuff needs someone who knows that stuff to fix it -
I think the jornada folk have been around recently so maybe they can
look at their driver.

The h3600 ts stuff also looks fairly easy to convert to gpiolib if
someone has the time.  Again, maybe if there's an interested party
with a device that they could test, it could happen.

All other cases of direct GPDR/GPLR/GPSR/GPCR access are in platform
initialization code in arch/arm/mach-sa1100.  So, we're actually very
close to having sa11x0 fully converted to gpiolib.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 21, 2012, 1:08 p.m. UTC | #8
On Tue, Feb 21, 2012 at 1:44 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> (It'd probably need the SA1100 to be a bit more strict in using
>> gpiolib in place for the direct assignments though, else the
>> abstractions get a bit pointless anyway.)
>
> That's mostly happened through my recent set of 100 or so patches.
> There's a few areas where that's not quite as easy as it should be,
> but on the whole, it's mostly complete.

Excellent!

> The other thing I forgot to mention, and I suspect it's particular to
> SA11x0, is that the GPDR must be set correctly according to the special
> function as well as GAFR.  So, if a special function involves driving
> a pin, the pin must be set as an output in GPDR.  Conversely, if the
> special function involves input only, the pin must be set as an input
> in GPDR.
>
> So, on SA11x0, gpio and pin configuration are intimately linked.

It's quite common I think, many platforms have an intimate
relation between GPIO and muxes/altfunctions. For example
it is common that the hardware engineer doesn't helpfully
enable on-die pull-ups on the I2C bus even though the I2C
block is muxed in, you have to go in and set the pull-up bits
separately from muxing the I2C in...

Basically it's expected from a generic pad I/O cell being
arrayed into a GPIO block to expose these things in the same
set of registers.

I made some presentation last week partly describing how
some hardware engineers I know go about creating GPIO
controllers from simpler I/O pad cells:
http://www.df.lth.se/~triad/papers/pincontrol.pdf

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 21, 2012, 6:50 p.m. UTC | #9
Linus Walleij wrote at Tuesday, February 21, 2012 3:42 AM:
> On Mon, Feb 20, 2012 at 8:39 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Feb 19, 2012 at 11:27:42PM -0700, Stephen Warren wrote:
> >> Update gpio.txt based on recent discussions regarding interaction with the
> >> pinctrl subsystem.
> >>
> >> Previously, gpio_request() was described as explicitly not performing any
> >> required mux setup operations etc.
> >>
> >> Now, gpio_request() is explicitly as explicitly performing any required mux
> >> setup operations where possible. In the case it isn't, platform code is
> >> required to have set up any required muxing or other configuration prior to
> >> gpio_request() being called, in order to maintain the same semantics.
> >
> > So what if you need to have the pin as a GPIO, manipulate it as a GPIO,
> > and then hand it off to a special function, and then take it back as
> > a GPIO before you shut the special function down ?
> 
> I remember this case very well and we designed for it, so it should be handled
> by pin control and GPIO thusly:
> 
> Example: use pins 1,2 as I2C, then convert them to GPIO for a while
> then back again:

The code below doesn't necessarily do exactly what Russell needs...

> // This call looks up a map containing pins 1,2 and reserve them
> p = pinctrl_get(dev, "i2c");
> if (IS_ERR(p))
>   ...
> pinctrl_enable(p);
> pinctrl_disable(p);

Here, the pinctrl core will call the pinctrl driver's pinmux_ops.free()
function. The whole point of this function is to change the pin's mux
selection to something that is guaranteed not to conflict with any other
pin's mux setting.

Now, if the HW only allows each signal to be routed to a specific pin
(or not routed), then free() might be a no-op.

However, if the HW allows the I2C module's signals to be routed to pins
A+B or X+Y, then free() on pins A/B would need to reprogram the mux to
route something else to pins A+B (or disable those pins or whatever the
HW needs) so that if I2C was later selected on pins X+Y, there would be
no conflict; it wouldn't be the case that both pins A+B and X+Y had I2C
mux'd on to them.

Hence, in general at least, pinctrl_disable() might glitch the output
signals.

As far as how to fix this; see my future responses to your future emails
:-)

> // This will free up the pins again
> pinctrl_put(p);
> // So now we can do this...
> // NB: the GPIO driver calls pinctr_request_gpio() to check
> // that it can take these pins
> gpio_request(1, "gpio1"):
> gpio_request(2, "gpio2");
> // This will trigger a reset or something
> gpio_direction_output(1, 1);
> gpio_direction_output(2, 1);
> // Release pins again
> gpio_free(1);
> gpio_free(2);
> // Take them back for this function
> p = pinctrl_get(dev, "i2c");
...
Stephen Warren Feb. 21, 2012, 7:14 p.m. UTC | #10
Linus Walleij wrote at Tuesday, February 21, 2012 5:40 AM:
> On Tue, Feb 21, 2012 at 12:06 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> >> It's a bit kludgy but works and makes sure the pins are only used
> >> for one thing at a time.
> >
> > No.  The case which I'm thinking of is for the Assabet audio, where
> > we have the following situation.
> 
> OK... (thanks for the explanation!)
> 
> > The solution is to wait for SSP to finish, and while SSP is still enabled,
> > switch the pins from the SSP block to the GPIO block and toggle the SFRM
> > signal to flip the WS output before removing power.  When starting up,
> > with the pins under control of GPIO, power up and them toggle hte SFRM
> > signal to restore the WS state before giving it back to the SSP block.
> >
> > However, here's the thing: SSP must be enabled at the point in time when
> > the pins are given or taken away from it, otherwise SFRM is indeterminant.
> > We can't allow gpio_request() to fail at the points where we toggle this
> > pin - failure is not really an option where causing hardware stress is
> > involved.  We can't allow the GPIO block to have the state of these pins
> > change state while the pins are given back to the GPIO block before the
> > GPIOs have been requested (glitches on the SFRM line will cause the state
> > of the FIFO to change.)
> 
> This can probably be done, mainly I'd say that control over the pins
> need not mean that they are decoupled from some specific hardware, or
> that the hardware is disabled or so, the semantics of doing say
> pinctrl_get/enable/disable/put aren't set in stone, it's just callbacks into
> the device driver so it could avoid doing anything in this case, keep
> the pins muxed for the SSP if the transition goes from SSP->GPIO
> and back.
> 
> However I guess it would be conceptually closer if say the pins could
> be used for a device and GPIO at the *same time*.

If we allowed that, it would also solve the following comment in patch
2 in this series, in the Tegra GPIO driver:

int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
{
	/*
	 * We should call pinctrl_request_gpio() here. However, we can't. Tegra
	 * muxes pins in groups not individually, and real boards exist where
	 * most of a pin group needs to be used for some function, yet some of
	 * the pins aren't actually used by that function in the mode the
	 * controller operates on the board, and so those pins can be used as
	 * GPIOs.
	 *
	 * This is true in practice on Seaboard/Springbank, where pin group ATC
	 * is used by the NAND controller, but pin GMI_IORDY_PI5 is used as a
	 * GPIO for the SD card slot's CD signal.
	 *
	 * On Tegra30, pins are muxed in groups and hence we could call
	 * pinctrl here, but we don't for now.
	 */
	tegra_gpio_mask_write(GPIO_MSK_CNF(offset), offset, 1);

	return 0;
}

i.e. we actually /could/ call pinctrl_request_gpio() here.

And in turn, the I2C driver (in your example) and SPI driver (in Russell's)
would never have to call pinctrl_disable() and hence the issue I raised
in my previous email would not occur.

The one downside here:

Ignoring WARs like we're discussing, it's typically true that a given pin
should either be a special function or a GPIO for any given board. If
we do allow a pin to be owned/used by both, then how do we indicate, on
a per-board rather than per-SoC basis, which pins we should allow both
gpio_request() and pinmux usage? The following considerations exist:

a) On Tegra, a pin group might include 10 pins that are mux'd as a group,
hence all owned e.g. by a NAND driver. If a few of those aren't used on a
particular board due to the way the NAND is hooked up and the driver
configured, do we only allow gpio_request() only on those pins we know the
NAND driver isn't actually using, to prevent someone using unexpected
pins as GPIO? We'd need a per-GPIO per-board way to represent this if
we care about this level of error-checking.

b) In Linus's snooping example, how do we know the SoC can physically
implement enabling a pin as both a GPIO and a special function, such
that the gpio_request() for the snooping won't interfere with the mux'd
function? On Tegra, any GPIO usage is mutually exclusive with mux usage;
enabling GPIO even for input, disconnects the input side of the pin from
any HW module other than GPIO irrespective of mux function. IIRC,
somewhere some per-pin data was mentioned for this.

We could either:

1) Just ignore these issues, and allow both mux and GPIO ownership at
once of any pin, and leave it up to platform data or device tree authors
to give the correct GPIO IDs to drivers. This is possibly reasonable, but
I just want us to make this a conscious decision.

2) Extend the pinctrl mapping table to explicitly represent GPIO usage.

This is required anyway for HW where there isn't a 1:1 mapping between
GPIO ID and pin ID, e.g. 1 pin could be used for two different GPIO
signals, e.g. 1 from each of 2 different GPIO HW modules, with different
capabilities, or where 1 GPIO could be routed to one of 2 or more pins.

In other words, make "gpio" an explicit mux function, thus allowing
pinctrl_select_state() to directly transition between special function
and GPIO usage, rather than having separate pinctrl and GPIO APIs for
this. This way would entail dropping my Documentation/gpio.txt patch
completely, I think.

If we do this though, I think we'd want to extend pinctrl to allow muxing
on pins too, so that we don't need to create a pin group for every pin to
support per-pin GPIO ownership. This would also help out all the HW that
already muxes everything per-pin, since they need to create these single-
pin groups right now... I somewhat anticpated this in my naming of the
mapping table convenience macros in the most recent pinctrl patch I posted
to add pin config mapping table entries.

Thoughts?
Linus Walleij Feb. 22, 2012, 6:01 a.m. UTC | #11
On Tue, Feb 21, 2012 at 8:14 PM, Stephen Warren <swarren@nvidia.com> wrote:

> Ignoring WARs like we're discussing, it's typically true that a given pin
> should either be a special function or a GPIO for any given board. If
> we do allow a pin to be owned/used by both, then how do we indicate, on
> a per-board rather than per-SoC basis, which pins we should allow both
> gpio_request() and pinmux usage?

I was thinking about making this a property of the physical pin i.e.
struct pin_desc and not of the board data.

It seems to me like this arbitration has to be very close to the driver,
since not all or even many controllers will support it (AFAICT).

> The following considerations exist:
> a) On Tegra, a pin group might include 10 pins that are mux'd as a group,
> hence all owned e.g. by a NAND driver. If a few of those aren't used on a
> particular board due to the way the NAND is hooked up and the driver
> configured, do we only allow gpio_request() only on those pins we know the
> NAND driver isn't actually using, to prevent someone using unexpected
> pins as GPIO? We'd need a per-GPIO per-board way to represent this if
> we care about this level of error-checking.

In this case I would strive not to present unused pins to the functions
in the first place. But maybe this collides with the new paradigm to
assign aquire all possible pins on pinctrl_get() time?

> b) In Linus's snooping example, how do we know the SoC can physically
> implement enabling a pin as both a GPIO and a special function, such
> that the gpio_request() for the snooping won't interfere with the mux'd
> function?

I think on U300 and SA1100 we can flag that in the pin descriptor
as described above.

> 2) Extend the pinctrl mapping table to explicitly represent GPIO usage.

Seems like it could get complicated. Since the only hardwares we have
that can do this would be fine with having it in their pin descriptor we
can do it there?

> This is required anyway for HW where there isn't a 1:1 mapping between
> GPIO ID and pin ID, e.g. 1 pin could be used for two different GPIO
> signals, e.g. 1 from each of 2 different GPIO HW modules, with different
> capabilities, or where 1 GPIO could be routed to one of 2 or more pins.
>
> In other words, make "gpio" an explicit mux function, thus allowing
> pinctrl_select_state() to directly transition between special function
> and GPIO usage, rather than having separate pinctrl and GPIO APIs for
> this. This way would entail dropping my Documentation/gpio.txt patch
> completely, I think.
>
> If we do this though, I think we'd want to extend pinctrl to allow muxing
> on pins too, so that we don't need to create a pin group for every pin to
> support per-pin GPIO ownership. This would also help out all the HW that
> already muxes everything per-pin, since they need to create these single-
> pin groups right now... I somewhat anticpated this in my naming of the
> mapping table convenience macros in the most recent pinctrl patch I posted
> to add pin config mapping table entries.

Hm I think Tony also mentioned that he wanted to allow muxing of
single pins from the mapping sometime so I sort of like the idea.

Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 23, 2012, 12:45 a.m. UTC | #12
Linus Walleij wrote at Tuesday, February 21, 2012 11:01 PM:
> On Tue, Feb 21, 2012 at 8:14 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Ignoring WARs like we're discussing, it's typically true that a given pin
> > should either be a special function or a GPIO for any given board. If
> > we do allow a pin to be owned/used by both, then how do we indicate, on
> > a per-board rather than per-SoC basis, which pins we should allow both
> > gpio_request() and pinmux usage?
> 
> I was thinking about making this a property of the physical pin i.e.
> struct pin_desc and not of the board data.
> 
> It seems to me like this arbitration has to be very close to the driver,
> since not all or even many controllers will support it (AFAICT).

Well, there are two aspects:

a) Can the pin controller do it, which is something per-SoC/driver?
b) Does it make sense for the board, given what each pin is connected to?

So unless we decide to ignore (b) (which may be perfectly fine but as I
mentioned I'd just like to explicitly decide this), there needs to be some
per-SoC way of representing this capability, /and/ some per-board way.

> > The following considerations exist:
> > a) On Tegra, a pin group might include 10 pins that are mux'd as a group,
> > hence all owned e.g. by a NAND driver. If a few of those aren't used on a
> > particular board due to the way the NAND is hooked up and the driver
> > configured, do we only allow gpio_request() only on those pins we know the
> > NAND driver isn't actually using, to prevent someone using unexpected
> > pins as GPIO? We'd need a per-GPIO per-board way to represent this if
> > we care about this level of error-checking.
> 
> In this case I would strive not to present unused pins to the functions
> in the first place. But maybe this collides with the new paradigm to
> assign aquire all possible pins on pinctrl_get() time?

The pins are part of the pin group in HW. It doesn't make sense to say
that they aren't, and indeed the SoC driver has no board knowledge and
couldn't exclude them anyway.
Grant Likely March 12, 2012, 4:32 p.m. UTC | #13
On Tue, 21 Feb 2012 11:46:03 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > Update gpio.txt based on recent discussions regarding interaction with the
> > pinctrl subsystem.
> >
> > Previously, gpio_request() was described as explicitly not performing any
> > required mux setup operations etc.
> >
> > Now, gpio_request() is explicitly as explicitly performing any required mux
> > setup operations where possible. In the case it isn't, platform code is
> > required to have set up any required muxing or other configuration prior to
> > gpio_request() being called, in order to maintain the same semantics.
> >
> > This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
> > their .request() operation.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Grant can you take this one? I'd prefer for you to have a look at
> it as well.

I've taken this one, but left the 2nd for Olof.

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren March 12, 2012, 5:19 p.m. UTC | #14
On 03/12/2012 10:32 AM, Grant Likely wrote:
> On Tue, 21 Feb 2012 11:46:03 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>
>>> Update gpio.txt based on recent discussions regarding interaction with the
>>> pinctrl subsystem.
>>>
>>> Previously, gpio_request() was described as explicitly not performing any
>>> required mux setup operations etc.
>>>
>>> Now, gpio_request() is explicitly as explicitly performing any required mux
>>> setup operations where possible. In the case it isn't, platform code is
>>> required to have set up any required muxing or other configuration prior to
>>> gpio_request() being called, in order to maintain the same semantics.
>>>
>>> This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
>>> their .request() operation.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Grant can you take this one? I'd prefer for you to have a look at
>> it as well.
> 
> I've taken this one, but left the 2nd for Olof.

Grant, did you take V2 of this patch? I assume not since you replied to V1.

For reference, that's:
http://lkml.org/lkml/2012/3/5/533

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 792faa3..3c4a702 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -271,9 +271,25 @@  Some platforms may also use knowledge about what GPIOs are active for
 power management, such as by powering down unused chip sectors and, more
 easily, gating off unused clocks.
 
-Note that requesting a GPIO does NOT cause it to be configured in any
-way; it just marks that GPIO as in use.  Separate code must handle any
-pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
+Requesting a GPIO should cause any pin multiplexing hardware to be programmed
+to route the GPIO signal to the appropriate pin. In some cases, this requires
+programming separate pin multiplexing hardware outside the GPIO controller.
+Equally, for GPIOs that use pins known to the pinctrl subsystem, that
+subsystem should be informed of their use. These requirements may be satisfied
+by having a gpiolib driver's .request() operation call pinctrl_request_gpio().
+Similarly, a gpiolib driver's .free() operation may call pinctrl_free_gpio().
+
+Some platforms allow some or all GPIO signals to be routed to different pins.
+Similarly, other aspects of the GPIO or pin may need to be configured, such as
+pullup/pulldown. Platform software should arrange that any such details are
+configured priored to gpio_request() being called for those GPIOs, such that
+GPIO users need not be aware of these details.
+
+gpiolib drivers may need to call additional pinctrl APIs to implement certain
+operations. This would be the case if e.g. GPIO input/output value is
+controlled by a GPIO HW module, whereas GPIO direction is controlled by a
+separate pin controller HW module. Functions pinctrl_gpio_direction_input()
+and pinctrl_gpio_direction_output() may be used to implement this.
 
 Also note that it's your responsibility to have stopped using a GPIO
 before you free it.