The PCI_INTA on Lantiq SoCs is a chained interrupt:
EBU configures the interrupt type, has a registers to enable/disable
and ACK the interrupt. This is chained with the ICU interrupt where the
parent interrupt of the EBU IRQ has to be ACK'ed.
Move all EBU interrupt logic into ebu.c and expose it using an
irq_domain and irq_chip.
Drop the hardcoded EBU interrupt configuration from pci-lantiq.c as this
can now be expressed in device tree by passing the EBU interrupt line to
PCI_INTA (using for example "... &ebu0 0 IRQ_TYPE_LEVEL_LOW").
Also drop the EBU interrupt masking from irq.c because that's now
managed by EBU's own irq_ack callback.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
.../include/asm/mach-lantiq/xway/lantiq_soc.h | 3 -
arch/mips/lantiq/ebu.c | 149 ++++++++++++++++++
arch/mips/lantiq/irq.c | 11 --
arch/mips/pci/pci-lantiq.c | 4 -
4 files changed, 149 insertions(+), 18 deletions(-)
diff --git a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
index 02a64ad6c0cc..5555deb02397 100644
--- a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
+++ b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
@@ -79,9 +79,6 @@ extern __iomem void *ltq_cgu_membase;
#define LTQ_EARLY_ASC KSEG1ADDR(LTQ_ASC1_BASE_ADDR)
/* EBU - external bus unit */
-#define LTQ_EBU_PCC_CON 0x0090
-#define LTQ_EBU_PCC_IEN 0x00A4
-#define LTQ_EBU_PCC_ISTAT 0x00A0
#define LTQ_EBU_BUSCON1 0x0064
#define LTQ_EBU_ADDRSEL1 0x0024
diff --git a/arch/mips/lantiq/ebu.c b/arch/mips/lantiq/ebu.c
index b2e84cf83f91..12951eb3c88f 100644
--- a/arch/mips/lantiq/ebu.c
+++ b/arch/mips/lantiq/ebu.c
@@ -7,7 +7,11 @@
#include <linux/bits.h>
#include <linux/export.h>
#include <linux/ioport.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
#include <linux/of.h>
+#include <linux/of_irq.h>
#include <linux/of_platform.h>
#include <linux/of_address.h>
@@ -15,6 +19,19 @@
#define LTQ_EBU_BUSCON0 0x0060
#define LTQ_EBU_BUSCON_WRDIS BIT(31)
+#define LTQ_EBU_PCC_CON 0x0090
+#define LTQ_EBU_PCC_CON_PCCARD_ON BIT(0)
+#define LTQ_EBU_PCC_CON_IREQ_RISING_EDGE 0x2
+#define LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE 0x4
+#define LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE 0x6
+#define LTQ_EBU_PCC_CON_IREQ_DIS 0x8
+#define LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT 0xa
+#define LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT 0xc
+#define LTQ_EBU_PCC_CON_IREQ_MASK 0xe
+#define LTQ_EBU_PCC_ISTAT 0x00a0
+#define LTQ_EBU_PCC_ISTAT_PCI BIT(4)
+#define LTQ_EBU_PCC_IEN 0x00a4
+#define LTQ_EBU_PCC_IEN_PCI_EN BIT(4)
void __iomem *ltq_ebu_membase;
@@ -52,6 +69,131 @@ static const struct of_device_id of_ebu_ids[] __initconst = {
{ /* sentinel */ },
};
+static void ltq_ebu_ack_irq(struct irq_data *d)
+{
+ ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | LTQ_EBU_PCC_ISTAT_PCI,
+ LTQ_EBU_PCC_ISTAT);
+}
+
+static void ltq_ebu_mask_irq(struct irq_data *d)
+{
+ ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) & ~LTQ_EBU_PCC_IEN_PCI_EN,
+ LTQ_EBU_PCC_IEN);
+}
+
+static void ltq_ebu_unmask_irq(struct irq_data *d)
+{
+ ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | LTQ_EBU_PCC_IEN_PCI_EN,
+ LTQ_EBU_PCC_IEN);
+}
+
+static int ltq_ebu_set_irq_type(struct irq_data *d, unsigned int flow_type)
+{
+ u32 val = ltq_ebu_r32(LTQ_EBU_PCC_CON);
+
+ val &= ~LTQ_EBU_PCC_CON_IREQ_MASK;
+
+ switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_NONE:
+ val |= LTQ_EBU_PCC_CON_IREQ_DIS;
+ break;
+
+ case IRQ_TYPE_EDGE_RISING:
+ val |= LTQ_EBU_PCC_CON_IREQ_RISING_EDGE;
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ val |= LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE;
+ break;
+
+ case IRQ_TYPE_EDGE_BOTH:
+ val |= LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE;
+ break;
+
+ case IRQ_TYPE_LEVEL_HIGH:
+ val |= LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT;
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ val |= LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT;
+ break;
+
+ default:
+ pr_err("Invalid trigger mode %x for IRQ %d\n", flow_type,
+ d->irq);
+ return -EINVAL;
+ }
+
+ ltq_ebu_w32(val, LTQ_EBU_PCC_CON);
+
+ return 0;
+}
+
+static void ltq_ebu_irq_handler(struct irq_desc *desc)
+{
+ struct irq_domain *domain = irq_desc_get_handler_data(desc);
+ struct irq_chip *irqchip = irq_desc_get_chip(desc);
+
+ chained_irq_enter(irqchip, desc);
+
+ generic_handle_irq(irq_find_mapping(domain, 0));
+
+ chained_irq_exit(irqchip, desc);
+}
+
+static struct irq_chip ltq_ebu_irq_chip = {
+ .name = "EBU",
+ .irq_ack = ltq_ebu_ack_irq,
+ .irq_mask = ltq_ebu_mask_irq,
+ .irq_unmask = ltq_ebu_unmask_irq,
+ .irq_set_type = ltq_ebu_set_irq_type,
+};
+
+static int ltq_ebu_irq_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_and_handler(irq, <q_ebu_irq_chip, handle_edge_irq);
+ irq_set_chip_data(irq, domain->host_data);
+
+ return 0;
+}
+
+static const struct irq_domain_ops ltq_ebu_irqdomain_ops = {
+ .map = ltq_ebu_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+static int ltq_ebu_add_irqchip(struct device_node *np)
+{
+ struct irq_domain *parent_domain, *domain;
+ int irq;
+
+ parent_domain = irq_find_host(of_irq_find_parent(np));
+ if (!parent_domain) {
+ pr_err("%pOF: No interrupt-parent found\n", np);
+ return -EINVAL;
+ }
+
+ domain = irq_domain_add_linear(np, 1, <q_ebu_irqdomain_ops, NULL);
+ if (!domain) {
+ pr_err("%pOF: Could not register EBU IRQ domain\n", np);
+ return -ENOMEM;
+ }
+
+ irq = irq_of_parse_and_map(np, 0);
+ if (!irq) {
+ pr_err("%pOF: Failed to map interrupt\n", np);
+ irq_domain_remove(domain);
+ return -EINVAL;
+ }
+
+ irq_create_mapping(domain, 0);
+
+ irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
+
+ return 0;
+}
+
static int __init ltq_ebu_setup(void)
{
const struct ltq_ebu_data *ebu_data;
@@ -59,6 +201,7 @@ static int __init ltq_ebu_setup(void)
struct resource res_ebu;
struct device_node *np;
u32 val;
+ int ret;
np = of_find_matching_node_and_match(NULL, of_ebu_ids, &match);
if (!np)
@@ -83,6 +226,12 @@ static int __init ltq_ebu_setup(void)
ltq_ebu_w32(val, LTQ_EBU_BUSCON0);
}
+ if (of_property_read_bool(np, "interrupt-controller")) {
+ ret = ltq_ebu_add_irqchip(np);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
diff --git a/arch/mips/lantiq/irq.c b/arch/mips/lantiq/irq.c
index 115b417dfb8e..cb9ab51fa748 100644
--- a/arch/mips/lantiq/irq.c
+++ b/arch/mips/lantiq/irq.c
@@ -40,12 +40,6 @@
/* the performance counter */
#define LTQ_PERF_IRQ (INT_NUM_IM4_IRL0 + 31)
-/*
- * irqs generated by devices attached to the EBU need to be acked in
- * a special manner
- */
-#define LTQ_ICU_EBU_IRQ 22
-
#define ltq_icu_w32(vpe, m, x, y) \
ltq_w32((x), ltq_icu_membase[vpe] + m*LTQ_ICU_IM_SIZE + (y))
@@ -300,11 +294,6 @@ static void ltq_hw_irq_handler(struct irq_desc *desc)
irq = __fls(irq);
hwirq = irq + MIPS_CPU_IRQ_CASCADE + (INT_NUM_IM_OFFSET * module);
generic_handle_irq(irq_linear_revmap(ltq_domain, hwirq));
-
- /* if this is a EBU irq, we need to ack it or get a deadlock */
- if ((irq == LTQ_ICU_EBU_IRQ) && (module == 0) && LTQ_EBU_PCC_ISTAT)
- ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | 0x10,
- LTQ_EBU_PCC_ISTAT);
}
static int icu_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c
index 1ca42f482130..a3f6ab94ee2c 100644
--- a/arch/mips/pci/pci-lantiq.c
+++ b/arch/mips/pci/pci-lantiq.c
@@ -190,10 +190,6 @@ static int ltq_pci_startup(struct platform_device *pdev)
ltq_pci_w32(ltq_pci_r32(PCI_CR_PCI_MOD) | (1 << 24), PCI_CR_PCI_MOD);
wmb();
- /* setup irq line */
- ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_CON) | 0xc, LTQ_EBU_PCC_CON);
- ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | 0x10, LTQ_EBU_PCC_IEN);
-
/* toggle reset pin */
if (gpio_is_valid(reset_gpio)) {
__gpio_set_value(reset_gpio, 0);
--
2.22.0
Hi Martin,
On Sat, 27 Jul 2019 18:53:13 +0100,
Martin Blumenstingl <[email protected]> wrote:
>
> The PCI_INTA on Lantiq SoCs is a chained interrupt:
> EBU configures the interrupt type, has a registers to enable/disable
> and ACK the interrupt. This is chained with the ICU interrupt where the
> parent interrupt of the EBU IRQ has to be ACK'ed.
>
> Move all EBU interrupt logic into ebu.c and expose it using an
> irq_domain and irq_chip.
> Drop the hardcoded EBU interrupt configuration from pci-lantiq.c as this
> can now be expressed in device tree by passing the EBU interrupt line to
> PCI_INTA (using for example "... &ebu0 0 IRQ_TYPE_LEVEL_LOW").
> Also drop the EBU interrupt masking from irq.c because that's now
> managed by EBU's own irq_ack callback.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> .../include/asm/mach-lantiq/xway/lantiq_soc.h | 3 -
> arch/mips/lantiq/ebu.c | 149 ++++++++++++++++++
> arch/mips/lantiq/irq.c | 11 --
> arch/mips/pci/pci-lantiq.c | 4 -
> 4 files changed, 149 insertions(+), 18 deletions(-)
>
> diff --git a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> index 02a64ad6c0cc..5555deb02397 100644
> --- a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> +++ b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> @@ -79,9 +79,6 @@ extern __iomem void *ltq_cgu_membase;
> #define LTQ_EARLY_ASC KSEG1ADDR(LTQ_ASC1_BASE_ADDR)
>
> /* EBU - external bus unit */
> -#define LTQ_EBU_PCC_CON 0x0090
> -#define LTQ_EBU_PCC_IEN 0x00A4
> -#define LTQ_EBU_PCC_ISTAT 0x00A0
> #define LTQ_EBU_BUSCON1 0x0064
> #define LTQ_EBU_ADDRSEL1 0x0024
>
> diff --git a/arch/mips/lantiq/ebu.c b/arch/mips/lantiq/ebu.c
> index b2e84cf83f91..12951eb3c88f 100644
> --- a/arch/mips/lantiq/ebu.c
> +++ b/arch/mips/lantiq/ebu.c
> @@ -7,7 +7,11 @@
> #include <linux/bits.h>
> #include <linux/export.h>
> #include <linux/ioport.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/of_address.h>
>
> @@ -15,6 +19,19 @@
>
> #define LTQ_EBU_BUSCON0 0x0060
> #define LTQ_EBU_BUSCON_WRDIS BIT(31)
> +#define LTQ_EBU_PCC_CON 0x0090
> +#define LTQ_EBU_PCC_CON_PCCARD_ON BIT(0)
> +#define LTQ_EBU_PCC_CON_IREQ_RISING_EDGE 0x2
> +#define LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE 0x4
> +#define LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE 0x6
So BOTH_EDGE is actually (RISING_EDGE | FALLING_EDGE). It'd be nice to
express it this way.
> +#define LTQ_EBU_PCC_CON_IREQ_DIS 0x8
What does "DIS" mean?
> +#define LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT 0xa
> +#define LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT 0xc
Again, these two are (DIS | RISING) and (DIS | FALLING).
> +#define LTQ_EBU_PCC_CON_IREQ_MASK 0xe
> +#define LTQ_EBU_PCC_ISTAT 0x00a0
> +#define LTQ_EBU_PCC_ISTAT_PCI BIT(4)
> +#define LTQ_EBU_PCC_IEN 0x00a4
> +#define LTQ_EBU_PCC_IEN_PCI_EN BIT(4)
>
> void __iomem *ltq_ebu_membase;
>
> @@ -52,6 +69,131 @@ static const struct of_device_id of_ebu_ids[] __initconst = {
> { /* sentinel */ },
> };
>
> +static void ltq_ebu_ack_irq(struct irq_data *d)
> +{
> + ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | LTQ_EBU_PCC_ISTAT_PCI,
> + LTQ_EBU_PCC_ISTAT);
> +}
> +
> +static void ltq_ebu_mask_irq(struct irq_data *d)
> +{
> + ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) & ~LTQ_EBU_PCC_IEN_PCI_EN,
> + LTQ_EBU_PCC_IEN);
> +}
> +
> +static void ltq_ebu_unmask_irq(struct irq_data *d)
> +{
> + ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | LTQ_EBU_PCC_IEN_PCI_EN,
> + LTQ_EBU_PCC_IEN);
> +}
> +
> +static int ltq_ebu_set_irq_type(struct irq_data *d, unsigned int flow_type)
> +{
> + u32 val = ltq_ebu_r32(LTQ_EBU_PCC_CON);
> +
> + val &= ~LTQ_EBU_PCC_CON_IREQ_MASK;
> +
> + switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_NONE:
> + val |= LTQ_EBU_PCC_CON_IREQ_DIS;
> + break;
I'm not sure IRQ_TYPE_NONE makes much sense here. What's the expected
semantic?
> +
> + case IRQ_TYPE_EDGE_RISING:
> + val |= LTQ_EBU_PCC_CON_IREQ_RISING_EDGE;
> + break;
> +
> + case IRQ_TYPE_EDGE_FALLING:
> + val |= LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE;
> + break;
> +
> + case IRQ_TYPE_EDGE_BOTH:
> + val |= LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE;
> + break;
> +
> + case IRQ_TYPE_LEVEL_HIGH:
> + val |= LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT;
> + break;
> +
> + case IRQ_TYPE_LEVEL_LOW:
> + val |= LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT;
> + break;
> +
> + default:
> + pr_err("Invalid trigger mode %x for IRQ %d\n", flow_type,
> + d->irq);
irq_set_type will already complain in the kernel log, no need to add
an extra message.
> + return -EINVAL;
> + }
> +
> + ltq_ebu_w32(val, LTQ_EBU_PCC_CON);
> +
> + return 0;
> +}
> +
> +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> +{
> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> +
> + chained_irq_enter(irqchip, desc);
> +
> + generic_handle_irq(irq_find_mapping(domain, 0));
Having an irqdomain for a single interrupt is a bit over the top... Is
that for the convenience of the DT infrastructure?
> +
> + chained_irq_exit(irqchip, desc);
> +}
> +
> +static struct irq_chip ltq_ebu_irq_chip = {
> + .name = "EBU",
> + .irq_ack = ltq_ebu_ack_irq,
> + .irq_mask = ltq_ebu_mask_irq,
> + .irq_unmask = ltq_ebu_unmask_irq,
> + .irq_set_type = ltq_ebu_set_irq_type,
> +};
> +
> +static int ltq_ebu_irq_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_and_handler(irq, <q_ebu_irq_chip, handle_edge_irq);
> + irq_set_chip_data(irq, domain->host_data);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops ltq_ebu_irqdomain_ops = {
> + .map = ltq_ebu_irq_map,
> + .xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int ltq_ebu_add_irqchip(struct device_node *np)
> +{
> + struct irq_domain *parent_domain, *domain;
> + int irq;
> +
> + parent_domain = irq_find_host(of_irq_find_parent(np));
> + if (!parent_domain) {
> + pr_err("%pOF: No interrupt-parent found\n", np);
> + return -EINVAL;
> + }
> +
> + domain = irq_domain_add_linear(np, 1, <q_ebu_irqdomain_ops, NULL);
> + if (!domain) {
> + pr_err("%pOF: Could not register EBU IRQ domain\n", np);
> + return -ENOMEM;
> + }
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (!irq) {
> + pr_err("%pOF: Failed to map interrupt\n", np);
> + irq_domain_remove(domain);
> + return -EINVAL;
> + }
> +
> + irq_create_mapping(domain, 0);
Why do you need to perform this eagerly? I'd expect this interrupt to
be mapped when it is actually claimed by a driver.
> +
> + irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
And there is no HW initialisation whatsoever? I'd expect, at the very
least, the sole interrupt to be configured as disabled/masked.
> +
> + return 0;
> +}
> +
> static int __init ltq_ebu_setup(void)
> {
> const struct ltq_ebu_data *ebu_data;
> @@ -59,6 +201,7 @@ static int __init ltq_ebu_setup(void)
> struct resource res_ebu;
> struct device_node *np;
> u32 val;
> + int ret;
>
> np = of_find_matching_node_and_match(NULL, of_ebu_ids, &match);
> if (!np)
> @@ -83,6 +226,12 @@ static int __init ltq_ebu_setup(void)
> ltq_ebu_w32(val, LTQ_EBU_BUSCON0);
> }
>
> + if (of_property_read_bool(np, "interrupt-controller")) {
> + ret = ltq_ebu_add_irqchip(np);
> + if (ret)
> + return ret;
> + }
> +
> return 0;
> }
>
> diff --git a/arch/mips/lantiq/irq.c b/arch/mips/lantiq/irq.c
> index 115b417dfb8e..cb9ab51fa748 100644
> --- a/arch/mips/lantiq/irq.c
> +++ b/arch/mips/lantiq/irq.c
> @@ -40,12 +40,6 @@
> /* the performance counter */
> #define LTQ_PERF_IRQ (INT_NUM_IM4_IRL0 + 31)
>
> -/*
> - * irqs generated by devices attached to the EBU need to be acked in
> - * a special manner
> - */
> -#define LTQ_ICU_EBU_IRQ 22
> -
> #define ltq_icu_w32(vpe, m, x, y) \
> ltq_w32((x), ltq_icu_membase[vpe] + m*LTQ_ICU_IM_SIZE + (y))
>
> @@ -300,11 +294,6 @@ static void ltq_hw_irq_handler(struct irq_desc *desc)
> irq = __fls(irq);
> hwirq = irq + MIPS_CPU_IRQ_CASCADE + (INT_NUM_IM_OFFSET * module);
> generic_handle_irq(irq_linear_revmap(ltq_domain, hwirq));
> -
> - /* if this is a EBU irq, we need to ack it or get a deadlock */
> - if ((irq == LTQ_ICU_EBU_IRQ) && (module == 0) && LTQ_EBU_PCC_ISTAT)
> - ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | 0x10,
> - LTQ_EBU_PCC_ISTAT);
> }
>
> static int icu_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c
> index 1ca42f482130..a3f6ab94ee2c 100644
> --- a/arch/mips/pci/pci-lantiq.c
> +++ b/arch/mips/pci/pci-lantiq.c
> @@ -190,10 +190,6 @@ static int ltq_pci_startup(struct platform_device *pdev)
> ltq_pci_w32(ltq_pci_r32(PCI_CR_PCI_MOD) | (1 << 24), PCI_CR_PCI_MOD);
> wmb();
>
> - /* setup irq line */
> - ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_CON) | 0xc, LTQ_EBU_PCC_CON);
> - ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | 0x10, LTQ_EBU_PCC_IEN);
> -
> /* toggle reset pin */
> if (gpio_is_valid(reset_gpio)) {
> __gpio_set_value(reset_gpio, 0);
> --
> 2.22.0
>
Thanks,
M.
--
Jazz is not dead, it just smells funny.
Hi Marc,
thank you for taking time to review this patch!
On Sun, Jul 28, 2019 at 12:01 PM Marc Zyngier <[email protected]> wrote:
[...]
> > @@ -15,6 +19,19 @@
> >
> > #define LTQ_EBU_BUSCON0 0x0060
> > #define LTQ_EBU_BUSCON_WRDIS BIT(31)
> > +#define LTQ_EBU_PCC_CON 0x0090
> > +#define LTQ_EBU_PCC_CON_PCCARD_ON BIT(0)
> > +#define LTQ_EBU_PCC_CON_IREQ_RISING_EDGE 0x2
> > +#define LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE 0x4
> > +#define LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE 0x6
>
> So BOTH_EDGE is actually (RISING_EDGE | FALLING_EDGE). It'd be nice to
> express it this way.
I only notice this now - thank you for the hint
v2 will have this cleaned up
> > +#define LTQ_EBU_PCC_CON_IREQ_DIS 0x8
>
> What does "DIS" mean?
after reading all of your comments it may be "disable edge detection"
I don't have access to the datasheet but I'll ask someone at Intel (Lantiq)
> > +#define LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT 0xa
> > +#define LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT 0xc
>
> Again, these two are (DIS | RISING) and (DIS | FALLING).
understood, v2 will use a better name for DIS (assuming there's a
better name) and I'll convert the macros based on your suggestion
[...]
> > + switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_NONE:
> > + val |= LTQ_EBU_PCC_CON_IREQ_DIS;
> > + break;
>
> I'm not sure IRQ_TYPE_NONE makes much sense here. What's the expected
> semantic?
if it's "disable edge detection" then this "case" will be removed
[...]
> > + default:
> > + pr_err("Invalid trigger mode %x for IRQ %d\n", flow_type,
> > + d->irq);
>
> irq_set_type will already complain in the kernel log, no need to add
> an extra message.
I'll drop this in v2, thank you for pointing this out
[...]
> > +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> > +{
> > + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> > + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > +
> > + chained_irq_enter(irqchip, desc);
> > +
> > + generic_handle_irq(irq_find_mapping(domain, 0));
>
> Having an irqdomain for a single interrupt is a bit over the top... Is
> that for the convenience of the DT infrastructure?
yes, I did it to get DT support
please let me know if there's a "better" way (preferably with another
driver as example)
[...]
> > + irq_create_mapping(domain, 0);
>
> Why do you need to perform this eagerly? I'd expect this interrupt to
> be mapped when it is actually claimed by a driver.
I don't remember why I added it, it may be left-over from copying from
another driver
in v2 I'll try to drop it
> > +
> > + irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
>
> And there is no HW initialisation whatsoever? I'd expect, at the very
> least, the sole interrupt to be configured as disabled/masked.
I can add that. is there any "best practice" on what I should
initialize (just disable it or also set a "default" mode like
LEVEL_LOW)?
Martin
Hi Martin,
On Thu, 01 Aug 2019 18:42:42 +0100,
Martin Blumenstingl <[email protected]> wrote:
[...]
> > > +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> > > +{
> > > + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> > > + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > > +
> > > + chained_irq_enter(irqchip, desc);
> > > +
> > > + generic_handle_irq(irq_find_mapping(domain, 0));
> >
> > Having an irqdomain for a single interrupt is a bit over the top... Is
> > that for the convenience of the DT infrastructure?
> yes, I did it to get DT support
> please let me know if there's a "better" way (preferably with another
> driver as example)
To be honest, the chained handler is what troubles me the most. You
normally would use such a construct if you had a multiplexer. In your
case, you have a 1:1 relationship between input and output. It is just
that this irqchip allows the trigger to be adapted, which normally
calls for a hierarchical implementation.
In your case, with only a single interrupt, it doesn't matter much
though.
>
> [...]
> > > + irq_create_mapping(domain, 0);
> >
> > Why do you need to perform this eagerly? I'd expect this interrupt to
> > be mapped when it is actually claimed by a driver.
> I don't remember why I added it, it may be left-over from copying from
> another driver
> in v2 I'll try to drop it
>
> > > +
> > > + irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
> >
> > And there is no HW initialisation whatsoever? I'd expect, at the very
> > least, the sole interrupt to be configured as disabled/masked.
> I can add that. is there any "best practice" on what I should
> initialize (just disable it or also set a "default" mode like
> LEVEL_LOW)?
Whichever default state makes sense. What you want to avoid is to boot
the kernel with a screaming interrupt because some firmware has left
it enabled.
Thanks,
M.
--
Jazz is not dead, it just smells funny.
Hi Marc,
On Sat, Aug 3, 2019 at 11:12 AM Marc Zyngier <[email protected]> wrote:
>
> Hi Martin,
>
> On Thu, 01 Aug 2019 18:42:42 +0100,
> Martin Blumenstingl <[email protected]> wrote:
>
> [...]
>
> > > > +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> > > > +{
> > > > + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> > > > + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > > > +
> > > > + chained_irq_enter(irqchip, desc);
> > > > +
> > > > + generic_handle_irq(irq_find_mapping(domain, 0));
> > >
> > > Having an irqdomain for a single interrupt is a bit over the top... Is
> > > that for the convenience of the DT infrastructure?
> > yes, I did it to get DT support
> > please let me know if there's a "better" way (preferably with another
> > driver as example)
>
> To be honest, the chained handler is what troubles me the most. You
> normally would use such a construct if you had a multiplexer. In your
> case, you have a 1:1 relationship between input and output. It is just
> that this irqchip allows the trigger to be adapted, which normally
> calls for a hierarchical implementation.
>
> In your case, with only a single interrupt, it doesn't matter much
> though.
I see, thank you for the explanation
can you name a driver for a hierarchical irqchip driver that you
consider "clean" which I could use as reference?
I am considering to still convert it to a hierarchical irqchip driver
to keep it consistent with at least two more upcoming Lantiq irqchip
drivers (which both seem to be similar use-cases as this one, they
just provide more than one interrupt):
- there's a PCI legacy interrupt controller in the PCIe controller's
app registers. it takes 4 parent interrupts and provides
PCI_INT{A,B,C,D}. the interrupts need to be enabled and ACK'ed in the
PCIe app registers as well as in the parent interrupt controller
- the EIU (External Interrupt Unit) in my own words is the GPIO
interrupt controller. it takes up to 6 parent interrupts and provides
one interrupt for each "EXIN GPIO". setting the IRQ type and ACK need
to happen through the EIU registers as well as the parent interrupt
controller
my initial thought is that it's best to follow one programming model
(which based on your suggestion would be a hierarchical irqchip) for
all three IRQ controllers
> >
> > [...]
> > > > + irq_create_mapping(domain, 0);
> > >
> > > Why do you need to perform this eagerly? I'd expect this interrupt to
> > > be mapped when it is actually claimed by a driver.
> > I don't remember why I added it, it may be left-over from copying from
> > another driver
> > in v2 I'll try to drop it
> >
> > > > +
> > > > + irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
> > >
> > > And there is no HW initialisation whatsoever? I'd expect, at the very
> > > least, the sole interrupt to be configured as disabled/masked.
> > I can add that. is there any "best practice" on what I should
> > initialize (just disable it or also set a "default" mode like
> > LEVEL_LOW)?
>
> Whichever default state makes sense. What you want to avoid is to boot
> the kernel with a screaming interrupt because some firmware has left
> it enabled.
noted, thank you
Martin
On 03/08/2019 18:33, Martin Blumenstingl wrote:
> Hi Marc,
>
> On Sat, Aug 3, 2019 at 11:12 AM Marc Zyngier <[email protected]> wrote:
>>
>> Hi Martin,
>>
>> On Thu, 01 Aug 2019 18:42:42 +0100,
>> Martin Blumenstingl <[email protected]> wrote:
>>
>> [...]
>>
>>>>> +static void ltq_ebu_irq_handler(struct irq_desc *desc)
>>>>> +{
>>>>> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
>>>>> + struct irq_chip *irqchip = irq_desc_get_chip(desc);
>>>>> +
>>>>> + chained_irq_enter(irqchip, desc);
>>>>> +
>>>>> + generic_handle_irq(irq_find_mapping(domain, 0));
>>>>
>>>> Having an irqdomain for a single interrupt is a bit over the top... Is
>>>> that for the convenience of the DT infrastructure?
>>> yes, I did it to get DT support
>>> please let me know if there's a "better" way (preferably with another
>>> driver as example)
>>
>> To be honest, the chained handler is what troubles me the most. You
>> normally would use such a construct if you had a multiplexer. In your
>> case, you have a 1:1 relationship between input and output. It is just
>> that this irqchip allows the trigger to be adapted, which normally
>> calls for a hierarchical implementation.
>>
>> In your case, with only a single interrupt, it doesn't matter much
>> though.
> I see, thank you for the explanation
>
> can you name a driver for a hierarchical irqchip driver that you
> consider "clean" which I could use as reference?
Finding a "clean" driver is a challenge, as the world of IRQ controllers
is where both HW and SW engineers (me included) love to "innovate" ;-).
I'd recommend you have a look at drivers/irqchip/irq-mtk-cirq.c, which
is almost as simple as it gets.
Thanks,
M.
--
Jazz is not dead, it just smells funny...