* [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-05 14:05 ` Mubin Usman Sayyed
0 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-05 14:05 UTC (permalink / raw)
To: tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: linux-kernel, siva.durga.paladugu, anirudha.sarangi, Mubin Sayyed
From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
This patch adds support for multiple instances of
xilinx interrupt controller. Below configurations are
supported by driver,
- peripheral->xilinx-intc->xilinx-intc->gic
- peripheral->xilinx-intc->xilinx-intc
Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
---
Changes for v2:
- Removed write_fn/read_fn hooks, used xintc_write/
xintc_read directly
- Moved primary_intc declaration after xintc_irq_chip
---
drivers/irqchip/irq-xilinx-intc.c | 121 +++++++++++++++++++++++---------------
1 file changed, 73 insertions(+), 48 deletions(-)
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index e3043de..14cb630 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -38,29 +38,32 @@ struct xintc_irq_chip {
void __iomem *base;
struct irq_domain *root_domain;
u32 intr_mask;
+ struct irq_chip *intc_dev;
+ u32 nr_irq;
};
-static struct xintc_irq_chip *xintc_irqc;
+static struct xintc_irq_chip *primary_intc;
-static void xintc_write(int reg, u32 data)
+static void xintc_write(void __iomem *addr, u32 data)
{
if (static_branch_unlikely(&xintc_is_be))
- iowrite32be(data, xintc_irqc->base + reg);
+ iowrite32be(data, addr);
else
- iowrite32(data, xintc_irqc->base + reg);
+ iowrite32(data, addr);
}
-static unsigned int xintc_read(int reg)
+static unsigned int xintc_read(void __iomem *addr)
{
if (static_branch_unlikely(&xintc_is_be))
- return ioread32be(xintc_irqc->base + reg);
+ return ioread32be(addr);
else
- return ioread32(xintc_irqc->base + reg);
+ return ioread32(addr);
}
static void intc_enable_or_unmask(struct irq_data *d)
{
unsigned long mask = 1 << d->hwirq;
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
@@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data *d)
* acks the irq before calling the interrupt handler
*/
if (irqd_is_level_type(d))
- xintc_write(IAR, mask);
+ xintc_write(local_intc->base + IAR, mask);
- xintc_write(SIE, mask);
+ xintc_write(local_intc->base + SIE, mask);
}
static void intc_disable_or_mask(struct irq_data *d)
{
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
+
pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
- xintc_write(CIE, 1 << d->hwirq);
+ xintc_write(local_intc->base + CIE, 1 << d->hwirq);
}
static void intc_ack(struct irq_data *d)
{
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
+
pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
- xintc_write(IAR, 1 << d->hwirq);
+ xintc_write(local_intc->base + IAR, 1 << d->hwirq);
}
static void intc_mask_ack(struct irq_data *d)
{
unsigned long mask = 1 << d->hwirq;
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
- xintc_write(CIE, mask);
- xintc_write(IAR, mask);
+ xintc_write(local_intc->base + CIE, mask);
+ xintc_write(local_intc->base + IAR, mask);
}
-static struct irq_chip intc_dev = {
- .name = "Xilinx INTC",
- .irq_unmask = intc_enable_or_unmask,
- .irq_mask = intc_disable_or_mask,
- .irq_ack = intc_ack,
- .irq_mask_ack = intc_mask_ack,
-};
+static unsigned int xintc_get_irq_local(struct xintc_irq_chip *local_intc)
+{
+ int hwirq, irq = -1;
+
+ hwirq = xintc_read(local_intc->base + IVR);
+ if (hwirq != -1U)
+ irq = irq_find_mapping(local_intc->root_domain, hwirq);
+
+ pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
+
+ return irq;
+}
unsigned int xintc_get_irq(void)
{
- unsigned int hwirq, irq = -1;
+ int hwirq, irq = -1;
- hwirq = xintc_read(IVR);
+ hwirq = xintc_read(primary_intc->base + IVR);
if (hwirq != -1U)
- irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
+ irq = irq_find_mapping(primary_intc->root_domain, hwirq);
pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
@@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
{
- if (xintc_irqc->intr_mask & (1 << hw)) {
- irq_set_chip_and_handler_name(irq, &intc_dev,
+ struct xintc_irq_chip *local_intc = d->host_data;
+
+ if (local_intc->intr_mask & (1 << hw)) {
+ irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
handle_edge_irq, "edge");
irq_clear_status_flags(irq, IRQ_LEVEL);
} else {
- irq_set_chip_and_handler_name(irq, &intc_dev,
+ irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
handle_level_irq, "level");
irq_set_status_flags(irq, IRQ_LEVEL);
}
+ irq_set_chip_data(irq, local_intc);
return 0;
}
@@ -138,11 +154,13 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
static void xil_intc_irq_handler(struct irq_desc *desc)
{
struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct xintc_irq_chip *local_intc =
+ irq_data_get_irq_handler_data(&desc->irq_data);
u32 pending;
chained_irq_enter(chip, desc);
do {
- pending = xintc_get_irq();
+ pending = xintc_get_irq_local(local_intc);
if (pending == -1U)
break;
generic_handle_irq(pending);
@@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
static int __init xilinx_intc_of_init(struct device_node *intc,
struct device_node *parent)
{
- u32 nr_irq;
int ret, irq;
struct xintc_irq_chip *irqc;
-
- if (xintc_irqc) {
- pr_err("irq-xilinx: Multiple instances aren't supported\n");
- return -EINVAL;
- }
+ struct irq_chip *intc_dev;
irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
if (!irqc)
return -ENOMEM;
-
- xintc_irqc = irqc;
-
irqc->base = of_iomap(intc, 0);
BUG_ON(!irqc->base);
- ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
+ ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &irqc->nr_irq);
if (ret < 0) {
pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
- goto err_alloc;
+ goto error;
}
ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
@@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
irqc->intr_mask = 0;
}
- if (irqc->intr_mask >> nr_irq)
+ if (irqc->intr_mask >> irqc->nr_irq)
pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
- intc, nr_irq, irqc->intr_mask);
+ intc, irqc->nr_irq, irqc->intr_mask);
+
+ intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
+ if (!intc_dev) {
+ ret = -ENOMEM;
+ goto error;
+ }
+ intc_dev->name = intc->full_name;
+ intc_dev->irq_unmask = intc_enable_or_unmask,
+ intc_dev->irq_mask = intc_disable_or_mask,
+ intc_dev->irq_ack = intc_ack,
+ intc_dev->irq_mask_ack = intc_mask_ack,
+ irqc->intc_dev = intc_dev;
/*
* Disable all external interrupts until they are
* explicity requested.
*/
- xintc_write(IER, 0);
+ xintc_write(irqc->base + IER, 0);
/* Acknowledge any pending interrupts just in case. */
- xintc_write(IAR, 0xffffffff);
+ xintc_write(irqc->base + IAR, 0xffffffff);
/* Turn on the Master Enable. */
- xintc_write(MER, MER_HIE | MER_ME);
- if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
+ xintc_write(irqc->base + MER, MER_HIE | MER_ME);
+ if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
static_branch_enable(&xintc_is_be);
- xintc_write(MER, MER_HIE | MER_ME);
+ xintc_write(irqc->base + MER, MER_HIE | MER_ME);
}
- irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
+ irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
&xintc_irq_domain_ops, irqc);
if (!irqc->root_domain) {
pr_err("irq-xilinx: Unable to create IRQ domain\n");
@@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
goto err_alloc;
}
} else {
- irq_set_default_host(irqc->root_domain);
+ primary_intc = irqc;
+ irq_set_default_host(primary_intc->root_domain);
}
return 0;
err_alloc:
- xintc_irqc = NULL;
+ kfree(intc_dev);
+error:
+ iounmap(irqc->base);
kfree(irqc);
return ret;
--
2.7.4
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-05 14:05 ` Mubin Usman Sayyed
0 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-05 14:05 UTC (permalink / raw)
To: tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: Mubin Sayyed, anirudha.sarangi, siva.durga.paladugu, linux-kernel
From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
This patch adds support for multiple instances of
xilinx interrupt controller. Below configurations are
supported by driver,
- peripheral->xilinx-intc->xilinx-intc->gic
- peripheral->xilinx-intc->xilinx-intc
Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
---
Changes for v2:
- Removed write_fn/read_fn hooks, used xintc_write/
xintc_read directly
- Moved primary_intc declaration after xintc_irq_chip
---
drivers/irqchip/irq-xilinx-intc.c | 121 +++++++++++++++++++++++---------------
1 file changed, 73 insertions(+), 48 deletions(-)
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
index e3043de..14cb630 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -38,29 +38,32 @@ struct xintc_irq_chip {
void __iomem *base;
struct irq_domain *root_domain;
u32 intr_mask;
+ struct irq_chip *intc_dev;
+ u32 nr_irq;
};
-static struct xintc_irq_chip *xintc_irqc;
+static struct xintc_irq_chip *primary_intc;
-static void xintc_write(int reg, u32 data)
+static void xintc_write(void __iomem *addr, u32 data)
{
if (static_branch_unlikely(&xintc_is_be))
- iowrite32be(data, xintc_irqc->base + reg);
+ iowrite32be(data, addr);
else
- iowrite32(data, xintc_irqc->base + reg);
+ iowrite32(data, addr);
}
-static unsigned int xintc_read(int reg)
+static unsigned int xintc_read(void __iomem *addr)
{
if (static_branch_unlikely(&xintc_is_be))
- return ioread32be(xintc_irqc->base + reg);
+ return ioread32be(addr);
else
- return ioread32(xintc_irqc->base + reg);
+ return ioread32(addr);
}
static void intc_enable_or_unmask(struct irq_data *d)
{
unsigned long mask = 1 << d->hwirq;
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
@@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data *d)
* acks the irq before calling the interrupt handler
*/
if (irqd_is_level_type(d))
- xintc_write(IAR, mask);
+ xintc_write(local_intc->base + IAR, mask);
- xintc_write(SIE, mask);
+ xintc_write(local_intc->base + SIE, mask);
}
static void intc_disable_or_mask(struct irq_data *d)
{
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
+
pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
- xintc_write(CIE, 1 << d->hwirq);
+ xintc_write(local_intc->base + CIE, 1 << d->hwirq);
}
static void intc_ack(struct irq_data *d)
{
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
+
pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
- xintc_write(IAR, 1 << d->hwirq);
+ xintc_write(local_intc->base + IAR, 1 << d->hwirq);
}
static void intc_mask_ack(struct irq_data *d)
{
unsigned long mask = 1 << d->hwirq;
+ struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
- xintc_write(CIE, mask);
- xintc_write(IAR, mask);
+ xintc_write(local_intc->base + CIE, mask);
+ xintc_write(local_intc->base + IAR, mask);
}
-static struct irq_chip intc_dev = {
- .name = "Xilinx INTC",
- .irq_unmask = intc_enable_or_unmask,
- .irq_mask = intc_disable_or_mask,
- .irq_ack = intc_ack,
- .irq_mask_ack = intc_mask_ack,
-};
+static unsigned int xintc_get_irq_local(struct xintc_irq_chip *local_intc)
+{
+ int hwirq, irq = -1;
+
+ hwirq = xintc_read(local_intc->base + IVR);
+ if (hwirq != -1U)
+ irq = irq_find_mapping(local_intc->root_domain, hwirq);
+
+ pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
+
+ return irq;
+}
unsigned int xintc_get_irq(void)
{
- unsigned int hwirq, irq = -1;
+ int hwirq, irq = -1;
- hwirq = xintc_read(IVR);
+ hwirq = xintc_read(primary_intc->base + IVR);
if (hwirq != -1U)
- irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
+ irq = irq_find_mapping(primary_intc->root_domain, hwirq);
pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
@@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
{
- if (xintc_irqc->intr_mask & (1 << hw)) {
- irq_set_chip_and_handler_name(irq, &intc_dev,
+ struct xintc_irq_chip *local_intc = d->host_data;
+
+ if (local_intc->intr_mask & (1 << hw)) {
+ irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
handle_edge_irq, "edge");
irq_clear_status_flags(irq, IRQ_LEVEL);
} else {
- irq_set_chip_and_handler_name(irq, &intc_dev,
+ irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
handle_level_irq, "level");
irq_set_status_flags(irq, IRQ_LEVEL);
}
+ irq_set_chip_data(irq, local_intc);
return 0;
}
@@ -138,11 +154,13 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
static void xil_intc_irq_handler(struct irq_desc *desc)
{
struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct xintc_irq_chip *local_intc =
+ irq_data_get_irq_handler_data(&desc->irq_data);
u32 pending;
chained_irq_enter(chip, desc);
do {
- pending = xintc_get_irq();
+ pending = xintc_get_irq_local(local_intc);
if (pending == -1U)
break;
generic_handle_irq(pending);
@@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
static int __init xilinx_intc_of_init(struct device_node *intc,
struct device_node *parent)
{
- u32 nr_irq;
int ret, irq;
struct xintc_irq_chip *irqc;
-
- if (xintc_irqc) {
- pr_err("irq-xilinx: Multiple instances aren't supported\n");
- return -EINVAL;
- }
+ struct irq_chip *intc_dev;
irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
if (!irqc)
return -ENOMEM;
-
- xintc_irqc = irqc;
-
irqc->base = of_iomap(intc, 0);
BUG_ON(!irqc->base);
- ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
+ ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &irqc->nr_irq);
if (ret < 0) {
pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
- goto err_alloc;
+ goto error;
}
ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
@@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
irqc->intr_mask = 0;
}
- if (irqc->intr_mask >> nr_irq)
+ if (irqc->intr_mask >> irqc->nr_irq)
pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
- intc, nr_irq, irqc->intr_mask);
+ intc, irqc->nr_irq, irqc->intr_mask);
+
+ intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
+ if (!intc_dev) {
+ ret = -ENOMEM;
+ goto error;
+ }
+ intc_dev->name = intc->full_name;
+ intc_dev->irq_unmask = intc_enable_or_unmask,
+ intc_dev->irq_mask = intc_disable_or_mask,
+ intc_dev->irq_ack = intc_ack,
+ intc_dev->irq_mask_ack = intc_mask_ack,
+ irqc->intc_dev = intc_dev;
/*
* Disable all external interrupts until they are
* explicity requested.
*/
- xintc_write(IER, 0);
+ xintc_write(irqc->base + IER, 0);
/* Acknowledge any pending interrupts just in case. */
- xintc_write(IAR, 0xffffffff);
+ xintc_write(irqc->base + IAR, 0xffffffff);
/* Turn on the Master Enable. */
- xintc_write(MER, MER_HIE | MER_ME);
- if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
+ xintc_write(irqc->base + MER, MER_HIE | MER_ME);
+ if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
static_branch_enable(&xintc_is_be);
- xintc_write(MER, MER_HIE | MER_ME);
+ xintc_write(irqc->base + MER, MER_HIE | MER_ME);
}
- irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
+ irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
&xintc_irq_domain_ops, irqc);
if (!irqc->root_domain) {
pr_err("irq-xilinx: Unable to create IRQ domain\n");
@@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
goto err_alloc;
}
} else {
- irq_set_default_host(irqc->root_domain);
+ primary_intc = irqc;
+ irq_set_default_host(primary_intc->root_domain);
}
return 0;
err_alloc:
- xintc_irqc = NULL;
+ kfree(intc_dev);
+error:
+ iounmap(irqc->base);
kfree(irqc);
return ret;
--
2.7.4
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 14:05 ` Mubin Usman Sayyed
@ 2020-02-05 14:15 ` David Laight
-1 siblings, 0 replies; 26+ messages in thread
From: David Laight @ 2020-02-05 14:15 UTC (permalink / raw)
To: 'Mubin Usman Sayyed',
tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: linux-kernel, siva.durga.paladugu, anirudha.sarangi
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s)
> confidential information that may be proprietary, privileged or copyrighted under applicable law. If
> you are not the intended recipient, do not read, copy, or forward this email message or any
> attachments. Delete this email message and any attachments immediately.
Deleted.....
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-05 14:15 ` David Laight
0 siblings, 0 replies; 26+ messages in thread
From: David Laight @ 2020-02-05 14:15 UTC (permalink / raw)
To: 'Mubin Usman Sayyed',
tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: anirudha.sarangi, siva.durga.paladugu, linux-kernel
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s)
> confidential information that may be proprietary, privileged or copyrighted under applicable law. If
> you are not the intended recipient, do not read, copy, or forward this email message or any
> attachments. Delete this email message and any attachments immediately.
Deleted.....
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 14:05 ` Mubin Usman Sayyed
@ 2020-02-05 14:53 ` Michal Simek
-1 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-05 14:53 UTC (permalink / raw)
To: Mubin Usman Sayyed, tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: linux-kernel, siva.durga.paladugu, anirudha.sarangi
Hi Mubin,
On 05. 02. 20 15:05, Mubin Usman Sayyed wrote:
> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>
> This patch adds support for multiple instances of
> xilinx interrupt controller. Below configurations are
> supported by driver,
>
> - peripheral->xilinx-intc->xilinx-intc->gic
> - peripheral->xilinx-intc->xilinx-intc
>
> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> ---
> Changes for v2:
> - Removed write_fn/read_fn hooks, used xintc_write/
> xintc_read directly
> - Moved primary_intc declaration after xintc_irq_chip
> ---
> drivers/irqchip/irq-xilinx-intc.c | 121 +++++++++++++++++++++++---------------
> 1 file changed, 73 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
> index e3043de..14cb630 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> void __iomem *base;
> struct irq_domain *root_domain;
> u32 intr_mask;
> + struct irq_chip *intc_dev;
> + u32 nr_irq;
indentation is weird.
> };
>
> -static struct xintc_irq_chip *xintc_irqc;
> +static struct xintc_irq_chip *primary_intc;
>
> -static void xintc_write(int reg, u32 data)
> +static void xintc_write(void __iomem *addr, u32 data)
The best would be if prototype is
static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data)
> {
> if (static_branch_unlikely(&xintc_is_be))
> - iowrite32be(data, xintc_irqc->base + reg);
> + iowrite32be(data, addr);
> else
> - iowrite32(data, xintc_irqc->base + reg);
> + iowrite32(data, addr);
> }
>
> -static unsigned int xintc_read(int reg)
> +static unsigned int xintc_read(void __iomem *addr)
And the same here.
static unsigned int xintc_read(struct xintc_irq_chip *irqc, int reg)
> {
> if (static_branch_unlikely(&xintc_is_be))
> - return ioread32be(xintc_irqc->base + reg);
> + return ioread32be(addr);
> else
> - return ioread32(xintc_irqc->base + reg);
> + return ioread32(addr);
> }
>
> static void intc_enable_or_unmask(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
code is using variable name as irqc below. I think that will be the best
to use it in all functions. It means s/local_intc/irqc/g'
>
> pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>
> @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data *d)
> * acks the irq before calling the interrupt handler
> */
> if (irqd_is_level_type(d))
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + IAR, mask);
>
> - xintc_write(SIE, mask);
> + xintc_write(local_intc->base + SIE, mask);
> }
>
> static void intc_disable_or_mask(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> - xintc_write(CIE, 1 << d->hwirq);
> + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> }
>
> static void intc_ack(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> - xintc_write(IAR, 1 << d->hwirq);
> + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> }
>
> static void intc_mask_ack(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
>
> pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> - xintc_write(CIE, mask);
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + CIE, mask);
> + xintc_write(local_intc->base + IAR, mask);
> }
>
> -static struct irq_chip intc_dev = {
> - .name = "Xilinx INTC",
> - .irq_unmask = intc_enable_or_unmask,
> - .irq_mask = intc_disable_or_mask,
> - .irq_ack = intc_ack,
> - .irq_mask_ack = intc_mask_ack,
> -};
> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *local_intc)
> +{
> + int hwirq, irq = -1;
> +
> + hwirq = xintc_read(local_intc->base + IVR);
> + if (hwirq != -1U)
> + irq = irq_find_mapping(local_intc->root_domain, hwirq);
> +
> + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +
> + return irq;
> +}
>
> unsigned int xintc_get_irq(void)
> {
> - unsigned int hwirq, irq = -1;
> + int hwirq, irq = -1;
>
> - hwirq = xintc_read(IVR);
> + hwirq = xintc_read(primary_intc->base + IVR);
> if (hwirq != -1U)
> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> + irq = irq_find_mapping(primary_intc->root_domain, hwirq);
>
> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>
> @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
>
> static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> {
> - if (xintc_irqc->intr_mask & (1 << hw)) {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + struct xintc_irq_chip *local_intc = d->host_data;
> +
> + if (local_intc->intr_mask & (1 << hw)) {
> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> handle_edge_irq, "edge");
> irq_clear_status_flags(irq, IRQ_LEVEL);
> } else {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> handle_level_irq, "level");
> irq_set_status_flags(irq, IRQ_LEVEL);
> }
> + irq_set_chip_data(irq, local_intc);
> return 0;
> }
>
> @@ -138,11 +154,13 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
> static void xil_intc_irq_handler(struct irq_desc *desc)
> {
> struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct xintc_irq_chip *local_intc =
> + irq_data_get_irq_handler_data(&desc->irq_data);
> u32 pending;
>
> chained_irq_enter(chip, desc);
> do {
> - pending = xintc_get_irq();
> + pending = xintc_get_irq_local(local_intc);
> if (pending == -1U)
> break;
> generic_handle_irq(pending);
> @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
> static int __init xilinx_intc_of_init(struct device_node *intc,
> struct device_node *parent)
> {
> - u32 nr_irq;
> int ret, irq;
> struct xintc_irq_chip *irqc;
> -
> - if (xintc_irqc) {
> - pr_err("irq-xilinx: Multiple instances aren't supported\n");
> - return -EINVAL;
> - }
> + struct irq_chip *intc_dev;
>
> irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> if (!irqc)
> return -ENOMEM;
> -
> - xintc_irqc = irqc;
> -
> irqc->base = of_iomap(intc, 0);
> BUG_ON(!irqc->base);
>
> - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
> + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &irqc->nr_irq);
> if (ret < 0) {
> pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
> - goto err_alloc;
> + goto error;
> }
>
> ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
> @@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
> irqc->intr_mask = 0;
> }
>
> - if (irqc->intr_mask >> nr_irq)
> + if (irqc->intr_mask >> irqc->nr_irq)
> pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
>
> pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> - intc, nr_irq, irqc->intr_mask);
> + intc, irqc->nr_irq, irqc->intr_mask);
> +
> + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> + if (!intc_dev) {
> + ret = -ENOMEM;
> + goto error;
> + }
>
> + intc_dev->name = intc->full_name;
> + intc_dev->irq_unmask = intc_enable_or_unmask,
> + intc_dev->irq_mask = intc_disable_or_mask,
> + intc_dev->irq_ack = intc_ack,
> + intc_dev->irq_mask_ack = intc_mask_ack,
> + irqc->intc_dev = intc_dev;
>
> /*
> * Disable all external interrupts until they are
> * explicity requested.
> */
> - xintc_write(IER, 0);
> + xintc_write(irqc->base + IER, 0);
>
> /* Acknowledge any pending interrupts just in case. */
> - xintc_write(IAR, 0xffffffff);
> + xintc_write(irqc->base + IAR, 0xffffffff);
>
> /* Turn on the Master Enable. */
> - xintc_write(MER, MER_HIE | MER_ME);
> - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> static_branch_enable(&xintc_is_be);
> - xintc_write(MER, MER_HIE | MER_ME);
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> }
>
> - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> &xintc_irq_domain_ops, irqc);
> if (!irqc->root_domain) {
> pr_err("irq-xilinx: Unable to create IRQ domain\n");
> @@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
> goto err_alloc;
> }
> } else {
> - irq_set_default_host(irqc->root_domain);
> + primary_intc = irqc;
> + irq_set_default_host(primary_intc->root_domain);
> }
>
> return 0;
>
> err_alloc:
> - xintc_irqc = NULL;
> + kfree(intc_dev);
> +error:
> + iounmap(irqc->base);
> kfree(irqc);
> return ret;
>
>
Thanks,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-05 14:53 ` Michal Simek
0 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-05 14:53 UTC (permalink / raw)
To: Mubin Usman Sayyed, tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: anirudha.sarangi, siva.durga.paladugu, linux-kernel
Hi Mubin,
On 05. 02. 20 15:05, Mubin Usman Sayyed wrote:
> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>
> This patch adds support for multiple instances of
> xilinx interrupt controller. Below configurations are
> supported by driver,
>
> - peripheral->xilinx-intc->xilinx-intc->gic
> - peripheral->xilinx-intc->xilinx-intc
>
> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> ---
> Changes for v2:
> - Removed write_fn/read_fn hooks, used xintc_write/
> xintc_read directly
> - Moved primary_intc declaration after xintc_irq_chip
> ---
> drivers/irqchip/irq-xilinx-intc.c | 121 +++++++++++++++++++++++---------------
> 1 file changed, 73 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
> index e3043de..14cb630 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> void __iomem *base;
> struct irq_domain *root_domain;
> u32 intr_mask;
> + struct irq_chip *intc_dev;
> + u32 nr_irq;
indentation is weird.
> };
>
> -static struct xintc_irq_chip *xintc_irqc;
> +static struct xintc_irq_chip *primary_intc;
>
> -static void xintc_write(int reg, u32 data)
> +static void xintc_write(void __iomem *addr, u32 data)
The best would be if prototype is
static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data)
> {
> if (static_branch_unlikely(&xintc_is_be))
> - iowrite32be(data, xintc_irqc->base + reg);
> + iowrite32be(data, addr);
> else
> - iowrite32(data, xintc_irqc->base + reg);
> + iowrite32(data, addr);
> }
>
> -static unsigned int xintc_read(int reg)
> +static unsigned int xintc_read(void __iomem *addr)
And the same here.
static unsigned int xintc_read(struct xintc_irq_chip *irqc, int reg)
> {
> if (static_branch_unlikely(&xintc_is_be))
> - return ioread32be(xintc_irqc->base + reg);
> + return ioread32be(addr);
> else
> - return ioread32(xintc_irqc->base + reg);
> + return ioread32(addr);
> }
>
> static void intc_enable_or_unmask(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
code is using variable name as irqc below. I think that will be the best
to use it in all functions. It means s/local_intc/irqc/g'
>
> pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>
> @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data *d)
> * acks the irq before calling the interrupt handler
> */
> if (irqd_is_level_type(d))
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + IAR, mask);
>
> - xintc_write(SIE, mask);
> + xintc_write(local_intc->base + SIE, mask);
> }
>
> static void intc_disable_or_mask(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> - xintc_write(CIE, 1 << d->hwirq);
> + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> }
>
> static void intc_ack(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> - xintc_write(IAR, 1 << d->hwirq);
> + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> }
>
> static void intc_mask_ack(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
>
> pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> - xintc_write(CIE, mask);
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + CIE, mask);
> + xintc_write(local_intc->base + IAR, mask);
> }
>
> -static struct irq_chip intc_dev = {
> - .name = "Xilinx INTC",
> - .irq_unmask = intc_enable_or_unmask,
> - .irq_mask = intc_disable_or_mask,
> - .irq_ack = intc_ack,
> - .irq_mask_ack = intc_mask_ack,
> -};
> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *local_intc)
> +{
> + int hwirq, irq = -1;
> +
> + hwirq = xintc_read(local_intc->base + IVR);
> + if (hwirq != -1U)
> + irq = irq_find_mapping(local_intc->root_domain, hwirq);
> +
> + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +
> + return irq;
> +}
>
> unsigned int xintc_get_irq(void)
> {
> - unsigned int hwirq, irq = -1;
> + int hwirq, irq = -1;
>
> - hwirq = xintc_read(IVR);
> + hwirq = xintc_read(primary_intc->base + IVR);
> if (hwirq != -1U)
> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> + irq = irq_find_mapping(primary_intc->root_domain, hwirq);
>
> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>
> @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
>
> static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> {
> - if (xintc_irqc->intr_mask & (1 << hw)) {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + struct xintc_irq_chip *local_intc = d->host_data;
> +
> + if (local_intc->intr_mask & (1 << hw)) {
> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> handle_edge_irq, "edge");
> irq_clear_status_flags(irq, IRQ_LEVEL);
> } else {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> handle_level_irq, "level");
> irq_set_status_flags(irq, IRQ_LEVEL);
> }
> + irq_set_chip_data(irq, local_intc);
> return 0;
> }
>
> @@ -138,11 +154,13 @@ static const struct irq_domain_ops xintc_irq_domain_ops = {
> static void xil_intc_irq_handler(struct irq_desc *desc)
> {
> struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct xintc_irq_chip *local_intc =
> + irq_data_get_irq_handler_data(&desc->irq_data);
> u32 pending;
>
> chained_irq_enter(chip, desc);
> do {
> - pending = xintc_get_irq();
> + pending = xintc_get_irq_local(local_intc);
> if (pending == -1U)
> break;
> generic_handle_irq(pending);
> @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
> static int __init xilinx_intc_of_init(struct device_node *intc,
> struct device_node *parent)
> {
> - u32 nr_irq;
> int ret, irq;
> struct xintc_irq_chip *irqc;
> -
> - if (xintc_irqc) {
> - pr_err("irq-xilinx: Multiple instances aren't supported\n");
> - return -EINVAL;
> - }
> + struct irq_chip *intc_dev;
>
> irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> if (!irqc)
> return -ENOMEM;
> -
> - xintc_irqc = irqc;
> -
> irqc->base = of_iomap(intc, 0);
> BUG_ON(!irqc->base);
>
> - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
> + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &irqc->nr_irq);
> if (ret < 0) {
> pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
> - goto err_alloc;
> + goto error;
> }
>
> ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
> @@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
> irqc->intr_mask = 0;
> }
>
> - if (irqc->intr_mask >> nr_irq)
> + if (irqc->intr_mask >> irqc->nr_irq)
> pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
>
> pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> - intc, nr_irq, irqc->intr_mask);
> + intc, irqc->nr_irq, irqc->intr_mask);
> +
> + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> + if (!intc_dev) {
> + ret = -ENOMEM;
> + goto error;
> + }
>
> + intc_dev->name = intc->full_name;
> + intc_dev->irq_unmask = intc_enable_or_unmask,
> + intc_dev->irq_mask = intc_disable_or_mask,
> + intc_dev->irq_ack = intc_ack,
> + intc_dev->irq_mask_ack = intc_mask_ack,
> + irqc->intc_dev = intc_dev;
>
> /*
> * Disable all external interrupts until they are
> * explicity requested.
> */
> - xintc_write(IER, 0);
> + xintc_write(irqc->base + IER, 0);
>
> /* Acknowledge any pending interrupts just in case. */
> - xintc_write(IAR, 0xffffffff);
> + xintc_write(irqc->base + IAR, 0xffffffff);
>
> /* Turn on the Master Enable. */
> - xintc_write(MER, MER_HIE | MER_ME);
> - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> static_branch_enable(&xintc_is_be);
> - xintc_write(MER, MER_HIE | MER_ME);
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> }
>
> - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> &xintc_irq_domain_ops, irqc);
> if (!irqc->root_domain) {
> pr_err("irq-xilinx: Unable to create IRQ domain\n");
> @@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
> goto err_alloc;
> }
> } else {
> - irq_set_default_host(irqc->root_domain);
> + primary_intc = irqc;
> + irq_set_default_host(primary_intc->root_domain);
> }
>
> return 0;
>
> err_alloc:
> - xintc_irqc = NULL;
> + kfree(intc_dev);
> +error:
> + iounmap(irqc->base);
> kfree(irqc);
> return ret;
>
>
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 14:15 ` David Laight
@ 2020-02-05 14:54 ` Michal Simek
-1 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-05 14:54 UTC (permalink / raw)
To: David Laight, 'Mubin Usman Sayyed',
tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: linux-kernel, siva.durga.paladugu, anirudha.sarangi
On 05. 02. 20 15:15, David Laight wrote:
>> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s)
>> confidential information that may be proprietary, privileged or copyrighted under applicable law. If
>> you are not the intended recipient, do not read, copy, or forward this email message or any
>> attachments. Delete this email message and any attachments immediately.
>
> Deleted.....
:-) I got two copies. One without it :-)
Mubin: Please fix it.
Thanks,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-05 14:54 ` Michal Simek
0 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-05 14:54 UTC (permalink / raw)
To: David Laight, 'Mubin Usman Sayyed',
tglx, jason, maz, michal.simek, linux-arm-kernel
Cc: anirudha.sarangi, siva.durga.paladugu, linux-kernel
On 05. 02. 20 15:15, David Laight wrote:
>> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s)
>> confidential information that may be proprietary, privileged or copyrighted under applicable law. If
>> you are not the intended recipient, do not read, copy, or forward this email message or any
>> attachments. Delete this email message and any attachments immediately.
>
> Deleted.....
:-) I got two copies. One without it :-)
Mubin: Please fix it.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 14:05 ` Mubin Usman Sayyed
@ 2020-02-05 16:53 ` Marc Zyngier
-1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2020-02-05 16:53 UTC (permalink / raw)
To: Mubin Usman Sayyed
Cc: tglx, jason, michal.simek, linux-arm-kernel, linux-kernel,
siva.durga.paladugu, anirudha.sarangi
On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>
> This patch adds support for multiple instances of
> xilinx interrupt controller. Below configurations are
> supported by driver,
>
> - peripheral->xilinx-intc->xilinx-intc->gic
> - peripheral->xilinx-intc->xilinx-intc
>
> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> ---
> Changes for v2:
> - Removed write_fn/read_fn hooks, used xintc_write/
> xintc_read directly
> - Moved primary_intc declaration after xintc_irq_chip
> ---
> drivers/irqchip/irq-xilinx-intc.c | 121
> +++++++++++++++++++++++---------------
> 1 file changed, 73 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/irqchip/irq-xilinx-intc.c
> b/drivers/irqchip/irq-xilinx-intc.c
> index e3043de..14cb630 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> void __iomem *base;
> struct irq_domain *root_domain;
> u32 intr_mask;
> + struct irq_chip *intc_dev;
> + u32 nr_irq;
> };
>
> -static struct xintc_irq_chip *xintc_irqc;
> +static struct xintc_irq_chip *primary_intc;
>
> -static void xintc_write(int reg, u32 data)
> +static void xintc_write(void __iomem *addr, u32 data)
> {
> if (static_branch_unlikely(&xintc_is_be))
> - iowrite32be(data, xintc_irqc->base + reg);
> + iowrite32be(data, addr);
> else
> - iowrite32(data, xintc_irqc->base + reg);
> + iowrite32(data, addr);
> }
>
> -static unsigned int xintc_read(int reg)
> +static unsigned int xintc_read(void __iomem *addr)
Since you are changing this, please change the return value to reflect
the size of what you're returning (u32 instead of unsigned int).
> {
> if (static_branch_unlikely(&xintc_is_be))
> - return ioread32be(xintc_irqc->base + reg);
> + return ioread32be(addr);
> else
> - return ioread32(xintc_irqc->base + reg);
> + return ioread32(addr);
> }
>
> static void intc_enable_or_unmask(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
>
> pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>
> @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data
> *d)
> * acks the irq before calling the interrupt handler
> */
> if (irqd_is_level_type(d))
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + IAR, mask);
>
> - xintc_write(SIE, mask);
> + xintc_write(local_intc->base + SIE, mask);
> }
>
> static void intc_disable_or_mask(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> - xintc_write(CIE, 1 << d->hwirq);
> + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> }
>
> static void intc_ack(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> - xintc_write(IAR, 1 << d->hwirq);
> + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> }
>
> static void intc_mask_ack(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
>
> pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> - xintc_write(CIE, mask);
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + CIE, mask);
> + xintc_write(local_intc->base + IAR, mask);
> }
>
> -static struct irq_chip intc_dev = {
> - .name = "Xilinx INTC",
> - .irq_unmask = intc_enable_or_unmask,
> - .irq_mask = intc_disable_or_mask,
> - .irq_ack = intc_ack,
> - .irq_mask_ack = intc_mask_ack,
> -};
> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
> *local_intc)
> +{
> + int hwirq, irq = -1;
Type consistency for hwirq.
> +
> + hwirq = xintc_read(local_intc->base + IVR);
> + if (hwirq != -1U)
> + irq = irq_find_mapping(local_intc->root_domain, hwirq);
> +
> + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +
> + return irq;
That now gives you both -1 and 0 for error values. Please stick with 0.
> +}
>
> unsigned int xintc_get_irq(void)
> {
> - unsigned int hwirq, irq = -1;
> + int hwirq, irq = -1;
>
> - hwirq = xintc_read(IVR);
> + hwirq = xintc_read(primary_intc->base + IVR);
> if (hwirq != -1U)
> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> + irq = irq_find_mapping(primary_intc->root_domain,
> hwirq);
>
> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
I have the ugly feeling I'm reading the same code twice... Surely you
can
make these two functions common code.
>
> @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
>
> static int xintc_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hw)
> {
> - if (xintc_irqc->intr_mask & (1 << hw)) {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + struct xintc_irq_chip *local_intc = d->host_data;
> +
> + if (local_intc->intr_mask & (1 << hw)) {
BIT(hw)
> + irq_set_chip_and_handler_name(irq,
> local_intc->intc_dev,
> handle_edge_irq,
> "edge");
> irq_clear_status_flags(irq, IRQ_LEVEL);
> } else {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + irq_set_chip_and_handler_name(irq,
> local_intc->intc_dev,
> handle_level_irq,
> "level");
> irq_set_status_flags(irq, IRQ_LEVEL);
> }
> + irq_set_chip_data(irq, local_intc);
> return 0;
> }
>
> @@ -138,11 +154,13 @@ static const struct irq_domain_ops
> xintc_irq_domain_ops = {
> static void xil_intc_irq_handler(struct irq_desc *desc)
> {
> struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct xintc_irq_chip *local_intc =
> + irq_data_get_irq_handler_data(&desc->irq_data);
> u32 pending;
>
> chained_irq_enter(chip, desc);
> do {
> - pending = xintc_get_irq();
> + pending = xintc_get_irq_local(local_intc);
> if (pending == -1U)
> break;
> generic_handle_irq(pending);
> @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc
> *desc)
> static int __init xilinx_intc_of_init(struct device_node *intc,
> struct device_node
> *parent)
> {
> - u32 nr_irq;
> int ret, irq;
> struct xintc_irq_chip *irqc;
> -
> - if (xintc_irqc) {
> - pr_err("irq-xilinx: Multiple instances aren't
> supported\n");
> - return -EINVAL;
> - }
> + struct irq_chip *intc_dev;
>
> irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> if (!irqc)
> return -ENOMEM;
> -
> - xintc_irqc = irqc;
> -
> irqc->base = of_iomap(intc, 0);
> BUG_ON(!irqc->base);
>
> - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> &nr_irq);
> + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> &irqc->nr_irq);
> if (ret < 0) {
> pr_err("irq-xilinx: unable to read
> xlnx,num-intr-inputs\n");
> - goto err_alloc;
> + goto error;
> }
>
> ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> &irqc->intr_mask);
> @@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct
> device_node *intc,
> irqc->intr_mask = 0;
> }
>
> - if (irqc->intr_mask >> nr_irq)
> + if (irqc->intr_mask >> irqc->nr_irq)
> pr_warn("irq-xilinx: mismatch in kind-of-intr
> param\n");
>
> pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> - intc, nr_irq, irqc->intr_mask);
> + intc, irqc->nr_irq, irqc->intr_mask);
> +
> + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> + if (!intc_dev) {
> + ret = -ENOMEM;
> + goto error;
> + }
>
> + intc_dev->name = intc->full_name;
No. The world doesn't need to see the OF path of your interrupt
controller in /proc/cpuinfo.
The name that was there before was perfectly descriptive, please stick
to it.
> + intc_dev->irq_unmask = intc_enable_or_unmask,
> + intc_dev->irq_mask = intc_disable_or_mask,
> + intc_dev->irq_ack = intc_ack,
> + intc_dev->irq_mask_ack = intc_mask_ack,
And this structure should stay static, as it was before.
> + irqc->intc_dev = intc_dev;
>
> /*
> * Disable all external interrupts until they are
> * explicity requested.
> */
> - xintc_write(IER, 0);
> + xintc_write(irqc->base + IER, 0);
>
> /* Acknowledge any pending interrupts just in case. */
> - xintc_write(IAR, 0xffffffff);
> + xintc_write(irqc->base + IAR, 0xffffffff);
>
> /* Turn on the Master Enable. */
> - xintc_write(MER, MER_HIE | MER_ME);
> - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> static_branch_enable(&xintc_is_be);
> - xintc_write(MER, MER_HIE | MER_ME);
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> }
>
> - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
>
> &xintc_irq_domain_ops, irqc);
> if (!irqc->root_domain) {
> pr_err("irq-xilinx: Unable to create IRQ domain\n");
> @@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct
> device_node *intc,
> goto err_alloc;
> }
> } else {
> - irq_set_default_host(irqc->root_domain);
> + primary_intc = irqc;
> + irq_set_default_host(primary_intc->root_domain);
> }
>
> return 0;
>
> err_alloc:
> - xintc_irqc = NULL;
> + kfree(intc_dev);
> +error:
> + iounmap(irqc->base);
> kfree(irqc);
> return ret;
>
> --
> 2.7.4
>
> This email and any attachments are intended for the sole use of the
> named recipient(s) and contain(s) confidential information that may be
> proprietary, privileged or copyrighted under applicable law. If you
> are not the intended recipient, do not read, copy, or forward this
> email message or any attachments. Delete this email message and any
> attachments immediately.
Please tell your employer to fix their email server.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-05 16:53 ` Marc Zyngier
0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2020-02-05 16:53 UTC (permalink / raw)
To: Mubin Usman Sayyed
Cc: jason, anirudha.sarangi, linux-kernel, michal.simek, tglx,
siva.durga.paladugu, linux-arm-kernel
On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>
> This patch adds support for multiple instances of
> xilinx interrupt controller. Below configurations are
> supported by driver,
>
> - peripheral->xilinx-intc->xilinx-intc->gic
> - peripheral->xilinx-intc->xilinx-intc
>
> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> ---
> Changes for v2:
> - Removed write_fn/read_fn hooks, used xintc_write/
> xintc_read directly
> - Moved primary_intc declaration after xintc_irq_chip
> ---
> drivers/irqchip/irq-xilinx-intc.c | 121
> +++++++++++++++++++++++---------------
> 1 file changed, 73 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/irqchip/irq-xilinx-intc.c
> b/drivers/irqchip/irq-xilinx-intc.c
> index e3043de..14cb630 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> void __iomem *base;
> struct irq_domain *root_domain;
> u32 intr_mask;
> + struct irq_chip *intc_dev;
> + u32 nr_irq;
> };
>
> -static struct xintc_irq_chip *xintc_irqc;
> +static struct xintc_irq_chip *primary_intc;
>
> -static void xintc_write(int reg, u32 data)
> +static void xintc_write(void __iomem *addr, u32 data)
> {
> if (static_branch_unlikely(&xintc_is_be))
> - iowrite32be(data, xintc_irqc->base + reg);
> + iowrite32be(data, addr);
> else
> - iowrite32(data, xintc_irqc->base + reg);
> + iowrite32(data, addr);
> }
>
> -static unsigned int xintc_read(int reg)
> +static unsigned int xintc_read(void __iomem *addr)
Since you are changing this, please change the return value to reflect
the size of what you're returning (u32 instead of unsigned int).
> {
> if (static_branch_unlikely(&xintc_is_be))
> - return ioread32be(xintc_irqc->base + reg);
> + return ioread32be(addr);
> else
> - return ioread32(xintc_irqc->base + reg);
> + return ioread32(addr);
> }
>
> static void intc_enable_or_unmask(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
>
> pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>
> @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data
> *d)
> * acks the irq before calling the interrupt handler
> */
> if (irqd_is_level_type(d))
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + IAR, mask);
>
> - xintc_write(SIE, mask);
> + xintc_write(local_intc->base + SIE, mask);
> }
>
> static void intc_disable_or_mask(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> - xintc_write(CIE, 1 << d->hwirq);
> + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> }
>
> static void intc_ack(struct irq_data *d)
> {
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
> +
> pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> - xintc_write(IAR, 1 << d->hwirq);
> + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> }
>
> static void intc_mask_ack(struct irq_data *d)
> {
> unsigned long mask = 1 << d->hwirq;
> + struct xintc_irq_chip *local_intc =
> irq_data_get_irq_chip_data(d);
>
> pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> - xintc_write(CIE, mask);
> - xintc_write(IAR, mask);
> + xintc_write(local_intc->base + CIE, mask);
> + xintc_write(local_intc->base + IAR, mask);
> }
>
> -static struct irq_chip intc_dev = {
> - .name = "Xilinx INTC",
> - .irq_unmask = intc_enable_or_unmask,
> - .irq_mask = intc_disable_or_mask,
> - .irq_ack = intc_ack,
> - .irq_mask_ack = intc_mask_ack,
> -};
> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
> *local_intc)
> +{
> + int hwirq, irq = -1;
Type consistency for hwirq.
> +
> + hwirq = xintc_read(local_intc->base + IVR);
> + if (hwirq != -1U)
> + irq = irq_find_mapping(local_intc->root_domain, hwirq);
> +
> + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +
> + return irq;
That now gives you both -1 and 0 for error values. Please stick with 0.
> +}
>
> unsigned int xintc_get_irq(void)
> {
> - unsigned int hwirq, irq = -1;
> + int hwirq, irq = -1;
>
> - hwirq = xintc_read(IVR);
> + hwirq = xintc_read(primary_intc->base + IVR);
> if (hwirq != -1U)
> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> + irq = irq_find_mapping(primary_intc->root_domain,
> hwirq);
>
> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
I have the ugly feeling I'm reading the same code twice... Surely you
can
make these two functions common code.
>
> @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
>
> static int xintc_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hw)
> {
> - if (xintc_irqc->intr_mask & (1 << hw)) {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + struct xintc_irq_chip *local_intc = d->host_data;
> +
> + if (local_intc->intr_mask & (1 << hw)) {
BIT(hw)
> + irq_set_chip_and_handler_name(irq,
> local_intc->intc_dev,
> handle_edge_irq,
> "edge");
> irq_clear_status_flags(irq, IRQ_LEVEL);
> } else {
> - irq_set_chip_and_handler_name(irq, &intc_dev,
> + irq_set_chip_and_handler_name(irq,
> local_intc->intc_dev,
> handle_level_irq,
> "level");
> irq_set_status_flags(irq, IRQ_LEVEL);
> }
> + irq_set_chip_data(irq, local_intc);
> return 0;
> }
>
> @@ -138,11 +154,13 @@ static const struct irq_domain_ops
> xintc_irq_domain_ops = {
> static void xil_intc_irq_handler(struct irq_desc *desc)
> {
> struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct xintc_irq_chip *local_intc =
> + irq_data_get_irq_handler_data(&desc->irq_data);
> u32 pending;
>
> chained_irq_enter(chip, desc);
> do {
> - pending = xintc_get_irq();
> + pending = xintc_get_irq_local(local_intc);
> if (pending == -1U)
> break;
> generic_handle_irq(pending);
> @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc
> *desc)
> static int __init xilinx_intc_of_init(struct device_node *intc,
> struct device_node
> *parent)
> {
> - u32 nr_irq;
> int ret, irq;
> struct xintc_irq_chip *irqc;
> -
> - if (xintc_irqc) {
> - pr_err("irq-xilinx: Multiple instances aren't
> supported\n");
> - return -EINVAL;
> - }
> + struct irq_chip *intc_dev;
>
> irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> if (!irqc)
> return -ENOMEM;
> -
> - xintc_irqc = irqc;
> -
> irqc->base = of_iomap(intc, 0);
> BUG_ON(!irqc->base);
>
> - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> &nr_irq);
> + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> &irqc->nr_irq);
> if (ret < 0) {
> pr_err("irq-xilinx: unable to read
> xlnx,num-intr-inputs\n");
> - goto err_alloc;
> + goto error;
> }
>
> ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> &irqc->intr_mask);
> @@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct
> device_node *intc,
> irqc->intr_mask = 0;
> }
>
> - if (irqc->intr_mask >> nr_irq)
> + if (irqc->intr_mask >> irqc->nr_irq)
> pr_warn("irq-xilinx: mismatch in kind-of-intr
> param\n");
>
> pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> - intc, nr_irq, irqc->intr_mask);
> + intc, irqc->nr_irq, irqc->intr_mask);
> +
> + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> + if (!intc_dev) {
> + ret = -ENOMEM;
> + goto error;
> + }
>
> + intc_dev->name = intc->full_name;
No. The world doesn't need to see the OF path of your interrupt
controller in /proc/cpuinfo.
The name that was there before was perfectly descriptive, please stick
to it.
> + intc_dev->irq_unmask = intc_enable_or_unmask,
> + intc_dev->irq_mask = intc_disable_or_mask,
> + intc_dev->irq_ack = intc_ack,
> + intc_dev->irq_mask_ack = intc_mask_ack,
And this structure should stay static, as it was before.
> + irqc->intc_dev = intc_dev;
>
> /*
> * Disable all external interrupts until they are
> * explicity requested.
> */
> - xintc_write(IER, 0);
> + xintc_write(irqc->base + IER, 0);
>
> /* Acknowledge any pending interrupts just in case. */
> - xintc_write(IAR, 0xffffffff);
> + xintc_write(irqc->base + IAR, 0xffffffff);
>
> /* Turn on the Master Enable. */
> - xintc_write(MER, MER_HIE | MER_ME);
> - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> static_branch_enable(&xintc_is_be);
> - xintc_write(MER, MER_HIE | MER_ME);
> + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> }
>
> - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
>
> &xintc_irq_domain_ops, irqc);
> if (!irqc->root_domain) {
> pr_err("irq-xilinx: Unable to create IRQ domain\n");
> @@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct
> device_node *intc,
> goto err_alloc;
> }
> } else {
> - irq_set_default_host(irqc->root_domain);
> + primary_intc = irqc;
> + irq_set_default_host(primary_intc->root_domain);
> }
>
> return 0;
>
> err_alloc:
> - xintc_irqc = NULL;
> + kfree(intc_dev);
> +error:
> + iounmap(irqc->base);
> kfree(irqc);
> return ret;
>
> --
> 2.7.4
>
> This email and any attachments are intended for the sole use of the
> named recipient(s) and contain(s) confidential information that may be
> proprietary, privileged or copyrighted under applicable law. If you
> are not the intended recipient, do not read, copy, or forward this
> email message or any attachments. Delete this email message and any
> attachments immediately.
Please tell your employer to fix their email server.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 16:53 ` Marc Zyngier
@ 2020-02-06 7:06 ` Michal Simek
-1 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-06 7:06 UTC (permalink / raw)
To: Marc Zyngier, Mubin Usman Sayyed
Cc: tglx, jason, michal.simek, linux-arm-kernel, linux-kernel,
siva.durga.paladugu, anirudha.sarangi
On 05. 02. 20 17:53, Marc Zyngier wrote:
> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>>
>> This patch adds support for multiple instances of
>> xilinx interrupt controller. Below configurations are
>> supported by driver,
>>
>> - peripheral->xilinx-intc->xilinx-intc->gic
>> - peripheral->xilinx-intc->xilinx-intc
>>
>> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
>> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>> ---
>> Changes for v2:
>> - Removed write_fn/read_fn hooks, used xintc_write/
>> xintc_read directly
>> - Moved primary_intc declaration after xintc_irq_chip
>> ---
>> drivers/irqchip/irq-xilinx-intc.c | 121
>> +++++++++++++++++++++++---------------
>> 1 file changed, 73 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-xilinx-intc.c
>> b/drivers/irqchip/irq-xilinx-intc.c
>> index e3043de..14cb630 100644
>> --- a/drivers/irqchip/irq-xilinx-intc.c
>> +++ b/drivers/irqchip/irq-xilinx-intc.c
>> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
>> void __iomem *base;
>> struct irq_domain *root_domain;
>> u32 intr_mask;
>> + struct irq_chip *intc_dev;
>> + u32 nr_irq;
>> };
>>
>> -static struct xintc_irq_chip *xintc_irqc;
>> +static struct xintc_irq_chip *primary_intc;
>>
>> -static void xintc_write(int reg, u32 data)
>> +static void xintc_write(void __iomem *addr, u32 data)
>> {
>> if (static_branch_unlikely(&xintc_is_be))
>> - iowrite32be(data, xintc_irqc->base + reg);
>> + iowrite32be(data, addr);
>> else
>> - iowrite32(data, xintc_irqc->base + reg);
>> + iowrite32(data, addr);
>> }
>>
>> -static unsigned int xintc_read(int reg)
>> +static unsigned int xintc_read(void __iomem *addr)
>
> Since you are changing this, please change the return value to reflect
> the size of what you're returning (u32 instead of unsigned int).
>
>> {
>> if (static_branch_unlikely(&xintc_is_be))
>> - return ioread32be(xintc_irqc->base + reg);
>> + return ioread32be(addr);
>> else
>> - return ioread32(xintc_irqc->base + reg);
>> + return ioread32(addr);
>> }
>>
>> static void intc_enable_or_unmask(struct irq_data *d)
>> {
>> unsigned long mask = 1 << d->hwirq;
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>>
>> pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>>
>> @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data *d)
>> * acks the irq before calling the interrupt handler
>> */
>> if (irqd_is_level_type(d))
>> - xintc_write(IAR, mask);
>> + xintc_write(local_intc->base + IAR, mask);
>>
>> - xintc_write(SIE, mask);
>> + xintc_write(local_intc->base + SIE, mask);
>> }
>>
>> static void intc_disable_or_mask(struct irq_data *d)
>> {
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>> +
>> pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
>> - xintc_write(CIE, 1 << d->hwirq);
>> + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
>> }
>>
>> static void intc_ack(struct irq_data *d)
>> {
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>> +
>> pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
>> - xintc_write(IAR, 1 << d->hwirq);
>> + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
>> }
>>
>> static void intc_mask_ack(struct irq_data *d)
>> {
>> unsigned long mask = 1 << d->hwirq;
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>>
>> pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
>> - xintc_write(CIE, mask);
>> - xintc_write(IAR, mask);
>> + xintc_write(local_intc->base + CIE, mask);
>> + xintc_write(local_intc->base + IAR, mask);
>> }
>>
>> -static struct irq_chip intc_dev = {
>> - .name = "Xilinx INTC",
>> - .irq_unmask = intc_enable_or_unmask,
>> - .irq_mask = intc_disable_or_mask,
>> - .irq_ack = intc_ack,
>> - .irq_mask_ack = intc_mask_ack,
>> -};
>> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
>> *local_intc)
>> +{
>> + int hwirq, irq = -1;
>
> Type consistency for hwirq.
>
>> +
>> + hwirq = xintc_read(local_intc->base + IVR);
>> + if (hwirq != -1U)
>> + irq = irq_find_mapping(local_intc->root_domain, hwirq);
>> +
>> + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>> +
>> + return irq;
>
> That now gives you both -1 and 0 for error values. Please stick with 0.
>
>> +}
>>
>> unsigned int xintc_get_irq(void)
>> {
>> - unsigned int hwirq, irq = -1;
>> + int hwirq, irq = -1;
>>
>> - hwirq = xintc_read(IVR);
>> + hwirq = xintc_read(primary_intc->base + IVR);
>> if (hwirq != -1U)
>> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
>> + irq = irq_find_mapping(primary_intc->root_domain, hwirq);
>>
>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>
> I have the ugly feeling I'm reading the same code twice... Surely you can
> make these two functions common code.
I have some questions regarding this.
I have updated one patchset which is adding support for Microblaze SMP.
And when I was looking at current wiring of this driver I have decided
to change it.
I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
This driver calls set_handle_irq(xil_intc_handle_irq)
and MB do_IRQ() call handle_arch_irq()
and IRQ routine here is using handle_domain_irq().
I would expect that this chained IRQ handler can also use
handle_domain_irq().
Is that correct understanding?
>>
>> @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
>>
>> static int xintc_map(struct irq_domain *d, unsigned int irq,
>> irq_hw_number_t hw)
>> {
>> - if (xintc_irqc->intr_mask & (1 << hw)) {
>> - irq_set_chip_and_handler_name(irq, &intc_dev,
>> + struct xintc_irq_chip *local_intc = d->host_data;
>> +
>> + if (local_intc->intr_mask & (1 << hw)) {
>
> BIT(hw)
>
>> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
>> handle_edge_irq, "edge");
>> irq_clear_status_flags(irq, IRQ_LEVEL);
>> } else {
>> - irq_set_chip_and_handler_name(irq, &intc_dev,
>> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
>> handle_level_irq,
>> "level");
>> irq_set_status_flags(irq, IRQ_LEVEL);
>> }
>> + irq_set_chip_data(irq, local_intc);
>> return 0;
>> }
>>
>> @@ -138,11 +154,13 @@ static const struct irq_domain_ops
>> xintc_irq_domain_ops = {
>> static void xil_intc_irq_handler(struct irq_desc *desc)
>> {
>> struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct xintc_irq_chip *local_intc =
>> + irq_data_get_irq_handler_data(&desc->irq_data);
>> u32 pending;
>>
>> chained_irq_enter(chip, desc);
>> do {
>> - pending = xintc_get_irq();
>> + pending = xintc_get_irq_local(local_intc);
>> if (pending == -1U)
>> break;
>> generic_handle_irq(pending);
>> @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc
>> *desc)
>> static int __init xilinx_intc_of_init(struct device_node *intc,
>> struct device_node *parent)
>> {
>> - u32 nr_irq;
>> int ret, irq;
>> struct xintc_irq_chip *irqc;
>> -
>> - if (xintc_irqc) {
>> - pr_err("irq-xilinx: Multiple instances aren't
>> supported\n");
>> - return -EINVAL;
>> - }
>> + struct irq_chip *intc_dev;
>>
>> irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>> if (!irqc)
>> return -ENOMEM;
>> -
>> - xintc_irqc = irqc;
>> -
>> irqc->base = of_iomap(intc, 0);
>> BUG_ON(!irqc->base);
>>
>> - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
>> &nr_irq);
>> + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
>> &irqc->nr_irq);
>> if (ret < 0) {
>> pr_err("irq-xilinx: unable to read
>> xlnx,num-intr-inputs\n");
>> - goto err_alloc;
>> + goto error;
>> }
>>
>> ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
>> &irqc->intr_mask);
>> @@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct
>> device_node *intc,
>> irqc->intr_mask = 0;
>> }
>>
>> - if (irqc->intr_mask >> nr_irq)
>> + if (irqc->intr_mask >> irqc->nr_irq)
>> pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
>>
>> pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
>> - intc, nr_irq, irqc->intr_mask);
>> + intc, irqc->nr_irq, irqc->intr_mask);
>> +
>> + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
>> + if (!intc_dev) {
>> + ret = -ENOMEM;
>> + goto error;
>> + }
>>
>> + intc_dev->name = intc->full_name;
>
> No. The world doesn't need to see the OF path of your interrupt
> controller in /proc/cpuinfo.
> The name that was there before was perfectly descriptive, please stick
> to it.
It should be showing name like interrupt-controller@41800000.
Do you think that we really should stick with just fixed name?
There could be multiple instances in the system and you will have no
idea how they are connected.
Thanks,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 7:06 ` Michal Simek
0 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-06 7:06 UTC (permalink / raw)
To: Marc Zyngier, Mubin Usman Sayyed
Cc: jason, anirudha.sarangi, linux-kernel, michal.simek, tglx,
siva.durga.paladugu, linux-arm-kernel
On 05. 02. 20 17:53, Marc Zyngier wrote:
> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>> From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>>
>> This patch adds support for multiple instances of
>> xilinx interrupt controller. Below configurations are
>> supported by driver,
>>
>> - peripheral->xilinx-intc->xilinx-intc->gic
>> - peripheral->xilinx-intc->xilinx-intc
>>
>> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
>> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
>> ---
>> Changes for v2:
>> - Removed write_fn/read_fn hooks, used xintc_write/
>> xintc_read directly
>> - Moved primary_intc declaration after xintc_irq_chip
>> ---
>> drivers/irqchip/irq-xilinx-intc.c | 121
>> +++++++++++++++++++++++---------------
>> 1 file changed, 73 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-xilinx-intc.c
>> b/drivers/irqchip/irq-xilinx-intc.c
>> index e3043de..14cb630 100644
>> --- a/drivers/irqchip/irq-xilinx-intc.c
>> +++ b/drivers/irqchip/irq-xilinx-intc.c
>> @@ -38,29 +38,32 @@ struct xintc_irq_chip {
>> void __iomem *base;
>> struct irq_domain *root_domain;
>> u32 intr_mask;
>> + struct irq_chip *intc_dev;
>> + u32 nr_irq;
>> };
>>
>> -static struct xintc_irq_chip *xintc_irqc;
>> +static struct xintc_irq_chip *primary_intc;
>>
>> -static void xintc_write(int reg, u32 data)
>> +static void xintc_write(void __iomem *addr, u32 data)
>> {
>> if (static_branch_unlikely(&xintc_is_be))
>> - iowrite32be(data, xintc_irqc->base + reg);
>> + iowrite32be(data, addr);
>> else
>> - iowrite32(data, xintc_irqc->base + reg);
>> + iowrite32(data, addr);
>> }
>>
>> -static unsigned int xintc_read(int reg)
>> +static unsigned int xintc_read(void __iomem *addr)
>
> Since you are changing this, please change the return value to reflect
> the size of what you're returning (u32 instead of unsigned int).
>
>> {
>> if (static_branch_unlikely(&xintc_is_be))
>> - return ioread32be(xintc_irqc->base + reg);
>> + return ioread32be(addr);
>> else
>> - return ioread32(xintc_irqc->base + reg);
>> + return ioread32(addr);
>> }
>>
>> static void intc_enable_or_unmask(struct irq_data *d)
>> {
>> unsigned long mask = 1 << d->hwirq;
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>>
>> pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>>
>> @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data *d)
>> * acks the irq before calling the interrupt handler
>> */
>> if (irqd_is_level_type(d))
>> - xintc_write(IAR, mask);
>> + xintc_write(local_intc->base + IAR, mask);
>>
>> - xintc_write(SIE, mask);
>> + xintc_write(local_intc->base + SIE, mask);
>> }
>>
>> static void intc_disable_or_mask(struct irq_data *d)
>> {
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>> +
>> pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
>> - xintc_write(CIE, 1 << d->hwirq);
>> + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
>> }
>>
>> static void intc_ack(struct irq_data *d)
>> {
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>> +
>> pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
>> - xintc_write(IAR, 1 << d->hwirq);
>> + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
>> }
>>
>> static void intc_mask_ack(struct irq_data *d)
>> {
>> unsigned long mask = 1 << d->hwirq;
>> + struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>>
>> pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
>> - xintc_write(CIE, mask);
>> - xintc_write(IAR, mask);
>> + xintc_write(local_intc->base + CIE, mask);
>> + xintc_write(local_intc->base + IAR, mask);
>> }
>>
>> -static struct irq_chip intc_dev = {
>> - .name = "Xilinx INTC",
>> - .irq_unmask = intc_enable_or_unmask,
>> - .irq_mask = intc_disable_or_mask,
>> - .irq_ack = intc_ack,
>> - .irq_mask_ack = intc_mask_ack,
>> -};
>> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
>> *local_intc)
>> +{
>> + int hwirq, irq = -1;
>
> Type consistency for hwirq.
>
>> +
>> + hwirq = xintc_read(local_intc->base + IVR);
>> + if (hwirq != -1U)
>> + irq = irq_find_mapping(local_intc->root_domain, hwirq);
>> +
>> + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>> +
>> + return irq;
>
> That now gives you both -1 and 0 for error values. Please stick with 0.
>
>> +}
>>
>> unsigned int xintc_get_irq(void)
>> {
>> - unsigned int hwirq, irq = -1;
>> + int hwirq, irq = -1;
>>
>> - hwirq = xintc_read(IVR);
>> + hwirq = xintc_read(primary_intc->base + IVR);
>> if (hwirq != -1U)
>> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
>> + irq = irq_find_mapping(primary_intc->root_domain, hwirq);
>>
>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>
> I have the ugly feeling I'm reading the same code twice... Surely you can
> make these two functions common code.
I have some questions regarding this.
I have updated one patchset which is adding support for Microblaze SMP.
And when I was looking at current wiring of this driver I have decided
to change it.
I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
This driver calls set_handle_irq(xil_intc_handle_irq)
and MB do_IRQ() call handle_arch_irq()
and IRQ routine here is using handle_domain_irq().
I would expect that this chained IRQ handler can also use
handle_domain_irq().
Is that correct understanding?
>>
>> @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
>>
>> static int xintc_map(struct irq_domain *d, unsigned int irq,
>> irq_hw_number_t hw)
>> {
>> - if (xintc_irqc->intr_mask & (1 << hw)) {
>> - irq_set_chip_and_handler_name(irq, &intc_dev,
>> + struct xintc_irq_chip *local_intc = d->host_data;
>> +
>> + if (local_intc->intr_mask & (1 << hw)) {
>
> BIT(hw)
>
>> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
>> handle_edge_irq, "edge");
>> irq_clear_status_flags(irq, IRQ_LEVEL);
>> } else {
>> - irq_set_chip_and_handler_name(irq, &intc_dev,
>> + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
>> handle_level_irq,
>> "level");
>> irq_set_status_flags(irq, IRQ_LEVEL);
>> }
>> + irq_set_chip_data(irq, local_intc);
>> return 0;
>> }
>>
>> @@ -138,11 +154,13 @@ static const struct irq_domain_ops
>> xintc_irq_domain_ops = {
>> static void xil_intc_irq_handler(struct irq_desc *desc)
>> {
>> struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct xintc_irq_chip *local_intc =
>> + irq_data_get_irq_handler_data(&desc->irq_data);
>> u32 pending;
>>
>> chained_irq_enter(chip, desc);
>> do {
>> - pending = xintc_get_irq();
>> + pending = xintc_get_irq_local(local_intc);
>> if (pending == -1U)
>> break;
>> generic_handle_irq(pending);
>> @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc
>> *desc)
>> static int __init xilinx_intc_of_init(struct device_node *intc,
>> struct device_node *parent)
>> {
>> - u32 nr_irq;
>> int ret, irq;
>> struct xintc_irq_chip *irqc;
>> -
>> - if (xintc_irqc) {
>> - pr_err("irq-xilinx: Multiple instances aren't
>> supported\n");
>> - return -EINVAL;
>> - }
>> + struct irq_chip *intc_dev;
>>
>> irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
>> if (!irqc)
>> return -ENOMEM;
>> -
>> - xintc_irqc = irqc;
>> -
>> irqc->base = of_iomap(intc, 0);
>> BUG_ON(!irqc->base);
>>
>> - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
>> &nr_irq);
>> + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
>> &irqc->nr_irq);
>> if (ret < 0) {
>> pr_err("irq-xilinx: unable to read
>> xlnx,num-intr-inputs\n");
>> - goto err_alloc;
>> + goto error;
>> }
>>
>> ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
>> &irqc->intr_mask);
>> @@ -183,30 +193,42 @@ static int __init xilinx_intc_of_init(struct
>> device_node *intc,
>> irqc->intr_mask = 0;
>> }
>>
>> - if (irqc->intr_mask >> nr_irq)
>> + if (irqc->intr_mask >> irqc->nr_irq)
>> pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
>>
>> pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
>> - intc, nr_irq, irqc->intr_mask);
>> + intc, irqc->nr_irq, irqc->intr_mask);
>> +
>> + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
>> + if (!intc_dev) {
>> + ret = -ENOMEM;
>> + goto error;
>> + }
>>
>> + intc_dev->name = intc->full_name;
>
> No. The world doesn't need to see the OF path of your interrupt
> controller in /proc/cpuinfo.
> The name that was there before was perfectly descriptive, please stick
> to it.
It should be showing name like interrupt-controller@41800000.
Do you think that we really should stick with just fixed name?
There could be multiple instances in the system and you will have no
idea how they are connected.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 14:53 ` Michal Simek
@ 2020-02-06 8:02 ` Mubin Usman Sayyed
-1 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-06 8:02 UTC (permalink / raw)
To: Michal Simek, tglx, jason, maz, Michal Simek, linux-arm-kernel
Cc: linux-kernel, Siva Durga Prasad Paladugu, Anirudha Sarangi
Hi Michal,
> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Wednesday, February 5, 2020 8:23 PM
> To: Mubin Usman Sayyed <MUBINUSM@xilinx.com>; tglx@linutronix.de;
> jason@lakedaemon.net; maz@kernel.org; Michal Simek
> <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org; Siva Durga Prasad Paladugu
> <sivadur@xilinx.com>; Anirudha Sarangi <anirudh@xilinx.com>
> Subject: Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
>
> Hi Mubin,
>
> On 05. 02. 20 15:05, Mubin Usman Sayyed wrote:
> > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> >
> > This patch adds support for multiple instances of xilinx interrupt
> > controller. Below configurations are supported by driver,
> >
> > - peripheral->xilinx-intc->xilinx-intc->gic
> > - peripheral->xilinx-intc->xilinx-intc
> >
> > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> > ---
> > Changes for v2:
> > - Removed write_fn/read_fn hooks, used xintc_write/
> > xintc_read directly
> > - Moved primary_intc declaration after xintc_irq_chip
> > ---
> > drivers/irqchip/irq-xilinx-intc.c | 121
> > +++++++++++++++++++++++---------------
> > 1 file changed, 73 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-xilinx-intc.c
> > b/drivers/irqchip/irq-xilinx-intc.c
> > index e3043de..14cb630 100644
> > --- a/drivers/irqchip/irq-xilinx-intc.c
> > +++ b/drivers/irqchip/irq-xilinx-intc.c
> > @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> > void __iomem *base;
> > struct irq_domain *root_domain;
> > u32 intr_mask;
> > + struct irq_chip *intc_dev;
> > + u32 nr_irq;
>
> indentation is weird.
[Mubin]: I will fix it in next version.
>
> > };
> >
> > -static struct xintc_irq_chip *xintc_irqc;
> > +static struct xintc_irq_chip *primary_intc;
> >
> > -static void xintc_write(int reg, u32 data)
> > +static void xintc_write(void __iomem *addr, u32 data)
>
> The best would be if prototype is
> static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data)
[Mubin]: Agreed, will change it in next version.
>
>
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - iowrite32be(data, xintc_irqc->base + reg);
> > + iowrite32be(data, addr);
> > else
> > - iowrite32(data, xintc_irqc->base + reg);
> > + iowrite32(data, addr);
> > }
> >
> > -static unsigned int xintc_read(int reg)
> > +static unsigned int xintc_read(void __iomem *addr)
>
> And the same here.
> static unsigned int xintc_read(struct xintc_irq_chip *irqc, int reg)
>
[Mubin]: Sure
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - return ioread32be(xintc_irqc->base + reg);
> > + return ioread32be(addr);
> > else
> > - return ioread32(xintc_irqc->base + reg);
> > + return ioread32(addr);
> > }
> >
> > static void intc_enable_or_unmask(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
>
> code is using variable name as irqc below. I think that will be the best to use it
> in all functions. It means s/local_intc/irqc/g'
[Mubin]: Will fix it in next version
Thanks,
Mubin
>
>
> >
> > pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
> >
> > @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data
> *d)
> > * acks the irq before calling the interrupt handler
> > */
> > if (irqd_is_level_type(d))
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> >
> > - xintc_write(SIE, mask);
> > + xintc_write(local_intc->base + SIE, mask);
> > }
> >
> > static void intc_disable_or_mask(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> > - xintc_write(CIE, 1 << d->hwirq);
> > + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> > }
> >
> > static void intc_ack(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> > - xintc_write(IAR, 1 << d->hwirq);
> > + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> > }
> >
> > static void intc_mask_ack(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> >
> > pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> > - xintc_write(CIE, mask);
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + CIE, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> > }
> >
> > -static struct irq_chip intc_dev = {
> > - .name = "Xilinx INTC",
> > - .irq_unmask = intc_enable_or_unmask,
> > - .irq_mask = intc_disable_or_mask,
> > - .irq_ack = intc_ack,
> > - .irq_mask_ack = intc_mask_ack,
> > -};
> > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
> > +*local_intc) {
> > + int hwirq, irq = -1;
> > +
> > + hwirq = xintc_read(local_intc->base + IVR);
> > + if (hwirq != -1U)
> > + irq = irq_find_mapping(local_intc->root_domain, hwirq);
> > +
> > + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> > +
> > + return irq;
> > +}
> >
> > unsigned int xintc_get_irq(void)
> > {
> > - unsigned int hwirq, irq = -1;
> > + int hwirq, irq = -1;
> >
> > - hwirq = xintc_read(IVR);
> > + hwirq = xintc_read(primary_intc->base + IVR);
> > if (hwirq != -1U)
> > - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> > + irq = irq_find_mapping(primary_intc->root_domain, hwirq);
> >
> > pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> >
> > @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
> >
> > static int xintc_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hw) {
> > - if (xintc_irqc->intr_mask & (1 << hw)) {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + struct xintc_irq_chip *local_intc = d->host_data;
> > +
> > + if (local_intc->intr_mask & (1 << hw)) {
> > + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> > handle_edge_irq, "edge");
> > irq_clear_status_flags(irq, IRQ_LEVEL);
> > } else {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> > handle_level_irq, "level");
> > irq_set_status_flags(irq, IRQ_LEVEL);
> > }
> > + irq_set_chip_data(irq, local_intc);
> > return 0;
> > }
> >
> > @@ -138,11 +154,13 @@ static const struct irq_domain_ops
> > xintc_irq_domain_ops = { static void xil_intc_irq_handler(struct
> > irq_desc *desc) {
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct xintc_irq_chip *local_intc =
> > + irq_data_get_irq_handler_data(&desc->irq_data);
> > u32 pending;
> >
> > chained_irq_enter(chip, desc);
> > do {
> > - pending = xintc_get_irq();
> > + pending = xintc_get_irq_local(local_intc);
> > if (pending == -1U)
> > break;
> > generic_handle_irq(pending);
> > @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc
> > *desc) static int __init xilinx_intc_of_init(struct device_node *intc,
> > struct device_node *parent) {
> > - u32 nr_irq;
> > int ret, irq;
> > struct xintc_irq_chip *irqc;
> > -
> > - if (xintc_irqc) {
> > - pr_err("irq-xilinx: Multiple instances aren't supported\n");
> > - return -EINVAL;
> > - }
> > + struct irq_chip *intc_dev;
> >
> > irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> > if (!irqc)
> > return -ENOMEM;
> > -
> > - xintc_irqc = irqc;
> > -
> > irqc->base = of_iomap(intc, 0);
> > BUG_ON(!irqc->base);
> >
> > - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
> > + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > +&irqc->nr_irq);
> > if (ret < 0) {
> > pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
> > - goto err_alloc;
> > + goto error;
> > }
> >
> > ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> > &irqc->intr_mask); @@ -183,30 +193,42 @@ static int __init
> xilinx_intc_of_init(struct device_node *intc,
> > irqc->intr_mask = 0;
> > }
> >
> > - if (irqc->intr_mask >> nr_irq)
> > + if (irqc->intr_mask >> irqc->nr_irq)
> > pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
> >
> > pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> > - intc, nr_irq, irqc->intr_mask);
> > + intc, irqc->nr_irq, irqc->intr_mask);
> > +
> > + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> > + if (!intc_dev) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> >
> > + intc_dev->name = intc->full_name;
> > + intc_dev->irq_unmask = intc_enable_or_unmask,
> > + intc_dev->irq_mask = intc_disable_or_mask,
> > + intc_dev->irq_ack = intc_ack,
> > + intc_dev->irq_mask_ack = intc_mask_ack,
> > + irqc->intc_dev = intc_dev;
> >
> > /*
> > * Disable all external interrupts until they are
> > * explicity requested.
> > */
> > - xintc_write(IER, 0);
> > + xintc_write(irqc->base + IER, 0);
> >
> > /* Acknowledge any pending interrupts just in case. */
> > - xintc_write(IAR, 0xffffffff);
> > + xintc_write(irqc->base + IAR, 0xffffffff);
> >
> > /* Turn on the Master Enable. */
> > - xintc_write(MER, MER_HIE | MER_ME);
> > - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> > static_branch_enable(&xintc_is_be);
> > - xintc_write(MER, MER_HIE | MER_ME);
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > }
> >
> > - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> > + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> > &xintc_irq_domain_ops,
> irqc);
> > if (!irqc->root_domain) {
> > pr_err("irq-xilinx: Unable to create IRQ domain\n"); @@ -
> 225,13
> > +247,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
> > goto err_alloc;
> > }
> > } else {
> > - irq_set_default_host(irqc->root_domain);
> > + primary_intc = irqc;
> > + irq_set_default_host(primary_intc->root_domain);
> > }
> >
> > return 0;
> >
> > err_alloc:
> > - xintc_irqc = NULL;
> > + kfree(intc_dev);
> > +error:
> > + iounmap(irqc->base);
> > kfree(irqc);
> > return ret;
> >
> >
>
> Thanks,
> Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 8:02 ` Mubin Usman Sayyed
0 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-06 8:02 UTC (permalink / raw)
To: Michal Simek, tglx, jason, maz, Michal Simek, linux-arm-kernel
Cc: linux-kernel, Anirudha Sarangi, Siva Durga Prasad Paladugu
Hi Michal,
> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Wednesday, February 5, 2020 8:23 PM
> To: Mubin Usman Sayyed <MUBINUSM@xilinx.com>; tglx@linutronix.de;
> jason@lakedaemon.net; maz@kernel.org; Michal Simek
> <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org; Siva Durga Prasad Paladugu
> <sivadur@xilinx.com>; Anirudha Sarangi <anirudh@xilinx.com>
> Subject: Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
>
> Hi Mubin,
>
> On 05. 02. 20 15:05, Mubin Usman Sayyed wrote:
> > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> >
> > This patch adds support for multiple instances of xilinx interrupt
> > controller. Below configurations are supported by driver,
> >
> > - peripheral->xilinx-intc->xilinx-intc->gic
> > - peripheral->xilinx-intc->xilinx-intc
> >
> > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> > ---
> > Changes for v2:
> > - Removed write_fn/read_fn hooks, used xintc_write/
> > xintc_read directly
> > - Moved primary_intc declaration after xintc_irq_chip
> > ---
> > drivers/irqchip/irq-xilinx-intc.c | 121
> > +++++++++++++++++++++++---------------
> > 1 file changed, 73 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-xilinx-intc.c
> > b/drivers/irqchip/irq-xilinx-intc.c
> > index e3043de..14cb630 100644
> > --- a/drivers/irqchip/irq-xilinx-intc.c
> > +++ b/drivers/irqchip/irq-xilinx-intc.c
> > @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> > void __iomem *base;
> > struct irq_domain *root_domain;
> > u32 intr_mask;
> > + struct irq_chip *intc_dev;
> > + u32 nr_irq;
>
> indentation is weird.
[Mubin]: I will fix it in next version.
>
> > };
> >
> > -static struct xintc_irq_chip *xintc_irqc;
> > +static struct xintc_irq_chip *primary_intc;
> >
> > -static void xintc_write(int reg, u32 data)
> > +static void xintc_write(void __iomem *addr, u32 data)
>
> The best would be if prototype is
> static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data)
[Mubin]: Agreed, will change it in next version.
>
>
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - iowrite32be(data, xintc_irqc->base + reg);
> > + iowrite32be(data, addr);
> > else
> > - iowrite32(data, xintc_irqc->base + reg);
> > + iowrite32(data, addr);
> > }
> >
> > -static unsigned int xintc_read(int reg)
> > +static unsigned int xintc_read(void __iomem *addr)
>
> And the same here.
> static unsigned int xintc_read(struct xintc_irq_chip *irqc, int reg)
>
[Mubin]: Sure
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - return ioread32be(xintc_irqc->base + reg);
> > + return ioread32be(addr);
> > else
> > - return ioread32(xintc_irqc->base + reg);
> > + return ioread32(addr);
> > }
> >
> > static void intc_enable_or_unmask(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
>
> code is using variable name as irqc below. I think that will be the best to use it
> in all functions. It means s/local_intc/irqc/g'
[Mubin]: Will fix it in next version
Thanks,
Mubin
>
>
> >
> > pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
> >
> > @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data
> *d)
> > * acks the irq before calling the interrupt handler
> > */
> > if (irqd_is_level_type(d))
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> >
> > - xintc_write(SIE, mask);
> > + xintc_write(local_intc->base + SIE, mask);
> > }
> >
> > static void intc_disable_or_mask(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> > - xintc_write(CIE, 1 << d->hwirq);
> > + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> > }
> >
> > static void intc_ack(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> > - xintc_write(IAR, 1 << d->hwirq);
> > + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> > }
> >
> > static void intc_mask_ack(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc = irq_data_get_irq_chip_data(d);
> >
> > pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> > - xintc_write(CIE, mask);
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + CIE, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> > }
> >
> > -static struct irq_chip intc_dev = {
> > - .name = "Xilinx INTC",
> > - .irq_unmask = intc_enable_or_unmask,
> > - .irq_mask = intc_disable_or_mask,
> > - .irq_ack = intc_ack,
> > - .irq_mask_ack = intc_mask_ack,
> > -};
> > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
> > +*local_intc) {
> > + int hwirq, irq = -1;
> > +
> > + hwirq = xintc_read(local_intc->base + IVR);
> > + if (hwirq != -1U)
> > + irq = irq_find_mapping(local_intc->root_domain, hwirq);
> > +
> > + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> > +
> > + return irq;
> > +}
> >
> > unsigned int xintc_get_irq(void)
> > {
> > - unsigned int hwirq, irq = -1;
> > + int hwirq, irq = -1;
> >
> > - hwirq = xintc_read(IVR);
> > + hwirq = xintc_read(primary_intc->base + IVR);
> > if (hwirq != -1U)
> > - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> > + irq = irq_find_mapping(primary_intc->root_domain, hwirq);
> >
> > pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> >
> > @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
> >
> > static int xintc_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hw) {
> > - if (xintc_irqc->intr_mask & (1 << hw)) {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + struct xintc_irq_chip *local_intc = d->host_data;
> > +
> > + if (local_intc->intr_mask & (1 << hw)) {
> > + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> > handle_edge_irq, "edge");
> > irq_clear_status_flags(irq, IRQ_LEVEL);
> > } else {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + irq_set_chip_and_handler_name(irq, local_intc->intc_dev,
> > handle_level_irq, "level");
> > irq_set_status_flags(irq, IRQ_LEVEL);
> > }
> > + irq_set_chip_data(irq, local_intc);
> > return 0;
> > }
> >
> > @@ -138,11 +154,13 @@ static const struct irq_domain_ops
> > xintc_irq_domain_ops = { static void xil_intc_irq_handler(struct
> > irq_desc *desc) {
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct xintc_irq_chip *local_intc =
> > + irq_data_get_irq_handler_data(&desc->irq_data);
> > u32 pending;
> >
> > chained_irq_enter(chip, desc);
> > do {
> > - pending = xintc_get_irq();
> > + pending = xintc_get_irq_local(local_intc);
> > if (pending == -1U)
> > break;
> > generic_handle_irq(pending);
> > @@ -153,28 +171,20 @@ static void xil_intc_irq_handler(struct irq_desc
> > *desc) static int __init xilinx_intc_of_init(struct device_node *intc,
> > struct device_node *parent) {
> > - u32 nr_irq;
> > int ret, irq;
> > struct xintc_irq_chip *irqc;
> > -
> > - if (xintc_irqc) {
> > - pr_err("irq-xilinx: Multiple instances aren't supported\n");
> > - return -EINVAL;
> > - }
> > + struct irq_chip *intc_dev;
> >
> > irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> > if (!irqc)
> > return -ENOMEM;
> > -
> > - xintc_irqc = irqc;
> > -
> > irqc->base = of_iomap(intc, 0);
> > BUG_ON(!irqc->base);
> >
> > - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
> > + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > +&irqc->nr_irq);
> > if (ret < 0) {
> > pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n");
> > - goto err_alloc;
> > + goto error;
> > }
> >
> > ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> > &irqc->intr_mask); @@ -183,30 +193,42 @@ static int __init
> xilinx_intc_of_init(struct device_node *intc,
> > irqc->intr_mask = 0;
> > }
> >
> > - if (irqc->intr_mask >> nr_irq)
> > + if (irqc->intr_mask >> irqc->nr_irq)
> > pr_warn("irq-xilinx: mismatch in kind-of-intr param\n");
> >
> > pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> > - intc, nr_irq, irqc->intr_mask);
> > + intc, irqc->nr_irq, irqc->intr_mask);
> > +
> > + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> > + if (!intc_dev) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> >
> > + intc_dev->name = intc->full_name;
> > + intc_dev->irq_unmask = intc_enable_or_unmask,
> > + intc_dev->irq_mask = intc_disable_or_mask,
> > + intc_dev->irq_ack = intc_ack,
> > + intc_dev->irq_mask_ack = intc_mask_ack,
> > + irqc->intc_dev = intc_dev;
> >
> > /*
> > * Disable all external interrupts until they are
> > * explicity requested.
> > */
> > - xintc_write(IER, 0);
> > + xintc_write(irqc->base + IER, 0);
> >
> > /* Acknowledge any pending interrupts just in case. */
> > - xintc_write(IAR, 0xffffffff);
> > + xintc_write(irqc->base + IAR, 0xffffffff);
> >
> > /* Turn on the Master Enable. */
> > - xintc_write(MER, MER_HIE | MER_ME);
> > - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> > static_branch_enable(&xintc_is_be);
> > - xintc_write(MER, MER_HIE | MER_ME);
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > }
> >
> > - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> > + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> > &xintc_irq_domain_ops,
> irqc);
> > if (!irqc->root_domain) {
> > pr_err("irq-xilinx: Unable to create IRQ domain\n"); @@ -
> 225,13
> > +247,16 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
> > goto err_alloc;
> > }
> > } else {
> > - irq_set_default_host(irqc->root_domain);
> > + primary_intc = irqc;
> > + irq_set_default_host(primary_intc->root_domain);
> > }
> >
> > return 0;
> >
> > err_alloc:
> > - xintc_irqc = NULL;
> > + kfree(intc_dev);
> > +error:
> > + iounmap(irqc->base);
> > kfree(irqc);
> > return ret;
> >
> >
>
> Thanks,
> Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 14:54 ` Michal Simek
@ 2020-02-06 8:07 ` Mubin Usman Sayyed
-1 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-06 8:07 UTC (permalink / raw)
To: Michal Simek, David Laight, tglx, jason, maz, Michal Simek,
linux-arm-kernel
Cc: linux-kernel, Siva Durga Prasad Paladugu, Anirudha Sarangi
Hi,
> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Wednesday, February 5, 2020 8:24 PM
> To: David Laight <David.Laight@ACULAB.COM>; Mubin Usman Sayyed
> <MUBINUSM@xilinx.com>; tglx@linutronix.de; jason@lakedaemon.net;
> maz@kernel.org; Michal Simek <michals@xilinx.com>; linux-arm-
> kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org; Siva Durga Prasad Paladugu
> <sivadur@xilinx.com>; Anirudha Sarangi <anirudh@xilinx.com>
> Subject: Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
>
> On 05. 02. 20 15:15, David Laight wrote:
> >> This email and any attachments are intended for the sole use of the
> >> named recipient(s) and contain(s) confidential information that may
> >> be proprietary, privileged or copyrighted under applicable law. If
> >> you are not the intended recipient, do not read, copy, or forward this
> email message or any attachments. Delete this email message and any
> attachments immediately.
> >
> > Deleted.....
>
> :-) I got two copies. One without it :-)
>
> Mubin: Please fix it.
Sorry, I missed to fix that footer, will do it from next version.
Thanks,
Mubin
>
> Thanks,
> Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 8:07 ` Mubin Usman Sayyed
0 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-06 8:07 UTC (permalink / raw)
To: Michal Simek, David Laight, tglx, jason, maz, Michal Simek,
linux-arm-kernel
Cc: linux-kernel, Anirudha Sarangi, Siva Durga Prasad Paladugu
Hi,
> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Wednesday, February 5, 2020 8:24 PM
> To: David Laight <David.Laight@ACULAB.COM>; Mubin Usman Sayyed
> <MUBINUSM@xilinx.com>; tglx@linutronix.de; jason@lakedaemon.net;
> maz@kernel.org; Michal Simek <michals@xilinx.com>; linux-arm-
> kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org; Siva Durga Prasad Paladugu
> <sivadur@xilinx.com>; Anirudha Sarangi <anirudh@xilinx.com>
> Subject: Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
>
> On 05. 02. 20 15:15, David Laight wrote:
> >> This email and any attachments are intended for the sole use of the
> >> named recipient(s) and contain(s) confidential information that may
> >> be proprietary, privileged or copyrighted under applicable law. If
> >> you are not the intended recipient, do not read, copy, or forward this
> email message or any attachments. Delete this email message and any
> attachments immediately.
> >
> > Deleted.....
>
> :-) I got two copies. One without it :-)
>
> Mubin: Please fix it.
Sorry, I missed to fix that footer, will do it from next version.
Thanks,
Mubin
>
> Thanks,
> Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-06 7:06 ` Michal Simek
@ 2020-02-06 9:09 ` Marc Zyngier
-1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2020-02-06 9:09 UTC (permalink / raw)
To: Michal Simek
Cc: Mubin Usman Sayyed, tglx, jason, linux-arm-kernel, linux-kernel,
siva.durga.paladugu, anirudha.sarangi
On 2020-02-06 07:06, Michal Simek wrote:
> On 05. 02. 20 17:53, Marc Zyngier wrote:
>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
[...]
>>> unsigned int xintc_get_irq(void)
>>> {
>>> - unsigned int hwirq, irq = -1;
>>> + int hwirq, irq = -1;
>>>
>>> - hwirq = xintc_read(IVR);
>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>> if (hwirq != -1U)
>>> - irq = irq_find_mapping(xintc_irqc->root_domain,
>>> hwirq);
>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>> hwirq);
>>>
>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>
>> I have the ugly feeling I'm reading the same code twice... Surely you
>> can
>> make these two functions common code.
>
> I have some questions regarding this.
> I have updated one patchset which is adding support for Microblaze SMP.
> And when I was looking at current wiring of this driver I have decided
> to change it.
>
> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
> This driver calls set_handle_irq(xil_intc_handle_irq)
> and MB do_IRQ() call handle_arch_irq()
> and IRQ routine here is using handle_domain_irq().
>
> I would expect that this chained IRQ handler can also use
> handle_domain_irq().
>
> Is that correct understanding?
handle_domain_irq() implies that you have a set of pt_regs, representing
the context you interrupted. You can't fake that up, so I can't see how
you use it in a chained context.
[...]
>>> + intc_dev->name = intc->full_name;
>>
>> No. The world doesn't need to see the OF path of your interrupt
>> controller in /proc/cpuinfo.
>> The name that was there before was perfectly descriptive, please stick
>> to it.
>
> It should be showing name like interrupt-controller@41800000.
> Do you think that we really should stick with just fixed name?
> There could be multiple instances in the system and you will have no
> idea how they are connected.
What is that used for? Debugging. We have a whole infrastructure for
that
(GENERIC_IRQ_DEBUGFS), which is the right tool for the job. If it needs
improvement, please let me know what is missing.
Also, anything in /proc is ABI, so we don't change it randomly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 9:09 ` Marc Zyngier
0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2020-02-06 9:09 UTC (permalink / raw)
To: Michal Simek
Cc: jason, Mubin Usman Sayyed, anirudha.sarangi, linux-kernel, tglx,
siva.durga.paladugu, linux-arm-kernel
On 2020-02-06 07:06, Michal Simek wrote:
> On 05. 02. 20 17:53, Marc Zyngier wrote:
>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
[...]
>>> unsigned int xintc_get_irq(void)
>>> {
>>> - unsigned int hwirq, irq = -1;
>>> + int hwirq, irq = -1;
>>>
>>> - hwirq = xintc_read(IVR);
>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>> if (hwirq != -1U)
>>> - irq = irq_find_mapping(xintc_irqc->root_domain,
>>> hwirq);
>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>> hwirq);
>>>
>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>
>> I have the ugly feeling I'm reading the same code twice... Surely you
>> can
>> make these two functions common code.
>
> I have some questions regarding this.
> I have updated one patchset which is adding support for Microblaze SMP.
> And when I was looking at current wiring of this driver I have decided
> to change it.
>
> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
> This driver calls set_handle_irq(xil_intc_handle_irq)
> and MB do_IRQ() call handle_arch_irq()
> and IRQ routine here is using handle_domain_irq().
>
> I would expect that this chained IRQ handler can also use
> handle_domain_irq().
>
> Is that correct understanding?
handle_domain_irq() implies that you have a set of pt_regs, representing
the context you interrupted. You can't fake that up, so I can't see how
you use it in a chained context.
[...]
>>> + intc_dev->name = intc->full_name;
>>
>> No. The world doesn't need to see the OF path of your interrupt
>> controller in /proc/cpuinfo.
>> The name that was there before was perfectly descriptive, please stick
>> to it.
>
> It should be showing name like interrupt-controller@41800000.
> Do you think that we really should stick with just fixed name?
> There could be multiple instances in the system and you will have no
> idea how they are connected.
What is that used for? Debugging. We have a whole infrastructure for
that
(GENERIC_IRQ_DEBUGFS), which is the right tool for the job. If it needs
improvement, please let me know what is missing.
Also, anything in /proc is ABI, so we don't change it randomly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-06 9:09 ` Marc Zyngier
@ 2020-02-06 9:11 ` Michal Simek
-1 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-06 9:11 UTC (permalink / raw)
To: Marc Zyngier, Michal Simek
Cc: Mubin Usman Sayyed, tglx, jason, linux-arm-kernel, linux-kernel,
siva.durga.paladugu, anirudha.sarangi
On 06. 02. 20 10:09, Marc Zyngier wrote:
> On 2020-02-06 07:06, Michal Simek wrote:
>> On 05. 02. 20 17:53, Marc Zyngier wrote:
>>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>
> [...]
>
>>>> unsigned int xintc_get_irq(void)
>>>> {
>>>> - unsigned int hwirq, irq = -1;
>>>> + int hwirq, irq = -1;
>>>>
>>>> - hwirq = xintc_read(IVR);
>>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>>> if (hwirq != -1U)
>>>> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
>>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>>> hwirq);
>>>>
>>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>>
>>> I have the ugly feeling I'm reading the same code twice... Surely you
>>> can
>>> make these two functions common code.
>>
>> I have some questions regarding this.
>> I have updated one patchset which is adding support for Microblaze SMP.
>> And when I was looking at current wiring of this driver I have decided
>> to change it.
>>
>> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
>> This driver calls set_handle_irq(xil_intc_handle_irq)
>> and MB do_IRQ() call handle_arch_irq()
>> and IRQ routine here is using handle_domain_irq().
>>
>> I would expect that this chained IRQ handler can also use
>> handle_domain_irq().
>>
>> Is that correct understanding?
>
> handle_domain_irq() implies that you have a set of pt_regs, representing
> the context you interrupted. You can't fake that up, so I can't see how
> you use it in a chained context.
ok. What's your recommendation for chained controller? Just go with
irq_find_mapping?
>
> [...]
>
>>>> + intc_dev->name = intc->full_name;
>>>
>>> No. The world doesn't need to see the OF path of your interrupt
>>> controller in /proc/cpuinfo.
>>> The name that was there before was perfectly descriptive, please stick
>>> to it.
>>
>> It should be showing name like interrupt-controller@41800000.
>> Do you think that we really should stick with just fixed name?
>> There could be multiple instances in the system and you will have no
>> idea how they are connected.
>
> What is that used for? Debugging. We have a whole infrastructure for that
> (GENERIC_IRQ_DEBUGFS), which is the right tool for the job. If it needs
> improvement, please let me know what is missing.
Let me take a look.
> Also, anything in /proc is ABI, so we don't change it randomly.
ok.
Thanks,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 9:11 ` Michal Simek
0 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-06 9:11 UTC (permalink / raw)
To: Marc Zyngier, Michal Simek
Cc: jason, Mubin Usman Sayyed, anirudha.sarangi, linux-kernel, tglx,
siva.durga.paladugu, linux-arm-kernel
On 06. 02. 20 10:09, Marc Zyngier wrote:
> On 2020-02-06 07:06, Michal Simek wrote:
>> On 05. 02. 20 17:53, Marc Zyngier wrote:
>>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>
> [...]
>
>>>> unsigned int xintc_get_irq(void)
>>>> {
>>>> - unsigned int hwirq, irq = -1;
>>>> + int hwirq, irq = -1;
>>>>
>>>> - hwirq = xintc_read(IVR);
>>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>>> if (hwirq != -1U)
>>>> - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
>>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>>> hwirq);
>>>>
>>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>>
>>> I have the ugly feeling I'm reading the same code twice... Surely you
>>> can
>>> make these two functions common code.
>>
>> I have some questions regarding this.
>> I have updated one patchset which is adding support for Microblaze SMP.
>> And when I was looking at current wiring of this driver I have decided
>> to change it.
>>
>> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
>> This driver calls set_handle_irq(xil_intc_handle_irq)
>> and MB do_IRQ() call handle_arch_irq()
>> and IRQ routine here is using handle_domain_irq().
>>
>> I would expect that this chained IRQ handler can also use
>> handle_domain_irq().
>>
>> Is that correct understanding?
>
> handle_domain_irq() implies that you have a set of pt_regs, representing
> the context you interrupted. You can't fake that up, so I can't see how
> you use it in a chained context.
ok. What's your recommendation for chained controller? Just go with
irq_find_mapping?
>
> [...]
>
>>>> + intc_dev->name = intc->full_name;
>>>
>>> No. The world doesn't need to see the OF path of your interrupt
>>> controller in /proc/cpuinfo.
>>> The name that was there before was perfectly descriptive, please stick
>>> to it.
>>
>> It should be showing name like interrupt-controller@41800000.
>> Do you think that we really should stick with just fixed name?
>> There could be multiple instances in the system and you will have no
>> idea how they are connected.
>
> What is that used for? Debugging. We have a whole infrastructure for that
> (GENERIC_IRQ_DEBUGFS), which is the right tool for the job. If it needs
> improvement, please let me know what is missing.
Let me take a look.
> Also, anything in /proc is ABI, so we don't change it randomly.
ok.
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-06 9:11 ` Michal Simek
@ 2020-02-06 9:15 ` Marc Zyngier
-1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2020-02-06 9:15 UTC (permalink / raw)
To: Michal Simek
Cc: Mubin Usman Sayyed, tglx, jason, linux-arm-kernel, linux-kernel,
siva.durga.paladugu, anirudha.sarangi
On 2020-02-06 09:11, Michal Simek wrote:
> On 06. 02. 20 10:09, Marc Zyngier wrote:
>> On 2020-02-06 07:06, Michal Simek wrote:
>>> On 05. 02. 20 17:53, Marc Zyngier wrote:
>>>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>>
>> [...]
>>
>>>>> unsigned int xintc_get_irq(void)
>>>>> {
>>>>> - unsigned int hwirq, irq = -1;
>>>>> + int hwirq, irq = -1;
>>>>>
>>>>> - hwirq = xintc_read(IVR);
>>>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>>>> if (hwirq != -1U)
>>>>> - irq = irq_find_mapping(xintc_irqc->root_domain,
>>>>> hwirq);
>>>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>>>> hwirq);
>>>>>
>>>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>>>
>>>> I have the ugly feeling I'm reading the same code twice... Surely
>>>> you
>>>> can
>>>> make these two functions common code.
>>>
>>> I have some questions regarding this.
>>> I have updated one patchset which is adding support for Microblaze
>>> SMP.
>>> And when I was looking at current wiring of this driver I have
>>> decided
>>> to change it.
>>>
>>> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
>>> This driver calls set_handle_irq(xil_intc_handle_irq)
>>> and MB do_IRQ() call handle_arch_irq()
>>> and IRQ routine here is using handle_domain_irq().
>>>
>>> I would expect that this chained IRQ handler can also use
>>> handle_domain_irq().
>>>
>>> Is that correct understanding?
>>
>> handle_domain_irq() implies that you have a set of pt_regs,
>> representing
>> the context you interrupted. You can't fake that up, so I can't see
>> how
>> you use it in a chained context.
>
> ok. What's your recommendation for chained controller? Just go with
> irq_find_mapping?
For now, yes. I have (distant) plans to improve this.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 9:15 ` Marc Zyngier
0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2020-02-06 9:15 UTC (permalink / raw)
To: Michal Simek
Cc: jason, Mubin Usman Sayyed, anirudha.sarangi, linux-kernel, tglx,
siva.durga.paladugu, linux-arm-kernel
On 2020-02-06 09:11, Michal Simek wrote:
> On 06. 02. 20 10:09, Marc Zyngier wrote:
>> On 2020-02-06 07:06, Michal Simek wrote:
>>> On 05. 02. 20 17:53, Marc Zyngier wrote:
>>>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>>
>> [...]
>>
>>>>> unsigned int xintc_get_irq(void)
>>>>> {
>>>>> - unsigned int hwirq, irq = -1;
>>>>> + int hwirq, irq = -1;
>>>>>
>>>>> - hwirq = xintc_read(IVR);
>>>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>>>> if (hwirq != -1U)
>>>>> - irq = irq_find_mapping(xintc_irqc->root_domain,
>>>>> hwirq);
>>>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>>>> hwirq);
>>>>>
>>>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>>>
>>>> I have the ugly feeling I'm reading the same code twice... Surely
>>>> you
>>>> can
>>>> make these two functions common code.
>>>
>>> I have some questions regarding this.
>>> I have updated one patchset which is adding support for Microblaze
>>> SMP.
>>> And when I was looking at current wiring of this driver I have
>>> decided
>>> to change it.
>>>
>>> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
>>> This driver calls set_handle_irq(xil_intc_handle_irq)
>>> and MB do_IRQ() call handle_arch_irq()
>>> and IRQ routine here is using handle_domain_irq().
>>>
>>> I would expect that this chained IRQ handler can also use
>>> handle_domain_irq().
>>>
>>> Is that correct understanding?
>>
>> handle_domain_irq() implies that you have a set of pt_regs,
>> representing
>> the context you interrupted. You can't fake that up, so I can't see
>> how
>> you use it in a chained context.
>
> ok. What's your recommendation for chained controller? Just go with
> irq_find_mapping?
For now, yes. I have (distant) plans to improve this.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-06 9:15 ` Marc Zyngier
@ 2020-02-06 9:17 ` Michal Simek
-1 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-06 9:17 UTC (permalink / raw)
To: Marc Zyngier, Michal Simek
Cc: Mubin Usman Sayyed, tglx, jason, linux-arm-kernel, linux-kernel,
siva.durga.paladugu, anirudha.sarangi
On 06. 02. 20 10:15, Marc Zyngier wrote:
> On 2020-02-06 09:11, Michal Simek wrote:
>> On 06. 02. 20 10:09, Marc Zyngier wrote:
>>> On 2020-02-06 07:06, Michal Simek wrote:
>>>> On 05. 02. 20 17:53, Marc Zyngier wrote:
>>>>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>>>
>>> [...]
>>>
>>>>>> unsigned int xintc_get_irq(void)
>>>>>> {
>>>>>> - unsigned int hwirq, irq = -1;
>>>>>> + int hwirq, irq = -1;
>>>>>>
>>>>>> - hwirq = xintc_read(IVR);
>>>>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>>>>> if (hwirq != -1U)
>>>>>> - irq = irq_find_mapping(xintc_irqc->root_domain,
>>>>>> hwirq);
>>>>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>>>>> hwirq);
>>>>>>
>>>>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>>>>
>>>>> I have the ugly feeling I'm reading the same code twice... Surely you
>>>>> can
>>>>> make these two functions common code.
>>>>
>>>> I have some questions regarding this.
>>>> I have updated one patchset which is adding support for Microblaze SMP.
>>>> And when I was looking at current wiring of this driver I have decided
>>>> to change it.
>>>>
>>>> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
>>>> This driver calls set_handle_irq(xil_intc_handle_irq)
>>>> and MB do_IRQ() call handle_arch_irq()
>>>> and IRQ routine here is using handle_domain_irq().
>>>>
>>>> I would expect that this chained IRQ handler can also use
>>>> handle_domain_irq().
>>>>
>>>> Is that correct understanding?
>>>
>>> handle_domain_irq() implies that you have a set of pt_regs, representing
>>> the context you interrupted. You can't fake that up, so I can't see how
>>> you use it in a chained context.
>>
>> ok. What's your recommendation for chained controller? Just go with
>> irq_find_mapping?
>
> For now, yes. I have (distant) plans to improve this.
Thanks.
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-06 9:17 ` Michal Simek
0 siblings, 0 replies; 26+ messages in thread
From: Michal Simek @ 2020-02-06 9:17 UTC (permalink / raw)
To: Marc Zyngier, Michal Simek
Cc: jason, Mubin Usman Sayyed, anirudha.sarangi, linux-kernel, tglx,
siva.durga.paladugu, linux-arm-kernel
On 06. 02. 20 10:15, Marc Zyngier wrote:
> On 2020-02-06 09:11, Michal Simek wrote:
>> On 06. 02. 20 10:09, Marc Zyngier wrote:
>>> On 2020-02-06 07:06, Michal Simek wrote:
>>>> On 05. 02. 20 17:53, Marc Zyngier wrote:
>>>>> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>>>
>>> [...]
>>>
>>>>>> unsigned int xintc_get_irq(void)
>>>>>> {
>>>>>> - unsigned int hwirq, irq = -1;
>>>>>> + int hwirq, irq = -1;
>>>>>>
>>>>>> - hwirq = xintc_read(IVR);
>>>>>> + hwirq = xintc_read(primary_intc->base + IVR);
>>>>>> if (hwirq != -1U)
>>>>>> - irq = irq_find_mapping(xintc_irqc->root_domain,
>>>>>> hwirq);
>>>>>> + irq = irq_find_mapping(primary_intc->root_domain,
>>>>>> hwirq);
>>>>>>
>>>>>> pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>>>>>
>>>>> I have the ugly feeling I'm reading the same code twice... Surely you
>>>>> can
>>>>> make these two functions common code.
>>>>
>>>> I have some questions regarding this.
>>>> I have updated one patchset which is adding support for Microblaze SMP.
>>>> And when I was looking at current wiring of this driver I have decided
>>>> to change it.
>>>>
>>>> I have enabled GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
>>>> This driver calls set_handle_irq(xil_intc_handle_irq)
>>>> and MB do_IRQ() call handle_arch_irq()
>>>> and IRQ routine here is using handle_domain_irq().
>>>>
>>>> I would expect that this chained IRQ handler can also use
>>>> handle_domain_irq().
>>>>
>>>> Is that correct understanding?
>>>
>>> handle_domain_irq() implies that you have a set of pt_regs, representing
>>> the context you interrupted. You can't fake that up, so I can't see how
>>> you use it in a chained context.
>>
>> ok. What's your recommendation for chained controller? Just go with
>> irq_find_mapping?
>
> For now, yes. I have (distant) plans to improve this.
Thanks.
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
2020-02-05 16:53 ` Marc Zyngier
@ 2020-02-11 19:38 ` Mubin Usman Sayyed
-1 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-11 19:38 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, Michal Simek, linux-arm-kernel, linux-kernel,
Siva Durga Prasad Paladugu, Anirudha Sarangi
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Wednesday, February 5, 2020 10:23 PM
> To: Mubin Usman Sayyed <MUBINUSM@xilinx.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; Michal Simek
> <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Siva Durga Prasad Paladugu <sivadur@xilinx.com>;
> Anirudha Sarangi <anirudh@xilinx.com>
> Subject: Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
>
> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
> > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> >
> > This patch adds support for multiple instances of xilinx interrupt
> > controller. Below configurations are supported by driver,
> >
> > - peripheral->xilinx-intc->xilinx-intc->gic
> > - peripheral->xilinx-intc->xilinx-intc
> >
> > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> > ---
> > Changes for v2:
> > - Removed write_fn/read_fn hooks, used xintc_write/
> > xintc_read directly
> > - Moved primary_intc declaration after xintc_irq_chip
> > ---
> > drivers/irqchip/irq-xilinx-intc.c | 121
> > +++++++++++++++++++++++---------------
> > 1 file changed, 73 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-xilinx-intc.c
> > b/drivers/irqchip/irq-xilinx-intc.c
> > index e3043de..14cb630 100644
> > --- a/drivers/irqchip/irq-xilinx-intc.c
> > +++ b/drivers/irqchip/irq-xilinx-intc.c
> > @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> > void __iomem *base;
> > struct irq_domain *root_domain;
> > u32 intr_mask;
> > + struct irq_chip *intc_dev;
> > + u32 nr_irq;
> > };
> >
> > -static struct xintc_irq_chip *xintc_irqc;
> > +static struct xintc_irq_chip *primary_intc;
> >
> > -static void xintc_write(int reg, u32 data)
> > +static void xintc_write(void __iomem *addr, u32 data)
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - iowrite32be(data, xintc_irqc->base + reg);
> > + iowrite32be(data, addr);
> > else
> > - iowrite32(data, xintc_irqc->base + reg);
> > + iowrite32(data, addr);
> > }
> >
> > -static unsigned int xintc_read(int reg)
> > +static unsigned int xintc_read(void __iomem *addr)
>
> Since you are changing this, please change the return value to reflect the size
> of what you're returning (u32 instead of unsigned int).
[Mubin]: Changed in v3
>
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - return ioread32be(xintc_irqc->base + reg);
> > + return ioread32be(addr);
> > else
> > - return ioread32(xintc_irqc->base + reg);
> > + return ioread32(addr);
> > }
> >
> > static void intc_enable_or_unmask(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> >
> > pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
> >
> > @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data
> > *d)
> > * acks the irq before calling the interrupt handler
> > */
> > if (irqd_is_level_type(d))
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> >
> > - xintc_write(SIE, mask);
> > + xintc_write(local_intc->base + SIE, mask);
> > }
> >
> > static void intc_disable_or_mask(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> > - xintc_write(CIE, 1 << d->hwirq);
> > + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> > }
> >
> > static void intc_ack(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> > - xintc_write(IAR, 1 << d->hwirq);
> > + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> > }
> >
> > static void intc_mask_ack(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> >
> > pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> > - xintc_write(CIE, mask);
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + CIE, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> > }
> >
> > -static struct irq_chip intc_dev = {
> > - .name = "Xilinx INTC",
> > - .irq_unmask = intc_enable_or_unmask,
> > - .irq_mask = intc_disable_or_mask,
> > - .irq_ack = intc_ack,
> > - .irq_mask_ack = intc_mask_ack,
> > -};
> > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
> > *local_intc)
> > +{
> > + int hwirq, irq = -1;
>
> Type consistency for hwirq.
[Mubin]: Fixed in v3
>
> > +
> > + hwirq = xintc_read(local_intc->base + IVR);
> > + if (hwirq != -1U)
> > + irq = irq_find_mapping(local_intc->root_domain,
> > + hwirq);
> > +
> > + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> > +
> > + return irq;
>
> That now gives you both -1 and 0 for error values. Please stick with 0.
[Mubin]: Fixed in v3
>
> > +}
> >
> > unsigned int xintc_get_irq(void)
> > {
> > - unsigned int hwirq, irq = -1;
> > + int hwirq, irq = -1;
> >
> > - hwirq = xintc_read(IVR);
> > + hwirq = xintc_read(primary_intc->base + IVR);
> > if (hwirq != -1U)
> > - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> > + irq = irq_find_mapping(primary_intc->root_domain,
> > hwirq);
> >
> > pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>
> I have the ugly feeling I'm reading the same code twice... Surely you can
> make these two functions common code.
[Mubin]: Upcoming patchset from Michal would be removing xintc_get_irq, instead of that "handle_domain_irq()" would be used independently. I hope that is fine.
> >
> > @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
> >
> > static int xintc_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hw) {
> > - if (xintc_irqc->intr_mask & (1 << hw)) {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + struct xintc_irq_chip *local_intc = d->host_data;
> > +
> > + if (local_intc->intr_mask & (1 << hw)) {
>
> BIT(hw)
[Mubin]: Fixed in v3
>
> > + irq_set_chip_and_handler_name(irq,
> > local_intc->intc_dev,
> > handle_edge_irq,
> > "edge");
> > irq_clear_status_flags(irq, IRQ_LEVEL);
> > } else {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + irq_set_chip_and_handler_name(irq,
> > local_intc->intc_dev,
> > handle_level_irq,
> > "level");
> > irq_set_status_flags(irq, IRQ_LEVEL);
> > }
> > + irq_set_chip_data(irq, local_intc);
> > return 0;
> > }
> >
> > @@ -138,11 +154,13 @@ static const struct irq_domain_ops
> > xintc_irq_domain_ops = { static void xil_intc_irq_handler(struct
> > irq_desc *desc) {
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct xintc_irq_chip *local_intc =
> > + irq_data_get_irq_handler_data(&desc->irq_data);
> > u32 pending;
> >
> > chained_irq_enter(chip, desc);
> > do {
> > - pending = xintc_get_irq();
> > + pending = xintc_get_irq_local(local_intc);
> > if (pending == -1U)
> > break;
> > generic_handle_irq(pending); @@ -153,28 +171,20 @@
> > static void xil_intc_irq_handler(struct irq_desc
> > *desc)
> > static int __init xilinx_intc_of_init(struct device_node *intc,
> > struct device_node
> > *parent)
> > {
> > - u32 nr_irq;
> > int ret, irq;
> > struct xintc_irq_chip *irqc;
> > -
> > - if (xintc_irqc) {
> > - pr_err("irq-xilinx: Multiple instances aren't
> > supported\n");
> > - return -EINVAL;
> > - }
> > + struct irq_chip *intc_dev;
> >
> > irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> > if (!irqc)
> > return -ENOMEM;
> > -
> > - xintc_irqc = irqc;
> > -
> > irqc->base = of_iomap(intc, 0);
> > BUG_ON(!irqc->base);
> >
> > - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > &nr_irq);
> > + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > &irqc->nr_irq);
> > if (ret < 0) {
> > pr_err("irq-xilinx: unable to read
> > xlnx,num-intr-inputs\n");
> > - goto err_alloc;
> > + goto error;
> > }
> >
> > ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> > &irqc->intr_mask); @@ -183,30 +193,42 @@ static int __init
> > xilinx_intc_of_init(struct device_node *intc,
> > irqc->intr_mask = 0;
> > }
> >
> > - if (irqc->intr_mask >> nr_irq)
> > + if (irqc->intr_mask >> irqc->nr_irq)
> > pr_warn("irq-xilinx: mismatch in kind-of-intr
> > param\n");
> >
> > pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> > - intc, nr_irq, irqc->intr_mask);
> > + intc, irqc->nr_irq, irqc->intr_mask);
> > +
> > + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> > + if (!intc_dev) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> >
> > + intc_dev->name = intc->full_name;
>
> No. The world doesn't need to see the OF path of your interrupt controller in
> /proc/cpuinfo.
> The name that was there before was perfectly descriptive, please stick to it.
[Mubin]: Reverted in v3
>
> > + intc_dev->irq_unmask = intc_enable_or_unmask,
> > + intc_dev->irq_mask = intc_disable_or_mask,
> > + intc_dev->irq_ack = intc_ack,
> > + intc_dev->irq_mask_ack = intc_mask_ack,
>
> And this structure should stay static, as it was before.
[Mubin]: Reverted in v3
>
> > + irqc->intc_dev = intc_dev;
> >
> > /*
> > * Disable all external interrupts until they are
> > * explicity requested.
> > */
> > - xintc_write(IER, 0);
> > + xintc_write(irqc->base + IER, 0);
> >
> > /* Acknowledge any pending interrupts just in case. */
> > - xintc_write(IAR, 0xffffffff);
> > + xintc_write(irqc->base + IAR, 0xffffffff);
> >
> > /* Turn on the Master Enable. */
> > - xintc_write(MER, MER_HIE | MER_ME);
> > - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> > static_branch_enable(&xintc_is_be);
> > - xintc_write(MER, MER_HIE | MER_ME);
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > }
> >
> > - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> > + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> >
> > &xintc_irq_domain_ops, irqc);
> > if (!irqc->root_domain) {
> > pr_err("irq-xilinx: Unable to create IRQ domain\n");
> > @@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct
> > device_node *intc,
> > goto err_alloc;
> > }
> > } else {
> > - irq_set_default_host(irqc->root_domain);
> > + primary_intc = irqc;
> > + irq_set_default_host(primary_intc->root_domain);
> > }
> >
> > return 0;
> >
> > err_alloc:
> > - xintc_irqc = NULL;
> > + kfree(intc_dev);
> > +error:
> > + iounmap(irqc->base);
> > kfree(irqc);
> > return ret;
> >
> > --
> > 2.7.4
> >
> > This email and any attachments are intended for the sole use of the
> > named recipient(s) and contain(s) confidential information that may be
> > proprietary, privileged or copyrighted under applicable law. If you
> > are not the intended recipient, do not read, copy, or forward this
> > email message or any attachments. Delete this email message and any
> > attachments immediately.
>
> Please tell your employer to fix their email server.
[Mubin]: Fixed in v3
Thanks,
Mubin
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v2] irqchip: xilinx: Add support for multiple instances
@ 2020-02-11 19:38 ` Mubin Usman Sayyed
0 siblings, 0 replies; 26+ messages in thread
From: Mubin Usman Sayyed @ 2020-02-11 19:38 UTC (permalink / raw)
To: Marc Zyngier
Cc: jason, Siva Durga Prasad Paladugu, linux-kernel, Michal Simek,
Anirudha Sarangi, tglx, linux-arm-kernel
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Wednesday, February 5, 2020 10:23 PM
> To: Mubin Usman Sayyed <MUBINUSM@xilinx.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; Michal Simek
> <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Siva Durga Prasad Paladugu <sivadur@xilinx.com>;
> Anirudha Sarangi <anirudh@xilinx.com>
> Subject: Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances
>
> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
> > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> >
> > This patch adds support for multiple instances of xilinx interrupt
> > controller. Below configurations are supported by driver,
> >
> > - peripheral->xilinx-intc->xilinx-intc->gic
> > - peripheral->xilinx-intc->xilinx-intc
> >
> > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com>
> > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com>
> > ---
> > Changes for v2:
> > - Removed write_fn/read_fn hooks, used xintc_write/
> > xintc_read directly
> > - Moved primary_intc declaration after xintc_irq_chip
> > ---
> > drivers/irqchip/irq-xilinx-intc.c | 121
> > +++++++++++++++++++++++---------------
> > 1 file changed, 73 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-xilinx-intc.c
> > b/drivers/irqchip/irq-xilinx-intc.c
> > index e3043de..14cb630 100644
> > --- a/drivers/irqchip/irq-xilinx-intc.c
> > +++ b/drivers/irqchip/irq-xilinx-intc.c
> > @@ -38,29 +38,32 @@ struct xintc_irq_chip {
> > void __iomem *base;
> > struct irq_domain *root_domain;
> > u32 intr_mask;
> > + struct irq_chip *intc_dev;
> > + u32 nr_irq;
> > };
> >
> > -static struct xintc_irq_chip *xintc_irqc;
> > +static struct xintc_irq_chip *primary_intc;
> >
> > -static void xintc_write(int reg, u32 data)
> > +static void xintc_write(void __iomem *addr, u32 data)
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - iowrite32be(data, xintc_irqc->base + reg);
> > + iowrite32be(data, addr);
> > else
> > - iowrite32(data, xintc_irqc->base + reg);
> > + iowrite32(data, addr);
> > }
> >
> > -static unsigned int xintc_read(int reg)
> > +static unsigned int xintc_read(void __iomem *addr)
>
> Since you are changing this, please change the return value to reflect the size
> of what you're returning (u32 instead of unsigned int).
[Mubin]: Changed in v3
>
> > {
> > if (static_branch_unlikely(&xintc_is_be))
> > - return ioread32be(xintc_irqc->base + reg);
> > + return ioread32be(addr);
> > else
> > - return ioread32(xintc_irqc->base + reg);
> > + return ioread32(addr);
> > }
> >
> > static void intc_enable_or_unmask(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> >
> > pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
> >
> > @@ -69,47 +72,57 @@ static void intc_enable_or_unmask(struct irq_data
> > *d)
> > * acks the irq before calling the interrupt handler
> > */
> > if (irqd_is_level_type(d))
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> >
> > - xintc_write(SIE, mask);
> > + xintc_write(local_intc->base + SIE, mask);
> > }
> >
> > static void intc_disable_or_mask(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> > - xintc_write(CIE, 1 << d->hwirq);
> > + xintc_write(local_intc->base + CIE, 1 << d->hwirq);
> > }
> >
> > static void intc_ack(struct irq_data *d) {
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> > +
> > pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> > - xintc_write(IAR, 1 << d->hwirq);
> > + xintc_write(local_intc->base + IAR, 1 << d->hwirq);
> > }
> >
> > static void intc_mask_ack(struct irq_data *d) {
> > unsigned long mask = 1 << d->hwirq;
> > + struct xintc_irq_chip *local_intc =
> > irq_data_get_irq_chip_data(d);
> >
> > pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> > - xintc_write(CIE, mask);
> > - xintc_write(IAR, mask);
> > + xintc_write(local_intc->base + CIE, mask);
> > + xintc_write(local_intc->base + IAR, mask);
> > }
> >
> > -static struct irq_chip intc_dev = {
> > - .name = "Xilinx INTC",
> > - .irq_unmask = intc_enable_or_unmask,
> > - .irq_mask = intc_disable_or_mask,
> > - .irq_ack = intc_ack,
> > - .irq_mask_ack = intc_mask_ack,
> > -};
> > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
> > *local_intc)
> > +{
> > + int hwirq, irq = -1;
>
> Type consistency for hwirq.
[Mubin]: Fixed in v3
>
> > +
> > + hwirq = xintc_read(local_intc->base + IVR);
> > + if (hwirq != -1U)
> > + irq = irq_find_mapping(local_intc->root_domain,
> > + hwirq);
> > +
> > + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> > +
> > + return irq;
>
> That now gives you both -1 and 0 for error values. Please stick with 0.
[Mubin]: Fixed in v3
>
> > +}
> >
> > unsigned int xintc_get_irq(void)
> > {
> > - unsigned int hwirq, irq = -1;
> > + int hwirq, irq = -1;
> >
> > - hwirq = xintc_read(IVR);
> > + hwirq = xintc_read(primary_intc->base + IVR);
> > if (hwirq != -1U)
> > - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
> > + irq = irq_find_mapping(primary_intc->root_domain,
> > hwirq);
> >
> > pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>
> I have the ugly feeling I'm reading the same code twice... Surely you can
> make these two functions common code.
[Mubin]: Upcoming patchset from Michal would be removing xintc_get_irq, instead of that "handle_domain_irq()" would be used independently. I hope that is fine.
> >
> > @@ -118,15 +131,18 @@ unsigned int xintc_get_irq(void)
> >
> > static int xintc_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hw) {
> > - if (xintc_irqc->intr_mask & (1 << hw)) {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + struct xintc_irq_chip *local_intc = d->host_data;
> > +
> > + if (local_intc->intr_mask & (1 << hw)) {
>
> BIT(hw)
[Mubin]: Fixed in v3
>
> > + irq_set_chip_and_handler_name(irq,
> > local_intc->intc_dev,
> > handle_edge_irq,
> > "edge");
> > irq_clear_status_flags(irq, IRQ_LEVEL);
> > } else {
> > - irq_set_chip_and_handler_name(irq, &intc_dev,
> > + irq_set_chip_and_handler_name(irq,
> > local_intc->intc_dev,
> > handle_level_irq,
> > "level");
> > irq_set_status_flags(irq, IRQ_LEVEL);
> > }
> > + irq_set_chip_data(irq, local_intc);
> > return 0;
> > }
> >
> > @@ -138,11 +154,13 @@ static const struct irq_domain_ops
> > xintc_irq_domain_ops = { static void xil_intc_irq_handler(struct
> > irq_desc *desc) {
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > + struct xintc_irq_chip *local_intc =
> > + irq_data_get_irq_handler_data(&desc->irq_data);
> > u32 pending;
> >
> > chained_irq_enter(chip, desc);
> > do {
> > - pending = xintc_get_irq();
> > + pending = xintc_get_irq_local(local_intc);
> > if (pending == -1U)
> > break;
> > generic_handle_irq(pending); @@ -153,28 +171,20 @@
> > static void xil_intc_irq_handler(struct irq_desc
> > *desc)
> > static int __init xilinx_intc_of_init(struct device_node *intc,
> > struct device_node
> > *parent)
> > {
> > - u32 nr_irq;
> > int ret, irq;
> > struct xintc_irq_chip *irqc;
> > -
> > - if (xintc_irqc) {
> > - pr_err("irq-xilinx: Multiple instances aren't
> > supported\n");
> > - return -EINVAL;
> > - }
> > + struct irq_chip *intc_dev;
> >
> > irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
> > if (!irqc)
> > return -ENOMEM;
> > -
> > - xintc_irqc = irqc;
> > -
> > irqc->base = of_iomap(intc, 0);
> > BUG_ON(!irqc->base);
> >
> > - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > &nr_irq);
> > + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs",
> > &irqc->nr_irq);
> > if (ret < 0) {
> > pr_err("irq-xilinx: unable to read
> > xlnx,num-intr-inputs\n");
> > - goto err_alloc;
> > + goto error;
> > }
> >
> > ret = of_property_read_u32(intc, "xlnx,kind-of-intr",
> > &irqc->intr_mask); @@ -183,30 +193,42 @@ static int __init
> > xilinx_intc_of_init(struct device_node *intc,
> > irqc->intr_mask = 0;
> > }
> >
> > - if (irqc->intr_mask >> nr_irq)
> > + if (irqc->intr_mask >> irqc->nr_irq)
> > pr_warn("irq-xilinx: mismatch in kind-of-intr
> > param\n");
> >
> > pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n",
> > - intc, nr_irq, irqc->intr_mask);
> > + intc, irqc->nr_irq, irqc->intr_mask);
> > +
> > + intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
> > + if (!intc_dev) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> >
> > + intc_dev->name = intc->full_name;
>
> No. The world doesn't need to see the OF path of your interrupt controller in
> /proc/cpuinfo.
> The name that was there before was perfectly descriptive, please stick to it.
[Mubin]: Reverted in v3
>
> > + intc_dev->irq_unmask = intc_enable_or_unmask,
> > + intc_dev->irq_mask = intc_disable_or_mask,
> > + intc_dev->irq_ack = intc_ack,
> > + intc_dev->irq_mask_ack = intc_mask_ack,
>
> And this structure should stay static, as it was before.
[Mubin]: Reverted in v3
>
> > + irqc->intc_dev = intc_dev;
> >
> > /*
> > * Disable all external interrupts until they are
> > * explicity requested.
> > */
> > - xintc_write(IER, 0);
> > + xintc_write(irqc->base + IER, 0);
> >
> > /* Acknowledge any pending interrupts just in case. */
> > - xintc_write(IAR, 0xffffffff);
> > + xintc_write(irqc->base + IAR, 0xffffffff);
> >
> > /* Turn on the Master Enable. */
> > - xintc_write(MER, MER_HIE | MER_ME);
> > - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) {
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > + if (!(xintc_read(irqc->base + MER) & (MER_HIE | MER_ME))) {
> > static_branch_enable(&xintc_is_be);
> > - xintc_write(MER, MER_HIE | MER_ME);
> > + xintc_write(irqc->base + MER, MER_HIE | MER_ME);
> > }
> >
> > - irqc->root_domain = irq_domain_add_linear(intc, nr_irq,
> > + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq,
> >
> > &xintc_irq_domain_ops, irqc);
> > if (!irqc->root_domain) {
> > pr_err("irq-xilinx: Unable to create IRQ domain\n");
> > @@ -225,13 +247,16 @@ static int __init xilinx_intc_of_init(struct
> > device_node *intc,
> > goto err_alloc;
> > }
> > } else {
> > - irq_set_default_host(irqc->root_domain);
> > + primary_intc = irqc;
> > + irq_set_default_host(primary_intc->root_domain);
> > }
> >
> > return 0;
> >
> > err_alloc:
> > - xintc_irqc = NULL;
> > + kfree(intc_dev);
> > +error:
> > + iounmap(irqc->base);
> > kfree(irqc);
> > return ret;
> >
> > --
> > 2.7.4
> >
> > This email and any attachments are intended for the sole use of the
> > named recipient(s) and contain(s) confidential information that may be
> > proprietary, privileged or copyrighted under applicable law. If you
> > are not the intended recipient, do not read, copy, or forward this
> > email message or any attachments. Delete this email message and any
> > attachments immediately.
>
> Please tell your employer to fix their email server.
[Mubin]: Fixed in v3
Thanks,
Mubin
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-02-11 19:38 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 14:05 [PATCH v2] irqchip: xilinx: Add support for multiple instances Mubin Usman Sayyed
2020-02-05 14:05 ` Mubin Usman Sayyed
2020-02-05 14:15 ` David Laight
2020-02-05 14:15 ` David Laight
2020-02-05 14:54 ` Michal Simek
2020-02-05 14:54 ` Michal Simek
2020-02-06 8:07 ` Mubin Usman Sayyed
2020-02-06 8:07 ` Mubin Usman Sayyed
2020-02-05 14:53 ` Michal Simek
2020-02-05 14:53 ` Michal Simek
2020-02-06 8:02 ` Mubin Usman Sayyed
2020-02-06 8:02 ` Mubin Usman Sayyed
2020-02-05 16:53 ` Marc Zyngier
2020-02-05 16:53 ` Marc Zyngier
2020-02-06 7:06 ` Michal Simek
2020-02-06 7:06 ` Michal Simek
2020-02-06 9:09 ` Marc Zyngier
2020-02-06 9:09 ` Marc Zyngier
2020-02-06 9:11 ` Michal Simek
2020-02-06 9:11 ` Michal Simek
2020-02-06 9:15 ` Marc Zyngier
2020-02-06 9:15 ` Marc Zyngier
2020-02-06 9:17 ` Michal Simek
2020-02-06 9:17 ` Michal Simek
2020-02-11 19:38 ` Mubin Usman Sayyed
2020-02-11 19:38 ` Mubin Usman Sayyed
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.