[PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[Jaehoon Chung]

Even though there are candiates value if can't find best value, it's
returned -EIO. It's not proper behavior.
If there is not best value, use a first candiate value to work eMMC.

Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Changelog V2:
- Add Marek's Tested-by tag
- Remove unnecessary code

 drivers/mmc/host/dw_mmc-exynos.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 0c75810812a0..1f8a3c0ddfe1 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -464,6 +464,18 @@ static s8 dw_mci_exynos_get_best_clksmpl(u8 candiates)
 		}
 	}
 
+	/*
+	 * If there is no cadiates value, then it needs to return -EIO.
+	 * If there are candiates values and don't find bset clk sample value,
+	 * then use a first candiates clock sample value.
+	 */
+	for (i = 0; i < iter; i++) {
+		__c = ror8(candiates, i);
+		if ((__c & 0x1) == 0x1) {
+			loc = i;
+			goto out;
+		}
+	}
 out:
 	return loc;
 }
@@ -494,6 +506,8 @@ static int dw_mci_exynos_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 		priv->tuned_sample = found;
 	} else {
 		ret = -EIO;
+		dev_warn(&mmc->class_dev,
+			"There is no candiates value about clksmpl!\n");
 	}
 
 	return ret;
-- 
2.29.0

Re: [PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[Christian Hewitt]

> On 22 Oct 2021, at 12:21 pm, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> 
> Even though there are candiates value if can't find best value, it's
> returned -EIO. It's not proper behavior.
> If there is not best value, use a first candiate value to work eMMC.
> 
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Christian Hewitt <christianshewitt@gmail.com>

v2 patch working fine with the module that triggered the original report:

[    2.902144] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
[    2.912118] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
[    3.142474] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz, actual 200000000HZ div = 0)
[    3.239339] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0)
[    3.241388] mmc_host mmc0: Bus speed (slot 0) = 266666666Hz (slot req 200000000Hz, actual 133333333HZ div = 1)
[    3.243310] mmc0: new HS400 MMC card at address 0001
[    3.259191] mmcblk0: mmc0:0001 8GME4R 7.28 GiB 
[    3.302621]  mmcblk0: p1 p2
[    3.311541] mmcblk0boot0: mmc0:0001 8GME4R 4.00 MiB 
[    3.327737] mmcblk0boot1: mmc0:0001 8GME4R 4.00 MiB 
[    3.340919] mmcblk0rpmb: mmc0:0001 8GME4R 512 KiB, chardev (246:0)

Thanks!

> ---
> Changelog V2:
> - Add Marek's Tested-by tag
> - Remove unnecessary code
> 
> drivers/mmc/host/dw_mmc-exynos.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index 0c75810812a0..1f8a3c0ddfe1 100644
> --- a/drivers/mmc/host/dw_mmc-exynos.c
> +++ b/drivers/mmc/host/dw_mmc-exynos.c
> @@ -464,6 +464,18 @@ static s8 dw_mci_exynos_get_best_clksmpl(u8 candiates)
> 		}
> 	}
> 
> +	/*
> +	 * If there is no cadiates value, then it needs to return -EIO.
> +	 * If there are candiates values and don't find bset clk sample value,
> +	 * then use a first candiates clock sample value.
> +	 */
> +	for (i = 0; i < iter; i++) {
> +		__c = ror8(candiates, i);
> +		if ((__c & 0x1) == 0x1) {
> +			loc = i;
> +			goto out;
> +		}
> +	}
> out:
> 	return loc;
> }
> @@ -494,6 +506,8 @@ static int dw_mci_exynos_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
> 		priv->tuned_sample = found;
> 	} else {
> 		ret = -EIO;
> +		dev_warn(&mmc->class_dev,
> +			"There is no candiates value about clksmpl!\n");
> 	}
> 
> 	return ret;
> -- 
> 2.29.0
> 

