[PATCH 1/3] mips: mt76x8: ddr_cal: Rename dqs_test_valid() to dqs_test_error()
This function returns "-1" (true) upon error. So the function name does not match its implementation which is confusing. This patch renames the function to dqs_test_error() which makes the code easier to read.
Also change the return type to bool and return "true" or "false".
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com --- arch/mips/mach-mtmips/ddr_cal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/mips/mach-mtmips/ddr_cal.c b/arch/mips/mach-mtmips/ddr_cal.c index 0ea7c7d5e1..b934c4a854 100644 --- a/arch/mips/mach-mtmips/ddr_cal.c +++ b/arch/mips/mach-mtmips/ddr_cal.c @@ -32,8 +32,8 @@ static inline void pref_op(int op, const volatile void *addr) __asm__ __volatile__("pref %0, 0(%1)" : : "i" (op), "r" (addr)); }
-static inline int dqs_test_valid(void __iomem *memc, u32 memsize, u32 dqsval, - u32 bias) +static inline bool dqs_test_error(void __iomem *memc, u32 memsize, u32 dqsval, + u32 bias) { u32 *nca, *ca; u32 off; @@ -64,11 +64,11 @@ static inline int dqs_test_valid(void __iomem *memc, u32 memsize, u32 dqsval,
for (i = 0; i < TEST_PAT_SIZE / sizeof(u32); i++) { if (ca[i] != (u32)nca + i + bias) - return -1; + return true; } }
- return 0; + return false; }
static inline u32 dqs_find_max(void __iomem *memc, u32 memsize, u32 initval, @@ -79,7 +79,7 @@ static inline u32 dqs_find_max(void __iomem *memc, u32 memsize, u32 initval, do { dqsval = regval | (fieldval << shift);
- if (dqs_test_valid(memc, memsize, dqsval, 3)) + if (dqs_test_error(memc, memsize, dqsval, 3)) break;
fieldval++; @@ -96,7 +96,7 @@ static inline u32 dqs_find_min(void __iomem *memc, u32 memsize, u32 initval, while (fieldval > minval) { dqsval = regval | (fieldval << shift);
- if (dqs_test_valid(memc, memsize, dqsval, 1)) { + if (dqs_test_error(memc, memsize, dqsval, 1)) { fieldval++; break; }
This change is made to enable comparison of integer variables, which might be negative in the next patch. No functional change is intended in this patch.
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com --- arch/mips/mach-mtmips/ddr_cal.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/mips/mach-mtmips/ddr_cal.c b/arch/mips/mach-mtmips/ddr_cal.c index b934c4a854..80a058d693 100644 --- a/arch/mips/mach-mtmips/ddr_cal.c +++ b/arch/mips/mach-mtmips/ddr_cal.c @@ -71,10 +71,11 @@ static inline bool dqs_test_error(void __iomem *memc, u32 memsize, u32 dqsval, return false; }
-static inline u32 dqs_find_max(void __iomem *memc, u32 memsize, u32 initval, - u32 maxval, u32 shift, u32 regval) +static inline int dqs_find_max(void __iomem *memc, u32 memsize, int initval, + int maxval, int shift, u32 regval) { - u32 fieldval = initval, dqsval; + int fieldval = initval; + u32 dqsval;
do { dqsval = regval | (fieldval << shift); @@ -88,10 +89,11 @@ static inline u32 dqs_find_max(void __iomem *memc, u32 memsize, u32 initval, return fieldval; }
-static inline u32 dqs_find_min(void __iomem *memc, u32 memsize, u32 initval, - u32 minval, u32 shift, u32 regval) +static inline int dqs_find_min(void __iomem *memc, u32 memsize, int initval, + int minval, int shift, u32 regval) { - u32 fieldval = initval, dqsval; + int fieldval = initval; + u32 dqsval;
while (fieldval > minval) { dqsval = regval | (fieldval << shift);
On Fri, 2020-03-06 at 15:14 +0100, Stefan Roese wrote:
This change is made to enable comparison of integer variables, which might be negative in the next patch. No functional change is intended in this patch.
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
arch/mips/mach-mtmips/ddr_cal.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
Reviewed-by: Weijie Gao weijie.gao@mediatek.com
The current implementations have some issues detecting the correct values:
dqs_find_max() will return "last passing fieldval + 1" instead of "last passing fieldval". Also it will return "maxval + 1" in the case that all fieldvals are tested valid (without error).
dqs_find_min() will not test the "lowest" value because of using ">" instead of ">=".
This patch now rewrites these functions to fix those issues. Also, this patch uses the same approach of a for loop in both functions making it easier to read and maintain.
Since the variables are integers now, we can use min()/max(), which handles the wrap around case for fieldval=0: return (0 - 1).
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com --- arch/mips/mach-mtmips/ddr_cal.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/arch/mips/mach-mtmips/ddr_cal.c b/arch/mips/mach-mtmips/ddr_cal.c index 80a058d693..71a53c3c9c 100644 --- a/arch/mips/mach-mtmips/ddr_cal.c +++ b/arch/mips/mach-mtmips/ddr_cal.c @@ -74,39 +74,31 @@ static inline bool dqs_test_error(void __iomem *memc, u32 memsize, u32 dqsval, static inline int dqs_find_max(void __iomem *memc, u32 memsize, int initval, int maxval, int shift, u32 regval) { - int fieldval = initval; + int fieldval; u32 dqsval;
- do { + for (fieldval = initval; fieldval <= maxval; fieldval++) { dqsval = regval | (fieldval << shift); - if (dqs_test_error(memc, memsize, dqsval, 3)) - break; - - fieldval++; - } while (fieldval <= maxval); + return max(fieldval - 1, initval); + }
- return fieldval; + return maxval; }
static inline int dqs_find_min(void __iomem *memc, u32 memsize, int initval, int minval, int shift, u32 regval) { - int fieldval = initval; + int fieldval; u32 dqsval;
- while (fieldval > minval) { + for (fieldval = initval; fieldval >= minval; fieldval--) { dqsval = regval | (fieldval << shift); - - if (dqs_test_error(memc, memsize, dqsval, 1)) { - fieldval++; - break; - } - - fieldval--; + if (dqs_test_error(memc, memsize, dqsval, 1)) + return min(fieldval + 1, initval); }
- return fieldval; + return minval; }
void ddr_calibrate(void __iomem *memc, u32 memsize, u32 bw)
On Fri, 2020-03-06 at 15:14 +0100, Stefan Roese wrote:
The current implementations have some issues detecting the correct values:
dqs_find_max() will return "last passing fieldval + 1" instead of "last passing fieldval". Also it will return "maxval + 1" in the case that all fieldvals are tested valid (without error).
dqs_find_min() will not test the "lowest" value because of using ">" instead of ">=".
This patch now rewrites these functions to fix those issues. Also, this patch uses the same approach of a for loop in both functions making it easier to read and maintain.
Since the variables are integers now, we can use min()/max(), which handles the wrap around case for fieldval=0: return (0 - 1).
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
arch/mips/mach-mtmips/ddr_cal.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)
Reviewed-by: Weijie Gao weijie.gao@mediatek.com
On Fri, 2020-03-06 at 15:14 +0100, Stefan Roese wrote:
This function returns "-1" (true) upon error. So the function name does not match its implementation which is confusing. This patch renames the function to dqs_test_error() which makes the code easier to read.
Also change the return type to bool and return "true" or "false".
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
arch/mips/mach-mtmips/ddr_cal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Weijie Gao weijie.gao@mediatek.com
Hi Weijie,
thanks for reviewing. Could you please also review the latest RFC version of my "generification" of the legacy lzma support as well? I really would like to see some review comments before sending v7 of this patchset to get it (hopefully) accepted in this merge window.
http://patchwork.ozlabs.org/project/uboot/patch/20200410110431.12256-1-sr@de... http://patchwork.ozlabs.org/project/uboot/patch/20200410110431.12256-2-sr@de... http://patchwork.ozlabs.org/project/uboot/patch/20200410110431.12256-2-sr@de...
Thanks, Stefan
On 15.04.20 09:19, Weijie Gao wrote:
On Fri, 2020-03-06 at 15:14 +0100, Stefan Roese wrote:
This function returns "-1" (true) upon error. So the function name does not match its implementation which is confusing. This patch renames the function to dqs_test_error() which makes the code easier to read.
Also change the return type to bool and return "true" or "false".
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
arch/mips/mach-mtmips/ddr_cal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Weijie Gao weijie.gao@mediatek.com
Viele Grüße, Stefan
Am 06.03.20 um 15:14 schrieb Stefan Roese:
This function returns "-1" (true) upon error. So the function name does not match its implementation which is confusing. This patch renames the function to dqs_test_error() which makes the code easier to read.
Also change the return type to bool and return "true" or "false".
Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com
arch/mips/mach-mtmips/ddr_cal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
series applied to u-boot-mips/next
participants (3)
-
Daniel Schwierzeck -
Stefan Roese -
Weijie Gao