mbox series

[net-next,0/6] net: ipa: simplify IPA interrupt handling

Message ID 20221230232230.2348757-1-elder@linaro.org
Headers show
Series net: ipa: simplify IPA interrupt handling | expand

Message

Alex Elder Dec. 30, 2022, 11:22 p.m. UTC 
One of the IPA's two IRQs fires when data on a suspended channel is
available (to request that the channel--or system--be resumed to
recieve the pending data).  This interrupt also handles a few
conditions signaled by the embedded microcontroller.

For this "IPA interrupt", the current code requires a handler to be
dynamically registered for each interrupt condition.  Any condition
that has no registered handler is quietly ignored.  This design is
derived from the downstream IPA driver implementation.

There isn't any need for this complexity.  Even in the downstream
code, only four of the available 30 or so IPA interrupt conditions
are ever handled.  So these handlers can pretty easily just be
called directly in the main IRQ handler function.

This series simplifies the interrupt handling code by having the
small number of IPA interrupt handlers be called directly, rather
than having them be registered dynamically.

					-Alex

Alex Elder (6):
  net: ipa: introduce a common microcontroller interrupt handler
  net: ipa: introduce ipa_interrupt_enable()
  net: ipa: enable IPA interrupt handlers separate from registration
  net: ipa: register IPA interrupt handlers directly
  net: ipa: kill ipa_interrupt_add()
  net: ipa: don't maintain IPA interrupt handler array

 drivers/net/ipa/ipa_interrupt.c | 103 ++++++++++++++------------------
 drivers/net/ipa/ipa_interrupt.h |  47 +++++----------
 drivers/net/ipa/ipa_power.c     |  19 ++----
 drivers/net/ipa/ipa_power.h     |  12 ++++
 drivers/net/ipa/ipa_uc.c        |  21 +++++--
 drivers/net/ipa/ipa_uc.h        |   8 +++
 6 files changed, 98 insertions(+), 112 deletions(-)

Comments

Jakub Kicinski Dec. 31, 2022, 3:52 a.m. UTC | #1 
On Fri, 30 Dec 2022 17:22:24 -0600 Alex Elder wrote:
> [PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling

We kept net-next closed for an extra week due to end-of-the-year
festivities, back in business next week, sorry.
Alex Elder Dec. 31, 2022, 12:21 p.m. UTC | #2 
On 12/30/22 9:52 PM, Jakub Kicinski wrote:
> On Fri, 30 Dec 2022 17:22:24 -0600 Alex Elder wrote:
>> [PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling
> 
> We kept net-next closed for an extra week due to end-of-the-year
> festivities, back in business next week, sorry.

No, *I* am sorry.  I forgot to even check this time, and I normally do.
   http://vger.kernel.org/~davem/net-next.html

It's perfectly fine, I'll wait.  I won't re-send next week unless it
becomes obvious I should.

Thanks, and happy new year!

					-Alex
Caleb Connolly Dec. 31, 2022, 5:56 p.m. UTC | #3 
On 30/12/2022 23:22, Alex Elder wrote:
> Expose ipa_interrupt_enable() and have functions that register
> IPA interrupt handlers enable them directly, rather than having the
> registration process do that.  Do the same for disabling IPA
> interrupt handlers.

Hi,
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>   drivers/net/ipa/ipa_interrupt.c |  8 ++------
>   drivers/net/ipa/ipa_interrupt.h | 14 ++++++++++++++
>   drivers/net/ipa/ipa_power.c     |  6 +++++-
>   drivers/net/ipa/ipa_uc.c        |  4 ++++
>   4 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
> index 7b7388c14806f..87f4b94d02a3f 100644
> --- a/drivers/net/ipa/ipa_interrupt.c
> +++ b/drivers/net/ipa/ipa_interrupt.c
> @@ -135,7 +135,7 @@ static void ipa_interrupt_enabled_update(struct ipa *ipa)
>   }
>   
>   /* Enable an IPA interrupt type */
> -static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>   {
>   	/* Update the IPA interrupt mask to enable it */
>   	ipa->interrupt->enabled |= BIT(ipa_irq);
> @@ -143,7 +143,7 @@ static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>   }
>   
>   /* Disable an IPA interrupt type */
> -static void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>   {
>   	/* Update the IPA interrupt mask to disable it */
>   	ipa->interrupt->enabled &= ~BIT(ipa_irq);
> @@ -232,8 +232,6 @@ void ipa_interrupt_add(struct ipa_interrupt *interrupt,
>   		return;
>   
>   	interrupt->handler[ipa_irq] = handler;
> -
> -	ipa_interrupt_enable(interrupt->ipa, ipa_irq);
>   }
>   
>   /* Remove the handler for an IPA interrupt type */
> @@ -243,8 +241,6 @@ ipa_interrupt_remove(struct ipa_interrupt *interrupt, enum ipa_irq_id ipa_irq)
>   	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
>   		return;
>   
> -	ipa_interrupt_disable(interrupt->ipa, ipa_irq);
> -
>   	interrupt->handler[ipa_irq] = NULL;
>   }
>   
> diff --git a/drivers/net/ipa/ipa_interrupt.h b/drivers/net/ipa/ipa_interrupt.h
> index f31fd9965fdc6..5f7d2e90ea337 100644
> --- a/drivers/net/ipa/ipa_interrupt.h
> +++ b/drivers/net/ipa/ipa_interrupt.h
> @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt);
>    */
>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>   
> +/**
> + * ipa_interrupt_enable() - Enable an IPA interrupt type
> + * @ipa:	IPA pointer
> + * @ipa_irq:	IPA interrupt ID
> + */
> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);

