Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On Mon, 12 Nov 2018 10:32:51 +,
Parthiban Nallathambi wrote:
>
>
>
> On 11/8/18 6:03 PM, Marc Zyngier wrote:
> > On 26/08/18 16:20, Parthiban Nallathambi wrote:
> >> Hello Marc,
> >>
> >> Thanks for your feedback.
> >>
> >> On 8/13/18 1:46 PM, Marc Zyngier wrote:
> >>> On 12/08/18 13:22, Parthiban Nallathambi wrote:
> Actions Semi Owl family SoC's S500, S700 and S900 provides support
> for 3 external interrupt controllers through SIRQ pins.
>
> Each line can be independently configured as interrupt and triggers
> on either of the edges (raising or falling) or either of the levels
> (high or low) . Each line can also be masked independently.
>
> Signed-off-by: Parthiban Nallathambi
> Signed-off-by: Saravanan Sekar
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 305
> +
> 2 files changed, 306 insertions(+)
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..072c4409e7c4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) +=
> irq-ath79-misc.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> obj-$(CONFIG_FARADAY_FTINTC010)+= irq-ftintc010.o
> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
> diff --git a/drivers/irqchip/irq-owl-sirq.c
> b/drivers/irqchip/irq-owl-sirq.c
> new file mode 100644
> index ..b69301388300
> --- /dev/null
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *
> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> + *
> + * Copyright (C) 2014 Actions Semi Inc.
> + * David Liu
> + *
> + * Author: Parthiban Nallathambi
> + * Author: Saravanan Sekar
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define INTC_GIC_INTERRUPT_PIN 13
> >>>
> >>> Why isn't that coming from the DT?
> >>
> >> DT numbering is taken irqchip local, by which hwirq is directly used to
> >> calculate the offset into register when it is shared. Even if this is
> >> directly from DT, I need the value to offset into the register. So
> >> maintianed
> >> inside the driver.
> >
> > This is normally shown as a property from DT, and is relative to the
> > parent irqchip. And I don't understand what you mean by "offset into the
> > register". The only use of this is to allocate the corresponding GIC
>
> We have two SoC's (s500, s700) with shared external interrupt control
> register and one (s900) with dedicated register for each external
> interrupt line. So the DT property "actions,sirq-offset" was introduced
> to access the register.
>
> In case of s500, s700 when it's shared, the idea is to use the "hwirq"
> variable value to offset into the control register INTC_EXTCTL. Even if
> 3 cell GIC value is directly used like
>
> interrupts = ;
>
> then hwirq - 13 is needed internally everywhere in this driver.
>
> In short, this value is defined inside driver for the ease of referring
> the offset with the register.
>
> Yes, it is possible to change the driver logic and use 3 cell interrupts
> from DT.
>
> > interrupt, and this definitely shouldn't be harcoded.
> >
> >>
> >> Should it make sense to move it to DT and use another macro (different
> >> name)
> >> for offsetting?
> >>
> >>>
> +#define INTC_EXTCTL_PENDING BIT(0)
> +#define INTC_EXTCTL_CLK_SEL BIT(4)
> +#define INTC_EXTCTL_EN BIT(5)
> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
> +#define INTC_EXTCTL_TYPE_HIGH 0
> +#define INTC_EXTCTL_TYPE_LOWBIT(6)
> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> +#define INTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
> +
> +#define get_sirq_offset(x) chip_data->sirq[x].offset
> +
> +/* Per SIRQ data */
> +struct owl_sirq {
> +u16 offset;
> +/* software is responsible to clear interrupt pending bit when
> + * type is edge triggered. This value is for per SIRQ line.
> + */
> >>>
> >>> Please follow the normal multi-line comment style:
> >>>
> >>> /*
> >>> * This is a comment, starting with a capital letter and ending with
> >>> * a full stop.
> >>> */
> >>
> >> Sure,
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On Mon, 12 Nov 2018 10:32:51 +,
Parthiban Nallathambi wrote:
>
>
>
> On 11/8/18 6:03 PM, Marc Zyngier wrote:
> > On 26/08/18 16:20, Parthiban Nallathambi wrote:
> >> Hello Marc,
> >>
> >> Thanks for your feedback.
> >>
> >> On 8/13/18 1:46 PM, Marc Zyngier wrote:
> >>> On 12/08/18 13:22, Parthiban Nallathambi wrote:
> Actions Semi Owl family SoC's S500, S700 and S900 provides support
> for 3 external interrupt controllers through SIRQ pins.
>
> Each line can be independently configured as interrupt and triggers
> on either of the edges (raising or falling) or either of the levels
> (high or low) . Each line can also be masked independently.
>
> Signed-off-by: Parthiban Nallathambi
> Signed-off-by: Saravanan Sekar
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 305
> +
> 2 files changed, 306 insertions(+)
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..072c4409e7c4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) +=
> irq-ath79-misc.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> obj-$(CONFIG_FARADAY_FTINTC010)+= irq-ftintc010.o
> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
> diff --git a/drivers/irqchip/irq-owl-sirq.c
> b/drivers/irqchip/irq-owl-sirq.c
> new file mode 100644
> index ..b69301388300
> --- /dev/null
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *
> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> + *
> + * Copyright (C) 2014 Actions Semi Inc.
> + * David Liu
> + *
> + * Author: Parthiban Nallathambi
> + * Author: Saravanan Sekar
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define INTC_GIC_INTERRUPT_PIN 13
> >>>
> >>> Why isn't that coming from the DT?
> >>
> >> DT numbering is taken irqchip local, by which hwirq is directly used to
> >> calculate the offset into register when it is shared. Even if this is
> >> directly from DT, I need the value to offset into the register. So
> >> maintianed
> >> inside the driver.
> >
> > This is normally shown as a property from DT, and is relative to the
> > parent irqchip. And I don't understand what you mean by "offset into the
> > register". The only use of this is to allocate the corresponding GIC
>
> We have two SoC's (s500, s700) with shared external interrupt control
> register and one (s900) with dedicated register for each external
> interrupt line. So the DT property "actions,sirq-offset" was introduced
> to access the register.
>
> In case of s500, s700 when it's shared, the idea is to use the "hwirq"
> variable value to offset into the control register INTC_EXTCTL. Even if
> 3 cell GIC value is directly used like
>
> interrupts = ;
>
> then hwirq - 13 is needed internally everywhere in this driver.
>
> In short, this value is defined inside driver for the ease of referring
> the offset with the register.
>
> Yes, it is possible to change the driver logic and use 3 cell interrupts
> from DT.
>
> > interrupt, and this definitely shouldn't be harcoded.
> >
> >>
> >> Should it make sense to move it to DT and use another macro (different
> >> name)
> >> for offsetting?
> >>
> >>>
> +#define INTC_EXTCTL_PENDING BIT(0)
> +#define INTC_EXTCTL_CLK_SEL BIT(4)
> +#define INTC_EXTCTL_EN BIT(5)
> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
> +#define INTC_EXTCTL_TYPE_HIGH 0
> +#define INTC_EXTCTL_TYPE_LOWBIT(6)
> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> +#define INTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
> +
> +#define get_sirq_offset(x) chip_data->sirq[x].offset
> +
> +/* Per SIRQ data */
> +struct owl_sirq {
> +u16 offset;
> +/* software is responsible to clear interrupt pending bit when
> + * type is edge triggered. This value is for per SIRQ line.
> + */
> >>>
> >>> Please follow the normal multi-line comment style:
> >>>
> >>> /*
> >>> * This is a comment, starting with a capital letter and ending with
> >>> * a full stop.
> >>> */
> >>
> >> Sure,
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On 11/8/18 6:03 PM, Marc Zyngier wrote:
On 26/08/18 16:20, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, Parthiban Nallathambi wrote:
Actions Semi Owl family SoC's S500, S700 and S900 provides support
for 3 external interrupt controllers through SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . Each line can also be masked independently.
Signed-off-by: Parthiban Nallathambi
Signed-off-by: Saravanan Sekar
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 305 +
2 files changed, 306 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..072c4409e7c4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2836.o
obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
obj-$(CONFIG_ARCH_LPC32XX)+= irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index ..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu
+ *
+ * Author: Parthiban Nallathambi
+ * Author: Saravanan Sekar
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So maintianed
inside the driver.
This is normally shown as a property from DT, and is relative to the
parent irqchip. And I don't understand what you mean by "offset into the
register". The only use of this is to allocate the corresponding GIC
We have two SoC's (s500, s700) with shared external interrupt control
register and one (s900) with dedicated register for each external
interrupt line. So the DT property "actions,sirq-offset" was introduced
to access the register.
In case of s500, s700 when it's shared, the idea is to use the "hwirq"
variable value to offset into the control register INTC_EXTCTL. Even if
3 cell GIC value is directly used like
interrupts = ;
then hwirq - 13 is needed internally everywhere in this driver.
In short, this value is defined inside driver for the ease of referring
the offset with the register.
Yes, it is possible to change the driver logic and use 3 cell interrupts
from DT.
interrupt, and this definitely shouldn't be harcoded.
Should it make sense to move it to DT and use another macro (different name)
for offsetting?
+#define INTC_EXTCTL_PENDINGBIT(0)
+#define INTC_EXTCTL_CLK_SELBIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#defineINTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
+#defineINTC_EXTCTL_TYPE_HIGH 0
+#defineINTC_EXTCTL_TYPE_LOWBIT(6)
+#defineINTC_EXTCTL_TYPE_RISING BIT(7)
+#defineINTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
+
+#define get_sirq_offset(x) chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ u16 offset;
+ /* software is responsible to clear interrupt pending bit when
+* type is edge triggered. This value is for per SIRQ line.
+*/
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
Sure, thanks.
+ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ /* some SoC's share the register for all SIRQ lines, so maintain
+* register is shared or not here. This value is from DT.
+*/
+ bool shared_reg;
+ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On 11/8/18 6:03 PM, Marc Zyngier wrote:
On 26/08/18 16:20, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, Parthiban Nallathambi wrote:
Actions Semi Owl family SoC's S500, S700 and S900 provides support
for 3 external interrupt controllers through SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . Each line can also be masked independently.
Signed-off-by: Parthiban Nallathambi
Signed-off-by: Saravanan Sekar
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 305 +
2 files changed, 306 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..072c4409e7c4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2836.o
obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
obj-$(CONFIG_ARCH_LPC32XX)+= irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index ..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu
+ *
+ * Author: Parthiban Nallathambi
+ * Author: Saravanan Sekar
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So maintianed
inside the driver.
This is normally shown as a property from DT, and is relative to the
parent irqchip. And I don't understand what you mean by "offset into the
register". The only use of this is to allocate the corresponding GIC
We have two SoC's (s500, s700) with shared external interrupt control
register and one (s900) with dedicated register for each external
interrupt line. So the DT property "actions,sirq-offset" was introduced
to access the register.
In case of s500, s700 when it's shared, the idea is to use the "hwirq"
variable value to offset into the control register INTC_EXTCTL. Even if
3 cell GIC value is directly used like
interrupts = ;
then hwirq - 13 is needed internally everywhere in this driver.
In short, this value is defined inside driver for the ease of referring
the offset with the register.
Yes, it is possible to change the driver logic and use 3 cell interrupts
from DT.
interrupt, and this definitely shouldn't be harcoded.
Should it make sense to move it to DT and use another macro (different name)
for offsetting?
+#define INTC_EXTCTL_PENDINGBIT(0)
+#define INTC_EXTCTL_CLK_SELBIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#defineINTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
+#defineINTC_EXTCTL_TYPE_HIGH 0
+#defineINTC_EXTCTL_TYPE_LOWBIT(6)
+#defineINTC_EXTCTL_TYPE_RISING BIT(7)
+#defineINTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
+
+#define get_sirq_offset(x) chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ u16 offset;
+ /* software is responsible to clear interrupt pending bit when
+* type is edge triggered. This value is for per SIRQ line.
+*/
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
Sure, thanks.
+ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ /* some SoC's share the register for all SIRQ lines, so maintain
+* register is shared or not here. This value is from DT.
+*/
+ bool shared_reg;
+ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On 26/08/18 16:20, Parthiban Nallathambi wrote:
> Hello Marc,
>
> Thanks for your feedback.
>
> On 8/13/18 1:46 PM, Marc Zyngier wrote:
>> On 12/08/18 13:22, Parthiban Nallathambi wrote:
>>> Actions Semi Owl family SoC's S500, S700 and S900 provides support
>>> for 3 external interrupt controllers through SIRQ pins.
>>>
>>> Each line can be independently configured as interrupt and triggers
>>> on either of the edges (raising or falling) or either of the levels
>>> (high or low) . Each line can also be masked independently.
>>>
>>> Signed-off-by: Parthiban Nallathambi
>>> Signed-off-by: Saravanan Sekar
>>> ---
>>> drivers/irqchip/Makefile | 1 +
>>> drivers/irqchip/irq-owl-sirq.c | 305
>>> +
>>> 2 files changed, 306 insertions(+)
>>> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>>>
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index 15f268f646bf..072c4409e7c4 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
>>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
>>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
>>> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
>>> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
>>> obj-$(CONFIG_FARADAY_FTINTC010)+= irq-ftintc010.o
>>> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
>>> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
>>> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
>>> new file mode 100644
>>> index ..b69301388300
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-owl-sirq.c
>>> @@ -0,0 +1,305 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + *
>>> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
>>> + *
>>> + * Copyright (C) 2014 Actions Semi Inc.
>>> + * David Liu
>>> + *
>>> + * Author: Parthiban Nallathambi
>>> + * Author: Saravanan Sekar
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>> +
>>> +#define INTC_GIC_INTERRUPT_PIN 13
>>
>> Why isn't that coming from the DT?
>
> DT numbering is taken irqchip local, by which hwirq is directly used to
> calculate the offset into register when it is shared. Even if this is
> directly from DT, I need the value to offset into the register. So maintianed
> inside the driver.
This is normally shown as a property from DT, and is relative to the
parent irqchip. And I don't understand what you mean by "offset into the
register". The only use of this is to allocate the corresponding GIC
interrupt, and this definitely shouldn't be harcoded.
>
> Should it make sense to move it to DT and use another macro (different name)
> for offsetting?
>
>>
>>> +#define INTC_EXTCTL_PENDINGBIT(0)
>>> +#define INTC_EXTCTL_CLK_SELBIT(4)
>>> +#define INTC_EXTCTL_EN BIT(5)
>>> +#defineINTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
>>> +#defineINTC_EXTCTL_TYPE_HIGH 0
>>> +#defineINTC_EXTCTL_TYPE_LOWBIT(6)
>>> +#defineINTC_EXTCTL_TYPE_RISING BIT(7)
>>> +#defineINTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
>>> +
>>> +#define get_sirq_offset(x) chip_data->sirq[x].offset
>>> +
>>> +/* Per SIRQ data */
>>> +struct owl_sirq {
>>> + u16 offset;
>>> + /* software is responsible to clear interrupt pending bit when
>>> +* type is edge triggered. This value is for per SIRQ line.
>>> +*/
>>
>> Please follow the normal multi-line comment style:
>>
>> /*
>> * This is a comment, starting with a capital letter and ending with
>> * a full stop.
>> */
>
> Sure, thanks.
>
>>
>>> + bool type_edge;
>>> +};
>>> +
>>> +struct owl_sirq_chip_data {
>>> + void __iomem *base;
>>> + raw_spinlock_t lock;
>>> + /* some SoC's share the register for all SIRQ lines, so maintain
>>> +* register is shared or not here. This value is from DT.
>>> +*/
>>> + bool shared_reg;
>>> + struct owl_sirq *sirq;
>>
>> Given that this driver handles at most 3 interrupts, do we need the
>> overhead of a pointer and an additional allocation, while we could store
>> all of this data in the space taken by the pointer itself?
>>
>> Something like:
>>
>> u16 offset[3];
>> u8 trigger; // Bit mask indicating edge-triggered interrupts
>>
>> and we're done.
>
> Sure, I will modify with one allocation.
>
>>
>>> +};
>>> +
>>> +static struct owl_sirq_chip_data *sirq_data;
>>> +
>>> +static unsigned int sirq_read_extctl(struct irq_data *data)
>>
>> Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
>> of always passing irq_data?
>>
>> Also, this should return a well defined size, which "unsigned int"
>> isn't. Make that u32.
>
> Sure, will adapt this.
>
>>
>>> +{
>>> + struct owl_sirq_chip_data *chip_data = data->chip_data;
>>> + unsigned int val;
>>
>> u32;
>
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On 26/08/18 16:20, Parthiban Nallathambi wrote:
> Hello Marc,
>
> Thanks for your feedback.
>
> On 8/13/18 1:46 PM, Marc Zyngier wrote:
>> On 12/08/18 13:22, Parthiban Nallathambi wrote:
>>> Actions Semi Owl family SoC's S500, S700 and S900 provides support
>>> for 3 external interrupt controllers through SIRQ pins.
>>>
>>> Each line can be independently configured as interrupt and triggers
>>> on either of the edges (raising or falling) or either of the levels
>>> (high or low) . Each line can also be masked independently.
>>>
>>> Signed-off-by: Parthiban Nallathambi
>>> Signed-off-by: Saravanan Sekar
>>> ---
>>> drivers/irqchip/Makefile | 1 +
>>> drivers/irqchip/irq-owl-sirq.c | 305
>>> +
>>> 2 files changed, 306 insertions(+)
>>> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>>>
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index 15f268f646bf..072c4409e7c4 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
>>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
>>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
>>> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
>>> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
>>> obj-$(CONFIG_FARADAY_FTINTC010)+= irq-ftintc010.o
>>> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
>>> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
>>> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
>>> new file mode 100644
>>> index ..b69301388300
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-owl-sirq.c
>>> @@ -0,0 +1,305 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + *
>>> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
>>> + *
>>> + * Copyright (C) 2014 Actions Semi Inc.
>>> + * David Liu
>>> + *
>>> + * Author: Parthiban Nallathambi
>>> + * Author: Saravanan Sekar
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>> +
>>> +#define INTC_GIC_INTERRUPT_PIN 13
>>
>> Why isn't that coming from the DT?
>
> DT numbering is taken irqchip local, by which hwirq is directly used to
> calculate the offset into register when it is shared. Even if this is
> directly from DT, I need the value to offset into the register. So maintianed
> inside the driver.
This is normally shown as a property from DT, and is relative to the
parent irqchip. And I don't understand what you mean by "offset into the
register". The only use of this is to allocate the corresponding GIC
interrupt, and this definitely shouldn't be harcoded.
>
> Should it make sense to move it to DT and use another macro (different name)
> for offsetting?
>
>>
>>> +#define INTC_EXTCTL_PENDINGBIT(0)
>>> +#define INTC_EXTCTL_CLK_SELBIT(4)
>>> +#define INTC_EXTCTL_EN BIT(5)
>>> +#defineINTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
>>> +#defineINTC_EXTCTL_TYPE_HIGH 0
>>> +#defineINTC_EXTCTL_TYPE_LOWBIT(6)
>>> +#defineINTC_EXTCTL_TYPE_RISING BIT(7)
>>> +#defineINTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
>>> +
>>> +#define get_sirq_offset(x) chip_data->sirq[x].offset
>>> +
>>> +/* Per SIRQ data */
>>> +struct owl_sirq {
>>> + u16 offset;
>>> + /* software is responsible to clear interrupt pending bit when
>>> +* type is edge triggered. This value is for per SIRQ line.
>>> +*/
>>
>> Please follow the normal multi-line comment style:
>>
>> /*
>> * This is a comment, starting with a capital letter and ending with
>> * a full stop.
>> */
>
> Sure, thanks.
>
>>
>>> + bool type_edge;
>>> +};
>>> +
>>> +struct owl_sirq_chip_data {
>>> + void __iomem *base;
>>> + raw_spinlock_t lock;
>>> + /* some SoC's share the register for all SIRQ lines, so maintain
>>> +* register is shared or not here. This value is from DT.
>>> +*/
>>> + bool shared_reg;
>>> + struct owl_sirq *sirq;
>>
>> Given that this driver handles at most 3 interrupts, do we need the
>> overhead of a pointer and an additional allocation, while we could store
>> all of this data in the space taken by the pointer itself?
>>
>> Something like:
>>
>> u16 offset[3];
>> u8 trigger; // Bit mask indicating edge-triggered interrupts
>>
>> and we're done.
>
> Sure, I will modify with one allocation.
>
>>
>>> +};
>>> +
>>> +static struct owl_sirq_chip_data *sirq_data;
>>> +
>>> +static unsigned int sirq_read_extctl(struct irq_data *data)
>>
>> Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
>> of always passing irq_data?
>>
>> Also, this should return a well defined size, which "unsigned int"
>> isn't. Make that u32.
>
> Sure, will adapt this.
>
>>
>>> +{
>>> + struct owl_sirq_chip_data *chip_data = data->chip_data;
>>> + unsigned int val;
>>
>> u32;
>
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
Hello Marc,
Ping on this patch for feedback.
On 9/20/18 11:42 AM, Parthiban Nallathambi wrote:
Hello Marc,
Ping on this patch for feedback.
On 08/26/2018 05:20 PM, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, Parthiban Nallathambi wrote:
Actions Semi Owl family SoC's S500, S700 and S900 provides support
for 3 external interrupt controllers through SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . Each line can also be masked independently.
Signed-off-by: Parthiban Nallathambi
Signed-off-by: Saravanan Sekar
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 305
+
2 files changed, 306 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..072c4409e7c4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-owl-sirq.c
b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index ..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu
+ *
+ * Author: Parthiban Nallathambi
+ * Author: Saravanan Sekar
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So
maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different
name)
for offsetting?
+#define INTC_EXTCTL_PENDING BIT(0)
+#define INTC_EXTCTL_CLK_SEL BIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
+#define INTC_EXTCTL_TYPE_HIGH 0
+#define INTC_EXTCTL_TYPE_LOW BIT(6)
+#define INTC_EXTCTL_TYPE_RISING BIT(7)
+#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
+
+#define get_sirq_offset(x) chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ u16 offset;
+ /* software is responsible to clear interrupt pending bit when
+ * type is edge triggered. This value is for per SIRQ line.
+ */
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
Sure, thanks.
+ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ /* some SoC's share the register for all SIRQ lines, so maintain
+ * register is shared or not here. This value is from DT.
+ */
+ bool shared_reg;
+ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
Sure, will adapt this.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ val = readl_relaxed(chip_data->base +
get_sirq_offset(data->hwirq));
+ if (chip_data->shared_reg)
+ val = (val >> (2 - data->hwirq) * 8) & 0xff;
+
+ return val;
+}
+
+static void sirq_write_extctl(struct irq_data *data, unsigned int
extctl)
Same comments.
Sure.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ if (chip_data->shared_reg) {
+ val = readl_relaxed(chip_data->base +
+ get_sirq_offset(data->hwirq));
Single line, please.
Sure.
+ val &= ~(0xff << (2 -
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
Hello Marc,
Ping on this patch for feedback.
On 9/20/18 11:42 AM, Parthiban Nallathambi wrote:
Hello Marc,
Ping on this patch for feedback.
On 08/26/2018 05:20 PM, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, Parthiban Nallathambi wrote:
Actions Semi Owl family SoC's S500, S700 and S900 provides support
for 3 external interrupt controllers through SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . Each line can also be masked independently.
Signed-off-by: Parthiban Nallathambi
Signed-off-by: Saravanan Sekar
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 305
+
2 files changed, 306 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..072c4409e7c4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-owl-sirq.c
b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index ..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu
+ *
+ * Author: Parthiban Nallathambi
+ * Author: Saravanan Sekar
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So
maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different
name)
for offsetting?
+#define INTC_EXTCTL_PENDING BIT(0)
+#define INTC_EXTCTL_CLK_SEL BIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
+#define INTC_EXTCTL_TYPE_HIGH 0
+#define INTC_EXTCTL_TYPE_LOW BIT(6)
+#define INTC_EXTCTL_TYPE_RISING BIT(7)
+#define INTC_EXTCTL_TYPE_FALLING (BIT(6) | BIT(7))
+
+#define get_sirq_offset(x) chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ u16 offset;
+ /* software is responsible to clear interrupt pending bit when
+ * type is edge triggered. This value is for per SIRQ line.
+ */
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
Sure, thanks.
+ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ /* some SoC's share the register for all SIRQ lines, so maintain
+ * register is shared or not here. This value is from DT.
+ */
+ bool shared_reg;
+ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
Sure, will adapt this.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ val = readl_relaxed(chip_data->base +
get_sirq_offset(data->hwirq));
+ if (chip_data->shared_reg)
+ val = (val >> (2 - data->hwirq) * 8) & 0xff;
+
+ return val;
+}
+
+static void sirq_write_extctl(struct irq_data *data, unsigned int
extctl)
Same comments.
Sure.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ if (chip_data->shared_reg) {
+ val = readl_relaxed(chip_data->base +
+ get_sirq_offset(data->hwirq));
Single line, please.
Sure.
+ val &= ~(0xff << (2 -
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
Hello Marc,
Ping on this patch for feedback.
On 08/26/2018 05:20 PM, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, Parthiban Nallathambi wrote:
Actions Semi Owl family SoC's S500, S700 and S900 provides support
for 3 external interrupt controllers through SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . Each line can also be masked independently.
Signed-off-by: Parthiban Nallathambi
Signed-off-by: Saravanan Sekar
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 305 +
2 files changed, 306 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..072c4409e7c4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2836.o
obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
obj-$(CONFIG_ARCH_LPC32XX)+= irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index ..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu
+ *
+ * Author: Parthiban Nallathambi
+ * Author: Saravanan Sekar
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different name)
for offsetting?
+#define INTC_EXTCTL_PENDINGBIT(0)
+#define INTC_EXTCTL_CLK_SELBIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#defineINTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
+#defineINTC_EXTCTL_TYPE_HIGH 0
+#defineINTC_EXTCTL_TYPE_LOWBIT(6)
+#defineINTC_EXTCTL_TYPE_RISING BIT(7)
+#defineINTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
+
+#define get_sirq_offset(x) chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ u16 offset;
+ /* software is responsible to clear interrupt pending bit when
+* type is edge triggered. This value is for per SIRQ line.
+*/
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
Sure, thanks.
+ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ /* some SoC's share the register for all SIRQ lines, so maintain
+* register is shared or not here. This value is from DT.
+*/
+ bool shared_reg;
+ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
Sure, will adapt this.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ val = readl_relaxed(chip_data->base + get_sirq_offset(data->hwirq));
+ if (chip_data->shared_reg)
+ val = (val >> (2 - data->hwirq) * 8) & 0xff;
+
+ return val;
+}
+
+static void sirq_write_extctl(struct irq_data *data, unsigned int extctl)
Same comments.
Sure.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ if (chip_data->shared_reg) {
+ val = readl_relaxed(chip_data->base +
+
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
Hello Marc,
Ping on this patch for feedback.
On 08/26/2018 05:20 PM, Parthiban Nallathambi wrote:
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
On 12/08/18 13:22, Parthiban Nallathambi wrote:
Actions Semi Owl family SoC's S500, S700 and S900 provides support
for 3 external interrupt controllers through SIRQ pins.
Each line can be independently configured as interrupt and triggers
on either of the edges (raising or falling) or either of the levels
(high or low) . Each line can also be masked independently.
Signed-off-by: Parthiban Nallathambi
Signed-off-by: Saravanan Sekar
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-owl-sirq.c | 305 +
2 files changed, 306 insertions(+)
create mode 100644 drivers/irqchip/irq-owl-sirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..072c4409e7c4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2835.o
obj-$(CONFIG_ARCH_BCM2835)+= irq-bcm2836.o
obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
+obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
obj-$(CONFIG_ARCH_LPC32XX)+= irq-lpc32xx.o
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
new file mode 100644
index ..b69301388300
--- /dev/null
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *
+ * Actions Semi Owl SoCs SIRQ interrupt controller driver
+ *
+ * Copyright (C) 2014 Actions Semi Inc.
+ * David Liu
+ *
+ * Author: Parthiban Nallathambi
+ * Author: Saravanan Sekar
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different name)
for offsetting?
+#define INTC_EXTCTL_PENDINGBIT(0)
+#define INTC_EXTCTL_CLK_SELBIT(4)
+#define INTC_EXTCTL_EN BIT(5)
+#defineINTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
+#defineINTC_EXTCTL_TYPE_HIGH 0
+#defineINTC_EXTCTL_TYPE_LOWBIT(6)
+#defineINTC_EXTCTL_TYPE_RISING BIT(7)
+#defineINTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
+
+#define get_sirq_offset(x) chip_data->sirq[x].offset
+
+/* Per SIRQ data */
+struct owl_sirq {
+ u16 offset;
+ /* software is responsible to clear interrupt pending bit when
+* type is edge triggered. This value is for per SIRQ line.
+*/
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
Sure, thanks.
+ bool type_edge;
+};
+
+struct owl_sirq_chip_data {
+ void __iomem *base;
+ raw_spinlock_t lock;
+ /* some SoC's share the register for all SIRQ lines, so maintain
+* register is shared or not here. This value is from DT.
+*/
+ bool shared_reg;
+ struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
Sure, I will modify with one allocation.
+};
+
+static struct owl_sirq_chip_data *sirq_data;
+
+static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
Sure, will adapt this.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ val = readl_relaxed(chip_data->base + get_sirq_offset(data->hwirq));
+ if (chip_data->shared_reg)
+ val = (val >> (2 - data->hwirq) * 8) & 0xff;
+
+ return val;
+}
+
+static void sirq_write_extctl(struct irq_data *data, unsigned int extctl)
Same comments.
Sure.
+{
+ struct owl_sirq_chip_data *chip_data = data->chip_data;
+ unsigned int val;
u32;
Sure.
+
+ if (chip_data->shared_reg) {
+ val = readl_relaxed(chip_data->base +
+
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
> On 12/08/18 13:22, Parthiban Nallathambi wrote:
>> Actions Semi Owl family SoC's S500, S700 and S900 provides support
>> for 3 external interrupt controllers through SIRQ pins.
>>
>> Each line can be independently configured as interrupt and triggers
>> on either of the edges (raising or falling) or either of the levels
>> (high or low) . Each line can also be masked independently.
>>
>> Signed-off-by: Parthiban Nallathambi
>> Signed-off-by: Saravanan Sekar
>> ---
>> drivers/irqchip/Makefile | 1 +
>> drivers/irqchip/irq-owl-sirq.c | 305
>> +
>> 2 files changed, 306 insertions(+)
>> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15f268f646bf..072c4409e7c4 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
>> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
>> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
>> obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
>> obj-$(CONFIG_ARCH_HIP04)+= irq-hip04.o
>> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
>> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
>> new file mode 100644
>> index ..b69301388300
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-owl-sirq.c
>> @@ -0,0 +1,305 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + *
>> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
>> + *
>> + * Copyright (C) 2014 Actions Semi Inc.
>> + * David Liu
>> + *
>> + * Author: Parthiban Nallathambi
>> + * Author: Saravanan Sekar
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +#define INTC_GIC_INTERRUPT_PIN 13
>
> Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different name)
for offsetting?
>
>> +#define INTC_EXTCTL_PENDING BIT(0)
>> +#define INTC_EXTCTL_CLK_SEL BIT(4)
>> +#define INTC_EXTCTL_EN BIT(5)
>> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
>> +#define INTC_EXTCTL_TYPE_HIGH 0
>> +#define INTC_EXTCTL_TYPE_LOWBIT(6)
>> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
>> +#define INTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
>> +
>> +#define get_sirq_offset(x) chip_data->sirq[x].offset
>> +
>> +/* Per SIRQ data */
>> +struct owl_sirq {
>> +u16 offset;
>> +/* software is responsible to clear interrupt pending bit when
>> + * type is edge triggered. This value is for per SIRQ line.
>> + */
>
> Please follow the normal multi-line comment style:
>
> /*
> * This is a comment, starting with a capital letter and ending with
> * a full stop.
> */
Sure, thanks.
>
>> +bool type_edge;
>> +};
>> +
>> +struct owl_sirq_chip_data {
>> +void __iomem *base;
>> +raw_spinlock_t lock;
>> +/* some SoC's share the register for all SIRQ lines, so maintain
>> + * register is shared or not here. This value is from DT.
>> + */
>> +bool shared_reg;
>> +struct owl_sirq *sirq;
>
> Given that this driver handles at most 3 interrupts, do we need the
> overhead of a pointer and an additional allocation, while we could store
> all of this data in the space taken by the pointer itself?
>
> Something like:
>
> u16 offset[3];
> u8 trigger; // Bit mask indicating edge-triggered interrupts
>
> and we're done.
Sure, I will modify with one allocation.
>
>> +};
>> +
>> +static struct owl_sirq_chip_data *sirq_data;
>> +
>> +static unsigned int sirq_read_extctl(struct irq_data *data)
>
> Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
> of always passing irq_data?
>
> Also, this should return a well defined size, which "unsigned int"
> isn't. Make that u32.
Sure, will adapt this.
>
>> +{
>> +struct owl_sirq_chip_data *chip_data = data->chip_data;
>> +unsigned int val;
>
> u32;
Sure.
>
>> +
>> +val = readl_relaxed(chip_data->base + get_sirq_offset(data->hwirq));
>> +if (chip_data->shared_reg)
>> +val = (val >> (2 - data->hwirq) * 8) & 0xff;
>> +
>> +return val;
>> +}
>> +
>> +static void sirq_write_extctl(struct irq_data *data, unsigned int extctl)
>
> Same comments.
Sure.
>
>> +{
>> +struct owl_sirq_chip_data *chip_data = data->chip_data;
>> +unsigned int val;
>
> u32;
Sure.
>
>> +
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
Hello Marc,
Thanks for your feedback.
On 8/13/18 1:46 PM, Marc Zyngier wrote:
> On 12/08/18 13:22, Parthiban Nallathambi wrote:
>> Actions Semi Owl family SoC's S500, S700 and S900 provides support
>> for 3 external interrupt controllers through SIRQ pins.
>>
>> Each line can be independently configured as interrupt and triggers
>> on either of the edges (raising or falling) or either of the levels
>> (high or low) . Each line can also be masked independently.
>>
>> Signed-off-by: Parthiban Nallathambi
>> Signed-off-by: Saravanan Sekar
>> ---
>> drivers/irqchip/Makefile | 1 +
>> drivers/irqchip/irq-owl-sirq.c | 305
>> +
>> 2 files changed, 306 insertions(+)
>> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15f268f646bf..072c4409e7c4 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
>> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
>> obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o
>> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
>> obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
>> obj-$(CONFIG_ARCH_HIP04)+= irq-hip04.o
>> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
>> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
>> new file mode 100644
>> index ..b69301388300
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-owl-sirq.c
>> @@ -0,0 +1,305 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + *
>> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
>> + *
>> + * Copyright (C) 2014 Actions Semi Inc.
>> + * David Liu
>> + *
>> + * Author: Parthiban Nallathambi
>> + * Author: Saravanan Sekar
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +#define INTC_GIC_INTERRUPT_PIN 13
>
> Why isn't that coming from the DT?
DT numbering is taken irqchip local, by which hwirq is directly used to
calculate the offset into register when it is shared. Even if this is
directly from DT, I need the value to offset into the register. So maintianed
inside the driver.
Should it make sense to move it to DT and use another macro (different name)
for offsetting?
>
>> +#define INTC_EXTCTL_PENDING BIT(0)
>> +#define INTC_EXTCTL_CLK_SEL BIT(4)
>> +#define INTC_EXTCTL_EN BIT(5)
>> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
>> +#define INTC_EXTCTL_TYPE_HIGH 0
>> +#define INTC_EXTCTL_TYPE_LOWBIT(6)
>> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
>> +#define INTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
>> +
>> +#define get_sirq_offset(x) chip_data->sirq[x].offset
>> +
>> +/* Per SIRQ data */
>> +struct owl_sirq {
>> +u16 offset;
>> +/* software is responsible to clear interrupt pending bit when
>> + * type is edge triggered. This value is for per SIRQ line.
>> + */
>
> Please follow the normal multi-line comment style:
>
> /*
> * This is a comment, starting with a capital letter and ending with
> * a full stop.
> */
Sure, thanks.
>
>> +bool type_edge;
>> +};
>> +
>> +struct owl_sirq_chip_data {
>> +void __iomem *base;
>> +raw_spinlock_t lock;
>> +/* some SoC's share the register for all SIRQ lines, so maintain
>> + * register is shared or not here. This value is from DT.
>> + */
>> +bool shared_reg;
>> +struct owl_sirq *sirq;
>
> Given that this driver handles at most 3 interrupts, do we need the
> overhead of a pointer and an additional allocation, while we could store
> all of this data in the space taken by the pointer itself?
>
> Something like:
>
> u16 offset[3];
> u8 trigger; // Bit mask indicating edge-triggered interrupts
>
> and we're done.
Sure, I will modify with one allocation.
>
>> +};
>> +
>> +static struct owl_sirq_chip_data *sirq_data;
>> +
>> +static unsigned int sirq_read_extctl(struct irq_data *data)
>
> Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
> of always passing irq_data?
>
> Also, this should return a well defined size, which "unsigned int"
> isn't. Make that u32.
Sure, will adapt this.
>
>> +{
>> +struct owl_sirq_chip_data *chip_data = data->chip_data;
>> +unsigned int val;
>
> u32;
Sure.
>
>> +
>> +val = readl_relaxed(chip_data->base + get_sirq_offset(data->hwirq));
>> +if (chip_data->shared_reg)
>> +val = (val >> (2 - data->hwirq) * 8) & 0xff;
>> +
>> +return val;
>> +}
>> +
>> +static void sirq_write_extctl(struct irq_data *data, unsigned int extctl)
>
> Same comments.
Sure.
>
>> +{
>> +struct owl_sirq_chip_data *chip_data = data->chip_data;
>> +unsigned int val;
>
> u32;
Sure.
>
>> +
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On 12/08/18 13:22, Parthiban Nallathambi wrote:
> Actions Semi Owl family SoC's S500, S700 and S900 provides support
> for 3 external interrupt controllers through SIRQ pins.
>
> Each line can be independently configured as interrupt and triggers
> on either of the edges (raising or falling) or either of the levels
> (high or low) . Each line can also be masked independently.
>
> Signed-off-by: Parthiban Nallathambi
> Signed-off-by: Saravanan Sekar
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 305
> +
> 2 files changed, 306 insertions(+)
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..072c4409e7c4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> obj-$(CONFIG_ARCH_EXYNOS)+= exynos-combiner.o
> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
> new file mode 100644
> index ..b69301388300
> --- /dev/null
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *
> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> + *
> + * Copyright (C) 2014 Actions Semi Inc.
> + * David Liu
> + *
> + * Author: Parthiban Nallathambi
> + * Author: Saravanan Sekar
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
> +#define INTC_EXTCTL_PENDING BIT(0)
> +#define INTC_EXTCTL_CLK_SEL BIT(4)
> +#define INTC_EXTCTL_EN BIT(5)
> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
> +#define INTC_EXTCTL_TYPE_HIGH 0
> +#define INTC_EXTCTL_TYPE_LOWBIT(6)
> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> +#define INTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
> +
> +#define get_sirq_offset(x) chip_data->sirq[x].offset
> +
> +/* Per SIRQ data */
> +struct owl_sirq {
> + u16 offset;
> + /* software is responsible to clear interrupt pending bit when
> + * type is edge triggered. This value is for per SIRQ line.
> + */
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
> + bool type_edge;
> +};
> +
> +struct owl_sirq_chip_data {
> + void __iomem *base;
> + raw_spinlock_t lock;
> + /* some SoC's share the register for all SIRQ lines, so maintain
> + * register is shared or not here. This value is from DT.
> + */
> + bool shared_reg;
> + struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
> +};
> +
> +static struct owl_sirq_chip_data *sirq_data;
> +
> +static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
> +{
> + struct owl_sirq_chip_data *chip_data = data->chip_data;
> + unsigned int val;
u32;
> +
> + val = readl_relaxed(chip_data->base + get_sirq_offset(data->hwirq));
> + if (chip_data->shared_reg)
> + val = (val >> (2 - data->hwirq) * 8) & 0xff;
> +
> + return val;
> +}
> +
> +static void sirq_write_extctl(struct irq_data *data, unsigned int extctl)
Same comments.
> +{
> + struct owl_sirq_chip_data *chip_data = data->chip_data;
> + unsigned int val;
u32;
> +
> + if (chip_data->shared_reg) {
> + val = readl_relaxed(chip_data->base +
> + get_sirq_offset(data->hwirq));
Single line, please.
> + val &= ~(0xff << (2 - data->hwirq) * 8);
> + extctl &= 0xff;
> + extctl = (extctl << (2 - data->hwirq) * 8) | val;
> + }
> +
> + writel_relaxed(extctl, chip_data->base +
> + get_sirq_offset(data->hwirq));
Single line.
> +}
> +
> +static void owl_sirq_ack(struct irq_data *data)
> +{
> + struct owl_sirq_chip_data *chip_data = data->chip_data;
> + unsigned int extctl;
> + unsigned long flags;
> +
> +
Re: [PATCH v2 2/3] drivers/irqchip: Add Actions external interrupts support
On 12/08/18 13:22, Parthiban Nallathambi wrote:
> Actions Semi Owl family SoC's S500, S700 and S900 provides support
> for 3 external interrupt controllers through SIRQ pins.
>
> Each line can be independently configured as interrupt and triggers
> on either of the edges (raising or falling) or either of the levels
> (high or low) . Each line can also be masked independently.
>
> Signed-off-by: Parthiban Nallathambi
> Signed-off-by: Saravanan Sekar
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-owl-sirq.c | 305
> +
> 2 files changed, 306 insertions(+)
> create mode 100644 drivers/irqchip/irq-owl-sirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..072c4409e7c4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ATH79) += irq-ath79-misc.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o
> obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o
> obj-$(CONFIG_ARCH_EXYNOS)+= exynos-combiner.o
> +obj-$(CONFIG_ARCH_ACTIONS) += irq-owl-sirq.o
> obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o
> obj-$(CONFIG_ARCH_HIP04) += irq-hip04.o
> obj-$(CONFIG_ARCH_LPC32XX) += irq-lpc32xx.o
> diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
> new file mode 100644
> index ..b69301388300
> --- /dev/null
> +++ b/drivers/irqchip/irq-owl-sirq.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *
> + * Actions Semi Owl SoCs SIRQ interrupt controller driver
> + *
> + * Copyright (C) 2014 Actions Semi Inc.
> + * David Liu
> + *
> + * Author: Parthiban Nallathambi
> + * Author: Saravanan Sekar
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define INTC_GIC_INTERRUPT_PIN 13
Why isn't that coming from the DT?
> +#define INTC_EXTCTL_PENDING BIT(0)
> +#define INTC_EXTCTL_CLK_SEL BIT(4)
> +#define INTC_EXTCTL_EN BIT(5)
> +#define INTC_EXTCTL_TYPE_MASK GENMASK(6, 7)
> +#define INTC_EXTCTL_TYPE_HIGH 0
> +#define INTC_EXTCTL_TYPE_LOWBIT(6)
> +#define INTC_EXTCTL_TYPE_RISING BIT(7)
> +#define INTC_EXTCTL_TYPE_FALLING(BIT(6) | BIT(7))
> +
> +#define get_sirq_offset(x) chip_data->sirq[x].offset
> +
> +/* Per SIRQ data */
> +struct owl_sirq {
> + u16 offset;
> + /* software is responsible to clear interrupt pending bit when
> + * type is edge triggered. This value is for per SIRQ line.
> + */
Please follow the normal multi-line comment style:
/*
* This is a comment, starting with a capital letter and ending with
* a full stop.
*/
> + bool type_edge;
> +};
> +
> +struct owl_sirq_chip_data {
> + void __iomem *base;
> + raw_spinlock_t lock;
> + /* some SoC's share the register for all SIRQ lines, so maintain
> + * register is shared or not here. This value is from DT.
> + */
> + bool shared_reg;
> + struct owl_sirq *sirq;
Given that this driver handles at most 3 interrupts, do we need the
overhead of a pointer and an additional allocation, while we could store
all of this data in the space taken by the pointer itself?
Something like:
u16 offset[3];
u8 trigger; // Bit mask indicating edge-triggered interrupts
and we're done.
> +};
> +
> +static struct owl_sirq_chip_data *sirq_data;
> +
> +static unsigned int sirq_read_extctl(struct irq_data *data)
Why isn't this taking a struct owl_sirq_chip_data as a parameter instead
of always passing irq_data?
Also, this should return a well defined size, which "unsigned int"
isn't. Make that u32.
> +{
> + struct owl_sirq_chip_data *chip_data = data->chip_data;
> + unsigned int val;
u32;
> +
> + val = readl_relaxed(chip_data->base + get_sirq_offset(data->hwirq));
> + if (chip_data->shared_reg)
> + val = (val >> (2 - data->hwirq) * 8) & 0xff;
> +
> + return val;
> +}
> +
> +static void sirq_write_extctl(struct irq_data *data, unsigned int extctl)
Same comments.
> +{
> + struct owl_sirq_chip_data *chip_data = data->chip_data;
> + unsigned int val;
u32;
> +
> + if (chip_data->shared_reg) {
> + val = readl_relaxed(chip_data->base +
> + get_sirq_offset(data->hwirq));
Single line, please.
> + val &= ~(0xff << (2 - data->hwirq) * 8);
> + extctl &= 0xff;
> + extctl = (extctl << (2 - data->hwirq) * 8) | val;
> + }
> +
> + writel_relaxed(extctl, chip_data->base +
> + get_sirq_offset(data->hwirq));
Single line.
> +}
> +
> +static void owl_sirq_ack(struct irq_data *data)
> +{
> + struct owl_sirq_chip_data *chip_data = data->chip_data;
> + unsigned int extctl;
> + unsigned long flags;
> +
> +
