by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 5/8] mfd: Provide the PRCMU with its own IRQ domain

From: Lee Jones <[email protected]>
Date: Fri, 3 Aug 2012 15:45:50 +0100
Subject: [PATCH 1/1] mfd: Provide the PRCMU with its own IRQ domain

The PRCMU has its own USB, Thermal, GPIO, Modem, HSI and RTC drivers,
amongst other things. This patch allows those subordinate devices to
use it as an interrupt controller as and when they are DT enabled.

CC: Samuel Ortiz <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/mfd/db8500-prcmu.c | 48 ++++++++++++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 7040a00..de09113 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -270,6 +270,8 @@ static struct {
struct prcmu_fw_version version;
} fw_info;

+static struct irq_domain *db8500_irq_domain;
+
/*
* This vector maps irq numbers to the bits in the bit field used in
* communication with the PRCMU firmware.
@@ -2583,7 +2585,7 @@ static void prcmu_irq_mask(struct irq_data *d)

spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags);

- mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE];
+ mb0_transfer.req.dbb_irqs &= ~prcmu_irq_bit[d->hwirq];

spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags);

@@ -2597,7 +2599,7 @@ static void prcmu_irq_unmask(struct irq_data *d)

spin_lock_irqsave(&mb0_transfer.dbb_irqs_lock, flags);

- mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->irq - IRQ_PRCMU_BASE];
+ mb0_transfer.req.dbb_irqs |= prcmu_irq_bit[d->hwirq];

spin_unlock_irqrestore(&mb0_transfer.dbb_irqs_lock, flags);

@@ -2637,9 +2639,37 @@ static char *fw_project_name(u8 project)
}
}

+static int db8500_irq_map(struct irq_domain *d, unsigned int virq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_and_handler(virq, &prcmu_irq_chip,
+ handle_simple_irq);
+ set_irq_flags(virq, IRQF_VALID);
+
+ return 0;
+}
+
+static struct irq_domain_ops db8500_irq_ops = {
+ .map = db8500_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+static int db8500_irq_init(struct device_node *np)
+{
+ db8500_irq_domain = irq_domain_add_legacy(
+ np, NUM_PRCMU_WAKEUPS, IRQ_PRCMU_BASE,
+ 0, &db8500_irq_ops, NULL);
+
+ if (!db8500_irq_domain) {
+ pr_err("Failed to create irqdomain\n");
+ return -ENOSYS;
+ }
+
+ return 0;
+}
+
void __init db8500_prcmu_early_init(void)
{
- unsigned int i;
if (cpu_is_u8500v2()) {
void *tcpm_base = ioremap_nocache(U8500_PRCMU_TCPM_BASE, SZ_4K);

@@ -2683,16 +2713,6 @@ void __init db8500_prcmu_early_init(void)
init_completion(&mb5_transfer.work);

INIT_WORK(&mb0_transfer.mask_work, prcmu_mask_work);
-
- /* Initalize irqs. */
- for (i = 0; i < NUM_PRCMU_WAKEUPS; i++) {
- unsigned int irq;
-
- irq = IRQ_PRCMU_BASE + i;
- irq_set_chip_and_handler(irq, &prcmu_irq_chip,
- handle_simple_irq);
- set_irq_flags(irq, IRQF_VALID);
- }
}

static void __init init_prcm_registers(void)
@@ -2999,6 +3019,8 @@ static int __devinit db8500_prcmu_probe(struct platform_device *pdev)
goto no_irq_return;
}