I think you forgot a forward declaration for enum ipa_irq_id

Kind Regards,
Caleb
> +
> +/**
> + * ipa_interrupt_disable() - Disable an IPA interrupt type
> + * @ipa:	IPA pointer
> + * @ipa_irq:	IPA interrupt ID
> + */
> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
> +
>   /**
>    * ipa_interrupt_config() - Configure the IPA interrupt framework
>    * @ipa:	IPA pointer
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 8420f93128a26..9148d606d5fc2 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -337,10 +337,13 @@ int ipa_power_setup(struct ipa *ipa)
>   
>   	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
>   			  ipa_suspend_handler);
> +	ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
>   
>   	ret = device_init_wakeup(&ipa->pdev->dev, true);
> -	if (ret)
> +	if (ret) {
> +		ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>   		ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
> +	}
>   
>   	return ret;
>   }
> @@ -348,6 +351,7 @@ int ipa_power_setup(struct ipa *ipa)
>   void ipa_power_teardown(struct ipa *ipa)
>   {
>   	(void)device_init_wakeup(&ipa->pdev->dev, false);
> +	ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>   	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>   }
>   
> diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
> index 0a890b44c09e1..af541758d047f 100644
> --- a/drivers/net/ipa/ipa_uc.c
> +++ b/drivers/net/ipa/ipa_uc.c
> @@ -187,7 +187,9 @@ void ipa_uc_config(struct ipa *ipa)
>   	ipa->uc_powered = false;
>   	ipa->uc_loaded = false;
>   	ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, ipa_uc_interrupt_handler);
> +	ipa_interrupt_enable(ipa, IPA_IRQ_UC_0);
>   	ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, ipa_uc_interrupt_handler);
> +	ipa_interrupt_enable(ipa, IPA_IRQ_UC_1);
>   }
>   
>   /* Inverse of ipa_uc_config() */
> @@ -195,7 +197,9 @@ void ipa_uc_deconfig(struct ipa *ipa)
>   {
>   	struct device *dev = &ipa->pdev->dev;
>   
> +	ipa_interrupt_disable(ipa, IPA_IRQ_UC_1);
>   	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
> +	ipa_interrupt_disable(ipa, IPA_IRQ_UC_0);
>   	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
>   	if (ipa->uc_loaded)
>   		ipa_power_retention(ipa, false);
Alex Elder Jan. 1, 2023, 6:32 p.m. UTC | #4 
On 12/31/22 11:56 AM, Caleb Connolly wrote:
> 
> 
> On 30/12/2022 23:22, Alex Elder wrote:
>> Expose ipa_interrupt_enable() and have functions that register
>> IPA interrupt handlers enable them directly, rather than having the
>> registration process do that.  Do the same for disabling IPA
>> interrupt handlers.
> 
> Hi,
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   drivers/net/ipa/ipa_interrupt.c |  8 ++------
>>   drivers/net/ipa/ipa_interrupt.h | 14 ++++++++++++++
>>   drivers/net/ipa/ipa_power.c     |  6 +++++-
>>   drivers/net/ipa/ipa_uc.c        |  4 ++++
>>   4 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_interrupt.c 
>> b/drivers/net/ipa/ipa_interrupt.c
>> index 7b7388c14806f..87f4b94d02a3f 100644
>> --- a/drivers/net/ipa/ipa_interrupt.c
>> +++ b/drivers/net/ipa/ipa_interrupt.c
>> @@ -135,7 +135,7 @@ static void ipa_interrupt_enabled_update(struct 
>> ipa *ipa)
>>   }
>>   /* Enable an IPA interrupt type */
>> -static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id 
>> ipa_irq)
>> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>>   {
>>       /* Update the IPA interrupt mask to enable it */
>>       ipa->interrupt->enabled |= BIT(ipa_irq);
>> @@ -143,7 +143,7 @@ static void ipa_interrupt_enable(struct ipa *ipa, 
>> enum ipa_irq_id ipa_irq)
>>   }
>>   /* Disable an IPA interrupt type */
>> -static void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id 
>> ipa_irq)
>> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>>   {
>>       /* Update the IPA interrupt mask to disable it */
>>       ipa->interrupt->enabled &= ~BIT(ipa_irq);
>> @@ -232,8 +232,6 @@ void ipa_interrupt_add(struct ipa_interrupt 
>> *interrupt,
>>           return;
>>       interrupt->handler[ipa_irq] = handler;
>> -
>> -    ipa_interrupt_enable(interrupt->ipa, ipa_irq);
>>   }
>>   /* Remove the handler for an IPA interrupt type */
>> @@ -243,8 +241,6 @@ ipa_interrupt_remove(struct ipa_interrupt 
>> *interrupt, enum ipa_irq_id ipa_irq)
>>       if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
>>           return;
>> -    ipa_interrupt_disable(interrupt->ipa, ipa_irq);
>> -
>>       interrupt->handler[ipa_irq] = NULL;
>>   }
>> diff --git a/drivers/net/ipa/ipa_interrupt.h 
>> b/drivers/net/ipa/ipa_interrupt.h
>> index f31fd9965fdc6..5f7d2e90ea337 100644
>> --- a/drivers/net/ipa/ipa_interrupt.h
>> +++ b/drivers/net/ipa/ipa_interrupt.h
>> @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct 
>> ipa_interrupt *interrupt);
>>    */
>>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>> +/**
>> + * ipa_interrupt_enable() - Enable an IPA interrupt type
>> + * @ipa:    IPA pointer
>> + * @ipa_irq:    IPA interrupt ID
>> + */
>> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
> 
> I think you forgot a forward declaration for enum ipa_irq_id

