mmc: dw_mmc: reduce timeout detection cycle

PATCH New Ziyuan Xu 3 participants 8 messages Jul 19, 2016 – Jul 25, 2016 MMC, SD and SDIO

lore

Share & tools

lore

Ziyuan Xu

author

2016-07-19 01:38 UTC #0 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.

Signed-off-by Ziyuan Xu <[email protected]>

---

 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
 				 data ? DIV_ROUND_UP(data->blocks, 8) : 0);
 	int ret = 0, flags = 0, i;
-	unsigned int timeout = 100000;
+	unsigned int timeout = 500;
 	u32 retry = 100000;
 	u32 mask, ctrl;
 	ulong start = get_timer(0);

Signature · 1 line

1.9.1

Jaehoon Chung

2016-07-19 02:03 UTC #1 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Hi Ziyuan,

Quoted 2 lines · Ziyuan Xu

It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.

I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?

Best Regards,
Jaehoon Chung

Quoted 21 lines

Signed-off-by Ziyuan Xu <[email protected]>

---

 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
 				 data ? DIV_ROUND_UP(data->blocks, 8) : 0);
 	int ret = 0, flags = 0, i;
-	unsigned int timeout = 100000;
+	unsigned int timeout = 500;
 	u32 retry = 100000;
 	u32 mask, ctrl;
 	ulong start = get_timer(0);

Ziyuan Xu

author

2016-07-19 02:33 UTC #2 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Hi Jaehoon,

Quoted 7 lines · Jaehoon Chung

Hi Ziyuan,
Quoted 2 lines · Ziyuan Xu
It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.
I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?

Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio iomux)
The device will be busy status until gpio iomux.

2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode. timeout = 0xffffff * 1/52M = 0.32s

3. voltage switch
U-BOOT doesn't support voltage switch.

In summary, I think 500 milliseconds is enough. What do you think?

Quoted 27 lines

Best Regards,
Jaehoon Chung

Quoted 20 lines

Signed-off-by Ziyuan Xu <[email protected]>

---

  drivers/mmc/dw_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
  	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
  				 data ? DIV_ROUND_UP(data->blocks, 8) : 0);
  	int ret = 0, flags = 0, i;
-	unsigned int timeout = 100000;
+	unsigned int timeout = 500;
  	u32 retry = 100000;
  	u32 mask, ctrl;
  	ulong start = get_timer(0);

Jaehoon Chung

2016-07-19 04:22 UTC #3 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Hi Ziyuan,

Quoted 14 lines · Ziyuan Xu

Hi Jaehoon,
Quoted 7 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 2 lines · Ziyuan Xu
It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.
I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?
Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio iomux)
The device will be busy status until gpio iomux.

Quoted 8 lines

2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode. timeout = 0xffffff * 1/52M = 0.32s
3. voltage switch
U-BOOT doesn't support voltage switch.
In summary, I think 500 milliseconds is enough. What do you think?

I think it's not important thing.

This is for checking whether card is busy or not before sending command.
I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command. And there is no effect for I/O performance, isn't?

But 50ms is not bad. :) It's personal preference.

Best Regards,
Jaehoon Chung

Quoted 33 lines

Quoted 28 lines

Best Regards,
Jaehoon Chung

Quoted 20 lines

Signed-off-by Ziyuan Xu <[email protected]>

---

  drivers/mmc/dw_mmc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
      ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
                   data ? DIV_ROUND_UP(data->blocks, 8) : 0);
      int ret = 0, flags = 0, i;
-    unsigned int timeout = 100000;
+    unsigned int timeout = 500;
      u32 retry = 100000;
      u32 mask, ctrl;
      ulong start = get_timer(0);

Ziyuan Xu

author

2016-07-19 07:40 UTC #4 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Hi Jaehoon,

Quoted 24 lines · Jaehoon Chung

Hi Ziyuan,
Quoted 21 lines · Ziyuan Xu
Hi Jaehoon,
Quoted 7 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 2 lines · Ziyuan Xu
It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.
I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?
Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio iomux)
The device will be busy status until gpio iomux.
2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode. timeout = 0xffffff * 1/52M = 0.32s
3. voltage switch
U-BOOT doesn't support voltage switch.

Quoted 6 lines

Quoted 1 line
In summary, I think 500 milliseconds is enough. What do you think?
I think it's not important thing.
This is for checking whether card is busy or not before sending command.
I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command. And there is no effect for I/O performance, isn't?

Yup, I agree with you. For scenarios 2, I mean that if the last command with data transfer, we will hit data_busy assertion probably. If the mmc device remains in a busy state more than 500ms, I think it may also be busy state after 10s.

Quoted 2 lines

But 50ms is not bad. :) It's personal preference.

BTW, the timeout value is 500ms in kernel.

Quoted 36 lines

Best Regards,
Jaehoon Chung

Quoted 29 lines

Quoted 25 lines

Best Regards,
Jaehoon Chung

Quoted 20 lines

Signed-off-by Ziyuan Xu <[email protected]>

---

   drivers/mmc/dw_mmc.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
       ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
                    data ? DIV_ROUND_UP(data->blocks, 8) : 0);
       int ret = 0, flags = 0, i;
-    unsigned int timeout = 100000;
+    unsigned int timeout = 500;
       u32 retry = 100000;
       u32 mask, ctrl;
       ulong start = get_timer(0);

Jaehoon Chung

2016-07-19 07:51 UTC #5 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Hi,

Quoted 30 lines · Ziyuan Xu

