Re: [PATCH] sh: extend INTC with optional ioremap() support

[Magnus Damm <magnus.damm@gmail.com>]

On Wed, Feb 10, 2010 at 9:55 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Feb 10, 2010 at 08:17:39PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Add the member "io_window" to intc_desc to allow
>> passing a physical memory window to the INTC code.
>> The INTC code will ioremap the window if present.
>>
>> Required to support INTCS on SH-Mobile G-series.
>>
>> Signed-off-by: Magnus Damm <damm@opensource.se>
>> ---
>>
>>  drivers/sh/intc.c       |   14 ++++++++++++++
>>  include/linux/sh_intc.h |    3 +++
>>  2 files changed, 17 insertions(+)
>>
>> --- 0006/drivers/sh/intc.c
>> +++ work/drivers/sh/intc.c    2010-02-10 19:31:14.000000000 +0900
>> @@ -45,6 +45,8 @@ struct intc_handle_int {
>>
>>  struct intc_desc_int {
>>       struct list_head list;
>> +     unsigned long virt_base;
>> +     unsigned long phys_base;
>>       struct sys_device sysdev;
>>       pm_message_t state;
>>       unsigned long *reg;
>
> phys_base can be a phys_addr_t, while virt_base wants to be a
> void __iomem *.

Right, I almost converted the code to use void __iomem *.

>> @@ -800,6 +808,12 @@ void __init register_intc_controller(str
>>       INIT_LIST_HEAD(&d->list);
>>       list_add(&d->list, &intc_list);
>>
>> +     if (desc->io_window) {
>> +             d->phys_base = desc->io_window->start;
>> +             d->virt_base = (unsigned long)ioremap_nocache(d->phys_base,
>> +                                            resource_size(desc->io_window));
>> +     }
>> +
> Error handling?

There is basically no error handling in the INTC code except some
BUG()s. Reworking the code a bit to get better error handling sounds
like a good plan.

>> --- 0006/include/linux/sh_intc.h
>> +++ work/include/linux/sh_intc.h      2010-02-10 19:31:14.000000000 +0900
>> @@ -71,6 +73,7 @@ struct intc_hw_desc {
>>
>>  struct intc_desc {
>>       char *name;
>> +     struct resource *io_window;
>>       intc_enum force_enable;
>>       struct intc_hw_desc hw;
>>  };
>
> io_window is pretty much completely lacking in meaning. Why does this
> platform want to pass in a resource that gets remapped while others do
> not?

Yes. I'm open to naming suggestions, but I don't think that's what
you're after. On G-series the INTCA block is covered by the 1:1 mapped
virt:phys, so it does not need to be ioremapped. The INTCS block OTOH,
it's not covered by any 1:1 mapping and therefore needs ioremap()
badly.

> This all looks terribly fragile. You're basically counting on the fact
> that all addresses are going to be covered by this window, which seems
> like a pretty lofty assumption given how some of the INTC2 registers were
> mapped out. It's also pretty ambiguous, given that this is obviously a
> an MMIO resource yet is named like a PIO one.

In some cases we may have multiple separate interrupt controllers that
happen to be covered by a single INTC table. It's of course possible
to have multiple ranges, but since it's not directly required for
INTCS support I thought I'd go with the simplest possible solution to
begin with. If the user is accessing registers outside the window then
it's a programming error - very similar to anyone accessing outside
ioremapped memory areas. Just don't. I guess you'd like to see some
range checking in the cold paths.

> Special casing the remapping is really just crap. Each of the register
> ranges wants to be remapped, while on SH given that these are in the P4
> space nothing of much note will happen. You are better off throwing in
> void __iomem * pointers for each of the register ranges that you are
> passing in and permit the data structures to use offsets in to that. This
> can be done incrementally for the SH parts, since remapping isn't going
> to change the underlying address.

So how exactly do you propose that we change the INTC data structures?
Interrupt controller code from the days before INTC used base
addresses and offsets if I'm not mistaken, and that was both hard to
read and quite error prone. I'm of course open to improve the INTC
code, but I don't see any obvious way to extend the tables as they are
today.

/ magnus