Thanks, I'll verify this and will send v2 with a fix once
net-next is open for business again.

					-Alex

> 
> Kind Regards,
> Caleb
>> +
>> +/**
>> + * ipa_interrupt_disable() - Disable an IPA interrupt type
>> + * @ipa:    IPA pointer
>> + * @ipa_irq:    IPA interrupt ID
>> + */
>> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
>> +
>>   /**
>>    * ipa_interrupt_config() - Configure the IPA interrupt framework
>>    * @ipa:    IPA pointer
>> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
>> index 8420f93128a26..9148d606d5fc2 100644
>> --- a/drivers/net/ipa/ipa_power.c
>> +++ b/drivers/net/ipa/ipa_power.c
>> @@ -337,10 +337,13 @@ int ipa_power_setup(struct ipa *ipa)
>>       ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
>>                 ipa_suspend_handler);
>> +    ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
>>       ret = device_init_wakeup(&ipa->pdev->dev, true);
>> -    if (ret)
>> +    if (ret) {
>> +        ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>>           ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>> +    }
>>       return ret;
>>   }
>> @@ -348,6 +351,7 @@ int ipa_power_setup(struct ipa *ipa)
>>   void ipa_power_teardown(struct ipa *ipa)
>>   {
>>       (void)device_init_wakeup(&ipa->pdev->dev, false);
>> +    ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>>       ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>>   }
>> diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
>> index 0a890b44c09e1..af541758d047f 100644
>> --- a/drivers/net/ipa/ipa_uc.c
>> +++ b/drivers/net/ipa/ipa_uc.c
>> @@ -187,7 +187,9 @@ void ipa_uc_config(struct ipa *ipa)
>>       ipa->uc_powered = false;
>>       ipa->uc_loaded = false;
>>       ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, 
>> ipa_uc_interrupt_handler);
>> +    ipa_interrupt_enable(ipa, IPA_IRQ_UC_0);
>>       ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, 
>> ipa_uc_interrupt_handler);
>> +    ipa_interrupt_enable(ipa, IPA_IRQ_UC_1);
>>   }
>>   /* Inverse of ipa_uc_config() */
>> @@ -195,7 +197,9 @@ void ipa_uc_deconfig(struct ipa *ipa)
>>   {
>>       struct device *dev = &ipa->pdev->dev;
>> +    ipa_interrupt_disable(ipa, IPA_IRQ_UC_1);
>>       ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
>> +    ipa_interrupt_disable(ipa, IPA_IRQ_UC_0);
>>       ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
>>       if (ipa->uc_loaded)
>>           ipa_power_retention(ipa, false);
Alex Elder Jan. 3, 2023, 8:25 p.m. UTC | #5 
On 12/31/22 11:56 AM, Caleb Connolly wrote:
>>
>> diff --git a/drivers/net/ipa/ipa_interrupt.h 
>> b/drivers/net/ipa/ipa_interrupt.h
>> index f31fd9965fdc6..5f7d2e90ea337 100644
>> --- a/drivers/net/ipa/ipa_interrupt.h
>> +++ b/drivers/net/ipa/ipa_interrupt.h
>> @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct 
>> ipa_interrupt *interrupt);
>>    */
>>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>> +/**
>> + * ipa_interrupt_enable() - Enable an IPA interrupt type
>> + * @ipa:    IPA pointer
>> + * @ipa_irq:    IPA interrupt ID
>> + */
>> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
> 
> I think you forgot a forward declaration for enum ipa_irq_id
> 
> Kind Regards,
> Caleb

