| Message ID | 1313716221-20136-5-git-send-email-horms@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote: > This is intended to make it easier to correctly order IRQs. > > As suggested by Guennadi Liakhovetski. > --- a/arch/arm/mach-shmobile/board-ag5evm.c > +++ b/arch/arm/mach-shmobile/board-ag5evm.c > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = { > .end = 0xee1000ff, > .flags = IORESOURCE_MEM, > }, > - [1] = { > + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { > .start = gic_spi(83), > .flags = IORESOURCE_IRQ, > }, > - [2] = { > + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { > .start = gic_spi(84), > .flags = IORESOURCE_IRQ, > }, > - [3] = { > + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { > .start = gic_spi(85), > .flags = IORESOURCE_IRQ, > }, Hm... So I know you guys have been discussing this back and forth, so I'm not sure if jumping in to this thread makes things any better. But anyhow, here are my opinions. Feel free to ignore them. =) First of all, having some kind of association with each IRQ is a good thing. I am however not convinced that using the index number of the platform device resource irq is the best option. Consider the case when someone modifies the SDHI resource in the code above to only include this interrupt: [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { .start = gic_spi(84), .flags = IORESOURCE_IRQ, }, From my point of view, the common sense for this would be that only the SDCARD interrupt would be enabled and the rest would be disabled since they are unspecified. However, with the current code the behavior would be something else, and since the index number of SDCARD is not matching it will be detected as CARD_DETECT. So isn't it really ugly to depend on the number of IRQs when they are supposed to be used as an index? I've been toying around with this driver for a few years now, and when I have a hard time creating correct platform data then it's _probably_ a sign that there must be better ways to implement this. I would propose just adding interrupts in struct resource [] as usual, and then have thee separate flags in the platform data for each interrupt type. If the number of IRQ bits set in the platform data flags doesn't match the number of interrupt resources then return error. If they match then simply go through each flag set in the platform data flags and assign next available interrupt resource. And if no flags are set then go for the combined interrupt handler for all available interrupt resources. That's what I would do at least. Any other ideas? Perhaps just keep an array of interrupt numbers in the platform data as the sh-sci driver does and use the fixed indexes there if non-zero? Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote: > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote: > > This is intended to make it easier to correctly order IRQs. > > > > As suggested by Guennadi Liakhovetski. > > > --- a/arch/arm/mach-shmobile/board-ag5evm.c > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = { > > .end = 0xee1000ff, > > .flags = IORESOURCE_MEM, > > }, > > - [1] = { > > + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { > > .start = gic_spi(83), > > .flags = IORESOURCE_IRQ, > > }, > > - [2] = { > > + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { > > .start = gic_spi(84), > > .flags = IORESOURCE_IRQ, > > }, > > - [3] = { > > + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { > > .start = gic_spi(85), > > .flags = IORESOURCE_IRQ, > > }, > > Hm... > > So I know you guys have been discussing this back and forth, so I'm > not sure if jumping in to this thread makes things any better. But > anyhow, here are my opinions. Feel free to ignore them. =) > > First of all, having some kind of association with each IRQ is a good > thing. I am however not convinced that using the index number of the > platform device resource irq is the best option. Consider the case > when someone modifies the SDHI resource in the code above to only > include this interrupt: > > [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { > .start = gic_spi(84), > .flags = IORESOURCE_IRQ, > }, > > >From my point of view, the common sense for this would be that only > the SDCARD interrupt would be enabled and the rest would be disabled > since they are unspecified. However, with the current code the > behavior would be something else, and since the index number of SDCARD > is not matching it will be detected as CARD_DETECT. > > So isn't it really ugly to depend on the number of IRQs when they are > supposed to be used as an index? I've been toying around with this > driver for a few years now, and when I have a hard time creating > correct platform data then it's _probably_ a sign that there must be > better ways to implement this. > > I would propose just adding interrupts in struct resource [] as usual, > and then have thee separate flags in the platform data for each > interrupt type. If the number of IRQ bits set in the platform data > flags doesn't match the number of interrupt resources then return > error. If they match then simply go through each flag set in the > platform data flags and assign next available interrupt resource. And > if no flags are set then go for the combined interrupt handler for all > available interrupt resources. > > That's what I would do at least. Any other ideas? Perhaps just keep an > array of interrupt numbers in the platform data as the sh-sci driver > does and use the fixed indexes there if non-zero? Hi Magnus, unfortunately during the course of the review of this series a number of changes have crept in which I regard as being tangential to the goal of the series - which is to introduce broken-out handlers (I have already introduce support for broken-out IRQ sources). One area where such changes have occurred is in the subtle altering of the list of IRQ sources that it is valid to supply (again, support for broken-out IRQ sources is not introduced by this series). With regards to your comment and example above. I believe that your understanding of my code is incorrect. The configuration you suggest will be rejected because CARD_DETECT is not supplied, not because of an index miss-match. It could be made acceptable within the current framework by simply loosening up the logic a little (specifically to allow CARD_DETECT to be omitted even if only one other IRQ source is supplied). Incidently, I think that would make sense but Guennadi specifically asked for that combination to be regarded as invalid. You suggestion to not rely on the array offset is an interesting one and perhaps it would make things easier (to understand). But I don't believe it would entirely eliminate the possibility of creating bogus platform data. And in any case I believe it is tangential to the aim of this series - to introduce broken-out handlers. Moreover, I think that there needs to be some agreement between Guennadi and yourself with regards to what IRQ combinations are valid before I (or anyone else) can implement something that is acceptable to you both. And I think that in the mean time the current implementation is reasonable given that it works with all current use-cases (and several non-existent ones too). In short, can we have this discussion in the context of further enhancements? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote: > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote: > > On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote: > >> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote: > >> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote: > >> > > This is intended to make it easier to correctly order IRQs. > >> > > > >> > > As suggested by Guennadi Liakhovetski. > >> > > >> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c > >> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c > >> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = { > >> > > .end = 0xee1000ff, > >> > > .flags = IORESOURCE_MEM, > >> > > }, > >> > > - [1] = { > >> > > + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { > >> > > .start = gic_spi(83), > >> > > .flags = IORESOURCE_IRQ, > >> > > }, > >> > > - [2] = { > >> > > + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { > >> > > .start = gic_spi(84), > >> > > .flags = IORESOURCE_IRQ, > >> > > }, > >> > > - [3] = { > >> > > + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { > >> > > .start = gic_spi(85), > >> > > .flags = IORESOURCE_IRQ, > >> > > }, > >> > > >> > Hm... > >> > > >> > So I know you guys have been discussing this back and forth, so I'm > >> > not sure if jumping in to this thread makes things any better. But > >> > anyhow, here are my opinions. Feel free to ignore them. =) > >> > > >> > First of all, having some kind of association with each IRQ is a good > >> > thing. I am however not convinced that using the index number of the > >> > platform device resource irq is the best option. Consider the case > >> > when someone modifies the SDHI resource in the code above to only > >> > include this interrupt: > >> > > >> > [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { > >> > .start = gic_spi(84), > >> > .flags = IORESOURCE_IRQ, > >> > }, > >> > > >> > >From my point of view, the common sense for this would be that only > >> > the SDCARD interrupt would be enabled and the rest would be disabled > >> > since they are unspecified. However, with the current code the > >> > behavior would be something else, and since the index number of SDCARD > >> > is not matching it will be detected as CARD_DETECT. > >> > > >> > So isn't it really ugly to depend on the number of IRQs when they are > >> > supposed to be used as an index? I've been toying around with this > >> > driver for a few years now, and when I have a hard time creating > >> > correct platform data then it's _probably_ a sign that there must be > >> > better ways to implement this. > >> > > >> > I would propose just adding interrupts in struct resource [] as usual, > >> > and then have thee separate flags in the platform data for each > >> > interrupt type. If the number of IRQ bits set in the platform data > >> > flags doesn't match the number of interrupt resources then return > >> > error. If they match then simply go through each flag set in the > >> > platform data flags and assign next available interrupt resource. And > >> > if no flags are set then go for the combined interrupt handler for all > >> > available interrupt resources. > >> > > >> > That's what I would do at least. Any other ideas? Perhaps just keep an > >> > array of interrupt numbers in the platform data as the sh-sci driver > >> > does and use the fixed indexes there if non-zero? > >> > >> Hi Magnus, > >> > >> unfortunately during the course of the review of this series a number > >> of changes have crept in which I regard as being tangential to the > >> goal of the series - which is to introduce broken-out handlers (I have > >> already introduce support for broken-out IRQ sources). > >> > >> One area where such changes have occurred is in the subtle altering of the > >> list of IRQ sources that it is valid to supply (again, support for > >> broken-out IRQ sources is not introduced by this series). > >> > >> With regards to your comment and example above. I believe that your > >> understanding of my code is incorrect. The configuration you suggest will > >> be rejected because CARD_DETECT is not supplied, not because of an index > >> miss-match. It could be made acceptable within the current framework by > >> simply loosening up the logic a little (specifically to allow CARD_DETECT > >> to be omitted even if only one other IRQ source is supplied). Incidently, > >> I think that would make sense but Guennadi specifically asked for that > >> combination to be regarded as invalid. > > > > [snip] > > > > After some discussion off-line I now realise that there is a problem > > with my implementation. Specifically that it assumes that > > platform_get_irq() takes into account the index of resource entries - > > it does not. > > Right, this is a thing that only bites you once. =) > > > As we are already on the slippery slope of allowing combinations > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement > > a variant of your flag idea. The variation being to use names instead > > because a) that allows the use of platform_get_irq_byname() and b) > > the flags bits seem to be full and not driver-specific. > > Great, platform_get_irq_byname() seems like a perfect match. > > May I suggest "hotplug", "data" and "sdio" as names? I don't care very > much about names except keeping them short and precise to prevent > errors that can only be caught during runtime. Earlier on in the life of this series Guennadi suggested the names "card_detect", "sdcard" and "sdio". While I am not particularly attached to those names the do seem reasonable and are already used consistently by this series. So I would prefer to use those names. > > And as are already on the slippery slope of allowing combinations > > if IRQ sources beyond 1 and 3 I intend to implement logic to allow > > any number of unique specific IRQ sources to be handled. In practice > > hardware for some of these combinations is unlikely to exist. But that > > doesn't seem to be a particularly good reason to add extra logic > > to disallow them in the driver - its up to platform data to specify > > something valid. > > Yes, I totally agree. Thanks a lot for your help! > > / magnus > -- > To unsubscribe from this list: send the line "unsubscribe linux-sh" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Aug 2011, Magnus Damm wrote: > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote: [snip] > > As we are already on the slippery slope of allowing combinations > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement > > a variant of your flag idea. The variation being to use names instead > > because a) that allows the use of platform_get_irq_byname() and b) > > the flags bits seem to be full and not driver-specific. > > Great, platform_get_irq_byname() seems like a perfect match. Indeed, good idea! This API is (unfortunately) so rarely used, that it was pretty much completely absent from my "active dictionary." > May I suggest "hotplug", "data" and "sdio" as names? I don't care very > much about names except keeping them short and precise to prevent > errors that can only be caught during runtime. I don't care about names all that much either, but as Simon suggested in his other mail, consistency is good. Besides, yes, I thought about "data" as a name for the main SDHI IRQ, but I decided it didn't reflect its function very well. Firstly, it is not only data, secondly, SDIO IRQs can also announce data availability, I think. The even more precise description of these two IRQs would be, perhaps, "synchronous" and "asynchronous," but that's probably not very intuitivy. So, I'll accept any names, that you two agree upon:-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Aug 2011, Simon Horman wrote: > On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote: > > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote: [snip] > > > As we are already on the slippery slope of allowing combinations > > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement > > > a variant of your flag idea. The variation being to use names instead > > > because a) that allows the use of platform_get_irq_byname() and b) > > > the flags bits seem to be full and not driver-specific. > > > > Great, platform_get_irq_byname() seems like a perfect match. > > > > May I suggest "hotplug", "data" and "sdio" as names? I don't care very > > much about names except keeping them short and precise to prevent > > errors that can only be caught during runtime. > > Earlier on in the life of this series Guennadi suggested > the names "card_detect", "sdcard" and "sdio". While I am > not particularly attached to those names the do seem > reasonable and are already used consistently by this series. > So I would prefer to use those names. Just one more thing I forgot to mention in the previous mail: using names also makes the transition simple: first patch all platforms with names (at least those with multiple IRQs, if your legacy fallback implementation will accept unnamed IRQs), and then patch sh_mobile_sdhi.c Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 19, 2011 at 09:52:17AM +0200, Guennadi Liakhovetski wrote: > On Fri, 19 Aug 2011, Simon Horman wrote: > > > On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote: > > > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote: > > [snip] > > > > > As we are already on the slippery slope of allowing combinations > > > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement > > > > a variant of your flag idea. The variation being to use names instead > > > > because a) that allows the use of platform_get_irq_byname() and b) > > > > the flags bits seem to be full and not driver-specific. > > > > > > Great, platform_get_irq_byname() seems like a perfect match. > > > > > > May I suggest "hotplug", "data" and "sdio" as names? I don't care very > > > much about names except keeping them short and precise to prevent > > > errors that can only be caught during runtime. > > > > Earlier on in the life of this series Guennadi suggested > > the names "card_detect", "sdcard" and "sdio". While I am > > not particularly attached to those names the do seem > > reasonable and are already used consistently by this series. > > So I would prefer to use those names. > > Just one more thing I forgot to mention in the previous mail: using names > also makes the transition simple: first patch all platforms with names (at > least those with multiple IRQs, if your legacy fallback implementation > will accept unnamed IRQs), and then patch sh_mobile_sdhi.c Yes, I agree that is a nice feature of using names. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c index ce5c251..0d543bb 100644 --- a/arch/arm/mach-shmobile/board-ag5evm.c +++ b/arch/arm/mach-shmobile/board-ag5evm.c @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = { .end = 0xee1000ff, .flags = IORESOURCE_MEM, }, - [1] = { + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { .start = gic_spi(83), .flags = IORESOURCE_IRQ, }, - [2] = { + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { .start = gic_spi(84), .flags = IORESOURCE_IRQ, }, - [3] = { + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { .start = gic_spi(85), .flags = IORESOURCE_IRQ, }, @@ -395,15 +395,15 @@ static struct resource sdhi1_resources[] = { .end = 0xee1200ff, .flags = IORESOURCE_MEM, }, - [1] = { + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { .start = gic_spi(87), .flags = IORESOURCE_IRQ, }, - [2] = { + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { .start = gic_spi(88), .flags = IORESOURCE_IRQ, }, - [3] = { + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { .start = gic_spi(89), .flags = IORESOURCE_IRQ, }, diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c index d41c01f..f9d3a93 100644 --- a/arch/arm/mach-shmobile/board-mackerel.c +++ b/arch/arm/mach-shmobile/board-mackerel.c @@ -1022,15 +1022,15 @@ static struct resource sdhi0_resources[] = { .end = 0xe68500ff, .flags = IORESOURCE_MEM, }, - [1] = { + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { .start = evt2irq(0x0e00) /* SDHI0_SDHI0I0 */, .flags = IORESOURCE_IRQ, }, - [2] = { + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { .start = evt2irq(0x0e20) /* SDHI0_SDHI0I1 */, .flags = IORESOURCE_IRQ, }, - [3] = { + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { .start = evt2irq(0x0e40) /* SDHI0_SDHI0I2 */, .flags = IORESOURCE_IRQ, }, @@ -1065,15 +1065,15 @@ static struct resource sdhi1_resources[] = { .end = 0xe68600ff, .flags = IORESOURCE_MEM, }, - [1] = { + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { .start = evt2irq(0x0e80), /* SDHI1_SDHI1I0 */ .flags = IORESOURCE_IRQ, }, - [2] = { + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { .start = evt2irq(0x0ea0), /* SDHI1_SDHI1I1 */ .flags = IORESOURCE_IRQ, }, - [3] = { + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { .start = evt2irq(0x0ec0), /* SDHI1_SDHI1I2 */ .flags = IORESOURCE_IRQ, }, @@ -1116,15 +1116,15 @@ static struct resource sdhi2_resources[] = { .end = 0xe68700ff, .flags = IORESOURCE_MEM, }, - [1] = { + [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = { .start = evt2irq(0x1200), /* SDHI2_SDHI2I0 */ .flags = IORESOURCE_IRQ, }, - [2] = { + [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = { .start = evt2irq(0x1220), /* SDHI2_SDHI2I1 */ .flags = IORESOURCE_IRQ, }, - [3] = { + [1 + SH_MOBILE_SDHI_IRQ_SDIO] = { .start = evt2irq(0x1240), /* SDHI2_SDHI2I2 */ .flags = IORESOURCE_IRQ, },
This is intended to make it easier to correctly order IRQs. As suggested by Guennadi Liakhovetski. Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Cc: Magnus Damm <magnus.damm@gmail.com> Cc: Paul Mundt <lethal@linux-sh.org> Signed-off-by: Simon Horman <horms@verge.net.au> --- Depends on "mmc: sdhi: Make use of per-source irq handlers" v4 * Update for corrected ordering of SH_MOBILE_SDHI_IRQ_SDCARD and SH_MOBILE_SDHI_IRQ_CARD_DETECT v2 * Initial release --- arch/arm/mach-shmobile/board-ag5evm.c | 12 ++++++------ arch/arm/mach-shmobile/board-mackerel.c | 18 +++++++++--------- 2 files changed, 15 insertions(+), 15 deletions(-)