qcom: GPIO IRQ wakeup using PDC irqchip
Subsystems (7)
v3 cover qcom: GPIO IRQ wakeup using PDC irqchip
Hi,
This is an attempt at using GPIO as wake up sources. Based on discussions with Stephen and Marc [1], the idea that is used here is to make PDC interrupt controller the parent of TLMM irqchip. Wakeup capable GPIO IRQs have corresponding parent interrupt in PDC and therefore the GIC. The TLMM irqchip has a summary line into the GIC for all regular non-wakeup interrupt.
This idea uses Thierry's hierarchical GPIO [2] and is dependent on [3]. Also PDC device bindings published in [4] is needed.
Kindly review the series.
Thanks,
Lina
[1]. https://lore.kernel.org/patchwork/patch/998238/
[2]. https://lkml.org/lkml/2018/9/21/471
[3]. https://lkml.org/lkml/2018/9/21/474
[4]. https://lore.kernel.org/patchwork/patch/1008046/
Lina Iyer (3):
drivers: pinctrl: msm: setup gpio irqchip in hierarchy with pdc
irqchip
dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
arm64: dts: msm: add PDC wake irq maps for GPIOs for SDM845
.../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 31 ++++- arch/arm64/boot/dts/qcom/sdm845.dtsi | 18 +++ drivers/pinctrl/qcom/pinctrl-msm.c | 125 +++++++++++++++++- 3 files changed, 169 insertions(+), 5 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
RFC v3 1/3 drivers: pinctrl: msm: setup gpio irqchip in hierarchy with pdc irqchip
--- drivers/pinctrl/qcom/pinctrl-msm.c | 125 ++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 4 deletions(-)
drivers/pinctrl/qcom/pinctrl-msm.c +121 -4
@@ -17,6 +17,7 @@#include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_irq.h> #include <linux/platform_device.h> #include <linux/pinctrl/machine.h> #include <linux/pinctrl/pinctrl.h>
@@ -735,6 +736,9 @@ static void msm_gpio_irq_mask(struct irq_data *d)clear_bit (d->hwirq, pctrl->enabled_irqs);raw_spin_unlock_irqrestore (&pctrl->lock, flags); + +if (d->parent_data) +irq_chip_mask_parent (d); }static void msm_gpio_irq_unmask (struct irq_data *d)
@@ -757,6 +761,9 @@ static void msm_gpio_irq_unmask(struct irq_data *d)set_bit (d->hwirq, pctrl->enabled_irqs);raw_spin_unlock_irqrestore (&pctrl->lock, flags); + +if (d->parent_data) +irq_chip_unmask_parent (d); }static void msm_gpio_irq_ack (struct irq_data *d)
@@ -875,6 +882,9 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)else if (type & (IRQ_TYPE_EDGE_FALLING |IRQ_TYPE_EDGE_RISING ))irq_set_handler_locked (d, handle_edge_irq); +if (d->parent_data) +irq_chip_set_type_parent (d, type); +return 0 ; }
@@ -890,6 +900,9 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)raw_spin_unlock_irqrestore (&pctrl->lock, flags); +if (d->parent_data) +irq_chip_set_wake_parent (d, on); +return 0 ; }
@@ -967,11 +980,103 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)return device_property_read_u16_array (pctrl->dev,"gpios" ,NULL ,0 ) >0 ; } +static int get_parent_hwirq (struct msm_pinctrl *pctrl,unsigned int hwirq) +{ +int i, n, ret; +int gpio, pdc = -EINVAL ; +struct device_node *np = pctrl->dev->of_node; + + n =of_property_count_elems_of_size (np,"wake-irq" ,sizeof (u32)); +if (n <=0 || n %2 ) +return -EINVAL ; + +for (i =0 ; i < n /2 ; i++) { + ret =of_property_read_u32_index (np,"wake-irq" ,2 * i, &gpio); +if (ret) +return ret; +if (gpio != hwirq) +continue ; + ret =of_property_read_u32_index (np,"wake-irq" ,2 * i +1 , + &pdc); +if (ret) +return ret; + } + +return pdc; +} + +static int msm_gpio_domain_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 <2 ) +return -EINVAL ; + *hwirq = fwspec->param[0 ]; + *type = fwspec->param[1 ] &IRQ_TYPE_SENSE_MASK ; +return 0 ; + } + +return -EINVAL ; +} + +static int msm_gpio_domain_alloc (struct irq_domain *domain,unsigned int virq, +unsigned int nr_irqs,void *arg) +{ +int ret =0 ; +irq_hw_number_t hwirq; +int parent_hwirq; +struct gpio_chip *gc = domain->host_data; +struct msm_pinctrl *pctrl =gpiochip_get_data (gc); +struct irq_fwspec parent_fwspec, *fwspec = arg; +unsigned int type; + + ret =msm_gpio_domain_translate (domain, fwspec, &hwirq, &type); +if (ret) +return ret; + + parent_hwirq =get_parent_hwirq (pctrl, hwirq); +if (parent_hwirq <0 ) +return 0 ; + + ret =irq_domain_set_hwirq_and_chip (domain, virq, hwirq, + &pctrl->irq_chip, gc); +if (ret <0 ) +return ret; + + parent_fwspec.fwnode = domain->parent->fwnode; + parent_fwspec.param_count =2 ; + parent_fwspec.param[0 ] = parent_hwirq; + parent_fwspec.param[1 ] = type; + +return irq_domain_alloc_irqs_parent (domain, virq, nr_irqs, + &parent_fwspec); +} + +static int msm_gpio_to_irq (struct gpio_chip *chip,unsigned int offset) +{ +struct irq_fwspec fwspec; + + fwspec.fwnode =of_node_to_fwnode (chip->of_node); + fwspec.param[0 ] = offset; + fwspec.param[1 ] =IRQ_TYPE_LEVEL_HIGH ; + fwspec.param_count =2 ; + +return irq_create_fwspec_mapping (&fwspec); +} + +static const struct irq_domain_ops msm_gpiod_ops = { + .translate = msm_gpio_domain_translate, + .alloc = msm_gpio_domain_alloc, + .free = irq_domain_free_irqs_top, +}; +static int msm_gpio_init (struct msm_pinctrl *pctrl) {struct gpio_chip *chip;int ret;unsigned ngpio = pctrl->soc->ngpios; +struct device_node *dn;if (WARN_ON (ngpio >MAX_NR_GPIO ))return -EINVAL ;
@@ -1015,9 +1120,19 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) dev_name(pctrl->dev), 0, 0, chip->ngpio);if (ret) {dev_err (pctrl->dev,"Failed to add pin range\n" ); -gpiochip_remove (&pctrl->chip); -return ret; + goto fail; + } + } + + dn =of_parse_phandle (pctrl->dev->of_node,"wakeup-parent" ,0 ); +if (dn) { + chip->irq.parent_domain =irq_find_host (dn); +of_node_put (dn); +if (!chip->irq.parent_domain) { + ret = -EPROBE_DEFER ; + goto fail; } + chip->to_irq = msm_gpio_to_irq; } ret = gpiochip_irqchip_add(chip,
@@ -1027,14 +1142,16 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) IRQ_TYPE_NONE);if (ret) {dev_err (pctrl->dev,"Failed to add irqchip to gpiochip\n" ); -gpiochip_remove (&pctrl->chip); -return -ENOSYS ; + goto fail; }gpiochip_set_chained_irqchip (chip, &pctrl->irq_chip, pctrl->irq, msm_gpio_irq_handler);return 0 ; +fail: +gpiochip_remove (&pctrl->chip); +return ret; } static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
RFC v3 2/3 dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
In reply to Lina IyerSDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.
--- .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +30 -1
@@ -29,6 +29,17 @@ SDM845 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/interrupt-controller/irq.h> +- wakeup-parent: + Usage: optional + Value type: <phandle> + Definition: A phandle to the wakeup interrupt controller for the SoC. + +- wakeup-irq: + Usage: optional: + Value type: <u32 array> + Definition: Specifies the map of the gpio and the corresponding PDC + output port. + - gpio-controller: Usage: required Value type: <none>
@@ -53,7 +64,6 @@ pin, a group, or a list of pins or groups. This configuration can include the mux function to select on those pin(s)/group(s), and various pin configuration parameters, such as pull-up, drive strength, etc. - PIN CONFIGURATION NODES: The name of each subnode is not important; all subnodes should be enumerated
@@ -160,6 +170,25 @@ Example: #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; + wake-parent = <&pdc>; + wake-irq = + <1 30>, <3 31>, <5 32>, <10 33>, <11 34>, + <20 35>, <22 36>, <24 37>, <26 38>, <30 39>, + <31 117>, <32 41>, <34 42>, <36 43>, <37 44>, + <38 45>, <39 46>, <40 47>, <41 115>, <43 49>, + <44 50>, <46 51>, <48 52>, <49 118>, <52 54>, + <53 55>, <54 56>, <56 57>, <57 58>, <58 59>, + <59 60>, <60 61>, <61 62>, <62 63>, <63 64>, + <64 65>, <66 66>, <68 67>, <71 68>, <73 69>, + <77 70>, <78 71>, <79 72>, <80 73>, <84 74>, + <85 75>, <86 76>, <88 77>, <89 116>, <91 79>, + <92 80>, <95 81>, <96 82>, <97 83>, <101 84>, + <103 85>, <104 86>, <115 90>, <116 91>, + <117 92>, <118 93>, <119 94>, <120 95>, + <121 96>, <122 97>, <123 98>, <124 99>, + <125 100>, <127 102>, <128 103>, <129 104>, + <130 105>, <132 106>, <133 107>, <145 108>; + qup9_active: qup9-active { mux {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoting Lina Iyer (2018-11-20 16:06:47)
Quoted 14 lines
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.
Signed-off-by Lina Iyer <[email protected]>--- .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +0
-0
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 37
Quoted 9 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +6
-0@@ -29,6 +29,17 @@ SDM845 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/interrupt-controller/irq.h> +- wakeup-parent: + Usage: optional + Value type: <phandle> + Definition: A phandle to the wakeup interrupt controller for the SoC. + +- wakeup-irq:
This shouldn't be needed. TLMM driver can probe for the possibility of wakeup capable irqs from irq allocation step. The only place we should need to know what TLMM pins map to what PDC lines is in the PDC driver.
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 18 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-20 16:06:47)
Quoted 17 lines
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.
Signed-off-by Lina Iyer <[email protected]>--- .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +0
-0@@ -29,6 +29,17 @@ SDM845 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/interrupt-controller/irq.h>
Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 37
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +6
-0+- wakeup-parent: + Usage: optional + Value type: <phandle> + Definition: A phandle to the wakeup interrupt controller for the SoC. + +- wakeup-irq:This shouldn't be needed. TLMM driver can probe for the possibility of wakeup capable irqs from irq allocation step. The only place we should need to know what TLMM pins map to what PDC lines is in the PDC driver.
Why? Every driver seems to translate the hardware IRQ and pass it to it's parent. Why should this be any different ? The PDC is an interrupt controller that just knows an interrupt port and maps it to the GIC. Not sure, I understand the reasoning for this. It seems to add more confusing relationship with the PDC interrupt controller, that way.
-- Lina
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoting Lina Iyer (2018-11-26 08:14:55)
Quoted 30 lines · Stephen Boyd
Quoted 18 lines
Quoting Lina Iyer (2018-11-20 16:06:47)
Quoted 17 lines
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.
Signed-off-by Lina Iyer <[email protected]>--- .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +0
-0@@ -29,6 +29,17 @@ SDM845 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/interrupt-controller/irq.h>Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 37
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +6
-0+- wakeup-parent: + Usage: optional + Value type: <phandle> + Definition: A phandle to the wakeup interrupt controller for the SoC. + +- wakeup-irq:This shouldn't be needed. TLMM driver can probe for the possibility of wakeup capable irqs from irq allocation step. The only place we should need to know what TLMM pins map to what PDC lines is in the PDC driver.
Quoted 6 lines
Why? Every driver seems to translate the hardware IRQ and pass it to it's parent. Why should this be any different ? The PDC is an interrupt controller that just knows an interrupt port and maps it to the GIC. Not sure, I understand the reasoning for this. It seems to add more confusing relationship with the PDC interrupt controller, that way.
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
I'm not sure why Marc wanted the pdc to GIC SPI line mapping to come from DT. I guess there were some contiguous ranges in there so making the mapping in DT works OK enough because there are only a few ranges to handle, but the GPIO map seems quite scattered so I don't see much of a benefit to trying to compress it.
I'm hacking on this code to make it all nice and clean, but here's my current WIP patch that at least works well enough for me to get gpio irqs at runtime. I haven't tested the suspend path yet, but presumably if irqs are working at runtime and the device isn't suspended then any wakeup irqs will be kept unmasked at the PDC and things would "just work". This is on top of these patches and the gpio hierarchical irqdomain patch.
A couple suggestions for starting points:
1) We could have PDC get a phandle to TLMM so that we can match up the irq domain in PDC to the irq domain inside TLMM with a irq domain ops::match/select function instead of using a token to match this up.
2) The PDC driver could be a little more friendly about globals and actually pass around pointers to things via the irq domain void pointer member.
3) We could have the GPIO PDC domain be a child domain of the main PDC domain and just map between GPIO number space and PDC number space. Otherwise we duplicate some code between the two alloc implementations for trigger type remapping.
---8<----
drivers/irqchip/qcom-pdc.c +167 -10
@@ -13,12 +13,13 @@#include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/soc/qcom/irq.h> #include <linux/spinlock.h> -#include <linux/platform_device.h> #include <linux/slab.h> #include <linux/types.h> #define PDC_MAX_IRQS 126 +#define PDC_MAX_GPIO_IRQS 256#define CLEAR_INTR (reg, intr) (reg & ~(1 << intr))#define ENABLE_INTR (reg, intr) (reg | (1 << intr))
@@ -32,6 +33,11 @@ struct pdc_pin_region {u32 cnt; }; +struct pdc_gpio_pin_map { +unsigned int gpio; +u32 pdc_pin; +}; +static DEFINE_RAW_SPINLOCK (pdc_lock);static void __iomem *pdc_base;static struct pdc_pin_region *pdc_region;
@@ -47,9 +53,8 @@ static u32 pdc_reg_read(int reg, u32 i)return readl_relaxed (pdc_base + reg + i *sizeof (u32)); } -static void pdc_enable_intr (struct irq_data *d,bool on) +static void pdc_enable_intr (irq_hw_number_t pin_out,bool on) { -int pin_out = d->hwirq;u32 index, mask;u32 enable;
@@ -65,13 +70,23 @@ static void pdc_enable_intr(struct irq_data *d, bool on)static void qcom_pdc_gic_mask (struct irq_data *d) { -pdc_enable_intr (d, false); +irq_hw_number_t hwirq = d->hwirq; + +if (hwirq ==ULONG_MAX ) +return ; + +pdc_enable_intr (hwirq, false);irq_chip_mask_parent (d); }static void qcom_pdc_gic_unmask (struct irq_data *d) { -pdc_enable_intr (d, true); +irq_hw_number_t hwirq = d->hwirq; + +if (hwirq ==ULONG_MAX ) +return ; + +pdc_enable_intr (hwirq, true);irq_chip_unmask_parent (d); }
@@ -111,9 +126,12 @@ enum pdc_irq_config_bits {*/ static int qcom_pdc_gic_set_type (struct irq_data *d,unsigned int type) { -int pin_out = d->hwirq; +irq_hw_number_t pin_out = d->hwirq;enum pdc_irq_config_bits pdc_type; +if (pin_out ==ULONG_MAX ) +return 0 ; +switch (type) {case IRQ_TYPE_EDGE_RISING : pdc_type =PDC_EDGE_RISING ;
@@ -157,7 +175,7 @@ static struct irq_chip qcom_pdc_gic_chip = { .irq_set_affinity = irq_chip_set_affinity_parent, }; -static irq_hw_number_t get_parent_hwirq (int pin) +static irq_hw_number_t get_parent_hwirq (irq_hw_number_t pin) {int i;struct pdc_pin_region *region;
@@ -169,7 +187,6 @@ static irq_hw_number_t get_parent_hwirq(int pin)return (region->parent_base + pin - region->pin_base); } -WARN_ON (1 );return ~0UL ; }
@@ -232,6 +249,141 @@ static const struct irq_domain_ops qcom_pdc_ops = { .free = irq_domain_free_irqs_common, }; +static const struct pdc_gpio_pin_map sdm845_gpio_map[] = { + {1 ,30 }, + {3 ,31 }, + {5 ,32 }, + {10 ,33 }, + {11 ,34 }, + {20 ,35 }, + {22 ,36 }, + {24 ,37 }, + {26 ,38 }, + {30 ,39 }, + {31 ,117 }, + {32 ,41 }, + {34 ,42 }, + {36 ,43 }, + {37 ,44 }, + {38 ,45 }, + {39 ,46 }, + {40 ,47 }, + {41 ,115 }, + {43 ,49 }, + {44 ,50 }, + {46 ,51 }, + {48 ,52 }, + {49 ,118 }, + {52 ,54 }, + {53 ,55 }, + {54 ,56 }, + {56 ,57 }, + {57 ,58 }, + {58 ,59 }, + {59 ,60 }, + {60 ,61 }, + {61 ,62 }, + {62 ,63 }, + {63 ,64 }, + {64 ,65 }, + {66 ,66 }, + {68 ,67 }, + {71 ,68 }, + {73 ,69 }, + {77 ,70 }, + {78 ,71 }, + {79 ,72 }, + {80 ,73 }, + {84 ,74 }, + {85 ,75 }, + {86 ,76 }, + {88 ,77 }, + {89 ,116 }, + {91 ,79 }, + {92 ,80 }, + {95 ,81 }, + {96 ,82 }, + {97 ,83 }, + {101 ,84 }, + {103 ,85 }, + {104 ,86 }, + {115 ,90 }, + {116 ,91 }, + {117 ,92 }, + {118 ,93 }, + {119 ,94 }, + {120 ,95 }, + {121 ,96 }, + {122 ,97 }, + {123 ,98 }, + {124 ,99 }, + {125 ,100 }, + {127 ,102 }, + {128 ,103 }, + {129 ,104 }, + {130 ,105 }, + {132 ,106 }, + {133 ,107 }, + {145 ,108 }, +}; + +static irq_hw_number_t qcom_gpio_to_pdc_pin (unsigned int gpio) +{ +const struct pdc_gpio_pin_map *map = sdm845_gpio_map; +unsigned int i; + + +for (i =0 ; i <ARRAY_SIZE (sdm845_gpio_map); i++, map++) +if (gpio == map->gpio) +return map->pdc_pin; + +return ULONG_MAX ; +} + +static int qcom_pdc_gpio_alloc (struct irq_domain *domain,unsigned int virq, +unsigned int nr_irqs,void *data) +{ +struct qcom_irq_fwspec *qcom_fwspec = data; +struct irq_fwspec *fwspec = &qcom_fwspec->fwspec; +struct irq_fwspec parent_fwspec; +irq_hw_number_t hwirq, parent_hwirq; +unsigned int type; +int ret; + + hwirq =qcom_gpio_to_pdc_pin (fwspec->param[0 ]); + parent_hwirq =get_parent_hwirq (hwirq); + + ret =irq_domain_set_hwirq_and_chip (domain, virq, hwirq, + &qcom_pdc_gic_chip,NULL ); +if (ret) +return ret; + +if (hwirq ==ULONG_MAX ) +return 0 ; + + qcom_fwspec->mask = true; + +if (type &IRQ_TYPE_EDGE_BOTH ) + type =IRQ_TYPE_EDGE_RISING ; + +if (type &IRQ_TYPE_LEVEL_MASK ) + type =IRQ_TYPE_LEVEL_HIGH ; + + parent_fwspec.fwnode = domain->parent->fwnode; + parent_fwspec.param_count =3 ; + parent_fwspec.param[0 ] =0 ; + parent_fwspec.param[1 ] = parent_hwirq; + parent_fwspec.param[2 ] = type; + +return irq_domain_alloc_irqs_parent (domain, virq, nr_irqs, + &parent_fwspec); +} + +static const struct irq_domain_ops qcom_pdc_gpio_ops = { + .alloc = qcom_pdc_gpio_alloc, + .free = irq_domain_free_irqs_common, +}; +static int pdc_setup_pin_mapping (struct device_node *np) {int ret, n;
@@ -270,7 +422,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)static int qcom_pdc_init (struct device_node *node,struct device_node *parent) { -struct irq_domain *parent_domain, *pdc_domain; +struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;int ret; pdc_base =of_iomap (node,0 );
@@ -301,6 +453,12 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) goto fail; } + pdc_gpio_domain =irq_domain_create_hierarchy (parent_domain,0 ,PDC_MAX_GPIO_IRQS , +of_fwnode_handle (node), + &qcom_pdc_gpio_ops,NULL ); +/* TODO: Replace DOMAIN_BUS_NEXUS with DOMAIN_GPIO_WAKEUP or something */ +irq_domain_update_bus_token (pdc_gpio_domain,DOMAIN_BUS_NEXUS ); +return 0 ; fail:
@@ -308,5 +466,4 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)iounmap (pdc_base);return ret; } -IRQCHIP_DECLARE (pdc_sdm845,"qcom,sdm845-pdc" , qcom_pdc_init);
drivers/pinctrl/qcom/pinctrl-msm.c +58 -66
219 lines not on this page · raw message
include/linux/soc/qcom/irq.h new file +23 -0
@@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __QCOM_IRQ_H +#define __QCOM_IRQ_H + +#include <linux/irqdomain.h> + +/** +* struct qcom_irq_fwspec - qcom specific irq fwspec wrapper +* @fwspec: irq fwspec +* @mask: if true, keep the irq masked in the gpio controller +* +* Use this structure to communicate between the parent irq chip, MPM or PDC, +* to the gpio chip, TLMM, about the gpio being allocated in the parent +* and if the gpio chip should keep the line masked because the parent irq +* chip is handling everything about the irq line. +*/ +struct qcom_irq_fwspec { +struct irq_fwspec fwspec; +bool mask; +}; + +#endif
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 44 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-26 08:14:55)
Quoted 30 lines · Stephen Boyd
Quoted 18 lines
Quoting Lina Iyer (2018-11-20 16:06:47)
Quoted 17 lines
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend.
Signed-off-by Lina Iyer <[email protected]>--- .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +0
-0@@ -29,6 +29,17 @@ SDM845 platform. Definition: must be 2. Specifying the pin number and flags, as defined in <dt-bindings/interrupt-controller/irq.h>Quoted 11 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt SDM845 platform. · line 37
Quoted 6 lines
Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +6
-0+- wakeup-parent: + Usage: optional + Value type: <phandle> + Definition: A phandle to the wakeup interrupt controller for the SoC. + +- wakeup-irq:This shouldn't be needed. TLMM driver can probe for the possibility of wakeup capable irqs from irq allocation step. The only place we should need to know what TLMM pins map to what PDC lines is in the PDC driver.
Quoted 6 lines
Why? Every driver seems to translate the hardware IRQ and pass it to it's parent. Why should this be any different ? The PDC is an interrupt controller that just knows an interrupt port and maps it to the GIC. Not sure, I understand the reasoning for this. It seems to add more confusing relationship with the PDC interrupt controller, that way.
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
Quoted 7 lines
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
Couldn't this be simply achieved by matching the compatible flags for PDC/MPM bindings for the wakeup-parent in the TLMM driver?
Thanks,
Lina
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 15 lines · Stephen Boyd
Quoted 7 lines
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
Quoted 7 lines
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
Quoted 3 lines
Couldn't this be simply achieved by matching the compatible flags for PDC/MPM bindings for the wakeup-parent in the TLMM driver?
It could be, but then we would be making TLMM highly aware of the wakeup parent and have to do compatible string matching in two places, instead of making TLMM more abstractly aware that it needs to keep things masked while irq parent deals with the interrupts.
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 16 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 15 lines · Stephen Boyd
Quoted 14 lines
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
Quoted 6 lines
Quoted 3 lines
Couldn't this be simply achieved by matching the compatible flags for PDC/MPM bindings for the wakeup-parent in the TLMM driver?
It could be, but then we would be making TLMM highly aware of the wakeup parent
It is is not aware of anything about the wakeup parent, just the compatible that indicates whether it needs to be mask locally or not.
Quoted 4 lines
and have to do compatible string matching in two places, instead of making TLMM more abstractly aware that it needs to keep things masked while irq parent deals with the interrupts.
Your approach seems too complex just to circumvent a simple match node. I think we are stretching too much to support something that is not a priority.
-- Lina
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 17 lines · Lina Iyer
Quoted 16 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 15 lines · Stephen Boyd
Quoted 14 lines
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Couldn't this be simply achieved by matching the compatible flags for PDC/MPM bindings for the wakeup-parent in the TLMM driver?
It could be, but then we would be making TLMM highly aware of the wakeup parent
It is is not aware of anything about the wakeup parent, just the compatible that indicates whether it needs to be mask locally or not.
But the information related to how the PDC is wired to GPIO pins is a property related to the PDC not of the TLMM, so it does make sense to represent this information there.
And iiuc it also makes sense to not encode it in DT because it's physical wiring, not configuration.
Quoted 8 lines
Quoted 4 lines
and have to do compatible string matching in two places, instead of making TLMM more abstractly aware that it needs to keep things masked while irq parent deals with the interrupts.
Your approach seems too complex just to circumvent a simple match node. I think we are stretching too much to support something that is not a priority.
What "something" are you referring to here?
Regards,
Bjorn
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 25 lines · Bjorn Andersson
Quoted 23 lines · Lina Iyer
Quoted 22 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 15 lines · Stephen Boyd
Quoted 7 lines
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
Quoted 7 lines
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
Quoted 3 lines
Couldn't this be simply achieved by matching the compatible flags for PDC/MPM bindings for the wakeup-parent in the TLMM driver?
It could be, but then we would be making TLMM highly aware of the wakeup parent
Quoted 8 lines
Quoted 3 lines
It is is not aware of anything about the wakeup parent, just the compatible that indicates whether it needs to be mask locally or not.
But the information related to how the PDC is wired to GPIO pins is a property related to the PDC not of the TLMM, so it does make sense to represent this information there.
Hmm.. But we are inconsistent in what we provide as input into the PDC driver.
Quoted 1 line
From the PDC driver perspective, it gets a hwirq number either when
driver requests a wakeup interrupt specified in DT as
<&pdc 5 IRQ_TYPE_LEVEL_HIGH>
or from GPIO, which translates the GPIO to the PDC hwirq and requests that.
Quoted 3 lines
And iiuc it also makes sense to not encode it in DT because it's physical wiring, not configuration.
I would agree.
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
and have to do compatible string matching in two places, instead of making TLMM more abstractly aware that it needs to keep things masked while irq parent deals with the interrupts.
Your approach seems too complex just to circumvent a simple match node. I think we are stretching too much to support something that is not a priority.
What "something" are you referring to here?
MPM :)
BTW, I am discussing with the internal folks here on if we need to mask TLMM when the wakeup-parent is MPM. If we don't have to, we should be able to follow the same model as we done in this patch and don't even have to check the compatible or use the approach suggested by Stephen.
-- Lina
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 34 lines · Lina Iyer
Quoted 25 lines · Bjorn Andersson
Quoted 23 lines · Lina Iyer
Quoted 22 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 15 lines · Stephen Boyd
Quoted 7 lines
Two reasons. First, simplicity. The TLMM driver just needs to pass the gpio number up to the PDC gpio domain and then that domain can figure out what hwirq it maps to within the PDC hw irq space. I don't see any reason why we have to know the hwirq of PDC within the TLMM driver besides "let's not be different".
Quoted 7 lines
And second, it makes it easier for us to implement the MPM case in the TLMM driver by letting the TLMM code just ask "should I mask the irq here or not?" by passing that with a wrapper struct around the fwspec and a dedicated domain in the PDC/MPM driver. Keeping less things in the TLMM driver and not driving the decision from DT but from tables in the PDC driver also keeps things simple and reduces DT parsing code/time.
Quoted 3 lines
Couldn't this be simply achieved by matching the compatible flags for PDC/MPM bindings for the wakeup-parent in the TLMM driver?
It could be, but then we would be making TLMM highly aware of the wakeup parent
Quoted 8 lines
Quoted 3 lines
It is is not aware of anything about the wakeup parent, just the compatible that indicates whether it needs to be mask locally or not.
But the information related to how the PDC is wired to GPIO pins is a property related to the PDC not of the TLMM, so it does make sense to represent this information there.
Quoted 8 lines
Hmm.. But we are inconsistent in what we provide as input into the PDC driver.
From the PDC driver perspective, it gets a hwirq number either when driver requests a wakeup interrupt specified in DT as
<&pdc 5IRQ_TYPE_LEVEL_HIGH>
or from GPIO, which translates the GPIO to the PDC hwirq and requests that.
I see what you're saying, but need to think some more about this...
Quoted 9 lines
Quoted 3 lines
And iiuc it also makes sense to not encode it in DT because it's physical wiring, not configuration.
I would agree.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
and have to do compatible string matching in two places, instead of making TLMM more abstractly aware that it needs to keep things masked while irq parent deals with the interrupts.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Your approach seems too complex just to circumvent a simple match node. I think we are stretching too much to support something that is not a priority.
What "something" are you referring to here?
MPM :)
There are still new platforms coming out with MPM, so there's even a business case to care about it.
Quoted 5 lines
BTW, I am discussing with the internal folks here on if we need to mask TLMM when the wakeup-parent is MPM. If we don't have to, we should be able to follow the same model as we done in this patch and don't even have to check the compatible or use the approach suggested by Stephen.
Looking forward to the result of that discussion.
Regards,
Bjorn
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
In reply to Lina IyerQuoted 6 lines · Lina Iyer
Quoted 5 lines · Bjorn Andersson
Quoted 3 lines · Lina Iyer
Quoted 2 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 1 line · Stephen Boyd
[...]
Quoted 5 lines
BTW, I am discussing with the internal folks here on if we need to mask TLMM when the wakeup-parent is MPM. If we don't have to, we should be able to follow the same model as we done in this patch and don't even have to check the compatible or use the approach suggested by Stephen.
The TLMM and the MPM are not active at the same time. However, there is a small chance they might be (a few clock cycles) when the system is going down, but even then, since we replay the interrupt from the MPM driver before the interrupts are serviced by Linux, we would not see multiple GPIO interrupts.
The way we have MPM working downstream, for a wakeup GPIO IRQ -
a. Application cores gets a wakeup interrupt either from RPM or GIC (if TLMM was not powered down) while still in the interrupt locked context.
b. In the hardware, apps core handshakes with the RPM and then starts resuming from the platform's system idle driver.
c. The first CPU to wake up calls MPM driver from the idle driver, which reads the shared memory to find the MPM pins that are set. Converts the MPM pins to their corresponding linux interrupt and replays the interrupt.
d. Idle driver exits and wakeup GPIO interrupt is handled.
The MPM pins are not updated after the RPM lets the application core to run. Since TLMM is functional after the RPM handshake, it takes over.
Note, the downstream design is predicated on the OS-Initiated support for all MPM based SoCs which serializes the last CPU going down and the first CPU coming out of idle.
Thanks,
Lina
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoting Lina Iyer (2018-11-30 10:33:17)
Quoted 33 lines · Lina Iyer
Quoted 6 lines · Bjorn Andersson
Quoted 5 lines · Lina Iyer
Quoted 4 lines · Stephen Boyd
Quoted 2 lines
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 1 line · Stephen Boyd
[...]
Quoted 5 lines
BTW, I am discussing with the internal folks here on if we need to mask TLMM when the wakeup-parent is MPM. If we don't have to, we should be able to follow the same model as we done in this patch and don't even have to check the compatible or use the approach suggested by Stephen.
The TLMM and the MPM are not active at the same time. However, there is a small chance they might be (a few clock cycles) when the system is going down, but even then, since we replay the interrupt from the MPM driver before the interrupts are serviced by Linux, we would not see multiple GPIO interrupts.
The way we have MPM working downstream, for a wakeup GPIO IRQ -
a. Application cores gets a wakeup interrupt either from RPM or GIC (if TLMM was not powered down) while still in the interrupt locked context.
b. In the hardware, apps core handshakes with the RPM and then starts resuming from the platform's system idle driver.
c. The first CPU to wake up calls MPM driver from the idle driver, which reads the shared memory to find the MPM pins that are set. Converts the MPM pins to their corresponding linux interrupt and replays the interrupt.
Quoted 5 lines
d. Idle driver exits and wakeup GPIO interrupt is handled.
The MPM pins are not updated after the RPM lets the application core to run. Since TLMM is functional after the RPM handshake, it takes over.
Thanks for the background info. I don't think it really changes anything that we've discussed though. We still need to mask the IRQ in TLMM all the time when we're using the PDC and we need to leave it unmasked and replay edges that the MPM sees when we use the MPM. Should I clean up my RFC patch and post it to the list? I'd like to see hierarchical gpio irqs work in general for this problem and also the SSBI/SPMI gpio irq problem that Linus pointed out last week.
Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Quoted 40 lines · Stephen Boyd
Quoting Lina Iyer (2018-11-30 10:33:17)
Quoted 33 lines · Lina Iyer
Quoted 6 lines · Bjorn Andersson
Quoted 5 lines · Lina Iyer
Quoted 4 lines · Stephen Boyd
Quoted 2 lines
Quoting Lina Iyer (2018-11-27 10:21:23)
Quoted 1 line · Stephen Boyd
[...]
Quoted 5 lines
BTW, I am discussing with the internal folks here on if we need to mask TLMM when the wakeup-parent is MPM. If we don't have to, we should be able to follow the same model as we done in this patch and don't even have to check the compatible or use the approach suggested by Stephen.
The TLMM and the MPM are not active at the same time. However, there is a small chance they might be (a few clock cycles) when the system is going down, but even then, since we replay the interrupt from the MPM driver before the interrupts are serviced by Linux, we would not see multiple GPIO interrupts.
The way we have MPM working downstream, for a wakeup GPIO IRQ -
a. Application cores gets a wakeup interrupt either from RPM or GIC (if TLMM was not powered down) while still in the interrupt locked context.
b. In the hardware, apps core handshakes with the RPM and then starts resuming from the platform's system idle driver.
c. The first CPU to wake up calls MPM driver from the idle driver, which reads the shared memory to find the MPM pins that are set. Converts the MPM pins to their corresponding linux interrupt and replays the interrupt.
Quoted 5 lines
d. Idle driver exits and wakeup GPIO interrupt is handled.
The MPM pins are not updated after the RPM lets the application core to run. Since TLMM is functional after the RPM handshake, it takes over.
Quoted 5 lines
Thanks for the background info. I don't think it really changes anything that we've discussed though. We still need to mask the IRQ in TLMM all the time when we're using the PDC and we need to leave it unmasked and replay edges that the MPM sees when we use the MPM. Should I clean up my RFC patch and post it to the list?
I have started to work on this. But feel free to post your version if you have it ready.
Thanks for the review Stephen. I think I understand now where you are getting with it. Sorry about all the back and forth.
Thanks,
Lina
Quoted 4 lines
I'd like to see hierarchical gpio
irqs work in general for this problem and also the SSBI/SPMI gpio irq problem that Linus pointed out last week.
RFC v3 3/3 arm64: dts: msm: add PDC wake irq maps for GPIOs for SDM845
In reply to Lina IyerAdd PDC wakeup irq for GPIOs and the wakeup parent for TLMM.
--- arch/arm64/boot/dts/qcom/sdm845.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
arch/arm64/boot/dts/qcom/sdm845.dtsi +18 -0
@@ -821,6 +821,24 @@ #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; + wake-parent = <&pdc>; + wake-irq = + <1 30>, <3 31>, <5 32>, <10 33>, <11 34>, + <20 35>, <22 36>, <24 37>, <26 38>, <30 39>, + <31 117>, <32 41>, <34 42>, <36 43>, <37 44>, + <38 45>, <39 46>, <40 47>, <41 115>, <43 49>, + <44 50>, <46 51>, <48 52>, <49 118>, <52 54>, + <53 55>, <54 56>, <56 57>, <57 58>, <58 59>, + <59 60>, <60 61>, <61 62>, <62 63>, <63 64>, + <64 65>, <66 66>, <68 67>, <71 68>, <73 69>, + <77 70>, <78 71>, <79 72>, <80 73>, <84 74>, + <85 75>, <86 76>, <88 77>, <89 116>, <91 79>, + <92 80>, <95 81>, <96 82>, <97 83>, <101 84>, + <103 85>, <104 86>, <115 90>, <116 91>, + <117 92>, <118 93>, <119 94>, <120 95>, + <121 96>, <122 97>, <123 98>, <124 99>, + <125 100>, <127 102>, <128 103>, <129 104>, + <130 105>, <132 106>, <133 107>, <145 108>; qup_i2c0_default: qup-i2c0-default { pinmux {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project