OK I checked this.

You are correct that ipa_irq_id should be declared as an
enum at the top of "ipa_interrupt.h".  In addition to the
new function declarations, there were some existing
references to the enumerated type.  I believe this became
a (not-reported) problem starting with this commit:

   322053105f095 net: ipa: move definition of enum ipa_irq_id

It being missing did not result in any build warnings,
however.  Here's why.

The ipa_irq_id enumerated type is defined in "ipa_reg.h".

Note that "ipa_reg.h" is included by "ipa_endpoint.h".  So if
"ipa_endpoint.h" is included before "ipa_interrupt.h", the
ipa_irq_id type will have been defined before it's referenced
in "ipa_interrupt.h".

These files include "ipa_interrupt.h":

ipa.h:
It is included after "ipa_endpoint.h", so the enumerated type
is defined before it's needed in "ipa_interrupt.h".

ipa_main.c:
Here too, "ipa_endpoint.h" (as well as "ipa_reg.h") is included
before "ipa_interrupt.h", so the enumerated type is defined
before it's used there.

ipa_interrupt.c
In this case "ipa_reg.h" is included, then "ipa_endpoint.h",
before "ipa_interrupt.h".  So again, the enumerated type is
defined before it's referenced in "ipa_interrupt.h".

Nevertheless, your point is a good one and I'm going to
implement your suggestion when I post version 2.

Thank you!

					-Alex