Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On Wed, 28 Mar 2012 18:41:03 -0700, David Daney wrote:
> On 03/28/2012 03:22 PM, Grant Likely wrote:
> > On Tue, 27 Mar 2012 11:24:51 -0700, David Daney
> > wrote:
> >> On 03/26/2012 06:56 PM, Rob Herring wrote:
> >>> On 03/26/2012 02:31 PM, David Daney wrote:
> From: David Daney
> >> [...]
> +static int octeon_irq_ciu_map(struct irq_domain *d,
> + unsigned int virq, irq_hw_number_t hw)
> +{
> +unsigned int line = hw>> 6;
> +unsigned int bit = hw& 63;
> +
> +if (virq>= 256)
> +return -EINVAL;
> >>>
> >>> Drop this. You should not care what the virq numbers are.
> >>
> >>
> >> I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).
> >>
> >> So really I want to say:
> >>
> >> if (virq>= (1<< sizeof (octeon_irq_ciu_to_irq[0][0]))) {
> >> WARN(...);
> >> return -EINVAL;
> >> }
> >>
> >>
> >> I need a map external to any one irq_domain. The irq handling code
> >> handles sources that come from two separate irq_domains, as well as irqs
> >> that are not part of any domain.
> >
> > You can get past this limitation by using the struct irq_data .hwirq and
> > .domain members for the irq ==> hwirq translation, and for hwirq ==>
> > irq the code should already have the context to know which user it is.
> >
> > For the irqs that are not covered by an irq_domain, the driver is free
> > to set the .hwirq value directly. Ultimately however, it will
> > probably be best to add an irq domain for those users also.
> >
> > ...
> >
> > Howver, I don't understand where the risk is in overflowing
> > octeon_irq_ciu_to_irq[][]. From what I can see, the virq value isn't
> > used at all to calculate the array dereference. line and bit are
> > calculated from the hwirq value. What am I missing?
> >
>
> We do the opposite. We extract the hwirq value from the interrupt
> controller and then look up virq in the table. If the range of virq
> overflows the width of u8, we would end up calling do_IRQ() with a bad
> value. Also this dispatch code is not aware of the various irq_domains
> and non irq_domain irqs, it is a single function that handles them all
> calling do_IRQ() with whatever it looks up in the table.
>
> We could use a wider type for this lookup array, but that would increase
> the cache footprint of the irq dispatcher...
Ah, I missed that octeon_irq_ciu_to_irq was a u8. You're using Linux
though; your cache footprint is already trashed. :-) Please just use
unsigned int for all irq storage since that is the type used by all
core interrupt handling code. Anything else smells like premature
optimization. :-)
Besides, now that we have it you should plan to switch to the common
mechanism of irq_domain for hwirq->irq reverse mapping anyway. It
doesn't make any sense for each platform to reinvent it's own reverse
mapping scheme.
g.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/28/2012 03:08 PM, Grant Likely wrote:
On Wed, 28 Mar 2012 09:16:05 -0700, David Daney wrote:
On 03/28/2012 07:21 AM, Rob Herring wrote:
On 03/27/2012 05:31 PM, David Daney wrote:
On 03/27/2012 03:05 PM, Rob Herring wrote:
On 03/27/2012 01:24 PM, David Daney wrote:
On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
From: David Daney
[...]
+static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int
bit)
+{
+bool edge = false;
+
+if (line == 0)
+switch (bit) {
+case 48 ... 49: /* GMX DRP */
+case 50: /* IPD_DRP */
+case 52 ... 55: /* Timers */
+case 58: /* MPI */
+edge = true;
+break;
+default:
+break;
+}
+else /* line == 1 */
+switch (bit) {
+case 47: /* PTP */
+edge = true;
+break;
+default:
+break;
+}
+return edge;
Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?
There are a several reasons, in no particular order they are:
o There is no 3rd cell. The bindings were discussed with Grant here:
http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
Then add one.
I can't. The dtb is already programmed into the bootloader ROMs,
changing the kernel code will not change that. It is fait accompli.
o The edge/level thing cannot be changed, and the irq lines don't leave
the SOC, so hard coding it is possible.
Right, but DT describes h/w connections and this is an aspect of the
connection. This may be fixed for the SOC, but it's not fixed for the
CIU (i.e. could change in future chips), right?
In theory yes. However:
1) The chip designers will not change it.
2) There will likely be no more designs with either CIU or CIU2, so we
know what all the different possibilities are today.
When CIU3 is deployed, we will use the lessons we have learned to do
things the Right Way.
There's 2 reasons why you would not put this into DTS:
- All irq lines' trigger type are the same, fixed and known.
- You can read a register to tell you the trigger type.
Even if it's not going to change ever, it's still worth putting into the
DTS as it is well suited for holding that data and it is just data.
Agreed that it could be in the DTS, and retrospect it probably should
have been put in the DTS, but it wasn't. So I think what we have now is
a workable solution, and has the added attraction of working with
already deployed boards.
Aren't you building in the dtb to the kernel?
No. This seems like a bit of a disconnect in this discussion. From the
cover-letter:
o A device tree template is statically linked into the kernel image.
Based on SOC type and board type, legacy configuration probing code
is used to prune and patch the device tree template.
o New SOCs and boards will directly use a device tree passed by the
bootloader.
I think the real surprise here is that Octeron is depending on a
masked-in copy of the device tree. It has alwasy been a strong
recommendation to store the .dtb along side the kernel in reflashable
storage for exactly the reason that updates may be required. Review
is vitally important, but nobody can foresee all the implications
until the bindings are actually used.
I'm not going to harp over this, I understand that you're hamstrung
here, but please do pass on to the firmware engineers that storing the
.dtb in masked rom is risky.
Well in most cases it is not masked ROM, but rather some sort of
persistent but modifiable storage. But still getting people (both
internal and external) to update it is not as easy as living with the
status quo. The idea is that someone should be able to get a kernel.orq
kernel and use it, without having to re-flash their board's bootloader.
David Daney
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/28/2012 03:22 PM, Grant Likely wrote:
On Tue, 27 Mar 2012 11:24:51 -0700, David Daney wrote:
On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
From: David Daney
[...]
+static int octeon_irq_ciu_map(struct irq_domain *d,
+ unsigned int virq, irq_hw_number_t hw)
+{
+ unsigned int line = hw>> 6;
+ unsigned int bit = hw& 63;
+
+ if (virq>= 256)
+ return -EINVAL;
Drop this. You should not care what the virq numbers are.
I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).
So really I want to say:
if (virq>= (1<< sizeof (octeon_irq_ciu_to_irq[0][0]))) {
WARN(...);
return -EINVAL;
}
I need a map external to any one irq_domain. The irq handling code
handles sources that come from two separate irq_domains, as well as irqs
that are not part of any domain.
You can get past this limitation by using the struct irq_data .hwirq and
.domain members for the irq ==> hwirq translation, and for hwirq ==>
irq the code should already have the context to know which user it is.
For the irqs that are not covered by an irq_domain, the driver is free
to set the .hwirq value directly. Ultimately however, it will
probably be best to add an irq domain for those users also.
...
Howver, I don't understand where the risk is in overflowing
octeon_irq_ciu_to_irq[][]. From what I can see, the virq value isn't
used at all to calculate the array dereference. line and bit are
calculated from the hwirq value. What am I missing?
We do the opposite. We extract the hwirq value from the interrupt
controller and then look up virq in the table. If the range of virq
overflows the width of u8, we would end up calling do_IRQ() with a bad
value. Also this dispatch code is not aware of the various irq_domains
and non irq_domain irqs, it is a single function that handles them all
calling do_IRQ() with whatever it looks up in the table.
We could use a wider type for this lookup array, but that would increase
the cache footprint of the irq dispatcher...
David Daney
David Daney
g.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/28/2012 03:31 PM, Grant Likely wrote:
On Mon, 26 Mar 2012 12:31:19 -0700, David Daney wrote:
From: David Daney
Create two domains. One for the GPIO lines, and the other for on-chip
sources.
Signed-off-by: David Daney
---
[...]
+struct octeon_irq_gpio_domain_data {
+ unsigned int base_hwirq;
+};
Hmmm...
+static int octeon_irq_gpio_xlat(struct irq_domain *d,
+ struct device_node *node,
+ const u32 *intspec,
+ unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
[...]
+ *out_hwirq = gpiod->base_hwirq + pin;
...base_hwirq is only used here...
[...]
+ gpiod = kzalloc(sizeof (*gpiod), GFP_KERNEL);
+ if (gpiod) {
+ /* gpio domain host_data is the base hwirq number. */
+ gpiod->base_hwirq = 16;
+ irq_domain_add_linear(gpio_node,
16,&octeon_irq_domain_gpio_ops, gpiod);
... and it is unconditionally set to 16. It looks to me like
base_hwirq and the associated kzalloc() is unnecessary.
There is a little information asymmetry here. You don't know that I
have a patch queued up to add another user of the GPIO irq_domain that
has a different base_hwirq.
I could re-do this to hard code it, and then add it back. But it would
really just be busy work.
David Daney
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On Mon, 26 Mar 2012 12:31:19 -0700, David Daney wrote:
> From: David Daney
>
> Create two domains. One for the GPIO lines, and the other for on-chip
> sources.
>
> Signed-off-by: David Daney
> ---
[...]
> +struct octeon_irq_gpio_domain_data {
> + unsigned int base_hwirq;
> +};
Hmmm...
> +static int octeon_irq_gpio_xlat(struct irq_domain *d,
> + struct device_node *node,
> + const u32 *intspec,
> + unsigned int intsize,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
[...]
> + *out_hwirq = gpiod->base_hwirq + pin;
...base_hwirq is only used here...
[...]
> + gpiod = kzalloc(sizeof (*gpiod), GFP_KERNEL);
> + if (gpiod) {
> + /* gpio domain host_data is the base hwirq number. */
> + gpiod->base_hwirq = 16;
> + irq_domain_add_linear(gpio_node, 16,
> &octeon_irq_domain_gpio_ops, gpiod);
... and it is unconditionally set to 16. It looks to me like
base_hwirq and the associated kzalloc() is unnecessary.
g.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On Tue, 27 Mar 2012 11:24:51 -0700, David Daney wrote:
> On 03/26/2012 06:56 PM, Rob Herring wrote:
> > On 03/26/2012 02:31 PM, David Daney wrote:
> >> From: David Daney
> [...]
> >> +static int octeon_irq_ciu_map(struct irq_domain *d,
> >> +unsigned int virq, irq_hw_number_t hw)
> >> +{
> >> + unsigned int line = hw>> 6;
> >> + unsigned int bit = hw& 63;
> >> +
> >> + if (virq>= 256)
> >> + return -EINVAL;
> >
> > Drop this. You should not care what the virq numbers are.
>
>
> I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).
>
> So really I want to say:
>
> if (virq >= (1 << sizeof (octeon_irq_ciu_to_irq[0][0]))) {
> WARN(...);
> return -EINVAL;
> }
>
>
> I need a map external to any one irq_domain. The irq handling code
> handles sources that come from two separate irq_domains, as well as irqs
> that are not part of any domain.
You can get past this limitation by using the struct irq_data .hwirq and
.domain members for the irq ==> hwirq translation, and for hwirq ==>
irq the code should already have the context to know which user it is.
For the irqs that are not covered by an irq_domain, the driver is free
to set the .hwirq value directly. Ultimately however, it will
probably be best to add an irq domain for those users also.
...
Howver, I don't understand where the risk is in overflowing
octeon_irq_ciu_to_irq[][]. From what I can see, the virq value isn't
used at all to calculate the array dereference. line and bit are
calculated from the hwirq value. What am I missing?
g.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On Wed, 28 Mar 2012 09:16:05 -0700, David Daney wrote:
> On 03/28/2012 07:21 AM, Rob Herring wrote:
> > On 03/27/2012 05:31 PM, David Daney wrote:
> >> On 03/27/2012 03:05 PM, Rob Herring wrote:
> >>> On 03/27/2012 01:24 PM, David Daney wrote:
> On 03/26/2012 06:56 PM, Rob Herring wrote:
> > On 03/26/2012 02:31 PM, David Daney wrote:
> >> From: David Daney
> [...]
> >> +static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int
> >> bit)
> >> +{
> >> +bool edge = false;
> >> +
> >> +if (line == 0)
> >> +switch (bit) {
> >> +case 48 ... 49: /* GMX DRP */
> >> +case 50: /* IPD_DRP */
> >> +case 52 ... 55: /* Timers */
> >> +case 58: /* MPI */
> >> +edge = true;
> >> +break;
> >> +default:
> >> +break;
> >> +}
> >> +else /* line == 1 */
> >> +switch (bit) {
> >> +case 47: /* PTP */
> >> +edge = true;
> >> +break;
> >> +default:
> >> +break;
> >> +}
> >> +return edge;
> >
> > Moving in the right direction, but I still don't get why this is not in
> > the CIU binding as a 3rd cell?
>
> There are a several reasons, in no particular order they are:
>
> o There is no 3rd cell. The bindings were discussed with Grant here:
> http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
>
> >>>
> >>> Then add one.
> >>
> >> I can't. The dtb is already programmed into the bootloader ROMs,
> >> changing the kernel code will not change that. It is fait accompli.
> >>
> >>>
> o The edge/level thing cannot be changed, and the irq lines don't leave
> the SOC, so hard coding it is possible.
> >>>
> >>> Right, but DT describes h/w connections and this is an aspect of the
> >>> connection. This may be fixed for the SOC, but it's not fixed for the
> >>> CIU (i.e. could change in future chips), right?
> >>
> >> In theory yes. However:
> >>
> >> 1) The chip designers will not change it.
> >>
> >> 2) There will likely be no more designs with either CIU or CIU2, so we
> >> know what all the different possibilities are today.
> >>
> >> When CIU3 is deployed, we will use the lessons we have learned to do
> >> things the Right Way.
> >>
> >>>
> >>> There's 2 reasons why you would not put this into DTS:
> >>>
> >>> - All irq lines' trigger type are the same, fixed and known.
> >>> - You can read a register to tell you the trigger type.
> >>>
> >>> Even if it's not going to change ever, it's still worth putting into the
> >>> DTS as it is well suited for holding that data and it is just data.
> >>
> >> Agreed that it could be in the DTS, and retrospect it probably should
> >> have been put in the DTS, but it wasn't. So I think what we have now is
> >> a workable solution, and has the added attraction of working with
> >> already deployed boards.
> >
> > Aren't you building in the dtb to the kernel?
>
> No. This seems like a bit of a disconnect in this discussion. From the
> cover-letter:
>
> o A device tree template is statically linked into the kernel image.
>Based on SOC type and board type, legacy configuration probing code
>is used to prune and patch the device tree template.
>
> o New SOCs and boards will directly use a device tree passed by the
>bootloader.
I think the real surprise here is that Octeron is depending on a
masked-in copy of the device tree. It has alwasy been a strong
recommendation to store the .dtb along side the kernel in reflashable
storage for exactly the reason that updates may be required. Review
is vitally important, but nobody can foresee all the implications
until the bindings are actually used.
I'm not going to harp over this, I understand that you're hamstrung
here, but please do pass on to the firmware engineers that storing the
.dtb in masked rom is risky.
>
>
> >
> > It could be argued it doesn't matter what you deployed without upstream
> > review.
>
> A moot point, as all the bindings were reviewed with Grant Likely
> precisely for the reason of minimizing surprises when merging upstream.
> In the intervening months, we have found a few rough edges like this,
> but we cannot say that there was no upstream review.
Yes, you've been good about this and I much appreciate it.
g.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/28/2012 07:21 AM, Rob Herring wrote:
On 03/27/2012 05:31 PM, David Daney wrote:
On 03/27/2012 03:05 PM, Rob Herring wrote:
On 03/27/2012 01:24 PM, David Daney wrote:
On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
From: David Daney
[...]
+static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int
bit)
+{
+bool edge = false;
+
+if (line == 0)
+switch (bit) {
+case 48 ... 49: /* GMX DRP */
+case 50: /* IPD_DRP */
+case 52 ... 55: /* Timers */
+case 58: /* MPI */
+edge = true;
+break;
+default:
+break;
+}
+else /* line == 1 */
+switch (bit) {
+case 47: /* PTP */
+edge = true;
+break;
+default:
+break;
+}
+return edge;
Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?
There are a several reasons, in no particular order they are:
o There is no 3rd cell. The bindings were discussed with Grant here:
http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
Then add one.
I can't. The dtb is already programmed into the bootloader ROMs,
changing the kernel code will not change that. It is fait accompli.
o The edge/level thing cannot be changed, and the irq lines don't leave
the SOC, so hard coding it is possible.
Right, but DT describes h/w connections and this is an aspect of the
connection. This may be fixed for the SOC, but it's not fixed for the
CIU (i.e. could change in future chips), right?
In theory yes. However:
1) The chip designers will not change it.
2) There will likely be no more designs with either CIU or CIU2, so we
know what all the different possibilities are today.
When CIU3 is deployed, we will use the lessons we have learned to do
things the Right Way.
There's 2 reasons why you would not put this into DTS:
- All irq lines' trigger type are the same, fixed and known.
- You can read a register to tell you the trigger type.
Even if it's not going to change ever, it's still worth putting into the
DTS as it is well suited for holding that data and it is just data.
Agreed that it could be in the DTS, and retrospect it probably should
have been put in the DTS, but it wasn't. So I think what we have now is
a workable solution, and has the added attraction of working with
already deployed boards.
Aren't you building in the dtb to the kernel?
No. This seems like a bit of a disconnect in this discussion. From the
cover-letter:
o A device tree template is statically linked into the kernel image.
Based on SOC type and board type, legacy configuration probing code
is used to prune and patch the device tree template.
o New SOCs and boards will directly use a device tree passed by the
bootloader.
It could be argued it doesn't matter what you deployed without upstream
review.
A moot point, as all the bindings were reviewed with Grant Likely
precisely for the reason of minimizing surprises when merging upstream.
In the intervening months, we have found a few rough edges like this,
but we cannot say that there was no upstream review.
But as long as this contained then this one is fine with me.
However, this needs to be understood for each binding as I'm sure there
must be some blocks which will get reused.
Yes, there will be reuse. There has in fact been reuse since the
bindings were created, and for the most part things are going smoothly.
Thanks,
David Daney
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/27/2012 05:31 PM, David Daney wrote:
> On 03/27/2012 03:05 PM, Rob Herring wrote:
>> On 03/27/2012 01:24 PM, David Daney wrote:
>>> On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
> From: David Daney
>>> [...]
> +static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int
> bit)
> +{
> +bool edge = false;
> +
> +if (line == 0)
> +switch (bit) {
> +case 48 ... 49: /* GMX DRP */
> +case 50: /* IPD_DRP */
> +case 52 ... 55: /* Timers */
> +case 58: /* MPI */
> +edge = true;
> +break;
> +default:
> +break;
> +}
> +else /* line == 1 */
> +switch (bit) {
> +case 47: /* PTP */
> +edge = true;
> +break;
> +default:
> +break;
> +}
> +return edge;
Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?
>>>
>>> There are a several reasons, in no particular order they are:
>>>
>>> o There is no 3rd cell. The bindings were discussed with Grant here:
>>>http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
>>>
>>
>> Then add one.
>
> I can't. The dtb is already programmed into the bootloader ROMs,
> changing the kernel code will not change that. It is fait accompli.
>
>>
>>> o The edge/level thing cannot be changed, and the irq lines don't leave
>>> the SOC, so hard coding it is possible.
>>
>> Right, but DT describes h/w connections and this is an aspect of the
>> connection. This may be fixed for the SOC, but it's not fixed for the
>> CIU (i.e. could change in future chips), right?
>
> In theory yes. However:
>
> 1) The chip designers will not change it.
>
> 2) There will likely be no more designs with either CIU or CIU2, so we
> know what all the different possibilities are today.
>
> When CIU3 is deployed, we will use the lessons we have learned to do
> things the Right Way.
>
>>
>> There's 2 reasons why you would not put this into DTS:
>>
>> - All irq lines' trigger type are the same, fixed and known.
>> - You can read a register to tell you the trigger type.
>>
>> Even if it's not going to change ever, it's still worth putting into the
>> DTS as it is well suited for holding that data and it is just data.
>
> Agreed that it could be in the DTS, and retrospect it probably should
> have been put in the DTS, but it wasn't. So I think what we have now is
> a workable solution, and has the added attraction of working with
> already deployed boards.
Aren't you building in the dtb to the kernel?
It could be argued it doesn't matter what you deployed without upstream
review. But as long as this contained then this one is fine with me.
However, this needs to be understood for each binding as I'm sure there
must be some blocks which will get reused.
Rob
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/27/2012 03:05 PM, Rob Herring wrote:
On 03/27/2012 01:24 PM, David Daney wrote:
On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
From: David Daney
[...]
+static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit)
+{
+bool edge = false;
+
+if (line == 0)
+switch (bit) {
+case 48 ... 49: /* GMX DRP */
+case 50: /* IPD_DRP */
+case 52 ... 55: /* Timers */
+case 58: /* MPI */
+edge = true;
+break;
+default:
+break;
+}
+else /* line == 1 */
+switch (bit) {
+case 47: /* PTP */
+edge = true;
+break;
+default:
+break;
+}
+return edge;
Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?
There are a several reasons, in no particular order they are:
o There is no 3rd cell. The bindings were discussed with Grant here:
http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
Then add one.
I can't. The dtb is already programmed into the bootloader ROMs,
changing the kernel code will not change that. It is fait accompli.
o The edge/level thing cannot be changed, and the irq lines don't leave
the SOC, so hard coding it is possible.
Right, but DT describes h/w connections and this is an aspect of the
connection. This may be fixed for the SOC, but it's not fixed for the
CIU (i.e. could change in future chips), right?
In theory yes. However:
1) The chip designers will not change it.
2) There will likely be no more designs with either CIU or CIU2, so we
know what all the different possibilities are today.
When CIU3 is deployed, we will use the lessons we have learned to do
things the Right Way.
There's 2 reasons why you would not put this into DTS:
- All irq lines' trigger type are the same, fixed and known.
- You can read a register to tell you the trigger type.
Even if it's not going to change ever, it's still worth putting into the
DTS as it is well suited for holding that data and it is just data.
Agreed that it could be in the DTS, and retrospect it probably should
have been put in the DTS, but it wasn't. So I think what we have now is
a workable solution, and has the added attraction of working with
already deployed boards.
David Daney
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/27/2012 01:24 PM, David Daney wrote:
> On 03/26/2012 06:56 PM, Rob Herring wrote:
>> On 03/26/2012 02:31 PM, David Daney wrote:
>>> From: David Daney
> [...]
>>> +static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit)
>>> +{
>>> +bool edge = false;
>>> +
>>> +if (line == 0)
>>> +switch (bit) {
>>> +case 48 ... 49: /* GMX DRP */
>>> +case 50: /* IPD_DRP */
>>> +case 52 ... 55: /* Timers */
>>> +case 58: /* MPI */
>>> +edge = true;
>>> +break;
>>> +default:
>>> +break;
>>> +}
>>> +else /* line == 1 */
>>> +switch (bit) {
>>> +case 47: /* PTP */
>>> +edge = true;
>>> +break;
>>> +default:
>>> +break;
>>> +}
>>> +return edge;
>>
>> Moving in the right direction, but I still don't get why this is not in
>> the CIU binding as a 3rd cell?
>
> There are a several reasons, in no particular order they are:
>
> o There is no 3rd cell. The bindings were discussed with Grant here:
> http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
>
Then add one.
> o The edge/level thing cannot be changed, and the irq lines don't leave
> the SOC, so hard coding it is possible.
Right, but DT describes h/w connections and this is an aspect of the
connection. This may be fixed for the SOC, but it's not fixed for the
CIU (i.e. could change in future chips), right?
There's 2 reasons why you would not put this into DTS:
- All irq lines' trigger type are the same, fixed and known.
- You can read a register to tell you the trigger type.
Even if it's not going to change ever, it's still worth putting into the
DTS as it is well suited for holding that data and it is just data.
>
>>
>>> +}
>>> +
>>> +struct octeon_irq_gpio_domain_data {
>>> +unsigned int base_hwirq;
>>> +};
>>> +
>>> +static int octeon_irq_gpio_xlat(struct irq_domain *d,
>>> +struct device_node *node,
>>> +const u32 *intspec,
>>> +unsigned int intsize,
>>> +unsigned long *out_hwirq,
>>> +unsigned int *out_type)
>>> +{
>>> +unsigned int type;
>>> +unsigned int pin;
>>> +unsigned int trigger;
>>> +bool set_edge_handler = false;
>>> +struct octeon_irq_gpio_domain_data *gpiod;
>>> +
>>> +if (d->of_node != node)
>>> +return -EINVAL;
>>> +
>>> +if (intsize< 2)
>>> +return -EINVAL;
>>> +
>>> +pin = intspec[0];
>>> +if (pin>= 16)
>>> +return -EINVAL;
>>> +
>>> +trigger = intspec[1];
>>> +
>>> +switch (trigger) {
>>> +case 1:
>>> +type = IRQ_TYPE_EDGE_RISING;
>>> +set_edge_handler = true;
>>
>> This is never used.
>
> Right, it was leftover from the previous version.
>
>>
>>> +break;
>>> +case 2:
>>> +type = IRQ_TYPE_EDGE_FALLING;
>>> +set_edge_handler = true;
>>> +break;
>>> +case 4:
>>> +type = IRQ_TYPE_LEVEL_HIGH;
>>> +break;
>>> +case 8:
>>> +type = IRQ_TYPE_LEVEL_LOW;
>>> +break;
>>> +default:
>>> +pr_err("Error: (%s) Invalid irq trigger specification: %x\n",
>>> + node->name,
>>> + trigger);
>>> +type = IRQ_TYPE_LEVEL_LOW;
>>> +break;
>>> +}
>>> +*out_type = type;
>>
>> Can't you get rid of the whole switch statement and just do:
>>
>> *out_type = intspec[1];
>
> That wouldn't catch erroneous values like 6.
>
if (!x || fls(x) != ffs(x))
// ERROR
>>
> [...]
>>> +static int octeon_irq_ciu_map(struct irq_domain *d,
>>> + unsigned int virq, irq_hw_number_t hw)
>>> +{
>>> +unsigned int line = hw>> 6;
>>> +unsigned int bit = hw& 63;
>>> +
>>> +if (virq>= 256)
>>> +return -EINVAL;
>>
>> Drop this. You should not care what the virq numbers are.
>
>
> I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).
>
> So really I want to say:
>
>if (virq >= (1 << sizeof (octeon_irq_ciu_to_irq[0][0]))) {
>WARN(...);
>return -EINVAL;
>}
>
>
> I need a map external to any one irq_domain. The irq handling code
> handles sources that come from two separate irq_domains, as well as irqs
> that are not part of any domain.
>
Okay, but ultimately this needs to go. The irqdomain provides no
guarantee of irq numbers assigned.
Rob
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
From: David Daney
[...]
+static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit)
+{
+ bool edge = false;
+
+ if (line == 0)
+ switch (bit) {
+ case 48 ... 49: /* GMX DRP */
+ case 50: /* IPD_DRP */
+ case 52 ... 55: /* Timers */
+ case 58: /* MPI */
+ edge = true;
+ break;
+ default:
+ break;
+ }
+ else /* line == 1 */
+ switch (bit) {
+ case 47: /* PTP */
+ edge = true;
+ break;
+ default:
+ break;
+ }
+ return edge;
Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?
There are a several reasons, in no particular order they are:
o There is no 3rd cell. The bindings were discussed with Grant here:
http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
o The edge/level thing cannot be changed, and the irq lines don't leave
the SOC, so hard coding it is possible.
+}
+
+struct octeon_irq_gpio_domain_data {
+ unsigned int base_hwirq;
+};
+
+static int octeon_irq_gpio_xlat(struct irq_domain *d,
+ struct device_node *node,
+ const u32 *intspec,
+ unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ unsigned int type;
+ unsigned int pin;
+ unsigned int trigger;
+ bool set_edge_handler = false;
+ struct octeon_irq_gpio_domain_data *gpiod;
+
+ if (d->of_node != node)
+ return -EINVAL;
+
+ if (intsize< 2)
+ return -EINVAL;
+
+ pin = intspec[0];
+ if (pin>= 16)
+ return -EINVAL;
+
+ trigger = intspec[1];
+
+ switch (trigger) {
+ case 1:
+ type = IRQ_TYPE_EDGE_RISING;
+ set_edge_handler = true;
This is never used.
Right, it was leftover from the previous version.
+ break;
+ case 2:
+ type = IRQ_TYPE_EDGE_FALLING;
+ set_edge_handler = true;
+ break;
+ case 4:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ case 8:
+ type = IRQ_TYPE_LEVEL_LOW;
+ break;
+ default:
+ pr_err("Error: (%s) Invalid irq trigger specification: %x\n",
+ node->name,
+ trigger);
+ type = IRQ_TYPE_LEVEL_LOW;
+ break;
+ }
+ *out_type = type;
Can't you get rid of the whole switch statement and just do:
*out_type = intspec[1];
That wouldn't catch erroneous values like 6.
[...]
+static int octeon_irq_ciu_map(struct irq_domain *d,
+ unsigned int virq, irq_hw_number_t hw)
+{
+ unsigned int line = hw>> 6;
+ unsigned int bit = hw& 63;
+
+ if (virq>= 256)
+ return -EINVAL;
Drop this. You should not care what the virq numbers are.
I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).
So really I want to say:
if (virq >= (1 << sizeof (octeon_irq_ciu_to_irq[0][0]))) {
WARN(...);
return -EINVAL;
}
I need a map external to any one irq_domain. The irq handling code
handles sources that come from two separate irq_domains, as well as irqs
that are not part of any domain.
Thanks for looking at this again, I will re-spin the patch,
David Daney
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
On 03/26/2012 02:31 PM, David Daney wrote:
> From: David Daney
>
> Create two domains. One for the GPIO lines, and the other for on-chip
> sources.
>
> Signed-off-by: David Daney
> ---
> arch/mips/cavium-octeon/octeon-irq.c | 208
> --
> 1 files changed, 199 insertions(+), 9 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-irq.c
> b/arch/mips/cavium-octeon/octeon-irq.c
> index 89b6f27..550c03d 100644
> --- a/arch/mips/cavium-octeon/octeon-irq.c
> +++ b/arch/mips/cavium-octeon/octeon-irq.c
> @@ -3,14 +3,17 @@
> * License. See the file "COPYING" in the main directory of this archive
> * for more details.
> *
> - * Copyright (C) 2004-2008, 2009, 2010, 2011 Cavium Networks
> + * Copyright (C) 2004-2012 Cavium, Inc.
> */
>
> #include
> +#include
> #include
> #include
> +#include
> #include
> #include
> +#include
>
> #include
>
> @@ -42,9 +45,9 @@ struct octeon_core_chip_data {
>
> static struct octeon_core_chip_data
> octeon_irq_core_chip_data[MIPS_CORE_IRQ_LINES];
>
> -static void __init octeon_irq_set_ciu_mapping(int irq, int line, int bit,
> - struct irq_chip *chip,
> - irq_flow_handler_t handler)
> +static void octeon_irq_set_ciu_mapping(int irq, int line, int bit,
> +struct irq_chip *chip,
> +irq_flow_handler_t handler)
> {
> union octeon_ciu_chip_data cd;
>
> @@ -838,6 +841,171 @@ static struct irq_chip octeon_irq_chip_ciu_wd = {
> .irq_mask = octeon_irq_dummy_mask,
> };
>
> +static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit)
> +{
> + bool edge = false;
> +
> + if (line == 0)
> + switch (bit) {
> + case 48 ... 49: /* GMX DRP */
> + case 50: /* IPD_DRP */
> + case 52 ... 55: /* Timers */
> + case 58: /* MPI */
> + edge = true;
> + break;
> + default:
> + break;
> + }
> + else /* line == 1 */
> + switch (bit) {
> + case 47: /* PTP */
> + edge = true;
> + break;
> + default:
> + break;
> + }
> + return edge;
Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?
> +}
> +
> +struct octeon_irq_gpio_domain_data {
> + unsigned int base_hwirq;
> +};
> +
> +static int octeon_irq_gpio_xlat(struct irq_domain *d,
> + struct device_node *node,
> + const u32 *intspec,
> + unsigned int intsize,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + unsigned int type;
> + unsigned int pin;
> + unsigned int trigger;
> + bool set_edge_handler = false;
> + struct octeon_irq_gpio_domain_data *gpiod;
> +
> + if (d->of_node != node)
> + return -EINVAL;
> +
> + if (intsize < 2)
> + return -EINVAL;
> +
> + pin = intspec[0];
> + if (pin >= 16)
> + return -EINVAL;
> +
> + trigger = intspec[1];
> +
> + switch (trigger) {
> + case 1:
> + type = IRQ_TYPE_EDGE_RISING;
> + set_edge_handler = true;
This is never used.
> + break;
> + case 2:
> + type = IRQ_TYPE_EDGE_FALLING;
> + set_edge_handler = true;
> + break;
> + case 4:
> + type = IRQ_TYPE_LEVEL_HIGH;
> + break;
> + case 8:
> + type = IRQ_TYPE_LEVEL_LOW;
> + break;
> + default:
> + pr_err("Error: (%s) Invalid irq trigger specification: %x\n",
> +node->name,
> +trigger);
> + type = IRQ_TYPE_LEVEL_LOW;
> + break;
> + }
> + *out_type = type;
Can't you get rid of the whole switch statement and just do:
*out_type = intspec[1];
> + gpiod = d->host_data;
> + *out_hwirq = gpiod->base_hwirq + pin;
> +
> + return 0;
> +}
> +
> +static int octeon_irq_ciu_xlat(struct irq_domain *d,
> +struct device_node *node,
> +const u32 *intspec,
> +unsigned int intsize,
> +unsigned long *out_hwirq,
> +unsigned int *out_type)
> +{
> + unsigned int ciu, bit;
> +
> + ciu = intspec[0];
> + bit = intspec[1];
> +
> + if (ciu > 1 || bit > 63)
> + return -EINVAL;
> +
> + /* These are the GPIO lines */
> + if (ciu == 0 && bit >= 16 && bit < 32)
> + return -EINVAL;
> +
> + *out_hwirq = (ciu << 6) | bit;
> + *out_type = 0;
> +
> + return 0;
> +}
> +
> +static st
[PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
From: David Daney
Create two domains. One for the GPIO lines, and the other for on-chip
sources.
Signed-off-by: David Daney
---
arch/mips/cavium-octeon/octeon-irq.c | 208 --
1 files changed, 199 insertions(+), 9 deletions(-)
diff --git a/arch/mips/cavium-octeon/octeon-irq.c
b/arch/mips/cavium-octeon/octeon-irq.c
index 89b6f27..550c03d 100644
--- a/arch/mips/cavium-octeon/octeon-irq.c
+++ b/arch/mips/cavium-octeon/octeon-irq.c
@@ -3,14 +3,17 @@
* License. See the file "COPYING" in the main directory of this archive
* for more details.
*
- * Copyright (C) 2004-2008, 2009, 2010, 2011 Cavium Networks
+ * Copyright (C) 2004-2012 Cavium, Inc.
*/
#include
+#include
#include
#include
+#include
#include
#include
+#include
#include
@@ -42,9 +45,9 @@ struct octeon_core_chip_data {
static struct octeon_core_chip_data
octeon_irq_core_chip_data[MIPS_CORE_IRQ_LINES];
-static void __init octeon_irq_set_ciu_mapping(int irq, int line, int bit,
- struct irq_chip *chip,
- irq_flow_handler_t handler)
+static void octeon_irq_set_ciu_mapping(int irq, int line, int bit,
+ struct irq_chip *chip,
+ irq_flow_handler_t handler)
{
union octeon_ciu_chip_data cd;
@@ -838,6 +841,171 @@ static struct irq_chip octeon_irq_chip_ciu_wd = {
.irq_mask = octeon_irq_dummy_mask,
};
+static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit)
+{
+ bool edge = false;
+
+ if (line == 0)
+ switch (bit) {
+ case 48 ... 49: /* GMX DRP */
+ case 50: /* IPD_DRP */
+ case 52 ... 55: /* Timers */
+ case 58: /* MPI */
+ edge = true;
+ break;
+ default:
+ break;
+ }
+ else /* line == 1 */
+ switch (bit) {
+ case 47: /* PTP */
+ edge = true;
+ break;
+ default:
+ break;
+ }
+ return edge;
+}
+
+struct octeon_irq_gpio_domain_data {
+ unsigned int base_hwirq;
+};
+
+static int octeon_irq_gpio_xlat(struct irq_domain *d,
+ struct device_node *node,
+ const u32 *intspec,
+ unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ unsigned int type;
+ unsigned int pin;
+ unsigned int trigger;
+ bool set_edge_handler = false;
+ struct octeon_irq_gpio_domain_data *gpiod;
+
+ if (d->of_node != node)
+ return -EINVAL;
+
+ if (intsize < 2)
+ return -EINVAL;
+
+ pin = intspec[0];
+ if (pin >= 16)
+ return -EINVAL;
+
+ trigger = intspec[1];
+
+ switch (trigger) {
+ case 1:
+ type = IRQ_TYPE_EDGE_RISING;
+ set_edge_handler = true;
+ break;
+ case 2:
+ type = IRQ_TYPE_EDGE_FALLING;
+ set_edge_handler = true;
+ break;
+ case 4:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ break;
+ case 8:
+ type = IRQ_TYPE_LEVEL_LOW;
+ break;
+ default:
+ pr_err("Error: (%s) Invalid irq trigger specification: %x\n",
+ node->name,
+ trigger);
+ type = IRQ_TYPE_LEVEL_LOW;
+ break;
+ }
+ *out_type = type;
+ gpiod = d->host_data;
+ *out_hwirq = gpiod->base_hwirq + pin;
+
+ return 0;
+}
+
+static int octeon_irq_ciu_xlat(struct irq_domain *d,
+ struct device_node *node,
+ const u32 *intspec,
+ unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ unsigned int ciu, bit;
+
+ ciu = intspec[0];
+ bit = intspec[1];
+
+ if (ciu > 1 || bit > 63)
+ return -EINVAL;
+
+ /* These are the GPIO lines */
+ if (ciu == 0 && bit >= 16 && bit < 32)
+ return -EINVAL;
+
+ *out_hwirq = (ciu << 6) | bit;
+ *out_type = 0;
+
+ return 0;
+}
+
+static struct irq_chip *octeon_irq_ciu_chip;
+static struct irq_chip *octeon_irq_gpio_chip;
+
+static int octeon_irq_ciu_map(struct irq_domain *d,
+ unsigned int virq, irq_hw_number_t hw)
+{
+ unsigned int line = hw >> 6;
+ unsigned int bit = hw & 63;
+
+ if (virq >= 256)
+ return -EINVAL;
+
+ if (line > 1 || octeon_irq_ciu_to_irq[line][bit] != 0)
+
