| Message ID | 1389185477-507-1-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 08 January 2014 13:51:17 Thierry Reding wrote: > When devices are probed from the device tree, any interrupts that they > reference are resolved at device creation time. This causes problems if > the interrupt provider hasn't been registered yet at that time, which > results in the interrupt being set to 0. Thanks for looking at this problem, it has bothered a lot of people for a long time. I'm sorry I wasn't there for the discussion in November, but when it came up before, I suggested a different solution that apparently didn't get implemented. > Note that this patch is the easy way out to fix a large part of the > problems for now. A more proper solution for the long term would be to > transition drivers to an API that always resolves resources of any kind > (not only interrupts) at probe time. > > For some background and discussion on possible solutions, see: > > https://lkml.org/lkml/2013/11/22/520 I hope I read this thread correctly, sorry if I missed an important part. My idea was to add the code not in platform_get_irq() but add the resource in platform_drv_probe(), and just bail out with -EPROBE_DEFER there if necessary. We could then skip adding the resources at device creation time. Is this something you already plan to do later, or is there a reason it wouldn't work? In the meantime, I don't see anything with your patch, but it also wouldn't hurt to do it now if it solves all the problems. Arnd
On Wed, Jan 08, 2014 at 02:41:19PM +0100, Arnd Bergmann wrote: > On Wednesday 08 January 2014 13:51:17 Thierry Reding wrote: > > When devices are probed from the device tree, any interrupts that they > > reference are resolved at device creation time. This causes problems if > > the interrupt provider hasn't been registered yet at that time, which > > results in the interrupt being set to 0. > > Thanks for looking at this problem, it has bothered a lot of people > for a long time. I'm sorry I wasn't there for the discussion in November, > but when it came up before, I suggested a different solution that > apparently didn't get implemented. > > > Note that this patch is the easy way out to fix a large part of the > > problems for now. A more proper solution for the long term would be to > > transition drivers to an API that always resolves resources of any kind > > (not only interrupts) at probe time. > > > > For some background and discussion on possible solutions, see: > > > > https://lkml.org/lkml/2013/11/22/520 > > I hope I read this thread correctly, sorry if I missed an important > part. My idea was to add the code not in platform_get_irq() but add > the resource in platform_drv_probe(), and just bail out with > -EPROBE_DEFER there if necessary. One of the reasons we can't do that just yet is that we don't actually get back an accurate error code from the OF IRQ helpers. Therefore if we want to support deferred probing we either have to return -EPROBE_DEFER for all errors (which is a bad idea because some errors just can't be resolved by deferral) or we change the OF IRQ functions to propagate the error code properly so that we know exactly when -EPROBE_DEFER makes sense (i.e. the IRQ domain for an interrupt-parent hasn't been registered yet). Actually I posted a round of patches quite a while back that did exactly this for interrupts. The changes were somewhat involved because it means propagating an error code from fairly deep down in the OF code back to the various higher-level helpers. If you're interested, the last version of that is here: https://lkml.org/lkml/2013/9/18/216 Grant in particular seemed to be uncomfortable about how invasive the patch series is. Perhaps I should step back for a minute and provide some background on how the initial idea came about. This was first triggered by the IOMMU probe ordering issue that you may have heard about. One of the reasons to do this for interrupts was because they are an existing type of resource yet suffer from a very similar problem, so I wanted to solve the issue for interrupts and thereby get a better overview of what needed to be done for IOMMUs. The most logical place for this code to reside seemed to be in the probe path of platform devices (or I2C and other devices for that matter). The patch series introduced an of_platform_probe() function that's called from platform_drv_probe(). This would automatically give us deferred probe support provided that we could propagate the appropriate error code while at the same time giving us some flexibility to hook up other resource types (such as IOMMUs). One problem with the IOMMU patches is that they've received some strong pushback from both Greg and Grant, arguing that it doesn't belong in the core. If you want to read up on that, here's a link to the latest version: https://lkml.org/lkml/2013/12/12/74 Some things had been discussed in earlier iterations of that series, but this should give you the basic idea. It stands to reason that if they push back on the IOMMU variant of what is essentially the same thing, they will push back on the IRQ variant as well. One alternative I proposed was to, just as you suggested earlier, move the code into platform_drv_probe() or rather a function called from it. That proposal never got any replies, though. https://lkml.org/lkml/2013/12/14/39 > We could then skip adding the resources at device creation time. > Is this something you already plan to do later, or is there a reason > it wouldn't work? The current thread here suggests that it would be preferable not to have any static allocations at all, but rather introduce a new API to resolve things at probe time if necessary. I think the idea was to have a set of functions like: int device_get_irq(struct device *dev, unsigned int num); struct resource *device_get_mem_region(struct device *dev, unsigned int num); Or even possible the more generic: struct resource *device_get_resource(struct device *dev, unsigned int type, unsigned int num); If every driver used these functions, all resources could trivially be resolved at probe time. That solution is also the most invasive one of course, because it requires all existing drivers to be converted. At least the API would be all new and therefore the conversion could happen piecemeal. One downside of that approach is that, while it maps well to platform devices or generic devices that have some sort of firmware interface such as OF or ACPI, I don't see how it can be made to work with an I2C client that's registered from board setup code for example. Well, I suppose that problem could be solved by throwing another lookup table at it, just like we do for clocks, regulators, PWMs and GPIOs. The good thing about it would be more consistency between the various types of resources. Eventually I could imagine that we could even get rid of struct resource (or at least only use it for a single type of resource). But as I said, it'll take quite a bit of work to convert everything to that. > In the meantime, I don't see anything with your patch, but it also > wouldn't hurt to do it now if it solves all the problems. Well, the commit message explicitly states that this is only a temporary measure, mostly to fix a number of regressions on OMAP where things were broken by the conversion to DT in 3.13. The same is probably true of other boards as well. I'm willing to help fix things properly in the long run, but I think a simple and low-risk patch like this would be worthwhile if it means that a good many boards aren't broken in 3.13. Also given the history of the above I can imagine that it will take more than the 3.14 cycle to get this resolved satisfactorily, so at least for interrupts this would give us a good stopgap solution in the meantime. Thierry
On Wednesday 08 January 2014 15:55:27 Thierry Reding wrote: > On Wed, Jan 08, 2014 at 02:41:19PM +0100, Arnd Bergmann wrote: > > On Wednesday 08 January 2014 13:51:17 Thierry Reding wrote: > > I hope I read this thread correctly, sorry if I missed an important > > part. My idea was to add the code not in platform_get_irq() but add > > the resource in platform_drv_probe(), and just bail out with > > -EPROBE_DEFER there if necessary. > > One of the reasons we can't do that just yet is that we don't actually > get back an accurate error code from the OF IRQ helpers. Therefore if we > want to support deferred probing we either have to return -EPROBE_DEFER > for all errors (which is a bad idea because some errors just can't be > resolved by deferral) or we change the OF IRQ functions to propagate the > error code properly so that we know exactly when -EPROBE_DEFER makes > sense (i.e. the IRQ domain for an interrupt-parent hasn't been > registered yet). I see. > Actually I posted a round of patches quite a while back that did exactly > this for interrupts. The changes were somewhat involved because it means > propagating an error code from fairly deep down in the OF code back to > the various higher-level helpers. If you're interested, the last version > of that is here: > > https://lkml.org/lkml/2013/9/18/216 > > Grant in particular seemed to be uncomfortable about how invasive the > patch series is. Interesting. It seems like a worthwhile thing to do, but I can understand Grant's reluctance. > > One problem with the IOMMU patches is that they've received some strong > pushback from both Greg and Grant, arguing that it doesn't belong in the > core. If you want to read up on that, here's a link to the latest > version: > > https://lkml.org/lkml/2013/12/12/74 > > Some things had been discussed in earlier iterations of that series, but > this should give you the basic idea. I'm skipping that discussion for now and stick with your summary > It stands to reason that if they push back on the IOMMU variant of what > is essentially the same thing, they will push back on the IRQ variant as > well. One alternative I proposed was to, just as you suggested earlier, > move the code into platform_drv_probe() or rather a function called from > it. That proposal never got any replies, though. > > https://lkml.org/lkml/2013/12/14/39 I guess putting it into the platform_drv_probe function seems reasonable, I would be more scared of the implications of a notifier based method. > > We could then skip adding the resources at device creation time. > > Is this something you already plan to do later, or is there a reason > > it wouldn't work? > > The current thread here suggests that it would be preferable not to have > any static allocations at all, but rather introduce a new API to resolve > things at probe time if necessary. I think the idea was to have a set of > functions like: > > int device_get_irq(struct device *dev, unsigned int num); > struct resource *device_get_mem_region(struct device *dev, > unsigned int num); > > Or even possible the more generic: > > struct resource *device_get_resource(struct device *dev, > unsigned int type, > unsigned int num); > > If every driver used these functions, all resources could trivially be > resolved at probe time. That solution is also the most invasive one of > course, because it requires all existing drivers to be converted. At > least the API would be all new and therefore the conversion could happen > piecemeal. Right, that does sound nice, and has the added benefit of saving some memory allocations. I'd prefer the less generic variant here, but I haven't given it much thought. > One downside of that approach is that, while it maps well to platform > devices or generic devices that have some sort of firmware interface > such as OF or ACPI, I don't see how it can be made to work with an I2C > client that's registered from board setup code for example. Well, I > suppose that problem could be solved by throwing another lookup table at > it, just like we do for clocks, regulators, PWMs and GPIOs. Wouldn't you still be able to attach resources in the traditional way for those, but use the same new interface to get at them? > The good thing about it would be more consistency between the various > types of resources. Eventually I could imagine that we could even get > rid of struct resource (or at least only use it for a single type of > resource). But as I said, it'll take quite a bit of work to convert > everything to that. struct resource is a structure with a long and complex history. I'd certainly like to put some of it behind us and do something that fits better into the 'struct device' concept which it predates. I agree it would be a big effort though. > > In the meantime, I don't see anything with your patch, but it also > > wouldn't hurt to do it now if it solves all the problems. > > Well, the commit message explicitly states that this is only a temporary > measure, mostly to fix a number of regressions on OMAP where things were > broken by the conversion to DT in 3.13. The same is probably true of > other boards as well. Right. > I'm willing to help fix things properly in the long run, but I think a > simple and low-risk patch like this would be worthwhile if it means that > a good many boards aren't broken in 3.13. Also given the history of the > above I can imagine that it will take more than the 3.14 cycle to get > this resolved satisfactorily, so at least for interrupts this would give > us a good stopgap solution in the meantime. Ok. Thanks so much for your detailed background information! Arnd
On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote: > On Wednesday 08 January 2014 15:55:27 Thierry Reding wrote: > > On Wed, Jan 08, 2014 at 02:41:19PM +0100, Arnd Bergmann wrote: > > > On Wednesday 08 January 2014 13:51:17 Thierry Reding wrote: [...] > > Actually I posted a round of patches quite a while back that did exactly > > this for interrupts. The changes were somewhat involved because it means > > propagating an error code from fairly deep down in the OF code back to > > the various higher-level helpers. If you're interested, the last version > > of that is here: > > > > https://lkml.org/lkml/2013/9/18/216 > > > > Grant in particular seemed to be uncomfortable about how invasive the > > patch series is. > > Interesting. It seems like a worthwhile thing to do, but I can understand > Grant's reluctance. To be fair, Grant didn't say outright no. Given how easily this could turn into a regression nightmare I do understand the reluctance as well. Merging things piece by piece would make it somewhat less risky but at the same time makes it hard to keep at it. > > It stands to reason that if they push back on the IOMMU variant of what > > is essentially the same thing, they will push back on the IRQ variant as > > well. One alternative I proposed was to, just as you suggested earlier, > > move the code into platform_drv_probe() or rather a function called from > > it. That proposal never got any replies, though. > > > > https://lkml.org/lkml/2013/12/14/39 > > I guess putting it into the platform_drv_probe function seems reasonable, > I would be more scared of the implications of a notifier based method. I fully agree. Of course if we decide against moving things into the core and in favour of a more generic API that drivers should use, then this issue goes away silently at least for resources that the driver needs to use explicitly (memory-mapped regions, interrupts, ...). The issue remains for IOMMU which is meant to be used transparently through the DMA API. Perhaps a good compromise would be to have some sort of generic helper that can be called to initialize IOMMU support for a particular device and support probe deferral on error. Something like this perhaps: int iommu_attach(struct device *dev); int iommu_detach(struct device *dev); I still don't like very much how that needs to be done in each driver explicitly, but if we can't do it in the core, then the only other clean way to handle it would be to treat it like any other sort of resource and handle it explicitly. Perhaps handing out some sort of cookie would be preferable to just an error code? > > > We could then skip adding the resources at device creation time. > > > Is this something you already plan to do later, or is there a reason > > > it wouldn't work? > > > > The current thread here suggests that it would be preferable not to have > > any static allocations at all, but rather introduce a new API to resolve > > things at probe time if necessary. I think the idea was to have a set of > > functions like: > > > > int device_get_irq(struct device *dev, unsigned int num); > > struct resource *device_get_mem_region(struct device *dev, > > unsigned int num); > > > > Or even possible the more generic: > > > > struct resource *device_get_resource(struct device *dev, > > unsigned int type, > > unsigned int num); > > > > If every driver used these functions, all resources could trivially be > > resolved at probe time. That solution is also the most invasive one of > > course, because it requires all existing drivers to be converted. At > > least the API would be all new and therefore the conversion could happen > > piecemeal. > > Right, that does sound nice, and has the added benefit of saving > some memory allocations. I'd prefer the less generic variant here, > but I haven't given it much thought. I do prefer the less generic ones as well. They seem to be more convenient to use. > > One downside of that approach is that, while it maps well to platform > > devices or generic devices that have some sort of firmware interface > > such as OF or ACPI, I don't see how it can be made to work with an I2C > > client that's registered from board setup code for example. Well, I > > suppose that problem could be solved by throwing another lookup table at > > it, just like we do for clocks, regulators, PWMs and GPIOs. > > Wouldn't you still be able to attach resources in the traditional > way for those, but use the same new interface to get at them? I wouldn't know how. For instance platform devices store the IRQ number within a struct resource of type IORESOURCE_IRQ, whereas I2C clients store them in the struct i2c_client's .irq field. So without actually introspecting the struct device (possibly using the .bus field for example) and upcasting you won't know how to get at the resources. One possibility to remedy that would be to try and unify the resources within struct device. But that doesn't feel right. One other thing I had considered at one point was to extend the bus_type structure and give it a way to obtain resources in a bus-specific way, but that feel even more wrong. Perhaps I'm missing something obvious, though, and this is actually much more trivial to solve. > > The good thing about it would be more consistency between the various > > types of resources. Eventually I could imagine that we could even get > > rid of struct resource (or at least only use it for a single type of > > resource). But as I said, it'll take quite a bit of work to convert > > everything to that. > > struct resource is a structure with a long and complex history. > I'd certainly like to put some of it behind us and do something > that fits better into the 'struct device' concept which it > predates. I agree it would be a big effort though. I concur. Thierry
On Wednesday 08 January 2014, Thierry Reding wrote: > On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote: > > On Wednesday 08 January 2014 15:55:27 Thierry Reding wrote: > > > It stands to reason that if they push back on the IOMMU variant of what > > > is essentially the same thing, they will push back on the IRQ variant as > > > well. One alternative I proposed was to, just as you suggested earlier, > > > move the code into platform_drv_probe() or rather a function called from > > > it. That proposal never got any replies, though. > > > > > > https://lkml.org/lkml/2013/12/14/39 > > > > I guess putting it into the platform_drv_probe function seems reasonable, > > I would be more scared of the implications of a notifier based method. > > I fully agree. Of course if we decide against moving things into the > core and in favour of a more generic API that drivers should use, then > this issue goes away silently at least for resources that the driver > needs to use explicitly (memory-mapped regions, interrupts, ...). > > The issue remains for IOMMU which is meant to be used transparently > through the DMA API. Perhaps a good compromise would be to have some > sort of generic helper that can be called to initialize IOMMU support > for a particular device and support probe deferral on error. Something > like this perhaps: > > int iommu_attach(struct device *dev); > int iommu_detach(struct device *dev); > > I still don't like very much how that needs to be done in each driver > explicitly, but if we can't do it in the core, then the only other clean > way to handle it would be to treat it like any other sort of resource > and handle it explicitly. Perhaps handing out some sort of cookie would > be preferable to just an error code? The more I think about the iommu case, the more I am convinced that it does belong into the core, in whatever form we can find. As far as I can tell from the little reliable information I have on the topic, I would assume that we can keep it in the DT probing code, as there won't be a need for multiple arbitrary IOMMUs with ACPI or with board files. > > > One downside of that approach is that, while it maps well to platform > > > devices or generic devices that have some sort of firmware interface > > > such as OF or ACPI, I don't see how it can be made to work with an I2C > > > client that's registered from board setup code for example. Well, I > > > suppose that problem could be solved by throwing another lookup table at > > > it, just like we do for clocks, regulators, PWMs and GPIOs. > > > > Wouldn't you still be able to attach resources in the traditional > > way for those, but use the same new interface to get at them? > > I wouldn't know how. For instance platform devices store the IRQ number > within a struct resource of type IORESOURCE_IRQ, whereas I2C clients > store them in the struct i2c_client's .irq field. Good point, I forgot about the special case for i2c_client->irq. I looked now and noticed that very few i2c devices actually use this field, but larger number uses platform_data, which has a similar problem. > So without actually introspecting the struct device (possibly using the > .bus field for example) and upcasting you won't know how to get at the > resources. One possibility to remedy that would be to try and unify the > resources within struct device. But that doesn't feel right. > > One other thing I had considered at one point was to extend the bus_type > structure and give it a way to obtain resources in a bus-specific way, > but that feel even more wrong. > > Perhaps I'm missing something obvious, though, and this is actually much > more trivial to solve. No trivial solution that I can see. I think we can deal with the case where platform code uses platform_device->resources, and everything else comes down to having multiple code branches in the driver, as we already have to deal with platform_data and DT properties describing stuff that doesn't fit in the resources. Arnd
On Wed, Jan 08, 2014 at 05:25:17PM +0100, Arnd Bergmann wrote: > On Wednesday 08 January 2014, Thierry Reding wrote: > > On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote: > > > On Wednesday 08 January 2014 15:55:27 Thierry Reding wrote: > > > > It stands to reason that if they push back on the IOMMU variant of what > > > > is essentially the same thing, they will push back on the IRQ variant as > > > > well. One alternative I proposed was to, just as you suggested earlier, > > > > move the code into platform_drv_probe() or rather a function called from > > > > it. That proposal never got any replies, though. > > > > > > > > https://lkml.org/lkml/2013/12/14/39 > > > > > > I guess putting it into the platform_drv_probe function seems reasonable, > > > I would be more scared of the implications of a notifier based method. > > > > I fully agree. Of course if we decide against moving things into the > > core and in favour of a more generic API that drivers should use, then > > this issue goes away silently at least for resources that the driver > > needs to use explicitly (memory-mapped regions, interrupts, ...). > > > > The issue remains for IOMMU which is meant to be used transparently > > through the DMA API. Perhaps a good compromise would be to have some > > sort of generic helper that can be called to initialize IOMMU support > > for a particular device and support probe deferral on error. Something > > like this perhaps: > > > > int iommu_attach(struct device *dev); > > int iommu_detach(struct device *dev); > > > > I still don't like very much how that needs to be done in each driver > > explicitly, but if we can't do it in the core, then the only other clean > > way to handle it would be to treat it like any other sort of resource > > and handle it explicitly. Perhaps handing out some sort of cookie would > > be preferable to just an error code? > > The more I think about the iommu case, the more I am convinced that it > does belong into the core, in whatever form we can find. As far as I > can tell from the little reliable information I have on the topic, I > would assume that we can keep it in the DT probing code, as there won't > be a need for multiple arbitrary IOMMUs with ACPI or with board files. I think part of the problem is that we don't have any DT probing code yet. of_platform_probe() would have been that code. Perhaps if you weigh in Grant and Greg will reconsider. > > > > One downside of that approach is that, while it maps well to platform > > > > devices or generic devices that have some sort of firmware interface > > > > such as OF or ACPI, I don't see how it can be made to work with an I2C > > > > client that's registered from board setup code for example. Well, I > > > > suppose that problem could be solved by throwing another lookup table at > > > > it, just like we do for clocks, regulators, PWMs and GPIOs. > > > > > > Wouldn't you still be able to attach resources in the traditional > > > way for those, but use the same new interface to get at them? > > > > I wouldn't know how. For instance platform devices store the IRQ number > > within a struct resource of type IORESOURCE_IRQ, whereas I2C clients > > store them in the struct i2c_client's .irq field. > > Good point, I forgot about the special case for i2c_client->irq. > I looked now and noticed that very few i2c devices actually use this > field, but larger number uses platform_data, which has a similar > problem. Yeah. It's kind of messy. The more I think about it, the more I begin to like the lookup table option. One big advantage of that is that we could actually unify much of the lookup code, much like we do for other types of resources. It's a pattern that has worked fairly well in a number of cases, so it seems natural to use it for interrupts as well. An even more extreme option would be to go all the way and introduce struct irq, along the same lines of the new struct gpiod. That would allow nice things such as storing trigger types and such within the IRQ handle and propagate them automatically. > > So without actually introspecting the struct device (possibly using the > > .bus field for example) and upcasting you won't know how to get at the > > resources. One possibility to remedy that would be to try and unify the > > resources within struct device. But that doesn't feel right. > > > > One other thing I had considered at one point was to extend the bus_type > > structure and give it a way to obtain resources in a bus-specific way, > > but that feel even more wrong. > > > > Perhaps I'm missing something obvious, though, and this is actually much > > more trivial to solve. > > No trivial solution that I can see. I think we can deal with the case > where platform code uses platform_device->resources, and everything else > comes down to having multiple code branches in the driver, as we already > have to deal with platform_data and DT properties describing stuff that > doesn't fit in the resources. That would be another argument in favour of the lookup table. It would provide a unified mechanism to define static interrupts if no firmware interface is available *independent* of the device type. You could have something like: static const struct irq_lookup board_irq_lookup[] = { IRQ_LOOKUP("gpio", 0, "0-0040", NULL), /* I2C client via GPIO expander */ IRQ_LOOKUP("intc", 0, "ehci.1", NULL), /* platform device via INTC */ }; Along with: struct irq *irq_get(struct device *dev, const char *con_id); To look it up via DT, ACPI, lookup table. That obviously means a more or less complete change in how interrupts are handled in the kernel, and it may not be worth it in the end. Thierry
On Wednesday 08 January 2014 20:59:10 Thierry Reding wrote: > On Wed, Jan 08, 2014 at 05:25:17PM +0100, Arnd Bergmann wrote: > > On Wednesday 08 January 2014, Thierry Reding wrote: > > > On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote: > > The more I think about the iommu case, the more I am convinced that it > > does belong into the core, in whatever form we can find. As far as I > > can tell from the little reliable information I have on the topic, I > > would assume that we can keep it in the DT probing code, as there won't > > be a need for multiple arbitrary IOMMUs with ACPI or with board files. > > I think part of the problem is that we don't have any DT probing code > yet. of_platform_probe() would have been that code. Perhaps if you weigh > in Grant and Greg will reconsider. For all I know, we don't even have a binding proposal, but I may have missed that. > > Good point, I forgot about the special case for i2c_client->irq. > > I looked now and noticed that very few i2c devices actually use this > > field, but larger number uses platform_data, which has a similar > > problem. > > Yeah. It's kind of messy. The more I think about it, the more I begin to > like the lookup table option. One big advantage of that is that we could > actually unify much of the lookup code, much like we do for other types > of resources. It's a pattern that has worked fairly well in a number of > cases, so it seems natural to use it for interrupts as well. > > An even more extreme option would be to go all the way and introduce > struct irq, along the same lines of the new struct gpiod. That would > allow nice things such as storing trigger types and such within the IRQ > handle and propagate them automatically. We already have struct irq_desc, and I believe everybody who works with interrupts supports migrating from irq number interfaces to irq descriptors as soon as we can find someone willing to do that work. > > No trivial solution that I can see. I think we can deal with the case > > where platform code uses platform_device->resources, and everything else > > comes down to having multiple code branches in the driver, as we already > > have to deal with platform_data and DT properties describing stuff that > > doesn't fit in the resources. > > That would be another argument in favour of the lookup table. It would > provide a unified mechanism to define static interrupts if no firmware > interface is available *independent* of the device type. You could have > something like: > > static const struct irq_lookup board_irq_lookup[] = { > IRQ_LOOKUP("gpio", 0, "0-0040", NULL), /* I2C client via GPIO expander */ > IRQ_LOOKUP("intc", 0, "ehci.1", NULL), /* platform device via INTC */ > }; > > Along with: > > struct irq *irq_get(struct device *dev, const char *con_id); > > To look it up via DT, ACPI, lookup table. That obviously means a more or > less complete change in how interrupts are handled in the kernel, and it > may not be worth it in the end. It would certainly need a long migration period, and a plan for how to get there without breaking things in the meantime. Rather than a lookup table like the kind we have for clocks, I'd prefer a general way to attach named properties to devices. My feeling is that building upon devres would be a good plan, since it already provides a way to attach arbitrary data to a device. Arnd
On Wed, Jan 08, 2014 at 09:09:17PM +0100, Arnd Bergmann wrote: > On Wednesday 08 January 2014 20:59:10 Thierry Reding wrote: > > On Wed, Jan 08, 2014 at 05:25:17PM +0100, Arnd Bergmann wrote: > > > On Wednesday 08 January 2014, Thierry Reding wrote: > > > > On Wed, Jan 08, 2014 at 04:11:08PM +0100, Arnd Bergmann wrote: > > > The more I think about the iommu case, the more I am convinced that it > > > does belong into the core, in whatever form we can find. As far as I > > > can tell from the little reliable information I have on the topic, I > > > would assume that we can keep it in the DT probing code, as there won't > > > be a need for multiple arbitrary IOMMUs with ACPI or with board files. > > > > I think part of the problem is that we don't have any DT probing code > > yet. of_platform_probe() would have been that code. Perhaps if you weigh > > in Grant and Greg will reconsider. > > For all I know, we don't even have a binding proposal, but I may > have missed that. Yeah, last time I checked there was no concensus on that. I'll try to dig up the thread and get it going again, adding you on Cc if you don't mind (and in case you weren't Cc'ed in the first place anyway). > > > Good point, I forgot about the special case for i2c_client->irq. > > > I looked now and noticed that very few i2c devices actually use this > > > field, but larger number uses platform_data, which has a similar > > > problem. > > > > Yeah. It's kind of messy. The more I think about it, the more I begin to > > like the lookup table option. One big advantage of that is that we could > > actually unify much of the lookup code, much like we do for other types > > of resources. It's a pattern that has worked fairly well in a number of > > cases, so it seems natural to use it for interrupts as well. > > > > An even more extreme option would be to go all the way and introduce > > struct irq, along the same lines of the new struct gpiod. That would > > allow nice things such as storing trigger types and such within the IRQ > > handle and propagate them automatically. > > We already have struct irq_desc, and I believe everybody who works > with interrupts supports migrating from irq number interfaces to > irq descriptors as soon as we can find someone willing to do that > work. I didn't know that. I had always assumed irq_desc was only for internal use by the IRQ code. Perhaps I'll look into that at some point. > > > No trivial solution that I can see. I think we can deal with the case > > > where platform code uses platform_device->resources, and everything else > > > comes down to having multiple code branches in the driver, as we already > > > have to deal with platform_data and DT properties describing stuff that > > > doesn't fit in the resources. > > > > That would be another argument in favour of the lookup table. It would > > provide a unified mechanism to define static interrupts if no firmware > > interface is available *independent* of the device type. You could have > > something like: > > > > static const struct irq_lookup board_irq_lookup[] = { > > IRQ_LOOKUP("gpio", 0, "0-0040", NULL), /* I2C client via GPIO expander */ > > IRQ_LOOKUP("intc", 0, "ehci.1", NULL), /* platform device via INTC */ > > }; > > > > Along with: > > > > struct irq *irq_get(struct device *dev, const char *con_id); > > > > To look it up via DT, ACPI, lookup table. That obviously means a more or > > less complete change in how interrupts are handled in the kernel, and it > > may not be worth it in the end. > > It would certainly need a long migration period, and a plan for how to > get there without breaking things in the meantime. Rather than a lookup > table like the kind we have for clocks, I'd prefer a general way to > attach named properties to devices. My feeling is that building upon > devres would be a good plan, since it already provides a way to attach > arbitrary data to a device. The problem with devres, or any other solution for that matter, is that for the cases where we'd need something like this (that is, statically allocated devices in board setup code) we don't have a fully initialized struct device. We need at least device_initialize() to have been called before devres can be used. Therefore we still need some sort of lookup table that can be used to seed devres objects. Also devres will go away completely when a driver is unloaded, so it'll have to be repopulated everytime. I don't think that would buy us much over a simple table lookup. Thierry
On Wednesday 08 January 2014 21:24:08 Thierry Reding wrote: > > The problem with devres, or any other solution for that matter, is that > for the cases where we'd need something like this (that is, statically > allocated devices in board setup code) we don't have a fully initialized > struct device. We need at least device_initialize() to have been called > before devres can be used. Therefore we still need some sort of lookup > table that can be used to seed devres objects. Also devres will go away > completely when a driver is unloaded, so it'll have to be repopulated > everytime. > > I don't think that would buy us much over a simple table lookup. I would think we can come up with a way to add data to a device that ends up in devres by the time the device gets registered. The question is more whether you want a global table (or a set of global tables for that matter) or rather have all the data local to the devices you register. I generally prefer the latter. There is an interesting question about what subsystems you'd want to include in this mechanism. We have a growing number of subsystems that want data represented in DT (clock, regulator, dmaengine, reset, led, pwm, irq, iommu, ...), most of which don't have a 'struct resource' equivalent. We could either try to create something generic enough to easily cover all of them, or we declare that if you actually want to use all of them you should really use DT, and we make it hard to add another subsystem specific lookup mechanism. Arnd
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 3a94b799f166..c894d1af3a5e 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -13,6 +13,7 @@ #include <linux/string.h> #include <linux/platform_device.h> #include <linux/of_device.h> +#include <linux/of_irq.h> #include <linux/module.h> #include <linux/init.h> #include <linux/dma-mapping.h> @@ -87,7 +88,12 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) return -ENXIO; return dev->archdata.irqs[num]; #else - struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num); + struct resource *r; + + if (IS_ENABLED(CONFIG_OF) && dev->dev.of_node) + return irq_of_parse_and_map(dev->dev.of_node, num); + + r = platform_get_resource(dev, IORESOURCE_IRQ, num); return r ? r->start : -ENXIO; #endif