* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-08 14:33 ` [RFC] irqchip: add support for LS1021A external interrupt lines Rasmus Villemoes
@ 2017-12-08 15:11 ` Alexander Stein
2017-12-08 16:09 ` Marc Zyngier
2017-12-08 16:02 ` Marc Zyngier
` (2 subsequent siblings)
3 siblings, 1 reply; 39+ messages in thread
From: Alexander Stein @ 2017-12-08 15:11 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
Mark Rutland, linux-kernel, devicetree
Hi Rasmus,
thanks for your effort. unfortunatly I won't be able to test it currently :(
But some comments below.
On Friday, December 8, 2017, 3:33:00 PM CET Rasmus Villemoes wrote:
> The LS1021A allows inverting the polarity of six interrupt lines
> IRQ[0:5] via the scfg_intpcr register, effectively allowing
> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
> check the type, set the relevant bit in INTPCR accordingly, and fixup
> the type argument before calling the GIC's irq_set_type.
>
> In fact, the power-on-reset value of the INTPCR register is so that all
> six lines have their polarity inverted. Hence any hardware connected to
> those lines is unusable without this: If the line is indeed active low,
> the generic GIC code will reject an irq spec with IRQ_TYPE_LEVEL_LOW,
> while if the line is active high, we must obviously disable the polarity
> inversion before unmasking the interrupt.
>
> I suspect other layerscape SOCs may have something similar, but I have
> neither hardware nor documentation.
>
> Since we only need to keep a single pointer in the chip_data (the syscon
> regmap), the code could be a little simpler by dropping the struct
> extirq_chip_data and just store the regmap directly - but I don't know
> if I do need to add a lock or something else to the chip_data, so for
> this RFC I've kept the struct.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> Marc, Alexander, thanks a lot for your hints. This is what I came up
> with, mostly just copy-pasted from the mtk-sysirq case. I've tested
> that it works as expected on my board.
>
> .../interrupt-controller/fsl,ls1021a-extirq.txt | 19 +++
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-ls1021a.c | 157 +++++++++++++++++++++
> 3 files changed, 177 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> create mode 100644 drivers/irqchip/irq-ls1021a.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> new file mode 100644
> index 000000000000..53b04b6e1a80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> @@ -0,0 +1,19 @@
> +* Freescale LS1021A external IRQs
> +
> +The LS1021A supports inverting the polarity of six external interrupt lines.
> +
> +Required properties:
> +- compatible: should be "fsl,ls1021a-extirq"
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
Do you really need 3 interrupt-cells here? As you've written below you don't
support PPI anyway the 1st flag might be dropped then. So support just 2 cells:
* IRQ number (IRQ0 - IRQ5)
* IRQ flags
> +- interrupt-parent: phandle of GIC.
> +- syscon: phandle of Supplemental Configuration Unit (scfg).
> +
> +Example:
> + extirq: interrupt-controller@15701ac {
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + syscon = <&scfg>;
> + };
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b842dfdc903f..d4576dce24b2 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
> obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> +obj-$(CONFIG_SOC_LS1021A) += irq-ls1021a.o
I guess this should be kept sorted alphabetically.
> diff --git a/drivers/irqchip/irq-ls1021a.c b/drivers/irqchip/irq-ls1021a.c
> new file mode 100644
> index 000000000000..2ec4fc023549
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls1021a.c
> @@ -0,0 +1,157 @@
> +#define pr_fmt(fmt) "irq-ls1021a: " fmt
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define INTPCR_REG 0x01ac
> +#define NIRQ 6
> +
> +struct extirq_chip_data {
> + struct regmap *syscon;
> +};
> +
> +static int
> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + irq_hw_number_t hwirq = data->hwirq;
> + struct extirq_chip_data *chip_data = data->chip_data;
> + u32 value, mask;
> + int ret;
> +
> + mask = 1U << (31 - hwirq);
Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
by the left most position in the register layout. This is just strange way
to express bit-endian access.
Anyway, please use BIT(x) instead.
> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> + if (type == IRQ_TYPE_LEVEL_LOW)
> + type = IRQ_TYPE_LEVEL_HIGH;
> + else
> + type = IRQ_TYPE_EDGE_RISING;
> + value = mask;
> + } else {
> + value = 0;
> + }
> +
> + /* Don't do the INTPCR_REG update if the parent irq_set_type will EINVAL. */
> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> + return -EINVAL;
I wonder if it is better to call data->parent_data->chip->irq_set_type(data, type)
here instead and call regmap if this suceeded.
> + /* regmap does internal locking, but do we need to provide our
> + * own across the parent irq_set_type call? */
> + regmap_update_bits(chip_data->syscon, INTPCR_REG, mask, value);
> +
> + data = data->parent_data;
> + ret = data->chip->irq_set_type(data, type);
> +
> + return ret;
> +}
> +
> +static struct irq_chip extirq_chip = {
> + .name = "LS1021A_EXTIRQ",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_type = ls1021a_extirq_set_type,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> +};
> +
> +static int
> +ls1021a_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> + unsigned long *hwirq, unsigned int *type)
> +{
> + if (!is_of_node(fwspec->fwnode))
> + return -EINVAL;
> +
> + if (fwspec->param_count != 3)
> + return -EINVAL;
> +
> + /* No PPI should point to this domain */
> + if (fwspec->param[0] != 0)
> + return -EINVAL;
> +
> + *hwirq = fwspec->param[1];
Is a check for the hwirq value required here? I'm not an expert on
irqchip API, so I just wonder.
> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> + return 0;
> +}
> +
> +static int
> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
^^^^^^
No need for static here.
> + int i;
> + irq_hw_number_t hwirq;
> + struct irq_fwspec *fwspec = arg;
> + struct irq_fwspec gic_fwspec;
> +
> + if (fwspec->param_count != 3)
> + return -EINVAL;
> +
> + if (fwspec->param[0])
> + return -EINVAL;
> +
> + hwirq = fwspec->param[1];
Is there any guarantee hwirq is in range 0-5?
> + for (i = 0; i < nr_irqs; i++)
> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> + &extirq_chip,
> + domain->host_data);
> +
> + gic_fwspec.fwnode = domain->parent->fwnode;
> + gic_fwspec.param_count = 3;
> + gic_fwspec.param[0] = 0;
As this param is fixed, you should be able to drop the 1st param in your
interrupt-cells.
> + gic_fwspec.param[1] = xlate[hwirq];
> + gic_fwspec.param[2] = fwspec->param[2];
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_fwspec);
> +}
> +
> +static const struct irq_domain_ops extirq_domain_ops = {
> + .translate = ls1021a_extirq_domain_translate,
> + .alloc = ls1021a_extirq_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> +};
> +
> +static int __init
> +ls1021a_extirq_of_init(struct device_node *node, struct device_node *parent)
> +{
> +
> + struct irq_domain *domain, *domain_parent;
> + struct extirq_chip_data *chip_data;
> + int ret;
> +
> + domain_parent = irq_find_host(parent);
> + if (!domain_parent) {
> + pr_err("interrupt-parent not found\n");
> + return -EINVAL;
> + }
Mh, does this mean if GIC has not been probed, this probe is not deferred?
Is there an API to check for that?
> + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
^^^^^^^
devm_kzalloc
> + if (!chip_data)
> + return -ENOMEM;
> +
> + chip_data->syscon = syscon_regmap_lookup_by_phandle(node, "syscon");
> + if (IS_ERR(chip_data->syscon)) {
> + ret = PTR_ERR(chip_data->syscon);
> + goto out_free_chip;
> + }
> +
> + domain = irq_domain_add_hierarchy(domain_parent, 0, NIRQ, node,
> + &extirq_domain_ops, chip_data);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out_free_chip;
> + }
> +
> + return 0;
> +
> +out_free_chip:
> + kfree(chip_data);
> + return ret;
Using devm_kzalloc this label can be skipped.
> +}
> +
> +IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls1021a_extirq_of_init);
Best regards,
Alexander
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-08 15:11 ` Alexander Stein
@ 2017-12-08 16:09 ` Marc Zyngier
2017-12-11 9:08 ` Rasmus Villemoes
0 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2017-12-08 16:09 UTC (permalink / raw)
To: Alexander Stein, Rasmus Villemoes
Cc: Thomas Gleixner, Jason Cooper, Rob Herring, Mark Rutland,
linux-kernel, devicetree
On 08/12/17 15:11, Alexander Stein wrote:
> Hi Rasmus,
>
> thanks for your effort. unfortunatly I won't be able to test it currently :(
> But some comments below.
>
> On Friday, December 8, 2017, 3:33:00 PM CET Rasmus Villemoes wrote:
>> The LS1021A allows inverting the polarity of six interrupt lines
>> IRQ[0:5] via the scfg_intpcr register, effectively allowing
>> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
>> check the type, set the relevant bit in INTPCR accordingly, and fixup
>> the type argument before calling the GIC's irq_set_type.
>>
>> In fact, the power-on-reset value of the INTPCR register is so that all
>> six lines have their polarity inverted. Hence any hardware connected to
>> those lines is unusable without this: If the line is indeed active low,
>> the generic GIC code will reject an irq spec with IRQ_TYPE_LEVEL_LOW,
>> while if the line is active high, we must obviously disable the polarity
>> inversion before unmasking the interrupt.
>>
>> I suspect other layerscape SOCs may have something similar, but I have
>> neither hardware nor documentation.
>>
>> Since we only need to keep a single pointer in the chip_data (the syscon
>> regmap), the code could be a little simpler by dropping the struct
>> extirq_chip_data and just store the regmap directly - but I don't know
>> if I do need to add a lock or something else to the chip_data, so for
>> this RFC I've kept the struct.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>> Marc, Alexander, thanks a lot for your hints. This is what I came up
>> with, mostly just copy-pasted from the mtk-sysirq case. I've tested
>> that it works as expected on my board.
>>
>> .../interrupt-controller/fsl,ls1021a-extirq.txt | 19 +++
>> drivers/irqchip/Makefile | 1 +
>> drivers/irqchip/irq-ls1021a.c | 157 +++++++++++++++++++++
>> 3 files changed, 177 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>> create mode 100644 drivers/irqchip/irq-ls1021a.c
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>> new file mode 100644
>> index 000000000000..53b04b6e1a80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>> @@ -0,0 +1,19 @@
>> +* Freescale LS1021A external IRQs
>> +
>> +The LS1021A supports inverting the polarity of six external interrupt lines.
>> +
>> +Required properties:
>> +- compatible: should be "fsl,ls1021a-extirq"
>> +- interrupt-controller: Identifies the node as an interrupt controller
>> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
>
> Do you really need 3 interrupt-cells here? As you've written below you don't
> support PPI anyway the 1st flag might be dropped then. So support just 2 cells:
> * IRQ number (IRQ0 - IRQ5)
> * IRQ flags
The convention for irqchip stacked on top of a GIC is to keep the
interrupt specifier the same. It makes the maintenance if the DT much
easier, and doesn't hurt at all.
>
>> +- interrupt-parent: phandle of GIC.
>> +- syscon: phandle of Supplemental Configuration Unit (scfg).
>> +
>> +Example:
>> + extirq: interrupt-controller@15701ac {
>> + compatible = "fsl,ls1021a-extirq";
>> + #interrupt-cells = <3>;
>> + interrupt-controller;
>> + interrupt-parent = <&gic>;
>> + syscon = <&scfg>;
>> + };
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b842dfdc903f..d4576dce24b2 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
>> obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
>> +obj-$(CONFIG_SOC_LS1021A) += irq-ls1021a.o
>
> I guess this should be kept sorted alphabetically.
There is no such requirement. But grouping it next to the other FSL
irqchip would make more sense.
>
>> diff --git a/drivers/irqchip/irq-ls1021a.c b/drivers/irqchip/irq-ls1021a.c
>> new file mode 100644
>> index 000000000000..2ec4fc023549
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-ls1021a.c
>> @@ -0,0 +1,157 @@
>> +#define pr_fmt(fmt) "irq-ls1021a: " fmt
>> +
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#define INTPCR_REG 0x01ac
>> +#define NIRQ 6
>> +
>> +struct extirq_chip_data {
>> + struct regmap *syscon;
>> +};
>> +
>> +static int
>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + irq_hw_number_t hwirq = data->hwirq;
>> + struct extirq_chip_data *chip_data = data->chip_data;
>> + u32 value, mask;
>> + int ret;
>> +
>> + mask = 1U << (31 - hwirq);
>
> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> by the left most position in the register layout. This is just strange way
> to express bit-endian access.
> Anyway, please use BIT(x) instead.
>
>> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
>> + if (type == IRQ_TYPE_LEVEL_LOW)
>> + type = IRQ_TYPE_LEVEL_HIGH;
>> + else
>> + type = IRQ_TYPE_EDGE_RISING;
>> + value = mask;
>> + } else {
>> + value = 0;
>> + }
>> +
>> + /* Don't do the INTPCR_REG update if the parent irq_set_type will EINVAL. */
>> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>> + return -EINVAL;
>
> I wonder if it is better to call data->parent_data->chip->irq_set_type(data, type)
> here instead and call regmap if this suceeded.
Not really. In both cases, you need to evaluate the failure (which is
not don here). So ordering doesn't matter. What actually matters is
error handling and atomicity (in this case, making sure that drivers
cannot observe an interrupt flood between the two reconfigurations).
>
>> + /* regmap does internal locking, but do we need to provide our
>> + * own across the parent irq_set_type call? */
>> + regmap_update_bits(chip_data->syscon, INTPCR_REG, mask, value);
>> +
>> + data = data->parent_data;
>> + ret = data->chip->irq_set_type(data, type);
>> +
>> + return ret;
>> +}
>> +
>> +static struct irq_chip extirq_chip = {
>> + .name = "LS1021A_EXTIRQ",
>> + .irq_mask = irq_chip_mask_parent,
>> + .irq_unmask = irq_chip_unmask_parent,
>> + .irq_eoi = irq_chip_eoi_parent,
>> + .irq_set_type = ls1021a_extirq_set_type,
>> + .irq_retrigger = irq_chip_retrigger_hierarchy,
>> + .irq_set_affinity = irq_chip_set_affinity_parent,
>> +};
>> +
>> +static int
>> +ls1021a_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
>> + unsigned long *hwirq, unsigned int *type)
>> +{
>> + if (!is_of_node(fwspec->fwnode))
>> + return -EINVAL;
>> +
>> + if (fwspec->param_count != 3)
>> + return -EINVAL;
>> +
>> + /* No PPI should point to this domain */
>> + if (fwspec->param[0] != 0)
>> + return -EINVAL;
>> +
>> + *hwirq = fwspec->param[1];
>
> Is a check for the hwirq value required here? I'm not an expert on
> irqchip API, so I just wonder.
In general, the driver is not in the business of validating the DT. But
that wouldn't hurt...
>
>> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
>> + return 0;
>> +}
>> +
>> +static int
>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> + unsigned int nr_irqs, void *arg)
>> +{
>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
> ^^^^^^
> No need for static here.
Why would you store this on the stack each time you enter the function?
That's the wrong construct (these values should come from DT), but
static is perfectly fine.
[...]
>> + domain_parent = irq_find_host(parent);
>> + if (!domain_parent) {
>> + pr_err("interrupt-parent not found\n");
>> + return -EINVAL;
>> + }
>
> Mh, does this mean if GIC has not been probed, this probe is not deferred?
> Is there an API to check for that?
This is not a normal driver, there is not deferred probing. You'd get
this error if the kernel had gone really wrong.
>
>> + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> ^^^^^^^
> devm_kzalloc
>> + if (!chip_data)
>> + return -ENOMEM;
>> +
>> + chip_data->syscon = syscon_regmap_lookup_by_phandle(node, "syscon");
>> + if (IS_ERR(chip_data->syscon)) {
>> + ret = PTR_ERR(chip_data->syscon);
>> + goto out_free_chip;
>> + }
>> +
>> + domain = irq_domain_add_hierarchy(domain_parent, 0, NIRQ, node,
>> + &extirq_domain_ops, chip_data);
>> + if (!domain) {
>> + ret = -ENOMEM;
>> + goto out_free_chip;
>> + }
>> +
>> + return 0;
>> +
>> +out_free_chip:
>> + kfree(chip_data);
>> + return ret;
>
> Using devm_kzalloc this label can be skipped.
Show me the struct device.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-08 16:09 ` Marc Zyngier
@ 2017-12-11 9:08 ` Rasmus Villemoes
2017-12-11 9:45 ` Alexander Stein
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-11 9:08 UTC (permalink / raw)
To: Marc Zyngier, Alexander Stein
Cc: Thomas Gleixner, Jason Cooper, Rob Herring, Mark Rutland,
linux-kernel, devicetree
On 2017-12-08 17:09, Marc Zyngier wrote:
> On 08/12/17 15:11, Alexander Stein wrote:
>> Hi Rasmus,
>>
>>> +
>>> +Required properties:
>>> +- compatible: should be "fsl,ls1021a-extirq"
>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
>>
>> Do you really need 3 interrupt-cells here? As you've written below you don't
>> support PPI anyway the 1st flag might be dropped then. So support just 2 cells:
>> * IRQ number (IRQ0 - IRQ5)
>> * IRQ flags
>
> The convention for irqchip stacked on top of a GIC is to keep the
> interrupt specifier the same. It makes the maintenance if the DT much
> easier, and doesn't hurt at all.
Yes, I just followed the lead of existing drivers.
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index b842dfdc903f..d4576dce24b2 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>>> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>>> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
>>> obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
>>> +obj-$(CONFIG_SOC_LS1021A) += irq-ls1021a.o
>>
>> I guess this should be kept sorted alphabetically.
>
> There is no such requirement. But grouping it next to the other FSL
> irqchip would make more sense.
Yeah, if the Makefile had been at least somewhat sorted already I'd have
followed that. I'll move it next to LS_SCFG_MSI in next version.
>>> +static int
>>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
>>> +{
>>> + irq_hw_number_t hwirq = data->hwirq;
>>> + struct extirq_chip_data *chip_data = data->chip_data;
>>> + u32 value, mask;
>>> + int ret;
>>> +
>>> + mask = 1U << (31 - hwirq);
>>
>> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
>> by the left most position in the register layout. This is just strange way
>> to express bit-endian access.
Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
reserved with a POR value of 0. Fortunately, they can still be set and
read back, and when I did 1U << hwirq it was some of those bits that got
set (the POR value of the six used bits are all 1, so the hardware still
worked on my board because all the lines happen to be of negative polarity).
>> Anyway, please use BIT(x) instead.
I really prefer not to, that macro obfuscates the type, and unsigned
long is the wrong thing to use for something that must be a 32 bit
quantity. Sure, BITS_PER_LONG==32 in this case, but I don't think
BIT(foo) is any easier to read than 1U << (foo).
>>> +
>>> + /* Don't do the INTPCR_REG update if the parent irq_set_type will EINVAL. */
>>> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>>> + return -EINVAL;
>>
>> I wonder if it is better to call data->parent_data->chip->irq_set_type(data, type)
>> here instead and call regmap if this suceeded.
>
> Not really. In both cases, you need to evaluate the failure (which is
> not don here). So ordering doesn't matter. What actually matters is
> error handling and atomicity (in this case, making sure that drivers
> cannot observe an interrupt flood between the two reconfigurations).
I'm not really sure when the interrupt gets unmasked, but if it happens
during the parent ->set_type, we must have set the polarity beforehand.
Also, I don't see why one would need to undo the INTPCR update - the
polarity of the external line is a property of whatever hardware is
attached (right?), so setting the INTPCR according to the DT just
ensures the GIC gets a positive signal. Anyway, if I do need to add
unwind code, I suppose the answer to
>>> + /* regmap does internal locking, but do we need to provide our
>>> + * own across the parent irq_set_type call? */
is yes.
>>> + *hwirq = fwspec->param[1];
>>
>> Is a check for the hwirq value required here? I'm not an expert on
>> irqchip API, so I just wonder.
>
> In general, the driver is not in the business of validating the DT. But
> that wouldn't hurt...
Yeah, wasn't sure about this, but I can certainly add a check.
>>
>>> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
>>> + return 0;
>>> +}
>>> +
>>> +static int
>>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>> + unsigned int nr_irqs, void *arg)
>>> +{
>>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
>> ^^^^^^
>> No need for static here.
>
> Why would you store this on the stack each time you enter the function?
Exactly, it takes a lot less .rodata to make this static than having gcc
generate .text to build this array on the stack.
> That's the wrong construct (these values should come from DT), but
> static is perfectly fine.
OK.
> [...]
>
>>> + domain_parent = irq_find_host(parent);
>>> + if (!domain_parent) {
>>> + pr_err("interrupt-parent not found\n");
>>> + return -EINVAL;
>>> + }
>>
>> Mh, does this mean if GIC has not been probed, this probe is not deferred?
>> Is there an API to check for that?
>
> This is not a normal driver, there is not deferred probing. You'd get
> this error if the kernel had gone really wrong.
Yes, isn't this what the code in of_irq_init does? Initialize parent
interrupt controllers before their children (even if this maybe doesn't
qualify as a real interrupt controller)?
Rasmus
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-11 9:08 ` Rasmus Villemoes
@ 2017-12-11 9:45 ` Alexander Stein
2017-12-11 10:02 ` Alexander Stein
0 siblings, 1 reply; 39+ messages in thread
From: Alexander Stein @ 2017-12-11 9:45 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Marc Zyngier, Thomas Gleixner, Jason Cooper, Rob Herring,
Mark Rutland, linux-kernel, devicetree
On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote:
> >>> +static int
> >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> >>> +{
> >>> + irq_hw_number_t hwirq = data->hwirq;
> >>> + struct extirq_chip_data *chip_data = data->chip_data;
> >>> + u32 value, mask;
> >>> + int ret;
> >>> +
> >>> + mask = 1U << (31 - hwirq);
> >>
> >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> >> by the left most position in the register layout. This is just strange way
> >> to express bit-endian access.
>
> Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
> reserved with a POR value of 0. Fortunately, they can still be set and
> read back, and when I did 1U << hwirq it was some of those bits that got
> set (the POR value of the six used bits are all 1, so the hardware still
> worked on my board because all the lines happen to be of negative polarity).
Which functions do reg_read and reg_write in chip_data->syscon->bus_context
actually point to?
bus_context is actually a struct regmap_mmio_context *.
> >> Anyway, please use BIT(x) instead.
>
> I really prefer not to, that macro obfuscates the type, and unsigned
> long is the wrong thing to use for something that must be a 32 bit
> quantity. Sure, BITS_PER_LONG==32 in this case, but I don't think
> BIT(foo) is any easier to read than 1U << (foo).
Well, there a lots of other places where BIT(x) is used for u32 data types,
or even 16 Bit types. IMHO BIT(x) is more obvious as it already says set Bit x
> >>> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >>> + unsigned int nr_irqs, void *arg)
> >>> +{
> >>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
> >> ^^^^^^
> >> No need for static here.
> >
> > Why would you store this on the stack each time you enter the function?
>
> Exactly, it takes a lot less .rodata to make this static than having gcc
> generate .text to build this array on the stack.
>
> > That's the wrong construct (these values should come from DT), but
> > static is perfectly fine.
>
> OK.
Intresting. Thanks for the info.
Regards,
Alexander
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-11 9:45 ` Alexander Stein
@ 2017-12-11 10:02 ` Alexander Stein
2017-12-11 13:45 ` Rasmus Villemoes
0 siblings, 1 reply; 39+ messages in thread
From: Alexander Stein @ 2017-12-11 10:02 UTC (permalink / raw)
To: Alexander Stein
Cc: Rasmus Villemoes, Marc Zyngier, Thomas Gleixner, Jason Cooper,
Rob Herring, Mark Rutland, linux-kernel, devicetree
On Monday, December 11, 2017, 10:45:09 AM CET Alexander Stein wrote:
> On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote:
> > >>> +static int
> > >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> > >>> +{
> > >>> + irq_hw_number_t hwirq = data->hwirq;
> > >>> + struct extirq_chip_data *chip_data = data->chip_data;
> > >>> + u32 value, mask;
> > >>> + int ret;
> > >>> +
> > >>> + mask = 1U << (31 - hwirq);
> > >>
> > >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead
> > >> by the left most position in the register layout. This is just strange way
> > >> to express bit-endian access.
> >
> > Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as
> > reserved with a POR value of 0. Fortunately, they can still be set and
> > read back, and when I did 1U << hwirq it was some of those bits that got
> > set (the POR value of the six used bits are all 1, so the hardware still
> > worked on my board because all the lines happen to be of negative polarity).
>
> Which functions do reg_read and reg_write in chip_data->syscon->bus_context
> actually point to?
> bus_context is actually a struct regmap_mmio_context *.
Oh, and what is the content of register SCFG_SCFGREVCR?
Alexander
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-11 10:02 ` Alexander Stein
@ 2017-12-11 13:45 ` Rasmus Villemoes
2017-12-11 14:06 ` Rasmus Villemoes
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-11 13:45 UTC (permalink / raw)
To: Alexander Stein
Cc: Marc Zyngier, Thomas Gleixner, Jason Cooper, Rob Herring,
Mark Rutland, linux-kernel, devicetree
On 2017-12-11 11:02, Alexander Stein wrote:
> Oh, and what is the content of register SCFG_SCFGREVCR?
Good point. On my board it's 0xffffffff, set even before U-boot starts,
and lots board support code in U-boot expects this. I can't immediately
find examples in the linux source code that actually writes to the scfg,
so I don't know if we already have that as an implicit assumption as
well. But it would be kind of nasty to have to make the code read
SCFG_SCFGREVCR and decide the bit pattern to use based on that -
especially since I wouldn't be able to test it.
Who thought such a magic switch could ever be a good idea?
--
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villemoes@prevas.dk
www.prevas.dk
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-11 13:45 ` Rasmus Villemoes
@ 2017-12-11 14:06 ` Rasmus Villemoes
2017-12-11 14:38 ` Alexander Stein
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-11 14:06 UTC (permalink / raw)
To: Alexander Stein
Cc: Marc Zyngier, Thomas Gleixner, Jason Cooper, Rob Herring,
Mark Rutland, linux-kernel, devicetree
On 2017-12-11 14:45, Rasmus Villemoes wrote:
> On 2017-12-11 11:02, Alexander Stein wrote:
>
>> Oh, and what is the content of register SCFG_SCFGREVCR?
>
> Good point. On my board it's 0xffffffff, set even before U-boot starts,
> and lots board support code in U-boot expects this. I can't immediately
> find examples in the linux source code that actually writes to the scfg,
Not a write, but we do already implicitly assume SCFG_SCFGREVCR is set
to all-ones: In drivers/pci/dwc/pci-layerscape.c, bits which are
numbered 6-11 in the reference manual are extracted with a regmap_read()
followed by a left-shift by 20 and mask with 0x3f. That's consistent
with me setting bit 0 (reference manual enumeration) using 1U<<31.
Rasmus
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-11 14:06 ` Rasmus Villemoes
@ 2017-12-11 14:38 ` Alexander Stein
0 siblings, 0 replies; 39+ messages in thread
From: Alexander Stein @ 2017-12-11 14:38 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Marc Zyngier, Thomas Gleixner, Jason Cooper, Rob Herring,
Mark Rutland, linux-kernel, devicetree
On Monday, December 11, 2017, 3:06:52 PM CET Rasmus Villemoes wrote:
> On 2017-12-11 14:45, Rasmus Villemoes wrote:
> > On 2017-12-11 11:02, Alexander Stein wrote:
> >
> >> Oh, and what is the content of register SCFG_SCFGREVCR?
> >
> > Good point. On my board it's 0xffffffff, set even before U-boot starts,
> > and lots board support code in U-boot expects this. I can't immediately
> > find examples in the linux source code that actually writes to the scfg,
>
> Not a write, but we do already implicitly assume SCFG_SCFGREVCR is set
> to all-ones: In drivers/pci/dwc/pci-layerscape.c, bits which are
> numbered 6-11 in the reference manual are extracted with a regmap_read()
> followed by a left-shift by 20 and mask with 0x3f. That's consistent
> with me setting bit 0 (reference manual enumeration) using 1U<<31.
We set SCFG_SCFGREVCR to all-ones too, even before u-boot in rcw. Problem
is, this bit-reversal is only valid for SCFG. It's a shame, but at least add
a comment in the code you expect SCFG_SCFGREVCR as 0xffffffff.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-08 14:33 ` [RFC] irqchip: add support for LS1021A external interrupt lines Rasmus Villemoes
2017-12-08 15:11 ` Alexander Stein
@ 2017-12-08 16:02 ` Marc Zyngier
2017-12-11 9:30 ` Rasmus Villemoes
2017-12-12 23:28 ` Rob Herring
2017-12-20 8:30 ` [PATCH v2 1/2] irqchip: add support for Layerscape " Rasmus Villemoes
3 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2017-12-08 16:02 UTC (permalink / raw)
To: Rasmus Villemoes, Thomas Gleixner, Jason Cooper, Rob Herring,
Mark Rutland
Cc: Alexander Stein, linux-kernel, devicetree
On 08/12/17 14:33, Rasmus Villemoes wrote:
> The LS1021A allows inverting the polarity of six interrupt lines
> IRQ[0:5] via the scfg_intpcr register, effectively allowing
> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
> check the type, set the relevant bit in INTPCR accordingly, and fixup
> the type argument before calling the GIC's irq_set_type.
>
> In fact, the power-on-reset value of the INTPCR register is so that all
> six lines have their polarity inverted. Hence any hardware connected to
> those lines is unusable without this: If the line is indeed active low,
> the generic GIC code will reject an irq spec with IRQ_TYPE_LEVEL_LOW,
> while if the line is active high, we must obviously disable the polarity
> inversion before unmasking the interrupt.
>
> I suspect other layerscape SOCs may have something similar, but I have
> neither hardware nor documentation.
>
> Since we only need to keep a single pointer in the chip_data (the syscon
> regmap), the code could be a little simpler by dropping the struct
> extirq_chip_data and just store the regmap directly - but I don't know
> if I do need to add a lock or something else to the chip_data, so for
> this RFC I've kept the struct.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> Marc, Alexander, thanks a lot for your hints. This is what I came up
> with, mostly just copy-pasted from the mtk-sysirq case. I've tested
> that it works as expected on my board.
>
> .../interrupt-controller/fsl,ls1021a-extirq.txt | 19 +++
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-ls1021a.c | 157 +++++++++++++++++++++
> 3 files changed, 177 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> create mode 100644 drivers/irqchip/irq-ls1021a.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> new file mode 100644
> index 000000000000..53b04b6e1a80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> @@ -0,0 +1,19 @@
> +* Freescale LS1021A external IRQs
> +
> +The LS1021A supports inverting the polarity of six external interrupt lines.
> +
> +Required properties:
> +- compatible: should be "fsl,ls1021a-extirq"
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
> +- interrupt-parent: phandle of GIC.
> +- syscon: phandle of Supplemental Configuration Unit (scfg).
> +
> +Example:
> + extirq: interrupt-controller@15701ac {
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + syscon = <&scfg>;
> + };
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b842dfdc903f..d4576dce24b2 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
> obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o
> +obj-$(CONFIG_SOC_LS1021A) += irq-ls1021a.o
> diff --git a/drivers/irqchip/irq-ls1021a.c b/drivers/irqchip/irq-ls1021a.c
> new file mode 100644
> index 000000000000..2ec4fc023549
> --- /dev/null
> +++ b/drivers/irqchip/irq-ls1021a.c
> @@ -0,0 +1,157 @@
> +#define pr_fmt(fmt) "irq-ls1021a: " fmt
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define INTPCR_REG 0x01ac
> +#define NIRQ 6
These should come from the DT, specially if as suggested above, there
are other similar HW in the wild.
> +
> +struct extirq_chip_data {
> + struct regmap *syscon;
> +};
> +
> +static int
> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + irq_hw_number_t hwirq = data->hwirq;
> + struct extirq_chip_data *chip_data = data->chip_data;
> + u32 value, mask;
> + int ret;
> +
> + mask = 1U << (31 - hwirq);
> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> + if (type == IRQ_TYPE_LEVEL_LOW)
> + type = IRQ_TYPE_LEVEL_HIGH;
> + else
> + type = IRQ_TYPE_EDGE_RISING;
> + value = mask;
> + } else {
> + value = 0;
> + }
> +
> + /* Don't do the INTPCR_REG update if the parent irq_set_type will EINVAL. */
> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> + return -EINVAL;
How about starting by rejecting the values that you cannot handle (which
seems to only be IRQ_TYPE_EDGE_BOTH)? Actually, if you wrote the whole
thing as a swtch/case, it'd be a lot more readable.
> +
> + /* regmap does internal locking, but do we need to provide our
> + * own across the parent irq_set_type call? */
Comment format.
> + regmap_update_bits(chip_data->syscon, INTPCR_REG, mask, value);
> +
> + data = data->parent_data;
> + ret = data->chip->irq_set_type(data, type);
Restore the previous regmap configuration on failure? Also, given that
you end-up changing the interrupt polarity in a non-atomic way (you have
two independent irqchips), it'd feel safer if you'd use
IRQCHIP_SET_TYPE_MASKED.
> +
> + return ret;
> +}
> +
> +static struct irq_chip extirq_chip = {
> + .name = "LS1021A_EXTIRQ",
Care to make this shorter?
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_set_type = ls1021a_extirq_set_type,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> +};
> +
> +static int
> +ls1021a_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
> + unsigned long *hwirq, unsigned int *type)
> +{
> + if (!is_of_node(fwspec->fwnode))
> + return -EINVAL;
> +
> + if (fwspec->param_count != 3)
> + return -EINVAL;
> +
> + /* No PPI should point to this domain */
> + if (fwspec->param[0] != 0)
> + return -EINVAL;
> +
> + *hwirq = fwspec->param[1];
> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> + return 0;
> +}
> +
> +static int
> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
This should really come from your DT.
> + int i;
> + irq_hw_number_t hwirq;
> + struct irq_fwspec *fwspec = arg;
> + struct irq_fwspec gic_fwspec;
> +
> + if (fwspec->param_count != 3)
> + return -EINVAL;
> +
> + if (fwspec->param[0])
> + return -EINVAL;
> +
> + hwirq = fwspec->param[1];
> + for (i = 0; i < nr_irqs; i++)
This loop is pointless, as nr_irqs can only be >1 in the multi-MSI case.
> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> + &extirq_chip,
> + domain->host_data);
> +
> + gic_fwspec.fwnode = domain->parent->fwnode;
> + gic_fwspec.param_count = 3;
> + gic_fwspec.param[0] = 0;
> + gic_fwspec.param[1] = xlate[hwirq];
> + gic_fwspec.param[2] = fwspec->param[2];
> +
> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_fwspec);
> +}
> +
> +static const struct irq_domain_ops extirq_domain_ops = {
> + .translate = ls1021a_extirq_domain_translate,
> + .alloc = ls1021a_extirq_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> +};
> +
> +static int __init
> +ls1021a_extirq_of_init(struct device_node *node, struct device_node *parent)
> +{
> +
> + struct irq_domain *domain, *domain_parent;
> + struct extirq_chip_data *chip_data;
> + int ret;
> +
> + domain_parent = irq_find_host(parent);
> + if (!domain_parent) {
> + pr_err("interrupt-parent not found\n");
> + return -EINVAL;
> + }
> +
> + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> + if (!chip_data)
> + return -ENOMEM;
> +
> + chip_data->syscon = syscon_regmap_lookup_by_phandle(node, "syscon");
> + if (IS_ERR(chip_data->syscon)) {
> + ret = PTR_ERR(chip_data->syscon);
> + goto out_free_chip;
> + }
> +
> + domain = irq_domain_add_hierarchy(domain_parent, 0, NIRQ, node,
> + &extirq_domain_ops, chip_data);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out_free_chip;
> + }
> +
> + return 0;
> +
> +out_free_chip:
> + kfree(chip_data);
> + return ret;
> +}
> +
> +IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls1021a_extirq_of_init);
>
Overall, it is a bit annoying that you just copied the driver altogether
instead of trying to allow the common stuff to be shared between
drivers. Most of this is just boilerplate code...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-08 16:02 ` Marc Zyngier
@ 2017-12-11 9:30 ` Rasmus Villemoes
2017-12-11 18:29 ` Marc Zyngier
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-11 9:30 UTC (permalink / raw)
To: Marc Zyngier, Thomas Gleixner, Jason Cooper, Rob Herring, Mark Rutland
Cc: Alexander Stein, linux-kernel, devicetree
On 2017-12-08 17:02, Marc Zyngier wrote:
>> +
>> +#define INTPCR_REG 0x01ac
>> +#define NIRQ 6
>
> These should come from the DT, specially if as suggested above, there
> are other similar HW in the wild.
OK, but see below.
>> +static int
>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + irq_hw_number_t hwirq = data->hwirq;
>> + struct extirq_chip_data *chip_data = data->chip_data;
>> + u32 value, mask;
>> + int ret;
>> +
>> + mask = 1U << (31 - hwirq);
>> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
>> + if (type == IRQ_TYPE_LEVEL_LOW)
>> + type = IRQ_TYPE_LEVEL_HIGH;
>> + else
>> + type = IRQ_TYPE_EDGE_RISING;
>> + value = mask;
>> + } else {
>> + value = 0;
>> + }
>> +
>> + /* Don't do the INTPCR_REG update if the parent irq_set_type will EINVAL. */
>> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>> + return -EINVAL;
>
> How about starting by rejecting the values that you cannot handle (which
> seems to only be IRQ_TYPE_EDGE_BOTH)? Actually, if you wrote the whole
> thing as a swtch/case, it'd be a lot more readable.
OK, will try that.
>> +
>> + /* regmap does internal locking, but do we need to provide our
>> + * own across the parent irq_set_type call? */
>
> Comment format.
[Somewhat deliberate, I never meant for that comment to stay in a final
version. It's gone once I figure out the answer.]
>> + regmap_update_bits(chip_data->syscon, INTPCR_REG, mask, value);
>> +
>> + data = data->parent_data;
>> + ret = data->chip->irq_set_type(data, type);
>
> Restore the previous regmap configuration on failure?
Not sure what one would get from that?
> Also, given that
> you end-up changing the interrupt polarity in a non-atomic way (you have
> two independent irqchips), it'd feel safer if you'd use
> IRQCHIP_SET_TYPE_MASKED.
Ah, yes, makes sense. Will do.
>> +
>> + return ret;
>> +}
>> +
>> +static struct irq_chip extirq_chip = {
>> + .name = "LS1021A_EXTIRQ",
>
> Care to make this shorter?
Sure, I'll just call it extirq.
>> +static int
>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> + unsigned int nr_irqs, void *arg)
>> +{
>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169};
>
> This should really come from your DT.
>
>> + int i;
>> + irq_hw_number_t hwirq;
>> + struct irq_fwspec *fwspec = arg;
>> + struct irq_fwspec gic_fwspec;
>> +
>> + if (fwspec->param_count != 3)
>> + return -EINVAL;
>> +
>> + if (fwspec->param[0])
>> + return -EINVAL;
>> +
>> + hwirq = fwspec->param[1];
>> + for (i = 0; i < nr_irqs; i++)
>
> This loop is pointless, as nr_irqs can only be >1 in the multi-MSI case.
OK, thanks.
>> +static int __init
>> +ls1021a_extirq_of_init(struct device_node *node, struct device_node *parent)
>> +{
>> +
>> + struct irq_domain *domain, *domain_parent;
>> + struct extirq_chip_data *chip_data;
>> + int ret;
>> +
>> + domain_parent = irq_find_host(parent);
>> + if (!domain_parent) {
>> + pr_err("interrupt-parent not found\n");
>> + return -EINVAL;
>> + }
>> +
>> + chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
>> + if (!chip_data)
>> + return -ENOMEM;
>> +
>> + chip_data->syscon = syscon_regmap_lookup_by_phandle(node, "syscon");
>> + if (IS_ERR(chip_data->syscon)) {
>> + ret = PTR_ERR(chip_data->syscon);
>> + goto out_free_chip;
>> + }
>> +
>> + domain = irq_domain_add_hierarchy(domain_parent, 0, NIRQ, node,
>> + &extirq_domain_ops, chip_data);
>> + if (!domain) {
>> + ret = -ENOMEM;
>> + goto out_free_chip;
>> + }
>> +
>> + return 0;
>> +
>> +out_free_chip:
>> + kfree(chip_data);
>> + return ret;
>> +}
>> +
>> +IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls1021a_extirq_of_init);
>>
>
> Overall, it is a bit annoying that you just copied the driver altogether
> instead of trying to allow the common stuff to be shared between
> drivers. Most of this is just boilerplate code...
Yes, it did annoy me as well. However, the real meat of this is which
bits of which register to poke to support a negative polarity irq, and
there doesn't seem to be a good way to express that in DT. The register
offset and the mapping from external irq# to the GIC one is reasonably
easy (and would thus get rid of my NIRQ and INTPCR macros), but
describing the mapping from IRQ# to the bit that needs to be set (or
cleared) seems much harder. I cannot generalize from one example, so
lacking documentation for any other Layerscape SOC, whatever I might
come up with might not actually be useful for other hardware, making it
rather pointless. But if you have any suggestions for how the DT
bindings might look, I'm all ears.
Thanks a lot for your feedback!
Rasmus
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-11 9:30 ` Rasmus Villemoes
@ 2017-12-11 18:29 ` Marc Zyngier
0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2017-12-11 18:29 UTC (permalink / raw)
To: Rasmus Villemoes, Thomas Gleixner, Jason Cooper, Rob Herring,
Mark Rutland
Cc: Alexander Stein, linux-kernel, devicetree
On 11/12/17 09:30, Rasmus Villemoes wrote:
> On 2017-12-08 17:02, Marc Zyngier wrote:
[...]
>> Overall, it is a bit annoying that you just copied the driver altogether
>> instead of trying to allow the common stuff to be shared between
>> drivers. Most of this is just boilerplate code...
>
> Yes, it did annoy me as well. However, the real meat of this is which
> bits of which register to poke to support a negative polarity irq, and
> there doesn't seem to be a good way to express that in DT. The register
> offset and the mapping from external irq# to the GIC one is reasonably
> easy (and would thus get rid of my NIRQ and INTPCR macros), but
> describing the mapping from IRQ# to the bit that needs to be set (or
> cleared) seems much harder. I cannot generalize from one example, so
> lacking documentation for any other Layerscape SOC, whatever I might
> come up with might not actually be useful for other hardware, making it
> rather pointless. But if you have any suggestions for how the DT
> bindings might look, I'm all ears.
You could have a list of <bit irq> pairs defining the mapping, for
example. But I'd encourage you to get in touch with the Freescale/NXP
folks and find out how this HW works. get_maintainers.pl gives me this:
Shawn Guo <shawnguo@kernel.org>
Tang Yuantian <Yuantian.Tang@nxp.com>
Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
Madalin Bucur <madalin.bucur@nxp.com>
Minghuan Lian <Minghuan.Lian@nxp.com>
Yuantian Tang <andy.tang@nxp.com>
Yangbo Lu <yangbo.lu@nxp.com>
"Horia Geantă" <horia.geanta@nxp.com>
I suggest you spam them and find out.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-08 14:33 ` [RFC] irqchip: add support for LS1021A external interrupt lines Rasmus Villemoes
2017-12-08 15:11 ` Alexander Stein
2017-12-08 16:02 ` Marc Zyngier
@ 2017-12-12 23:28 ` Rob Herring
2017-12-15 22:55 ` Rasmus Villemoes
2017-12-20 8:30 ` [PATCH v2 1/2] irqchip: add support for Layerscape " Rasmus Villemoes
3 siblings, 1 reply; 39+ messages in thread
From: Rob Herring @ 2017-12-12 23:28 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland,
Alexander Stein, linux-kernel, devicetree
On Fri, Dec 08, 2017 at 03:33:00PM +0100, Rasmus Villemoes wrote:
> The LS1021A allows inverting the polarity of six interrupt lines
> IRQ[0:5] via the scfg_intpcr register, effectively allowing
> IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
> check the type, set the relevant bit in INTPCR accordingly, and fixup
> the type argument before calling the GIC's irq_set_type.
>
> In fact, the power-on-reset value of the INTPCR register is so that all
> six lines have their polarity inverted. Hence any hardware connected to
> those lines is unusable without this: If the line is indeed active low,
> the generic GIC code will reject an irq spec with IRQ_TYPE_LEVEL_LOW,
> while if the line is active high, we must obviously disable the polarity
> inversion before unmasking the interrupt.
>
> I suspect other layerscape SOCs may have something similar, but I have
> neither hardware nor documentation.
>
> Since we only need to keep a single pointer in the chip_data (the syscon
> regmap), the code could be a little simpler by dropping the struct
> extirq_chip_data and just store the regmap directly - but I don't know
> if I do need to add a lock or something else to the chip_data, so for
> this RFC I've kept the struct.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> Marc, Alexander, thanks a lot for your hints. This is what I came up
> with, mostly just copy-pasted from the mtk-sysirq case. I've tested
> that it works as expected on my board.
>
> .../interrupt-controller/fsl,ls1021a-extirq.txt | 19 +++
Please split to separate patch.
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-ls1021a.c | 157 +++++++++++++++++++++
> 3 files changed, 177 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> create mode 100644 drivers/irqchip/irq-ls1021a.c
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> new file mode 100644
> index 000000000000..53b04b6e1a80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> @@ -0,0 +1,19 @@
> +* Freescale LS1021A external IRQs
> +
> +The LS1021A supports inverting the polarity of six external interrupt lines.
> +
> +Required properties:
> +- compatible: should be "fsl,ls1021a-extirq"
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
> +- interrupt-parent: phandle of GIC.
> +- syscon: phandle of Supplemental Configuration Unit (scfg).
Can this be a child of that node instead?
> +
> +Example:
> + extirq: interrupt-controller@15701ac {
Unit-address without reg is not valid. Building with W=1 will tell you
this.
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + syscon = <&scfg>;
> + };
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-12 23:28 ` Rob Herring
@ 2017-12-15 22:55 ` Rasmus Villemoes
2017-12-21 22:45 ` Rob Herring
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-15 22:55 UTC (permalink / raw)
To: Rob Herring
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland,
Alexander Stein, linux-kernel, devicetree
On 2017-12-13 00:28, Rob Herring wrote:
> On Fri, Dec 08, 2017 at 03:33:00PM +0100, Rasmus Villemoes wrote:
>>
>> .../interrupt-controller/fsl,ls1021a-extirq.txt | 19 +++
>
> Please split to separate patch.
Will do.
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
>> @@ -0,0 +1,19 @@
>> +* Freescale LS1021A external IRQs
>> +
>> +The LS1021A supports inverting the polarity of six external interrupt lines.
>> +
>> +Required properties:
>> +- compatible: should be "fsl,ls1021a-extirq"
>> +- interrupt-controller: Identifies the node as an interrupt controller
>> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
>> +- interrupt-parent: phandle of GIC.
>> +- syscon: phandle of Supplemental Configuration Unit (scfg).
>
> Can this be a child of that node instead?
I suppose it could, but I don't think it would make much sense. In any
case, I did it this way because that seemed to be the way the syscon
driver is used in lots of other cases, cf. all the occurrences of
syscon_regmap_lookup_by_phandle() and the corresponding bindings - I
don't think I've seen any of those cases represent the syscon-using node
as a child of the syscon node.
>> +
>> +Example:
>> + extirq: interrupt-controller@15701ac {
>
> Unit-address without reg is not valid. Building with W=1 will tell you
> this.
Thanks, that was actually a leftover from an earlier version.
Rasmus
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC] irqchip: add support for LS1021A external interrupt lines
2017-12-15 22:55 ` Rasmus Villemoes
@ 2017-12-21 22:45 ` Rob Herring
0 siblings, 0 replies; 39+ messages in thread
From: Rob Herring @ 2017-12-21 22:45 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland,
Alexander Stein, linux-kernel, devicetree
On Fri, Dec 15, 2017 at 11:55:34PM +0100, Rasmus Villemoes wrote:
> On 2017-12-13 00:28, Rob Herring wrote:
> > On Fri, Dec 08, 2017 at 03:33:00PM +0100, Rasmus Villemoes wrote:
> >>
> >> .../interrupt-controller/fsl,ls1021a-extirq.txt | 19 +++
> >
> > Please split to separate patch.
>
> Will do.
>
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls1021a-extirq.txt
> >> @@ -0,0 +1,19 @@
> >> +* Freescale LS1021A external IRQs
> >> +
> >> +The LS1021A supports inverting the polarity of six external interrupt lines.
> >> +
> >> +Required properties:
> >> +- compatible: should be "fsl,ls1021a-extirq"
> >> +- interrupt-controller: Identifies the node as an interrupt controller
> >> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
> >> +- interrupt-parent: phandle of GIC.
> >> +- syscon: phandle of Supplemental Configuration Unit (scfg).
> >
> > Can this be a child of that node instead?
>
> I suppose it could, but I don't think it would make much sense. In any
> case, I did it this way because that seemed to be the way the syscon
> driver is used in lots of other cases, cf. all the occurrences of
> syscon_regmap_lookup_by_phandle() and the corresponding bindings - I
> don't think I've seen any of those cases represent the syscon-using node
> as a child of the syscon node.
I'm sure there are examples because this is a frequent review comment.
In any case, define the binding by what the h/w looks like, not what the
kernel *currently* wants.
Rob
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 1/2] irqchip: add support for Layerscape external interrupt lines
2017-12-08 14:33 ` [RFC] irqchip: add support for LS1021A external interrupt lines Rasmus Villemoes
` (2 preceding siblings ...)
2017-12-12 23:28 ` Rob Herring
@ 2017-12-20 8:30 ` Rasmus Villemoes
2017-12-20 8:30 ` [PATCH v2 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
2018-01-22 9:21 ` [PATCH v3 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
3 siblings, 2 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-20 8:30 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Andy Tang, Shawn Guo, Rob Herring, Alexander Stein,
Rasmus Villemoes, linux-kernel
The LS1021A allows inverting the polarity of six interrupt lines
IRQ[0:5] via the scfg_intpcr register, effectively allowing
IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
check the type, set the relevant bit in INTPCR accordingly, and fixup
the type argument before calling the GIC's irq_set_type.
In fact, the power-on-reset value of the INTPCR register on the LS1021A
is so that all six lines have their polarity inverted. Hence any
hardware connected to those lines is unusable without this: If the line
is indeed active low, the generic GIC code will reject an irq spec with
IRQ_TYPE_LEVEL_LOW, while if the line is active high, we must obviously
disable the polarity inversion (writing 0 to the relevant bit) before
unmasking the interrupt.
Some other Layerscape SOCs (LS1043A, LS1046A) reportedly have a similar
feature, just with a different number of external interrupt lines (and a
different POR value for the INTPCR register). This driver should be
prepared for supporting those by properly filling out the device tree
node, but I don't have the full reference manual, nor the hardware to be
able to test it. I do know, from a tiny clipout from one of the other
reference manuals I was shown, that 1U<<n is the right formula to
use for setting/clearing the bit corresponding to the external IRQn, but
I don't know which interrupts on the GIC those lines represent.
There's also a little Kconfig/Kbuild issue: For now, I've let the driver
be built if CONFIG_SOC_LS1021A is set. For the others, it might make
sense to instead use CONFIG_ARCH_LAYERSCAPE, but SOC_LS1021A does not
select ARCH_LAYERSCAPE. The simplest solution is probably to do what is
done for irq-ls-scfg-msi.c: introduce a "def_bool y if SOC_LS1021A ||
ARCH_LAYERSCAPE" symbol. But I think that can wait until somebody with
the hardware actually tests that this works for the other platforms. At
that point, one can also add the extra IRQCHIP_DECLARE instances.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
Changes since the RFC:
- documentation update in separate patch
- take offset to INTPCR register from DT
- take number of external irqs and their mapping to GIC interrupt numbers from DT
- make name a little more generic
- use IRQCHIP_SET_TYPE_MASKED
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-ls-extirq.c | 173 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 174 insertions(+)
create mode 100644 drivers/irqchip/irq-ls-extirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b842dfdc903f..32d7160680fe 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_MVEBU_ICU) += irq-mvebu-icu.o
obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o
obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o
obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
+obj-$(CONFIG_SOC_LS1021A) += irq-ls-extirq.o
obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
new file mode 100644
index 000000000000..c7969514eea2
--- /dev/null
+++ b/drivers/irqchip/irq-ls-extirq.c
@@ -0,0 +1,173 @@
+#define pr_fmt(fmt) "irq-ls-extirq: " fmt
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define MAXIRQ 12
+
+struct extirq_chip_data {
+ struct regmap *syscon;
+ u32 intpcr;
+ bool bit_reverse;
+ u32 nirq;
+ u32 parent_irq[MAXIRQ];
+};
+
+static int
+ls_extirq_set_type(struct irq_data *data, unsigned int type)
+{
+ irq_hw_number_t hwirq = data->hwirq;
+ struct extirq_chip_data *chip_data = data->chip_data;
+ u32 value, mask;
+
+ if (chip_data->bit_reverse)
+ mask = 1U << (31 - hwirq);
+ else
+ mask = 1U << hwirq;
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ value = mask;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ type = IRQ_TYPE_EDGE_RISING;
+ value = mask;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ case IRQ_TYPE_EDGE_RISING:
+ value = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(chip_data->syscon, chip_data->intpcr, mask, value);
+
+ data = data->parent_data;
+ return data->chip->irq_set_type(data, type);
+}
+
+static struct irq_chip extirq_chip = {
+ .name = "extirq",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = ls_extirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .flags = IRQCHIP_SET_TYPE_MASKED,
+};
+
+static int
+ls_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
+ unsigned long *hwirq, unsigned int *type)
+{
+ if (!is_of_node(fwspec->fwnode))
+ return -EINVAL;
+
+ if (fwspec->param_count != 3)
+ return -EINVAL;
+
+ /* No PPI should point to this domain */
+ if (fwspec->param[0] != GIC_SPI)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[1];
+ *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int
+ls_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ irq_hw_number_t hwirq;
+ struct irq_fwspec *fwspec = arg;
+ struct irq_fwspec gic_fwspec;
+ struct extirq_chip_data *chip_data = domain->host_data;
+
+ if (fwspec->param_count != 3)
+ return -EINVAL;
+
+ if (fwspec->param[0] != GIC_SPI)
+ return -EINVAL;
+
+ hwirq = fwspec->param[1];
+ if (hwirq >= chip_data->nirq)
+ return -EINVAL;
+
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &extirq_chip,
+ chip_data);
+
+ gic_fwspec.fwnode = domain->parent->fwnode;
+ gic_fwspec.param_count = 3;
+ gic_fwspec.param[0] = GIC_SPI;
+ gic_fwspec.param[1] = chip_data->parent_irq[hwirq];
+ gic_fwspec.param[2] = fwspec->param[2];
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &gic_fwspec);
+}
+
+static const struct irq_domain_ops extirq_domain_ops = {
+ .translate = ls_extirq_domain_translate,
+ .alloc = ls_extirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static int __init
+ls_extirq_of_init(struct device_node *node, struct device_node *parent)
+{
+
+ struct irq_domain *domain, *domain_parent;
+ struct extirq_chip_data *chip_data;
+ int ret;
+
+ domain_parent = irq_find_host(parent);
+ if (!domain_parent) {
+ pr_err("interrupt-parent not found\n");
+ return -EINVAL;
+ }
+
+ chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+ if (!chip_data)
+ return -ENOMEM;
+
+ chip_data->syscon = syscon_regmap_lookup_by_phandle(node, "syscon");
+ ret = of_property_read_u32_index(node, "syscon", 1, &chip_data->intpcr);
+
+ if (IS_ERR(chip_data->syscon))
+ ret = PTR_ERR(chip_data->syscon);
+ if (ret)
+ goto out_free_chip;
+
+ ret = of_property_read_variable_u32_array(node, "interrupts", chip_data->parent_irq,
+ 1, ARRAY_SIZE(chip_data->parent_irq));
+ if (ret < 0)
+ goto out_free_chip;
+ chip_data->nirq = ret;
+ chip_data->bit_reverse = of_property_read_bool(node, "bit-reverse");
+
+ domain = irq_domain_add_hierarchy(domain_parent, 0, chip_data->nirq, node,
+ &extirq_domain_ops, chip_data);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out_free_chip;
+ }
+
+ return 0;
+
+out_free_chip:
+ kfree(chip_data);
+ return ret;
+}
+
+IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls_extirq_of_init);
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v2 2/2] dt/bindings: Add bindings for Layerscape external irqs
2017-12-20 8:30 ` [PATCH v2 1/2] irqchip: add support for Layerscape " Rasmus Villemoes
@ 2017-12-20 8:30 ` Rasmus Villemoes
2017-12-21 22:44 ` Rob Herring
2018-01-22 9:21 ` [PATCH v3 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
1 sibling, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2017-12-20 8:30 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, Mark Rutland
Cc: Andy Tang, Shawn Guo, Alexander Stein, Rasmus Villemoes,
linux-kernel, devicetree
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
.../interrupt-controller/fsl,ls-extirq.txt | 37 ++++++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
new file mode 100644
index 000000000000..7e4680866364
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
@@ -0,0 +1,37 @@
+* Freescale Layerscape external IRQs
+
+Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
+the polarity of certain external interrupt lines.
+
+Required properties:
+- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
+- interrupt-parent: phandle of GIC.
+- syscon: phandle of Supplemental Configuration Unit (scfg) and offset
+ to the INTPCR register.
+- interrupts: Specifies the mapping to interrupt numbers in the parent
+ interrupt controller. Interrupts are mapped one-to-one to parent
+ interrupts.
+
+Optional properties:
+- bit-reverse: This boolean property should be set on the LS1021A if
+ the SCFGREVCR register has been set to all-ones (which is usually
+ the case), meaning that all reads and writes of SCFG registers are
+ implicitly bit-reversed. Other compatible platforms do not have such
+ a register.
+
+Example:
+ extirq: extirq {
+ compatible = "fsl,ls1021a-extirq";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ interrupt-parent = <&gic>;
+ syscon = <&scfg 0x1ac>;
+ interrupts = <163 164 165 167 168 169>;
+ bit-reverse;
+ };
+
+
+ interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
+ <&extirq GIC_SPI 1 IRQ_TYPE_LEVEL_LOW>;
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v2 2/2] dt/bindings: Add bindings for Layerscape external irqs
2017-12-20 8:30 ` [PATCH v2 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2017-12-21 22:44 ` Rob Herring
0 siblings, 0 replies; 39+ messages in thread
From: Rob Herring @ 2017-12-21 22:44 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland,
Andy Tang, Shawn Guo, Alexander Stein, linux-kernel, devicetree
On Wed, Dec 20, 2017 at 09:30:30AM +0100, Rasmus Villemoes wrote:
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> .../interrupt-controller/fsl,ls-extirq.txt | 37 ++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
> new file mode 100644
> index 000000000000..7e4680866364
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
> @@ -0,0 +1,37 @@
> +* Freescale Layerscape external IRQs
> +
> +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
> +the polarity of certain external interrupt lines.
> +
> +Required properties:
> +- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
> +- interrupt-parent: phandle of GIC.
> +- syscon: phandle of Supplemental Configuration Unit (scfg) and offset
> + to the INTPCR register.
> +- interrupts: Specifies the mapping to interrupt numbers in the parent
> + interrupt controller. Interrupts are mapped one-to-one to parent
> + interrupts.
> +
> +Optional properties:
> +- bit-reverse: This boolean property should be set on the LS1021A if
fsl,bit-reverse
> + the SCFGREVCR register has been set to all-ones (which is usually
> + the case), meaning that all reads and writes of SCFG registers are
> + implicitly bit-reversed. Other compatible platforms do not have such
> + a register.
> +
> +Example:
> + extirq: extirq {
Node name should still be "interrupt-controller".
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + syscon = <&scfg 0x1ac>;
> + interrupts = <163 164 165 167 168 169>;
> + bit-reverse;
> + };
> +
> +
> + interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
> + <&extirq GIC_SPI 1 IRQ_TYPE_LEVEL_LOW>;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3 1/2] irqchip: add support for Layerscape external interrupt lines
2017-12-20 8:30 ` [PATCH v2 1/2] irqchip: add support for Layerscape " Rasmus Villemoes
2017-12-20 8:30 ` [PATCH v2 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2018-01-22 9:21 ` Rasmus Villemoes
2018-01-22 9:21 ` [PATCH v3 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
2018-01-25 15:02 ` [PATCH v4 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
1 sibling, 2 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2018-01-22 9:21 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes, linux-kernel
The LS1021A allows inverting the polarity of six interrupt lines
IRQ[0:5] via the scfg_intpcr register, effectively allowing
IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
check the type, set the relevant bit in INTPCR accordingly, and fixup
the type argument before calling the GIC's irq_set_type.
In fact, the power-on-reset value of the INTPCR register on the LS1021A
is so that all six lines have their polarity inverted. Hence any
hardware connected to those lines is unusable without this: If the line
is indeed active low, the generic GIC code will reject an irq spec with
IRQ_TYPE_LEVEL_LOW, while if the line is active high, we must obviously
disable the polarity inversion (writing 0 to the relevant bit) before
unmasking the interrupt.
Some other Layerscape SOCs (LS1043A, LS1046A) reportedly have a similar
feature, just with a different number of external interrupt lines (and a
different POR value for the INTPCR register). This driver should be
prepared for supporting those by properly filling out the device tree
node, but I don't have the full reference manual, nor the hardware to be
able to test it. I do know, from a tiny clipout from one of the other
reference manuals I was shown, that 1U<<n is the right formula to
use for setting/clearing the bit corresponding to the external IRQn, but
I don't know which interrupts on the GIC those lines represent.
There's also a little Kconfig/Kbuild issue: For now, I've let the driver
be built if CONFIG_SOC_LS1021A is set. For the others, it might make
sense to instead use CONFIG_ARCH_LAYERSCAPE, but SOC_LS1021A does not
select ARCH_LAYERSCAPE. The simplest solution is probably to do what is
done for irq-ls-scfg-msi.c: introduce a "def_bool y if SOC_LS1021A ||
ARCH_LAYERSCAPE" symbol. But I think that can wait until somebody with
the hardware actually tests that this works for the other platforms. At
that point, one can also add the extra IRQCHIP_DECLARE instances.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
Changes since v2 (all addressing comments from Rob Herring):
- use fsl,bit-reverse rather than bit-reverse
- make the dts node a child of the scfg node
- make the node name "interrupt-controller"
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-ls-extirq.c | 173 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 174 insertions(+)
create mode 100644 drivers/irqchip/irq-ls-extirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b842dfdc903f..32d7160680fe 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_MVEBU_ICU) += irq-mvebu-icu.o
obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o
obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o
obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
+obj-$(CONFIG_SOC_LS1021A) += irq-ls-extirq.o
obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
new file mode 100644
index 000000000000..ac84ad053b51
--- /dev/null
+++ b/drivers/irqchip/irq-ls-extirq.c
@@ -0,0 +1,173 @@
+#define pr_fmt(fmt) "irq-ls-extirq: " fmt
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define MAXIRQ 12
+
+struct extirq_chip_data {
+ struct regmap *syscon;
+ u32 intpcr;
+ bool bit_reverse;
+ u32 nirq;
+ u32 parent_irq[MAXIRQ];
+};
+
+static int
+ls_extirq_set_type(struct irq_data *data, unsigned int type)
+{
+ irq_hw_number_t hwirq = data->hwirq;
+ struct extirq_chip_data *chip_data = data->chip_data;
+ u32 value, mask;
+
+ if (chip_data->bit_reverse)
+ mask = 1U << (31 - hwirq);
+ else
+ mask = 1U << hwirq;
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ value = mask;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ type = IRQ_TYPE_EDGE_RISING;
+ value = mask;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ case IRQ_TYPE_EDGE_RISING:
+ value = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(chip_data->syscon, chip_data->intpcr, mask, value);
+
+ data = data->parent_data;
+ return data->chip->irq_set_type(data, type);
+}
+
+static struct irq_chip extirq_chip = {
+ .name = "extirq",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = ls_extirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .flags = IRQCHIP_SET_TYPE_MASKED,
+};
+
+static int
+ls_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
+ unsigned long *hwirq, unsigned int *type)
+{
+ if (!is_of_node(fwspec->fwnode))
+ return -EINVAL;
+
+ if (fwspec->param_count != 3)
+ return -EINVAL;
+
+ /* No PPI should point to this domain */
+ if (fwspec->param[0] != GIC_SPI)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[1];
+ *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int
+ls_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ irq_hw_number_t hwirq;
+ struct irq_fwspec *fwspec = arg;
+ struct irq_fwspec gic_fwspec;
+ struct extirq_chip_data *chip_data = domain->host_data;
+
+ if (fwspec->param_count != 3)
+ return -EINVAL;
+
+ if (fwspec->param[0] != GIC_SPI)
+ return -EINVAL;
+
+ hwirq = fwspec->param[1];
+ if (hwirq >= chip_data->nirq)
+ return -EINVAL;
+
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &extirq_chip,
+ chip_data);
+
+ gic_fwspec.fwnode = domain->parent->fwnode;
+ gic_fwspec.param_count = 3;
+ gic_fwspec.param[0] = GIC_SPI;
+ gic_fwspec.param[1] = chip_data->parent_irq[hwirq];
+ gic_fwspec.param[2] = fwspec->param[2];
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &gic_fwspec);
+}
+
+static const struct irq_domain_ops extirq_domain_ops = {
+ .translate = ls_extirq_domain_translate,
+ .alloc = ls_extirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static int __init
+ls_extirq_of_init(struct device_node *node, struct device_node *parent)
+{
+
+ struct irq_domain *domain, *domain_parent;
+ struct extirq_chip_data *chip_data;
+ int ret;
+
+ domain_parent = irq_find_host(parent);
+ if (!domain_parent) {
+ pr_err("interrupt-parent not found\n");
+ return -EINVAL;
+ }
+
+ chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+ if (!chip_data)
+ return -ENOMEM;
+
+ chip_data->syscon = syscon_node_to_regmap(node->parent);
+ ret = of_property_read_u32(node, "offset", &chip_data->intpcr);
+
+ if (IS_ERR(chip_data->syscon))
+ ret = PTR_ERR(chip_data->syscon);
+ if (ret)
+ goto out_free_chip;
+
+ ret = of_property_read_variable_u32_array(node, "interrupts", chip_data->parent_irq,
+ 1, ARRAY_SIZE(chip_data->parent_irq));
+ if (ret < 0)
+ goto out_free_chip;
+ chip_data->nirq = ret;
+ chip_data->bit_reverse = of_property_read_bool(node, "fsl,bit-reverse");
+
+ domain = irq_domain_add_hierarchy(domain_parent, 0, chip_data->nirq, node,
+ &extirq_domain_ops, chip_data);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out_free_chip;
+ }
+
+ return 0;
+
+out_free_chip:
+ kfree(chip_data);
+ return ret;
+}
+
+IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls_extirq_of_init);
--
2.15.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v3 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-01-22 9:21 ` [PATCH v3 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
@ 2018-01-22 9:21 ` Rasmus Villemoes
2018-01-24 15:28 ` Marc Zyngier
2018-01-25 15:02 ` [PATCH v4 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
1 sibling, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2018-01-22 9:21 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Mark Rutland
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes, linux-kernel, devicetree
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
.../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
new file mode 100644
index 000000000000..a71ce2c3eeae
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
@@ -0,0 +1,44 @@
+* Freescale Layerscape external IRQs
+
+Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
+the polarity of certain external interrupt lines.
+
+The device node must be a child of the node representing the
+Supplemental Configuration Unit (SCFG).
+
+Required properties:
+- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
+- interrupt-parent: phandle of GIC.
+- offset: offset to the Interrupt Polarity Control Register (INTPCR)
+ register in the SCFG.
+- interrupts: Specifies the mapping to interrupt numbers in the parent
+ interrupt controller. Interrupts are mapped one-to-one to parent
+ interrupts.
+
+Optional properties:
+- fsl,bit-reverse: This boolean property should be set on the LS1021A
+ if the SCFGREVCR register has been set to all-ones (which is usually
+ the case), meaning that all reads and writes of SCFG registers are
+ implicitly bit-reversed. Other compatible platforms do not have such
+ a register.
+
+Example:
+ scfg: scfg@1570000 {
+ compatible = "fsl,ls1021a-scfg", "syscon";
+ ...
+ extirq: interrupt-controller {
+ compatible = "fsl,ls1021a-extirq";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ interrupt-parent = <&gic>;
+ offset = <0x1ac>;
+ interrupts = <163 164 165 167 168 169>;
+ fsl,bit-reverse;
+ };
+ };
+
+
+ interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
+ <&extirq GIC_SPI 1 IRQ_TYPE_LEVEL_LOW>;
--
2.15.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v3 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-01-22 9:21 ` [PATCH v3 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2018-01-24 15:28 ` Marc Zyngier
0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2018-01-24 15:28 UTC (permalink / raw)
To: Rasmus Villemoes, Rob Herring, Shawn Guo, Thomas Gleixner,
Jason Cooper, Mark Rutland
Cc: Andy Tang, Alexander Stein, linux-kernel, devicetree
On 22/01/18 09:21, Rasmus Villemoes wrote:
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> .../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
Please write a sensible commit log.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v4 1/2] irqchip: add support for Layerscape external interrupt lines
2018-01-22 9:21 ` [PATCH v3 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
2018-01-22 9:21 ` [PATCH v3 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2018-01-25 15:02 ` Rasmus Villemoes
2018-01-25 15:02 ` [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
2018-02-23 21:08 ` [PATCH v5 0/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
1 sibling, 2 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2018-01-25 15:02 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes, linux-kernel
The LS1021A allows inverting the polarity of six interrupt lines
IRQ[0:5] via the scfg_intpcr register, effectively allowing
IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
check the type, set the relevant bit in INTPCR accordingly, and fixup
the type argument before calling the GIC's irq_set_type.
In fact, the power-on-reset value of the INTPCR register on the LS1021A
is so that all six lines have their polarity inverted. Hence any
hardware connected to those lines is unusable without this: If the line
is indeed active low, the generic GIC code will reject an irq spec with
IRQ_TYPE_LEVEL_LOW, while if the line is active high, we must obviously
disable the polarity inversion (writing 0 to the relevant bit) before
unmasking the interrupt.
Some other Layerscape SOCs (LS1043A, LS1046A) reportedly have a similar
feature, just with a different number of external interrupt lines (and a
different POR value for the INTPCR register). This driver should be
prepared for supporting those by properly filling out the device tree
node, but I don't have the full reference manual, nor the hardware to be
able to test it. I do know, from a tiny clipout from one of the other
reference manuals I was shown, that 1U<<n is the right formula to
use for setting/clearing the bit corresponding to the external IRQn, but
I don't know which interrupts on the GIC those lines represent.
There's also a little Kconfig/Kbuild issue: For now, I've let the driver
be built if CONFIG_SOC_LS1021A is set. For the others, it might make
sense to instead use CONFIG_ARCH_LAYERSCAPE, but SOC_LS1021A does not
select ARCH_LAYERSCAPE. The simplest solution is probably to do what is
done for irq-ls-scfg-msi.c: introduce a "def_bool y if SOC_LS1021A ||
ARCH_LAYERSCAPE" symbol. But I think that can wait until somebody with
the hardware actually tests that this works for the other platforms. At
that point, one can also add the extra IRQCHIP_DECLARE instances.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
Changes since v2 (all addressing comments from Rob Herring):
- use fsl,bit-reverse rather than bit-reverse
- make the dts node a child of the scfg node
- make the node name "interrupt-controller"
Changes since v3: None
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-ls-extirq.c | 173 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 174 insertions(+)
create mode 100644 drivers/irqchip/irq-ls-extirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b842dfdc903f..32d7160680fe 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_MVEBU_ICU) += irq-mvebu-icu.o
obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o
obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o
obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
+obj-$(CONFIG_SOC_LS1021A) += irq-ls-extirq.o
obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
new file mode 100644
index 000000000000..ac84ad053b51
--- /dev/null
+++ b/drivers/irqchip/irq-ls-extirq.c
@@ -0,0 +1,173 @@
+#define pr_fmt(fmt) "irq-ls-extirq: " fmt
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define MAXIRQ 12
+
+struct extirq_chip_data {
+ struct regmap *syscon;
+ u32 intpcr;
+ bool bit_reverse;
+ u32 nirq;
+ u32 parent_irq[MAXIRQ];
+};
+
+static int
+ls_extirq_set_type(struct irq_data *data, unsigned int type)
+{
+ irq_hw_number_t hwirq = data->hwirq;
+ struct extirq_chip_data *chip_data = data->chip_data;
+ u32 value, mask;
+
+ if (chip_data->bit_reverse)
+ mask = 1U << (31 - hwirq);
+ else
+ mask = 1U << hwirq;
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ value = mask;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ type = IRQ_TYPE_EDGE_RISING;
+ value = mask;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ case IRQ_TYPE_EDGE_RISING:
+ value = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(chip_data->syscon, chip_data->intpcr, mask, value);
+
+ data = data->parent_data;
+ return data->chip->irq_set_type(data, type);
+}
+
+static struct irq_chip extirq_chip = {
+ .name = "extirq",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = ls_extirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .flags = IRQCHIP_SET_TYPE_MASKED,
+};
+
+static int
+ls_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
+ unsigned long *hwirq, unsigned int *type)
+{
+ if (!is_of_node(fwspec->fwnode))
+ return -EINVAL;
+
+ if (fwspec->param_count != 3)
+ return -EINVAL;
+
+ /* No PPI should point to this domain */
+ if (fwspec->param[0] != GIC_SPI)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[1];
+ *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int
+ls_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ irq_hw_number_t hwirq;
+ struct irq_fwspec *fwspec = arg;
+ struct irq_fwspec gic_fwspec;
+ struct extirq_chip_data *chip_data = domain->host_data;
+
+ if (fwspec->param_count != 3)
+ return -EINVAL;
+
+ if (fwspec->param[0] != GIC_SPI)
+ return -EINVAL;
+
+ hwirq = fwspec->param[1];
+ if (hwirq >= chip_data->nirq)
+ return -EINVAL;
+
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &extirq_chip,
+ chip_data);
+
+ gic_fwspec.fwnode = domain->parent->fwnode;
+ gic_fwspec.param_count = 3;
+ gic_fwspec.param[0] = GIC_SPI;
+ gic_fwspec.param[1] = chip_data->parent_irq[hwirq];
+ gic_fwspec.param[2] = fwspec->param[2];
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &gic_fwspec);
+}
+
+static const struct irq_domain_ops extirq_domain_ops = {
+ .translate = ls_extirq_domain_translate,
+ .alloc = ls_extirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static int __init
+ls_extirq_of_init(struct device_node *node, struct device_node *parent)
+{
+
+ struct irq_domain *domain, *domain_parent;
+ struct extirq_chip_data *chip_data;
+ int ret;
+
+ domain_parent = irq_find_host(parent);
+ if (!domain_parent) {
+ pr_err("interrupt-parent not found\n");
+ return -EINVAL;
+ }
+
+ chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+ if (!chip_data)
+ return -ENOMEM;
+
+ chip_data->syscon = syscon_node_to_regmap(node->parent);
+ ret = of_property_read_u32(node, "offset", &chip_data->intpcr);
+
+ if (IS_ERR(chip_data->syscon))
+ ret = PTR_ERR(chip_data->syscon);
+ if (ret)
+ goto out_free_chip;
+
+ ret = of_property_read_variable_u32_array(node, "interrupts", chip_data->parent_irq,
+ 1, ARRAY_SIZE(chip_data->parent_irq));
+ if (ret < 0)
+ goto out_free_chip;
+ chip_data->nirq = ret;
+ chip_data->bit_reverse = of_property_read_bool(node, "fsl,bit-reverse");
+
+ domain = irq_domain_add_hierarchy(domain_parent, 0, chip_data->nirq, node,
+ &extirq_domain_ops, chip_data);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out_free_chip;
+ }
+
+ return 0;
+
+out_free_chip:
+ kfree(chip_data);
+ return ret;
+}
+
+IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls_extirq_of_init);
--
2.15.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-01-25 15:02 ` [PATCH v4 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
@ 2018-01-25 15:02 ` Rasmus Villemoes
2018-02-05 6:07 ` Rob Herring
2018-02-23 21:08 ` [PATCH v5 0/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
1 sibling, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2018-01-25 15:02 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Mark Rutland
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes, linux-kernel, devicetree
This adds Device Tree binding documentation for the external interrupt
lines with configurable polarity present on some Layerscape SOCs.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
Changes since v3: Add non-empty commit log.
.../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
new file mode 100644
index 000000000000..a71ce2c3eeae
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
@@ -0,0 +1,44 @@
+* Freescale Layerscape external IRQs
+
+Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
+the polarity of certain external interrupt lines.
+
+The device node must be a child of the node representing the
+Supplemental Configuration Unit (SCFG).
+
+Required properties:
+- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
+- interrupt-parent: phandle of GIC.
+- offset: offset to the Interrupt Polarity Control Register (INTPCR)
+ register in the SCFG.
+- interrupts: Specifies the mapping to interrupt numbers in the parent
+ interrupt controller. Interrupts are mapped one-to-one to parent
+ interrupts.
+
+Optional properties:
+- fsl,bit-reverse: This boolean property should be set on the LS1021A
+ if the SCFGREVCR register has been set to all-ones (which is usually
+ the case), meaning that all reads and writes of SCFG registers are
+ implicitly bit-reversed. Other compatible platforms do not have such
+ a register.
+
+Example:
+ scfg: scfg@1570000 {
+ compatible = "fsl,ls1021a-scfg", "syscon";
+ ...
+ extirq: interrupt-controller {
+ compatible = "fsl,ls1021a-extirq";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ interrupt-parent = <&gic>;
+ offset = <0x1ac>;
+ interrupts = <163 164 165 167 168 169>;
+ fsl,bit-reverse;
+ };
+ };
+
+
+ interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
+ <&extirq GIC_SPI 1 IRQ_TYPE_LEVEL_LOW>;
--
2.15.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-01-25 15:02 ` [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2018-02-05 6:07 ` Rob Herring
2018-02-08 15:08 ` Rasmus Villemoes
0 siblings, 1 reply; 39+ messages in thread
From: Rob Herring @ 2018-02-05 6:07 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Mark Rutland, Andy Tang, Alexander Stein, linux-kernel,
devicetree
On Thu, Jan 25, 2018 at 04:02:30PM +0100, Rasmus Villemoes wrote:
> This adds Device Tree binding documentation for the external interrupt
> lines with configurable polarity present on some Layerscape SOCs.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> Changes since v3: Add non-empty commit log.
>
> .../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
> new file mode 100644
> index 000000000000..a71ce2c3eeae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
> @@ -0,0 +1,44 @@
> +* Freescale Layerscape external IRQs
> +
> +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
> +the polarity of certain external interrupt lines.
> +
> +The device node must be a child of the node representing the
> +Supplemental Configuration Unit (SCFG).
> +
> +Required properties:
> +- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Use the same format as specified by GIC in arm,gic.txt.
> +- interrupt-parent: phandle of GIC.
> +- offset: offset to the Interrupt Polarity Control Register (INTPCR)
> + register in the SCFG.
> +- interrupts: Specifies the mapping to interrupt numbers in the parent
> + interrupt controller. Interrupts are mapped one-to-one to parent
> + interrupts.
> +
> +Optional properties:
> +- fsl,bit-reverse: This boolean property should be set on the LS1021A
> + if the SCFGREVCR register has been set to all-ones (which is usually
> + the case), meaning that all reads and writes of SCFG registers are
> + implicitly bit-reversed. Other compatible platforms do not have such
> + a register.
> +
> +Example:
> + scfg: scfg@1570000 {
> + compatible = "fsl,ls1021a-scfg", "syscon";
> + ...
> + extirq: interrupt-controller {
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + offset = <0x1ac>;
Use reg here instead (with a length).
> + interrupts = <163 164 165 167 168 169>;
These don't look like GIC interrupt cells. Building this with current
dtc will have errors.
> + fsl,bit-reverse;
> + };
> + };
> +
> +
> + interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
> + <&extirq GIC_SPI 1 IRQ_TYPE_LEVEL_LOW>;
> --
> 2.15.1
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-02-05 6:07 ` Rob Herring
@ 2018-02-08 15:08 ` Rasmus Villemoes
2018-02-09 9:47 ` Marc Zyngier
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2018-02-08 15:08 UTC (permalink / raw)
To: Rob Herring
Cc: Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Mark Rutland, Andy Tang, Alexander Stein, linux-kernel,
devicetree
On 2018-02-05 07:07, Rob Herring wrote:
>> +Example:
>> + scfg: scfg@1570000 {
>> + compatible = "fsl,ls1021a-scfg", "syscon";
>> + ...
>> + extirq: interrupt-controller {
>> + compatible = "fsl,ls1021a-extirq";
>> + #interrupt-cells = <3>;
>> + interrupt-controller;
>> + interrupt-parent = <&gic>;
>> + offset = <0x1ac>;
>
> Use reg here instead (with a length).
Hm, ok, but what does the length buy us? Should the driver just ignore
it, or should it check that it is 4 and bail out if not?
>> + interrupts = <163 164 165 167 168 169>;
>
> These don't look like GIC interrupt cells. Building this with current
> dtc will have errors.
Indeed, they are not. They simply record which interrupt lines on the
GIC the external interrupt lines IRQ0...IRQ5 map to (the arm64 socs
apparently have 12 such lines, but I don't know what they map to). I
originally had that mapping in the driver, but I was asked to move it to
DT. Is the problem the use of the name "interrupts" for this property?
I'm happy to use something else (parent-interrupts, interrupt-mapping,
...) I find it very hard to figure out which property names have
magic/reserved meanings.
I don't see any warnings/errors from dtc in the 4.14 tree I'm working
on. Does it require an even newer dtc than that?
Thanks,
Rasmus
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-02-08 15:08 ` Rasmus Villemoes
@ 2018-02-09 9:47 ` Marc Zyngier
0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2018-02-09 9:47 UTC (permalink / raw)
To: Rasmus Villemoes, Rob Herring
Cc: Shawn Guo, Thomas Gleixner, Jason Cooper, Mark Rutland,
Andy Tang, Alexander Stein, linux-kernel, devicetree
On 08/02/18 15:08, Rasmus Villemoes wrote:
> On 2018-02-05 07:07, Rob Herring wrote:
>>> +Example:
>>> + scfg: scfg@1570000 {
>>> + compatible = "fsl,ls1021a-scfg", "syscon";
>>> + ...
>>> + extirq: interrupt-controller {
>>> + compatible = "fsl,ls1021a-extirq";
>>> + #interrupt-cells = <3>;
>>> + interrupt-controller;
>>> + interrupt-parent = <&gic>;
>>> + offset = <0x1ac>;
>>
>> Use reg here instead (with a length).
>
> Hm, ok, but what does the length buy us? Should the driver just ignore
> it, or should it check that it is 4 and bail out if not?
>
>>> + interrupts = <163 164 165 167 168 169>;
>>
>> These don't look like GIC interrupt cells. Building this with current
>> dtc will have errors.
>
> Indeed, they are not. They simply record which interrupt lines on the
> GIC the external interrupt lines IRQ0...IRQ5 map to (the arm64 socs
> apparently have 12 such lines, but I don't know what they map to). I
> originally had that mapping in the driver, but I was asked to move it to
> DT. Is the problem the use of the name "interrupts" for this property?
> I'm happy to use something else (parent-interrupts, interrupt-mapping,
> ...) I find it very hard to figure out which property names have
> magic/reserved meanings.
>
> I don't see any warnings/errors from dtc in the 4.14 tree I'm working
> on. Does it require an even newer dtc than that?
Most interrupt controllers use a private property, potentially with a
range (see the recent example of the Qualcomm PDC [1]).
Thanks,
M.
[1] https://patchwork.kernel.org/patch/10208037/
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v5 0/2] irqchip: add support for Layerscape external interrupt lines
2018-01-25 15:02 ` [PATCH v4 1/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
2018-01-25 15:02 ` [PATCH v4 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2018-02-23 21:08 ` Rasmus Villemoes
2018-02-23 21:08 ` [PATCH v5 1/2] " Rasmus Villemoes
2018-02-23 21:09 ` [PATCH v5 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
1 sibling, 2 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2018-02-23 21:08 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, linux-kernel
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes
Changes since v4:
- add SPDX license identifier to .c file
- use 'reg' property to specify INTPCR register
- use 'fsl,extirq-map' as property name for the mapping of interrupt numbers
- rebase to v4.16-rc2
- change #interrupt-cells to 2 to avoid redundancy in interrupt specifiers
Changes since v3:
- Add non-empty commit log to 2/2
Changes since v2:
- use fsl,bit-reverse rather than bit-reverse
- make the dts node a child of the scfg node
- make the node name "interrupt-controller"
Rasmus Villemoes (2):
irqchip: add support for Layerscape external interrupt lines
dt/bindings: Add bindings for Layerscape external irqs
.../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-ls-extirq.c | 176 +++++++++++++++++++++
3 files changed, 221 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
create mode 100644 drivers/irqchip/irq-ls-extirq.c
--
2.15.1
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v5 1/2] irqchip: add support for Layerscape external interrupt lines
2018-02-23 21:08 ` [PATCH v5 0/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
@ 2018-02-23 21:08 ` Rasmus Villemoes
2018-03-01 12:16 ` Thomas Gleixner
2018-02-23 21:09 ` [PATCH v5 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
1 sibling, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2018-02-23 21:08 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes, linux-kernel
The LS1021A allows inverting the polarity of six interrupt lines
IRQ[0:5] via the scfg_intpcr register, effectively allowing
IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_EDGE_FALLING for those. We just need to
check the type, set the relevant bit in INTPCR accordingly, and fixup
the type argument before calling the GIC's irq_set_type.
In fact, the power-on-reset value of the INTPCR register on the LS1021A
is so that all six lines have their polarity inverted. Hence any
hardware connected to those lines is unusable without this: If the line
is indeed active low, the generic GIC code will reject an irq spec with
IRQ_TYPE_LEVEL_LOW, while if the line is active high, we must obviously
disable the polarity inversion (writing 0 to the relevant bit) before
unmasking the interrupt.
Some other Layerscape SOCs (LS1043A, LS1046A) reportedly have a similar
feature, just with a different number of external interrupt lines (and a
different POR value for the INTPCR register). This driver should be
prepared for supporting those by properly filling out the device tree
node, but I don't have the full reference manual, nor the hardware to be
able to test it. I do know, from a tiny clipout from one of the other
reference manuals I was shown, that 1U<<n is the right formula to
use for setting/clearing the bit corresponding to the external IRQn, but
I don't know which interrupts on the GIC those lines represent.
There's also a little Kconfig/Kbuild issue: For now, I've let the driver
be built if CONFIG_SOC_LS1021A is set. For the others, it might make
sense to instead use CONFIG_ARCH_LAYERSCAPE, but SOC_LS1021A does not
select ARCH_LAYERSCAPE. The simplest solution is probably to do what is
done for irq-ls-scfg-msi.c: introduce a "def_bool y if SOC_LS1021A ||
ARCH_LAYERSCAPE" symbol. But I think that can wait until somebody with
the hardware actually tests that this works for the other platforms. At
that point, one can also add the extra IRQCHIP_DECLARE instances.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-ls-extirq.c | 176 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 177 insertions(+)
create mode 100644 drivers/irqchip/irq-ls-extirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index d27e3e3619e0..6fe8612e6a49 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_MVEBU_ICU) += irq-mvebu-icu.o
obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o
obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o
obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
+obj-$(CONFIG_SOC_LS1021A) += irq-ls-extirq.o
obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o
obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
new file mode 100644
index 000000000000..e5665ad867d6
--- /dev/null
+++ b/drivers/irqchip/irq-ls-extirq.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) "irq-ls-extirq: " fmt
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define MAXIRQ 12
+
+struct extirq_chip_data {
+ struct regmap *syscon;
+ u32 intpcr;
+ bool bit_reverse;
+ u32 nirq;
+ u32 parent_irq[MAXIRQ];
+};
+
+static int
+ls_extirq_set_type(struct irq_data *data, unsigned int type)
+{
+ irq_hw_number_t hwirq = data->hwirq;
+ struct extirq_chip_data *chip_data = data->chip_data;
+ u32 value, mask;
+
+ if (chip_data->bit_reverse)
+ mask = 1U << (31 - hwirq);
+ else
+ mask = 1U << hwirq;
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ type = IRQ_TYPE_LEVEL_HIGH;
+ value = mask;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ type = IRQ_TYPE_EDGE_RISING;
+ value = mask;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ case IRQ_TYPE_EDGE_RISING:
+ value = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(chip_data->syscon, chip_data->intpcr, mask, value);
+
+ data = data->parent_data;
+ return data->chip->irq_set_type(data, type);
+}
+
+static struct irq_chip extirq_chip = {
+ .name = "extirq",
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_set_type = ls_extirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .flags = IRQCHIP_SET_TYPE_MASKED,
+};
+
+static int
+ls_extirq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
+ unsigned long *hwirq, unsigned int *type)
+{
+ if (!is_of_node(fwspec->fwnode))
+ return -EINVAL;
+
+ if (fwspec->param_count != 2)
+ return -EINVAL;
+
+ *hwirq = fwspec->param[0];
+ *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int
+ls_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ irq_hw_number_t hwirq;
+ struct irq_fwspec *fwspec = arg;
+ struct irq_fwspec gic_fwspec;
+ struct extirq_chip_data *chip_data = domain->host_data;
+
+ if (fwspec->param_count != 2)
+ return -EINVAL;
+
+ hwirq = fwspec->param[0];
+ if (hwirq >= chip_data->nirq)
+ return -EINVAL;
+
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &extirq_chip,
+ chip_data);
+
+ gic_fwspec.fwnode = domain->parent->fwnode;
+ gic_fwspec.param_count = 3;
+ gic_fwspec.param[0] = GIC_SPI;
+ gic_fwspec.param[1] = chip_data->parent_irq[hwirq];
+ gic_fwspec.param[2] = fwspec->param[1];
+
+ return irq_domain_alloc_irqs_parent(domain, virq, 1, &gic_fwspec);
+}
+
+static const struct irq_domain_ops extirq_domain_ops = {
+ .translate = ls_extirq_domain_translate,
+ .alloc = ls_extirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static int __init
+ls_extirq_of_init(struct device_node *node, struct device_node *parent)
+{
+
+ struct irq_domain *domain, *domain_parent;
+ struct extirq_chip_data *chip;
+ const __be32 *intpcr;
+ int ret;
+
+ domain_parent = irq_find_host(parent);
+ if (!domain_parent) {
+ pr_err("interrupt-parent not found\n");
+ return -EINVAL;
+ }
+
+ chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->syscon = syscon_node_to_regmap(node->parent);
+ if (IS_ERR(chip->syscon)) {
+ ret = PTR_ERR(chip->syscon);
+ pr_err("Failed to lookup parent regmap\n");
+ goto err;
+ }
+ intpcr = of_get_address(node, 0, NULL, NULL);
+ if (!intpcr) {
+ ret = -ENOENT;
+ pr_err("Missing INTPCR offset value\n");
+ goto err;
+ }
+ chip->intpcr = __be32_to_cpu(*intpcr);
+
+ ret = of_property_read_variable_u32_array(node, "fsl,extirq-map",
+ chip->parent_irq,
+ 1, ARRAY_SIZE(chip->parent_irq));
+ if (ret < 0)
+ goto err;
+ chip->nirq = ret;
+ chip->bit_reverse = of_property_read_bool(node, "fsl,bit-reverse");
+
+ domain = irq_domain_add_hierarchy(domain_parent, 0, chip->nirq, node,
+ &extirq_domain_ops, chip);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ return 0;
+
+err:
+ kfree(chip);
+ return ret;
+}
+
+IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls_extirq_of_init);
--
2.15.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v5 1/2] irqchip: add support for Layerscape external interrupt lines
2018-02-23 21:08 ` [PATCH v5 1/2] " Rasmus Villemoes
@ 2018-03-01 12:16 ` Thomas Gleixner
2018-05-04 7:44 ` Rasmus Villemoes
0 siblings, 1 reply; 39+ messages in thread
From: Thomas Gleixner @ 2018-03-01 12:16 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Rob Herring, Shawn Guo, Jason Cooper, Marc Zyngier, Andy Tang,
Alexander Stein, linux-kernel
On Fri, 23 Feb 2018, Rasmus Villemoes wrote:
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
of.h is already included from of_irq.h and of_address.h
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +static int
> +ls_extirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + irq_hw_number_t hwirq = data->hwirq;
> + struct extirq_chip_data *chip_data = data->chip_data;
> + u32 value, mask;
Please order local variables in reverse fir tree fashion whenever
possible. That's way simpler to read:
struct extirq_chip_data *chip_data = data->chip_data;
irq_hw_number_t hwirq = data->hwirq;
u32 value, mask;
> +
> + if (chip_data->bit_reverse)
> + mask = 1U << (31 - hwirq);
> + else
> + mask = 1U << hwirq;
> +
> + switch (type) {
> + case IRQ_TYPE_LEVEL_LOW:
> + type = IRQ_TYPE_LEVEL_HIGH;
> + value = mask;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + type = IRQ_TYPE_EDGE_RISING;
> + value = mask;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + case IRQ_TYPE_EDGE_RISING:
> + value = 0;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + regmap_update_bits(chip_data->syscon, chip_data->intpcr, mask, value);
> +
> + data = data->parent_data;
> + return data->chip->irq_set_type(data, type);
irq_chip_set_type_parent()
Thanks,
tglx
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v5 1/2] irqchip: add support for Layerscape external interrupt lines
2018-03-01 12:16 ` Thomas Gleixner
@ 2018-05-04 7:44 ` Rasmus Villemoes
2019-09-17 9:39 ` Kurt Kanzenbach
0 siblings, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2018-05-04 7:44 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Rob Herring, Shawn Guo, Jason Cooper, Marc Zyngier, Andy Tang,
Alexander Stein, linux-kernel
On 2018-03-01 13:16, Thomas Gleixner wrote:
> On Fri, 23 Feb 2018, Rasmus Villemoes wrote:
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>
> of.h is already included from of_irq.h and of_address.h
Yes. But if we're ever going to have a chance of reducing the median
header bloat factor from the current ~200x [1], one must stop relying on
headers pulling in other headers, so I much prefer including the headers
that declare the interfaces I use. And now that I look closer, I don't
actually use anything from of_irq.h, so that's gone. But I'll keep both
of.h and of_address.h.
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +static int
>> +ls_extirq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + irq_hw_number_t hwirq = data->hwirq;
>> + struct extirq_chip_data *chip_data = data->chip_data;
>> + u32 value, mask;
>
> Please order local variables in reverse fir tree fashion whenever
> possible. That's way simpler to read:
>
> struct extirq_chip_data *chip_data = data->chip_data;
> irq_hw_number_t hwirq = data->hwirq;
> u32 value, mask;
Fixed, thanks.
Rasmus
[1] https://wildmoose.dk/header-bloat/
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v5 1/2] irqchip: add support for Layerscape external interrupt lines
2018-05-04 7:44 ` Rasmus Villemoes
@ 2019-09-17 9:39 ` Kurt Kanzenbach
0 siblings, 0 replies; 39+ messages in thread
From: Kurt Kanzenbach @ 2019-09-17 9:39 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Thomas Gleixner, Rob Herring, Shawn Guo, Jason Cooper,
Marc Zyngier, Andy Tang, Alexander Stein, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 745 bytes --]
Hi,
On Fri, May 04, 2018 at 09:44:25AM +0200, Rasmus Villemoes wrote:
> >> +static int
> >> +ls_extirq_set_type(struct irq_data *data, unsigned int type)
> >> +{
> >> + irq_hw_number_t hwirq = data->hwirq;
> >> + struct extirq_chip_data *chip_data = data->chip_data;
> >> + u32 value, mask;
> >
> > Please order local variables in reverse fir tree fashion whenever
> > possible. That's way simpler to read:
> >
> > struct extirq_chip_data *chip_data = data->chip_data;
> > irq_hw_number_t hwirq = data->hwirq;
> > u32 value, mask;
>
> Fixed, thanks.
Did you send a sixth version of this patch set? It seems like the code
hasn't been merged, yet. I also need support for the external interrupt
lines on a different Layerscape.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v5 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-02-23 21:08 ` [PATCH v5 0/2] irqchip: add support for Layerscape external interrupt lines Rasmus Villemoes
2018-02-23 21:08 ` [PATCH v5 1/2] " Rasmus Villemoes
@ 2018-02-23 21:09 ` Rasmus Villemoes
2018-03-02 19:49 ` Rob Herring
1 sibling, 1 reply; 39+ messages in thread
From: Rasmus Villemoes @ 2018-02-23 21:09 UTC (permalink / raw)
To: Rob Herring, Shawn Guo, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Mark Rutland
Cc: Andy Tang, Alexander Stein, Rasmus Villemoes, linux-kernel, devicetree
This adds Device Tree binding documentation for the external interrupt
lines with configurable polarity present on some Layerscape SOCs.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
.../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
new file mode 100644
index 000000000000..e510c715e8f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
@@ -0,0 +1,44 @@
+* Freescale Layerscape external IRQs
+
+Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
+the polarity of certain external interrupt lines.
+
+The device node must be a child of the node representing the
+Supplemental Configuration Unit (SCFG).
+
+Required properties:
+- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells: Must be 2. The first element is the index of the
+ external interrupt line. The second element is the trigger type.
+- interrupt-parent: phandle of GIC.
+- reg: Specifies the Interrupt Polarity Control Register (INTPCR) in the SCFG.
+- fsl,extirq-map: Specifies the mapping to interrupt numbers in the parent
+ interrupt controller. Interrupts are mapped one-to-one to parent
+ interrupts.
+
+Optional properties:
+- fsl,bit-reverse: This boolean property should be set on the LS1021A
+ if the SCFGREVCR register has been set to all-ones (which is usually
+ the case), meaning that all reads and writes of SCFG registers are
+ implicitly bit-reversed. Other compatible platforms do not have such
+ a register.
+
+Example:
+ scfg: scfg@1570000 {
+ compatible = "fsl,ls1021a-scfg", "syscon";
+ ...
+ extirq: interrupt-controller {
+ compatible = "fsl,ls1021a-extirq";
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ interrupt-parent = <&gic>;
+ reg = <0x1ac>;
+ fsl,extirq-map = <163 164 165 167 168 169>;
+ fsl,bit-reverse;
+ };
+ };
+
+
+ interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
+ <&extirq 1 IRQ_TYPE_LEVEL_LOW>;
--
2.15.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v5 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-02-23 21:09 ` [PATCH v5 2/2] dt/bindings: Add bindings for Layerscape external irqs Rasmus Villemoes
@ 2018-03-02 19:49 ` Rob Herring
2018-05-04 8:07 ` Rasmus Villemoes
0 siblings, 1 reply; 39+ messages in thread
From: Rob Herring @ 2018-03-02 19:49 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Mark Rutland, Andy Tang, Alexander Stein, linux-kernel,
devicetree
On Fri, Feb 23, 2018 at 10:09:00PM +0100, Rasmus Villemoes wrote:
> This adds Device Tree binding documentation for the external interrupt
> lines with configurable polarity present on some Layerscape SOCs.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> .../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
> new file mode 100644
> index 000000000000..e510c715e8f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
> @@ -0,0 +1,44 @@
> +* Freescale Layerscape external IRQs
> +
> +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
> +the polarity of certain external interrupt lines.
> +
> +The device node must be a child of the node representing the
> +Supplemental Configuration Unit (SCFG).
> +
> +Required properties:
> +- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
> +- interrupt-controller: Identifies the node as an interrupt controller
> +- #interrupt-cells: Must be 2. The first element is the index of the
> + external interrupt line. The second element is the trigger type.
> +- interrupt-parent: phandle of GIC.
> +- reg: Specifies the Interrupt Polarity Control Register (INTPCR) in the SCFG.
> +- fsl,extirq-map: Specifies the mapping to interrupt numbers in the parent
> + interrupt controller. Interrupts are mapped one-to-one to parent
> + interrupts.
Use the interrupt-map property for this.
> +
> +Optional properties:
> +- fsl,bit-reverse: This boolean property should be set on the LS1021A
> + if the SCFGREVCR register has been set to all-ones (which is usually
> + the case), meaning that all reads and writes of SCFG registers are
> + implicitly bit-reversed. Other compatible platforms do not have such
> + a register.
> +
> +Example:
> + scfg: scfg@1570000 {
> + compatible = "fsl,ls1021a-scfg", "syscon";
> + ...
> + extirq: interrupt-controller {
> + compatible = "fsl,ls1021a-extirq";
> + #interrupt-cells = <2>;
> + interrupt-controller;
> + interrupt-parent = <&gic>;
> + reg = <0x1ac>;
This needs the length too. What is buys us is following the standard in
which mmio has a #size-cells >= 1. BTW, you need a #size-cells and
#address-cells properties in the parent. (I think dtc will complain if
not).
> + fsl,extirq-map = <163 164 165 167 168 169>;
> + fsl,bit-reverse;
> + };
> + };
> +
> +
> + interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
> + <&extirq 1 IRQ_TYPE_LEVEL_LOW>;
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v5 2/2] dt/bindings: Add bindings for Layerscape external irqs
2018-03-02 19:49 ` Rob Herring
@ 2018-05-04 8:07 ` Rasmus Villemoes
0 siblings, 0 replies; 39+ messages in thread
From: Rasmus Villemoes @ 2018-05-04 8:07 UTC (permalink / raw)
To: Rob Herring
Cc: Shawn Guo, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Mark Rutland, Andy Tang, Alexander Stein, linux-kernel,
devicetree
On 2018-03-02 20:49, Rob Herring wrote:
> On Fri, Feb 23, 2018 at 10:09:00PM +0100, Rasmus Villemoes wrote:
>> This adds Device Tree binding documentation for the external interrupt
>> lines with configurable polarity present on some Layerscape SOCs.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>> .../interrupt-controller/fsl,ls-extirq.txt | 44 ++++++++++++++++++++++
>> 1 file changed, 44 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>> new file mode 100644
>> index 000000000000..e510c715e8f6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>> @@ -0,0 +1,44 @@
>> +* Freescale Layerscape external IRQs
>> +
>> +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A) support inverting
>> +the polarity of certain external interrupt lines.
>> +
>> +The device node must be a child of the node representing the
>> +Supplemental Configuration Unit (SCFG).
>> +
>> +Required properties:
>> +- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
>> +- interrupt-controller: Identifies the node as an interrupt controller
>> +- #interrupt-cells: Must be 2. The first element is the index of the
>> + external interrupt line. The second element is the trigger type.
>> +- interrupt-parent: phandle of GIC.
>> +- reg: Specifies the Interrupt Polarity Control Register (INTPCR) in the SCFG.
>> +- fsl,extirq-map: Specifies the mapping to interrupt numbers in the parent
>> + interrupt controller. Interrupts are mapped one-to-one to parent
>> + interrupts.
>
> Use the interrupt-map property for this.
Please point me at some documentation for that. AFAICT, that would
require the property to consist of n*(3+2)*4 values, instead of just n
(with 3+2 being sum of #interrupt-cells and 4 being the four different
allowed incoming IRQ_TYPE_*). That seems quite excessive.
I used a private property based on advice from Marc
Most interrupt controllers use a private property, potentially with a
range (see the recent example of the Qualcomm PDC [1]).
[1] https://patchwork.kernel.org/patch/10208037/
and the qcom,pdc-ranges has this description (and that patch has your
Reviewed-by):
+- qcom,pdc-ranges:
+ Usage: required
+ Value type: <u32 array>
+ Definition: Specifies the PDC pin offset and the number of PDC
ports.
+ The tuples indicates the valid mapping of valid PDC
ports
+ and their hwirq mapping.
+ The first element of the tuple is the starting PDC port.
+ The second element is the GIC hwirq number for the
PDC port.
+ The third element is the number of interrupts in
sequence.
In my case, this is simplified by the external irq lines being numbered
consecutively from 0, so the array index itself serves as the "starting
pdc port". I also omit the "number of interrupts in sequence", and have
that be 1 implicitly, since it will only ever be either 6 or 12
elements. So I end up with a simple array of GIC hwirq numbers.
>> +
>> +Optional properties:
>> +- fsl,bit-reverse: This boolean property should be set on the LS1021A
>> + if the SCFGREVCR register has been set to all-ones (which is usually
>> + the case), meaning that all reads and writes of SCFG registers are
>> + implicitly bit-reversed. Other compatible platforms do not have such
>> + a register.
>> +
>> +Example:
>> + scfg: scfg@1570000 {
>> + compatible = "fsl,ls1021a-scfg", "syscon";
>> + ...
>> + extirq: interrupt-controller {
>> + compatible = "fsl,ls1021a-extirq";
>> + #interrupt-cells = <2>;
>> + interrupt-controller;
>> + interrupt-parent = <&gic>;
>> + reg = <0x1ac>;
>
> This needs the length too. What is buys us is following the standard in
> which mmio has a #size-cells >= 1. BTW, you need a #size-cells and
> #address-cells properties in the parent. (I think dtc will complain if
> not).
Well, the parent consists solely of 32 bit registers, so I think it
would make sense to have #size-cells = 0, to avoid redundant boilerplate
in subnodes' reg properties. But you're right, the ls1021a scfg node
currently has neither #size-cells or #address-cells, so I'll have to add
a patch adding those before adding this subnode. And if #size-cells=0 is
somehow frowned upon, I'll just make it 1.
--
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villemoes@prevas.dk
www.prevas.dk
^ permalink raw reply [flat|nested] 39+ messages in thread