[PATCH 1/3] mips: mt76x8: ddr_cal: Rename dqs_test_valid() to dqs_test_error()

[PATCH 1/3] mips: mt76x8: ddr_cal: Rename dqs_test_valid() to dqs_test_error()

[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(-)

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;
 		}
-- 
2.25.1

[PATCH 2/3] mips: mt76x8: ddr_cal: Change types from u32 to int in dqs_find_min/max

[Stefan Roese]

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);
-- 
2.25.1

[PATCH 2/3] mips: mt76x8: ddr_cal: Change types from u32 to int in dqs_find_min/max

[Weijie Gao]

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>

[PATCH 3/3] mips: mt76x8: ddr_cal: Correct dqs_find_min/max implementations

[Stefan Roese]

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)
-- 
2.25.1

[PATCH 3/3] mips: mt76x8: ddr_cal: Correct dqs_find_min/max implementations

[Weijie Gao]

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>

[PATCH 1/3] mips: mt76x8: ddr_cal: Rename dqs_test_valid() to dqs_test_error()

[Weijie Gao]

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>

[PATCH 1/3] mips: mt76x8: ddr_cal: Rename dqs_test_valid() to dqs_test_error()

[Stefan Roese]

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 at denx.de/
http://patchwork.ozlabs.org/project/uboot/patch/20200410110431.12256-2-sr at denx.de/
http://patchwork.ozlabs.org/project/uboot/patch/20200410110431.12256-2-sr at denx.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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH 1/3] mips: mt76x8: ddr_cal: Rename dqs_test_valid() to dqs_test_error()

[Daniel Schwierzeck]

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

-- 
- Daniel