Hi Jaehoon,
Quoted 27 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 14 lines · Ziyuan Xu
Hi Jaehoon,
Quoted 7 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 2 lines · Ziyuan Xu
It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.
I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?
Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio iomux)
The device will be busy status until gpio iomux.
Quoted 8 lines
2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode. timeout = 0xffffff * 1/52M = 0.32s
3. voltage switch
U-BOOT doesn't support voltage switch.
In summary, I think 500 milliseconds is enough. What do you think?
I think it's not important thing.

Quoted 7 lines

Quoted 3 lines
This is for checking whether card is busy or not before sending command.
I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command. And there is no effect for I/O performance, isn't?
Yup, I agree with you. For scenarios 2, I mean that if the last command with data transfer, we will hit data_busy assertion probably. If the mmc device remains in a busy state more than 500ms, I think it may also be busy state after 10s.
Quoted 2 lines
But 50ms is not bad. :) It's personal preference.
BTW, the timeout value is 500ms in kernel.

Yep, it looks good to me. :)
I have tested your patch with Exynos SoCs.

Reviewed-by Jaehoon Chung <[email protected]>

Tested-by Jaehoon Chung <[email protected]>

Best Regards,
Jaehoon Chung

Quoted 41 lines

Quoted 36 lines

Best Regards,
Jaehoon Chung

Quoted 29 lines

Quoted 25 lines

Best Regards,
Jaehoon Chung

Quoted 20 lines

Signed-off-by Ziyuan Xu <[email protected]>

---

   drivers/mmc/dw_mmc.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
       ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
                    data ? DIV_ROUND_UP(data->blocks, 8) : 0);
       int ret = 0, flags = 0, i;
-    unsigned int timeout = 100000;
+    unsigned int timeout = 500;
       u32 retry = 100000;
       u32 mask, ctrl;
       ulong start = get_timer(0);

Ziyuan Xu

author

2016-07-19 08:08 UTC #6 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Hi Jaehoon,

Quoted 36 lines · Jaehoon Chung

Hi,
Quoted 33 lines · Ziyuan Xu
Hi Jaehoon,
Quoted 30 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 14 lines · Ziyuan Xu
Hi Jaehoon,
Quoted 7 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 2 lines · Ziyuan Xu
It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.
I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?
Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio iomux)
The device will be busy status until gpio iomux.
Quoted 8 lines
2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode. timeout = 0xffffff * 1/52M = 0.32s
3. voltage switch
U-BOOT doesn't support voltage switch.
In summary, I think 500 milliseconds is enough. What do you think?
I think it's not important thing.
This is for checking whether card is busy or not before sending command.
I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command. And there is no effect for I/O performance, isn't?

Quoted 8 lines

Quoted 3 lines
Yup, I agree with you. For scenarios 2, I mean that if the last command with data transfer, we will hit data_busy assertion probably. If the mmc device remains in a busy state more than 500ms, I think it may also be busy state after 10s.
Quoted 1 line
But 50ms is not bad. :) It's personal preference.
BTW, the timeout value is 500ms in kernel.
Yep, it looks good to me. :)
I have tested your patch with Exynos SoCs.
Reviewed-by Jaehoon Chung <[email protected]>
Tested-by Jaehoon Chung <[email protected]>

Thanks for your test and review!

Quoted 43 lines

Best Regards,
Jaehoon Chung

Quoted 36 lines

Quoted 32 lines

Best Regards,
Jaehoon Chung

Quoted 27 lines

Quoted 24 lines

Best Regards,
Jaehoon Chung

Quoted 20 lines

Signed-off-by Ziyuan Xu <[email protected]>

---

    drivers/mmc/dw_mmc.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mmc/dw_mmc.c +1 -1

index 2cf7bae..790a166 100644
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
        ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
                     data ? DIV_ROUND_UP(data->blocks, 8) : 0);
        int ret = 0, flags = 0, i;
-    unsigned int timeout = 100000;
+    unsigned int timeout = 500;
        u32 retry = 100000;
        u32 mask, ctrl;
        ulong start = get_timer(0);

Simon Glass

2016-07-25 02:07 UTC #7 lore raw

PATCH 1/1 mmc: dw_mmc: reduce timeout detection cycle

Quoted 62 lines · Ziyuan Xu

Hi Jaehoon,
Quoted 58 lines · Jaehoon Chung
Hi,
Quoted 53 lines · Ziyuan Xu
Hi Jaehoon,
Quoted 32 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 28 lines · Ziyuan Xu
Hi Jaehoon,
Quoted 10 lines · Jaehoon Chung
Hi Ziyuan,
Quoted 3 lines · Ziyuan Xu
It's no need to speed 10 seconds to wait the mmc device out from busy status. 500 milliseconds enough.
I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?
Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio iomux) The device will be busy status until gpio iomux.
2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode. timeout = 0xffffff * 1/52M = 0.32s
3. voltage switch
U-BOOT doesn't support voltage switch.
Quoted 8 lines
Quoted 1 line
In summary, I think 500 milliseconds is enough. What do you think?
I think it's not important thing.
This is for checking whether card is busy or not before sending command. I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command.
And there is no effect for I/O performance, isn't?
Yup, I agree with you. For scenarios 2, I mean that if the last command with data transfer, we will hit data_busy assertion probably. If the mmc device remains in a busy state more than 500ms, I think it may also be busy state after 10s.
Quoted 2 lines
But 50ms is not bad. :) It's personal preference.
BTW, the timeout value is 500ms in kernel.

Quoted 8 lines

Quoted 5 lines
Yep, it looks good to me. :)
I have tested your patch with Exynos SoCs.
Reviewed-by Jaehoon Chung <[email protected]>
Tested-by Jaehoon Chung <[email protected]>
Thanks for your test and review!

Let's get others to test this.

Applied to u-boot-dm, thanks!