Re: [PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[Marek Szyprowski]

Hi,

On 22.10.2021 12:53, Christian Hewitt wrote:
>> On 22 Oct 2021, at 12:21 pm, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>
>> Even though there are candiates value if can't find best value, it's
>> returned -EIO. It's not proper behavior.
>> If there is not best value, use a first candiate value to work eMMC.
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Christian Hewitt <christianshewitt@gmail.com>
>
> v2 patch working fine with the module that triggered the original report:
>
> [    2.902144] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
> [    2.912118] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
> [    3.142474] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz, actual 200000000HZ div = 0)
> [    3.239339] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0)
> [    3.241388] mmc_host mmc0: Bus speed (slot 0) = 266666666Hz (slot req 200000000Hz, actual 133333333HZ div = 1)

I wonder why 266666666Hz bus speed is selected instead of the 
400000000Hz one. Did you remove the workaround patch which changed the 
divider value from your kernel tree? I didn't analyze the code, so maybe 
this change is intentional result of this patch? On my XU4 I get 
400000000Hz bus clock for the eMMC dw-mmc controller.

> [    3.243310] mmc0: new HS400 MMC card at address 0001
> [    3.259191] mmcblk0: mmc0:0001 8GME4R 7.28 GiB
> [    3.302621]  mmcblk0: p1 p2
> [    3.311541] mmcblk0boot0: mmc0:0001 8GME4R 4.00 MiB
> [    3.327737] mmcblk0boot1: mmc0:0001 8GME4R 4.00 MiB
> [    3.340919] mmcblk0rpmb: mmc0:0001 8GME4R 512 KiB, chardev (246:0)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[Christian Hewitt]

> On 22 Oct 2021, at 3:35 pm, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> Hi,
> 
> On 22.10.2021 12:53, Christian Hewitt wrote:
>>> On 22 Oct 2021, at 12:21 pm, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> 
>>> Even though there are candiates value if can't find best value, it's
>>> returned -EIO. It's not proper behavior.
>>> If there is not best value, use a first candiate value to work eMMC.
>>> 
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Tested-by: Christian Hewitt <christianshewitt@gmail.com>
>> 
>> v2 patch working fine with the module that triggered the original report:
>> 
>> [    2.902144] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
>> [    2.912118] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
>> [    3.142474] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz, actual 200000000HZ div = 0)
>> [    3.239339] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0)
>> [    3.241388] mmc_host mmc0: Bus speed (slot 0) = 266666666Hz (slot req 200000000Hz, actual 133333333HZ div = 1)
> 
> I wonder why 266666666Hz bus speed is selected instead of the 
> 400000000Hz one. Did you remove the workaround patch which changed the 
> divider value from your kernel tree? I didn't analyze the code, so maybe 
> this change is intentional result of this patch? On my XU4 I get 
> 400000000Hz bus clock for the eMMC dw-mmc controller.

Yes, I removed the workaround patch before testing. It’s delivering
the same result as the workaround so perhaps it’s normal for this
module. All the emmc modules I have (all samples from HK sent at the
same time) are identical so there’s nothing else I can test with.

Christian

>> [    3.243310] mmc0: new HS400 MMC card at address 0001
>> [    3.259191] mmcblk0: mmc0:0001 8GME4R 7.28 GiB
>> [    3.302621]  mmcblk0: p1 p2
>> [    3.311541] mmcblk0boot0: mmc0:0001 8GME4R 4.00 MiB
>> [    3.327737] mmcblk0boot1: mmc0:0001 8GME4R 4.00 MiB
>> [    3.340919] mmcblk0rpmb: mmc0:0001 8GME4R 512 KiB, chardev (246:0)
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

