Re: [PATCH v2 3/3] irqchip/qcom-pdc: Start getting rid of the GPIO_NO_WAKE_IRQ

[Maulik Shah <mkshah@codeaurora.org>]

Hi,

On 8/20/2021 9:08 PM, Marc Zyngier wrote:
> On Thu, 19 Aug 2021 12:53:13 +0100,
> Maulik Shah <mkshah@codeaurora.org> wrote:
>> From: Marc Zyngier <maz@kernel.org>
>>
>> gpio_to_irq() reports error at irq_domain_trim_hierarchy() for non
>> wakeup capable GPIOs that do not have dedicated interrupt at GIC.
>>
>> Since PDC irqchip do not allocate irq at parent GIC domain for such
>> GPIOs indicate same by using irq_domain_disconnect_hierarchy() for
>> PDC and parent GIC domains.
>>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> [mkshah: Add loop to disconnect for all parents]
>> Tested-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/irqchip/qcom-pdc.c | 75 +++++++++++-----------------------------------
>>   1 file changed, 18 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 32d5920..696afca 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -53,26 +53,6 @@ static u32 pdc_reg_read(int reg, u32 i)
>>   	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
>>   }
>>   
>> -static int qcom_pdc_gic_get_irqchip_state(struct irq_data *d,
>> -					  enum irqchip_irq_state which,
>> -					  bool *state)
>> -{
>> -	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -		return 0;
>> -
>> -	return irq_chip_get_parent_state(d, which, state);
>> -}
>> -
>> -static int qcom_pdc_gic_set_irqchip_state(struct irq_data *d,
>> -					  enum irqchip_irq_state which,
>> -					  bool value)
>> -{
>> -	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -		return 0;
>> -
>> -	return irq_chip_set_parent_state(d, which, value);
>> -}
>> -
>>   static void pdc_enable_intr(struct irq_data *d, bool on)
>>   {
>>   	int pin_out = d->hwirq;
>> @@ -91,38 +71,16 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>>   
>>   static void qcom_pdc_gic_disable(struct irq_data *d)
>>   {
>> -	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -		return;
>> -
>>   	pdc_enable_intr(d, false);
>>   	irq_chip_disable_parent(d);
>>   }
>>   
>>   static void qcom_pdc_gic_enable(struct irq_data *d)
>>   {
>> -	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -		return;
>> -
>>   	pdc_enable_intr(d, true);
>>   	irq_chip_enable_parent(d);
>>   }
>>   
>> -static void qcom_pdc_gic_mask(struct irq_data *d)
>> -{
>> -	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -		return;
>> -
>> -	irq_chip_mask_parent(d);
>> -}
>> -
>> -static void qcom_pdc_gic_unmask(struct irq_data *d)
>> -{
>> -	if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> -		return;
>> -
>> -	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
>> @@ -159,14 +117,10 @@ enum pdc_irq_config_bits {
>>    */
>>   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;
>>   	enum pdc_irq_config_bits old_pdc_type;
>>   	int ret;
>>   
>> -	if (pin_out == GPIO_NO_WAKE_IRQ)
>> -		return 0;
>> -
>>   	switch (type) {
>>   	case IRQ_TYPE_EDGE_RISING:
>>   		pdc_type = PDC_EDGE_RISING;
>> @@ -191,8 +145,8 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>   		return -EINVAL;
>>   	}
>>   
>> -	old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
>> -	pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>> +	old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>> +	pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>   
>>   	ret = irq_chip_set_type_parent(d, type);
>>   	if (ret)
>> @@ -216,12 +170,12 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>   static struct irq_chip qcom_pdc_gic_chip = {
>>   	.name			= "PDC",
>>   	.irq_eoi		= irq_chip_eoi_parent,
>> -	.irq_mask		= qcom_pdc_gic_mask,
>> -	.irq_unmask		= qcom_pdc_gic_unmask,
>> +	.irq_mask		= irq_chip_mask_parent,
>> +	.irq_unmask		= irq_chip_unmask_parent,
>>   	.irq_disable		= qcom_pdc_gic_disable,
>>   	.irq_enable		= qcom_pdc_gic_enable,
>> -	.irq_get_irqchip_state	= qcom_pdc_gic_get_irqchip_state,
>> -	.irq_set_irqchip_state	= qcom_pdc_gic_set_irqchip_state,
>> +	.irq_get_irqchip_state	= irq_chip_get_parent_state,
>> +	.irq_set_irqchip_state	= irq_chip_set_parent_state,
>>   	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>   	.irq_set_type		= qcom_pdc_gic_set_type,
>>   	.flags			= IRQCHIP_MASK_ON_SUSPEND |
>> @@ -282,7 +236,7 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
>>   
>>   	parent_hwirq = get_parent_hwirq(hwirq);
>>   	if (parent_hwirq == PDC_NO_PARENT_IRQ)
>> -		return 0;
>> +		return irq_domain_disconnect_hierarchy(domain->parent, virq);
>>   
>>   	if (type & IRQ_TYPE_EDGE_BOTH)
>>   		type = IRQ_TYPE_EDGE_RISING;
>> @@ -314,22 +268,29 @@ static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
>>   	irq_hw_number_t hwirq, parent_hwirq;
>>   	unsigned int type;
>>   	int ret;
>> +	struct irq_domain *parent;
>>   
>>   	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>>   	if (ret)
>>   		return ret;
>>   
>> +	if (hwirq == GPIO_NO_WAKE_IRQ) {
>> +		for (parent = domain; parent; parent = parent->parent) {
>> +			ret = irq_domain_disconnect_hierarchy(parent, virq);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +		return 0;
>> +	}
>> +
> No, this is wrong. Please read the documentation for
> irq_domain_disconnect_hierarchy(): the disconnect can only take place
> *once* per interrupt, right at the point where you need to terminate
> the hierarchy.
>
> irq_domain_trim_hierarchy() should already do the right thing by
> iterating over the domains and free the unused irq_data.
>
> 	M.
Thanks for the review. Seems we need disconnect only once per interrupt.
with this i see patch 2 in this series also of no use since it already 
takes care of removing irq_data for all the parents.

Addressed in v3 series.

Thanks,
Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation