Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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 [EMAIL PROTECTED] --- 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! -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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 [EMAIL PROTECTED] --- 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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 [EMAIL PROTECTED] --- 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, -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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;
...
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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. ;-) -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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. ;) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] powerpc/qe: new call to revert a gpio to a dedicated function
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 [EMAIL PROTECTED] --- 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
