Patchwork [2/4] powerpc/qe: new call to revert a gpio to a dedicated function

login
register
mail settings
Submitter Anton Vorontsov
Date Sept. 24, 2008, 12:03 a.m.
Message ID <20080924000336.GB29733@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/1209/
State Changes Requested, archived
Headers show

Comments

Anton Vorontsov - Sept. 24, 2008, 12:03 a.m.
qe_gpio_set_dedicated() is a platform specific function, which is used
to revert a pin to a dedicated function. Caller should have already
obtained the gpio via gpio_request().

This is needed to support Freescale USB Host Controller.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/include/asm/qe.h     |    1 +
 arch/powerpc/sysdev/qe_lib/gpio.c |   46 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 0 deletions(-)
Kumar Gala - Sept. 24, 2008, 4:07 a.m.
On Sep 23, 2008, at 7:03 PM, Anton Vorontsov wrote:

> qe_gpio_set_dedicated() is a platform specific function, which is used
> to revert a pin to a dedicated function. Caller should have already
> obtained the gpio via gpio_request().
>
> This is needed to support Freescale USB Host Controller.
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> arch/powerpc/include/asm/qe.h     |    1 +
> arch/powerpc/sysdev/qe_lib/gpio.c |   46 ++++++++++++++++++++++++++++ 
> +++++++++
> 2 files changed, 47 insertions(+), 0 deletions(-)

what do you mean by dedicated function.. be a bit clearer in the  
commit log.  Also, does this depend on gpio_to_chip() patch?

- k
Anton Vorontsov - Sept. 24, 2008, 11:42 a.m.
On Tue, Sep 23, 2008 at 11:07:00PM -0500, Kumar Gala wrote:
>
> On Sep 23, 2008, at 7:03 PM, Anton Vorontsov wrote:
>
>> qe_gpio_set_dedicated() is a platform specific function, which is used
>> to revert a pin to a dedicated function. Caller should have already
>> obtained the gpio via gpio_request().
>>
>> This is needed to support Freescale USB Host Controller.
>>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>> ---
>> arch/powerpc/include/asm/qe.h     |    1 +
>> arch/powerpc/sysdev/qe_lib/gpio.c |   46 ++++++++++++++++++++++++++++ 
>> +++++++++
>> 2 files changed, 47 insertions(+), 0 deletions(-)
>
> what do you mean by dedicated function.. be a bit clearer in the commit 
> log.

This term is from the QE spec, I didn't invent anything. ;-)

"Each pin in the I/O ports can be configured as a general-purpose
I/O signal or as a dedicated peripheral interface signal. ...many
dedicated peripheral functions are multiplexed onto the ports."

> Also, does this depend on gpio_to_chip() patch?

Yeah, the point of exported gpio_to_chip is to let us write
this function.

Thanks!
Kumar Gala - Sept. 24, 2008, 2 p.m.
On Sep 24, 2008, at 6:42 AM, Anton Vorontsov wrote:

> On Tue, Sep 23, 2008 at 11:07:00PM -0500, Kumar Gala wrote:
>>
>> On Sep 23, 2008, at 7:03 PM, Anton Vorontsov wrote:
>>
>>> qe_gpio_set_dedicated() is a platform specific function, which is  
>>> used
>>> to revert a pin to a dedicated function. Caller should have already
>>> obtained the gpio via gpio_request().
>>>
>>> This is needed to support Freescale USB Host Controller.
>>>
>>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>>> ---
>>> arch/powerpc/include/asm/qe.h     |    1 +
>>> arch/powerpc/sysdev/qe_lib/gpio.c |   46 ++++++++++++++++++++++++++ 
>>> ++
>>> +++++++++
>>> 2 files changed, 47 insertions(+), 0 deletions(-)
>>
>> what do you mean by dedicated function.. be a bit clearer in the  
>> commit
>> log.
>
> This term is from the QE spec, I didn't invent anything. ;-)
>
> "Each pin in the I/O ports can be configured as a general-purpose
> I/O signal or as a dedicated peripheral interface signal. ...many
> dedicated peripheral functions are multiplexed onto the ports."

I understand but I think 'dedicated' could be interpreted in another  
way (like the GPIO pin is dedicated, not that the pin is used for a  
dedicated SoC block).

If it the commit message had said 'to a dedicated on chip peripheral'  
it would be clearer.

>> Also, does this depend on gpio_to_chip() patch?
>
> Yeah, the point of exported gpio_to_chip is to let us write
> this function.

I meant can I take this patch w/o the gpio_to_chip() patch? (not clear  
from your response)

