On Sat, Jan 14, 2023 at 2:53 PM Pali Rohár pali@kernel.org wrote:
On Saturday 14 January 2023 14:48:28 Tony Dinh wrote:
Hi Pali,
On Sat, Jan 14, 2023 at 2:12 PM Tony Dinh mibodhi@gmail.com wrote:
Hi Pali,
On Sat, Jan 14, 2023 at 1:44 PM Pali Rohár pali@kernel.org wrote:
On Saturday 14 January 2023 13:35:08 Tony Dinh wrote:
Hi Pali,
On Fri, Jan 13, 2023 at 5:32 PM Pali Rohár pali@kernel.org wrote:
On Wednesday 11 January 2023 21:56:41 Pali Rohár wrote: > On Wednesday 11 January 2023 12:44:45 Tony Dinh wrote: > > Hi Pali, > > > > On Wed, Jan 11, 2023 at 12:04 AM Pali Rohár pali@kernel.org wrote: > > > > > > On Tuesday 10 January 2023 21:07:45 Tony Dinh wrote: > > > > Hi Pali, > > > > > > > > I got burned for being lazy :) it turned out that the mix of #ifdef > > > > and #if defined was the problem. Just stepped away and came back for a > > > > few minutes and I can see that I just need to define the CONFIG_DDR4 > > > > properly in arch/arm/mach-mvebu/Kconfig and build it to pass the > > > > CONFIG_DDR4 stuff. > > > > > > > > One more small hurdle after that was CONFIG_USE_PRIVATE_LIBGCC must be > > > > undefined for it to build (so I am using > > > > /usr/lib/gcc/arm-linux-gnueabi/10/libgcc.a, and not using > > > > arch/arm/lib/lib.a) > > > > > > Hello! It is normally a good idea to unset CONFIG_USE_PRIVATE_LIBGCC > > > unless you have compiled gcc for target CPU. > > > > CONFIG_USE_PRIVATE_LIBGCC was set as a default for ARM target, since > > u-boot has arch/arm/lib/lib.a. But using arch/arm/lib/lib.a I got > > several build errors like these: > > > > ld.bfd: drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.o: in > > function `mv_ddr4_copt_get': > > /usr/src/u-boot/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:809: > > undefined reference to `__aeabi_i2d' > > ld.bfd: /usr/src/u-boot/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:809: > > undefined reference to `__aeabi_dmul' > > ld.bfd: /usr/src/u-boot/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:809: > > undefined reference to `__aeabi_d2uiz' > > This looks like a bug in that training code. I would need to see source > code, so I can figure out how to fix it.
Have you solved this issue? Or if not, can you show what you had on that problematic line 809?
No, I have not and actually have no idea what that code does! And I thought you recognized the error, so I submitted the DDR4 patch so you can compile and see the error. Here is the code snipet, the error is now at 717.
# grep -n10 PBS_VALUE_FACTOR /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c 704- u8 copt_val; 705- u8 dq_idx; 706- u8 center_zone_max_low = 0; 707- u8 center_zone_min_high = 128; 708- u8 vw_zone_max_low = 0; 709- u8 vw_zone_min_high = 128; 710- u8 min_vw = 63; /* minimum valid window between all bits */ 711- u8 center_low_el; 712- u8 center_high_el; 713- 714: /* lambda calculated as D * PBS_VALUE_FACTOR / d */ 715- //printf("Copt::Debug::\t"); 716- for (dq_idx = 0; dq_idx < 8; dq_idx++) { 717- center_per_dq[dq_idx] = 0.5 * (vw_h[dq_idx] + vw_l[dq_idx]); 718- vw_per_dq[dq_idx] = 1 + (vw_h[dq_idx] - vw_l[dq_idx]); 719- if (min_vw > vw_per_dq[dq_idx]) 720- min_vw = vw_per_dq[dq_idx]; 721- } 722- 723- /* calculate center zone */ 724- for (dq_idx = 0; dq_idx < 8; dq_idx++) {
Haha! That is pretty simple. On line 717 you have fractional number 0.5 which is represented by floating point and all numeric operation on that line are done as floating point.
U-Boot and kernel does not floating point HW and instruct compiler to replace all floating point operations by function calls (which implement software floating point support).
U-Boot (for very good reasons) do not implement any floating point operations.
Fix should be very simple, use only integer operations. So replace
0.5 * something
by:
something / 2
And check if it compiles and if it works.
I'm surprised that Marvell was trying to do floating point...
Yes!!! that was impressive how quick you saw the root cause. I've replaced those floating point operations with integer operations and it built fine. Let see it will run.
Indeed! It runs without problem with the changes below.
diff --git a/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c b/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c index e4e86c6f2e..7e596ce78a 100644 --- a/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c +++ b/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c @@ -714,7 +714,7 @@ static int mv_ddr4_copt_get(u8 dir, u16 *lambda, u8 *vw_l, u8 *vw_h, u8 *pbs_res /* lambda calculated as D * PBS_VALUE_FACTOR / d */ //printf("Copt::Debug::\t"); for (dq_idx = 0; dq_idx < 8; dq_idx++) {
center_per_dq[dq_idx] = 0.5 * (vw_h[dq_idx] + vw_l[dq_idx]);
center_per_dq[dq_idx] = (vw_h[dq_idx] + vw_l[dq_idx]) / 2; vw_per_dq[dq_idx] = 1 + (vw_h[dq_idx] - vw_l[dq_idx]); if (min_vw > vw_per_dq[dq_idx]) min_vw = vw_per_dq[dq_idx];@@ -754,9 +754,9 @@ static int mv_ddr4_copt_get(u8 dir, u16 *lambda, u8 *vw_l, u8 *vw_h, u8 *pbs_res return MV_OK; } else { /* not center zone visib */ for (dq_idx = 0; dq_idx < 8; dq_idx++) {
if ((center_zone_low[dq_idx] + 1) > (0.5 *vw_per_dq[dq_idx] + vw_per_dq[dq_idx] % 2)) {
if ((center_zone_low[dq_idx] + 1) >(vw_per_dq[dq_idx] / 2 + vw_per_dq[dq_idx] % 2)) { vw_zone_low[dq_idx] = (center_zone_low[dq_idx] + 1) -
(0.5 *vw_per_dq[dq_idx] + vw_per_dq[dq_idx] % 2);
(vw_per_dq[dq_idx] / 2 + vw_per_dq[dq_idx] % 2); } else { vw_zone_low[dq_idx] = 0; DEBUG_CALIBRATION(DEBUG_LEVEL_INFO, @@ -767,7 +767,7 @@ static int mv_ddr4_copt_get(u8 dir, u16 *lambda, u8 *vw_l, u8 *vw_h, u8 *pbs_res vw_l[dq_idx], vw_h[dq_idx], lambda[dq_idx])); }
vw_zone_high[dq_idx] =center_zone_high[dq_idx] + 0.5 * vw_per_dq[dq_idx];
vw_zone_high[dq_idx] =center_zone_high[dq_idx] + vw_per_dq[dq_idx] / 2;
if (vw_zone_max_low < vw_zone_low[dq_idx]) vw_zone_max_low = vw_zone_low[dq_idx];@@ -1121,7 +1121,7 @@ static int mv_ddr4_tap_tuning(u8 dev, u16 (*pbs_tap_factor)[MAX_BUS_NUM][BUS_WID int dq_to_dqs_min_delta = dq_to_dqs_min_delta_threshold * 2; u32 pbs_tap_factor0 = PBS_VAL_FACTOR * NOMINAL_PBS_DLY / adll_tap; /* init lambda */ /* adapt pbs to frequency */
u32 new_pbs = (18100 - (3.45 * freq)) / 1000;
u32 new_pbs = (18100 - (freq * 345 / 100)) / 1000; int stage_num, loop; int wl_tap, new_wl_tap; int pbs_tap_factor_avg;Note that I don't like the last change very much (potential for overflow). But freq must be a lot smaller than 18100 (by a factor of 3.45) in original Marvell code. So I think we are OK.
So then maybe something like this to prevent loosing precision?
u32 new_pbs = (1810000 - (345 * freq)) / 100000;
Yes! indeed.
Thanks, Tony
Thanks, Tony
Thanks, Tony
Here are the build errors with the current code.
<BEGIN ERRORS BUILD LOG > rm -f spl/common/spl/built-in.o; ar cDPrsT spl/common/spl/built-in.o spl/common/spl/spl.o spl/common/spl/spl_bootrom.o spl/common/spl/spl_spi.o ( cd spl && ld.bfd -T u-boot-spl.lds --gc-sections -Bstatic --gc-sections --no-dynamic-linker --build-id=none -Ttext 0x40000030 arch/arm/cpu/armv7/start.o --whole-archive arch/arm/mach-mvebu/built-in.o arch/arm/cpu/armv7/built-in.o arch/arm/cpu/built-in.o arch/arm/lib/built-in.o board/thecus/n2350/built-in.o common/spl/built-in.o common/init/built-in.o boot/built-in.o common/built-in.o cmd/built-in.o env/built-in.o lib/built-in.o disk/built-in.o drivers/built-in.o dts/built-in.o fs/built-in.o --no-whole-archive arch/arm/lib/eabi_compat.o arch/arm/lib/lib.a -Map u-boot-spl.map -o u-boot-spl ) ld.bfd: drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.o: in function `mv_ddr4_copt_get': /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:717: undefined reference to `__aeabi_i2d' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:717: undefined reference to `__aeabi_dmul' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:717: undefined reference to `__aeabi_d2uiz' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:757: undefined reference to `__aeabi_i2d' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:757: undefined reference to `__aeabi_i2d' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:757: undefined reference to `__aeabi_dmul' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:757: undefined reference to `__aeabi_i2d' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:757: undefined reference to `__aeabi_dadd' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:757: undefined reference to `__aeabi_dcmpgt' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:758: undefined reference to `__aeabi_dsub' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:758: undefined reference to `__aeabi_d2uiz' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:770: undefined reference to `__aeabi_i2d' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:770: undefined reference to `__aeabi_dadd' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:770: undefined reference to `__aeabi_d2uiz' ld.bfd: drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.o: in function `mv_ddr4_tap_tuning': /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:1124: undefined reference to `__aeabi_ui2d' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:1124: undefined reference to `__aeabi_dmul' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:1124: undefined reference to `__aeabi_dsub' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:1124: undefined reference to `__aeabi_ddiv' ld.bfd: /usr/src/u-boot-master/drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c:1124: undefined reference to `__aeabi_d2uiz' make[1]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1 <END ERRORS BUILD LOG >
And the config option that's relevant here is CONFIG_USE_PRIVATE_LIBGCC (default is Y) which enabled the usage of arch/arm/lib/lib.a, and caused the build errors.
# grep GCC .config CONFIG_CC_IS_GCC=y CONFIG_GCC_VERSION=100201 CONFIG_HAVE_PRIVATE_LIBGCC=y CONFIG_USE_PRIVATE_LIBGCC=y
When I unset CONFIG_USE_PRIVATE_LIBGCC, the GGC library used is /usr/lib/gcc/arm-linux-gnueabi/10 and everything built.
# grep CONFIG_USE_PRIVATE_LIBGCC .config # CONFIG_USE_PRIVATE_LIBGCC is not set
<BEGIN GOOD BUILD LOG> rm -f spl/common/spl/built-in.o; ar cDPrsT spl/common/spl/built-in.o spl/common/spl/spl.o spl/common/spl/spl_bootrom.o spl/common/spl/spl_spi.o ( cd spl && ld.bfd -T u-boot-spl.lds --gc-sections -Bstatic --gc-sections --no-dynamic-linker --build-id=none -Ttext 0x40000030 arch/arm/cpu/armv7/start.o --whole-archive arch/arm/mach-mvebu/built-in.o arch/arm/cpu/armv7/built-in.o arch/arm/cpu/built-in.o arch/arm/lib/built-in.o board/thecus/n2350/built-in.o common/spl/built-in.o common/init/built-in.o boot/built-in.o common/built-in.o cmd/built-in.o env/built-in.o lib/built-in.o disk/built-in.o drivers/built-in.o dts/built-in.o fs/built-in.o --no-whole-archive arch/arm/lib/eabi_compat.o -L /usr/lib/gcc/arm-linux-gnueabi/10 -lgcc -Map u-boot-spl.map -o u-boot-spl ) ld.bfd: warning: /usr/lib/gcc/arm-linux-gnueabi/10/libgcc.a(_udivmoddi4.o) uses 4-byte wchar_t yet the output is to use 2-byte wchar_t; use of wchar_t values across objects may fail objcopy -j .text -j .secure_text -j .secure_data -j .rodata -j .hash -j .data -j .got -j .got.plt -j __u_boot_list -j .rel.dyn -j .binman_sym_table -j .text_rest -j .dtb.init.rodata -j .efi_runtime -j .efi_runtime_rel -O binary spl/u-boot-spl spl/u-boot-spl-nodtb.bin objdump -t spl/u-boot-spl > spl/u-boot-spl.sym cat spl/u-boot-spl-nodtb.bin spl/u-boot-spl-pad.bin spl/u-boot-spl.dtb > spl/u-boot-spl-dtb.bin cp spl/u-boot-spl-dtb.bin spl/u-boot-spl.bin ./tools/mkimage -n ./arch/arm/mach-mvebu/kwbimage.cfg -T kwbimage -a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot-with-spl.kwb >/dev/null && cat /dev/null ./scripts/check-of.sh .config ./scripts/of_allowlist.txt make: Leaving directory '/usr/src/u-boot-master' + /bin/ls -lh u-boot-with-spl.kwb -rw-r--r-- 1 root root 581K Jan 14 13:12 u-boot-with-spl.kwb <END LOG>
I can also send you the whole log if you want. I'm preparing the Thecus N2350 patch and will send it within a few days.
Thanks, Tony