[PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[Florian Fainelli]

The interrupt controller shutdown path does not need to save the mask of
enabled interrupts because the next state the system is going to be in
is akin to a cold boot, or a kexec'd kernel.

Reported-by: Tim Ross <tim.ross@broadcom.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index 2b0b3175cea0..c988886917f7 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -118,7 +118,7 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static void brcmstb_l2_intc_suspend(struct irq_data *d)
+static void __brcmstb_l2_intc_suspend(struct irq_data *d, bool save)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct irq_chip_type *ct = irq_data_get_chip_type(d);
@@ -127,7 +127,8 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
 
 	irq_gc_lock_irqsave(gc, flags);
 	/* Save the current mask */
-	b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
+	if (save)
+		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
 
 	if (b->can_wake) {
 		/* Program the wakeup mask */
@@ -137,6 +138,16 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
 	irq_gc_unlock_irqrestore(gc, flags);
 }
 
+static void brcmstb_l2_intc_shutdown(struct irq_data *d)
+{
+	__brcmstb_l2_intc_suspend(d, false);
+}
+
+static void brcmstb_l2_intc_suspend(struct irq_data *d)
+{
+	__brcmstb_l2_intc_suspend(d, true);
+}
+
 static void brcmstb_l2_intc_resume(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
@@ -252,7 +263,7 @@ static int __init brcmstb_l2_intc_of_init(struct device_node *np,
 
 	ct->chip.irq_suspend = brcmstb_l2_intc_suspend;
 	ct->chip.irq_resume = brcmstb_l2_intc_resume;
-	ct->chip.irq_pm_shutdown = brcmstb_l2_intc_suspend;
+	ct->chip.irq_pm_shutdown = brcmstb_l2_intc_shutdown;
 
 	if (data->can_wake) {
 		/* This IRQ chip can wake the system, set all child interrupts
-- 
2.34.1

Re: [PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[Thomas Gleixner]

On Tue, Apr 16 2024 at 12:43, Florian Fainelli wrote:
> The interrupt controller shutdown path does not need to save the mask of
> enabled interrupts because the next state the system is going to be in
> is akin to a cold boot, or a kexec'd kernel.

Sure, but

> Reported-by: Tim Ross <tim.ross@broadcom.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/irqchip/irq-brcmstb-l2.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
> index 2b0b3175cea0..c988886917f7 100644
> --- a/drivers/irqchip/irq-brcmstb-l2.c
> +++ b/drivers/irqchip/irq-brcmstb-l2.c
> @@ -118,7 +118,7 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> -static void brcmstb_l2_intc_suspend(struct irq_data *d)
> +static void __brcmstb_l2_intc_suspend(struct irq_data *d, bool save)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> @@ -127,7 +127,8 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
>  
>  	irq_gc_lock_irqsave(gc, flags);
>  	/* Save the current mask */
> -	b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
> +	if (save)
> +		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);

what's the conditional actually buying you except more complex code?

Thanks,

        tglx

        

Re: [PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[Florian Fainelli]

On 4/22/24 14:29, Thomas Gleixner wrote:
> On Tue, Apr 16 2024 at 12:43, Florian Fainelli wrote:
>> The interrupt controller shutdown path does not need to save the mask of
>> enabled interrupts because the next state the system is going to be in
>> is akin to a cold boot, or a kexec'd kernel.
> 
> Sure, but
> 
>> Reported-by: Tim Ross <tim.ross@broadcom.com>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   drivers/irqchip/irq-brcmstb-l2.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
>> index 2b0b3175cea0..c988886917f7 100644
>> --- a/drivers/irqchip/irq-brcmstb-l2.c
>> +++ b/drivers/irqchip/irq-brcmstb-l2.c
>> @@ -118,7 +118,7 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
>>   	chained_irq_exit(chip, desc);
>>   }
>>   
>> -static void brcmstb_l2_intc_suspend(struct irq_data *d)
>> +static void __brcmstb_l2_intc_suspend(struct irq_data *d, bool save)
>>   {
>>   	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>>   	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>> @@ -127,7 +127,8 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
>>   
>>   	irq_gc_lock_irqsave(gc, flags);
>>   	/* Save the current mask */
>> -	b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
>> +	if (save)
>> +		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
> 
> what's the conditional actually buying you except more complex code?

Not much this is an optimization that is simple to carry out. There can 
be dozens of such L2 interrupt controllers in a given system and the 
MMIO accesses start adding up eventually.
-- 
Florian

Re: [PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[Thomas Gleixner]

On Mon, Apr 22 2024 at 15:26, Florian Fainelli wrote:
> On 4/22/24 14:29, Thomas Gleixner wrote:
>>> +	if (save)
>>> +		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
>> 
>> what's the conditional actually buying you except more complex code?
>
> Not much this is an optimization that is simple to carry out. There can 
> be dozens of such L2 interrupt controllers in a given system and the 
> MMIO accesses start adding up eventually.

I'm impressed by saving ~12 microseconds per one dozen of interrupt
controllers on system shutdown :)

Re: [PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[Florian Fainelli]

On 4/22/24 16:45, Thomas Gleixner wrote:
> On Mon, Apr 22 2024 at 15:26, Florian Fainelli wrote:
>> On 4/22/24 14:29, Thomas Gleixner wrote:
>>>> +	if (save)
>>>> +		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
>>>
>>> what's the conditional actually buying you except more complex code?
>>
>> Not much this is an optimization that is simple to carry out. There can
>> be dozens of such L2 interrupt controllers in a given system and the
>> MMIO accesses start adding up eventually.
> 
> I'm impressed by saving ~12 microseconds per one dozen of interrupt
> controllers on system shutdown :)

I know, right? More seriously are you willing to take that patch, should 
I write a better justification?
-- 
Florian

Re: [PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown

[Thomas Gleixner]

On Wed, Apr 24 2024 at 09:50, Florian Fainelli wrote:
> On 4/22/24 16:45, Thomas Gleixner wrote:
>> On Mon, Apr 22 2024 at 15:26, Florian Fainelli wrote:
>>> On 4/22/24 14:29, Thomas Gleixner wrote:
>>>>> +	if (save)
>>>>> +		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
>>>>
>>>> what's the conditional actually buying you except more complex code?
>>>
>>> Not much this is an optimization that is simple to carry out. There can
>>> be dozens of such L2 interrupt controllers in a given system and the
>>> MMIO accesses start adding up eventually.
>> 
>> I'm impressed by saving ~12 microseconds per one dozen of interrupt
>> controllers on system shutdown :)
>
> I know, right? More seriously are you willing to take that patch, should 
> I write a better justification?

I don't have real objections other than rolling my eyes, but a better
justification would be useful.