| Message ID | 20080924000336.GB29733@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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
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!
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
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,
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; > ...
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.
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. ;-)
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. ;)
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(®s->cpdata); + qe_gc->saved_regs.cpdata = qe_gc->cpdata; + qe_gc->saved_regs.cpdir1 = in_be32(®s->cpdir1); + qe_gc->saved_regs.cpdir2 = in_be32(®s->cpdir2); + qe_gc->saved_regs.cppar1 = in_be32(®s->cppar1); + qe_gc->saved_regs.cppar2 = in_be32(®s->cppar2); + qe_gc->saved_regs.cpodr = in_be32(®s->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(®s->cpdir2, mask2, sregs->cpdir2 & mask2); + clrsetbits_be32(®s->cppar2, mask2, sregs->cppar2 & mask2); + } else { + clrsetbits_be32(®s->cpdir1, mask2, sregs->cpdir1 & mask2); + clrsetbits_be32(®s->cppar1, mask2, sregs->cppar1 & mask2); + } + + if (sregs->cpdata & mask1) + qe_gc->cpdata |= mask1; + else + qe_gc->cpdata &= ~mask1; + + out_be32(®s->cpdata, qe_gc->cpdata); + clrsetbits_be32(®s->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;
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(-)