On Thu, Apr 14, 2011 at 11:41:46AM +0200, Michal Simek wrote: > Support OF support. "generic-uio" compatible property is used.
This description is not true anymore. Please also add a short paragrpah how it is intended to be used now.
> Signed-off-by: Michal Simek <mon...@monstr.eu> > + uioinfo->version = "dt";
Minor nit: "devicetree" is probably more descriptive.
> + > + /* Multiple IRQs are not supported */ > + if (pdev->num_resources > 1) { > + struct resource *r = &pdev->resource[1];
Are you sure the irq-ressource is always [1]? (Similar question for the if()-block above). Try platform_get_irq().
> +#ifdef CONFIG_OF > +/* > + * Empty match table for of_platform binding
While it probably doesn't make change to put every supported device in upstream, it still deosn't technically have to be empty. So, mabye drop this comment and add something like "/* empty for now */" to the table?
> Minor nit: "devicetree" is probably more descriptive.
no problem to change it
>> + >> + /* Multiple IRQs are not supported */ >> + if (pdev->num_resources > 1) { >> + struct resource *r = &pdev->resource[1];
> Are you sure the irq-ressource is always [1]? (Similar question for the > if()-block above). Try platform_get_irq().
I wasn't aware about platform_get_irq. You are right.
What "if()-block above" are you talking about? Above is kzalloc uioinfo allocation. I am going to send v3 that's why please comment this there.
>> +#ifdef CONFIG_OF >> +/* >> + * Empty match table for of_platform binding
> While it probably doesn't make change to put every supported device in > upstream, it still deosn't technically have to be empty. So, mabye drop this > comment and add something like "/* empty for now */" to the table?
Perhaps a silly question, but how are you planning on differentiating between uio_pdrv and uio_pdrv_genirq binding if someone has both enabled?
uio_pdrv obviously doesn't have OF bindings at the moment, but it seems like you could easily parse the memory ranges in addition to the IRQ and come up with a generic binding that would work for both.
We also have a shiny new Documentation/devicetree these days, so it would be nice to see the binding documented at the same time. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Paul Mundt wrote: > On Mon, Apr 18, 2011 at 10:50:54AM +0200, Michal Simek wrote: >> Adding OF binding to genirq. >> Version string is setup to the "devicetree".
>> Compatible string is not setup for now but you can add your >> custom compatible string to uio_of_genirq_match structure.
>> For example with "uio" compatible string: >> static const struct of_device_id __devinitconst uio_of_genirq_match[] = { >> { .compatible = "uio", }, >> { /* empty for now */ }, >> };
> Perhaps a silly question, but how are you planning on differentiating > between uio_pdrv and uio_pdrv_genirq binding if someone has both enabled?
It is not a silly question. OF support in uio_pdrv_genirq can handle both cases with/without IRQ (I tested it) and I don't want to add OF support to uio_pdrv. Not sure if uio_pdrv_genirq without OF can handle UIO without IRQ.
> uio_pdrv obviously doesn't have OF bindings at the moment, but it seems > like you could easily parse the memory ranges in addition to the IRQ and > come up with a generic binding that would work for both.
I think the question is if uio_pdrv_genirq can handle both cases, if yes, we can completely remove uio_pdrv. But it is up to UIO maintainers.
> We also have a shiny new Documentation/devicetree these days, so it would > be nice to see the binding documented at the same time.
Come to think of it, the driver so far does not print any dev_info messages, only err and warn. So, I'd think these messages should go, too, but this is up to Hans.
On Tue, Apr 19, 2011 at 2:06 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: > Hi,
>> For example with "uio" compatible string: >> static const struct of_device_id __devinitconst uio_of_genirq_match[] = { >> { .compatible = "uio", }, >> { /* empty for now */ }, >> };
> Please use a proper example with "vendor,device". > (And after that it won't be empty anymore)
My vote is, and always has been 'generic-uio' :)
Putting some random vendor/device string in there is just nuts. Do you really want a kernel patch every time some one binds their device to it?
Or, is there no expectation that anybody would attempt to merge such a pointless patch to begin with?
As we discussed at ELC, putting a real vendor/device in there is also broken because all instances in the system wil bind to the generic uio, which is not necessarily what is desired.
I know the arguments against the 'generic-uio' tag, but come on, let's look at the lesser of two evils here! I call BS on this DTS purity.
On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote: > On Tue, Apr 19, 2011 at 2:06 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: > > Hi,
> >> For example with "uio" compatible string: > >> static const struct of_device_id __devinitconst uio_of_genirq_match[] = { > >> { .compatible = "uio", }, > >> { /* empty for now */ }, > >> };
> > Please use a proper example with "vendor,device". > > (And after that it won't be empty anymore)
> My vote is, and always has been 'generic-uio' :)
> Putting some random vendor/device string in there is just nuts. Do you > really want a kernel patch every time some one binds their device to > it?
> Or, is there no expectation that anybody would attempt to merge such a > pointless patch to begin with?
> As we discussed at ELC, putting a real vendor/device in there is also > broken because all instances in the system wil bind to the generic > uio, which is not necessarily what is desired.
> I know the arguments against the 'generic-uio' tag, but come on, let's > look at the lesser of two evils here! I call BS on this DTS purity.
Call it what you like, but the reasons are well founded. The alternative that has been proposed which I am in agreement with is to investigate giving userspace the hook to tell the kernel at runtime which devices should be picked up by the uio driver.
In the mean time, explicitly modifying the match table is an okay compromise.
> On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote:
> > I know the arguments against the 'generic-uio' tag, but come on, let's > > look at the lesser of two evils here! I call BS on this DTS purity.
Both a specific device ID and something like "generic-uio" are equally broken:
If you have generic-uio, it is impossible to write an in-kernel driver for the same hardware without changing the device tree, meaning that it is impossible to correctly describe the hardware in the device tree.
If you put a meaningful identifier into the match table, it is also impossible to have an in-kernel driver for the hardware, because now you have no way to choose whether to handle the device with UIO or an in-kernel driver.
There may be cases where you have two instances of the same device in a machine and want one of them to be driven by UIO and the other by another driver. A common example of this would be a virtual machine where one device is passed through to the guest and the other is used by the host. I've done this for USB input devices and PCI network interfaces.
> Call it what you like, but the reasons are well founded. The alternative > that has been proposed which I am in agreement with is to investigate > giving userspace the hook to tell the kernel at runtime which devices > should be picked up by the uio driver.
Yes, I believe this is the best option.
> In the mean time, explicitly modifying the match table is an okay > compromise.
> Come to think of it, the driver so far does not print any dev_info messages, > only err and warn. So, I'd think these messages should go, too, but this is up > to Hans.
I used that because I wanted to see more information about UIO in bootlog. If you and Hans want to remove it, I will do it.
Grant Likely wrote: > On Mon, Apr 18, 2011 at 10:50:54AM +0200, Michal Simek wrote: >> Adding OF binding to genirq. >> Version string is setup to the "devicetree".
>> Compatible string is not setup for now but you can add your >> custom compatible string to uio_of_genirq_match structure.
>> For example with "uio" compatible string: >> static const struct of_device_id __devinitconst uio_of_genirq_match[] = { >> { .compatible = "uio", }, >> { /* empty for now */ }, >> };
>> struct uio_pdrv_genirq_platdata { >> @@ -97,6 +101,30 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev) >> int ret = -EINVAL; >> int i;
>> + if (!uioinfo) { >> + int irq; >> + >> + /* alloc uioinfo for one device */ >> + uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL); >> + if (!uioinfo) { >> + ret = -ENOMEM; >> + dev_err(&pdev->dev, "unable to kmalloc\n"); >> + goto bad2; >> + } >> + uioinfo->name = pdev->dev.of_node->name; >> + uioinfo->version = "devicetree"; >> + >> + /* Multiple IRQs are not supported */ >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) {
> PowerPC and x86 will return 0 for an unassigned IRQ, as will most platforms.
Not for Microblaze but anyway I am OK to change it and I will look at that (maybe even your) patch to fix Microblaze code to return 0 for an unassigned IRQs.
On Tue, Apr 19, 2011 at 4:11 PM, Grant Likely <grant.lik...@secretlab.ca> wrote: > On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote: >> On Tue, Apr 19, 2011 at 2:06 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: >> > Hi,
>> >> For example with "uio" compatible string: >> >> static const struct of_device_id __devinitconst uio_of_genirq_match[] = { >> >> { .compatible = "uio", }, >> >> { /* empty for now */ }, >> >> };
>> > Please use a proper example with "vendor,device". >> > (And after that it won't be empty anymore)
>> My vote is, and always has been 'generic-uio' :)
>> Putting some random vendor/device string in there is just nuts. Do you >> really want a kernel patch every time some one binds their device to >> it?
>> Or, is there no expectation that anybody would attempt to merge such a >> pointless patch to begin with?
>> As we discussed at ELC, putting a real vendor/device in there is also >> broken because all instances in the system wil bind to the generic >> uio, which is not necessarily what is desired.
>> I know the arguments against the 'generic-uio' tag, but come on, let's >> look at the lesser of two evils here! I call BS on this DTS purity.
> Call it what you like, but the reasons are well founded. The alternative > that has been proposed which I am in agreement with is to investigate > giving userspace the hook to tell the kernel at runtime which devices > should be picked up by the uio driver.
OK, so let's talk about this interface. As I see it, it must be able to handle bind per-instance, not per compatibility.
For example, we make systems with multiple, identical timers. One will be used as the system timer, the others need to be (optionally) bound to generic UIO.
or whatever, as this would bind all instances matching vendor,device.
So, the question I have is, how to handle bind per-instance?
I can accept that the generic-uio idea is permanently blocked, but please can we have some concrete suggestions on an approach that would be acceptable?
> In the mean time, explicitly modifying the match table is an okay > compromise.
My mind is still boggling that in this day and age it could possibly be preferred to modify code, instead of a data structure. However, clearly this is a lost cause!
> OK, so let's talk about this interface. As I see it, it must be able > to handle bind per-instance, not per compatibility.
Yes.
> For example, we make systems with multiple, identical timers. One > will be used as the system timer, the others need to be (optionally) > bound to generic UIO.
> or whatever, as this would bind all instances matching vendor,device.
> So, the question I have is, how to handle bind per-instance?
> I can accept that the generic-uio idea is permanently blocked, but > please can we have some concrete suggestions on an approach that would > be acceptable?
I think nobody has had a good idea so far, unfortunately. It would be nice if you could research how libusb, vfio and qemu pci passthrough work. Hopefully one of these uses a method that we can do here, too.
> > In the mean time, explicitly modifying the match table is an okay > > compromise.
> My mind is still boggling that in this day and age it could possibly > be preferred to modify code, instead of a data structure. However, > clearly this is a lost cause!
It's preferred to do a local modification to a single kernel build over introducing an interface that we have to maintain compatibility with when we already know we don't want it.
<john.willi...@petalogix.com> wrote: > On Tue, Apr 19, 2011 at 4:11 PM, Grant Likely <grant.lik...@secretlab.ca> wrote: >> On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote: >>> On Tue, Apr 19, 2011 at 2:06 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: >>> > Hi,
>>> >> For example with "uio" compatible string: >>> >> static const struct of_device_id __devinitconst uio_of_genirq_match[] = { >>> >> { .compatible = "uio", }, >>> >> { /* empty for now */ }, >>> >> };
>>> > Please use a proper example with "vendor,device". >>> > (And after that it won't be empty anymore)
>>> My vote is, and always has been 'generic-uio' :)
>>> Putting some random vendor/device string in there is just nuts. Do you >>> really want a kernel patch every time some one binds their device to >>> it?
>>> Or, is there no expectation that anybody would attempt to merge such a >>> pointless patch to begin with?
>>> As we discussed at ELC, putting a real vendor/device in there is also >>> broken because all instances in the system wil bind to the generic >>> uio, which is not necessarily what is desired.
>>> I know the arguments against the 'generic-uio' tag, but come on, let's >>> look at the lesser of two evils here! I call BS on this DTS purity.
>> Call it what you like, but the reasons are well founded. The alternative >> that has been proposed which I am in agreement with is to investigate >> giving userspace the hook to tell the kernel at runtime which devices >> should be picked up by the uio driver.
> OK, so let's talk about this interface. As I see it, it must be able > to handle bind per-instance, not per compatibility.
> For example, we make systems with multiple, identical timers. One > will be used as the system timer, the others need to be (optionally) > bound to generic UIO.
>> In the mean time, explicitly modifying the match table is an okay >> compromise.
> My mind is still boggling that in this day and age it could possibly > be preferred to modify code, instead of a data structure. However, > clearly this is a lost cause!
I don't think anybody wants this as a long term solution. It is merely a stop-gap so that development is not stalled while working out a real interface.
But what code would create that in sysfs, and based on what properties of the device? Should we do that for every platform device, or perhaps for every one that has any resources?
On Tue, Apr 19, 2011 at 9:07 AM, Arnd Bergmann <a...@arndb.de> wrote: > On Tuesday 19 April 2011, Grant Likely wrote: >> > or whatever, as this would bind all instances matching vendor,device.
>> > So, the question I have is, how to handle bind per-instance?
>> By manipulating a property on the device instance of course! :-)
> But what code would create that in sysfs, and based on what properties > of the device? Should we do that for every platform device, or perhaps > for every one that has any resources?
Yes, I think it can be a generic facility for all platform_devices. We already have some code to support user-space manipulated driver binding/unbinding. This is very much in the same vein.
On Tue, Apr 19, 2011 at 12:08:16AM -0600, Grant Likely wrote: > On Mon, Apr 18, 2011 at 10:50:54AM +0200, Michal Simek wrote: > > Adding OF binding to genirq. > > Version string is setup to the "devicetree".
> > Compatible string is not setup for now but you can add your > > custom compatible string to uio_of_genirq_match structure.
> > For example with "uio" compatible string: > > static const struct of_device_id __devinitconst uio_of_genirq_match[] = { > > { .compatible = "uio", }, > > { /* empty for now */ }, > > };
> > Signed-off-by: Michal Simek <mon...@monstr.eu>
> > ---
> > v3: Fix irq binding > > Use "devicetree" as version string
> > struct uio_pdrv_genirq_platdata { > > @@ -97,6 +101,30 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev) > > int ret = -EINVAL; > > int i;
> > + if (!uioinfo) { > > + int irq; > > + > > + /* alloc uioinfo for one device */ > > + uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL); > > + if (!uioinfo) { > > + ret = -ENOMEM; > > + dev_err(&pdev->dev, "unable to kmalloc\n"); > > + goto bad2; > > + } > > + uioinfo->name = pdev->dev.of_node->name; > > + uioinfo->version = "devicetree"; > > + > > + /* Multiple IRQs are not supported */ > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) {
> PowerPC and x86 will return 0 for an unassigned IRQ, as will most platforms.
That might be right for these architectures. On ARM SoCs, IRQ0 is often a normal irq like any other (e.g. "Audio DMA Controller 1" on Telechips TCC8000).
We've had this discussion before. In my original uio_driver.h, UIO_IRQ_NONE was #defined as -1. There was heavy opposition against that, and I finally accepted a patch changing UIO_IRQ_NONE to 0. It's a tradeoff between 0 being a valid interrupt number on most archs and the convenience of the compiler automatically inserting a 0 into an unassigned variable.
The above line
if (irq < 0) {
is probably a copy-paste leftover of some older code. Today, irq < 0 should never be true in UIO, so the line is wrong.
On Wed, 20 Apr 2011 00:00:18 +0200 "Hans J. Koch" <h...@hansjkoch.de> wrote:
> On Tue, Apr 19, 2011 at 12:08:16AM -0600, Grant Likely wrote: > > PowerPC and x86 will return 0 for an unassigned IRQ, as will most platforms.
> That might be right for these architectures. On ARM SoCs, IRQ0 is often a > normal irq like any other (e.g. "Audio DMA Controller 1" on Telechips TCC8000).
It's true on at least some powerpc and x86 interrupt controllers as well. ARM isn't special. :-)
I'm not sure what goes on on x86, as I see a real " 0:" in /proc/interrupts. But on powerpc, Linux's IRQ numberspace is decoupled from that of any IRQ controller. This is mainly to accommodate multiple IRQ controllers with their own numberspaces in the same system; being able to avoid irq 0 is just a bonus.