- k
Anton Vorontsov - Sept. 24, 2008, 2:11 p.m.
On Wed, Sep 24, 2008 at 09:00:03AM -0500, Kumar Gala wrote:
>
> On Sep 24, 2008, at 6:42 AM, Anton Vorontsov wrote:
>
>> On Tue, Sep 23, 2008 at 11:07:00PM -0500, Kumar Gala wrote:
>>>
>>> On Sep 23, 2008, at 7:03 PM, Anton Vorontsov wrote:
>>>
>>>> qe_gpio_set_dedicated() is a platform specific function, which is  
>>>> used
>>>> to revert a pin to a dedicated function. Caller should have already
>>>> obtained the gpio via gpio_request().
>>>>
>>>> This is needed to support Freescale USB Host Controller.
>>>>
>>>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>>>> ---
>>>> arch/powerpc/include/asm/qe.h     |    1 +
>>>> arch/powerpc/sysdev/qe_lib/gpio.c |   46 ++++++++++++++++++++++++++ 
>>>> ++
>>>> +++++++++
>>>> 2 files changed, 47 insertions(+), 0 deletions(-)
>>>
>>> what do you mean by dedicated function.. be a bit clearer in the  
>>> commit
>>> log.
>>
>> This term is from the QE spec, I didn't invent anything. ;-)
>>
>> "Each pin in the I/O ports can be configured as a general-purpose
>> I/O signal or as a dedicated peripheral interface signal. ...many
>> dedicated peripheral functions are multiplexed onto the ports."
>
> I understand but I think 'dedicated' could be interpreted in another way 
> (like the GPIO pin is dedicated, not that the pin is used for a  
> dedicated SoC block).
>
> If it the commit message had said 'to a dedicated on chip peripheral' it 
> would be clearer.

Ah, ok, I'll fix that.

>>> Also, does this depend on gpio_to_chip() patch?
>>
>> Yeah, the point of exported gpio_to_chip is to let us write
>> this function.
>
> I meant can I take this patch w/o the gpio_to_chip() patch? (not clear  
> from your response)

No, unfortunately you can't. It would be great if we could pass the
whole patchset via single tree (USB? Or -mm to Linus directly?),
so Acks are more than appreciated.


Thanks again,
David Brownell - Sept. 24, 2008, 6:54 p.m.
On Tuesday 23 September 2008, Anton Vorontsov wrote:
> qe_gpio_set_dedicated() is a platform specific function, which is used
> to revert a pin to a dedicated function. Caller should have already
> obtained the gpio via gpio_request().

Note the missing sibling function:  putting the pin back into
GPIO mode!!  You seem to assume that some of the GPIO calls
will be performing that pinmux function.  But those calls are
explicitly defined as NOT incorporating any pinmux tasks.

Also note your nonportable assumption that GPIOs and pins are
the same concept ... they aren't.


You'd be better off calling something other than of_get_gpio()
for those three pins in of_fhci_probe() ... call something
that returns a "qe_pin" structure (e.g. wrapping an instance of
the misnamed qe_gpio_chip plus an offset) which holds the
pinmux primitives you need.  Like this one to put the pin into
its normal mode.

When I look at patch 4 of this series I observe that only
two pins are true GPIOs:  the optional POWER and SPEED pins.
(External transceiver support?)


> +int qe_gpio_set_dedicated(unsigned int gpio)
> +{
> +	struct gpio_chip *gc = gpio_to_chip(gpio);

So the caller must already have requested it, yes -- that's a
needed for any stable mapping between GPIO and controller inside
the GPIO library.

For the record, this single call seems to be the entire reason
motivating that rather ugly patch #1.  (Ugly for more than just
the confusion between pin, which is what you need, and GPIO.
There's no need to export those internal data structures.)


And in turn, the reason to want this call is so that you can
have io_port_generate_reset() generate a short reset on the
single downstream USB port.  ("Short" meaning "45 msec below
USB spec requirements for root hub resets" ...)

And to top it off ... that driver does gpio_request(), runs
those pins as GPIOs for virtually no time, and then uses
them as "dedicated functions" the rest of the time (after
the reset completes)!!


Which highlights the fact that these pins are fundamentally
NOT used as GPIOs.  They're function pins that need brief
detours as GPIOs because, it seems, those functions only
support differential signaling (USB J and K states) instead
of the full set of USB states.  (It's not quite clear from
the driver.  Are the pins expected to be using a 3-wire
external transciever hookup?  4-wire?  6-wire?)



But there are other requirements for this no-kerneldoc call:

> +	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);

... it must be part of an of_mm_gpio_chip (in <linux/of_gpio.h>,
which might seem less odd to me if I read its supporting code).

Can you first ensure that it *is* an of_mm_gpio_chip
instance?  When it isn't, this code will oops rudely.


> +	struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc);

