irqchip: qcom: add support for PDC interrupt controller
Subsystems (4)
v2 cover irqchip: qcom: add support for PDC interrupt controller
Changes in v2:
- Drviers will specify PDC pin as their interrupt and PDC as interrupt parent
- Updated driver to pick up PDC pin mappings from DT
- Cleanup driver per Marc's suggestions
- Addressed DT bindings comments from Rob H
- Addressed FTRACE comments from Steven R
On newer Qualcomm Techonologies Inc's SoCs like the SDM845, the GIC is in a power domain that can be powered off when not needed. Interrupts that need to be sensed even when the GIC is powered off, are routed through an interrupt controller in an always-on domain called the Power Domain Controller a.k.a PDC. This series adds support for the PDC's interrupt controller.
Please consider reviewing these patches.
RFC v1: https://patchwork.kernel.org/patch/10180857/
Lina Iyer (3):
drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs dt-bindings/interrupt-controller: pdc: descibe PDC device binding
drivers: irqchip: pdc: log PDC info in FTRACE
.../bindings/interrupt-controller/qcom,pdc.txt | 78 +++++ drivers/irqchip/Kconfig | 9 + drivers/irqchip/Makefile | 1 + drivers/irqchip/qcom-pdc.c | 333 +++++++++++++++++++++ include/trace/events/pdc.h | 55 ++++ 5 files changed, 476 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt create mode 100644 drivers/irqchip/qcom-pdc.c create mode 100644 include/trace/events/pdc.h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
RFC PATCH v2 1/3 drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
Quoted 1 line
From : Archana Sathyakumar <[email protected]>
The Power Domain Controller (PDC) on QTI SoCs like SDM845 houses an interrupt controller along with other domain control functions to handle interrupt related functions like handle falling edge or active low which are not detected at the GIC and handle wakeup interrupts.
The interrupt controller is on an always-on domain for the purpose of waking up the processor. Only a subset of the processor's interrupts are routed through the PDC to the GIC. The PDC powers on the processors' domain, when in low power mode and replays pending interrupts so the GIC may wake up the processor.
--- drivers/irqchip/Kconfig | 9 ++ drivers/irqchip/Makefile | 1 + drivers/irqchip/qcom-pdc.c | 326 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 336 insertions(+) create mode 100644 drivers/irqchip/qcom-pdc.c
drivers/irqchip/Kconfig +9 -0
@@ -343,4 +343,13 @@ config MESON_IRQ_GPIO help Support Meson SoC Family GPIO Interrupt Multiplexer +config QCOM_PDC + bool "QCOM PDC" + depends on ARCH_QCOM + select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY + help + Power Domain Controller driver to manage and configure wakeup + IRQs for Qualcomm Technologies Inc (QTI) mobile chips. + endmenu
drivers/irqchip/Makefile +1 -0
@@ -84,3 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o obj-$(CONFIG_ARCH_SYNQUACER) += irq-sni-exiu.o obj-$(CONFIG_MESON_IRQ_GPIO) += irq-meson-gpio.o +obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o
drivers/irqchip/qcom-pdc.c new file +326 -0
326 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
drivers/irqchip/qcom-pdc.c line 66
Quoted 11 lines · Lina Iyer
drivers/irqchip/qcom-pdc.c new file +11
-0+static inline void pdc_enable_intr (struct irq_data *d,bool on) +{ +int pin_out = d->hwirq; +u32 index, mask; +u32 enable; +unsigned long flags; + + index = pin_out /32 ; + mask = pin_out %32 ; + +spin_lock_irqsave (&pdc_lock, flags);
Please make this a raw spinlock. Aside of that the _irqsave() is pointless as the chip callbacks are already called with interrupts disabled.
drivers/irqchip/qcom-pdc.c line 68
Quoted 2 lines
drivers/irqchip/qcom-pdc.c new file +2
-0+ enable =pdc_reg_read (IRQ_ENABLE_BANK , index); + enable = on ?ENABLE_INTR (enable, mask) :CLEAR_INTR (enable, mask);
You really should cache the enable register content to avoid the read back
Quoted 24 lines
drivers/irqchip/qcom-pdc.c new file +24
-0+pdc_reg_write (IRQ_ENABLE_BANK , index, enable); +spin_unlock_irqrestore (&pdc_lock, flags); +} + +static void qcom_pdc_gic_mask (struct irq_data *d) +{ +pdc_enable_intr (d, false); +irq_chip_mask_parent (d); +} + +static void qcom_pdc_gic_unmask (struct irq_data *d) +{ +pdc_enable_intr (d, true); +irq_chip_unmask_parent (d); +} + +/* +* GIC does not handle falling edge or active low. To allow falling edge and +* active low interrupts to be handled at GIC, PDC has an inverter that inverts +* falling edge into a rising edge and active low into an active high. +* For the inverter to work, the polarity bit in the IRQ_CONFIG register has to +* set as per the table below. +* (polarity, falling edge, rising edge ) POLARITY +* 3'b0 00 Level sensitive active low LOW
drivers/irqchip/qcom-pdc.c line 102
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+* 3'b0 01 Rising edge sensitive NOT USED +* 3'b0 10 Falling edge sensitive LOW +* 3'b0 11 Dual Edge sensitive NOT USED +* 3'b1 00 Level senstive active High HIGH +* 3'b1 01 Falling Edge sensitive NOT USED +* 3'b1 10 Rising edge sensitive HIGH +* 3'b1 11 Dual Edge sensitive HIGH +*/ +enum pdc_irq_config_bits { +PDC_POLARITY_LOW =0 ,// 0 00
What's wrong with
PDC_POLARITY_LOW = 000b,
PDC_FALLING_EDGE = 010b,instead of decimal and these weird comments ?
drivers/irqchip/qcom-pdc.c line 177
Quoted 7 lines
drivers/irqchip/qcom-pdc.c new file +7
-0+static irq_hw_number_t get_irq_for_pin (int pin,struct pdc_pin_data *pdc_data) +{ +int i; + +for (i =0 ; pdc_data[i].pin >=0 ; i++) +if (pdc_data[i].pin == pin) +return pdc_data[i].hwirq;
Please let the for() loop have braces. See:
https://marc.info/?l=linux-kernel&m=148467980905537&w=2
drivers/irqchip/qcom-pdc.c line 183
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+ +return pin; +} + +static int qcom_pdc_translate (struct irq_domain *d, +struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type)
Please align the arguments right of the opening brace:
static int qcom_pdc_translate(struct irq_domain *d,
struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
Quoted 5 lines
drivers/irqchip/qcom-pdc.c new file +5
-0+{ +if (is_of_node (fwspec->fwnode)) { +if (fwspec->param_count <3 ) +return -EINVAL ; +
drivers/irqchip/qcom-pdc.c line 198
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ *hwirq = fwspec->param[1 ]; + *type = fwspec->param[2 ] &IRQ_TYPE_SENSE_MASK ; +return 0 ; + } + +return -EINVAL ; +} + +static int qcom_pdc_alloc (struct irq_domain *domain, +unsigned int virq,unsigned int nr_irqs,void *data)
Ditto
drivers/irqchip/qcom-pdc.c line 243
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+static int pdc_setup_pin_mapping (struct device_node *np, +struct pdc_pin_data **data) +{ +int ret; +int n, i, j, k, pins =0 ; +int *val;
I have no idea what's the rationale behind these 3 lines of int declarations.
Quoted 22 lines
drivers/irqchip/qcom-pdc.c new file +22
-0+struct pdc_pin_data *map; + + n =of_property_count_elems_of_size (np,"qcom,pdc-ranges" ,sizeof (u32)); +if (!n || n %3 ) +return -EINVAL ; + + val =kcalloc (n,sizeof (u32),GFP_KERNEL ); +if (!val) +return -ENOMEM ; + + ret =of_property_read_u32_array (np,"qcom,pdc-ranges" , val, n); +if (ret) +return ret; + +for (i =0 ; i < n; i +=3 ) + pins += val[i +2 ]; + +if (pins >PDC_MAX_IRQS ) +return -EFAULT ; + + map =kcalloc (pins +1 ,sizeof (*map),GFP_KERNEL ); +if (!map) {
drivers/irqchip/qcom-pdc.c line 275
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ ret = -ENOMEM ; + goto fail; + } + +for (i =0 , k =0 ; i < n; i +=3 ) { +for (j =0 ; j < val[i +2 ]; j++, k++) { + map[k].pin = val[i] + j; + map[k].hwirq = val[i +1 ] + j; + } + }
This all is really horrible to read. First of all the val[] array. That can be represented in a structure, right? Without looking at the DT patch the code lets me assume:
struct pdcrange {
u32 pin; u32 hwirq; u32 numpins; u32 unused;
};
So the above becomes:
nelm = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32)); if (!nelm || nelm % 3) return -EINVAL; nranges = nelm / 4; ranges = kcalloc(nranges, sizeof(*prng), GFP_KERNEL); if (!ranges) return -ENOMEM;
which makes the pin counting readable:
for (i = 0; i < nranges; i++) pins += ranges[i].numpins;
And then allows to write the map initialization with:
*data = map;
for (i = 0; i < nranges; i++) {
for (j = 0; j < ranges[i].numpins; j++, map++) {
map->pin = ranges[i].pin + j;
map->hwirq = ranges[i].hwirq + j;
}
}
map->pin = -1;Hmm?
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+int qcom_pdc_init (struct device_node *node,struct device_node *parent) +{ +struct irq_domain *parent_domain, *pdc_domain; +struct pdc_pin_data *pdc_data =NULL ; +int ret; + + pdc_base =of_iomap (node,0 ); +if (!pdc_base) { +pr_err ("%s(): unable to map PDC registers\n" , node->full_name); +return -ENXIO ;
drivers/irqchip/qcom-pdc.c line 303
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ } + + parent_domain =irq_find_host (parent); +if (!parent_domain) { +pr_err ("unable to obtain PDC parent domain\n" ); + ret = -ENXIO ; + goto failure; + } + + ret =pdc_setup_pin_mapping (node, &pdc_data);
You can let pdc_setup_pin_mapping() return a pointer to pdc_data or NULL and check the pointer for ERR or NULL instead of having that ret indirection.
drivers/irqchip/qcom-pdc.c line 311
Quoted 8 lines
drivers/irqchip/qcom-pdc.c new file +8
-0+if (ret) { +pr_err ("failed to setup PDC pin mapping\n" ); + goto failure; + } + + pdc_domain =irq_domain_create_hierarchy (parent_domain,0 ,PDC_MAX_IRQS , +of_fwnode_handle (node), &qcom_pdc_ops, + pdc_data);
See comment about argument alignement above. Hint: shortening the *_domain variable names helps.
Thanks,
tglx
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
All valid comments. Will fix them all in the next rev.
Thanks Thomas.
-- Lina
Quoted 229 lines · Thomas Gleixner
drivers/irqchip/qcom-pdc.c line 66
Quoted 11 lines · Lina Iyer
drivers/irqchip/qcom-pdc.c new file +11
-0+static inline void pdc_enable_intr (struct irq_data *d,bool on) +{ +int pin_out = d->hwirq; +u32 index, mask; +u32 enable; +unsigned long flags; + + index = pin_out /32 ; + mask = pin_out %32 ; + +spin_lock_irqsave (&pdc_lock, flags);Please make this a raw spinlock. Aside of that the
_irqsave()is pointless as the chip callbacks are already called with interrupts disabled.drivers/irqchip/qcom-pdc.c line 68
Quoted 2 lines
drivers/irqchip/qcom-pdc.c new file +2
-0+ enable =pdc_reg_read (IRQ_ENABLE_BANK , index); + enable = on ?ENABLE_INTR (enable, mask) :CLEAR_INTR (enable, mask);You really should cache the enable register content to avoid the read back
Quoted 24 lines
drivers/irqchip/qcom-pdc.c new file +24
-0+pdc_reg_write (IRQ_ENABLE_BANK , index, enable); +spin_unlock_irqrestore (&pdc_lock, flags); +} + +static void qcom_pdc_gic_mask (struct irq_data *d) +{ +pdc_enable_intr (d, false); +irq_chip_mask_parent (d); +} + +static void qcom_pdc_gic_unmask (struct irq_data *d) +{ +pdc_enable_intr (d, true); +irq_chip_unmask_parent (d); +} + +/* +* GIC does not handle falling edge or active low. To allow falling edge and +* active low interrupts to be handled at GIC, PDC has an inverter that inverts +* falling edge into a rising edge and active low into an active high. +* For the inverter to work, the polarity bit in the IRQ_CONFIG register has to +* set as per the table below. +* (polarity, falling edge, rising edge ) POLARITY +* 3'b0 00 Level sensitive active low LOW drivers/irqchip/qcom-pdc.c line 102
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+* 3'b0 01 Rising edge sensitive NOT USED +* 3'b0 10 Falling edge sensitive LOW +* 3'b0 11 Dual Edge sensitive NOT USED +* 3'b1 00 Level senstive active High HIGH +* 3'b1 01 Falling Edge sensitive NOT USED +* 3'b1 10 Rising edge sensitive HIGH +* 3'b1 11 Dual Edge sensitive HIGH +*/ +enum pdc_irq_config_bits { +PDC_POLARITY_LOW =0 ,// 0 00 What's wrong with
PDC_POLARITY_LOW = 000b, PDC_FALLING_EDGE = 010b,instead of decimal and these weird comments ?
drivers/irqchip/qcom-pdc.c line 177
Quoted 7 lines
drivers/irqchip/qcom-pdc.c new file +7
-0+static irq_hw_number_t get_irq_for_pin (int pin,struct pdc_pin_data *pdc_data) +{ +int i; + +for (i =0 ; pdc_data[i].pin >=0 ; i++) +if (pdc_data[i].pin == pin) +return pdc_data[i].hwirq;Please let the
for()loop have braces. See:https://marc.info/?l=linux-kernel&m=148467980905537&w=2drivers/irqchip/qcom-pdc.c line 183
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+ +return pin; +} + +static int qcom_pdc_translate (struct irq_domain *d, +struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type)Please align the arguments right of the opening brace:
static int qcom_pdc_translate(struct
irq_domain*d,
structirq_fwspec*fwspec, unsigned long *hwirq, unsigned int *type)Quoted 5 lines
drivers/irqchip/qcom-pdc.c new file +5
-0+{ +if (is_of_node (fwspec->fwnode)) { +if (fwspec->param_count <3 ) +return -EINVAL ; +drivers/irqchip/qcom-pdc.c line 198
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ *hwirq = fwspec->param[1 ]; + *type = fwspec->param[2 ] &IRQ_TYPE_SENSE_MASK ; +return 0 ; + } + +return -EINVAL ; +} + +static int qcom_pdc_alloc (struct irq_domain *domain, +unsigned int virq,unsigned int nr_irqs,void *data)Ditto
drivers/irqchip/qcom-pdc.c line 243
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+static int pdc_setup_pin_mapping (struct device_node *np, +struct pdc_pin_data **data) +{ +int ret; +int n, i, j, k, pins =0 ; +int *val;I have no idea what's the rationale behind these 3 lines of int declarations.
Quoted 22 lines
drivers/irqchip/qcom-pdc.c new file +22
-0+struct pdc_pin_data *map; + + n =of_property_count_elems_of_size (np,"qcom,pdc-ranges" ,sizeof (u32)); +if (!n || n %3 ) +return -EINVAL ; + + val =kcalloc (n,sizeof (u32),GFP_KERNEL ); +if (!val) +return -ENOMEM ; + + ret =of_property_read_u32_array (np,"qcom,pdc-ranges" , val, n); +if (ret) +return ret; + +for (i =0 ; i < n; i +=3 ) + pins += val[i +2 ]; + +if (pins >PDC_MAX_IRQS ) +return -EFAULT ; + + map =kcalloc (pins +1 ,sizeof (*map),GFP_KERNEL ); +if (!map) {drivers/irqchip/qcom-pdc.c line 275
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ ret = -ENOMEM ; + goto fail; + } + +for (i =0 , k =0 ; i < n; i +=3 ) { +for (j =0 ; j < val[i +2 ]; j++, k++) { + map[k].pin = val[i] + j; + map[k].hwirq = val[i +1 ] + j; + } + }This all is really horrible to read. First of all the val[] array. That can be represented in a structure, right? Without looking at the DT patch the code lets me assume:
struct pdcrange {
u32 pin; u32 hwirq; u32 numpins; u32 unused;};
So the above becomes:
nelm = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32)); if (!nelm || nelm % 3) return -EINVAL; nranges = nelm / 4; ranges = kcalloc(nranges, sizeof(*prng), GFP_KERNEL); if (!ranges) return -ENOMEM;which makes the pin counting readable:
for (i = 0; i < nranges; i++) pins += ranges[i].numpins;And then allows to write the map initialization with:
*data = map; for (i = 0; i < nranges; i++) { for (j = 0; j < ranges[i].numpins; j++, map++) { map->pin = ranges[i].pin + j; map->hwirq = ranges[i].hwirq + j; } } map->pin = -1;Hmm?
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+int qcom_pdc_init (struct device_node *node,struct device_node *parent) +{ +struct irq_domain *parent_domain, *pdc_domain; +struct pdc_pin_data *pdc_data =NULL ; +int ret; + + pdc_base =of_iomap (node,0 ); +if (!pdc_base) { +pr_err ("%s(): unable to map PDC registers\n" , node->full_name); +return -ENXIO ;drivers/irqchip/qcom-pdc.c line 303
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ } + + parent_domain =irq_find_host (parent); +if (!parent_domain) { +pr_err ("unable to obtain PDC parent domain\n" ); + ret = -ENXIO ; + goto failure; + } + + ret =pdc_setup_pin_mapping (node, &pdc_data);You can let
pdc_setup_pin_mapping()return a pointer topdc_dataor NULL and check the pointer for ERR or NULL instead of having that ret indirection.drivers/irqchip/qcom-pdc.c line 311
Quoted 8 lines
drivers/irqchip/qcom-pdc.c new file +8
-0+if (ret) { +pr_err ("failed to setup PDC pin mapping\n" ); + goto failure; + } + + pdc_domain =irq_domain_create_hierarchy (parent_domain,0 ,PDC_MAX_IRQS , +of_fwnode_handle (node), &qcom_pdc_ops, + pdc_data);See comment about argument alignement above. Hint: shortening the
*_domainvariable names helps.Thanks,
tglx
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
In reply to Lina IyerHi Lina,
Quoted 75 lines · Lina Iyer
From : Archana Sathyakumar <[email protected]>
The Power Domain Controller (PDC) on QTI SoCs like SDM845 houses an interrupt controller along with other domain control functions to handle interrupt related functions like handle falling edge or active low which are not detected at the GIC and handle wakeup interrupts.
The interrupt controller is on an always-on domain for the purpose of waking up the processor. Only a subset of the processor's interrupts are routed through the PDC to the GIC. The PDC powers on the processors' domain, when in low power mode and replays pending interrupts so the GIC may wake up the processor.
Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>--- drivers/irqchip/Kconfig | 9 ++ drivers/irqchip/Makefile | 1 + drivers/irqchip/qcom-pdc.c | 326 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 336 insertions(+) create mode 100644 drivers/irqchip/qcom-pdc.cdrivers/irqchip/Kconfig +9
-0@@ -343,4 +343,13 @@ config MESON_IRQ_GPIO help Support Meson SoC Family GPIO Interrupt Multiplexer +config QCOM_PDC + bool "QCOM PDC" + depends on ARCH_QCOM + select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY + help + Power Domain Controller driver to manage and configure wakeup + IRQs for Qualcomm Technologies Inc (QTI) mobile chips. + endmenudrivers/irqchip/Makefile +1
-0@@ -84,3 +84,4 @@ obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o obj-$(CONFIG_ARCH_SYNQUACER) += irq-sni-exiu.o obj-$(CONFIG_MESON_IRQ_GPIO) += irq-meson-gpio.o +obj-$(CONFIG_QCOM_PDC) += qcom-pdc.odrivers/irqchip/qcom-pdc.c new file +20
-0@@ -0,0 +1,326 @@ +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +*/ + +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/irq.h> +#include <linux/irqchip.h> +#include <linux/irqdomain.h> +#include <linux/io.h>
drivers/irqchip/qcom-pdc.c line 30
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+#include <linux/kernel.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/spinlock.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/types.h> + +#define PDC_MAX_IRQS 126
Is that an absolute, architectural maximum? Or should it come from the DT (being the sum of all ranges that are provided by this PDC)?
drivers/irqchip/qcom-pdc.c line 41
Quoted 11 lines
drivers/irqchip/qcom-pdc.c new file +11
-0+ +#define CLEAR_INTR (reg, intr) (reg & ~(1 << intr)) +#define ENABLE_INTR (reg, intr) (reg | (1 << intr)) + +#define IRQ_ENABLE_BANK 0x10 +#define IRQ_i_CFG 0x110 + +struct pdc_pin_data { +int pin; +int hwirq; +};
I seriously doubt you need this structure, see below.
drivers/irqchip/qcom-pdc.c line 44
Quoted 3 lines
drivers/irqchip/qcom-pdc.c new file +3
-0+ +static DEFINE_SPINLOCK (pdc_lock); +static void __iomem *pdc_base;
You only have one of those per SoC?
Quoted 93 lines
drivers/irqchip/qcom-pdc.c new file +93
-0+ +static inline void pdc_reg_write (int reg,u32 i,u32 val) +{ +writel_relaxed (val, pdc_base + reg + i *sizeof (u32)); +} + +static inline u32 pdc_reg_read (int reg,u32 i) +{ +return readl_relaxed (pdc_base + reg + i *sizeof (u32)); +} + +static inline void pdc_enable_intr (struct irq_data *d,bool on) +{ +int pin_out = d->hwirq; +u32 index, mask; +u32 enable; +unsigned long flags; + + index = pin_out /32 ; + mask = pin_out %32 ; + +spin_lock_irqsave (&pdc_lock, flags); + enable =pdc_reg_read (IRQ_ENABLE_BANK , index); + enable = on ?ENABLE_INTR (enable, mask) :CLEAR_INTR (enable, mask); +pdc_reg_write (IRQ_ENABLE_BANK , index, enable); +spin_unlock_irqrestore (&pdc_lock, flags); +} + +static void qcom_pdc_gic_mask (struct irq_data *d) +{ +pdc_enable_intr (d, false); +irq_chip_mask_parent (d); +} + +static void qcom_pdc_gic_unmask (struct irq_data *d) +{ +pdc_enable_intr (d, true); +irq_chip_unmask_parent (d); +} + +/* +* GIC does not handle falling edge or active low. To allow falling edge and +* active low interrupts to be handled at GIC, PDC has an inverter that inverts +* falling edge into a rising edge and active low into an active high. +* For the inverter to work, the polarity bit in the IRQ_CONFIG register has to +* set as per the table below. +* (polarity, falling edge, rising edge ) POLARITY +* 3'b0 00 Level sensitive active low LOW +* 3'b0 01 Rising edge sensitive NOT USED +* 3'b0 10 Falling edge sensitive LOW +* 3'b0 11 Dual Edge sensitive NOT USED +* 3'b1 00 Level senstive active High HIGH +* 3'b1 01 Falling Edge sensitive NOT USED +* 3'b1 10 Rising edge sensitive HIGH +* 3'b1 11 Dual Edge sensitive HIGH +*/ +enum pdc_irq_config_bits { +PDC_POLARITY_LOW =0 ,// 0 00 +PDC_FALLING_EDGE =2 ,// 0 10 +PDC_POLARITY_HIGH =4 ,// 1 00 +PDC_RISING_EDGE =6 ,// 1 10 +PDC_DUAL_EDGE =7 ,// 1 11 +}; + +/** +* qcom_pdc_gic_set_type: Configure PDC for the interrupt +* +* @d: the interrupt data +* @type: the interrupt type +* +* If @type is edge triggered, forward that as Rising edge as PDC +* takes care of converting falling edge to rising edge signal +* If @type is level, then forward that as level high as PDC +* takes care of converting falling edge to rising edge signal +*/ +static int qcom_pdc_gic_set_type (struct irq_data *d,unsigned int type) +{ +int pin_out = d->hwirq; +enum pdc_irq_config_bits pdc_type; + +switch (type) { +case IRQ_TYPE_EDGE_RISING : + pdc_type =PDC_RISING_EDGE ; + type =IRQ_TYPE_EDGE_RISING ; +break ; +case IRQ_TYPE_EDGE_FALLING : + pdc_type =PDC_FALLING_EDGE ; + type =IRQ_TYPE_EDGE_RISING ; +break ; +case IRQ_TYPE_EDGE_BOTH : + pdc_type =PDC_DUAL_EDGE ; +break ; +case IRQ_TYPE_LEVEL_HIGH :
drivers/irqchip/qcom-pdc.c line 147
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ pdc_type =PDC_POLARITY_HIGH ; + type =IRQ_TYPE_LEVEL_HIGH ; +break ; +case IRQ_TYPE_LEVEL_LOW : + pdc_type =PDC_POLARITY_LOW ; + type =IRQ_TYPE_LEVEL_HIGH ; +break ; +default : + pdc_type =PDC_POLARITY_HIGH ; +break ;
If this default clause triggers, something is pretty wrong. You may want to warn and bail out instead.
Quoted 23 lines
drivers/irqchip/qcom-pdc.c new file +23
-0+ } + +pdc_reg_write (IRQ_i_CFG, pin_out, pdc_type); + +return irq_chip_set_type_parent (d, type); +} + +static struct irq_chip qcom_pdc_gic_chip = { + .name ="pdc-gic" , + .irq_eoi = irq_chip_eoi_parent, + .irq_mask = qcom_pdc_gic_mask, + .irq_unmask = qcom_pdc_gic_unmask, + .irq_retrigger = irq_chip_retrigger_hierarchy, + .irq_set_type = qcom_pdc_gic_set_type, + .flags =IRQCHIP_MASK_ON_SUSPEND | +IRQCHIP_SET_TYPE_MASKED | +IRQCHIP_SKIP_SET_WAKE , + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, +#ifdef CONFIG_SMP + .irq_set_affinity = irq_chip_set_affinity_parent, +#endif +}; +
drivers/irqchip/qcom-pdc.c line 180
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+static irq_hw_number_t get_irq_for_pin (int pin,struct pdc_pin_data *pdc_data) +{ +int i; + +for (i =0 ; pdc_data[i].pin >=0 ; i++) +if (pdc_data[i].pin == pin) +return pdc_data[i].hwirq; + +return pin; +}
This is the function that irks me. The DT already gives you a nice set of ranges with all the information you need. And yet, you expand the whole thing into at most 127 structures, wasting memory and making the search time a function of the number of interrupts instead of being that of the number of regions. Not very nice. How about something like this (untested):
struct pin_region {
u32 pin_base;
u32 parent_base;
u32 cnt;
};struct pin_region *pin_regions;
int pin_region_count;
static int pdc_pin_to_parent_hwirq(int pin)
{
int i;
for (i = 0; i < pin_region_count; i++) {
if (pin >= pin_regions[i].pin_base &&
pin < (pin_regions[i].pin_base + pin_regions[i].cnt)
return (pin_regions[i].parent_base + pin -
pin_regions[i].pin_base);
}
WARN();
return -1;
}Less memory, less comparisons.
drivers/irqchip/qcom-pdc.c line 187
Quoted 7 lines
drivers/irqchip/qcom-pdc.c new file +7
-0+ +static int qcom_pdc_translate (struct irq_domain *d, +struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type) +{ +if (is_of_node (fwspec->fwnode)) { +if (fwspec->param_count <3 ) +return -EINVAL ;
Why 3? Reading the DT binding, this is indeed set to 3 without any reason. I'd suggest this becomes 2, encoding the pin number and the trigger information, as the leading 0 is quite useless. Yes, I know other examples in the kernel are using this 0, and that was a consequence of retrofitting the omitted interrupt controllers (back in the days of the stupid gic_arch_extn...). Don't do that.
drivers/irqchip/qcom-pdc.c line 190
Quoted 3 lines
drivers/irqchip/qcom-pdc.c new file +3
-0+ + *hwirq = fwspec->param[1 ]; + *type = fwspec->param[2 ] &IRQ_TYPE_SENSE_MASK ;
... and fix this accordingly.
Quoted 26 lines
drivers/irqchip/qcom-pdc.c new file +26
-0+return 0 ; + } + +return -EINVAL ; +} + +static int qcom_pdc_alloc (struct irq_domain *domain, +unsigned int virq,unsigned int nr_irqs,void *data) +{ +struct irq_fwspec *fwspec = data; +struct irq_fwspec parent_fwspec; +irq_hw_number_t hwirq, parent_hwirq; +unsigned int type; +int ret; + + ret =qcom_pdc_translate (domain, fwspec, &hwirq, &type); +if (ret) +return -EINVAL ; + + parent_hwirq =get_irq_for_pin (hwirq, domain->host_data); + ret =irq_domain_set_hwirq_and_chip (domain, virq, hwirq, + &qcom_pdc_gic_chip, (void *)parent_hwirq); +if (ret) +return ret; + +if (type &IRQ_TYPE_EDGE_BOTH )
drivers/irqchip/qcom-pdc.c line 226
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ type =IRQ_TYPE_EDGE_RISING ; + +if (type &IRQ_TYPE_LEVEL_MASK ) + type =IRQ_TYPE_LEVEL_HIGH ; + + parent_fwspec = *fwspec; + parent_fwspec.param[1 ] = parent_hwirq; + parent_fwspec.param[2 ] &= ~IRQ_TYPE_SENSE_MASK ; + parent_fwspec.param[2 ] |= type; + parent_fwspec.fwnode = domain->parent->fwnode;
This becomes:
parent_fwspec.fwnode = domain->parent->fwnode; parent_fwspec.param_count = 3; // That's what the GIC wants parent_fwspec.param[0] = 0; //GIC_SPI parent_fwspec.param[1] = parent_hwirq; parent_fwspec.param[2] = type;
Quoted 20 lines
drivers/irqchip/qcom-pdc.c new file +20
-0+ +return irq_domain_alloc_irqs_parent (domain, virq, nr_irqs, + &parent_fwspec); +} + +static const struct irq_domain_ops qcom_pdc_ops = { + .translate = qcom_pdc_translate, + .alloc = qcom_pdc_alloc, + .free = irq_domain_free_irqs_common, +}; + +static int pdc_setup_pin_mapping (struct device_node *np, +struct pdc_pin_data **data) +{ +int ret; +int n, i, j, k, pins =0 ; +int *val; +struct pdc_pin_data *map; + + n =of_property_count_elems_of_size (np,"qcom,pdc-ranges" ,sizeof (u32));
drivers/irqchip/qcom-pdc.c line 256
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+if (!n || n %3 ) +return -EINVAL ; + + val =kcalloc (n,sizeof (u32),GFP_KERNEL ); +if (!val) +return -ENOMEM ; + + ret =of_property_read_u32_array (np,"qcom,pdc-ranges" , val, n); +if (ret) +return ret;
Memory leak.
drivers/irqchip/qcom-pdc.c line 262
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+ +for (i =0 ; i < n; i +=3 ) + pins += val[i +2 ]; + +if (pins >PDC_MAX_IRQS ) +return -EFAULT ;
EFAULT is for an actual page fault. If you get one here, you have bigger problems. EINVAL is better. is You're also leaking memory.
drivers/irqchip/qcom-pdc.c line 275
Quoted 13 lines
drivers/irqchip/qcom-pdc.c new file +13
-0+ + map =kcalloc (pins +1 ,sizeof (*map),GFP_KERNEL ); +if (!map) { + ret = -ENOMEM ; + goto fail; + } + +for (i =0 , k =0 ; i < n; i +=3 ) { +for (j =0 ; j < val[i +2 ]; j++, k++) { + map[k].pin = val[i] + j; + map[k].hwirq = val[i +1 ] + j; + } + }
That's the thing that needs to go. Just store the ranges as exposed by the DT, maybe with some checking that there is no overlap in the ranges (not that it matters much...).
Quoted 42 lines
drivers/irqchip/qcom-pdc.c new file +41
-0+ + map[k].pin =-1 ; + *data = map; +fail: +kfree (val); +return ret; +} + +int qcom_pdc_init (struct device_node *node,struct device_node *parent) +{ +struct irq_domain *parent_domain, *pdc_domain; +struct pdc_pin_data *pdc_data =NULL ; +int ret; + + pdc_base =of_iomap (node,0 ); +if (!pdc_base) { +pr_err ("%s(): unable to map PDC registers\n" , node->full_name); +return -ENXIO ; + } + + parent_domain =irq_find_host (parent); +if (!parent_domain) { +pr_err ("unable to obtain PDC parent domain\n" ); + ret = -ENXIO ; + goto failure; + } + + ret =pdc_setup_pin_mapping (node, &pdc_data); +if (ret) { +pr_err ("failed to setup PDC pin mapping\n" ); + goto failure; + } + + pdc_domain =irq_domain_create_hierarchy (parent_domain,0 ,PDC_MAX_IRQS , +of_fwnode_handle (node), &qcom_pdc_ops, + pdc_data); +if (!pdc_domain) { +pr_err ("GIC domain add failed\n" ); + ret = -ENOMEM ; + goto failure; + }
drivers/irqchip/qcom-pdc.c line 326
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ +return 0 ; + +failure: +kfree (pdc_data); +iounmap (pdc_base); +return ret; +} + +IRQCHIP_DECLARE (pdc_sdm845,"qcom,sdm845-pdc" , qcom_pdc_init);
Thanks,
M.
Signature · 4 lines
Jazz is not dead. It just smells funny... -- Jazz is not dead, it just smell funny.
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
Quoted 26 lines · Marc Zyngier
Hi Lina,
Quoted 16 lines · Lina Iyer
From : Archana Sathyakumar <[email protected]>
The Power Domain Controller (PDC) on QTI SoCs like SDM845 houses an interrupt controller along with other domain control functions to handle interrupt related functions like handle falling edge or active low which are not detected at the GIC and handle wakeup interrupts.
The interrupt controller is on an always-on domain for the purpose of waking up the processor. Only a subset of the processor's interrupts are routed through the PDC to the GIC. The PDC powers on the processors' domain, when in low power mode and replays pending interrupts so the GIC may wake up the processor.
Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>---
drivers/irqchip/qcom-pdc.c line 41
Quoted 4 lines
drivers/irqchip/qcom-pdc.c new file +4
-0+struct pdc_pin_data { +int pin; +int hwirq; +};I seriously doubt you need this structure, see below.
Quoted 6 lines
drivers/irqchip/qcom-pdc.c line 44
Quoted 3 lines
drivers/irqchip/qcom-pdc.c new file +3
-0+ +static DEFINE_SPINLOCK (pdc_lock); +static void __iomem *pdc_base;You only have one of those per SoC?
There is only one that Linux can control.
Quoted 23 lines
Quoted 23 lines
drivers/irqchip/qcom-pdc.c new file +23
-0+static int qcom_pdc_gic_set_type (struct irq_data *d,unsigned int type) +{ +int pin_out = d->hwirq; +enum pdc_irq_config_bits pdc_type; + +switch (type) { +case IRQ_TYPE_EDGE_RISING : + pdc_type =PDC_RISING_EDGE ; + type =IRQ_TYPE_EDGE_RISING ; +break ; +case IRQ_TYPE_EDGE_FALLING : + pdc_type =PDC_FALLING_EDGE ; + type =IRQ_TYPE_EDGE_RISING ; +break ; +case IRQ_TYPE_EDGE_BOTH : + pdc_type =PDC_DUAL_EDGE ; +break ; +case IRQ_TYPE_LEVEL_HIGH : + pdc_type =PDC_POLARITY_HIGH ; + type =IRQ_TYPE_LEVEL_HIGH ; +break ; +case IRQ_TYPE_LEVEL_LOW : + pdc_type =PDC_POLARITY_LOW ;
Quoted 9 lines
drivers/irqchip/qcom-pdc.c line 147
Quoted 5 lines
drivers/irqchip/qcom-pdc.c new file +5
-0+ type =IRQ_TYPE_LEVEL_HIGH ; +break ; +default : + pdc_type =PDC_POLARITY_HIGH ; +break ;If this default clause triggers, something is pretty wrong. You may want to warn and bail out instead.
The hardware defaults to this. I can bail out as well.
Quoted 32 lines
Quoted 32 lines
drivers/irqchip/qcom-pdc.c new file +32
-0+ } + +pdc_reg_write (IRQ_i_CFG, pin_out, pdc_type); + +return irq_chip_set_type_parent (d, type); +} + +static struct irq_chip qcom_pdc_gic_chip = { + .name ="pdc-gic" , + .irq_eoi = irq_chip_eoi_parent, + .irq_mask = qcom_pdc_gic_mask, + .irq_unmask = qcom_pdc_gic_unmask, + .irq_retrigger = irq_chip_retrigger_hierarchy, + .irq_set_type = qcom_pdc_gic_set_type, + .flags =IRQCHIP_MASK_ON_SUSPEND | +IRQCHIP_SET_TYPE_MASKED | +IRQCHIP_SKIP_SET_WAKE , + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, +#ifdef CONFIG_SMP + .irq_set_affinity = irq_chip_set_affinity_parent, +#endif +}; + +static irq_hw_number_t get_irq_for_pin (int pin,struct pdc_pin_data *pdc_data) +{ +int i; + +for (i =0 ; pdc_data[i].pin >=0 ; i++) +if (pdc_data[i].pin == pin) +return pdc_data[i].hwirq; + +return pin;
Quoted 9 lines
Quoted 1 line
+}
This is the function that irks me. The DT already gives you a nice set of ranges with all the information you need. And yet, you expand the whole thing into at most 127 structures, wasting memory and making the search time a function of the number of interrupts instead of being that of the number of regions. Not very nice. How about something like this (untested):
Duh! Why didn't I think of this.. Thanks.
Quoted 24 lines
struct pin_region { u32 pin_base; u32 parent_base; u32 cnt; };struct
pin_region*pin_regions;
intpin_region_count;static int
pdc_pin_to_parent_hwirq(int pin){ int i; for (i = 0; i < pin_region_count; i++) { if (pin >= pin_regions[i].pin_base && pin < (pin_regions[i].pin_base + pin_regions[i].cnt) return (pin_regions[i].parent_base + pin - pin_regions[i].pin_base); } WARN(); return -1; }
Quoted 2 lines
Less memory, less comparisons.
Agreed.
Quoted 60 lines
drivers/irqchip/qcom-pdc.c line 187
Quoted 7 lines
drivers/irqchip/qcom-pdc.c new file +7
-0+ +static int qcom_pdc_translate (struct irq_domain *d, +struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type) +{ +if (is_of_node (fwspec->fwnode)) { +if (fwspec->param_count <3 ) +return -EINVAL ;Why 3? Reading the DT binding, this is indeed set to 3 without any reason. I'd suggest this becomes 2, encoding the pin number and the trigger information, as the leading 0 is quite useless. Yes, I know other examples in the kernel are using this 0, and that was a consequence of retrofitting the omitted interrupt controllers (back in the days of the stupid
gic_arch_extn...). Don't do that.drivers/irqchip/qcom-pdc.c line 190
Quoted 3 lines
drivers/irqchip/qcom-pdc.c new file +3
-0+ + *hwirq = fwspec->param[1 ]; + *type = fwspec->param[2 ] &IRQ_TYPE_SENSE_MASK ;... and fix this accordingly.
Quoted 26 lines
drivers/irqchip/qcom-pdc.c new file +26
-0+return 0 ; + } + +return -EINVAL ; +} + +static int qcom_pdc_alloc (struct irq_domain *domain, +unsigned int virq,unsigned int nr_irqs,void *data) +{ +struct irq_fwspec *fwspec = data; +struct irq_fwspec parent_fwspec; +irq_hw_number_t hwirq, parent_hwirq; +unsigned int type; +int ret; + + ret =qcom_pdc_translate (domain, fwspec, &hwirq, &type); +if (ret) +return -EINVAL ; + + parent_hwirq =get_irq_for_pin (hwirq, domain->host_data); + ret =irq_domain_set_hwirq_and_chip (domain, virq, hwirq, + &qcom_pdc_gic_chip, (void *)parent_hwirq); +if (ret) +return ret; + +if (type &IRQ_TYPE_EDGE_BOTH )drivers/irqchip/qcom-pdc.c line 226
Quoted 10 lines
drivers/irqchip/qcom-pdc.c new file +10
-0+ type =IRQ_TYPE_EDGE_RISING ; + +if (type &IRQ_TYPE_LEVEL_MASK ) + type =IRQ_TYPE_LEVEL_HIGH ; + + parent_fwspec = *fwspec; + parent_fwspec.param[1 ] = parent_hwirq; + parent_fwspec.param[2 ] &= ~IRQ_TYPE_SENSE_MASK ; + parent_fwspec.param[2 ] |= type; + parent_fwspec.fwnode = domain->parent->fwnode;This becomes:
Quoted 6 lines
parent_fwspec.fwnode = domain->parent->fwnode; parent_fwspec.param_count = 3; // That's what the GIC wants parent_fwspec.param[0] = 0; //GIC_SPI parent_fwspec.param[1] = parent_hwirq; parent_fwspec.param[2] = type;
Sure.
Quoted 24 lines
Quoted 24 lines
drivers/irqchip/qcom-pdc.c new file +24
-0+ +return irq_domain_alloc_irqs_parent (domain, virq, nr_irqs, + &parent_fwspec); +} + +static const struct irq_domain_ops qcom_pdc_ops = { + .translate = qcom_pdc_translate, + .alloc = qcom_pdc_alloc, + .free = irq_domain_free_irqs_common, +}; + +static int pdc_setup_pin_mapping (struct device_node *np, +struct pdc_pin_data **data) +{ +int ret; +int n, i, j, k, pins =0 ; +int *val; +struct pdc_pin_data *map; + + n =of_property_count_elems_of_size (np,"qcom,pdc-ranges" ,sizeof (u32)); +if (!n || n %3 ) +return -EINVAL ; + + val =kcalloc (n,sizeof (u32),GFP_KERNEL );
Quoted 9 lines
drivers/irqchip/qcom-pdc.c line 256
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+if (!val) +return -ENOMEM ; + + ret =of_property_read_u32_array (np,"qcom,pdc-ranges" , val, n); +if (ret) +return ret;Memory leak.
Ok.
Quoted 10 lines
drivers/irqchip/qcom-pdc.c line 262
Quoted 6 lines
drivers/irqchip/qcom-pdc.c new file +6
-0+ +for (i =0 ; i < n; i +=3 ) + pins += val[i +2 ]; + +if (pins >PDC_MAX_IRQS ) +return -EFAULT ;EFAULT is for an actual page fault. If you get one here, you have bigger problems. EINVAL is better. is You're also leaking memory.
Ok.
Quoted 9 lines
Quoted 9 lines
drivers/irqchip/qcom-pdc.c new file +9
-0+ + map =kcalloc (pins +1 ,sizeof (*map),GFP_KERNEL ); +if (!map) { + ret = -ENOMEM ; + goto fail; + } + +for (i =0 , k =0 ; i < n; i +=3 ) { +for (j =0 ; j < val[i +2 ]; j++, k++) {
Quoted 9 lines
drivers/irqchip/qcom-pdc.c line 275
Quoted 4 lines
drivers/irqchip/qcom-pdc.c new file +4
-0+ map[k].pin = val[i] + j; + map[k].hwirq = val[i +1 ] + j; + } + }That's the thing that needs to go. Just store the ranges as exposed by the DT, maybe with some checking that there is no overlap in the ranges (not that it matters much...).
Makes sense.
Thanks,
Lina
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
In reply to Marc ZyngierQuoted 5 lines · Marc Zyngier
Hi Lina,
Quoted 2 lines · Lina Iyer
From : Archana Sathyakumar <[email protected]>
Quoted 15 lines
drivers/irqchip/qcom-pdc.c line 187
Quoted 7 lines
drivers/irqchip/qcom-pdc.c new file +7
-0+ +static int qcom_pdc_translate (struct irq_domain *d, +struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type) +{ +if (is_of_node (fwspec->fwnode)) { +if (fwspec->param_count <3 ) +return -EINVAL ;Why 3? Reading the DT binding, this is indeed set to 3 without any reason. I'd suggest this becomes 2, encoding the pin number and the trigger information, as the leading 0 is quite useless. Yes, I know other examples in the kernel are using this 0, and that was a consequence of retrofitting the omitted interrupt controllers (back in the days of the stupid
gic_arch_extn...). Don't do that.
Hi Marc
I'm about to send out a new revision of the ls-extirq patchset, and thanks to you pointing me to these patches, I've read the comments on the various revisions of this series and tried to take those into account. But the above confused me, because in response to my first RFC
(https://patchwork.kernel.org/patch/10102643/) we have
Quoted 8 lines · Marc Zyngier
Quoted 7 lines · Alexander Stein
Hi Rasmus,
Review
Quoted 5 lines
(quoted diff) +5
-0+ +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.
Quoted 2 lines
Quoted 2 lines
Do you really need 3 interrupt-cells here? As you've written below
you don't
Quoted 1 line
Quoted 1 line
support PPI anyway the 1st flag might be dropped then. So support
just 2 cells:
Quoted 6 lines
Quoted 2 lines
* IRQ number (IRQ0 - IRQ5)
* IRQ flagsThe 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.
Personally, I'd actually prefer the simpler interrupt specifiers without a redundant 0. Maybe I'm just missing some difference between this case and the ls-extirq one?
Thanks,
Rasmus
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
Hi Rasmus,
Quoted 48 lines · Rasmus Villemoes
Quoted 5 lines · Marc Zyngier
Hi Lina,
Quoted 2 lines · Lina Iyer
From : Archana Sathyakumar <[email protected]>
Quoted 15 lines
drivers/irqchip/qcom-pdc.c line 187
Quoted 7 lines
drivers/irqchip/qcom-pdc.c new file +7
-0+ +static int qcom_pdc_translate (struct irq_domain *d, +struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type) +{ +if (is_of_node (fwspec->fwnode)) { +if (fwspec->param_count <3 ) +return -EINVAL ;Why 3? Reading the DT binding, this is indeed set to 3 without any reason. I'd suggest this becomes 2, encoding the pin number and the trigger information, as the leading 0 is quite useless. Yes, I know other examples in the kernel are using this 0, and that was a consequence of retrofitting the omitted interrupt controllers (back in the days of the stupid
gic_arch_extn...). Don't do that.Hi Marc
I'm about to send out a new revision of the ls-extirq patchset, and thanks to you pointing me to these patches, I've read the comments on the various revisions of this series and tried to take those into account. But the above confused me, because in response to my first RFC
(https://patchwork.kernel.org/patch/10102643/) we haveQuoted 8 lines · Marc Zyngier
Quoted 7 lines · Alexander Stein
Hi Rasmus,
Review
Quoted 5 lines
(quoted diff) +5
-0+ +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 inarm,gic.txt.
Quoted 2 lines
Quoted 2 lines
Do you really need 3 interrupt-cells here? As you've written below
you don't
Quoted 1 line
Quoted 1 line
support PPI anyway the 1st flag might be dropped then. So support
just 2 cells:
Quoted 3 lines
Quoted 2 lines
* IRQ number (IRQ0 - IRQ5)
* IRQ flags
Quoted 7 lines
Quoted 3 lines
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.
Personally, I'd actually prefer the simpler interrupt specifiers without a redundant 0. Maybe I'm just missing some difference between this case and the ls-extirq one?
The difference is that you're adding a new irqchip to an existing DT, and you get some possible breakage. Maybe you'd be happy with the breakage, that's your call (and the maintainer's). In the QC case, it is old brand new, so no harm in doing the right thing from day one.
It is in the end an implementation decision, and you could go either way.
Hope this helps,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
Quoted 43 lines · Marc Zyngier
Hi Rasmus,
Quoted 40 lines · Rasmus Villemoes
Quoted 7 lines · Marc Zyngier
Why 3? Reading the DT binding, this is indeed set to 3 without any reason. I'd suggest this becomes 2, encoding the pin number and the trigger information, as the leading 0 is quite useless. Yes, I know other examples in the kernel are using this 0, and that was a consequence of retrofitting the omitted interrupt controllers (back in the days of the stupid
gic_arch_extn...). Don't do that.Hi Marc
I'm about to send out a new revision of the ls-extirq patchset, and thanks to you pointing me to these patches, I've read the comments on the various revisions of this series and tried to take those into account. But the above confused me, because in response to my first RFC
(https://patchwork.kernel.org/patch/10102643/) we haveQuoted 8 lines · Marc Zyngier
Quoted 7 lines · Alexander Stein
Hi Rasmus,
Review
Quoted 5 lines
(quoted diff) +5
-0+ +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 inarm,gic.txt.
Quoted 2 lines
Quoted 2 lines
Do you really need 3 interrupt-cells here? As you've written below
you don't
Quoted 1 line
Quoted 1 line
support PPI anyway the 1st flag might be dropped then. So support
just 2 cells:
Quoted 6 lines
Quoted 2 lines
* IRQ number (IRQ0 - IRQ5)
* IRQ flagsThe 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.
Quoted 6 lines
Quoted 3 lines
Personally, I'd actually prefer the simpler interrupt specifiers without a redundant 0. Maybe I'm just missing some difference between this case and the ls-extirq one?
The difference is that you're adding a new irqchip to an existing DT, and you get some possible breakage. Maybe you'd be happy with the breakage, that's your call (and the maintainer's).
OK. In the ls1021a case, I actually think "breaking" any existing users if and when they move to the new driver/irqchip is a good thing: the power-on-reset value is such that the lines have the polarity inverted. So there could be some board with a device with either a EDGE_FALLING or LEVEL_LOW interrupt connected to one of the external interrupt lines, which is described in DT by "lying" and using the opposite flag. Changing #interrupt-cells prevents such (ab)users from just changing interrupt-parent and calling it a day.
In the QC case, it is
Quoted 2 lines
old brand new, so no harm in doing the right thing from day one> It is in the end an implementation decision, and you could go either way.
Doing the right thing sounds nice, so I'll go with that :)
Thanks,
Rasmus
RFC PATCH v2 2/3 dt-bindings/interrupt-controller: pdc: descibe PDC device binding
In reply to Lina IyerFrom: Archana Sathyakumar <[email protected]>
Add device binding documentation for the PDC Interrupt controller on QCOM SoC's like the SDM845. The interrupt-controller can be used to sense edge low interrupts and wakeup interrupts when the GIC is non-operational.
--- .../bindings/interrupt-controller/qcom,pdc.txt | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +78 -0
@@ -0,0 +1,78 @@ +PDC interrupt controller + +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture have a +Power Domain Controller (PDC) that is on always-on domain. In addition to +providing power control for the power domains, the hardware also has an +interrupt controller that can be used to help detect edge low interrupts as +well detect interrupts when the GIC is non-operational. + +GIC is parent interrupt controller at the highest level. Platform interrupt +controller PDC is next in hierarchy, followed by others. Drivers requiring +wakeup capabilities of their device interrupts routed through the PDC, must +specify PDC as their interrupt controller and request the PDC port associated +with the GIC interrupt. See example below. + +Properties: + +- compatible: + Usage: required + Value type: <string> + Definition: Should contain "qcom,<soc>-pdc" + - "qcom,sdm845-pdc": For SDM845 + +- reg: + Usage: required + Value type: <prop-encoded-array> + Definition: Specifies the base physical address for PDC hardware. + +- interrupt-cells: + Usage: required + Value type: <u32> + Definition: Specifies the number of cells needed to encode an interrupt + source. + The value must match that of the parent interrupt + controller defined in the DT. + The encoding of these cells are same as described in [1]. + +- interrupt-parent: + Usage: required + Value type: <phandle> + Definition: Specifies the interrupt parent necessary for hierarchical + domain to operate. + +- interrupt-controller: + Usage: required + Value type: <bool> + Definition: Identifies the node as an interrupt controller. + +- qcom,pdc-range: + 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 staring PDC port num. + The second element is the hwirq number for the PDC port. + The third element is the number of elements in sequence. + +Example: + + pdc: interrupt-controller@b220000 { + compatible = "qcom,sdm845-pdc"; + reg = <0xb220000 0x30000>; + qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>; + #interrupt-cells = <3>; + interrupt-parent = <&intc>; + interrupt-controller; + }; + +The DT binding of a device that wants to use the GIC SPI 514 as a wakeup +interrupt, would look like this - + + wake-device { + [...] + interrupt-parent = <&pdc>; + interrupt = <0 2 0>; + }; + +[1]. Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 2/3] dt-bindings/interrupt-controller: pdc: descibe PDC device binding
Quoted 46 lines · Lina Iyer
From: Archana Sathyakumar <[email protected]>
Add device binding documentation for the PDC Interrupt controller on QCOM SoC's like the SDM845. The interrupt-controller can be used to sense edge low interrupts and wakeup interrupts when the GIC is non-operational.
Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>--- .../bindings/interrupt-controller/qcom,pdc.txt | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txtDocumentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +25
-0@@ -0,0 +1,78 @@ +PDC interrupt controller + +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture have a +Power Domain Controller (PDC) that is on always-on domain. In addition to +providing power control for the power domains, the hardware also has an +interrupt controller that can be used to help detect edge low interrupts as +well detect interrupts when the GIC is non-operational. + +GIC is parent interrupt controller at the highest level. Platform interrupt +controller PDC is next in hierarchy, followed by others. Drivers requiring +wakeup capabilities of their device interrupts routed through the PDC, must +specify PDC as their interrupt controller and request the PDC port associated +with the GIC interrupt. See example below. + +Properties: + +- compatible: + Usage: required + Value type: <string> + Definition: Should contain "qcom,<soc>-pdc" + - "qcom,sdm845-pdc": For SDM845 + +- reg: + Usage: required + Value type: <prop-encoded-array>
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt line 35
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +10
-0+ Definition: Specifies the base physical address for PDC hardware. + +- interrupt-cells: + Usage: required + Value type: <u32> + Definition: Specifies the number of cells needed to encode an interrupt + source. + The value must match that of the parent interrupt + controller defined in the DT. + The encoding of these cells are same as described in [1].
There shouldn't be such a requirement. These are two independent pieces of HW, and the first parameter doesn't mean anything for the PDC.
Quoted 30 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +30
-0+ +- interrupt-parent: + Usage: required + Value type: <phandle> + Definition: Specifies the interrupt parent necessary for hierarchical + domain to operate. + +- interrupt-controller: + Usage: required + Value type: <bool> + Definition: Identifies the node as an interrupt controller. + +- qcom,pdc-range: + 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 staring PDC port num. + The second element is the hwirq number for the PDC port. + The third element is the number of elements in sequence. + +Example: + + pdc: interrupt-controller@b220000 { + compatible = "qcom,sdm845-pdc"; + reg = <0xb220000 0x30000>; + qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>; + #interrupt-cells = <3>; + interrupt-parent = <&intc>;
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt line 75
Quoted 10 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +10
-0+ interrupt-controller; + }; + +The DT binding of a device that wants to use the GIC SPI 514 as a wakeup +interrupt, would look like this - + + wake-device { + [...] + interrupt-parent = <&pdc>; + interrupt = <0 2 0>;
Again: 0 is not a valid trigger value.
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt line 78
Quoted 4 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +3
-0+ }; + +[1]. Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH RFC v2 2/3] dt-bindings/interrupt-controller: pdc: descibe PDC device binding
Quoted 52 lines · Marc Zyngier
Quoted 51 lines · Lina Iyer
From: Archana Sathyakumar <[email protected]>
Add device binding documentation for the PDC Interrupt controller on QCOM SoC's like the SDM845. The interrupt-controller can be used to sense edge low interrupts and wakeup interrupts when the GIC is non-operational.
Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>--- .../bindings/interrupt-controller/qcom,pdc.txt | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txtDocumentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +30
-0@@ -0,0 +1,78 @@ +PDC interrupt controller + +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture have a +Power Domain Controller (PDC) that is on always-on domain. In addition to +providing power control for the power domains, the hardware also has an +interrupt controller that can be used to help detect edge low interrupts as +well detect interrupts when the GIC is non-operational. + +GIC is parent interrupt controller at the highest level. Platform interrupt +controller PDC is next in hierarchy, followed by others. Drivers requiring +wakeup capabilities of their device interrupts routed through the PDC, must +specify PDC as their interrupt controller and request the PDC port associated +with the GIC interrupt. See example below. + +Properties: + +- compatible: + Usage: required + Value type: <string> + Definition: Should contain "qcom,<soc>-pdc" + - "qcom,sdm845-pdc": For SDM845 + +- reg: + Usage: required + Value type: <prop-encoded-array> + Definition: Specifies the base physical address for PDC hardware. + +- interrupt-cells: + Usage: required + Value type: <u32>
Quoted 9 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt line 35
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +5
-0+ Definition: Specifies the number of cells needed to encode an interrupt + source. + The value must match that of the parent interrupt + controller defined in the DT. + The encoding of these cells are same as described in [1].There shouldn't be such a requirement. These are two independent pieces of HW, and the first parameter doesn't mean anything for the PDC.
Wouldn't that be confusing - that we have different definitions for interrupts in the same DT? I agree that they are different interrupt controllers, but it just feels odd.
I will change this to just take 2 cells.
Quoted 34 lines
Quoted 34 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +34
-0+ +- interrupt-parent: + Usage: required + Value type: <phandle> + Definition: Specifies the interrupt parent necessary for hierarchical + domain to operate. + +- interrupt-controller: + Usage: required + Value type: <bool> + Definition: Identifies the node as an interrupt controller. + +- qcom,pdc-range: + 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 staring PDC port num. + The second element is the hwirq number for the PDC port. + The third element is the number of elements in sequence. + +Example: + + pdc: interrupt-controller@b220000 { + compatible = "qcom,sdm845-pdc"; + reg = <0xb220000 0x30000>; + qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>; + #interrupt-cells = <3>; + interrupt-parent = <&intc>; + interrupt-controller; + }; + +The DT binding of a device that wants to use the GIC SPI 514 as a wakeup
Quoted 9 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt line 75
Quoted 6 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +6
-0+interrupt, would look like this - + + wake-device { + [...] + interrupt-parent = <&pdc>; + interrupt = <0 2 0>;Again: 0 is not a valid trigger value.
Ok.
Thanks,
Lina
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 2/3] dt-bindings/interrupt-controller: pdc: descibe PDC device binding
Quoted 62 lines · Lina Iyer
Quoted 52 lines · Marc Zyngier
Quoted 51 lines · Lina Iyer
From: Archana Sathyakumar <[email protected]>
Add device binding documentation for the PDC Interrupt controller on QCOM SoC's like the SDM845. The interrupt-controller can be used to sense edge low interrupts and wakeup interrupts when the GIC is non-operational.
Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>--- .../bindings/interrupt-controller/qcom,pdc.txt | 78 ++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txtDocumentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +30
-0@@ -0,0 +1,78 @@ +PDC interrupt controller + +Qualcomm Technologies Inc. SoCs based on the RPM Hardened architecture have a +Power Domain Controller (PDC) that is on always-on domain. In addition to +providing power control for the power domains, the hardware also has an +interrupt controller that can be used to help detect edge low interrupts as +well detect interrupts when the GIC is non-operational. + +GIC is parent interrupt controller at the highest level. Platform interrupt +controller PDC is next in hierarchy, followed by others. Drivers requiring +wakeup capabilities of their device interrupts routed through the PDC, must +specify PDC as their interrupt controller and request the PDC port associated +with the GIC interrupt. See example below. + +Properties: + +- compatible: + Usage: required + Value type: <string> + Definition: Should contain "qcom,<soc>-pdc" + - "qcom,sdm845-pdc": For SDM845 + +- reg: + Usage: required + Value type: <prop-encoded-array> + Definition: Specifies the base physical address for PDC hardware. + +- interrupt-cells: + Usage: required + Value type: <u32>Quoted 9 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt line 35
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt new file +5
-0+ Definition: Specifies the number of cells needed to encode an interrupt + source. + The value must match that of the parent interrupt + controller defined in the DT. + The encoding of these cells are same as described in [1].There shouldn't be such a requirement. These are two independent pieces of HW, and the first parameter doesn't mean anything for the PDC.
Quoted 3 lines
Wouldn't that be confusing - that we have different definitions for interrupts in the same DT? I agree that they are different interrupt controllers, but it just feels odd.
I think it feels more bizarre to have pointless fields in the interrupt specifier. And most DTs have some sort of secondary interrupt controller that only take two (or even one) parameters. I don't think we should treat this any differently.
Quoted 1 line
I will change this to just take 2 cells.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
RFC PATCH v2 3/3 drivers: irqchip: pdc: log PDC info in FTRACE
In reply to Lina IyerFrom: Archana Sathyakumar <[email protected]>
Log key PDC pin configuration in FTRACE.
--- drivers/irqchip/qcom-pdc.c | 7 ++++++ include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 include/trace/events/pdc.h
drivers/irqchip/qcom-pdc.c +7 -0
@@ -26,6 +26,8 @@#include <linux/platform_device.h> #include <linux/slab.h> #include <linux/types.h> +#define CREATE_TRACE_POINTS +#include "trace/events/pdc.h" #define PDC_MAX_IRQS 126
@@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on) enable = on ?ENABLE_INTR (enable, mask) :CLEAR_INTR (enable, mask);pdc_reg_write (IRQ_ENABLE_BANK , index, enable);spin_unlock_irqrestore (&pdc_lock, flags); + +trace_irq_pin_config (PDC_ENTRY , pin_out, (u64 )d->chip_data,0 , on); }static void qcom_pdc_gic_mask (struct irq_data *d)
@@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)pdc_reg_write (IRQ_i_CFG, pin_out, pdc_type); +trace_irq_pin_config (PDC_TYPE_CONFIG , pin_out, (u64 )d->chip_data, + pdc_type,0 ); +return irq_chip_set_type_parent (d, type); }
include/trace/events/pdc.h new file +55 -0
55 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
include/trace/events/pdc.h line 10
Quoted 12 lines · Lina Iyer
+++
b/include/trace/events/pdc.hinclude/trace/events/pdc.h new file +10
-0@@ -0,0 +1,55 @@ +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details.
Can you please use the proper SPDX identifiers instead of the boiler plate? Same for the driver source file.
Quoted 4 lines
include/trace/events/pdc.h new file +4
-0+*/ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM pdc
include/trace/events/pdc.h line 24
Quoted 10 lines
include/trace/events/pdc.h new file +10
-0+ +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ ) +#define _TRACE_PDC_H_ + +#include <linux/tracepoint.h> + +#define PDC_ENTRY 1 +#define PDC_TYPE_CONFIG 2 + +TRACE_EVENT(irq_pin_config,
This is really a too generic name for a PDC specific breakpoint.
Aside of that the question is whether this really justifies a trace point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS infrastructure to make this accessible via debugfs?
Thanks,
tglx
Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
Quoted 17 lines · Thomas Gleixner
include/trace/events/pdc.h line 10
Quoted 12 lines · Lina Iyer
+++
b/include/trace/events/pdc.hinclude/trace/events/pdc.h new file +10
-0@@ -0,0 +1,55 @@ +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. Can you please use the proper SPDX identifiers instead of the boiler plate? Same for the driver source file.
Sure.
Quoted 8 lines
Quoted 8 lines
include/trace/events/pdc.h new file +8
-0+*/ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM pdc + +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ ) +#define _TRACE_PDC_H_ +
Quoted 9 lines
include/trace/events/pdc.h line 24
Quoted 6 lines
include/trace/events/pdc.h new file +6
-0+#include <linux/tracepoint.h> + +#define PDC_ENTRY 1 +#define PDC_TYPE_CONFIG 2 + +TRACE_EVENT(irq_pin_config,This is really a too generic name for a PDC specific breakpoint.
Hmm.. right.
Quoted 5 lines
Aside of that the question is whether this really justifies a trace point. Wouldn't it be sufficient to use the
GENERIC_IRQ_DEBUGFSinfrastructure to make this accessible via debugfs?
Some product configurations disable debugfs. I will ask around if this can be dropped.
-- Lina
Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
Quoted 39 lines · Lina Iyer
Quoted 17 lines · Thomas Gleixner
include/trace/events/pdc.h line 10
Quoted 12 lines · Lina Iyer
+++
b/include/trace/events/pdc.hinclude/trace/events/pdc.h new file +10
-0@@ -0,0 +1,55 @@ +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. Can you please use the proper SPDX identifiers instead of the boiler plate? Same for the driver source file.
Sure.
Quoted 8 lines
Quoted 8 lines
include/trace/events/pdc.h new file +8
-0+*/ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM pdc + +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ ) +#define _TRACE_PDC_H_ +Quoted 9 lines
include/trace/events/pdc.h line 24
Quoted 6 lines
include/trace/events/pdc.h new file +6
-0+#include <linux/tracepoint.h> + +#define PDC_ENTRY 1 +#define PDC_TYPE_CONFIG 2 + +TRACE_EVENT(irq_pin_config,This is really a too generic name for a PDC specific breakpoint.
Hmm.. right.
Quoted 8 lines
Quoted 5 lines
Aside of that the question is whether this really justifies a trace point. Wouldn't it be sufficient to use the
GENERIC_IRQ_DEBUGFSinfrastructure to make this accessible via debugfs?Some product configurations disable debugfs. I will ask around if this can be dropped.
Memory dumps after a crash have support for FTRACE and it helps greatly on production issues. Hence the preference for FTRACE support.
-- Lina
Steven Rostedt
maintainerRe: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
Quoted 5 lines · On Mon, 5 Feb 2018 15:18:46 +0000 Lina Iyer <[email protected]> wrote:
Quoted 3 lines
Some product configurations disable debugfs. I will ask around if this can be dropped.
Memory dumps after a crash have support for FTRACE and it helps greatly on production issues. Hence the preference for FTRACE support.
Note, ftrace no longer depends on debugfs. It has its own tracefs file system:
# mount -t tracefs nodev /sys/kernel/tracing
-- Steve
Steven Rostedt
maintainerRe: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
In reply to Lina IyerOn Fri, 2 Feb 2018 07:22:00 -0700
Lina Iyer <[email protected]> wrote:
Hi Lina,
This looks really good. I have one nit below.
Quoted 36 lines
From: Archana Sathyakumar <[email protected]>
Log key PDC pin configuration in FTRACE.
Cc Steven Rostedt <[email protected]>Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>--- drivers/irqchip/qcom-pdc.c | 7 ++++++ include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 include/trace/events/pdc.hdrivers/irqchip/qcom-pdc.c +4
-0@@ -26,6 +26,8 @@#include <linux/platform_device.h> #include <linux/slab.h> #include <linux/types.h> +#define CREATE_TRACE_POINTS +#include "trace/events/pdc.h" #define PDC_MAX_IRQS 126@@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on) enable = on ?ENABLE_INTR (enable, mask) :CLEAR_INTR (enable, mask);pdc_reg_write (IRQ_ENABLE_BANK , index, enable);spin_unlock_irqrestore (&pdc_lock, flags); + +trace_irq_pin_config (PDC_ENTRY , pin_out, (u64 )d->chip_data,0 , on); }static void qcom_pdc_gic_mask (struct irq_data *d)
drivers/irqchip/qcom-pdc.c static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type) · line 157
Quoted 5 lines
drivers/irqchip/qcom-pdc.c +2
-0@@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)pdc_reg_write (IRQ_i_CFG, pin_out, pdc_type); +trace_irq_pin_config (PDC_TYPE_CONFIG , pin_out, (u64 )d->chip_data, + pdc_type,0 );
I wonder if it makes more sense to just pass 'd' into the trace events, and then do the dereference there. The reason is to try to get as much code out of the calling path as possible. Even though trace events use jump labels and have no conditional branches, the code to call the function is still within the code using the trace events. By passing in 'd' and doing the redirect in the trace event code, we remove the setting up of the redirect from the caller, and save some cache lines in the process.
-- Steve
Quoted 65 lines
drivers/irqchip/qcom-pdc.c +1
-0+return irq_chip_set_type_parent (d, type); }include/trace/events/pdc.h new file +55
-0@@ -0,0 +1,55 @@ +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM pdc + +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ ) +#define _TRACE_PDC_H_ + +#include <linux/tracepoint.h> + +#define PDC_ENTRY 1 +#define PDC_TYPE_CONFIG 2 + +TRACE_EVENT (irq_pin_config, + + TP_PROTO(u32 func, int pin, u64 hwirq, u32 type, u32 enable), + +TP_ARGS (func, pin, hwirq, type, enable), + +TP_STRUCT__entry ( + __field(u32, func) + __field(int, pin) + __field(u64, hwirq) + __field(u32, type) +__field (u32, enable) + ), + +TP_fast_assign ( + __entry->func = func; + __entry->pin = pin; + __entry->hwirq = hwirq; + __entry->type = type; + __entry->enable = enable; + ), + +TP_printk ("%s hwirq:%lu pin:%d type:%u enable:%u" , +print_symbolic (__entry->func, + { PDC_ENTRY, "pdc_entry" }, + { PDC_TYPE_CONFIG, "pdc_type_config" }), + __entry->pin, __entry->hwirq, __entry->type, __entry->enable) +); + +#endif +#define TRACE_INCLUDE_FILE pdc +#include <trace/define_trace.h>
Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
Quoted 47 lines · Steven Rostedt
On Fri, 2 Feb 2018 07:22:00 -0700
Lina Iyer <[email protected]> wrote:Hi Lina,
This looks really good. I have one nit below.
Quoted 39 lines
From: Archana Sathyakumar <[email protected]>
Log key PDC pin configuration in FTRACE.
Cc Steven Rostedt <[email protected]>Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>--- drivers/irqchip/qcom-pdc.c | 7 ++++++ include/trace/events/pdc.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 include/trace/events/pdc.hdrivers/irqchip/qcom-pdc.c +4
-0@@ -26,6 +26,8 @@#include <linux/platform_device.h> #include <linux/slab.h> #include <linux/types.h> +#define CREATE_TRACE_POINTS +#include "trace/events/pdc.h" #define PDC_MAX_IRQS 126@@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool on) enable = on ?ENABLE_INTR (enable, mask) :CLEAR_INTR (enable, mask);pdc_reg_write (IRQ_ENABLE_BANK , index, enable);spin_unlock_irqrestore (&pdc_lock, flags); + +trace_irq_pin_config (PDC_ENTRY , pin_out, (u64 )d->chip_data,0 , on); }static void qcom_pdc_gic_mask (struct irq_data *d)@@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)pdc_reg_write (IRQ_i_CFG, pin_out, pdc_type);
Quoted 11 lines
drivers/irqchip/qcom-pdc.c static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type) · line 157
Quoted 2 lines
drivers/irqchip/qcom-pdc.c +2
-0+trace_irq_pin_config (PDC_TYPE_CONFIG , pin_out, (u64 )d->chip_data, + pdc_type,0 );I wonder if it makes more sense to just pass 'd' into the trace events, and then do the dereference there. The reason is to try to get as much code out of the calling path as possible. Even though trace events use jump labels and have no conditional branches, the code to call the function is still within the code using the trace events. By passing in 'd' and doing the redirect in the trace event code, we remove the setting up of the redirect from the caller, and save some cache lines in the process.
Makes sense. Will fix it.
Thanks Steve.
-- Lina
Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
In reply to Lina IyerHi Steve,
Quoted 18 lines · Lina Iyer
From: Archana Sathyakumar <[email protected]>
Log key PDC pin configuration in FTRACE.
Cc Steven Rostedt <[email protected]>Signed-off-by Archana Sathyakumar <[email protected]>Signed-off-by Lina Iyer <[email protected]>---
drivers/irqchip/qcom-pdc.c| 7 ++++++include/trace/events/pdc.h| 55 ++++++++++++++++++++++++++++++++++++++++++++++2 files changed, 62 insertions(+)create mode 100644
include/trace/events/pdc.hdrivers/irqchip/qcom-pdc.c +0
-0
drivers/irqchip/qcom-pdc.c line 31
Quoted 6 lines
drivers/irqchip/qcom-pdc.c +2
-0@@ -26,6 +26,8 @@#include <linux/platform_device.h> #include <linux/slab.h> #include <linux/types.h> +#define CREATE_TRACE_POINTS +#include "trace/events/pdc.h"
In addition to this PDC patch, there are a few drivers with FTRACE support coming for up QCOM SoCs. Would it make sense to open up a new sub-folder for SoC specific FTRACE like say, trace/events/soc/
trace/events/soc/qcom/
What would be your recommendation?
At the very least, I am thinking of renaming this file to trace/events/qcom-pdc.h.
Thanks,
Lina
Steven Rostedt
maintainerRe: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE
Quoted 17 lines · On Mon, 5 Feb 2018 15:50:48 +0000 Lina Iyer <[email protected]> wrote:
drivers/irqchip/qcom-pdc.c line 31
Quoted 11 lines
drivers/irqchip/qcom-pdc.c +2
-0@@ -26,6 +26,8 @@#include <linux/platform_device.h> #include <linux/slab.h> #include <linux/types.h> +#define CREATE_TRACE_POINTS +#include "trace/events/pdc.h" In addition to this PDC patch, there are a few drivers with FTRACE support coming for up QCOM SoCs. Would it make sense to open up a new sub-folder for SoC specific FTRACE like say, trace/events/soc/
trace/events/soc/qcom/
Quoted 4 lines
What would be your recommendation?
At the very least, I am thinking of renaming this file to
trace/events/qcom-pdc.h.
I'm thinking it makes more sense to create the trace.h file inside the drivers/irqchip/ directory. xfs does this, as well as other locations.
The include/trace/events/ was more of a generic method, but when events start getting more specific, they should be in their own directories.
See samples/trace_events/* on how to have the file locally (or look at xfs, search for trace in fs/xfs/Makefile).
-- Steve