* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
@ 2012-03-28 13:15 ` Arnd Bergmann
2012-03-29 5:29 ` Magnus Damm
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2012-03-28 13:15 UTC (permalink / raw)
To: linux-sh
On Wednesday 28 March 2012, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
>
> This prototype patch extends the sh7372 DT support with actual
> IRQ support. The heavy lifting is all done by the prototype patch
> "[PATCH] sh: INTC IRQ domain and DT support prototype".
>
> It is worth noting that we have 3 root interrupt controllers on
> sh7372 and to set the parent pointer to NULL we let the device
> tree point to itself as interrupt-parent. Seems a bit overly
> complex so perhaps it is not the right way forward.
>
> All root interrupt controllers are using IRQ domains but their
> allocation is kept the same as the non-DT case to allow us to
> leave the demux code as-is without any modifications. So in
> the DT case INTCA, IRQ16L and IRQ16H are kept at their original
> IRQ positions. INTCS is allocated dynamically in case of DT
> and at a fixed location as usual for non-DT boards.
>
> Anyone that wants to play with DT with interrupts on sh7372
> will need to include this patch. The INTC IRQ domain patch above
> needs further work which will delay the merge.
>
> Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
Hi Magnus,
I'm trying to find my way through your patches, but I still have
a little trouble figuring out how it all fits together.
My feeling is that the soc specific parts can be done better
if you generalize the interrupt controller bindings so that
you can describe the controller(s) as a single device, and
allow all the information that is now in the intc-*.c files
to be moved into device tree attributes to be parsed at boot
time, or from the translate() function of the interrupt controller
driver.
That feeling may of course be completely wrong. Do you have
a link to a data sheet describing how that controller actually
works, or can you explain what the various arrays are needed
for today, and how the various interrupt controllers you register
fit together?
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
2012-03-28 13:15 ` Arnd Bergmann
@ 2012-03-29 5:29 ` Magnus Damm
2012-03-29 13:06 ` Arnd Bergmann
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Magnus Damm @ 2012-03-29 5:29 UTC (permalink / raw)
To: linux-sh
Hello Arnd,
On Wed, Mar 28, 2012 at 10:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 28 March 2012, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> This prototype patch extends the sh7372 DT support with actual
>> IRQ support. The heavy lifting is all done by the prototype patch
>> "[PATCH] sh: INTC IRQ domain and DT support prototype".
>>
>> It is worth noting that we have 3 root interrupt controllers on
>> sh7372 and to set the parent pointer to NULL we let the device
>> tree point to itself as interrupt-parent. Seems a bit overly
>> complex so perhaps it is not the right way forward.
>>
>> All root interrupt controllers are using IRQ domains but their
>> allocation is kept the same as the non-DT case to allow us to
>> leave the demux code as-is without any modifications. So in
>> the DT case INTCA, IRQ16L and IRQ16H are kept at their original
>> IRQ positions. INTCS is allocated dynamically in case of DT
>> and at a fixed location as usual for non-DT boards.
>>
>> Anyone that wants to play with DT with interrupts on sh7372
>> will need to include this patch. The INTC IRQ domain patch above
>> needs further work which will delay the merge.
>>
>> Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
>
> Hi Magnus,
>
> I'm trying to find my way through your patches, but I still have
> a little trouble figuring out how it all fits together.
Right, sorry for spitting out patches to the left and to the right. =)
So the full dependencies for "base DT support" are:
renesas.git a6e24019468009a21b674e392d74283a90f415dd (origin/master)
[PATCH 00/06] mach-shmobile device tree preparation patches V2
[PATCH] ARM: mach-shmobile: sh7372 generic board support via DT V2
The patches above are rather ready IMO, and they've got Signed-off-by.
To get prototype level support of IRQs you will also need the following:
[PATCH] sh: INTC IRQ domain and DT support prototype
[PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
The two patches above are experimental with Not-yet-signed-off-by.
There are more patches as well, but most depend on the prototype IRQ code above.
> My feeling is that the soc specific parts can be done better
> if you generalize the interrupt controller bindings so that
> you can describe the controller(s) as a single device, and
> allow all the information that is now in the intc-*.c files
> to be moved into device tree attributes to be parsed at boot
> time, or from the translate() function of the interrupt controller
> driver.
For sh7372 and most other of our SoCs I believe it will be difficult
to use a single device for interrupts. This since there are a few
different interrupt controllers working together, and they have
different interrupt ranges associated with them. They fit quite well
with IRQ domains and the ->dt_translate() callback and gluing them
back together would sort of defeat the point with IRQ domains to begin
with. It would also make it more difficult to implement proper power
management.
However, the idea that you'd like to describe the controller with the
device tree is interesting. At this point I'm unsure how to do that.
Also, I do wonder if it makes sense to do so at all - most new parts
use the GIC as the main interrupt controller anyway. I do agree with
the plan of separating configuration from code, and in our INTC case
we've sort of already done so about 5 years ago. At that point I
converted a few separate random implementations to the current table
driven INTC code base. Since then we have soon close to 100 different
users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
free to grep for "register_intc_controller" to explore by yourself. It
would be fun to try to use the device tree for this, but I wonder how
we can describe anything half-complex without a preprocessor. Both the
INTC code and the PFC used for GPIO and pinmux use C enums a lot.
> That feeling may of course be completely wrong. Do you have
> a link to a data sheet describing how that controller actually
> works, or can you explain what the various arrays are needed
> for today, and how the various interrupt controllers you register
> fit together?
Sorry to say this, but I doubt that there are any public documents for
our ARM based SoCs. Next time we meet face to face perhaps we can
discuss about providing a board and documentation to you?
I'd be happy to try to describe how the INTC code works and how the
sh7372 interrupt controllers are hooked together. Perhaps we can
combine this with a chat for some more interactivity?
Thanks for your help!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
2012-03-28 13:15 ` Arnd Bergmann
2012-03-29 5:29 ` Magnus Damm
@ 2012-03-29 13:06 ` Arnd Bergmann
2012-03-30 4:01 ` Paul Mundt
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2012-03-29 13:06 UTC (permalink / raw)
To: linux-sh
On Thursday 29 March 2012, Magnus Damm wrote:
> > Hi Magnus,
> >
> > I'm trying to find my way through your patches, but I still have
> > a little trouble figuring out how it all fits together.
>
> Right, sorry for spitting out patches to the left and to the right. =)
>
> So the full dependencies for "base DT support" are:
>
> renesas.git a6e24019468009a21b674e392d74283a90f415dd (origin/master)
> [PATCH 00/06] mach-shmobile device tree preparation patches V2
> [PATCH] ARM: mach-shmobile: sh7372 generic board support via DT V2
>
> The patches above are rather ready IMO, and they've got Signed-off-by.
>
> To get prototype level support of IRQs you will also need the following:
> [PATCH] sh: INTC IRQ domain and DT support prototype
> [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
>
> The two patches above are experimental with Not-yet-signed-off-by.
>
> There are more patches as well, but most depend on the prototype IRQ code above.
Do you have a public git tree that has all of them applied?
The separate patches are good for reviewing when one already
knows the code, but in trying to get the big picture, I'
prefer to look at the state after the patches.
> > My feeling is that the soc specific parts can be done better
> > if you generalize the interrupt controller bindings so that
> > you can describe the controller(s) as a single device, and
> > allow all the information that is now in the intc-*.c files
> > to be moved into device tree attributes to be parsed at boot
> > time, or from the translate() function of the interrupt controller
> > driver.
>
> For sh7372 and most other of our SoCs I believe it will be difficult
> to use a single device for interrupts. This since there are a few
> different interrupt controllers working together, and they have
> different interrupt ranges associated with them. They fit quite well
> with IRQ domains and the ->dt_translate() callback and gluing them
> back together would sort of defeat the point with IRQ domains to begin
> with. It would also make it more difficult to implement proper power
> management.
Right. It wasn't clear to what degree they are actually separate bits
of hardware. If the controllers are nested in some way, you should
definitely describe them as individual device nodes.
> However, the idea that you'd like to describe the controller with the
> device tree is interesting. At this point I'm unsure how to do that.
> Also, I do wonder if it makes sense to do so at all - most new parts
> use the GIC as the main interrupt controller anyway. I do agree with
> the plan of separating configuration from code, and in our INTC case
> we've sort of already done so about 5 years ago. At that point I
> converted a few separate random implementations to the current table
> driven INTC code base. Since then we have soon close to 100 different
> users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
> free to grep for "register_intc_controller" to explore by yourself. It
> would be fun to try to use the device tree for this, but I wonder how
> we can describe anything half-complex without a preprocessor. Both the
> INTC code and the PFC used for GPIO and pinmux use C enums a lot.
Ah, so the vast majority of these are not for shmobile but for arch/sh
and I support you have no plans to move those to DT along with shmobile,
right?
From my reading of the source code, your INTCA and INTCS controllers
all follow the same basic model, but they all differ in the mapping
between interrupt vectors to register locations.
I would probably not try to describe this mapping in the interrupt
controller node, but instead use a more elaborate way to describe
an interrupt than just the vector.
Taking the sh7372 USB0_USB0I0 interrupt as an arbitrary example,
you could encode it as
interrupts = <0x1ca0 /* vector */
0xa4 5 /* mask register offset and bit position */
0x4c 1>; /* prio register offset and bit position */
An interrupt controller that only needs these (no sense/ack registers),
would set #interrupt-cells=<5>;, while other interrupt controllers would
use a larger number of interrupt cells to also encode the extra data.
You can also use the interrupt-map to turn these into more readable
numbers for use by the devices.
I don't understand what your interrupt groups are, or whether you need
even more attributes somewhere.
> > That feeling may of course be completely wrong. Do you have
> > a link to a data sheet describing how that controller actually
> > works, or can you explain what the various arrays are needed
> > for today, and how the various interrupt controllers you register
> > fit together?
>
> Sorry to say this, but I doubt that there are any public documents for
> our ARM based SoCs. Next time we meet face to face perhaps we can
> discuss about providing a board and documentation to you?
Sounds good. I'll be at LinuxCon Japan in June, after the Hong Kong
Linaro Connect.
> I'd be happy to try to describe how the INTC code works and how the
> sh7372 interrupt controllers are hooked together. Perhaps we can
> combine this with a chat for some more interactivity?
Ok, let's try to meet up on IRC on #armlinux on freenode then.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
` (2 preceding siblings ...)
2012-03-29 13:06 ` Arnd Bergmann
@ 2012-03-30 4:01 ` Paul Mundt
2012-03-30 7:26 ` Magnus Damm
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Paul Mundt @ 2012-03-30 4:01 UTC (permalink / raw)
To: linux-sh
On Thu, Mar 29, 2012 at 01:06:38PM +0000, Arnd Bergmann wrote:
> On Thursday 29 March 2012, Magnus Damm wrote:
> > However, the idea that you'd like to describe the controller with the
> > device tree is interesting. At this point I'm unsure how to do that.
> > Also, I do wonder if it makes sense to do so at all - most new parts
> > use the GIC as the main interrupt controller anyway. I do agree with
> > the plan of separating configuration from code, and in our INTC case
> > we've sort of already done so about 5 years ago. At that point I
> > converted a few separate random implementations to the current table
> > driven INTC code base. Since then we have soon close to 100 different
> > users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
> > free to grep for "register_intc_controller" to explore by yourself. It
> > would be fun to try to use the device tree for this, but I wonder how
> > we can describe anything half-complex without a preprocessor. Both the
> > INTC code and the PFC used for GPIO and pinmux use C enums a lot.
>
> Ah, so the vast majority of these are not for shmobile but for arch/sh
> and I support you have no plans to move those to DT along with shmobile,
> right?
>
At the moment the main priority is really just the shared SoC blocks, so
things like the interrupt controllers, serial driver, pfc/pinmux/gpios,
dmaengine, etc. The interrupt controller subystem that we use for all of
our cases is sufficiently complex that it's really not an ideal choice in
terms of low-hanging fruit for DT support, in fact, it's probably the
worst possible place to start.
I'm in the middle of irqdomain refactoring for it now, which will also
entail quite a lot of rework with regards to how we deal with our virtual
IRQs, radix tree utilization, and so forth. What sort of information we
need to still keep around ultimately won't be known until we have a
clearer picture of how the irqdomain tie-in will look for the existing
cases. Only after that does it really make any sense to start on anything
but the most trivial of DT bindings.
Beyond that, while the vast majority of the INTC cases are in arch/sh
today, infrastructure-wise everything is sufficiently coupled together
that it wouldn't make things any simpler to treat INTCA/INTCS specially.
Once we start seeing DT bindings in shared infrastructure (especially
drivers/sh and so on) then I'll probably start gradually migrating
arch/sh users over to it, too. For the majority of users we don't really
have any sort of coherent boot ABI, which means that we're more than
likely going to always have to link the blob in to the kernel directly,
but that's another matter.
The GPIO/PFC pinmux/pinctrl bits are likewise quite complex and in flux
while I work on pinctrl migration (some work done now, but more
realistically after the irqdomain rework has happened), so it's not worth
spending any time on DT bindings there for the moment either.
At the moment pretty much any other platform device is orders of
magnitude more simplistic and workable though. I'm not sure why DT
prototyping didn't start there, instead.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
` (3 preceding siblings ...)
2012-03-30 4:01 ` Paul Mundt
@ 2012-03-30 7:26 ` Magnus Damm
2012-03-30 7:27 ` Magnus Damm
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Magnus Damm @ 2012-03-30 7:26 UTC (permalink / raw)
To: linux-sh
Hello Arnd,
On Thu, Mar 29, 2012 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 29 March 2012, Magnus Damm wrote:
>
>> > Hi Magnus,
>> >
>> > I'm trying to find my way through your patches, but I still have
>> > a little trouble figuring out how it all fits together.
>>
>> Right, sorry for spitting out patches to the left and to the right. =)
>>
>> So the full dependencies for "base DT support" are:
>>
>> renesas.git a6e24019468009a21b674e392d74283a90f415dd (origin/master)
>> [PATCH 00/06] mach-shmobile device tree preparation patches V2
>> [PATCH] ARM: mach-shmobile: sh7372 generic board support via DT V2
>>
>> The patches above are rather ready IMO, and they've got Signed-off-by.
>>
>> To get prototype level support of IRQs you will also need the following:
>> [PATCH] sh: INTC IRQ domain and DT support prototype
>> [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
>>
>> The two patches above are experimental with Not-yet-signed-off-by.
>>
>> There are more patches as well, but most depend on the prototype IRQ code above.
>
> Do you have a public git tree that has all of them applied?
Sorry, there is no such thing. I believe Rafael will pick up the ones
with Signed-off-by and put them in a branch based on v3.4-rc1. He may
be able to pull in other more experimental patches based on that tree,
but I am somewhat hesitant since I don't want to give people the false
impression that the code is ready when it's certainly not. The
experimental code will change a lot, that's a fact.
> The separate patches are good for reviewing when one already
> knows the code, but in trying to get the big picture, I'
> prefer to look at the state after the patches.
I understand. I suppose the branch created by Rafael will take you
half-way at least.
>> > My feeling is that the soc specific parts can be done better
>> > if you generalize the interrupt controller bindings so that
>> > you can describe the controller(s) as a single device, and
>> > allow all the information that is now in the intc-*.c files
>> > to be moved into device tree attributes to be parsed at boot
>> > time, or from the translate() function of the interrupt controller
>> > driver.
>>
>> For sh7372 and most other of our SoCs I believe it will be difficult
>> to use a single device for interrupts. This since there are a few
>> different interrupt controllers working together, and they have
>> different interrupt ranges associated with them. They fit quite well
>> with IRQ domains and the ->dt_translate() callback and gluing them
>> back together would sort of defeat the point with IRQ domains to begin
>> with. It would also make it more difficult to implement proper power
>> management.
>
> Right. It wasn't clear to what degree they are actually separate bits
> of hardware. If the controllers are nested in some way, you should
> definitely describe them as individual device nodes.
Good!
>> However, the idea that you'd like to describe the controller with the
>> device tree is interesting. At this point I'm unsure how to do that.
>> Also, I do wonder if it makes sense to do so at all - most new parts
>> use the GIC as the main interrupt controller anyway. I do agree with
>> the plan of separating configuration from code, and in our INTC case
>> we've sort of already done so about 5 years ago. At that point I
>> converted a few separate random implementations to the current table
>> driven INTC code base. Since then we have soon close to 100 different
>> users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
>> free to grep for "register_intc_controller" to explore by yourself. It
>> would be fun to try to use the device tree for this, but I wonder how
>> we can describe anything half-complex without a preprocessor. Both the
>> INTC code and the PFC used for GPIO and pinmux use C enums a lot.
>
> Ah, so the vast majority of these are not for shmobile but for arch/sh
> and I support you have no plans to move those to DT along with shmobile,
> right?
Most use cases are still on SH, but for mach-shmobile we make use of
either GIC of INTCA together with a cascaded INTCS. We also have IRQ
pins that can be controlled from either INTCA or INTCS or just
level/edge trigger configuration in front of GIC. In some cases we
have an interrupt controller called PINT. Some PFC/GPIO controllers
have built-in interrupt controllers, and some GPIOs have their
interrupts tied into the INTC as IRQ pins. There is a big jambalaya of
hardware IP with history from Mitsubishi, Hitachi and NEC, plus the
usual not-so-exotic ones.
So with this in mind I believe we will need INTC support for
mach-shmobile in the future as well. The INTC controllers seem to
become more like leaf nodes these days though. =)
> From my reading of the source code, your INTCA and INTCS controllers
> all follow the same basic model, but they all differ in the mapping
> between interrupt vectors to register locations.
All INTC tables are unique. So the INTCS in sh7372 is not the same as
the INTCS in sh7367. Some parts of the tables may look similar, but
low hanging fruit such as external IRQ pin handling has been broken
out into reusable components already.
> I would probably not try to describe this mapping in the interrupt
> controller node, but instead use a more elaborate way to describe
> an interrupt than just the vector.
That's certainly one way to split out the information, but I wonder if
that's the right way to go. I worry that with such setup the
correctness of the mappings between vectors and register fields is
dependent on the system integrator doing proper copy-paste
"development". One character wrong and you will have a silent error
somewhere with the wrong interrupt being masked by accident for
instance.
> Taking the sh7372 USB0_USB0I0 interrupt as an arbitrary example,
> you could encode it as
>
> interrupts = <0x1ca0 /* vector */
> 0xa4 5 /* mask register offset and bit position */
> 0x4c 1>; /* prio register offset and bit position */
>
> An interrupt controller that only needs these (no sense/ack registers),
> would set #interrupt-cells=<5>;, while other interrupt controllers would
> use a larger number of interrupt cells to also encode the extra data.
Thanks for showing this example. I think your proposal is interesting.
I believe I get the big picture, but I wonder if it is that simple
after all. In this specific example the USB0_USB0I0 interrupt source
also needs a 0xe4 associated with the mask register. Some mask
registers come in pairs with one clear and one set register. We also
have the width of the register and width of the priority field as
data. Of course the actual register mapping with such information
could be kept together with the INTC configuration data for the
interrupt controller, and we can for instance use register offset and
bit field index to locate the actual bit field or bit. This becomes
quite close to how INTC is working today though, with the exception
that we use enums with the same names as the data sheet. I don't see
how this can be done in Device Tree lingo.
If possible I'd like to keep the interrupt tables self-contained in
one place. I don't mind so much if it is kept in C or DT, but I'd like
to keep the same goals as we had with our INTC tables:
- Easy data entry straight from data sheet
- O(1) interrupt handling for timing critical interrupt handling
- Low memory consumption
- Some way to reduce risk of typos (preprocessor in case of C)
> You can also use the interrupt-map to turn these into more readable
> numbers for use by the devices.
Ok, perhaps that will remove the undesired side effects then. If so it
may be a nice idea.
> I don't understand what your interrupt groups are, or whether you need
> even more attributes somewhere.
The groups are used to link together multiple interrupt sources
(enums) that share a priority setting.
Anyway, just to be clear, at this point i mainly wanted to enable DT
support on the mach-shmobile subarch and sh7372 in particular. So in
my mind this excludes reworking the INTC and PFC tables. Such
activities will take quite some time due to them being shared with SH.
Of course we should work in that direction, but at this point I 'd
like to focus on getting other people to work with DT support in the
drivers. So because of that I spent time on getting interrupts going
with the device tree instead of figuring out how to replace the INTC
tables with something else. Step by step.
I assume that your current merge policy is that boards should be
implemented in DT, but we can still merge new SoCs with current style
of INTC and PFC tables as usual?
I hope that's the case, and if not then there is an increased risk of
out of tree SoCs development. This to be able to meet customer demand
on schedule. I'd like to keep the customer bits synchronized with
upstream, and if we can't get things merged due to lacking
dependencies then we will need to rethink how to deal with this.
Thanks for your help with the review. I very much welcome your
comments on the patches with Signed-off-by, and as for the patches
with Not-yet-signed-off-by, please note that they are work in
progress. The IRQ related bits will be reworked and upstreamed by Paul
Mundt. So I see my work in the IRQ department close to done - apart
from chatting with you about it of course.
>> > That feeling may of course be completely wrong. Do you have
>> > a link to a data sheet describing how that controller actually
>> > works, or can you explain what the various arrays are needed
>> > for today, and how the various interrupt controllers you register
>> > fit together?
>>
>> Sorry to say this, but I doubt that there are any public documents for
>> our ARM based SoCs. Next time we meet face to face perhaps we can
>> discuss about providing a board and documentation to you?
>
> Sounds good. I'll be at LinuxCon Japan in June, after the Hong Kong
> Linaro Connect.
Cool, see you then!
>> I'd be happy to try to describe how the INTC code works and how the
>> sh7372 interrupt controllers are hooked together. Perhaps we can
>> combine this with a chat for some more interactivity?
>
> Ok, let's try to meet up on IRC on #armlinux on freenode then.
Good idea. I'll contact you privately to agree on some time that works
with both meeting schedules and time zones.
Thanks,
/ magnus
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
` (4 preceding siblings ...)
2012-03-30 7:26 ` Magnus Damm
@ 2012-03-30 7:27 ` Magnus Damm
2012-03-30 14:38 ` Arnd Bergmann
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Magnus Damm @ 2012-03-30 7:27 UTC (permalink / raw)
To: linux-sh
On Fri, Mar 30, 2012 at 1:01 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Thu, Mar 29, 2012 at 01:06:38PM +0000, Arnd Bergmann wrote:
>> On Thursday 29 March 2012, Magnus Damm wrote:
>> > However, the idea that you'd like to describe the controller with the
>> > device tree is interesting. At this point I'm unsure how to do that.
>> > Also, I do wonder if it makes sense to do so at all - most new parts
>> > use the GIC as the main interrupt controller anyway. I do agree with
>> > the plan of separating configuration from code, and in our INTC case
>> > we've sort of already done so about 5 years ago. At that point I
>> > converted a few separate random implementations to the current table
>> > driven INTC code base. Since then we have soon close to 100 different
>> > users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
>> > free to grep for "register_intc_controller" to explore by yourself. It
>> > would be fun to try to use the device tree for this, but I wonder how
>> > we can describe anything half-complex without a preprocessor. Both the
>> > INTC code and the PFC used for GPIO and pinmux use C enums a lot.
>>
>> Ah, so the vast majority of these are not for shmobile but for arch/sh
>> and I support you have no plans to move those to DT along with shmobile,
>> right?
>>
> At the moment the main priority is really just the shared SoC blocks, so
> things like the interrupt controllers, serial driver, pfc/pinmux/gpios,
> dmaengine, etc. The interrupt controller subystem that we use for all of
> our cases is sufficiently complex that it's really not an ideal choice in
> terms of low-hanging fruit for DT support, in fact, it's probably the
> worst possible place to start.
>
> I'm in the middle of irqdomain refactoring for it now, which will also
> entail quite a lot of rework with regards to how we deal with our virtual
> IRQs, radix tree utilization, and so forth. What sort of information we
> need to still keep around ultimately won't be known until we have a
> clearer picture of how the irqdomain tie-in will look for the existing
> cases. Only after that does it really make any sense to start on anything
> but the most trivial of DT bindings.
>
> Beyond that, while the vast majority of the INTC cases are in arch/sh
> today, infrastructure-wise everything is sufficiently coupled together
> that it wouldn't make things any simpler to treat INTCA/INTCS specially.
>
> Once we start seeing DT bindings in shared infrastructure (especially
> drivers/sh and so on) then I'll probably start gradually migrating
> arch/sh users over to it, too. For the majority of users we don't really
> have any sort of coherent boot ABI, which means that we're more than
> likely going to always have to link the blob in to the kernel directly,
> but that's another matter.
>
> The GPIO/PFC pinmux/pinctrl bits are likewise quite complex and in flux
> while I work on pinctrl migration (some work done now, but more
> realistically after the irqdomain rework has happened), so it's not worth
> spending any time on DT bindings there for the moment either.
>
> At the moment pretty much any other platform device is orders of
> magnitude more simplistic and workable though. I'm not sure why DT
> prototyping didn't start there, instead.
It did, but more or less all device drivers need IRQs.
/ magnus
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
` (5 preceding siblings ...)
2012-03-30 7:27 ` Magnus Damm
@ 2012-03-30 14:38 ` Arnd Bergmann
2012-03-30 14:43 ` Arnd Bergmann
2012-04-03 9:40 ` Magnus Damm
8 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2012-03-30 14:38 UTC (permalink / raw)
To: linux-sh
On Friday 30 March 2012, Magnus Damm wrote:
> >> However, the idea that you'd like to describe the controller with the
> >> device tree is interesting. At this point I'm unsure how to do that.
> >> Also, I do wonder if it makes sense to do so at all - most new parts
> >> use the GIC as the main interrupt controller anyway. I do agree with
> >> the plan of separating configuration from code, and in our INTC case
> >> we've sort of already done so about 5 years ago. At that point I
> >> converted a few separate random implementations to the current table
> >> driven INTC code base. Since then we have soon close to 100 different
> >> users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
> >> free to grep for "register_intc_controller" to explore by yourself. It
> >> would be fun to try to use the device tree for this, but I wonder how
> >> we can describe anything half-complex without a preprocessor. Both the
> >> INTC code and the PFC used for GPIO and pinmux use C enums a lot.
> >
> > Ah, so the vast majority of these are not for shmobile but for arch/sh
> > and I support you have no plans to move those to DT along with shmobile,
> > right?
>
> Most use cases are still on SH, but for mach-shmobile we make use of
> either GIC of INTCA together with a cascaded INTCS. We also have IRQ
> pins that can be controlled from either INTCA or INTCS or just
> level/edge trigger configuration in front of GIC. In some cases we
> have an interrupt controller called PINT. Some PFC/GPIO controllers
> have built-in interrupt controllers, and some GPIOs have their
> interrupts tied into the INTC as IRQ pins. There is a big jambalaya of
> hardware IP with history from Mitsubishi, Hitachi and NEC, plus the
> usual not-so-exotic ones.
>
> So with this in mind I believe we will need INTC support for
> mach-shmobile in the future as well. The INTC controllers seem to
> become more like leaf nodes these days though. =)
Ok, I see
> > From my reading of the source code, your INTCA and INTCS controllers
> > all follow the same basic model, but they all differ in the mapping
> > between interrupt vectors to register locations.
>
> All INTC tables are unique. So the INTCS in sh7372 is not the same as
> the INTCS in sh7367. Some parts of the tables may look similar, but
> low hanging fruit such as external IRQ pin handling has been broken
> out into reusable components already.
What I meant was that the table structure is always the same, even
if the contents are are different. It's fairly easy to replace
data from the kernel sources with data in the device tree, but
replacing code from the kernel with data is much harder.
> > I would probably not try to describe this mapping in the interrupt
> > controller node, but instead use a more elaborate way to describe
> > an interrupt than just the vector.
>
> That's certainly one way to split out the information, but I wonder if
> that's the right way to go. I worry that with such setup the
> correctness of the mappings between vectors and register fields is
> dependent on the system integrator doing proper copy-paste
> "development". One character wrong and you will have a silent error
> somewhere with the wrong interrupt being masked by accident for
> instance.
All the bits describing the interrupt controller node specific
to one soc should be able to go into the .dtsi file for that soc,
so that a system integrator only needs to hook up the devices that
are outside of the soc.
> > Taking the sh7372 USB0_USB0I0 interrupt as an arbitrary example,
> > you could encode it as
> >
> > interrupts = <0x1ca0 /* vector */
> > 0xa4 5 /* mask register offset and bit position */
> > 0x4c 1>; /* prio register offset and bit position */
> >
> > An interrupt controller that only needs these (no sense/ack registers),
> > would set #interrupt-cells=<5>;, while other interrupt controllers would
> > use a larger number of interrupt cells to also encode the extra data.
>
> Thanks for showing this example. I think your proposal is interesting.
> I believe I get the big picture, but I wonder if it is that simple
> after all. In this specific example the USB0_USB0I0 interrupt source
> also needs a 0xe4 associated with the mask register. Some mask
> registers come in pairs with one clear and one set register. We also
> have the width of the register and width of the priority field as
> data. Of course the actual register mapping with such information
> could be kept together with the INTC configuration data for the
> interrupt controller, and we can for instance use register offset and
> bit field index to locate the actual bit field or bit. This becomes
> quite close to how INTC is working today though, with the exception
> that we use enums with the same names as the data sheet. I don't see
> how this can be done in Device Tree lingo.
There are two places where you could put that information. The one
I described above is the interrupts property, but if you have to
add many more fields, this gets impractical.
The other place would be sub-nodes of the interrupt controller node
that don't get translated into individual platform devices.
You can have one node per interrupt line, like this:
intc@6940000 {
#address-cells = <2>;
#size-cells = <0>;
reg = <0x6940000 0x1000>;
ranges = <1 0 0x6940000 0x54 /* prio */
2 0 0x6940080 0x3c /* mask */
3 0 0x69400c0 0x3c>; /* unmask /
/* common properties for priority nodes,
could get overridden if necessary */
prio-reg-width = <16>;
prio-width = <4>;
iprta3 {
#address-cells = <2>;
#size-cells = <0>;
ranges;
reg = <1 0x4c> /* priority reg 0x4c */
usb0_usb0i0 {
reg = <0 0x1ca0 /* vector 0x1ca0 */
1 0x34 /* mask 0x80 + 0x34 */
2 0x34>; /* unmask 0xc0 + 0x34 */
reg-mask = <0x04>; /* bitmask for registers */
};
};
};
> > I don't understand what your interrupt groups are, or whether you need
> > even more attributes somewhere.
>
> The groups are used to link together multiple interrupt sources
> (enums) that share a priority setting.
>
> Anyway, just to be clear, at this point i mainly wanted to enable DT
> support on the mach-shmobile subarch and sh7372 in particular. So in
> my mind this excludes reworking the INTC and PFC tables. Such
> activities will take quite some time due to them being shared with SH.
> Of course we should work in that direction, but at this point I 'd
> like to focus on getting other people to work with DT support in the
> drivers. So because of that I spent time on getting interrupts going
> with the device tree instead of figuring out how to replace the INTC
> tables with something else. Step by step.
>
> I assume that your current merge policy is that boards should be
> implemented in DT, but we can still merge new SoCs with current style
> of INTC and PFC tables as usual?
Yes, that sounds ok. The part that I would not want to see though
is having a preliminary binding for INTC that you later have to
replace by another binding, breaking all device tree files that
were produced at first.
It's ok if you add more data to the intc nodes at a later
point while incrementally moving stuff into dts files, but I think
you should decide on how to represent an interrupt line in the devices
using intc.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
` (6 preceding siblings ...)
2012-03-30 14:38 ` Arnd Bergmann
@ 2012-03-30 14:43 ` Arnd Bergmann
2012-04-03 9:40 ` Magnus Damm
8 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2012-03-30 14:43 UTC (permalink / raw)
To: linux-sh
On Friday 30 March 2012, Paul Mundt wrote:
> On Thu, Mar 29, 2012 at 01:06:38PM +0000, Arnd Bergmann wrote:
> > On Thursday 29 March 2012, Magnus Damm wrote:
> > > However, the idea that you'd like to describe the controller with the
> > > device tree is interesting. At this point I'm unsure how to do that.
> > > Also, I do wonder if it makes sense to do so at all - most new parts
> > > use the GIC as the main interrupt controller anyway. I do agree with
> > > the plan of separating configuration from code, and in our INTC case
> > > we've sort of already done so about 5 years ago. At that point I
> > > converted a few separate random implementations to the current table
> > > driven INTC code base. Since then we have soon close to 100 different
> > > users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
> > > free to grep for "register_intc_controller" to explore by yourself. It
> > > would be fun to try to use the device tree for this, but I wonder how
> > > we can describe anything half-complex without a preprocessor. Both the
> > > INTC code and the PFC used for GPIO and pinmux use C enums a lot.
> >
> > Ah, so the vast majority of these are not for shmobile but for arch/sh
> > and I support you have no plans to move those to DT along with shmobile,
> > right?
> >
> At the moment the main priority is really just the shared SoC blocks, so
> things like the interrupt controllers, serial driver, pfc/pinmux/gpios,
> dmaengine, etc. The interrupt controller subystem that we use for all of
> our cases is sufficiently complex that it's really not an ideal choice in
> terms of low-hanging fruit for DT support, in fact, it's probably the
> worst possible place to start.
>
> I'm in the middle of irqdomain refactoring for it now, which will also
> entail quite a lot of rework with regards to how we deal with our virtual
> IRQs, radix tree utilization, and so forth. What sort of information we
> need to still keep around ultimately won't be known until we have a
> clearer picture of how the irqdomain tie-in will look for the existing
> cases. Only after that does it really make any sense to start on anything
> but the most trivial of DT bindings.
In my experience, the interrupt controller bindings are among the first
things that one wants to work on, followed by the gpio bindings, because
most other things tend to depend on them. Now that doesn't mean the code
has to actually use all the information you put into the device tree,
but since it describes the hardware, it should be possible to find out
what information is really needed in there to describe each interrupt
line.
For INTC, you basically have two options of describing them:
a) As in Magnus' patch, give a separate "compatible" name to each
version, and let it be handled by its own code.
b) As I suggested, make a fully generic INTC binding that encodes
all the differences in device tree properties.
The first one has the significant advantage of requiring only
moderate changes to the source code, and you already have a prototype
for them. The second one requires much more work but has the advantage
that you can use it later to add support for new SoC versions
without having to add source code for the new instance of that
interrupt controller. If all other drivers for the new Soc are already
present in the kernel, you can even keep using the same kernel binaries,
which you would not be able to do with approach a).
You can probably defer the problem for a bit if you first do DT work
only for the SoCs that use GIC, which already has an established
binding. Also, it's possible to come up with a binding that you
expect to work and that can represent all the data you need, but
still keep using the existing code for as long as you need before
you get to convert it to actually use the data from the binding.
Just mark the node as compatible with both the generic INTC binding
as well as the specific itnerrupt controller for one of the SoCs,
so you can use the existing code with the tables in the kernel,
and ignore all the duplicate data from the device tree (possibly
checking the data to make sure it matches).
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype
2012-03-28 10:39 [PATCH] ARM: mach-shmobile: sh7372 DT IRQ prototype Magnus Damm
` (7 preceding siblings ...)
2012-03-30 14:43 ` Arnd Bergmann
@ 2012-04-03 9:40 ` Magnus Damm
8 siblings, 0 replies; 10+ messages in thread
From: Magnus Damm @ 2012-04-03 9:40 UTC (permalink / raw)
To: linux-sh
On Fri, Mar 30, 2012 at 11:38 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 30 March 2012, Magnus Damm wrote:
>
>> >> However, the idea that you'd like to describe the controller with the
>> >> device tree is interesting. At this point I'm unsure how to do that.
>> >> Also, I do wonder if it makes sense to do so at all - most new parts
>> >> use the GIC as the main interrupt controller anyway. I do agree with
>> >> the plan of separating configuration from code, and in our INTC case
>> >> we've sort of already done so about 5 years ago. At that point I
>> >> converted a few separate random implementations to the current table
>> >> driven INTC code base. Since then we have soon close to 100 different
>> >> users in arch/sh and arch/arm/mach-shmobile. Each is different. Feel
>> >> free to grep for "register_intc_controller" to explore by yourself. It
>> >> would be fun to try to use the device tree for this, but I wonder how
>> >> we can describe anything half-complex without a preprocessor. Both the
>> >> INTC code and the PFC used for GPIO and pinmux use C enums a lot.
>> >
>> > Ah, so the vast majority of these are not for shmobile but for arch/sh
>> > and I support you have no plans to move those to DT along with shmobile,
>> > right?
>>
>> Most use cases are still on SH, but for mach-shmobile we make use of
>> either GIC of INTCA together with a cascaded INTCS. We also have IRQ
>> pins that can be controlled from either INTCA or INTCS or just
>> level/edge trigger configuration in front of GIC. In some cases we
>> have an interrupt controller called PINT. Some PFC/GPIO controllers
>> have built-in interrupt controllers, and some GPIOs have their
>> interrupts tied into the INTC as IRQ pins. There is a big jambalaya of
>> hardware IP with history from Mitsubishi, Hitachi and NEC, plus the
>> usual not-so-exotic ones.
>>
>> So with this in mind I believe we will need INTC support for
>> mach-shmobile in the future as well. The INTC controllers seem to
>> become more like leaf nodes these days though. =)
>
> Ok, I see
>
>> > From my reading of the source code, your INTCA and INTCS controllers
>> > all follow the same basic model, but they all differ in the mapping
>> > between interrupt vectors to register locations.
>>
>> All INTC tables are unique. So the INTCS in sh7372 is not the same as
>> the INTCS in sh7367. Some parts of the tables may look similar, but
>> low hanging fruit such as external IRQ pin handling has been broken
>> out into reusable components already.
>
> What I meant was that the table structure is always the same, even
> if the contents are are different. It's fairly easy to replace
> data from the kernel sources with data in the device tree, but
> replacing code from the kernel with data is much harder.
Right. =)
>> > I would probably not try to describe this mapping in the interrupt
>> > controller node, but instead use a more elaborate way to describe
>> > an interrupt than just the vector.
>>
>> That's certainly one way to split out the information, but I wonder if
>> that's the right way to go. I worry that with such setup the
>> correctness of the mappings between vectors and register fields is
>> dependent on the system integrator doing proper copy-paste
>> "development". One character wrong and you will have a silent error
>> somewhere with the wrong interrupt being masked by accident for
>> instance.
>
> All the bits describing the interrupt controller node specific
> to one soc should be able to go into the .dtsi file for that soc,
> so that a system integrator only needs to hook up the devices that
> are outside of the soc.
Sure, splitting data that way makes sense. That's how I'd like it to be.
I was more confused about the example of "interrupts = < vector, foo,
bar >;". I got the impression that this information was going to be
kept together with each device that makes use of interrupts, and we
would need such vector/foo/bar information also for interrupts used by
board specific devices. So the ethernet chip on our Mackerel board
would look something like this:
lan9220@14000000 {
compatible = "smsc,lan9220", "smsc,lan9115";
reg = <0x14000000 0x2000000>;
phy-mode = "mii";
interrupt-parent = <&intca>;
interrupts = <0x2c0, foo, bar>;
reg-io-width = <4>;
smsc,irq-push-pull;
};
I'm concerned that system integrators would have trouble copy-pasting
the above "interrupts" correctly. I'm sure there's some elegant
solution to this issue though.
>> > Taking the sh7372 USB0_USB0I0 interrupt as an arbitrary example,
>> > you could encode it as
>> >
>> > interrupts = <0x1ca0 /* vector */
>> > 0xa4 5 /* mask register offset and bit position */
>> > 0x4c 1>; /* prio register offset and bit position */
>> >
>> > An interrupt controller that only needs these (no sense/ack registers),
>> > would set #interrupt-cells=<5>;, while other interrupt controllers would
>> > use a larger number of interrupt cells to also encode the extra data.
>>
>> Thanks for showing this example. I think your proposal is interesting.
>> I believe I get the big picture, but I wonder if it is that simple
>> after all. In this specific example the USB0_USB0I0 interrupt source
>> also needs a 0xe4 associated with the mask register. Some mask
>> registers come in pairs with one clear and one set register. We also
>> have the width of the register and width of the priority field as
>> data. Of course the actual register mapping with such information
>> could be kept together with the INTC configuration data for the
>> interrupt controller, and we can for instance use register offset and
>> bit field index to locate the actual bit field or bit. This becomes
>> quite close to how INTC is working today though, with the exception
>> that we use enums with the same names as the data sheet. I don't see
>> how this can be done in Device Tree lingo.
>
> There are two places where you could put that information. The one
> I described above is the interrupts property, but if you have to
> add many more fields, this gets impractical.
I agree!
> The other place would be sub-nodes of the interrupt controller node
> that don't get translated into individual platform devices.
>
> You can have one node per interrupt line, like this:
>
> intc@6940000 {
> #address-cells = <2>;
> #size-cells = <0>;
> reg = <0x6940000 0x1000>;
> ranges = <1 0 0x6940000 0x54 /* prio */
> 2 0 0x6940080 0x3c /* mask */
> 3 0 0x69400c0 0x3c>; /* unmask /
>
> /* common properties for priority nodes,
> could get overridden if necessary */
> prio-reg-width = <16>;
> prio-width = <4>;
>
> iprta3 {
> #address-cells = <2>;
> #size-cells = <0>;
> ranges;
>
> reg = <1 0x4c> /* priority reg 0x4c */
>
> usb0_usb0i0 {
> reg = <0 0x1ca0 /* vector 0x1ca0 */
> 1 0x34 /* mask 0x80 + 0x34 */
> 2 0x34>; /* unmask 0xc0 + 0x34 */
> reg-mask = <0x04>; /* bitmask for registers */
> };
> };
> };
Thanks, that looks more like the self-contained INTC tables that we
represent in C today.
>> > I don't understand what your interrupt groups are, or whether you need
>> > even more attributes somewhere.
>>
>> The groups are used to link together multiple interrupt sources
>> (enums) that share a priority setting.
>>
>> Anyway, just to be clear, at this point i mainly wanted to enable DT
>> support on the mach-shmobile subarch and sh7372 in particular. So in
>> my mind this excludes reworking the INTC and PFC tables. Such
>> activities will take quite some time due to them being shared with SH.
>> Of course we should work in that direction, but at this point I 'd
>> like to focus on getting other people to work with DT support in the
>> drivers. So because of that I spent time on getting interrupts going
>> with the device tree instead of figuring out how to replace the INTC
>> tables with something else. Step by step.
>>
>> I assume that your current merge policy is that boards should be
>> implemented in DT, but we can still merge new SoCs with current style
>> of INTC and PFC tables as usual?
>
> Yes, that sounds ok. The part that I would not want to see though
> is having a preliminary binding for INTC that you later have to
> replace by another binding, breaking all device tree files that
> were produced at first.
Sure, but since we enable SoC one by one, can't we just start with
something simple for sh7372, and then decide on a per-SoC basis
depending on the state of device tree support in the INTC code versus
the complexity of the INTC hardware block?
So for instance on sh7372 we use just vector numbers as interrupt. In
parallel we can work towards representing the INTC tables as device
tree data. And when that is done, and we happen to have a SoC that can
implement support for INTC via just device tree only then we can use
the device tree for the INTC table information. So before we have INTC
table support via device tree our only choice is to do as sh7372. And
the same for the corner cases where we cannot use the device tree for
INTC tables - perhaps because it needs some special handling or it
uses something that doesn't have a proper device tree binding yet.
My point is that we can evolve forward for each new SoC. Replacing
bindings for a certain SoC sounds very bad.
> It's ok if you add more data to the intc nodes at a later
> point while incrementally moving stuff into dts files, but I think
> you should decide on how to represent an interrupt line in the devices
> using intc.
So you would like an interrupt line for "renesas,intca-sh7372" to be
the same as "renesas,intca-potential-new-soc"?
Thanks,
/ magnus
^ permalink raw reply [flat|nested] 10+ messages in thread