LinuxLists.cc - [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Tony Lindgren <[email protected]> [131002 22:51]:
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> +/**
> + * 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;

Oops, this should be pcswi->irq instead, otherwise this will
never match. And it just happened to work for me as I had
the wake-up flags set from the bootloader..

> + 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);
> + }
> +}

And then the if (enable) mask needs to be swapped around for
this to work when the wake up events are not set by the
bootloader :)

> +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;
> +}

Looks like handle_bad_irq is not exported for modules, so I've
just made it into a comment for now.

> + /* FIXME: Test rmmod */

And this can be removed now, I've briefly tested it as a
loadable module by not claiming any pins.

Updated patch below,

Tony

8<---------------------

From: Tony Lindgren <[email protected]>
Date: Wed, 2 Oct 2013 21:39:40 -0700
Subject: [PATCH] pinctrl: single: Add support for wake-up interrupts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <[email protected]>.

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 <[email protected]>
Cc: Grygorii Strashko <[email protected]>
Cc: Prakash Manjunathappa <[email protected]>
Cc: Roger Quadros <[email protected]>
Cc: Haojian Zhuang <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: [email protected]
Cc: Benoît Cousson <[email protected]>
Cc: [email protected]
Signed-off-by: Tony Lindgren <[email protected]>

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..dad65df 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,256 @@ 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->irq)
+ 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);
+ /* REVISIT: export and add 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);
+ }
+
+ /*
+ * 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 +1850,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 +1945,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 +1978,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 +1992,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 },
{ },

2013-10-08 12:10:30

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Thu, Oct 3, 2013 at 7:42 AM, Tony Lindgren <[email protected]> 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

2013-10-08 16:05:45

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Linus Walleij <[email protected]> [131008 05:18]:
> On Thu, Oct 3, 2013 at 7:42 AM, Tony Lindgren <[email protected]> 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

2013-10-10 13:24:46

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

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 <[email protected]>.
>
> 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 <[email protected]>
> Cc: Grygorii Strashko <[email protected]>
> Cc: Prakash Manjunathappa <[email protected]>
> Cc: Roger Quadros <[email protected]>
> Cc: Haojian Zhuang <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: [email protected]
> Cc: Benoît Cousson <[email protected]>
> Cc: [email protected]
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> .../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

2013-10-10 14:04:19

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Thu, Oct 10, 2013 at 3:24 PM, Roger Quadros <[email protected]> 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

2013-10-10 14:35:51

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On 10/10/2013 05:04 PM, Linus Walleij wrote:
> On Thu, Oct 10, 2013 at 3:24 PM, Roger Quadros <[email protected]> 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

2013-10-10 15:32:52

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Thu, Oct 10, 2013 at 4:35 PM, Roger Quadros <[email protected]> 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 <&ampersand> 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

2013-10-10 16:00:27

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Roger Quadros <[email protected]> [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

2013-10-10 16:11:33

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <[email protected]> wrote:
> * Roger Quadros <[email protected]> [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

2013-10-10 16:15:54

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Linus Walleij <[email protected]> [131010 08:40]:
> On Thu, Oct 10, 2013 at 4:35 PM, Roger Quadros <[email protected]> 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 <&ampersand> 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

2013-10-10 16:20:24

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Linus Walleij <[email protected]> [131010 09:19]:
> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <[email protected]> wrote:
> > * Roger Quadros <[email protected]> [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

2013-10-10 16:23:20

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Tony Lindgren <[email protected]> [131010 09:09]:
> * Roger Quadros <[email protected]> [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

2013-10-11 08:00:38

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Thu, Oct 10, 2013 at 6:20 PM, Tony Lindgren <[email protected]> wrote:
> * Linus Walleij <[email protected]> [131010 09:19]:
>> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <[email protected]> wrote:
>> > * Roger Quadros <[email protected]> [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

2013-10-11 08:46:31

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On 10/10/2013 07:23 PM, Tony Lindgren wrote:
> * Tony Lindgren <[email protected]> [131010 09:09]:
>> * Roger Quadros <[email protected]> [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

2013-10-11 08:49:48

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On 10/10/2013 07:00 PM, Tony Lindgren wrote:
> * Roger Quadros <[email protected]> [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

2013-10-11 08:57:06

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On 10/11/2013 11:00 AM, Linus Walleij wrote:
> On Thu, Oct 10, 2013 at 6:20 PM, Tony Lindgren <[email protected]> wrote:
>> * Linus Walleij <[email protected]> [131010 09:19]:
>>> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <[email protected]> wrote:
>>>> * Roger Quadros <[email protected]> [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

2013-10-11 10:32:49

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <[email protected]> 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

2013-10-11 13:59:51

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On 10/11/2013 11:49 AM, Roger Quadros wrote:
> On 10/10/2013 07:00 PM, Tony Lindgren wrote:
>> * Roger Quadros <[email protected]> [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 <[email protected]>

cheers,
-roger

2013-10-11 15:18:48

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Roger Quadros <[email protected]> [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 <[email protected]>

Thanks, for testing, sorry I already pushed them out after Kevin
ran his PM tests on them.

Regards,

Tony

2013-10-11 15:37:05

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Roger Quadros <[email protected]> [131011 02:04]:
> On 10/11/2013 11:00 AM, Linus Walleij wrote:
> > On Thu, Oct 10, 2013 at 6:20 PM, Tony Lindgren <[email protected]> wrote:
> >> * Linus Walleij <[email protected]> [131010 09:19]:
> >>> On Thu, Oct 10, 2013 at 6:00 PM, Tony Lindgren <[email protected]> wrote:
> >>>> * Roger Quadros <[email protected]> [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

2013-10-11 15:43:59

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Linus Walleij <[email protected]> [131011 03:40]:
> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <[email protected]> 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

2013-10-11 15:44:08

by Balaji T K

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

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

2013-10-11 15:48:15

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Balaji T K <[email protected]> [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

2013-10-11 15:57:01

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Fri, Oct 11, 2013 at 5:43 PM, Tony Lindgren <[email protected]> wrote:
> * Linus Walleij <[email protected]> [131011 03:40]:
>> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <[email protected]> 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

2013-10-11 16:01:51

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Linus Walleij <[email protected]> [131011 09:05]:
> On Fri, Oct 11, 2013 at 5:43 PM, Tony Lindgren <[email protected]> wrote:
> > * Linus Walleij <[email protected]> [131011 03:40]:
> >> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <[email protected]> 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

2013-10-18 07:41:25

by Balaji T K

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Friday 11 October 2013 09:31 PM, Tony Lindgren wrote:
> * Linus Walleij <[email protected]> [131011 09:05]:
>> On Fri, Oct 11, 2013 at 5:43 PM, Tony Lindgren <[email protected]> wrote:
>>> * Linus Walleij <[email protected]> [131011 03:40]:
>>>> On Fri, Oct 11, 2013 at 10:56 AM, Roger Quadros <[email protected]> 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

2013-10-18 15:35:48

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

* Balaji T K <[email protected]> [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

2013-10-18 15:59:47

by Balaji T K

[permalink] [raw]

Subject: Re: [PATCH 4/6] pinctrl: single: Add support for wake-up interrupts

On Friday 18 October 2013 09:05 PM, Tony Lindgren wrote:
> * Balaji T K <[email protected]> [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
>

2013-10-18 16:06:21