Re: [PATCH v2 08/13] dm: irq: Add support for requesting interrupts
Hi Simon,
On Tue, Feb 4, 2020 at 8:19 AM Simon Glass wrote:
>
> Hi Bin,
>
> On Mon, 3 Feb 2020 at 10:05, Bin Meng wrote:
> >
> > Hi Simon,
> >
> > On Sun, Dec 22, 2019 at 2:16 AM Simon Glass wrote:
> > >
> > > At present driver model supports the IRQ uclass but there is no way to
> > > request a particular interrupt for a driver.
> > >
> > > Add a mechanism, similar to clock and reset, to read the interrupts
> > > required by a device from the device tree and to request those interrupts.
> > >
> > > U-Boot itself does not have interrupt-driven handlers, so just provide a
> > > means to read and clear an interrupt. This can be useful to handle
> > > peripherals which must use an interrupt to determine when data is
> > > available, for example.
> > >
> > > Bring over the basic binding file as well, from Linux v5.4. Note that the
> > > older binding is not supported in U-Boot; the newer 'special form' must be
> > > used.
> > >
> > > Add a simple test of the new functionality.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > Changes in v2: None
> > >
> > > arch/sandbox/dts/test.dts | 5 +-
> > > .../interrupt-controller/interrupts.txt | 131 ++
> > > drivers/misc/irq-uclass.c | 116
> > > drivers/misc/irq_sandbox.c| 41 ++
> > > include/irq.h | 115 +++
> > > test/dm/irq.c | 31 +
> > > 6 files changed, 438 insertions(+), 1 deletion(-)
> > > create mode 100644
> > > doc/device-tree-bindings/interrupt-controller/interrupts.txt
> > >
>
> [..]
> > > diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> > > index 38498a66a4..7d65192858 100644
> > > --- a/drivers/misc/irq-uclass.c
> > > +++ b/drivers/misc/irq-uclass.c
> > > @@ -4,8 +4,11 @@
> > > * Written by Simon Glass
> > > */
> > >
> > > +#define LOG_CATEGORY UCLASS_IRQ
> > > +
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -49,6 +52,119 @@ int irq_restore_polarities(struct udevice *dev)
> > > return ops->restore_polarities(dev);
> > > }
> > >
> > > +int irq_read_and_clear(struct irq *irq)
> > > +{
> > > + const struct irq_ops *ops = irq_get_ops(irq->dev);
> > > +
> > > + if (!ops->read_and_clear)
> > > + return -ENOSYS;
> > > +
> > > + return ops->read_and_clear(irq);
> > > +}
> > > +
> > > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > > +int irq_get_by_index_platdata(struct udevice *dev, int index,
> > > + struct phandle_1_arg *cells, struct irq
> > > *irq)
> > > +{
> > > + int ret;
> > > +
> > > + if (index != 0)
> > > + return -ENOSYS;
> > > + ret = uclass_get_device(UCLASS_IRQ, 0, >dev);
> > > + if (ret)
> > > + return ret;
> > > + irq->id = cells[0].arg[0];
> > > +
> > > + return 0;
> > > +}
> > > +#else
> > > +static int irq_of_xlate_default(struct irq *irq,
> > > + struct ofnode_phandle_args *args)
> > > +{
> > > + log_debug("(irq=%p)\n", irq);
> > > +
> > > + if (args->args_count > 1) {
> > > + log_debug("Invaild args_count: %d\n", args->args_count);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (args->args_count)
> > > + irq->id = args->args[0];
> > > + else
> > > + irq->id = 0;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int irq_get_by_index_tail(int ret, ofnode node,
> >
> > It's odd to pass the return value of some other functions to this
> > function and check it here. Can we please remove ret, and check its
> > value at where we get that?
>
> This pattern is similar to how it is done in DM core. It routes all
> return paths through a single function where you can do error
> checking, etc. So I think this is better than the alternative. The
> only strange thing here is that this function is only used one. I did
> this since I suspected there would be others coming. But I could
> inline if it you like..
If that's the way how other DM codes do, I am okay with that.
>
> [..]
>
> > > +static int sandbox_irq_of_xlate(struct irq *irq,
> > > + struct ofnode_phandle_args *args)
> > > +{
> > > + irq->id = args->args[0];
> > > +
> > > + return 0;
> > > +}
> >
> > This
>
> ...?
I must have been working too long yesterday ...
Regards,
Bin
Re: [PATCH v2 08/13] dm: irq: Add support for requesting interrupts
Hi Bin,
On Mon, 3 Feb 2020 at 10:05, Bin Meng wrote:
>
> Hi Simon,
>
> On Sun, Dec 22, 2019 at 2:16 AM Simon Glass wrote:
> >
> > At present driver model supports the IRQ uclass but there is no way to
> > request a particular interrupt for a driver.
> >
> > Add a mechanism, similar to clock and reset, to read the interrupts
> > required by a device from the device tree and to request those interrupts.
> >
> > U-Boot itself does not have interrupt-driven handlers, so just provide a
> > means to read and clear an interrupt. This can be useful to handle
> > peripherals which must use an interrupt to determine when data is
> > available, for example.
> >
> > Bring over the basic binding file as well, from Linux v5.4. Note that the
> > older binding is not supported in U-Boot; the newer 'special form' must be
> > used.
> >
> > Add a simple test of the new functionality.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > Changes in v2: None
> >
> > arch/sandbox/dts/test.dts | 5 +-
> > .../interrupt-controller/interrupts.txt | 131 ++
> > drivers/misc/irq-uclass.c | 116
> > drivers/misc/irq_sandbox.c| 41 ++
> > include/irq.h | 115 +++
> > test/dm/irq.c | 31 +
> > 6 files changed, 438 insertions(+), 1 deletion(-)
> > create mode 100644
> > doc/device-tree-bindings/interrupt-controller/interrupts.txt
> >
[..]
> > diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> > index 38498a66a4..7d65192858 100644
> > --- a/drivers/misc/irq-uclass.c
> > +++ b/drivers/misc/irq-uclass.c
> > @@ -4,8 +4,11 @@
> > * Written by Simon Glass
> > */
> >
> > +#define LOG_CATEGORY UCLASS_IRQ
> > +
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -49,6 +52,119 @@ int irq_restore_polarities(struct udevice *dev)
> > return ops->restore_polarities(dev);
> > }
> >
> > +int irq_read_and_clear(struct irq *irq)
> > +{
> > + const struct irq_ops *ops = irq_get_ops(irq->dev);
> > +
> > + if (!ops->read_and_clear)
> > + return -ENOSYS;
> > +
> > + return ops->read_and_clear(irq);
> > +}
> > +
> > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > +int irq_get_by_index_platdata(struct udevice *dev, int index,
> > + struct phandle_1_arg *cells, struct irq *irq)
> > +{
> > + int ret;
> > +
> > + if (index != 0)
> > + return -ENOSYS;
> > + ret = uclass_get_device(UCLASS_IRQ, 0, >dev);
> > + if (ret)
> > + return ret;
> > + irq->id = cells[0].arg[0];
> > +
> > + return 0;
> > +}
> > +#else
> > +static int irq_of_xlate_default(struct irq *irq,
> > + struct ofnode_phandle_args *args)
> > +{
> > + log_debug("(irq=%p)\n", irq);
> > +
> > + if (args->args_count > 1) {
> > + log_debug("Invaild args_count: %d\n", args->args_count);
> > + return -EINVAL;
> > + }
> > +
> > + if (args->args_count)
> > + irq->id = args->args[0];
> > + else
> > + irq->id = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int irq_get_by_index_tail(int ret, ofnode node,
>
> It's odd to pass the return value of some other functions to this
> function and check it here. Can we please remove ret, and check its
> value at where we get that?
This pattern is similar to how it is done in DM core. It routes all
return paths through a single function where you can do error
checking, etc. So I think this is better than the alternative. The
only strange thing here is that this function is only used one. I did
this since I suspected there would be others coming. But I could
inline if it you like..
[..]
> > +static int sandbox_irq_of_xlate(struct irq *irq,
> > + struct ofnode_phandle_args *args)
> > +{
> > + irq->id = args->args[0];
> > +
> > + return 0;
> > +}
>
> This
...?
Regards,
Simon
Re: [PATCH v2 08/13] dm: irq: Add support for requesting interrupts
Hi Simon,
On Sun, Dec 22, 2019 at 2:16 AM Simon Glass wrote:
>
> At present driver model supports the IRQ uclass but there is no way to
> request a particular interrupt for a driver.
>
> Add a mechanism, similar to clock and reset, to read the interrupts
> required by a device from the device tree and to request those interrupts.
>
> U-Boot itself does not have interrupt-driven handlers, so just provide a
> means to read and clear an interrupt. This can be useful to handle
> peripherals which must use an interrupt to determine when data is
> available, for example.
>
> Bring over the basic binding file as well, from Linux v5.4. Note that the
> older binding is not supported in U-Boot; the newer 'special form' must be
> used.
>
> Add a simple test of the new functionality.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v2: None
>
> arch/sandbox/dts/test.dts | 5 +-
> .../interrupt-controller/interrupts.txt | 131 ++
> drivers/misc/irq-uclass.c | 116
> drivers/misc/irq_sandbox.c| 41 ++
> include/irq.h | 115 +++
> test/dm/irq.c | 31 +
> 6 files changed, 438 insertions(+), 1 deletion(-)
> create mode 100644
> doc/device-tree-bindings/interrupt-controller/interrupts.txt
>
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index 57513a449f..0620f9aa2a 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -93,6 +93,7 @@
> <_b 9 0xc 3 2 1>;
> int-value = <1234>;
> uint-value = <(-1234)>;
> + interrupts-extended = < 3 0>;
> };
>
> junk {
> @@ -353,8 +354,10 @@
> vss-microvolts = <0>;
> };
>
> - irq {
> + irq: irq {
> compatible = "sandbox,irq";
> + interrupt-controller;
> + #interrupt-cells = <2>;
> };
>
> lcd {
> diff --git a/doc/device-tree-bindings/interrupt-controller/interrupts.txt
> b/doc/device-tree-bindings/interrupt-controller/interrupts.txt
> new file mode 100644
> index 00..38a399a6b1
> --- /dev/null
> +++ b/doc/device-tree-bindings/interrupt-controller/interrupts.txt
> @@ -0,0 +1,131 @@
> +Specifying interrupt information for devices
> +
> +
> +1) Interrupt client nodes
> +-
> +
> +Nodes that describe devices which generate interrupts must contain an
> +"interrupts" property, an "interrupts-extended" property, or both. If both
> are
> +present, the latter should take precedence; the former may be provided simply
> +for compatibility with software that does not recognize the latter. These
> +properties contain a list of interrupt specifiers, one per output interrupt.
> The
> +format of the interrupt specifier is determined by the interrupt controller
> to
> +which the interrupts are routed; see section 2 below for details.
> +
> + Example:
> + interrupt-parent = <>;
> + interrupts = <5 0>, <6 0>;
> +
> +The "interrupt-parent" property is used to specify the controller to which
> +interrupts are routed and contains a single phandle referring to the
> interrupt
> +controller node. This property is inherited, so it may be specified in an
> +interrupt client node or in any of its parent nodes. Interrupts listed in the
> +"interrupts" property are always in reference to the node's interrupt parent.
> +
> +The "interrupts-extended" property is a special form; useful when a node
> needs
> +to reference multiple interrupt parents or a different interrupt parent than
> +the inherited one. Each entry in this property contains both the parent
> phandle
> +and the interrupt specifier.
> +
> + Example:
> + interrupts-extended = < 5 1>, < 1 0>;
> +
> +(NOTE: only this 'special form' is supported in U-Boot)
> +
> +
> +2) Interrupt controller nodes
> +-
> +
> +A device is marked as an interrupt controller with the "interrupt-controller"
> +property. This is a empty, boolean property. An additional "#interrupt-cells"
> +property defines the number of cells needed to specify a single interrupt.
> +
> +It is the responsibility of the interrupt controller's binding to define the
> +length and format of the interrupt specifier. The following two variants are
> +commonly used:
> +
> + a) one cell
> + ---
> + The #interrupt-cells property is set to 1 and the single cell defines the
> + index of the interrupt within the controller.
> +
> + Example:
> +
> + vic: intc@1014 {
> + compatible = "arm,versatile-vic";
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + reg = <0x1014 0x1000>;
> + };
> +
> + sic: intc@10003000 {
> + compatible = "arm,versatile-sic";
> +
[PATCH v2 08/13] dm: irq: Add support for requesting interrupts
At present driver model supports the IRQ uclass but there is no way to
request a particular interrupt for a driver.
Add a mechanism, similar to clock and reset, to read the interrupts
required by a device from the device tree and to request those interrupts.
U-Boot itself does not have interrupt-driven handlers, so just provide a
means to read and clear an interrupt. This can be useful to handle
peripherals which must use an interrupt to determine when data is
available, for example.
Bring over the basic binding file as well, from Linux v5.4. Note that the
older binding is not supported in U-Boot; the newer 'special form' must be
used.
Add a simple test of the new functionality.
Signed-off-by: Simon Glass
---
Changes in v2: None
arch/sandbox/dts/test.dts | 5 +-
.../interrupt-controller/interrupts.txt | 131 ++
drivers/misc/irq-uclass.c | 116
drivers/misc/irq_sandbox.c| 41 ++
include/irq.h | 115 +++
test/dm/irq.c | 31 +
6 files changed, 438 insertions(+), 1 deletion(-)
create mode 100644 doc/device-tree-bindings/interrupt-controller/interrupts.txt
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 57513a449f..0620f9aa2a 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -93,6 +93,7 @@
<_b 9 0xc 3 2 1>;
int-value = <1234>;
uint-value = <(-1234)>;
+ interrupts-extended = < 3 0>;
};
junk {
@@ -353,8 +354,10 @@
vss-microvolts = <0>;
};
- irq {
+ irq: irq {
compatible = "sandbox,irq";
+ interrupt-controller;
+ #interrupt-cells = <2>;
};
lcd {
diff --git a/doc/device-tree-bindings/interrupt-controller/interrupts.txt
b/doc/device-tree-bindings/interrupt-controller/interrupts.txt
new file mode 100644
index 00..38a399a6b1
--- /dev/null
+++ b/doc/device-tree-bindings/interrupt-controller/interrupts.txt
@@ -0,0 +1,131 @@
+Specifying interrupt information for devices
+
+
+1) Interrupt client nodes
+-
+
+Nodes that describe devices which generate interrupts must contain an
+"interrupts" property, an "interrupts-extended" property, or both. If both are
+present, the latter should take precedence; the former may be provided simply
+for compatibility with software that does not recognize the latter. These
+properties contain a list of interrupt specifiers, one per output interrupt.
The
+format of the interrupt specifier is determined by the interrupt controller to
+which the interrupts are routed; see section 2 below for details.
+
+ Example:
+ interrupt-parent = <>;
+ interrupts = <5 0>, <6 0>;
+
+The "interrupt-parent" property is used to specify the controller to which
+interrupts are routed and contains a single phandle referring to the interrupt
+controller node. This property is inherited, so it may be specified in an
+interrupt client node or in any of its parent nodes. Interrupts listed in the
+"interrupts" property are always in reference to the node's interrupt parent.
+
+The "interrupts-extended" property is a special form; useful when a node needs
+to reference multiple interrupt parents or a different interrupt parent than
+the inherited one. Each entry in this property contains both the parent phandle
+and the interrupt specifier.
+
+ Example:
+ interrupts-extended = < 5 1>, < 1 0>;
+
+(NOTE: only this 'special form' is supported in U-Boot)
+
+
+2) Interrupt controller nodes
+-
+
+A device is marked as an interrupt controller with the "interrupt-controller"
+property. This is a empty, boolean property. An additional "#interrupt-cells"
+property defines the number of cells needed to specify a single interrupt.
+
+It is the responsibility of the interrupt controller's binding to define the
+length and format of the interrupt specifier. The following two variants are
+commonly used:
+
+ a) one cell
+ ---
+ The #interrupt-cells property is set to 1 and the single cell defines the
+ index of the interrupt within the controller.
+
+ Example:
+
+ vic: intc@1014 {
+ compatible = "arm,versatile-vic";
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ reg = <0x1014 0x1000>;
+ };
+
+ sic: intc@10003000 {
+ compatible = "arm,versatile-sic";
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ reg = <0x10003000 0x1000>;
+ interrupt-parent = <>;
+ interrupts = <31>; /* Cascaded to vic */
+ };
+
+ b) two cells
+
+ The #interrupt-cells property is set to 2 and the first cell
