| Message ID | 20131003054221.8941.87801.stgit@localhost |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 3, 2013 at 7:42 AM, Tony Lindgren <tony@atomide.com> wrote: > This patch adds support for interrupts in a way that > should be pretty generic, and works for the omaps that > support wake-up interrupts. On omaps, there's an > interrupt enable and interrupt status bit for each pin. So to be clear: is this enable and status bit in the *same* register as all other settings? Then it warrants having this under pinctrl-single still and I feel a bit better about this. Second: how does this relate to the case where say gpio-omap is using the same pins as a back-end (this is a real usecase right)? So gpio-omap already supports gpio_to_irq() and it does not support enable_irq_wake() on the GPIO lines as well. How does this play together? Is it like the pins have a set of wakeup IRQs and then the GPIO block has *another* set of wakeup IRQs? And if you do enable_irq_wake() on the GPIO line, should this not fall through to the pinctrl driver, so we should add pinctrl_gpio_enable_wake() and pinctrl_gpio_disable_wake() just like we have pinctrl_gpio_direction_input() and friends today, so that this request falls through to the pinctrl driver when a wakeup on a certain GPIO line is requested? Now gpio-omap.c does not seem to use the pinctrl back-end commands, instead of using e.g. pinctrl_gpio_request() and GPIO to pin ranges it seems to use this hack: static void _enable_gpio_module(struct gpio_bank *bank, unsigned offset) { if (bank->regs->pinctrl) { void __iomem *reg = bank->base + bank->regs->pinctrl; /* Claim the pin for MPU */ __raw_writel(__raw_readl(reg) | (1 << offset), reg); } Can't we please make the OMAP GPIO driver use the pinctrl back-end with gpio ranges properly *before* we proceed to doing this kind of stuff? I think it is already looking like a bit of layered hacks :-( Haojian is already using the pinctrl-single as backend to another GPIO controller (I think it's pinctrl-pl061.c) so surely this should be possible to do right. IIRC there are also other OMAP GPIO blocks so we need to look at the large picture here, for all of them. Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [131008 05:18]: > On Thu, Oct 3, 2013 at 7:42 AM, Tony Lindgren <tony@atomide.com> wrote: > > > This patch adds support for interrupts in a way that > > should be pretty generic, and works for the omaps that > > support wake-up interrupts. On omaps, there's an > > interrupt enable and interrupt status bit for each pin. > > So to be clear: is this enable and status bit in the *same* > register as all other settings? Then it warrants having this > under pinctrl-single still and I feel a bit better about this. Yes there's a status bit on most omaps in the same register for each pin. There is a PRM interrupt that has wake-up events for internal domain wake-ups and io chain wake-ups. The PRM interrupt is already a chained IRQ. After we get the wake-up interrupt we need to check the pinctrl registers that have wake-up bit enabled for the wake-up status bit. > Second: how does this relate to the case where say > gpio-omap is using the same pins as a back-end (this > is a real usecase right)? The GPIO is a separate hardware block on omaps and has it's own separate wake-up events. > So gpio-omap already supports gpio_to_irq() and > it does not support enable_irq_wake() on the GPIO > lines as well. Yeah those are specific to the GPIO block, and not related to this driver. The GPIO lines have their own wake-up mechanism that works for retention idle. For off-idle, only for the first GPIO bank stays powered. The other GPIO banks get powered down in off-idle mode. > How does this play together? Is it like the pins have a set > of wakeup IRQs and then the GPIO block has *another* > set of wakeup IRQs? Yes that's correct, they are completely separate. > And if you do enable_irq_wake() on the GPIO line, > should this not fall through to the pinctrl driver, so we > should add pinctrl_gpio_enable_wake() and > pinctrl_gpio_disable_wake() just like we have > pinctrl_gpio_direction_input() and friends today, > so that this request falls through to the pinctrl driver > when a wakeup on a certain GPIO line is requested? That's not needed for most cases at this point, the GPIO block is already handling it's internal wake-up events. Eventually we should also handle the corner case of GPIO wake-up line connected to a GPIO bank that's not the first bank for off-idle. For most low-power hardware designs that's not needed as the critial pins typically are placed in the first GPIO bank for this reason. But let's first figure out how we want to handle the mapping of wake-up interrupts to device interrupts in general. Most omap devices have a dedicated io chain wake-up line, so that's really the key thing to get working first. > Now gpio-omap.c does not seem to use the pinctrl > back-end commands, instead of using e.g. > pinctrl_gpio_request() and GPIO to pin ranges it > seems to use this hack: > > static void _enable_gpio_module(struct gpio_bank *bank, unsigned offset) > { > if (bank->regs->pinctrl) { > void __iomem *reg = bank->base + bank->regs->pinctrl; > > /* Claim the pin for MPU */ > __raw_writel(__raw_readl(reg) | (1 << offset), reg); > } > > Can't we please make the OMAP GPIO driver use the > pinctrl back-end with gpio ranges properly *before* we proceed > to doing this kind of stuff? I think it is already looking > like a bit of layered hacks :-( Heh, that's not true. And I can totally see where the confusion comes from :) The naming "pinctrl" in the GPIO driver is a bit confusing and it's use predates the pinctrl framework and has been in the GPIO driver for probably 10 years or so. The above does not have anything to do with the pinctrl framework or this pinctrl driver. That is to mux the GPIO pins inside the GPIO block between the ARM core and the DSP. > Haojian is already using the pinctrl-single as backend to > another GPIO controller (I think it's pinctrl-pl061.c) so surely > this should be possible to do right. Sure we can make the GPIO off-idle wake-up handling automatic for the GPIO not in the first bank, but that's really a separate patch. > IIRC there are also other OMAP GPIO blocks so we need > to look at the large picture here, for all of them. Sorry I don't know which other OMAP GPIO blocks you're talking about, care to be a bit more specific? All the omaps I've seen use the gpio-omap.c. The other GPIOs are typically on I2C chips. Regards, Tony
Hi Tony, On 10/03/2013 08:42 AM, Tony Lindgren wrote: > The pin control registers can have interrupts for example > for device wake-up. These interrupts can be treated as a > chained interrupt controller as suggested earlier by > Linus Walleij <linus.walleij@linaro.org>. > > This patch adds support for interrupts in a way that > should be pretty generic, and works for the omaps that > support wake-up interrupts. On omaps, there's an > interrupt enable and interrupt status bit for each pin. > The two pinctrl domains on omaps share a single interrupt > from the PRM chained interrupt handler. Support for > other similar hardware should be easy to add. > > Note that this patch does not attempt to handle the > wake-up interrupts automatically unlike the earlier > patches. This patch allows the device drivers to do > a request_irq() on the wake-up pins as needed. I'll > try to do also a separate generic patch for handling > the wake-up events automatically. > > Also note that as this patch makes the pinctrl-single > an irq controller, the current bindings need some > extra trickery to use interrupts from two different > interrupt controllers for the same driver. So it > might be worth waiting a little on the patches > enabling the wake-up interrupts from drivers as there > should be a generic way to handle it coming. And also > there's been discussion of interrupts-extended binding > for using interrupts from multiple interrupt controllers. > > In any case, this patch should be ready to go allowing > handling the wake-up interrupts in a generic way, or > separately from the device drivers. > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Prakash Manjunathappa <prakash.pm@ti.com> > Cc: Roger Quadros <rogerq@ti.com> > Cc: Haojian Zhuang <haojian.zhuang@linaro.org> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: linux-kernel@vger.kernel.org > Cc: Benoît Cousson <bcousson@baylibre.com> > Cc: devicetree@vger.kernel.org > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > .../devicetree/bindings/pinctrl/pinctrl-single.txt | 11 + > drivers/pinctrl/pinctrl-single.c | 325 ++++++++++++++++++++ > 2 files changed, 336 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > index 5a02e30..7069a0b 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > @@ -72,6 +72,13 @@ Optional properties: > /* pin base, nr pins & gpio function */ > pinctrl-single,gpio-range = <&range 0 3 0 &range 3 9 1>; > > +- interrupt-controller : standard interrupt controller binding if using > + interrupts for wake-up events for example. In this case pinctrl-single > + is set up as a chained interrupt controller and the wake-up interrupts > + can be requested by the drivers using request_irq(). > + > +- #interrupt-cells : standard interrupt binding if using interrupts > + > This driver assumes that there is only one register for each pin (unless the > pinctrl-single,bit-per-mux is set), and uses the common pinctrl bindings as > specified in the pinctrl-bindings.txt document in this directory. > @@ -121,6 +128,8 @@ pmx_core: pinmux@4a100040 { > reg = <0x4a100040 0x0196>; > #address-cells = <1>; > #size-cells = <0>; > + #interrupt-cells = <1>; > + interrupt-controller; > pinctrl-single,register-width = <16>; > pinctrl-single,function-mask = <0xffff>; > }; > @@ -131,6 +140,8 @@ pmx_wkup: pinmux@4a31e040 { > reg = <0x4a31e040 0x0038>; > #address-cells = <1>; > #size-cells = <0>; > + #interrupt-cells = <1>; > + interrupt-controller; > pinctrl-single,register-width = <16>; > pinctrl-single,function-mask = <0xffff>; > }; > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > index f88d3d1..b4ff055 100644 > --- a/drivers/pinctrl/pinctrl-single.c > +++ b/drivers/pinctrl/pinctrl-single.c > @@ -15,10 +15,14 @@ > #include <linux/slab.h> > #include <linux/err.h> > #include <linux/list.h> > +#include <linux/interrupt.h> > + > +#include <linux/irqchip/chained_irq.h> > > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/of_address.h> > +#include <linux/of_irq.h> > > #include <linux/pinctrl/pinctrl.h> > #include <linux/pinctrl/pinmux.h> > @@ -152,9 +156,15 @@ struct pcs_name { > /** > * struct pcs_soc_data - SoC specific settings > * @flags: initial SoC specific PCS_FEAT_xxx values > + * @irq: optional interrupt for the controller > + * @irq_enable_mask: optional SoC specific interrupt enable mask > + * @irq_status_mask: optional SoC specific interrupt status mask > */ > struct pcs_soc_data { > unsigned flags; > + int irq; > + unsigned irq_enable_mask; > + unsigned irq_status_mask; > }; > > /** > @@ -165,6 +175,7 @@ struct pcs_soc_data { > * @dev: device entry > * @pctl: pin controller device > * @flags: mask of PCS_FEAT_xxx values > + * @lock: spinlock for register access > * @mutex: mutex protecting the lists > * @width: bits per mux register > * @fmask: function register mask > @@ -179,6 +190,9 @@ struct pcs_soc_data { > * @pingroups: list of pingroups > * @functions: list of functions > * @gpiofuncs: list of gpio functions > + * @irqs: list of interrupt registers > + * @chip: chip container for this instance > + * @domain: IRQ domain for this instance > * @ngroups: number of pingroups > * @nfuncs: number of functions > * @desc: pin controller descriptor > @@ -192,7 +206,11 @@ struct pcs_device { > struct device *dev; > struct pinctrl_dev *pctl; > unsigned flags; > +#define PCS_QUIRK_SHARED_IRQ (1 << 2) > +#define PCS_FEAT_IRQ (1 << 1) > #define PCS_FEAT_PINCONF (1 << 0) > + struct pcs_soc_data socdata; > + raw_spinlock_t lock; > struct mutex mutex; > unsigned width; > unsigned fmask; > @@ -208,6 +226,9 @@ struct pcs_device { > struct list_head pingroups; > struct list_head functions; > struct list_head gpiofuncs; > + struct list_head irqs; > + struct irq_chip chip; > + struct irq_domain *domain; > unsigned ngroups; > unsigned nfuncs; > struct pinctrl_desc desc; > @@ -215,6 +236,8 @@ struct pcs_device { > void (*write)(unsigned val, void __iomem *reg); > }; > > +#define PCS_QUIRK_HAS_SHARED_IRQ (pcs->flags & PCS_QUIRK_SHARED_IRQ) > +#define PCS_HAS_IRQ (pcs->flags & PCS_FEAT_IRQ) > #define PCS_HAS_PINCONF (pcs->flags & PCS_FEAT_PINCONF) > > static int pcs_pinconf_get(struct pinctrl_dev *pctldev, unsigned pin, > @@ -440,9 +463,11 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, > > for (i = 0; i < func->nvals; i++) { > struct pcs_func_vals *vals; > + unsigned long flags; > unsigned val, mask; > > vals = &func->vals[i]; > + raw_spin_lock_irqsave(&pcs->lock, flags); > val = pcs->read(vals->reg); > > if (pcs->bits_per_mux) > @@ -453,6 +478,7 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, > val &= ~mask; > val |= (vals->val & mask); > pcs->write(val, vals->reg); > + raw_spin_unlock_irqrestore(&pcs->lock, flags); > } > > return 0; > @@ -494,13 +520,16 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector, > > for (i = 0; i < func->nvals; i++) { > struct pcs_func_vals *vals; > + unsigned long flags; > unsigned val; > > vals = &func->vals[i]; > + raw_spin_lock_irqsave(&pcs->lock, flags); > val = pcs->read(vals->reg); > val &= ~pcs->fmask; > val |= pcs->foff << pcs->fshift; > pcs->write(val, vals->reg); > + raw_spin_unlock_irqrestore(&pcs->lock, flags); > } > } > > @@ -1451,11 +1480,33 @@ static void pcs_free_pingroups(struct pcs_device *pcs) > } > > /** > + * pcs_irq_free() - free interrupt > + * @pcs: pcs driver instance > + */ > +static void pcs_irq_free(struct pcs_device *pcs) > +{ > + struct pcs_soc_data *pcs_soc = &pcs->socdata; > + > + if (pcs_soc->irq < 0) > + return; > + > + if (pcs->domain) > + irq_domain_remove(pcs->domain); > + > + if (PCS_QUIRK_HAS_SHARED_IRQ) > + free_irq(pcs_soc->irq, pcs_soc); > + else > + irq_set_chained_handler(pcs_soc->irq, NULL); > +} > + > +/** > * pcs_free_resources() - free memory used by this driver > * @pcs: pcs driver instance > */ > static void pcs_free_resources(struct pcs_device *pcs) > { > + pcs_irq_free(pcs); > + > if (pcs->pctl) > pinctrl_unregister(pcs->pctl); > > @@ -1504,6 +1555,259 @@ static int pcs_add_gpio_func(struct device_node *node, struct pcs_device *pcs) > } > return ret; > } > +/** > + * @reg: virtual address of interrupt register > + * @hwirq: hardware irq number > + * @irq: virtual irq number > + * @node: list node > + */ > +struct pcs_interrupt { > + void __iomem *reg; > + irq_hw_number_t hwirq; > + unsigned int irq; > + struct list_head node; > +}; > + > +/** > + * pcs_irq_set() - enables or disables an interrupt > + * > + * Note that this currently assumes one interrupt per pinctrl > + * register that is typically used for wake-up events. > + */ > +static inline void pcs_irq_set(struct pcs_soc_data *pcs_soc, > + int irq, const bool enable) > +{ > + struct pcs_device *pcs; > + struct list_head *pos; > + unsigned mask; > + > + pcs = container_of(pcs_soc, struct pcs_device, socdata); > + list_for_each(pos, &pcs->irqs) { > + struct pcs_interrupt *pcswi; > + unsigned soc_mask; > + > + pcswi = list_entry(pos, struct pcs_interrupt, node); > + if (irq != pcswi->hwirq) > + continue; > + > + soc_mask = pcs_soc->irq_enable_mask; > + raw_spin_lock(&pcs->lock); > + mask = pcs->read(pcswi->reg); > + if (enable) > + mask &= ~soc_mask; > + else > + mask |= soc_mask; > + pcs->write(mask, pcswi->reg); > + raw_spin_unlock(&pcs->lock); > + } > +} > + > +/** > + * pcs_irq_mask() - mask pinctrl interrupt > + * @d: interrupt data > + */ > +static void pcs_irq_mask(struct irq_data *d) > +{ > + struct pcs_soc_data *pcs_soc = irq_data_get_irq_chip_data(d); > + > + pcs_irq_set(pcs_soc, d->irq, false); > +} > + > +/** > + * pcs_irq_unmask() - unmask pinctrl interrupt > + * @d: interrupt data > + */ > +static void pcs_irq_unmask(struct irq_data *d) > +{ > + struct pcs_soc_data *pcs_soc = irq_data_get_irq_chip_data(d); > + > + pcs_irq_set(pcs_soc, d->irq, true); > +} > + > +/** > + * pcs_irq_set_wake() - toggle the suspend and resume wake up > + * @d: interrupt data > + * @state: wake-up state > + * > + * Note that this should be called only for suspend and resume. > + * For runtime PM, the wake-up events should be enabled by default. > + */ > +static int pcs_irq_set_wake(struct irq_data *d, unsigned int state) > +{ > + if (state) > + pcs_irq_unmask(d); > + else > + pcs_irq_mask(d); > + > + return 0; > +} > + I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts while the system is still running and only the EHCI controller is runtime suspended. It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain whenever the pad wakeup_enable bit is changed. I think pcs_irq_set_wake() is where need to control system wakeup behaviour for the irq. This is where we should be able to change WAKEUP_EN bit of the pad to enable/disable system wakeup for that pad and also call _reconfigure_io_chain(). This would mean that we don't really need to set WAKEUP_EN for the pads in the DTS file. cheers, -roger
On Thu, Oct 10, 2013 at 3:24 PM, Roger Quadros <rogerq@ti.com> wrote: > I think pcs_irq_set_wake() is where need to control system wakeup behaviour for the irq. > This is where we should be able to change WAKEUP_EN bit of the pad > to enable/disable system wakeup for that pad and also call _reconfigure_io_chain(). As an innocent bystander who has no clue what the _reconfigure_io_chain() is about can you tell me what this is all about? Is this another one of the OMAP forked paths where you must call back into the machine with a special callback from each and every driver? Yours, Linus Walleij
On 10/10/2013 05:04 PM, Linus Walleij wrote: > On Thu, Oct 10, 2013 at 3:24 PM, Roger Quadros <rogerq@ti.com> wrote: > >> I think pcs_irq_set_wake() is where need to control system wakeup behaviour for the irq. >> This is where we should be able to change WAKEUP_EN bit of the pad >> to enable/disable system wakeup for that pad and also call _reconfigure_io_chain(). > > As an innocent bystander who has no clue what the _reconfigure_io_chain() > is about can you tell me what this is all about? The OMAP SoC has a mechanism to monitor and wakeup from a low power state by creating an IO ring of all the pads. But there is one bit in one of the control registers that needs to be toggled each time the pad configuration is changed to re-arm the IO ring. This is exactly what _reconfigure_io_chain() does. > > Is this another one of the OMAP forked paths where you must call back into > the machine with a special callback from each and every driver? _reconfigure_io_chain() is not available for public use and is not called by any driver yet. However, it somehow needs to be called from this pinctrl-single driver each time the wakeup configuration for any pad is changed. cheers, -roger
On Thu, Oct 10, 2013 at 4:35 PM, Roger Quadros <rogerq@ti.com> wrote: > On 10/10/2013 05:04 PM, Linus Walleij wrote: >> As an innocent bystander who has no clue what the _reconfigure_io_chain() >> is about can you tell me what this is all about? > > The OMAP SoC has a mechanism to monitor and wakeup from a low power state by creating > an IO ring of all the pads. But there is one bit in one of the control registers that > needs to be toggled each time the pad configuration is changed to re-arm the IO ring. > This is exactly what _reconfigure_io_chain() does. OK I get it, I checked the function in the machone. >> Is this another one of the OMAP forked paths where you must call back into >> the machine with a special callback from each and every driver? > > _reconfigure_io_chain() is not available for public use and is not called by any driver yet. > However, it somehow needs to be called from this pinctrl-single driver each time the > wakeup configuration for any pad is changed. Actually this is one of those things where the complexity of the platform seems to start to leak into the nice picture of one-register-per-pin. If this was *not* pinctrl-single but pinctrl-omap.c, it would make sense to move this _reconfigure_io code and the PRM registers it is touching down into the pinctrl driver, because it seems like absolutely no-one else is using it. Both the other occurences seem to be in omap_hwmod.c, and seems to be related to pin muxing, which is now supposed to be handled by the pin control driver, right? I think the real solution to this would be something like: - Add the compatible strings "pinctrl-single-omap3" and "pinctrl-single-omap4" to drivers/pinctrl/pinctrl-single.c, - Pass an additional <&ersand> resource for the prm thing to the single driver in the OMAP platform. - Move this _reconfigure_io code out of the mach-omap2 machines and hwmod and down into the pinctrl-single driver, it can be #ifdef ARCH_OMAP with stubs or whatever, the important thing is that it lives with the pinctrl driver. This does the right thing and let the pinctrl driver handle the io ring, instead of artificially trying to hide that in the mach-omap2 directory which is only creating a mess of things. I know this is sort of breaking the promise of pinctrl-single.c only handling single registers but we just need to accept the fact that if this idea is not perfect, trying to hide the facts of life isn't any good either. What do you say about this Tony? Good/bad/Linus is a moron? ;-) Yours, Linus Walleij
* Roger Quadros <rogerq@ti.com> [131010 06:32]: > > I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts > while the system is still running and only the EHCI controller is runtime suspended. > > It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain > whenever the pad wakeup_enable bit is changed. Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the pcs_irq_handle() like the comments there suggest? At least for me that keeps the wake-up interrupts continuously running on omap3 instead of just idle modes. Now on omap4, I've noticed the wake up interrupts are on all the time based on tests with the serial driver. > I think pcs_irq_set_wake() is where need to control system wakeup behaviour for the irq. > This is where we should be able to change WAKEUP_EN bit of the pad > to enable/disable system wakeup for that pad and also call _reconfigure_io_chain(). Well the irq_set_wake() should only be needed for suspend and resume. For runtime PM the wake-events should be always enabled by default as pointed out by Alan Stern a while back. > This would mean that we don't really need to set WAKEUP_EN for the pads in the DTS file. Well for runtime PM, we should also do the automatic handling if configured. But how to do that best is still open.. Regards, Tony
On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <tony@atomide.com> wrote: > * Roger Quadros <rogerq@ti.com> [131010 06:32]: >> >> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts >> while the system is still running and only the EHCI controller is runtime suspended. >> >> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain >> whenever the pad wakeup_enable bit is changed. > > Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the > pcs_irq_handle() like the comments there suggest? At least for me that keeps > the wake-up interrupts continuously running on omap3 instead of just idle modes. If the rearm() function is calling this _reconfigure_io_chain my comments on the fact that this is something that should be handled by the pin control driver still apply I think .... Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [131010 08:40]: > On Thu, Oct 10, 2013 at 4:35 PM, Roger Quadros <rogerq@ti.com> wrote: > > On 10/10/2013 05:04 PM, Linus Walleij wrote: > > >> As an innocent bystander who has no clue what the _reconfigure_io_chain() > >> is about can you tell me what this is all about? > > > > The OMAP SoC has a mechanism to monitor and wakeup from a low power state by creating > > an IO ring of all the pads. But there is one bit in one of the control registers that > > needs to be toggled each time the pad configuration is changed to re-arm the IO ring. > > This is exactly what _reconfigure_io_chain() does. > > OK I get it, I checked the function in the machone. > > >> Is this another one of the OMAP forked paths where you must call back into > >> the machine with a special callback from each and every driver? > > > > _reconfigure_io_chain() is not available for public use and is not called by any driver yet. > > However, it somehow needs to be called from this pinctrl-single driver each time the > > wakeup configuration for any pad is changed. > > Actually this is one of those things where the complexity of the platform > seems to start to leak into the nice picture of one-register-per-pin. > > If this was *not* pinctrl-single but pinctrl-omap.c, it would make sense to > move this _reconfigure_io code and the PRM registers it is touching down > into the pinctrl driver, because it seems like absolutely no-one else > is using it. > > Both the other occurences seem to be in omap_hwmod.c, and seems > to be related to pin muxing, which is now supposed to be handled by > the pin control driver, right? Right, that's for the legacy muxing code. With the legacy code we know the device using the pins and it's interrupt in the hwmod code, so transparent handling of the runtime PM wake-up events is easy. But that is at the cost of huge data tables for every SoC variant, which is what we're trying to avoid here. > I think the real solution to this would be something like: > > - Add the compatible strings "pinctrl-single-omap3" and > "pinctrl-single-omap4" to drivers/pinctrl/pinctrl-single.c, > > - Pass an additional <&ersand> resource for the prm > thing to the single driver in the OMAP platform. > > - Move this _reconfigure_io code out of the mach-omap2 > machines and hwmod and down into the pinctrl-single driver, > it can be #ifdef ARCH_OMAP with stubs or whatever, the > important thing is that it lives with the pinctrl driver. > > This does the right thing and let the pinctrl driver handle the io ring, > instead of artificially trying to hide that in the mach-omap2 directory > which is only creating a mess of things. > > I know this is sort of breaking the promise of pinctrl-single.c only > handling single registers but we just need to accept the fact that > if this idea is not perfect, trying to hide the facts of life isn't any > good either. > > What do you say about this Tony? Good/bad/Linus is a moron? ;-) Yes once we have made omap3 to be DT only, a lot of the legacy mux stuff will clear out. And at that point we can start moving things into PRCM drivers to handle the single shared wake-up interrupt that PRM and also pinctrl-single is using. However, the reconfigure_io_chain() registers are in the PRM module, so it still should be the PRM driver managing it rather than pinctrl-single that's in the SCM module as they can at least in theory live a separate power state lifes. But in any case, things will get simpler once the dependencies to the legacy code are cut. Regards, Tony
* Linus Walleij <linus.walleij@linaro.org> [131010 09:19]: > On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Roger Quadros <rogerq@ti.com> [131010 06:32]: > >> > >> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts > >> while the system is still running and only the EHCI controller is runtime suspended. > >> > >> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain > >> whenever the pad wakeup_enable bit is changed. > > > > Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the > > pcs_irq_handle() like the comments there suggest? At least for me that keeps > > the wake-up interrupts continuously running on omap3 instead of just idle modes. > > If the rearm() function is calling this _reconfigure_io_chain my comments > on the fact that this is something that should be handled by the pin > control driver still apply I think .... Yes, except that the reconfigure_io_chain registers are in the PRM module, not in the SCM module where the pinctrl registers are.. And that shared PRM interrupt is used mostly for the internal domain wake-ups, so we should keep that in the PRM driver. Regards, Tony
* Tony Lindgren <tony@atomide.com> [131010 09:09]: > * Roger Quadros <rogerq@ti.com> [131010 06:32]: > > > > I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts > > while the system is still running and only the EHCI controller is runtime suspended. > > > > It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain > > whenever the pad wakeup_enable bit is changed. > > Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the > pcs_irq_handle() like the comments there suggest? At least for me that keeps > the wake-up interrupts continuously running on omap3 instead of just idle modes. > > Now on omap4, I've noticed the wake up interrupts are on all the time based on tests > with the serial driver. Oh, and if you're runtime suspending EHCI only, and if the EHCI module has wake-up registers, it should be able to wake EHCI from retention on it's own without a need for the io chain at all. At least the serial driver does that if it's internal wake-up registers are configured right. But for off-idle, the serial driver needs the io chain wake-up events. Regards, Tony
On Thu, Oct 10, 2013 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote: > * Linus Walleij <linus.walleij@linaro.org> [131010 09:19]: >> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <tony@atomide.com> wrote: >> > * Roger Quadros <rogerq@ti.com> [131010 06:32]: >> >> >> >> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts >> >> while the system is still running and only the EHCI controller is runtime suspended. >> >> >> >> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain >> >> whenever the pad wakeup_enable bit is changed. >> > >> > Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the >> > pcs_irq_handle() like the comments there suggest? At least for me that keeps >> > the wake-up interrupts continuously running on omap3 instead of just idle modes. >> >> If the rearm() function is calling this _reconfigure_io_chain my comments >> on the fact that this is something that should be handled by the pin >> control driver still apply I think .... > > Yes, except that the reconfigure_io_chain registers are in the PRM module, not in > the SCM module where the pinctrl registers are.. And that shared PRM interrupt is > used mostly for the internal domain wake-ups, so we should keep that in the PRM > driver. That depends. One-iorange-equals-one-driver is a fallacy, especially given that MFD for memory-mapped things exist for a reason. What the pin control driver should do is control the pins. Whether the registers are spread out in the entire IO-memory does not matter. We did have one system which placed the IO-muxing together with each peripheral (!) and I did still want that to be handled by a single pinctrl driver picking out windows to all these IO-ranges. Things like the PRM which has (my guess) a gazillion registers related to its deep-core SoC stuff should be handled by things like drivers/mfd/syscon.c, which means it is dead simple for some other driver using "just this one register" in that range to get a handle at it and poke it using syscon_node_to_regmap() (just derference an ampersand ref) syscon_regmap_lookup_by_compatible() (use a compatible string) all returning a regmap * that you can use to poke these registers. Yours, Linus Walleij
On 10/10/2013 07:23 PM, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [131010 09:09]: >> * Roger Quadros <rogerq@ti.com> [131010 06:32]: >>> >>> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts >>> while the system is still running and only the EHCI controller is runtime suspended. >>> >>> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain >>> whenever the pad wakeup_enable bit is changed. >> >> Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the >> pcs_irq_handle() like the comments there suggest? At least for me that keeps >> the wake-up interrupts continuously running on omap3 instead of just idle modes. >> >> Now on omap4, I've noticed the wake up interrupts are on all the time based on tests >> with the serial driver. > > Oh, and if you're runtime suspending EHCI only, and if the EHCI module has > wake-up registers, it should be able to wake EHCI from retention on it's own > without a need for the io chain at all. > The problem is that the asynchronous wake up mechanism for USB Host module is broken in the design so we have to rely on IO daisy chain every time. :( cheers, -roger
On 10/10/2013 07:00 PM, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [131010 06:32]: >> >> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts >> while the system is still running and only the EHCI controller is runtime suspended. >> >> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain >> whenever the pad wakeup_enable bit is changed. > > Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the > pcs_irq_handle() like the comments there suggest? At least for me that keeps > the wake-up interrupts continuously running on omap3 instead of just idle modes. Yes it is on OMAP3. I haven't tried with pcs_soc->rearm(). I will give it a try and let you know. > > Now on omap4, I've noticed the wake up interrupts are on all the time based on tests > with the serial driver. > >> I think pcs_irq_set_wake() is where need to control system wakeup behaviour for the irq. >> This is where we should be able to change WAKEUP_EN bit of the pad >> to enable/disable system wakeup for that pad and also call _reconfigure_io_chain(). > > Well the irq_set_wake() should only be needed for suspend and resume. For runtime PM > the wake-events should be always enabled by default as pointed out by Alan Stern > a while back. Right, but we need to update the WAKEUP_EN bit in the pad control register for that to work, no?. This is something we are not doing in the driver. > >> This would mean that we don't really need to set WAKEUP_EN for the pads in the DTS file. > > Well for runtime PM, we should also do the automatic handling if configured. > But how to do that best is still open.. I didn't get this part. I was thinking that irq_set_wake() should map directly to WAKEUP_EN bit for the pin. cheers, -roger
On 10/11/2013 11:00 AM, Linus Walleij wrote: > On Thu, Oct 10, 2013 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote: >> * Linus Walleij <linus.walleij@linaro.org> [131010 09:19]: >>> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <tony@atomide.com> wrote: >>>> * Roger Quadros <rogerq@ti.com> [131010 06:32]: >>>>> >>>>> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts >>>>> while the system is still running and only the EHCI controller is runtime suspended. >>>>> >>>>> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain >>>>> whenever the pad wakeup_enable bit is changed. >>>> >>>> Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the >>>> pcs_irq_handle() like the comments there suggest? At least for me that keeps >>>> the wake-up interrupts continuously running on omap3 instead of just idle modes. >>> >>> If the rearm() function is calling this _reconfigure_io_chain my comments >>> on the fact that this is something that should be handled by the pin >>> control driver still apply I think .... >> >> Yes, except that the reconfigure_io_chain registers are in the PRM module, not in >> the SCM module where the pinctrl registers are.. And that shared PRM interrupt is >> used mostly for the internal domain wake-ups, so we should keep that in the PRM >> driver. > > That depends. > > One-iorange-equals-one-driver is a fallacy, especially given that MFD for > memory-mapped things exist for a reason. +1 Another place I faced a similar problem was the OMAP control module, which contains registers for a number of different non related peripherals. (e.g. PHY for USB, SATA, Display clock, etc) > > What the pin control driver should do is control the pins. Whether the registers > are spread out in the entire IO-memory does not matter. We did have one system > which placed the IO-muxing together with each peripheral (!) and I did > still want > that to be handled by a single pinctrl driver picking out windows to all these > IO-ranges. > > Things like the PRM which has (my guess) a gazillion registers related to its > deep-core SoC stuff should be handled by things like > drivers/mfd/syscon.c, which means it is dead simple for some other driver > using "just this one register" in that range to get a handle at it and poke it > using syscon_node_to_regmap() (just derference an ampersand ref) > syscon_regmap_lookup_by_compatible() (use a compatible string) > all returning a regmap * that you can use to poke these registers. The register handling is fine. But how do we deal with resource handling? e.g. the block that has the deep-core registers might need to be clocked or powered before the registers can be accessed. cheers, -roger
On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <rogerq@ti.com> wrote: > The register handling is fine. But how do we deal with resource handling? > e.g. the block that has the deep-core registers might need to be clocked or powered > before the registers can be accessed. Yeah I saw this in the code there. In this case it seems syscon-type regmap access can be used to read/write the registers, so maybe the pin controller also need to get a handle on some clock etc? The general idea is however that large monolitic "drivers" for a certain IO-range such as arch/arm/mach-omap2/prm3xxx.c doesn't scale - we saw this with the Ux500 PRCMU driver in mfd/* to the point that our patches to extend it were NACK:ed until we refactor it. This stuff in mach-omap2 is in the same bad design pattern, and need to get out of it. The approach chosen for the Ux500 PRCMU was to distribute out the driver into the places where it's actually used, like the clock driver etc. The accessor functions to do some stuff over in the PRCMU was just adding a layer of cruft. Yours, Linus Walleij
On 10/11/2013 11:49 AM, Roger Quadros wrote: > On 10/10/2013 07:00 PM, Tony Lindgren wrote: >> * Roger Quadros <rogerq@ti.com> [131010 06:32]: >>> >>> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts >>> while the system is still running and only the EHCI controller is runtime suspended. >>> >>> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain >>> whenever the pad wakeup_enable bit is changed. >> >> Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the >> pcs_irq_handle() like the comments there suggest? At least for me that keeps >> the wake-up interrupts continuously running on omap3 instead of just idle modes. > > Yes it is on OMAP3. I haven't tried with pcs_soc->rearm(). I will give it a try and > let you know. >> >> Now on omap4, I've noticed the wake up interrupts are on all the time based on tests >> with the serial driver. >> >>> I think pcs_irq_set_wake() is where need to control system wakeup behaviour for the irq. >>> This is where we should be able to change WAKEUP_EN bit of the pad >>> to enable/disable system wakeup for that pad and also call _reconfigure_io_chain(). >> >> Well the irq_set_wake() should only be needed for suspend and resume. For runtime PM >> the wake-events should be always enabled by default as pointed out by Alan Stern >> a while back. > > Right, but we need to update the WAKEUP_EN bit in the pad control register for that > to work, no?. This is something we are not doing in the driver. OK sorry, just figured out that we are doing it indeed in pcs_irq_set(). Wasn't able to test it yet with USB. But I don't see any issues except that pcs_soc->rearm() needs to be called from pcs_irq_set() instead of from pcs_irq_unmask(). After that you can add my Reviewed-by: Roger Quadros <rogerq@ti.com> cheers, -roger
* Roger Quadros <rogerq@ti.com> [131011 07:07]: > On 10/11/2013 11:49 AM, Roger Quadros wrote: > > On 10/10/2013 07:00 PM, Tony Lindgren wrote: > >> > >> Well the irq_set_wake() should only be needed for suspend and resume. For runtime PM > >> the wake-events should be always enabled by default as pointed out by Alan Stern > >> a while back. > > > > Right, but we need to update the WAKEUP_EN bit in the pad control register for that > > to work, no?. This is something we are not doing in the driver. > > OK sorry, just figured out that we are doing it indeed in pcs_irq_set(). > Wasn't able to test it yet with USB. But I don't see any issues except that > pcs_soc->rearm() needs to be called from pcs_irq_set() instead of from pcs_irq_unmask(). Hmm that sounds like a different behaviour compared to what I'm seeing on omap3. Care to send a little fix on top of these patches so I can test it with my set up too? It seems that the only difference would be that we're calling rearm() after both masking and unmasking, which seemed unnecessary to me earlier. > After that you can add my > > Reviewed-by: Roger Quadros <rogerq@ti.com> Thanks, for testing, sorry I already pushed them out after Kevin ran his PM tests on them. Regards, Tony
* Roger Quadros <rogerq@ti.com> [131011 02:04]: > On 10/11/2013 11:00 AM, Linus Walleij wrote: > > On Thu, Oct 10, 2013 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote: > >> * Linus Walleij <linus.walleij@linaro.org> [131010 09:19]: > >>> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <tony@atomide.com> wrote: > >>>> * Roger Quadros <rogerq@ti.com> [131010 06:32]: > >>>>> > >>>>> I tried testing this with the USB EHCI driver, but I'm not getting wake up interrupts > >>>>> while the system is still running and only the EHCI controller is runtime suspended. > >>>>> > >>>>> It seems we need to somehow call _reconfigure_io_chain() to update the daisy chain > >>>>> whenever the pad wakeup_enable bit is changed. > >>>> > >>>> Sounds like this is on omap3? Have you tried calling pcs_soc->rearm() in the > >>>> pcs_irq_handle() like the comments there suggest? At least for me that keeps > >>>> the wake-up interrupts continuously running on omap3 instead of just idle modes. > >>> > >>> If the rearm() function is calling this _reconfigure_io_chain my comments > >>> on the fact that this is something that should be handled by the pin > >>> control driver still apply I think .... > >> > >> Yes, except that the reconfigure_io_chain registers are in the PRM module, not in > >> the SCM module where the pinctrl registers are.. And that shared PRM interrupt is > >> used mostly for the internal domain wake-ups, so we should keep that in the PRM > >> driver. > > > > That depends. > > > > One-iorange-equals-one-driver is a fallacy, especially given that MFD for > > memory-mapped things exist for a reason. > > +1 > > Another place I faced a similar problem was the OMAP control module, which contains > registers for a number of different non related peripherals. (e.g. PHY for USB, SATA, > Display clock, etc) Guys, we really need to keep the register access between hardware modules under control because the hardware blocks can live separate life from clocking and power state point of view. Regmap could be probably used for the register access across various hardware modules in a controlled way that is also aware of the clocking and PM state of the hardware modules in question. As long as it's done sanely, I'm OK with that. But for any other kind of random direct register tinkering between hardware modules, that's a no no. > > What the pin control driver should do is control the pins. Whether the registers > > are spread out in the entire IO-memory does not matter. We did have one system > > which placed the IO-muxing together with each peripheral (!) and I did > > still want > > that to be handled by a single pinctrl driver picking out windows to all these > > IO-ranges. > > > > Things like the PRM which has (my guess) a gazillion registers related to its > > deep-core SoC stuff should be handled by things like > > drivers/mfd/syscon.c, which means it is dead simple for some other driver > > using "just this one register" in that range to get a handle at it and poke it > > using syscon_node_to_regmap() (just derference an ampersand ref) > > syscon_regmap_lookup_by_compatible() (use a compatible string) > > all returning a regmap * that you can use to poke these registers. > > The register handling is fine. But how do we deal with resource handling? > e.g. the block that has the deep-core registers might need to be clocked or powered > before the registers can be accessed. Right, that's the key issue here. The register access would have to be conditional based on the hardware modules PM state. Otherwise we'll have hard to trace hangs and oopses. Regards, Tony
On Friday 11 October 2013 09:06 PM, Tony Lindgren wrote: >>> What the pin control driver should do is control the pins. Whether the registers >>> are spread out in the entire IO-memory does not matter. We did have one system >>> which placed the IO-muxing together with each peripheral (!) and I did >>> still want >>> that to be handled by a single pinctrl driver picking out windows to all these >>> IO-ranges. >>> >>> Things like the PRM which has (my guess) a gazillion registers related to its >>> deep-core SoC stuff should be handled by things like >>> drivers/mfd/syscon.c, which means it is dead simple for some other driver >>> using "just this one register" in that range to get a handle at it and poke it >>> using syscon_node_to_regmap() (just derference an ampersand ref) >>> syscon_regmap_lookup_by_compatible() (use a compatible string) >>> all returning a regmap * that you can use to poke these registers. >> >> The register handling is fine. But how do we deal with resource handling? >> e.g. the block that has the deep-core registers might need to be clocked or powered >> before the registers can be accessed. > > Right, that's the key issue here. The register access would have to be conditional > based on the hardware modules PM state. Otherwise we'll have hard to trace hangs > and oopses. > Hi Tony, How are the clocks/power state currently handled in case of omap4_pmx_core, omap4_pmx_wkup register access via pinctrl-single ? Thanks and Regards, Balaji T K
* Linus Walleij <linus.walleij@linaro.org> [131011 03:40]: > On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <rogerq@ti.com> wrote: > > > The register handling is fine. But how do we deal with resource handling? > > e.g. the block that has the deep-core registers might need to be clocked or powered > > before the registers can be accessed. > > Yeah I saw this in the code there. > > In this case it seems syscon-type regmap access can be used to > read/write the registers, so maybe the pin controller also need to > get a handle on some clock etc? Uhh, let's not go there.. The resource availability needs to be handled transparently in regmap code and the reg provider hardware module driver using runtime PM. > The general idea is however that large monolitic "drivers" for a > certain IO-range such as arch/arm/mach-omap2/prm3xxx.c > doesn't scale - we saw this with the Ux500 PRCMU driver in > mfd/* to the point that our patches to extend it were NACK:ed > until we refactor it. This stuff in mach-omap2 is in the same bad > design pattern, and need to get out of it. > > The approach chosen for the Ux500 PRCMU was to distribute > out the driver into the places where it's actually used, like the > clock driver etc. The accessor functions to do some stuff over > in the PRCMU was just adding a layer of cruft. Yes I'm all in favor of having a minimal PRM core driver that manages resources and provides register access services in a controlled way to it's client drivers. As long as the emphasis is "in a controlled way". Regards, Tony
* Balaji T K <balajitk@ti.com> [131011 08:51]: > On Friday 11 October 2013 09:06 PM, Tony Lindgren wrote: > >>>What the pin control driver should do is control the pins. Whether the registers > >>>are spread out in the entire IO-memory does not matter. We did have one system > >>>which placed the IO-muxing together with each peripheral (!) and I did > >>>still want > >>>that to be handled by a single pinctrl driver picking out windows to all these > >>>IO-ranges. > >>> > >>>Things like the PRM which has (my guess) a gazillion registers related to its > >>>deep-core SoC stuff should be handled by things like > >>>drivers/mfd/syscon.c, which means it is dead simple for some other driver > >>>using "just this one register" in that range to get a handle at it and poke it > >>>using syscon_node_to_regmap() (just derference an ampersand ref) > >>>syscon_regmap_lookup_by_compatible() (use a compatible string) > >>>all returning a regmap * that you can use to poke these registers. > >> > >>The register handling is fine. But how do we deal with resource handling? > >>e.g. the block that has the deep-core registers might need to be clocked or powered > >>before the registers can be accessed. > > > >Right, that's the key issue here. The register access would have to be conditional > >based on the hardware modules PM state. Otherwise we'll have hard to trace hangs > >and oopses. > > > Hi Tony, > > How are the clocks/power state currently handled in case of omap4_pmx_core, > omap4_pmx_wkup register access via pinctrl-single ? It's currently always on during runtime and managed in for the whole SCM by mach-omap2/control.c. Then there's a separate SCM register that triggers the save and restore of the padconf registers in hardware for off-idle along with other SCM related things, see the *_control_save/restore_context() functions. Regards, Tony
On Fri, Oct 11, 2013 at 5:43 PM, Tony Lindgren <tony@atomide.com> wrote: > * Linus Walleij <linus.walleij@linaro.org> [131011 03:40]: >> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <rogerq@ti.com> wrote: >> >> > The register handling is fine. But how do we deal with resource handling? >> > e.g. the block that has the deep-core registers might need to be clocked or powered >> > before the registers can be accessed. >> >> Yeah I saw this in the code there. >> >> In this case it seems syscon-type regmap access can be used to >> read/write the registers, so maybe the pin controller also need to >> get a handle on some clock etc? > > Uhh, let's not go there.. The resource availability needs to be > handled transparently in regmap code and the reg provider hardware > module driver using runtime PM. OK we can surely discuss this with broonie, it makes sense to have regmap be able to do its deed transparently. Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [131011 09:05]: > On Fri, Oct 11, 2013 at 5:43 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Linus Walleij <linus.walleij@linaro.org> [131011 03:40]: > >> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <rogerq@ti.com> wrote: > >> > >> > The register handling is fine. But how do we deal with resource handling? > >> > e.g. the block that has the deep-core registers might need to be clocked or powered > >> > before the registers can be accessed. > >> > >> Yeah I saw this in the code there. > >> > >> In this case it seems syscon-type regmap access can be used to > >> read/write the registers, so maybe the pin controller also need to > >> get a handle on some clock etc? > > > > Uhh, let's not go there.. The resource availability needs to be > > handled transparently in regmap code and the reg provider hardware > > module driver using runtime PM. > > OK we can surely discuss this with broonie, it makes sense to > have regmap be able to do its deed transparently. Yes I think so too. Sounds like we need callbacks for the runtime PM checks to the "register provider" driver. Hwo knows, maybe those features are there already :) Tony
On Friday 11 October 2013 09:31 PM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [131011 09:05]: >> On Fri, Oct 11, 2013 at 5:43 PM, Tony Lindgren <tony@atomide.com> wrote: >>> * Linus Walleij <linus.walleij@linaro.org> [131011 03:40]: >>>> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <rogerq@ti.com> wrote: >>>> >>>>> The register handling is fine. But how do we deal with resource handling? >>>>> e.g. the block that has the deep-core registers might need to be clocked or powered >>>>> before the registers can be accessed. >>>> >>>> Yeah I saw this in the code there. >>>> >>>> In this case it seems syscon-type regmap access can be used to >>>> read/write the registers, so maybe the pin controller also need to >>>> get a handle on some clock etc? >>> >>> Uhh, let's not go there.. The resource availability needs to be >>> handled transparently in regmap code and the reg provider hardware >>> module driver using runtime PM. >> >> OK we can surely discuss this with broonie, it makes sense to >> have regmap be able to do its deed transparently. > > Yes I think so too. Sounds like we need callbacks for the runtime PM > checks to the "register provider" driver. Hwo knows, maybe those > features are there already :) > Hi Tony, Any conclusion on using regmap for omap control module non-mux registers ? Thanks and Regards, Balaji T K
* Balaji T K <balajitk@ti.com> [131018 00:41]: > > Any conclusion on using regmap for omap control module non-mux registers ? I don't think anybody has even started looking into a SCM driver yet considering there are tons of other issues to sort out. If you're thinking about implementing the MMC PBIAS driver, I would just implement it as a standalone driver. It seems that the PBIAS interface to the MMC driver can be just regulator framework. Then when we have the SCM driver available, this driver can be updated to coordinate things with the core SCM driver. Regards, Tony
On Friday 18 October 2013 09:05 PM, Tony Lindgren wrote: > * Balaji T K <balajitk@ti.com> [131018 00:41]: >> >> Any conclusion on using regmap for omap control module non-mux registers ? > > I don't think anybody has even started looking into a SCM driver > yet considering there are tons of other issues to sort out. > > If you're thinking about implementing the MMC PBIAS driver, I would > just implement it as a standalone driver. It seems that the PBIAS > interface to the MMC driver can be just regulator framework. > Hi Tony, I am testing pbias regulator, Just thinking whether it should be using regmap api or direct readl/writel > Then when we have the SCM driver available, this driver can be > updated to coordinate things with the core SCM driver. Do you see regmap getting used for the SCM driver ? Thanks and Regards, Balaji T K > > Regards, > > Tony >
* Balaji T K <balajitk@ti.com> [131018 08:59]: > On Friday 18 October 2013 09:05 PM, Tony Lindgren wrote: > >* Balaji T K <balajitk@ti.com> [131018 00:41]: > >> > >>Any conclusion on using regmap for omap control module non-mux registers ? > > > >I don't think anybody has even started looking into a SCM driver > >yet considering there are tons of other issues to sort out. > > > >If you're thinking about implementing the MMC PBIAS driver, I would > >just implement it as a standalone driver. It seems that the PBIAS > >interface to the MMC driver can be just regulator framework. > > > Hi Tony, > > I am testing pbias regulator, Just thinking whether it should be > using regmap api or direct readl/writel OK either or should work. It seems that can easily be changed later on though. > >Then when we have the SCM driver available, this driver can be > >updated to coordinate things with the core SCM driver. > > Do you see regmap getting used for the SCM driver ? Yes eventually :) Tony
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt index 5a02e30..7069a0b 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt @@ -72,6 +72,13 @@ Optional properties: /* pin base, nr pins & gpio function */ pinctrl-single,gpio-range = <&range 0 3 0 &range 3 9 1>; +- interrupt-controller : standard interrupt controller binding if using + interrupts for wake-up events for example. In this case pinctrl-single + is set up as a chained interrupt controller and the wake-up interrupts + can be requested by the drivers using request_irq(). + +- #interrupt-cells : standard interrupt binding if using interrupts + This driver assumes that there is only one register for each pin (unless the pinctrl-single,bit-per-mux is set), and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt document in this directory. @@ -121,6 +128,8 @@ pmx_core: pinmux@4a100040 { reg = <0x4a100040 0x0196>; #address-cells = <1>; #size-cells = <0>; + #interrupt-cells = <1>; + interrupt-controller; pinctrl-single,register-width = <16>; pinctrl-single,function-mask = <0xffff>; }; @@ -131,6 +140,8 @@ pmx_wkup: pinmux@4a31e040 { reg = <0x4a31e040 0x0038>; #address-cells = <1>; #size-cells = <0>; + #interrupt-cells = <1>; + interrupt-controller; pinctrl-single,register-width = <16>; pinctrl-single,function-mask = <0xffff>; }; diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index f88d3d1..b4ff055 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -15,10 +15,14 @@ #include <linux/slab.h> #include <linux/err.h> #include <linux/list.h> +#include <linux/interrupt.h> + +#include <linux/irqchip/chained_irq.h> #include <linux/of.h> #include <linux/of_device.h> #include <linux/of_address.h> +#include <linux/of_irq.h> #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> @@ -152,9 +156,15 @@ struct pcs_name { /** * struct pcs_soc_data - SoC specific settings * @flags: initial SoC specific PCS_FEAT_xxx values + * @irq: optional interrupt for the controller + * @irq_enable_mask: optional SoC specific interrupt enable mask + * @irq_status_mask: optional SoC specific interrupt status mask */ struct pcs_soc_data { unsigned flags; + int irq; + unsigned irq_enable_mask; + unsigned irq_status_mask; }; /** @@ -165,6 +175,7 @@ struct pcs_soc_data { * @dev: device entry * @pctl: pin controller device * @flags: mask of PCS_FEAT_xxx values + * @lock: spinlock for register access * @mutex: mutex protecting the lists * @width: bits per mux register * @fmask: function register mask @@ -179,6 +190,9 @@ struct pcs_soc_data { * @pingroups: list of pingroups * @functions: list of functions * @gpiofuncs: list of gpio functions + * @irqs: list of interrupt registers + * @chip: chip container for this instance + * @domain: IRQ domain for this instance * @ngroups: number of pingroups * @nfuncs: number of functions * @desc: pin controller descriptor @@ -192,7 +206,11 @@ struct pcs_device { struct device *dev; struct pinctrl_dev *pctl; unsigned flags; +#define PCS_QUIRK_SHARED_IRQ (1 << 2) +#define PCS_FEAT_IRQ (1 << 1) #define PCS_FEAT_PINCONF (1 << 0) + struct pcs_soc_data socdata; + raw_spinlock_t lock; struct mutex mutex; unsigned width; unsigned fmask; @@ -208,6 +226,9 @@ struct pcs_device { struct list_head pingroups; struct list_head functions; struct list_head gpiofuncs; + struct list_head irqs; + struct irq_chip chip; + struct irq_domain *domain; unsigned ngroups; unsigned nfuncs; struct pinctrl_desc desc; @@ -215,6 +236,8 @@ struct pcs_device { void (*write)(unsigned val, void __iomem *reg); }; +#define PCS_QUIRK_HAS_SHARED_IRQ (pcs->flags & PCS_QUIRK_SHARED_IRQ) +#define PCS_HAS_IRQ (pcs->flags & PCS_FEAT_IRQ) #define PCS_HAS_PINCONF (pcs->flags & PCS_FEAT_PINCONF) static int pcs_pinconf_get(struct pinctrl_dev *pctldev, unsigned pin, @@ -440,9 +463,11 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, for (i = 0; i < func->nvals; i++) { struct pcs_func_vals *vals; + unsigned long flags; unsigned val, mask; vals = &func->vals[i]; + raw_spin_lock_irqsave(&pcs->lock, flags); val = pcs->read(vals->reg); if (pcs->bits_per_mux) @@ -453,6 +478,7 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, val &= ~mask; val |= (vals->val & mask); pcs->write(val, vals->reg); + raw_spin_unlock_irqrestore(&pcs->lock, flags); } return 0; @@ -494,13 +520,16 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector, for (i = 0; i < func->nvals; i++) { struct pcs_func_vals *vals; + unsigned long flags; unsigned val; vals = &func->vals[i]; + raw_spin_lock_irqsave(&pcs->lock, flags); val = pcs->read(vals->reg); val &= ~pcs->fmask; val |= pcs->foff << pcs->fshift; pcs->write(val, vals->reg); + raw_spin_unlock_irqrestore(&pcs->lock, flags); } } @@ -1451,11 +1480,33 @@ static void pcs_free_pingroups(struct pcs_device *pcs) } /** + * pcs_irq_free() - free interrupt + * @pcs: pcs driver instance + */ +static void pcs_irq_free(struct pcs_device *pcs) +{ + struct pcs_soc_data *pcs_soc = &pcs->socdata; + + if (pcs_soc->irq < 0) + return; + + if (pcs->domain) + irq_domain_remove(pcs->domain); + + if (PCS_QUIRK_HAS_SHARED_IRQ) + free_irq(pcs_soc->irq, pcs_soc); + else + irq_set_chained_handler(pcs_soc->irq, NULL); +} + +/** * pcs_free_resources() - free memory used by this driver * @pcs: pcs driver instance */ static void pcs_free_resources(struct pcs_device *pcs) { + pcs_irq_free(pcs); + if (pcs->pctl) pinctrl_unregister(pcs->pctl); @@ -1504,6 +1555,259 @@ static int pcs_add_gpio_func(struct device_node *node, struct pcs_device *pcs) } return ret; } +/** + * @reg: virtual address of interrupt register + * @hwirq: hardware irq number + * @irq: virtual irq number + * @node: list node + */ +struct pcs_interrupt { + void __iomem *reg; + irq_hw_number_t hwirq; + unsigned int irq; + struct list_head node; +}; + +/** + * pcs_irq_set() - enables or disables an interrupt + * + * Note that this currently assumes one interrupt per pinctrl + * register that is typically used for wake-up events. + */ +static inline void pcs_irq_set(struct pcs_soc_data *pcs_soc, + int irq, const bool enable) +{ + struct pcs_device *pcs; + struct list_head *pos; + unsigned mask; + + pcs = container_of(pcs_soc, struct pcs_device, socdata); + list_for_each(pos, &pcs->irqs) { + struct pcs_interrupt *pcswi; + unsigned soc_mask; + + pcswi = list_entry(pos, struct pcs_interrupt, node); + if (irq != pcswi->hwirq) + continue; + + soc_mask = pcs_soc->irq_enable_mask; + raw_spin_lock(&pcs->lock); + mask = pcs->read(pcswi->reg); + if (enable) + mask &= ~soc_mask; + else + mask |= soc_mask; + pcs->write(mask, pcswi->reg); + raw_spin_unlock(&pcs->lock); + } +} + +/** + * pcs_irq_mask() - mask pinctrl interrupt + * @d: interrupt data + */ +static void pcs_irq_mask(struct irq_data *d) +{ + struct pcs_soc_data *pcs_soc = irq_data_get_irq_chip_data(d); + + pcs_irq_set(pcs_soc, d->irq, false); +} + +/** + * pcs_irq_unmask() - unmask pinctrl interrupt + * @d: interrupt data + */ +static void pcs_irq_unmask(struct irq_data *d) +{ + struct pcs_soc_data *pcs_soc = irq_data_get_irq_chip_data(d); + + pcs_irq_set(pcs_soc, d->irq, true); +} + +/** + * pcs_irq_set_wake() - toggle the suspend and resume wake up + * @d: interrupt data + * @state: wake-up state + * + * Note that this should be called only for suspend and resume. + * For runtime PM, the wake-up events should be enabled by default. + */ +static int pcs_irq_set_wake(struct irq_data *d, unsigned int state) +{ + if (state) + pcs_irq_unmask(d); + else + pcs_irq_mask(d); + + return 0; +} + +/** + * pcs_irq_handle() - common interrupt handler + * @pcs_irq: interrupt data + * + * Note that this currently assumes we have one interrupt bit per + * mux register. This interrupt is typically used for wake-up events. + * For more complex interrupts different handlers can be specified. + */ +static int pcs_irq_handle(struct pcs_soc_data *pcs_soc) +{ + struct pcs_device *pcs; + struct list_head *pos; + int count = 0; + + pcs = container_of(pcs_soc, struct pcs_device, socdata); + list_for_each(pos, &pcs->irqs) { + struct pcs_interrupt *pcswi; + unsigned mask; + + pcswi = list_entry(pos, struct pcs_interrupt, node); + raw_spin_lock(&pcs->lock); + mask = pcs->read(pcswi->reg); + raw_spin_unlock(&pcs->lock); + if (mask & pcs_soc->irq_status_mask) { + generic_handle_irq(irq_find_mapping(pcs->domain, + pcswi->hwirq)); + count++; + } + } + + return count; +} + +/** + * pcs_irq_handler() - handler for the shared interrupt case + * @irq: interrupt + * @d: data + * + * Use this for cases where multiple instances of + * pinctrl-single share a single interrupt like on omaps. + */ +static irqreturn_t pcs_irq_handler(int irq, void *d) +{ + struct pcs_soc_data *pcs_soc = d; + + return pcs_irq_handle(pcs_soc) ? IRQ_HANDLED : IRQ_NONE; +} + +/** + * pcs_irq_handle() - handler for the dedicated chained interrupt case + * @irq: interrupt + * @desc: interrupt descriptor + * + * Use this if you have a separate interrupt for each + * pinctrl-single instance. + */ +static void pcs_irq_chain_handler(unsigned int irq, struct irq_desc *desc) +{ + struct pcs_soc_data *pcs_soc = irq_desc_get_handler_data(desc); + struct irq_chip *chip; + int res; + + chip = irq_get_chip(irq); + chained_irq_enter(chip, desc); + res = pcs_irq_handle(pcs_soc); + if (!res) + handle_bad_irq(irq, desc); + chained_irq_exit(chip, desc); + + return; +} + +static int pcs_irqdomain_map(struct irq_domain *d, unsigned int irq, + irq_hw_number_t hwirq) +{ + struct pcs_soc_data *pcs_soc = d->host_data; + struct pcs_device *pcs; + struct pcs_interrupt *pcswi; + + pcs = container_of(pcs_soc, struct pcs_device, socdata); + pcswi = devm_kzalloc(pcs->dev, sizeof(*pcswi), GFP_KERNEL); + if (!pcswi) + return -ENOMEM; + + pcswi->reg = pcs->base + hwirq; + pcswi->hwirq = hwirq; + pcswi->irq = irq; + + mutex_lock(&pcs->mutex); + list_add_tail(&pcswi->node, &pcs->irqs); + mutex_unlock(&pcs->mutex); + + irq_set_chip_data(irq, pcs_soc); + irq_set_chip_and_handler(irq, &pcs->chip, + handle_level_irq); + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); + + return 0; +} + +static struct irq_domain_ops pcs_irqdomain_ops = { + .map = pcs_irqdomain_map, + .xlate = irq_domain_xlate_onecell, +}; + +/** + * pcs_irq_init_chained_handler() - set up a chained interrupt handler + * @pcs: pcs driver instance + * @np: device node pointer + */ +static int pcs_irq_init_chained_handler(struct pcs_device *pcs, + struct device_node *np) +{ + struct pcs_soc_data *pcs_soc = &pcs->socdata; + const char *name = "pinctrl"; + int num_irqs; + + if (!pcs_soc->irq_enable_mask || + !pcs_soc->irq_status_mask) { + pcs_soc->irq = -1; + return -EINVAL; + } + + INIT_LIST_HEAD(&pcs->irqs); + pcs->chip.name = name; + pcs->chip.irq_ack = pcs_irq_mask; + pcs->chip.irq_mask = pcs_irq_mask; + pcs->chip.irq_unmask = pcs_irq_unmask; + pcs->chip.irq_set_wake = pcs_irq_set_wake; + + if (PCS_QUIRK_HAS_SHARED_IRQ) { + int res; + + res = request_irq(pcs_soc->irq, pcs_irq_handler, + IRQF_SHARED | IRQF_NO_SUSPEND, + name, pcs_soc); + if (res) { + pcs_soc->irq = -1; + return res; + } + } else { + irq_set_handler_data(pcs_soc->irq, pcs_soc); + irq_set_chained_handler(pcs_soc->irq, + pcs_irq_chain_handler); + } + + /* FIXME: Test rmmod */ + + /* + * We can use the register offset as the hardirq + * number as irq_domain_add_simple maps them lazily. + * This way we can easily support more than one + * interrupt per function if needed. + */ + num_irqs = pcs->size; + + pcs->domain = irq_domain_add_simple(np, num_irqs, 0, + &pcs_irqdomain_ops, + pcs_soc); + if (!pcs->domain) { + irq_set_chained_handler(pcs_soc->irq, NULL); + return -EINVAL; + } + + return 0; +} #ifdef CONFIG_PM static int pinctrl_single_suspend(struct platform_device *pdev, @@ -1549,12 +1853,14 @@ static int pcs_probe(struct platform_device *pdev) return -ENOMEM; } pcs->dev = &pdev->dev; + raw_spin_lock_init(&pcs->lock); mutex_init(&pcs->mutex); INIT_LIST_HEAD(&pcs->pingroups); INIT_LIST_HEAD(&pcs->functions); INIT_LIST_HEAD(&pcs->gpiofuncs); soc = match->data; pcs->flags = soc->flags; + memcpy(&pcs->socdata, soc, sizeof(*soc)); PCS_GET_PROP_U32("pinctrl-single,register-width", &pcs->width, "register width not specified\n"); @@ -1642,6 +1948,16 @@ static int pcs_probe(struct platform_device *pdev) if (ret < 0) goto free; + pcs->socdata.irq = irq_of_parse_and_map(np, 0); + if (pcs->socdata.irq) + pcs->flags |= PCS_FEAT_IRQ; + + if (PCS_HAS_IRQ) { + ret = pcs_irq_init_chained_handler(pcs, np); + if (ret < 0) + dev_warn(pcs->dev, "initialized with no interrupts\n"); + } + dev_info(pcs->dev, "%i pins at pa %p size %u\n", pcs->desc.npins, pcs->base, pcs->size); @@ -1665,6 +1981,12 @@ static int pcs_remove(struct platform_device *pdev) return 0; } +static const struct pcs_soc_data pinctrl_single_omap_wkup = { + .flags = PCS_QUIRK_SHARED_IRQ, + .irq_enable_mask = (1 << 14), /* OMAP_WAKEUP_EN */ + .irq_status_mask = (1 << 15), /* OMAP_WAKEUP_EVENT */ +}; + static const struct pcs_soc_data pinctrl_single = { }; @@ -1673,6 +1995,9 @@ static const struct pcs_soc_data pinconf_single = { }; static struct of_device_id pcs_of_match[] = { + { .compatible = "ti,omap3-padconf", .data = &pinctrl_single_omap_wkup }, + { .compatible = "ti,omap4-padconf", .data = &pinctrl_single_omap_wkup }, + { .compatible = "ti,omap5-padconf", .data = &pinctrl_single_omap_wkup }, { .compatible = "pinctrl-single", .data = &pinctrl_single }, { .compatible = "pinconf-single", .data = &pinconf_single }, { },
The pin control registers can have interrupts for example for device wake-up. These interrupts can be treated as a chained interrupt controller as suggested earlier by Linus Walleij <linus.walleij@linaro.org>. This patch adds support for interrupts in a way that should be pretty generic, and works for the omaps that support wake-up interrupts. On omaps, there's an interrupt enable and interrupt status bit for each pin. The two pinctrl domains on omaps share a single interrupt from the PRM chained interrupt handler. Support for other similar hardware should be easy to add. Note that this patch does not attempt to handle the wake-up interrupts automatically unlike the earlier patches. This patch allows the device drivers to do a request_irq() on the wake-up pins as needed. I'll try to do also a separate generic patch for handling the wake-up events automatically. Also note that as this patch makes the pinctrl-single an irq controller, the current bindings need some extra trickery to use interrupts from two different interrupt controllers for the same driver. So it might be worth waiting a little on the patches enabling the wake-up interrupts from drivers as there should be a generic way to handle it coming. And also there's been discussion of interrupts-extended binding for using interrupts from multiple interrupt controllers. In any case, this patch should be ready to go allowing handling the wake-up interrupts in a generic way, or separately from the device drivers. Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> Cc: Grygorii Strashko <grygorii.strashko@ti.com> Cc: Prakash Manjunathappa <prakash.pm@ti.com> Cc: Roger Quadros <rogerq@ti.com> Cc: Haojian Zhuang <haojian.zhuang@linaro.org> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: linux-kernel@vger.kernel.org Cc: Benoît Cousson <bcousson@baylibre.com> Cc: devicetree@vger.kernel.org Signed-off-by: Tony Lindgren <tony@atomide.com> --- .../devicetree/bindings/pinctrl/pinctrl-single.txt | 11 + drivers/pinctrl/pinctrl-single.c | 325 ++++++++++++++++++++ 2 files changed, 336 insertions(+)