+ db8500_irq_init(np);
+
for (i = 0; i < ARRAY_SIZE(db8500_prcmu_devs); i++) {
if (!strcmp(db8500_prcmu_devs[i].name, "ab8500-core")) {
db8500_prcmu_devs[i].platform_data = ab8500_platdata;
--
1.7.9.5

2012-08-20 12:11:00

On Mon, Aug 20, 2012 at 01:55:32PM +0100, Lee Jones wrote:

> Right, that was the initial intention. It would be a trivial semantic
> change if drivers without DT support wished to use the functionality
> though. However, the only examples I found of a non-DT enabled driver
> that could make good use of it in order to strip out some cruft would
> be the Arizona and one of the Samsung drivers, and they each have

All of the regmap devices could use this.

> their own hand-rolled methods of hwirq -> virq conversion now, so any
> change to support them would result in multiple invocations of
> irq_create_mapping which would likely cause breakage.

Multiple calls to irq_create_mapping() are totally fine.

Attachments:

(No filename) (710.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-08-20 16:50:00

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 5/8] mfd: Provide the PRCMU with its own IRQ domain

On Mon, Aug 20, 2012 at 05:29:23PM +0100, Mark Brown wrote:
> On Mon, Aug 20, 2012 at 01:55:32PM +0100, Lee Jones wrote:
>
> > Right, that was the initial intention. It would be a trivial semantic
> > change if drivers without DT support wished to use the functionality
> > though. However, the only examples I found of a non-DT enabled driver
> > that could make good use of it in order to strip out some cruft would
> > be the Arizona and one of the Samsung drivers, and they each have
>
> All of the regmap devices could use this.

Only if they have linear domains and don't support DT.

If they don't have linear domains there's no point, if they support DT
then they can use it as it is.

> > their own hand-rolled methods of hwirq -> virq conversion now, so any
> > change to support them would result in multiple invocations of
> > irq_create_mapping which would likely cause breakage.
>
> Multiple calls to irq_create_mapping() are totally fine.

Sorry, perhaps I wasn't clear.

<mfd_parent_device>_probe()
|
+-> mfd_add_device()
|
+-> res->start = res->end = irq_create_mapping(res->start); // Now a VIRQ

<mfd_child_device>_probe()
|
+-> <mfd_parent_device>_irq_get_virq(res->start) // Ahhh, double VIRQ conversion

To stop being DT dependent we'd need to remove all hand-rolled stuff
that these drivers are currently using, or else they will be attempting
to convert and already converted VIRQ value.

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2012-08-20 17:52:00

On Tue, Aug 21, 2012 at 09:56:19AM +0100, Lee Jones wrote:

> Wherever we do this from to be able to obtain the IRQ domain pointer,
> which is where I'm currently struggling. Our options are:

> - If we're only talking MFD here, we can handle this stuff in the MFD
> core, but we need more information. The IRQ domain subsystem only allows
> domain look-up via a Device Tree node, so we need to get our hands on

What makes you say this? This is just a convenience for finding a
domain, irqdomains are *completely* indepentant of device tree.

> the domain another way in the case of non-DT enabled devices. Either we
> add another parameter to mfd_add_device(irq_domain, ...), or we
> standardise the 'irq_domain' variable name and use:
> irq_domain = container_of(parent, struct irq_domain, irq_domain);

Passing the domain into mfd seems like the obvious solution here - it's
exactly what we currently do for legacy stuff (where we pass in the irq
base), ideally what we'd end up with over time is that we could just
remove the irq_base parameter entirely as everything would in time be
moved over to using irqdomains.

> - I know that you have interest in pushing the functionality into the
> IRQ domain subsystem, but I'm struggling to see how. It's calling into
> the IRQ domain where we're seeing issues in the first place, specifically
> irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
> when requesting the IRQ? That way we can pass the correct IRQ without
> worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
> can do the necessary conversions. If 'irq_domain' is NULL it continues to
> use the requested IRQ as a virq.

This is totally orthogonal to doing the mapping in the MFD subsystem
which is the issue here.

Attachments:

(No filename) (1.74 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-08-21 10:54:22

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 5/8] mfd: Provide the PRCMU with its own IRQ domain

On Tue, Aug 21, 2012 at 10:50:27AM +0100, Mark Brown wrote:
> On Tue, Aug 21, 2012 at 09:56:19AM +0100, Lee Jones wrote:
>
> > Wherever we do this from to be able to obtain the IRQ domain pointer,
> > which is where I'm currently struggling. Our options are:
>
> > - If we're only talking MFD here, we can handle this stuff in the MFD
> > core, but we need more information. The IRQ domain subsystem only allows
> > domain look-up via a Device Tree node, so we need to get our hands on
>
> What makes you say this? This is just a convenience for finding a
> domain, irqdomains are *completely* indepentant of device tree.

How can you say that? I think you mean _can_ be independent of DT. If
that's what you mean then yes, that's true. All I'm saying is we need
another way to get hold of the domain, because the only way to obtain
it without having direct access is via a device node.

> > the domain another way in the case of non-DT enabled devices. Either we
> > add another parameter to mfd_add_device(irq_domain, ...), or we
> > standardise the 'irq_domain' variable name and use:
> > irq_domain = container_of(parent, struct irq_domain, irq_domain);
>
> Passing the domain into mfd seems like the obvious solution here - it's
> exactly what we currently do for legacy stuff (where we pass in the irq
> base), ideally what we'd end up with over time is that we could just
> remove the irq_base parameter entirely as everything would in time be
> moved over to using irqdomains.

Right. That's fine (and easy) then. You threw me when you said you wanted
it handled higher-up in the framework, as I didn't see how we could do
this in the irqdomain itself.

> > - I know that you have interest in pushing the functionality into the
> > IRQ domain subsystem, but I'm struggling to see how. It's calling into
> > the IRQ domain where we're seeing issues in the first place, specifically
> > irq_create_mapping(). How about if we passed 'irq_domain' as a parameter
> > when requesting the IRQ? That way we can pass the correct IRQ without
> > worry of conversion. If 'irq_domain' is !NULL the IRQ management subsystem
> > can do the necessary conversions. If 'irq_domain' is NULL it continues to
> > use the requested IRQ as a virq.
>
> This is totally orthogonal to doing the mapping in the MFD subsystem
> which is the issue here.

Again, I only mentioned this because you said you wanted it to be handled
by the irqdomain.

I'll code up the second suggestion now.

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2012-08-21 11:03:33

On Tue, Aug 21, 2012 at 01:02:46PM +0100, Lee Jones wrote:
> On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote:

> > > another way to get hold of the domain, because the only way to obtain
> > > it without having direct access is via a device node.

> > This doesn't actually hold.

> Okay, besides irq_find_host(struct device_node *np), how else can you fetch
> a domain from the irqdomain?

I'm not sure I can parse the above, sorry - I'm not sure I can
distinguish "domain" and "irqdomain".

> What have you done already?

Implemented a patch for this which I've now tested a bit and will
probably post in the next hour or so.

> Why make suggestions if you're just going to do the work yourself?

I made the suggestion then later on realised that this was actively
going to break things I care about so I actually need it fixing.

Attachments:

(No filename) (844.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-08-22 08:18:01

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 5/8] mfd: Provide the PRCMU with its own IRQ domain

On Tue, Aug 21, 2012 at 05:52:07PM +0100, Mark Brown wrote:
> On Tue, Aug 21, 2012 at 01:02:46PM +0100, Lee Jones wrote:
> > On Tue, Aug 21, 2012 at 12:03:29PM +0100, Mark Brown wrote:
>
> > > > another way to get hold of the domain, because the only way to obtain
> > > > it without having direct access is via a device node.
>
> > > This doesn't actually hold.
>
> > Okay, besides irq_find_host(struct device_node *np), how else can you fetch
> > a domain from the irqdomain?
>
> I'm not sure I can parse the above, sorry - I'm not sure I can
> distinguish "domain" and "irqdomain".

Are you being intentionally awkward on this? Picking holes when you know
exactly what I'm trying to say? Not that it matters now, but just out of
principle, let me try to put it in a very clear way so there can be no
misinterpretation.

I was saying that in order for the MFD core to carry out the hwirq->virq
conversion, it needed to obtain the irqdomain pointer pertaining to the
provided hwirq. The only helper function the irqdomain subsystem provides
requires a device node pointer to be passed as an argument, hence the
mention of 'irq_find_host(struct device_node *np)'. Then the Device Tree
is traversed until a specified 'interrupt-controller' is stumbled upon
or is pointed to by the 'interrupt-parent' property. Hence, we have to
find another way to find the irqdomain pointers for non-DT based MFDs. To
which we now have a solution.

> > What have you done already?
>
> Implemented a patch for this which I've now tested a bit and will
> probably post in the next hour or so.
>
> > Why make suggestions if you're just going to do the work yourself?
>
> I made the suggestion then later on realised that this was actively
> going to break things I care about so I actually need it fixing.

I'm a little taken aback and annoyed by this. In a previous email thread
you categorically requested that I discuss some of the important changes
with maintainers and people in-the-know prior to actually writing any
code. I was obviously actively working on, had put time into, and was in
the mist of discussing this with you. Then you just go ahead and code it
(the easy part) yourself, essentially wasting my time. Surely there's
some kind of etiquette surrounding such things?

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2012-08-22 11:19:19

[permalink] [raw]

Subject: Re: [PATCH 2/8] irqdomain: Take interrupt-parent property into account if specified

On 08/31/2012 04:44 AM, Lee Jones wrote:
> On Tue, Aug 14, 2012 at 10:19:08AM +0200, Linus Walleij wrote:
>> On Thu, Aug 9, 2012 at 5:53 PM, Lee Jones <[email protected]> wrote:
>>
>>> irq_find_host() currently ignores the 'interrupt-parent' property
>>> even if it's specified in the Device Tree. Meaning that a node can
>>> match to a domain in its hierarchy even if it doesn't belong to it.
>>> By searching for the parent first using of_irq_find_parent() we
>>> insist that the 'interrupt-parent' property is taken into account
>>> ensuring a greater chance of returning the correct domain.
>>>
>>> CC: Benjamin Herrenschmidt <[email protected]>
>>> CC: Grant Likely <[email protected]>
>>> Signed-off-by: Lee Jones <[email protected]>
>>
>> This (with 1/8) is the right solution. Thanks, and I think
>> Mark may want to look at this too since I recognize he asked
>> you to work in this direction.
>>
>> Acked-by: Linus Walleij <[email protected]>
>
> Rob, did this get your Ack too?

Acked-by: Rob Herring <[email protected]>

Rob

>
> Ben, Grant, Mark, would you like to take a look also?
>

2012-08-31 14:50:44

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH 8/8] input: ab8500-ponkey: Rely on MFD core to convert IRQs to virtual

On Fri, Aug 31, 2012 at 08:31:33AM +0100, Lee Jones wrote:
> On Thu, Aug 30, 2012 at 04:03:23PM -0700, Dmitry Torokhov wrote:
> > On Thu, Aug 30, 2012 at 04:02:21PM -0700, Dmitry Torokhov wrote:
> > > On Thu, Aug 30, 2012 at 02:12:04PM +0100, Lee Jones wrote:
> > > > > Sorry for the delay. Yes, this shoudl be fine, but since it is
> > > > > essentially a revert of the original patch it should be pushed in as
> > > > > such.
> > > >
> > > > How's this?
> > > >
> > >
> > > Excellent.
> >
> > I assume you will be merging it with the rest of AB8500 patches, right?
>
> Yes, if that's okay with you of course?

Sure, please go ahead,

Thanks.

--
Dmitry