... it must be the qe_gpio_chip flavor (defined only in this
very file, arch/powerpc/sysdev/qe_lib/gpio.c); IMO the code
would be cleaner if you just did

	qe_gc = container_of(gpio_to_chip(gpio),
			struct qe_gpio_chip, mm_gc.of_gc.gc);

and had just one pointer (not three!) for all these purposes.

(And cleaner still if it didn't require whacking the GPIO
framework out of shape to have a hope of working.)

But again:  you're trusting, with no evident basis for that
trust, that it's a qe_gpio_chip instance.  Oops if it isn't.
Much better for these calls to take e.g. a "qe_pin" parameter,
struct pointer or whatever ... not a GPIO number.


> +	struct qe_pio_regs __iomem *regs = mm_gc->regs;
> +	struct qe_pio_regs *sregs = &qe_gc->saved_regs;
> +	u8 pin = gpio - gc->base;
> +	u32 mask1 = 1 << (QE_PIO_PINS - (pin + 1));
> +	u32 mask2 = 0x3 << (QE_PIO_PINS - (pin % (QE_PIO_PINS / 2) + 1) * 2);
> +	bool second_reg = pin > (QE_PIO_PINS / 2) - 1;
>	...
David Brownell - Sept. 24, 2008, 6:54 p.m.
On Wednesday 24 September 2008, Anton Vorontsov wrote:
> > what do you mean by dedicated function.. be a bit clearer in the commit 
> > log.
> 
> This term is from the QE spec, I didn't invent anything. ;-)
> 
> "Each pin in the I/O ports can be configured as a general-purpose
> I/O signal or as a dedicated peripheral interface signal. ...many
> dedicated peripheral functions are multiplexed onto the ports."

Which, to me, highlights the point I've made previously:  the
right abstraction for you to work with is "pin", not GPIO.

You need to switch a QE "pin" from one of its roles to the other.

Evil things, like oopsing, will happen if you try to use this call
on a GPIO that's not backed by a QE pin.
Anton Vorontsov - Sept. 24, 2008, 11:29 p.m.
On Wed, Sep 24, 2008 at 11:54:24AM -0700, David Brownell wrote:
[...]
> You'd be better off calling something other than of_get_gpio()
> for those three pins in of_fhci_probe() ... call something
> that returns a "qe_pin" structure (e.g. wrapping an instance of
> the misnamed qe_gpio_chip plus an offset) which holds the
> pinmux primitives you need.  Like this one to put the pin into
> its normal mode.

The driver anyway will have to call of_get_gpio(), because it
have to use the pins as gpios. At least it have to specify a
direction, luckily for this we have the gpio_set_direction call
already. ;-)

But true, switching a pin to a dedicated function isn't a gpio
controller's job, it is a pinmuxing.

> When I look at patch 4 of this series I observe that only
> two pins are true GPIOs:  the optional POWER and SPEED pins.
> (External transceiver support?)

Yes.

[...]
> And in turn, the reason to want this call is so that you can
> have io_port_generate_reset() generate a short reset on the
> single downstream USB port.  ("Short" meaning "45 msec below
> USB spec requirements for root hub resets" ...)
> 
> And to top it off ... that driver does gpio_request(), runs
> those pins as GPIOs for virtually no time, and then uses
> them as "dedicated functions" the rest of the time (after
> the reset completes)!!
> 
> 
> Which highlights the fact that these pins are fundamentally
> NOT used as GPIOs.

Well.. they are used as gpios anyway, to signal a reset.
This is host's duty, and we have to support it, otherwise
things won't work. There is no other way to signal a reset
than turning these *pins* to a gpio state, setting the
direction and then reverting them back to a dedicated function.

But true, most of it isn't gpio controller's authority.

[...]
> But there are other requirements for this no-kerneldoc call:
> 
> > +	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> 
> ... it must be part of an of_mm_gpio_chip (in <linux/of_gpio.h>,
> which might seem less odd to me if I read its supporting code).
> 
> Can you first ensure that it *is* an of_mm_gpio_chip
> instance?  When it isn't, this code will oops rudely.

Yeah, unfortunately. The "oops" thought didn't visit my mind though,
because I'm still thinking it terms of this patch:

http://ozlabs.org/pipermail/linuxppc-dev/2008-February/051230.html

^^ with that patch no oops is possible, and we could detect anything
you pointed out here and below in your post. Which doesn't mean that
the patch above was ideologically correct, though.

But you clearly pointed out the issues which ruin the whole approach.


Anyway, just want to thank you for your time and persistence on this
matter, you're forcing others' people brains to *work*. And since you
rejected this approach too, I have no other option but to implement
something else... something better. ;-)
David Brownell - Sept. 25, 2008, 4:13 a.m.
On Wednesday 24 September 2008, Anton Vorontsov wrote:
> 
> Anyway, just want to thank you for your time and persistence on this
> matter, you're forcing others' people brains to *work*.  And since you 
> rejected this approach too, I have no other option but to implement
> something else... something better. ;-)

