| Message ID | 20250302-04-gpio-irq-threecell-v2-1-34f13ad37ea4@gentoo.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: irq: support describing three-cell interrupts | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On Sun, Mar 02 2025 at 07:15, Yixun Lan wrote: > The is a prerequisite patch to support parsing three-cell > interrupts which encoded as <instance hwirq irqflag>, > the translate function will always retrieve irq number and > flag from last two cells. > > In this patch, we introduce a generic interrupt cells translation > function, others functions will be inline version. Please read: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes > +int irq_domain_translate_cells(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *out_hwirq, > + unsigned int *out_type); Please get rid of the extra line breaks. You have 100 (99) characters available. > +static inline int irq_domain_translate_onecell(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > +} > + > +static inline int irq_domain_translate_twocell(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > +} > + > +static inline int irq_domain_translate_threecell(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > +} What's this for? It's not used. The onecell/twocell wrappers are just there to keep the current code working. > +int irq_domain_translate_cells(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *out_hwirq, > + unsigned int *out_type) Please remove the extra line breaks. int irq_domain_translate_cells(struct irq_domain *d, struct irq_fwspec *fwspec, unsigned long *out_hwirq, unsigned int *out_type) is perfectly fine. > { > - if (WARN_ON(fwspec->param_count < 1)) > - return -EINVAL; > - *out_hwirq = fwspec->param[0]; > - *out_type = IRQ_TYPE_NONE; > - return 0; > -} > -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell); > + unsigned int cells = fwspec->param_count; > > -/** > - * irq_domain_translate_twocell() - Generic translate for direct two cell > - * bindings > - * @d: Interrupt domain involved in the translation > - * @fwspec: The firmware interrupt specifier to translate > - * @out_hwirq: Pointer to storage for the hardware interrupt number > - * @out_type: Pointer to storage for the interrupt type > - * > - * Device Tree IRQ specifier translation function which works with two cell > - * bindings where the cell values map directly to the hwirq number > - * and linux irq flags. > - */ > -int irq_domain_translate_twocell(struct irq_domain *d, > - struct irq_fwspec *fwspec, > - unsigned long *out_hwirq, > - unsigned int *out_type) > -{ > - if (WARN_ON(fwspec->param_count < 2)) > + switch (cells) { > + case 1: > + *out_hwirq = fwspec->param[0]; > + *out_type = IRQ_TYPE_NONE; > + return 0; > + case 2 ... 3: I have second thoughts about this when looking deeper. The current one/two cell implementations validate that param_count is at least the number of parameters. Which means that the parameter count could be larger, but only evaluates the first one or the first two. I have no idea whether this matters or not, but arguably a two cell fwspec could be successfully fed into translate_onecell(), no? And that triggers a related question. Why is the three cell translation not following the one/two cell scheme and has the parameters at the same place (index 0,1), i.e. adding the extra information at the end? That makes sense to me as the extra cell is obviously not directly related to the interrupt mapping. Thanks, tglx
Hello, Thomas Gleixner: On 19:30 Sun 02 Mar , Thomas Gleixner wrote: > On Sun, Mar 02 2025 at 07:15, Yixun Lan wrote: > > The is a prerequisite patch to support parsing three-cell > > interrupts which encoded as <instance hwirq irqflag>, > > the translate function will always retrieve irq number and > > flag from last two cells. > > > > In this patch, we introduce a generic interrupt cells translation > > function, others functions will be inline version. > > Please read: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes > > > +int irq_domain_translate_cells(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type); > > Please get rid of the extra line breaks. You have 100 (99) characters available. > > > +static inline int irq_domain_translate_onecell(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > > +} > > + > > +static inline int irq_domain_translate_twocell(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > > +} > > + > > +static inline int irq_domain_translate_threecell(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > > +} > > What's this for? It's not used. The onecell/twocell wrappers are just > there to keep the current code working. > > > +int irq_domain_translate_cells(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > Please remove the extra line breaks. > > int irq_domain_translate_cells(struct irq_domain *d, struct irq_fwspec *fwspec, > unsigned long *out_hwirq, unsigned int *out_type) > > is perfectly fine. > > > { > > - if (WARN_ON(fwspec->param_count < 1)) > > - return -EINVAL; > > - *out_hwirq = fwspec->param[0]; > > - *out_type = IRQ_TYPE_NONE; > > - return 0; > > -} > > -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell); > > + unsigned int cells = fwspec->param_count; > > > > -/** > > - * irq_domain_translate_twocell() - Generic translate for direct two cell > > - * bindings > > - * @d: Interrupt domain involved in the translation > > - * @fwspec: The firmware interrupt specifier to translate > > - * @out_hwirq: Pointer to storage for the hardware interrupt number > > - * @out_type: Pointer to storage for the interrupt type > > - * > > - * Device Tree IRQ specifier translation function which works with two cell > > - * bindings where the cell values map directly to the hwirq number > > - * and linux irq flags. > > - */ > > -int irq_domain_translate_twocell(struct irq_domain *d, > > - struct irq_fwspec *fwspec, > > - unsigned long *out_hwirq, > > - unsigned int *out_type) > > -{ > > - if (WARN_ON(fwspec->param_count < 2)) > > + switch (cells) { > > + case 1: > > + *out_hwirq = fwspec->param[0]; > > + *out_type = IRQ_TYPE_NONE; > > + return 0; > > + case 2 ... 3: > > I have second thoughts about this when looking deeper. > > The current one/two cell implementations validate that param_count is at > least the number of parameters. Which means that the parameter count > could be larger, but only evaluates the first one or the first two. > > I have no idea whether this matters or not, but arguably a two cell > fwspec could be successfully fed into translate_onecell(), no? > > And that triggers a related question. > > Why is the three cell translation not following the one/two cell scheme > and has the parameters at the same place (index 0,1), i.e. adding the > extra information at the end? That makes sense to me as the extra cell > is obviously not directly related to the interrupt mapping. this from gpio DT gpios = <&gpio instance offset flags>; I think we currently just following the scheme with gpio cells order scheme, which is (index(instance) offset flag..), the index and offset are parameters to locate the irq which can easily derive from global gpio pin number, so I thought it's more intuitive to group them orderly together.. But you really raise a good suggestion here, if we follow appending extra information at the end, then it will make threecell translate scheme compatible with twocell, which mean we don't really need extra function to prase, and can eventually drop this patch, I personally like this direction. hi, Linus Walleij, what do you think on this?
On Mon, Mar 3, 2025 at 1:40 PM Yixun Lan <dlan@gentoo.org> wrote: > On 19:30 Sun 02 Mar , Thomas Gleixner wrote: > > Why is the three cell translation not following the one/two cell scheme > > and has the parameters at the same place (index 0,1), i.e. adding the > > extra information at the end? That makes sense to me as the extra cell > > is obviously not directly related to the interrupt mapping. > > I think we currently just following the scheme with gpio cells order > scheme, which is (index(instance) offset flag..), the index and offset > are parameters to locate the irq which can easily derive from global > gpio pin number, so I thought it's more intuitive to group them > orderly together.. Right, the DT bindings are mainly for human consumption, and the cells are positioned in left-to-right intuitive order. If they were only for machines it would be another issue, but it's people who have to write and maintain these files. For example, in a library a machine could arrange books by first letter in the title, then by second letter in the title etc, but that would be very confusing for humans who expect to find them in author order. There are many examples of this in the DT bindings. Yours, Linus Walleij
Hi Linus Walleij: On 08:31 Tue 04 Mar , Linus Walleij wrote: > On Mon, Mar 3, 2025 at 1:40 PM Yixun Lan <dlan@gentoo.org> wrote: > > On 19:30 Sun 02 Mar , Thomas Gleixner wrote: > > > > Why is the three cell translation not following the one/two cell scheme > > > and has the parameters at the same place (index 0,1), i.e. adding the > > > extra information at the end? That makes sense to me as the extra cell > > > is obviously not directly related to the interrupt mapping. > > > > I think we currently just following the scheme with gpio cells order > > scheme, which is (index(instance) offset flag..), the index and offset > > are parameters to locate the irq which can easily derive from global > > gpio pin number, so I thought it's more intuitive to group them > > orderly together.. > > Right, the DT bindings are mainly for human consumption, and the > cells are positioned in left-to-right intuitive order. > > If they were only for machines it would be another issue, but it's > people who have to write and maintain these files. > > For example, in a library a machine could arrange books by > first letter in the title, then by second letter in the title etc, but > that would be very confusing for humans who expect to find > them in author order. > > There are many examples of this in the DT bindings. > Ok, I got your idea.. thanks I will rework the patch to address Thomas's concern > Yours, > Linus Walleij >
Hi Thomas: On 19:30 Sun 02 Mar , Thomas Gleixner wrote: > On Sun, Mar 02 2025 at 07:15, Yixun Lan wrote: > > The is a prerequisite patch to support parsing three-cell > > interrupts which encoded as <instance hwirq irqflag>, > > the translate function will always retrieve irq number and > > flag from last two cells. > > > > In this patch, we introduce a generic interrupt cells translation > > function, others functions will be inline version. > > Please read: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes > > > +int irq_domain_translate_cells(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type); > > Please get rid of the extra line breaks. You have 100 (99) characters available. > > > +static inline int irq_domain_translate_onecell(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > > +} > > + > > +static inline int irq_domain_translate_twocell(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > > +} > > + > > +static inline int irq_domain_translate_threecell(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > +{ > > + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); > > +} > > What's this for? It's not used. The onecell/twocell wrappers are just > there to keep the current code working. > > > +int irq_domain_translate_cells(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *out_hwirq, > > + unsigned int *out_type) > > Please remove the extra line breaks. > > int irq_domain_translate_cells(struct irq_domain *d, struct irq_fwspec *fwspec, > unsigned long *out_hwirq, unsigned int *out_type) > > is perfectly fine. > > > { > > - if (WARN_ON(fwspec->param_count < 1)) > > - return -EINVAL; > > - *out_hwirq = fwspec->param[0]; > > - *out_type = IRQ_TYPE_NONE; > > - return 0; > > -} > > -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell); > > + unsigned int cells = fwspec->param_count; > > > > -/** > > - * irq_domain_translate_twocell() - Generic translate for direct two cell > > - * bindings > > - * @d: Interrupt domain involved in the translation > > - * @fwspec: The firmware interrupt specifier to translate > > - * @out_hwirq: Pointer to storage for the hardware interrupt number > > - * @out_type: Pointer to storage for the interrupt type > > - * > > - * Device Tree IRQ specifier translation function which works with two cell > > - * bindings where the cell values map directly to the hwirq number > > - * and linux irq flags. > > - */ > > -int irq_domain_translate_twocell(struct irq_domain *d, > > - struct irq_fwspec *fwspec, > > - unsigned long *out_hwirq, > > - unsigned int *out_type) > > -{ > > - if (WARN_ON(fwspec->param_count < 2)) > > + switch (cells) { > > + case 1: > > + *out_hwirq = fwspec->param[0]; > > + *out_type = IRQ_TYPE_NONE; > > + return 0; > > + case 2 ... 3: > > I have second thoughts about this when looking deeper. > > The current one/two cell implementations validate that param_count is at > least the number of parameters. Which means that the parameter count > could be larger, but only evaluates the first one or the first two. > > I have no idea whether this matters or not, but arguably a two cell > fwspec could be successfully fed into translate_onecell(), no? > I think this isn't a problem, the function translate_onecell() or twocell() will be called explicitly, so they will parse cells with only wanted number but ignore additional one when come to this, I do think there is problem in my previous patch that implicitly extend the translate_twocell() to parse three cells, I will try to fix this in next version by introducing a threecell() or translate_twothreecell() function for corresponding cases. > And that triggers a related question. > > Why is the three cell translation not following the one/two cell scheme > and has the parameters at the same place (index 0,1), i.e. adding the > extra information at the end? That makes sense to me as the extra cell > is obviously not directly related to the interrupt mapping. > > Thanks, > > tglx
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index e432b6a12a32f9f16ec1ea2fa8e24a649d55caae..d96796263a2e177140f928cb369656a44dd45dda 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -572,15 +572,34 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr, const u32 *intspec, unsigned int intsize, irq_hw_number_t *out_hwirq, unsigned int *out_type); -int irq_domain_translate_twocell(struct irq_domain *d, - struct irq_fwspec *fwspec, - unsigned long *out_hwirq, - unsigned int *out_type); - -int irq_domain_translate_onecell(struct irq_domain *d, - struct irq_fwspec *fwspec, - unsigned long *out_hwirq, - unsigned int *out_type); +int irq_domain_translate_cells(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *out_hwirq, + unsigned int *out_type); + +static inline int irq_domain_translate_onecell(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *out_hwirq, + unsigned int *out_type) +{ + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); +} + +static inline int irq_domain_translate_twocell(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *out_hwirq, + unsigned int *out_type) +{ + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); +} + +static inline int irq_domain_translate_threecell(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *out_hwirq, + unsigned int *out_type) +{ + return irq_domain_translate_cells(d, fwspec, out_hwirq, out_type); +} /* IPI functions */ int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask *dest); diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index ec6d8e72d980f604ded2bfa2143420e0e0095920..8d3b357b7dedbb2c274d4761c315e430b1d35610 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -1171,50 +1171,38 @@ const struct irq_domain_ops irq_domain_simple_ops = { EXPORT_SYMBOL_GPL(irq_domain_simple_ops); /** - * irq_domain_translate_onecell() - Generic translate for direct one cell + * irq_domain_translate_cells() - Generic translate for up to three cells * bindings * @d: Interrupt domain involved in the translation * @fwspec: The firmware interrupt specifier to translate * @out_hwirq: Pointer to storage for the hardware interrupt number * @out_type: Pointer to storage for the interrupt type */ -int irq_domain_translate_onecell(struct irq_domain *d, - struct irq_fwspec *fwspec, - unsigned long *out_hwirq, - unsigned int *out_type) +int irq_domain_translate_cells(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *out_hwirq, + unsigned int *out_type) { - if (WARN_ON(fwspec->param_count < 1)) - return -EINVAL; - *out_hwirq = fwspec->param[0]; - *out_type = IRQ_TYPE_NONE; - return 0; -} -EXPORT_SYMBOL_GPL(irq_domain_translate_onecell); + unsigned int cells = fwspec->param_count; -/** - * irq_domain_translate_twocell() - Generic translate for direct two cell - * bindings - * @d: Interrupt domain involved in the translation - * @fwspec: The firmware interrupt specifier to translate - * @out_hwirq: Pointer to storage for the hardware interrupt number - * @out_type: Pointer to storage for the interrupt type - * - * Device Tree IRQ specifier translation function which works with two cell - * bindings where the cell values map directly to the hwirq number - * and linux irq flags. - */ -int irq_domain_translate_twocell(struct irq_domain *d, - struct irq_fwspec *fwspec, - unsigned long *out_hwirq, - unsigned int *out_type) -{ - if (WARN_ON(fwspec->param_count < 2)) + switch (cells) { + case 1: + *out_hwirq = fwspec->param[0]; + *out_type = IRQ_TYPE_NONE; + return 0; + case 2 ... 3: + /* + * For multi cell translations the hardware interrupt number + * and type are in the last two cells. + */ + *out_hwirq = fwspec->param[cells - 2]; + *out_type = fwspec->param[cells - 1] & IRQ_TYPE_SENSE_MASK; + return 0; + default: return -EINVAL; - *out_hwirq = fwspec->param[0]; - *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; - return 0; + } } -EXPORT_SYMBOL_GPL(irq_domain_translate_twocell); +EXPORT_SYMBOL_GPL(irq_domain_translate_cells); int irq_domain_alloc_descs(int virq, unsigned int cnt, irq_hw_number_t hwirq, int node, const struct irq_affinity_desc *affinity)
The is a prerequisite patch to support parsing three-cell interrupts which encoded as <instance hwirq irqflag>, the translate function will always retrieve irq number and flag from last two cells. In this patch, we introduce a generic interrupt cells translation function, others functions will be inline version. Signed-off-by: Yixun Lan <dlan@gentoo.org> --- include/linux/irqdomain.h | 37 +++++++++++++++++++++++-------- kernel/irq/irqdomain.c | 56 +++++++++++++++++++---------------------------- 2 files changed, 50 insertions(+), 43 deletions(-)