On Sat, Jan 14, 2023 at 2:53 PM Pali Rohár <p...@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 <mibo...@gmail.com> wrote:
> > >
> > > Hi Pali,
> > >
> > > On Sat, Jan 14, 2023 at 1:44 PM Pali Rohár <p...@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 <p...@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 <p...@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
