* [PATCH v6 0/2] Add support for Layerscape external interrupt lines @ 2019-09-23 10:15 Kurt Kanzenbach 2019-09-23 10:15 ` [PATCH v6 1/2] irqchip: " Kurt Kanzenbach 2019-09-23 10:15 ` [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs Kurt Kanzenbach 0 siblings, 2 replies; 7+ messages in thread From: Kurt Kanzenbach @ 2019-09-23 10:15 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, Mark Rutland Cc: linux-kernel, Rasmus Villemoes, devicetree, Kurt Kanzenbach Hi, this is a respin of getting the support for the Layerscape's external interrupt lines. The last version from Rasmus Villemoes can be found here: https://lkml.kernel.org/r/20180223210901.23480-1-rasmus.villemoes@prevas.dk/ Rasmus Villemoes ran out of time, so I prepared v6. Changes since v5: - Rebase to v5.3.0 - Integrated Thomas Gleixner comments: - Adjust order of local variables - Use irq_chip_set_type_parent() - Cleanup of irq headers - Integrated Rob Herring comments: - Add #address-cells and #size-cells to parent - Use ARCH_LAYERSCAPE, as this feature is not LS1021A specific It is tested on a Layerscape LS2088A. Thanks, Kurt 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 | 47 +++++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ls-extirq.c | 174 ++++++++++++++++++ 3 files changed, 222 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt create mode 100644 drivers/irqchip/irq-ls-extirq.c -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v6 1/2] irqchip: Add support for Layerscape external interrupt lines 2019-09-23 10:15 [PATCH v6 0/2] Add support for Layerscape external interrupt lines Kurt Kanzenbach @ 2019-09-23 10:15 ` Kurt Kanzenbach 2019-09-23 10:15 ` [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs Kurt Kanzenbach 1 sibling, 0 replies; 7+ messages in thread From: Kurt Kanzenbach @ 2019-09-23 10:15 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, Mark Rutland Cc: linux-kernel, Rasmus Villemoes, devicetree, Kurt Kanzenbach From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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, LS2088A) 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. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- Changes since v5: - Adjust order of local variables - Cleanup of irq headers - Use irq_chip_set_type_parent() - Compile with ARCH_LAYERSCAPE - Removed last paragraph of changelog, as ARCH_LAYERSCAPE should be used - Also mention the LS2088A SoC in changelog drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-ls-extirq.c | 174 ++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 drivers/irqchip/irq-ls-extirq.c diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 8d0fcec6ab23..ce6db511124a 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_MVEBU_ODMI) += irq-mvebu-odmi.o obj-$(CONFIG_MVEBU_PIC) += irq-mvebu-pic.o obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.o obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o +obj-$(CONFIG_ARCH_LAYERSCAPE) += 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..c1d1bfee585c --- /dev/null +++ b/drivers/irqchip/irq-ls-extirq.c @@ -0,0 +1,174 @@ +// 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_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) +{ + 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); + + return irq_chip_set_type_parent(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.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs 2019-09-23 10:15 [PATCH v6 0/2] Add support for Layerscape external interrupt lines Kurt Kanzenbach 2019-09-23 10:15 ` [PATCH v6 1/2] irqchip: " Kurt Kanzenbach @ 2019-09-23 10:15 ` Kurt Kanzenbach 2019-09-27 16:11 ` Rob Herring 1 sibling, 1 reply; 7+ messages in thread From: Kurt Kanzenbach @ 2019-09-23 10:15 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, Mark Rutland Cc: linux-kernel, Rasmus Villemoes, devicetree, Kurt Kanzenbach From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> 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> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- Changes since v5: - Add #address-cells and #size-cells to parent - Mention LS2088A and the ISC unit .../interrupt-controller/fsl,ls-extirq.txt | 47 +++++++++++++++++++ 1 file changed, 47 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..7b53f9cc8019 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt @@ -0,0 +1,47 @@ +* Freescale Layerscape external IRQs + +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A, LS2088A) 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) or the Interrupt Sampling +Control (ISC) Unit. + +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"; + #address-cells = <1>; + #size-cells = <0>; + ... + 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.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs 2019-09-23 10:15 ` [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs Kurt Kanzenbach @ 2019-09-27 16:11 ` Rob Herring 2019-09-27 21:16 ` Rasmus Villemoes 0 siblings, 1 reply; 7+ messages in thread From: Rob Herring @ 2019-09-27 16:11 UTC (permalink / raw) To: Kurt Kanzenbach Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland, linux-kernel, Rasmus Villemoes, devicetree On Mon, Sep 23, 2019 at 12:15:13PM +0200, Kurt Kanzenbach wrote: > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > > 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> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > > Changes since v5: > > - Add #address-cells and #size-cells to parent > - Mention LS2088A and the ISC unit Repeating some of my lost comments from v2 2 years ago... > > .../interrupt-controller/fsl,ls-extirq.txt | 47 +++++++++++++++++++ > 1 file changed, 47 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..7b53f9cc8019 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt > @@ -0,0 +1,47 @@ > +* Freescale Layerscape external IRQs > + > +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A, LS2088A) 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) or the Interrupt Sampling > +Control (ISC) Unit. > + > +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. This should be an 'interrupt-map' instead. > + > +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. Couldn't you just read that register and tell? Does this apply to only the extirq register or all of scfg? > + > +Example: > + scfg: scfg@1570000 { > + compatible = "fsl,ls1021a-scfg", "syscon"; > + #address-cells = <1>; > + #size-cells = <0>; As the child node(s) are memory mapped, this should not be 0. And you need 'ranges'. > + ... > + 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.20.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs 2019-09-27 16:11 ` Rob Herring @ 2019-09-27 21:16 ` Rasmus Villemoes 2019-09-28 9:23 ` Kurt Kanzenbach 2019-09-30 12:22 ` Rob Herring 0 siblings, 2 replies; 7+ messages in thread From: Rasmus Villemoes @ 2019-09-27 21:16 UTC (permalink / raw) To: Rob Herring, Kurt Kanzenbach Cc: Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland, linux-kernel, devicetree On 27/09/2019 18.11, Rob Herring wrote: > On Mon, Sep 23, 2019 at 12:15:13PM +0200, Kurt Kanzenbach wrote: >> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> >> >> 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> >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> >> --- >> >> +* Freescale Layerscape external IRQs >> + >> +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A, LS2088A) 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) or the Interrupt Sampling >> +Control (ISC) Unit. >> + >> +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. > > This should be an 'interrupt-map' instead. Rob, thanks for your review comments. I know you said the same thing at v5, and it might seem like they are ignored. However, I asked a couple of followup questions (https://lore.kernel.org/lkml/0bb4533d-c749-d8ff-e1f2-4b08eb724713@prevas.dk/). I'd really appreciate it if you could (a) point to some documentation on how to write that interrupt-map and (b) explain how this is different from the Qualcomm PDC case I tried to copy and which had your Reviewed-By. >> + >> +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. > > Couldn't you just read that register and tell? In theory, yes, but as far as I understand (and as I wrote) it's specific to the ls1021a. Of course one can decide whether it's necessary/possible to read it based on the compatible string, but one would also need an extra reg property to have its address - but that register is not really part of the extirq "device" we're trying to describe. So would it need to be represented as its own subnode of scfg? If it is set at all, it's done within the first few instructions after reset (before control is even handed to the bootloader), so I see it as a kind of quirk of the hardware. The data sheet says "SCFG bit reverse (SCFG_SCFGREVCR) must be written 0xFFFF_FFFF as a part of initialization sequence before writing to any other SCFG register." which, taken literally, means we don't need the property at all and can just assume it for the ls1021a (i.e., do it based on compatible string alone) - but I think it should be read as "if you're going to write this register, it must be done first thing". > Does this apply to only the extirq register or all of scfg? All of scfg. It really seems like some accident/bad joke coming out of a discussion between a hardware and software engineer on the enumeration of bits, with the hardware guy ending up saying "alright, have it whichever way you want it", causing even more pain :( >> + >> +Example: >> + scfg: scfg@1570000 { >> + compatible = "fsl,ls1021a-scfg", "syscon"; >> + #address-cells = <1>; >> + #size-cells = <0>; > > As the child node(s) are memory mapped, this should not be 0. And you > need 'ranges'. Indeed - I think I understand this a little better now than I did back then. Thanks, Rasmus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs 2019-09-27 21:16 ` Rasmus Villemoes @ 2019-09-28 9:23 ` Kurt Kanzenbach 2019-09-30 12:22 ` Rob Herring 1 sibling, 0 replies; 7+ messages in thread From: Kurt Kanzenbach @ 2019-09-28 9:23 UTC (permalink / raw) To: Rasmus Villemoes Cc: Rob Herring, Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland, linux-kernel, devicetree [-- Attachment #1: Type: text/plain, Size: 4174 bytes --] On Fri, Sep 27, 2019 at 09:16:50PM +0000, Rasmus Villemoes wrote: > On 27/09/2019 18.11, Rob Herring wrote: > > On Mon, Sep 23, 2019 at 12:15:13PM +0200, Kurt Kanzenbach wrote: > >> +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. > > > > This should be an 'interrupt-map' instead. > > Rob, thanks for your review comments. I know you said the same thing at > v5, and it might seem like they are ignored. Well, I didn't ignore them. It just wasn't clear to me from the previous discussions which way you want to go. > However, I asked a couple of followup questions > (https://lore.kernel.org/lkml/0bb4533d-c749-d8ff-e1f2-4b08eb724713@prevas.dk/). > I'd really appreciate it if you could (a) point to some documentation > on how to write that interrupt-map and (b) explain how this is > different from the Qualcomm PDC case I tried to copy and which had > your Reviewed-By. I guess, we can have a look at other interrupt controllers and how they handle the interrupt-map property. For example: https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt I need to send a v7 anyway, because I forgot to include the SOC_LS1021A in the build process. > > >> + > >> +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. > > > > Couldn't you just read that register and tell? > > In theory, yes, but as far as I understand (and as I wrote) it's > specific to the ls1021a. Of course one can decide whether it's > necessary/possible to read it based on the compatible string, but one > would also need an extra reg property to have its address - but that > register is not really part of the extirq "device" we're trying to > describe. So would it need to be represented as its own subnode of scfg? Keep in mind, that not all Layerscapes have that feature and the corresponding register. As you said that may be handled via compatible string. However, the bit-reverse property looks like the simplest solution to me. > > If it is set at all, it's done within the first few instructions after > reset (before control is even handed to the bootloader), so I see it as > a kind of quirk of the hardware. The data sheet says "SCFG bit reverse > (SCFG_SCFGREVCR) must be written 0xFFFF_FFFF as a part of initialization > sequence before writing to any other SCFG register." which, taken > literally, means we don't need the property at all and can just assume > it for the ls1021a (i.e., do it based on compatible string alone) - but > I think it should be read as "if you're going to write this register, it > must be done first thing". > > > Does this apply to only the extirq register or all of scfg? > > All of scfg. It really seems like some accident/bad joke coming out of a > discussion between a hardware and software engineer on the enumeration > of bits, with the hardware guy ending up saying "alright, have it > whichever way you want it", causing even more pain :( > > >> + > >> +Example: > >> + scfg: scfg@1570000 { > >> + compatible = "fsl,ls1021a-scfg", "syscon"; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > > > > As the child node(s) are memory mapped, this should not be 0. And you > > need 'ranges'. > > Indeed - I think I understand this a little better now than I did back then. > Okay. > Thanks, > Rasmus Thanks, Kurt [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs 2019-09-27 21:16 ` Rasmus Villemoes 2019-09-28 9:23 ` Kurt Kanzenbach @ 2019-09-30 12:22 ` Rob Herring 1 sibling, 0 replies; 7+ messages in thread From: Rob Herring @ 2019-09-30 12:22 UTC (permalink / raw) To: Rasmus Villemoes Cc: Kurt Kanzenbach, Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Rutland, linux-kernel, devicetree On Fri, Sep 27, 2019 at 4:16 PM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 27/09/2019 18.11, Rob Herring wrote: > > On Mon, Sep 23, 2019 at 12:15:13PM +0200, Kurt Kanzenbach wrote: > >> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > >> > >> 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> > >> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > >> --- > >> > >> +* Freescale Layerscape external IRQs > >> + > >> +Some Layerscape SOCs (LS1021A, LS1043A, LS1046A, LS2088A) 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) or the Interrupt Sampling > >> +Control (ISC) Unit. > >> + > >> +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. > > > > This should be an 'interrupt-map' instead. > > Rob, thanks for your review comments. I know you said the same thing at > v5, and it might seem like they are ignored. However, I asked a couple > of followup questions > (https://lore.kernel.org/lkml/0bb4533d-c749-d8ff-e1f2-4b08eb724713@prevas.dk/). > I'd really appreciate it if you could (a) point to some documentation on > how to write that interrupt-map and (b) explain how this is different > from the Qualcomm PDC case I tried to copy and which had your Reviewed-By. https://www.devicetree.org/specifications/ https://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping For QC PDC, it could be a large number of interrupts to remap which interrupt-map doesn't scale to very well. Also, I think it sits in parallel to the GIC rather than downstream. The interrupt binding doesn't really have a way to express that. > >> +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. > > > > Couldn't you just read that register and tell? > > In theory, yes, but as far as I understand (and as I wrote) it's > specific to the ls1021a. Of course one can decide whether it's > necessary/possible to read it based on the compatible string, but one > would also need an extra reg property to have its address - but that > register is not really part of the extirq "device" we're trying to > describe. So would it need to be represented as its own subnode of scfg? Why? You should know where the register is because you know what chip you are on (from the compatible). > > If it is set at all, it's done within the first few instructions after > reset (before control is even handed to the bootloader), so I see it as > a kind of quirk of the hardware. The data sheet says "SCFG bit reverse > (SCFG_SCFGREVCR) must be written 0xFFFF_FFFF as a part of initialization > sequence before writing to any other SCFG register." which, taken > literally, means we don't need the property at all and can just assume > it for the ls1021a (i.e., do it based on compatible string alone) - but > I think it should be read as "if you're going to write this register, it > must be done first thing". > > > Does this apply to only the extirq register or all of scfg? > > All of scfg. It really seems like some accident/bad joke coming out of a > discussion between a hardware and software engineer on the enumeration > of bits, with the hardware guy ending up saying "alright, have it > whichever way you want it", causing even more pain :( If all of the scfg bits change, then if you have this property, it should be in the scfg node. But I prefer you use the compatible instead if that works. Rob ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-30 12:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-23 10:15 [PATCH v6 0/2] Add support for Layerscape external interrupt lines Kurt Kanzenbach 2019-09-23 10:15 ` [PATCH v6 1/2] irqchip: " Kurt Kanzenbach 2019-09-23 10:15 ` [PATCH v6 2/2] dt/bindings: Add bindings for Layerscape external irqs Kurt Kanzenbach 2019-09-27 16:11 ` Rob Herring 2019-09-27 21:16 ` Rasmus Villemoes 2019-09-28 9:23 ` Kurt Kanzenbach 2019-09-30 12:22 ` Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.