diff mbox series

[v1,2/2] scsi: ufs: disable interrupt during clock-gating

Message ID 1575721321-8071-3-git-send-email-stanley.chu@mediatek.com (mailing list archive)
State Mainlined
Commit 8b0bbf002a1eb9074ab770e21cd454137c76c106
Headers show
Series scsi: ufs: fixup active period of ufshcd interrupt | expand

Commit Message

Stanley Chu Dec. 7, 2019, 12:22 p.m. UTC
Similar to suspend, ufshcd interrupt can be disabled since
there won't be any host controller transaction expected till
clocks ungated.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Asutosh Das (asd) Dec. 17, 2019, 11:25 p.m. UTC | #1
On 12/7/2019 4:22 AM, Stanley Chu wrote:
> Similar to suspend, ufshcd interrupt can be disabled since
> there won't be any host controller transaction expected till
> clocks ungated.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>   drivers/scsi/ufs/ufshcd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f80bd4e811cb..5de105e82c32 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1490,6 +1490,8 @@ static void ufshcd_ungate_work(struct work_struct *work)
>   	spin_unlock_irqrestore(hba->host->host_lock, flags);
>   	ufshcd_setup_clocks(hba, true);
>   
> +	ufshcd_enable_irq(hba);
> +
>   	/* Exit from hibern8 */
>   	if (ufshcd_can_hibern8_during_gating(hba)) {
>   		/* Prevent gating in this path */
> @@ -1636,6 +1638,8 @@ static void ufshcd_gate_work(struct work_struct *work)
>   		ufshcd_set_link_hibern8(hba);
>   	}
>   
> +	ufshcd_disable_irq(hba);
> +
>   	if (!ufshcd_is_link_active(hba))
>   		ufshcd_setup_clocks(hba, false);
>   	else
> 

Hi,
Does this save significant power? I see that gate/ungate of clocks 
happen far too frequently than suspend/resume.

Have you considered how much latency this would add to the 
gating/ungating path?

-asd
Stanley Chu Dec. 18, 2019, 3:52 a.m. UTC | #2
Hi Asutosh,

On Tue, 2019-12-17 at 15:25 -0800, Asutosh Das (asd) wrote:
> > 
> 
> Hi,
> Does this save significant power? I see that gate/ungate of clocks 
> happen far too frequently than suspend/resume.
> 
> Have you considered how much latency this would add to the 
> gating/ungating path?
> 
> -asd
> 

Yes, we have measured 200 times clk-gating/clk-ungating and latency data
is showed as below,

For clk-gating with interrupt disabling toggled,

	Average latency of each clk-gating: 55.117 us
	Average latency of irq-disabling during clk-gating: 4.2 us

For clk-ungating with interrupt enabling toggled,
	Average latency of each clk-ungating: 118.324 us
	Average latency of irq-enabling during clk-ungating: 2.9 us

The evaluation here is based on below Can's patch therefore the
interrupt control (enable_irq/disable_irq) latency is much shorter than
before (request_irq/free_irq).

scsi: ufs: Do not free irq in suspend
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/scsi/ufs/ufshcd.c?id=8709c1f68536e256668812788af5b2bb027f49c3

BTW, the main purpose of this patch is aimed to protect ufshcd register
from accessing while host clocks are disabled to fix potential system
hang issue. The possible scenario is mentioned in commit message of
patch "scsi: ufs: disable irq before disabling clocks" in the same
series.

Thanks
Stanley
Asutosh Das (asd) Dec. 18, 2019, 11:37 p.m. UTC | #3
On 12/17/2019 7:52 PM, Stanley Chu wrote:
> Hi Asutosh,
> 
> On Tue, 2019-12-17 at 15:25 -0800, Asutosh Das (asd) wrote:
>>>
>>
>> Hi,
>> Does this save significant power? I see that gate/ungate of clocks
>> happen far too frequently than suspend/resume.
>>
>> Have you considered how much latency this would add to the
>> gating/ungating path?
>>
>> -asd
>>
> 
> Yes, we have measured 200 times clk-gating/clk-ungating and latency data
> is showed as below,
> 
> For clk-gating with interrupt disabling toggled,
> 
> 	Average latency of each clk-gating: 55.117 us
> 	Average latency of irq-disabling during clk-gating: 4.2 us
> 
> For clk-ungating with interrupt enabling toggled,
> 	Average latency of each clk-ungating: 118.324 us
> 	Average latency of irq-enabling during clk-ungating: 2.9 us
> 
> The evaluation here is based on below Can's patch therefore the
> interrupt control (enable_irq/disable_irq) latency is much shorter than
> before (request_irq/free_irq).
> 
> scsi: ufs: Do not free irq in suspend
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/scsi/ufs/ufshcd.c?id=8709c1f68536e256668812788af5b2bb027f49c3
> 
> BTW, the main purpose of this patch is aimed to protect ufshcd register
> from accessing while host clocks are disabled to fix potential system
> hang issue. The possible scenario is mentioned in commit message of
> patch "scsi: ufs: disable irq before disabling clocks" in the same
> series.
> 
> Thanks
> Stanley
> 

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

Thanks for the data.
It'd be interesting to know more on the - misrouted interrupt recovery 
feature though.

-
Thanks
Asutosh
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f80bd4e811cb..5de105e82c32 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1490,6 +1490,8 @@  static void ufshcd_ungate_work(struct work_struct *work)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_setup_clocks(hba, true);
 
+	ufshcd_enable_irq(hba);
+
 	/* Exit from hibern8 */
 	if (ufshcd_can_hibern8_during_gating(hba)) {
 		/* Prevent gating in this path */
@@ -1636,6 +1638,8 @@  static void ufshcd_gate_work(struct work_struct *work)
 		ufshcd_set_link_hibern8(hba);
 	}
 
+	ufshcd_disable_irq(hba);
+
 	if (!ufshcd_is_link_active(hba))
 		ufshcd_setup_clocks(hba, false);
 	else