I think you have enough pieces in place to get that
"something better" _very_ quickly.  Or I'd feel worse
about abusing those poor little grey cells.  ;)

Patch

diff --git a/arch/powerpc/include/asm/qe.h b/arch/powerpc/include/asm/qe.h
index edee15d..c926147 100644
--- a/arch/powerpc/include/asm/qe.h
+++ b/arch/powerpc/include/asm/qe.h
@@ -111,6 +111,7 @@  extern void __par_io_config_pin(struct qe_pio_regs __iomem *par_io, u8 pin,
 extern int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
 			     int assignment, int has_irq);
 extern int par_io_data_set(u8 port, u8 pin, u8 val);
+extern int qe_gpio_set_dedicated(unsigned int gpio);
 
 /* QE internal API */
 int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input);
diff --git a/arch/powerpc/sysdev/qe_lib/gpio.c b/arch/powerpc/sysdev/qe_lib/gpio.c
index 8e5a0bc..bd7278f 100644
--- a/arch/powerpc/sysdev/qe_lib/gpio.c
+++ b/arch/powerpc/sysdev/qe_lib/gpio.c
@@ -26,6 +26,9 @@  struct qe_gpio_chip {
 
 	/* shadowed data register to clear/set bits safely */
 	u32 cpdata;
+
+	/* saved_regs used to restore dedicated functions */
+	struct qe_pio_regs saved_regs;
 };
 
 static inline struct qe_gpio_chip *
@@ -40,6 +43,12 @@  static void qe_gpio_save_regs(struct of_mm_gpio_chip *mm_gc)
 	struct qe_pio_regs __iomem *regs = mm_gc->regs;
 
 	qe_gc->cpdata = in_be32(&regs->cpdata);
+	qe_gc->saved_regs.cpdata = qe_gc->cpdata;
+	qe_gc->saved_regs.cpdir1 = in_be32(&regs->cpdir1);
+	qe_gc->saved_regs.cpdir2 = in_be32(&regs->cpdir2);
+	qe_gc->saved_regs.cppar1 = in_be32(&regs->cppar1);
+	qe_gc->saved_regs.cppar2 = in_be32(&regs->cppar2);
+	qe_gc->saved_regs.cpodr = in_be32(&regs->cpodr);
 }
 
 static int qe_gpio_get(struct gpio_chip *gc, unsigned int gpio)
@@ -103,6 +112,43 @@  static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
+int qe_gpio_set_dedicated(unsigned int gpio)
+{
+	struct gpio_chip *gc = gpio_to_chip(gpio);
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc);
+	struct qe_pio_regs __iomem *regs = mm_gc->regs;
+	struct qe_pio_regs *sregs = &qe_gc->saved_regs;
+	u8 pin = gpio - gc->base;
+	u32 mask1 = 1 << (QE_PIO_PINS - (pin + 1));
+	u32 mask2 = 0x3 << (QE_PIO_PINS - (pin % (QE_PIO_PINS / 2) + 1) * 2);
+	bool second_reg = pin > (QE_PIO_PINS / 2) - 1;
+	unsigned long flags;
+
+	spin_lock_irqsave(&qe_gc->lock, flags);
+
+	if (second_reg) {
+		clrsetbits_be32(&regs->cpdir2, mask2, sregs->cpdir2 & mask2);
+		clrsetbits_be32(&regs->cppar2, mask2, sregs->cppar2 & mask2);
+	} else {
+		clrsetbits_be32(&regs->cpdir1, mask2, sregs->cpdir1 & mask2);
+		clrsetbits_be32(&regs->cppar1, mask2, sregs->cppar1 & mask2);
+	}
+
+	if (sregs->cpdata & mask1)
+		qe_gc->cpdata |= mask1;
+	else
+		qe_gc->cpdata &= ~mask1;
+
+	out_be32(&regs->cpdata, qe_gc->cpdata);
+	clrsetbits_be32(&regs->cpodr, mask1, sregs->cpodr & mask1);
+
+	spin_unlock_irqrestore(&qe_gc->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(qe_gpio_set_dedicated);
+
 static int __init qe_add_gpiochips(void)
 {
 	struct device_node *np;