Re: [PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[Jaehoon Chung]

On 10/22/21 8:42 PM, Christian Hewitt wrote:
> 
>> On 22 Oct 2021, at 3:35 pm, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>
>> Hi,
>>
>> On 22.10.2021 12:53, Christian Hewitt wrote:
>>>> On 22 Oct 2021, at 12:21 pm, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>
>>>> Even though there are candiates value if can't find best value, it's
>>>> returned -EIO. It's not proper behavior.
>>>> If there is not best value, use a first candiate value to work eMMC.
>>>>
>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Tested-by: Christian Hewitt <christianshewitt@gmail.com>
>>>
>>> v2 patch working fine with the module that triggered the original report:
>>>
>>> [    2.902144] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
>>> [    2.912118] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63)
>>> [    3.142474] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot req 200000000Hz, actual 200000000HZ div = 0)
>>> [    3.239339] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0)
>>> [    3.241388] mmc_host mmc0: Bus speed (slot 0) = 266666666Hz (slot req 200000000Hz, actual 133333333HZ div = 1)
>>
>> I wonder why 266666666Hz bus speed is selected instead of the 
>> 400000000Hz one. Did you remove the workaround patch which changed the 
>> divider value from your kernel tree? I didn't analyze the code, so maybe 
>> this change is intentional result of this patch? On my XU4 I get 
>> 400000000Hz bus clock for the eMMC dw-mmc controller.
> 
> Yes, I removed the workaround patch before testing. It’s delivering
> the same result as the workaround so perhaps it’s normal for this
> module. All the emmc modules I have (all samples from HK sent at the
> same time) are identical so there’s nothing else I can test with.

As Marek was mentioned, I wonder why 266666666Hz is selected.
On my XU4, it's selected 400MHz as Marek's. 

Best Regards,
Jaehoon Chung

> 
> Christian
> 
>>> [    3.243310] mmc0: new HS400 MMC card at address 0001
>>> [    3.259191] mmcblk0: mmc0:0001 8GME4R 7.28 GiB
>>> [    3.302621]  mmcblk0: p1 p2
>>> [    3.311541] mmcblk0boot0: mmc0:0001 8GME4R 4.00 MiB
>>> [    3.327737] mmcblk0boot1: mmc0:0001 8GME4R 4.00 MiB
>>> [    3.340919] mmcblk0rpmb: mmc0:0001 8GME4R 512 KiB, chardev (246:0)
>>
>> Best regards
>> -- 
>> Marek Szyprowski, PhD
>> Samsung R&D Institute Poland
> 
> 

Re: [PATCH V2] mmc: dw_mmc: exynos: fix the finding clock sample value

[Ulf Hansson]

On Fri, 22 Oct 2021 at 10:20, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>
> Even though there are candiates value if can't find best value, it's
> returned -EIO. It's not proper behavior.
> If there is not best value, use a first candiate value to work eMMC.
>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Applied for fixes and by adding the below tags, thanks!

Cc: stable@vger.kernel.org
Fixes: c537a1c5ff63 ("mmc: dw_mmc: exynos: add variable delay tuning sequence")

Please tell me if that doesn't make sense!

Kind regards
Uffe


> ---
> Changelog V2:
> - Add Marek's Tested-by tag
> - Remove unnecessary code
>
>  drivers/mmc/host/dw_mmc-exynos.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index 0c75810812a0..1f8a3c0ddfe1 100644
> --- a/drivers/mmc/host/dw_mmc-exynos.c
> +++ b/drivers/mmc/host/dw_mmc-exynos.c
> @@ -464,6 +464,18 @@ static s8 dw_mci_exynos_get_best_clksmpl(u8 candiates)
>                 }
>         }
>
> +       /*
> +        * If there is no cadiates value, then it needs to return -EIO.
> +        * If there are candiates values and don't find bset clk sample value,
> +        * then use a first candiates clock sample value.
> +        */
> +       for (i = 0; i < iter; i++) {
> +               __c = ror8(candiates, i);
> +               if ((__c & 0x1) == 0x1) {
> +                       loc = i;
> +                       goto out;
> +               }
> +       }
>  out:
>         return loc;
>  }
> @@ -494,6 +506,8 @@ static int dw_mci_exynos_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>                 priv->tuned_sample = found;
>         } else {
>                 ret = -EIO;
> +               dev_warn(&mmc->class_dev,
> +                       "There is no candiates value about clksmpl!\n");
>         }
>
>         return ret;
> --
> 2.29.0
>