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 <[email protected]>
---
v3: Fix irq binding
Use "devicetree" as version string
v2: Remove additional resource binding
Setup correct version string
Clear compatible string
---
drivers/uio/uio_pdrv_genirq.c | 42 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 7174d51..c8d565a 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -23,6 +23,10 @@
#include <linux/pm_runtime.h>
#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+
#define DRIVER_NAME "uio_pdrv_genirq"
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) {
+ uioinfo->irq = UIO_IRQ_NONE;
+ dev_info(&pdev->dev, "no IRQ found\n");
+ } else {
+ uioinfo->irq = irq;
+ dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
+ }
+ }
+
if (!uioinfo || !uioinfo->name || !uioinfo->version) {
dev_err(&pdev->dev, "missing platform_data\n");
goto bad0;
@@ -180,6 +208,10 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
kfree(priv);
pm_runtime_disable(&pdev->dev);
bad0:
+ /* kfree uioinfo for CONFIG_OF */
+ if (!pdev->dev.platform_data)
+ kfree(uioinfo);
+ bad2:
return ret;
}
@@ -215,6 +247,15 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
.runtime_resume = uio_pdrv_genirq_runtime_nop,
};
+#ifdef CONFIG_OF
+static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
+ { /* empty for now */ },
+};
+MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
+#else
+# define uio_of_genirq_match NULL
+#endif
+
static struct platform_driver uio_pdrv_genirq = {
.probe = uio_pdrv_genirq_probe,
.remove = uio_pdrv_genirq_remove,
@@ -222,6 +263,7 @@ static struct platform_driver uio_pdrv_genirq = {
.name = DRIVER_NAME,
.owner = THIS_MODULE,
.pm = &uio_pdrv_genirq_dev_pm_ops,
+ .of_match_table = uio_of_genirq_match,
},
};
--
1.5.5.6
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 <[email protected]>
>
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.
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 */ },
>> };
>>
>> Signed-off-by: Michal Simek <[email protected]>
>>
> 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.
Sure - make sense.
Michal
--
Michal Simek, Ing. (M.Eng)
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
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)
> + /* Multiple IRQs are not supported */
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + uioinfo->irq = UIO_IRQ_NONE;
> + dev_info(&pdev->dev, "no IRQ found\n");
> + } else {
> + uioinfo->irq = irq;
> + dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
> + }
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.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Tue, Apr 19, 2011 at 2:06 AM, Wolfram Sang <[email protected]> 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.
John
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 <[email protected]>
>
> ---
>
> v3: Fix irq binding
> Use "devicetree" as version string
>
> v2: Remove additional resource binding
> Setup correct version string
> Clear compatible string
> ---
> drivers/uio/uio_pdrv_genirq.c | 42 +++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 42 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 7174d51..c8d565a 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -23,6 +23,10 @@
> #include <linux/pm_runtime.h>
> #include <linux/slab.h>
>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +
> #define DRIVER_NAME "uio_pdrv_genirq"
>
> 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.
> + uioinfo->irq = UIO_IRQ_NONE;
> + dev_info(&pdev->dev, "no IRQ found\n");
> + } else {
> + uioinfo->irq = irq;
> + dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
> + }
> + }
> +
> if (!uioinfo || !uioinfo->name || !uioinfo->version) {
> dev_err(&pdev->dev, "missing platform_data\n");
> goto bad0;
> @@ -180,6 +208,10 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
> kfree(priv);
> pm_runtime_disable(&pdev->dev);
> bad0:
> + /* kfree uioinfo for CONFIG_OF */
> + if (!pdev->dev.platform_data)
> + kfree(uioinfo);
> + bad2:
> return ret;
> }
>
> @@ -215,6 +247,15 @@ static const struct dev_pm_ops uio_pdrv_genirq_dev_pm_ops = {
> .runtime_resume = uio_pdrv_genirq_runtime_nop,
> };
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id __devinitconst uio_of_genirq_match[] = {
> + { /* empty for now */ },
> +};
> +MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
> +#else
> +# define uio_of_genirq_match NULL
> +#endif
> +
> static struct platform_driver uio_pdrv_genirq = {
> .probe = uio_pdrv_genirq_probe,
> .remove = uio_pdrv_genirq_remove,
> @@ -222,6 +263,7 @@ static struct platform_driver uio_pdrv_genirq = {
> .name = DRIVER_NAME,
> .owner = THIS_MODULE,
> .pm = &uio_pdrv_genirq_dev_pm_ops,
> + .of_match_table = uio_of_genirq_match,
> },
> };
>
> --
> 1.5.5.6
>
On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote:
> On Tue, Apr 19, 2011 at 2:06 AM, Wolfram Sang <[email protected]> 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.
g.
On Tuesday 19 April 2011, Grant Likely wrote:
> 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.
Agreed.
Arnd
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 <[email protected]>
>>
>> ---
>>
>> v3: Fix irq binding
>> Use "devicetree" as version string
>>
>> v2: Remove additional resource binding
>> Setup correct version string
>> Clear compatible string
>> ---
>> drivers/uio/uio_pdrv_genirq.c | 42 +++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>> index 7174d51..c8d565a 100644
>> --- a/drivers/uio/uio_pdrv_genirq.c
>> +++ b/drivers/uio/uio_pdrv_genirq.c
>> @@ -23,6 +23,10 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/slab.h>
>>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +
>> #define DRIVER_NAME "uio_pdrv_genirq"
>>
>> 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.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
Wolfram Sang 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)
>
>> + /* Multiple IRQs are not supported */
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + uioinfo->irq = UIO_IRQ_NONE;
>> + dev_info(&pdev->dev, "no IRQ found\n");
>> + } else {
>> + uioinfo->irq = irq;
>> + dev_info(&pdev->dev, "irq %d\n", (u32)uioinfo->irq);
>> + }
>
> 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.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
On Tue, Apr 19, 2011 at 4:11 PM, Grant Likely <[email protected]> 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 <[email protected]> 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.
Therefore, it's not OK to just do
echo "vendor,device" >> /sys/class/something/generic-uio/compatlist
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!
John
On Tuesday 19 April 2011, John Williams wrote:
> 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.
>
> Therefore, it's not OK to just do
>
> echo "vendor,device" >> /sys/class/something/generic-uio/compatlist
>
> 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.
Arnd
On Tue, Apr 19, 2011 at 6:37 AM, John Williams
<[email protected]> wrote:
> On Tue, Apr 19, 2011 at 4:11 PM, Grant Likely <[email protected]> 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 <[email protected]> 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.
>
> Therefore, it's not OK to just do
>
> echo "vendor,device" >> /sys/class/something/generic-uio/compatlist
>
> 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! :-)
Something like: echo "generic-uio" >>
/sys/devices/path/to/device/a-property-that-changes-the-driver-it-will-bind-to.
>> 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.
g.
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! :-)
>
> Something like: echo "generic-uio" >>
> /sys/devices/path/to/device/a-property-that-changes-the-driver-it-will-bind-to.
>
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?
Arnd
On Tue, Apr 19, 2011 at 9:07 AM, Arnd Bergmann <[email protected]> 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! ?:-)
>>
>> Something like: ?echo "generic-uio" >>
>> /sys/devices/path/to/device/a-property-that-changes-the-driver-it-will-bind-to.
>>
>
> 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.
g.
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 <[email protected]>
> >
> > ---
> >
> > v3: Fix irq binding
> > Use "devicetree" as version string
> >
> > v2: Remove additional resource binding
> > Setup correct version string
> > Clear compatible string
> > ---
> > drivers/uio/uio_pdrv_genirq.c | 42 +++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 42 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> > index 7174d51..c8d565a 100644
> > --- a/drivers/uio/uio_pdrv_genirq.c
> > +++ b/drivers/uio/uio_pdrv_genirq.c
> > @@ -23,6 +23,10 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/slab.h>
> >
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +
> > #define DRIVER_NAME "uio_pdrv_genirq"
> >
> > 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.
Thanks for pointing to it,
Hans
On Wed, 20 Apr 2011 00:00:18 +0200
"Hans J. Koch" <[email protected]> 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.
-Scott
On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote:
> 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.
IIRC I pointed you to mpc5200b-psc devices which also have multiple functions
and have seperate bindings for these functions. It fits the "hardware
description" character of a dt, because it says this PSC is a UART or an
SPI or whatever.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Wolfram,
On Thu, Apr 21, 2011 at 10:08 PM, Wolfram Sang <[email protected]> wrote:
> On Tue, Apr 19, 2011 at 11:58:25AM +1000, John Williams wrote:
>
>> 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.
>
> IIRC I pointed you to mpc5200b-psc devices which also have multiple functions
> and have seperate bindings for these functions. It fits the "hardware
> description" character of a dt, because it says this PSC is a UART or an
> SPI or whatever.
Right, and my interpretation of that was basically - modify the device
tree as required to make the correct driver bind to the device.
Is that correct?
It's the same underlying piece of silicon, you are just using the
device tree to tell Linux how it should behave at runtime, and which
driver to bind.
If so, it seems to be exactly what Arnd and Grant don't want - having
the device tree describe Linux behaviour.
In the hardware, it's a multifunction device, so surely a clean DTS
would just have 'mpc5200b', but no attempt to describe how Linux
should bind to it and configure it. Instead this should be done at
runtime, using some yet-to-be-determined mechanism.
The difference between what you are doing for these multifunction
devices, versus the generic-uio approach, is surely only a matter of
degree rather than fundamentals?
Regards,
John
> The difference between what you are doing for these multifunction
> devices, versus the generic-uio approach, is surely only a matter of
> degree rather than fundamentals?
I don't think so.
If you write "uio-generic", then you explicitly specify uio which is linux
specific and not hardware related. There is no uio-hardware :)
If you write "fsl,mpc5200b-psc-uart" then you describe that in your hardware
the psc is used as an UART (and not SPI) and every OS can make its assumption
about it.
So, you could have "company, super-fpga64-uart" which would bind to the
corresponding UART driver. And "company, super-fpga64-encryptor" would be
handled in Linux by the uio-driver if you setup this way, for example. If you
later have a seperate driver for it, then it will match against this
compatible-entry instead of uio. No change of the devicetree, the hardware did
not change.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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 <[email protected]>
>>
>> ---
>>
>> v3: Fix irq binding
>> Use "devicetree" as version string
>>
>> v2: Remove additional resource binding
>> Setup correct version string
>> Clear compatible string
>> ---
>> drivers/uio/uio_pdrv_genirq.c | 42 +++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
>> index 7174d51..c8d565a 100644
>> --- a/drivers/uio/uio_pdrv_genirq.c
>> +++ b/drivers/uio/uio_pdrv_genirq.c
>> @@ -23,6 +23,10 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/slab.h>
>>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +
>> #define DRIVER_NAME "uio_pdrv_genirq"
>>
>> 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.
I have looked at it and platform_get_irq for unassigned IRQ returns -ENXIO.
It is there from 2006 (drivers/base/platform.c)
"[PATCH] driver core: platform_get_irq*(): return -ENXIO on error"
(sha1 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0)
Maybe you have this patch in your queue but it is not in the mainline 2.6.39-rc5.
Maybe should return NO_IRQ instead.
But anyway for current code I would prefer to use this condition to cover all
architectures NO_IRQ definition.
irq = platform_get_irq(pdev, 0);
if (irq == -ENXIO || irq == NO_IRQ)
uioinfo->irq = UIO_IRQ_NONE;
else
uioinfo->irq = irq;
or of course to only NO_IRQ if we do s/-ENXIO/NO_IRQ in platform_get_irq.
if (irq == NO_IRQ)
uioinfo->irq = UIO_IRQ_NONE;
else
uioinfo->irq = irq;
What do you think?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian