[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[Alexey Brodkin]

Removed code works properly only once after power-up because on every
"first" invocation of "dwmci_init" existing value of "fifo_size" is used
for calculation of itself. More over other bits in the same register
(namely TX watermark and burst size) get corrupted (lost forever till
the next power-toggle).

I understand that usually (in production systems) U-Boot starts only
once and then passes control to Linux kernel etc. But when you do
U-Boot debugging it might be loaded in memory via JTAG multiple times.

Also I wasn't able to find any instance of "host->fifoth_val" usage so I
believe it's safe to just remove this code completely.

Another important thing - in the latest Linux kernel driver for DW MMC
comtroller I don't see any utilization (configuration/modificatoin) of
the register in question (SDMMC_FIFOTH). That's why I think we may want
to keep it untouched as well so we don't force a situation when kernel
driver gets DW MMC unexpectedly configured by U-Boot.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Rajeshwari Shinde <rajeshwari.s@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Amar <amarendra.xt@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Hatim Ali <hatim.rv@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Andy Fleming <afleming@freescale.com>
---
 drivers/mmc/dw_mmc.c | 9 ---------
 include/dwmmc.h      | 1 -
 2 files changed, 10 deletions(-)

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
index 1e0f72b..166124d 100644
--- a/drivers/mmc/dw_mmc.c
+++ b/drivers/mmc/dw_mmc.c
@@ -300,7 +300,6 @@ static void dwmci_set_ios(struct mmc *mmc)
 static int dwmci_init(struct mmc *mmc)
 {
 	struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
-	u32 fifo_size;
 
 	if (host->quirks & DWMCI_QUIRK_DISABLE_SMU) {
 		dwmci_writel(host, EMMCP_MPSBEGIN0, 0);
@@ -330,14 +329,6 @@ static int dwmci_init(struct mmc *mmc)
 	dwmci_writel(host, DWMCI_IDINTEN, 0);
 	dwmci_writel(host, DWMCI_BMOD, 1);
 
-	if (!host->fifoth_val) {
-		fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
-		fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;
-		host->fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size / 2 - 1) |
-			TX_WMARK(fifo_size / 2);
-	}
-	dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
-
 	dwmci_writel(host, DWMCI_CLKENA, 0);
 	dwmci_writel(host, DWMCI_CLKSRC, 0);
 
diff --git a/include/dwmmc.h b/include/dwmmc.h
index 6c91143..20cd1eb 100644
--- a/include/dwmmc.h
+++ b/include/dwmmc.h
@@ -137,7 +137,6 @@ struct dwmci_host {
 	int dev_index;
 	int buswidth;
 	u32 clksel_val;
-	u32 fifoth_val;
 	struct mmc *mmc;
 
 	void (*clksel)(struct dwmci_host *host);
-- 
1.8.4.2

[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[Jaehoon Chung]

Hi, Alexey,

I think good that use the initial fifoth value at register.
Then we need not to change the value.
But according to my experiment, some SoC needs to change the fifoth value.

On 11/27/2013 12:08 AM, Alexey Brodkin wrote:
> Removed code works properly only once after power-up because on every
> "first" invocation of "dwmci_init" existing value of "fifo_size" is used
> for calculation of itself. More over other bits in the same register
> (namely TX watermark and burst size) get corrupted (lost forever till
> the next power-toggle).
> 
> I understand that usually (in production systems) U-Boot starts only
> once and then passes control to Linux kernel etc. But when you do
> U-Boot debugging it might be loaded in memory via JTAG multiple times.
> 
> Also I wasn't able to find any instance of "host->fifoth_val" usage so I
> believe it's safe to just remove this code completely.
If developer is need to change the fifoth value, the use the host->fifoth_val.
So it's existed.

> 
> Another important thing - in the latest Linux kernel driver for DW MMC
> comtroller I don't see any utilization (configuration/modificatoin) of
> the register in question (SDMMC_FIFOTH). That's why I think we may want
> to keep it untouched as well so we don't force a situation when kernel
> driver gets DW MMC unexpectedly configured by U-Boot.
Right, if fifoth register is changed into U-boot,
then kernel driver use the value that fifoth register was set to value at U-Boot.
I known that kernel driver can set to the value with dt file.

Best Regards,
Jaehoon Chung
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> Cc: Mischa Jonker <mjonker@synopsys.com>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Rajeshwari Shinde <rajeshwari.s@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Amar <amarendra.xt@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Hatim Ali <hatim.rv@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Andy Fleming <afleming@freescale.com>
> ---
>  drivers/mmc/dw_mmc.c | 9 ---------
>  include/dwmmc.h      | 1 -
>  2 files changed, 10 deletions(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index 1e0f72b..166124d 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -300,7 +300,6 @@ static void dwmci_set_ios(struct mmc *mmc)
>  static int dwmci_init(struct mmc *mmc)
>  {
>  	struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
> -	u32 fifo_size;
>  
>  	if (host->quirks & DWMCI_QUIRK_DISABLE_SMU) {
>  		dwmci_writel(host, EMMCP_MPSBEGIN0, 0);
> @@ -330,14 +329,6 @@ static int dwmci_init(struct mmc *mmc)
>  	dwmci_writel(host, DWMCI_IDINTEN, 0);
>  	dwmci_writel(host, DWMCI_BMOD, 1);
>  
> -	if (!host->fifoth_val) {
> -		fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
> -		fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;
> -		host->fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size / 2 - 1) |
> -			TX_WMARK(fifo_size / 2);
> -	}
> -	dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
> -
>  	dwmci_writel(host, DWMCI_CLKENA, 0);
>  	dwmci_writel(host, DWMCI_CLKSRC, 0);
>  
> diff --git a/include/dwmmc.h b/include/dwmmc.h
> index 6c91143..20cd1eb 100644
> --- a/include/dwmmc.h
> +++ b/include/dwmmc.h
> @@ -137,7 +137,6 @@ struct dwmci_host {
>  	int dev_index;
>  	int buswidth;
>  	u32 clksel_val;
> -	u32 fifoth_val;
>  	struct mmc *mmc;
>  
>  	void (*clksel)(struct dwmci_host *host);
> 

[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[Pantelis Antoniou]

Hi Alexey,

On Nov 27, 2013, at 9:11 AM, Alexey Brodkin wrote:

> Hi Jaehoon,
> 
> On Wed, 2013-11-27 at 14:33 +0900, Jaehoon Chung wrote:
>> Hi, Alexey,
>> 
>> I think good that use the initial fifoth value at register.
>> Then we need not to change the value.
>> But according to my experiment, some SoC needs to change the fifoth value.
>> 
> 
> Ok, so let's leave possibility to set "fifoth" manually (if
> "host->fifoth_val" != 0 then use provided value; else do nothing) on mmc
> controller instantiation.
> 
> Will it work for you?
> 

ATM if no host->fifoth_val value is provided the code will calculate one and write it.
Otherwise it will write the one configured.

> Still I have a couple of questions to you:
> 1. Why there's no example/instance of "host->fifoth_val" setup/usage in
> U-Boot source tree? If there was at least one I would leave
> "host->fifoth_val".
> 

I would guess it's because everyone uses the default setting of 0 which
results in a somewhat sane value.

> 2. How come there's no way to set/modify the register in question in
> Linux kernel? Don't people use Linux kernel on mentioned selected SoCs
> without U-Boot (i.e. "fifoth" gets set by U-Boot before kernel starts)?
> 

That's a good question for the kernel driver devs.

> Regards,
> Alexey

Regards

-- Pantelis

[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[Pantelis Antoniou]

Hi Alexey,

On Nov 27, 2013, at 9:45 AM, Alexey Brodkin wrote:

> Hi Pantelis,
> 
> On Wed, 2013-11-27 at 09:21 +0200, Pantelis Antoniou wrote:
>> Hi Alexey,
>> 
>> On Nov 27, 2013, at 9:11 AM, Alexey Brodkin wrote:
>> 
> 
>> ATM if no host->fifoth_val value is provided the code will calculate one and write it.
>> Otherwise it will write the one configured.
> 
> Indeed. But what I faced - if you try to run U-Boot more than 1 time
> after powering up your board then this calculated value will be
> calculated improperly. And in my comment to the patch itself I described
> it.
> 
> So there're 2 options:
> 1. Re-implement calculation in such a way so it gets the same value on
> 2nd, 3rd and other U-Boot runs. Moreover it's not clear to me how this
> calculation is intended to work?
> 
> We take some initial value and then modify it - note it's not simply
> overrided with some value, but initial/default value is read from the
> register, then goes some math, and then we put new value back.
> 
> How do we know which value/configuration was by default so we do
> particular modifications later? What is a logic behind this?
> 

Indeed this is bogus. What is the reset value of the fifoth register?

Instead of reading the register, and doing the weird calculations, just
use the reset value of the register as if out of power on reset. 

> So why we need this functionality (unclear reconfiguration) if default
> register value works flawlessly in majority of cases and for some corner
> cases users may specify their custom value that will simply override
> default value.
> 

Beats me. Let's fix this properly.

>>> Still I have a couple of questions to you:
>>> 1. Why there's no example/instance of "host->fifoth_val" setup/usage in
>>> U-Boot source tree? If there was at least one I would leave
>>> "host->fifoth_val".
>>> 
>> 
>> I would guess it's because everyone uses the default setting of 0 which
>> results in a somewhat sane value.
> 
> Regards,
> Alexey

Regards

-- Pantelis

[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[Jaehoon Chung]

Hi, Alexey.

On 11/27/2013 04:45 PM, Alexey Brodkin wrote:
> Hi Pantelis,
> 
> On Wed, 2013-11-27 at 09:21 +0200, Pantelis Antoniou wrote:
>> Hi Alexey,
>>
>> On Nov 27, 2013, at 9:11 AM, Alexey Brodkin wrote:
>>
> 
>> ATM if no host->fifoth_val value is provided the code will calculate one and write it.
>> Otherwise it will write the one configured.
> 
> Indeed. But what I faced - if you try to run U-Boot more than 1 time
> after powering up your board then this calculated value will be
> calculated improperly. And in my comment to the patch itself I described
> it.
I understood your point. It's possible. If we set to calculated value, 
then it's set to value lower than previous value. right?
if host->fifoth_val is always used somewhere, this value should be fixed.
But host->fifoth val was not used anywhere, so need to calculate the value.
This is problem.

Best Regards,
Jaehoon Chung

> 
> So there're 2 options:
> 1. Re-implement calculation in such a way so it gets the same value on
> 2nd, 3rd and other U-Boot runs. Moreover it's not clear to me how this
> calculation is intended to work?
> 
> We take some initial value and then modify it - note it's not simply
> overrided with some value, but initial/default value is read from the
> register, then goes some math, and then we put new value back.
> 
> How do we know which value/configuration was by default so we do
> particular modifications later? What is a logic behind this?
> 
> So why we need this functionality (unclear reconfiguration) if default
> register value works flawlessly in majority of cases and for some corner
> cases users may specify their custom value that will simply override
> default value.
> 
>>> Still I have a couple of questions to you:
>>> 1. Why there's no example/instance of "host->fifoth_val" setup/usage in
>>> U-Boot source tree? If there was at least one I would leave
>>> "host->fifoth_val".
>>>
>>
>> I would guess it's because everyone uses the default setting of 0 which
>> results in a somewhat sane value.
> 
> Regards,
> Alexey
> 

[U-Boot] [PATCH] mmc/dwmmc: remove recursive FIFO threshold setup

[Alexey Brodkin]

Hi Pantelis,

Indeed this is bogus. What is the reset value of the fifoth register?
> Below is a description for the register in question with default
values specified.
----------------------------------------------
FIFO Threshold Watermark Register
----------------------------------------------
x - reserved
 000 - Burst size of multiple transaction
    yyyyyyyyyyy - Rx watermark = (FIFO_DEPTH - 1)
               xxxx - reserved
                   000000000000 - Tx watermark
----------------------------------------------

Also databook says:
----------------------------------------------
During end of packet, request is generated regardless of threshold
programming in order to complete any remaining data.
----------------------------------------------
So it's safe to leave watermark = 0.

Instead of reading the register, and doing the weird calculations, just
> use the reset value of the register as if out of power on reset. 
> 
In other words both Rx and Tx watermarks should be sane for usage as
they are on power-on.

> So why we need this functionality (unclear reconfiguration) if default
> > register value works flawlessly in majority of cases and for some
corner
> > cases users may specify their custom value that will simply override
> > default value.
> > 
> 
> Beats me. Let's fix this properly.
> 
So I'll submit v2 shortly.

-Alexey