Re: [PATCH] riscv: fix compitible with binutils 2.38

[Leo Liang <ycliang@andestech.com>]

On Fri, Aug 19, 2022 at 09:24:53AM -0600, Simon Glass wrote:
> Hi Leo,
> 
> On Fri, 19 Aug 2022 at 03:09, Leo Liang <ycliang@andestech.com> wrote:
> >
> > On Thu, Aug 11, 2022 at 10:23:02PM +0000, Leo Liang wrote:
> > > On Mon, May 30, 2022 at 11:05:54AM -0400, Tom Rini wrote:
> > > > On Sat, May 28, 2022 at 09:02:09AM +0000, Leo Liang wrote:
> > > > > On Fri, May 27, 2022 at 09:30:49AM -0400, Tom Rini wrote:
> > > > > > On Fri, May 27, 2022 at 02:36:29AM +0000, Leo Liang wrote:
> > > > > >
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > The following changes since commit 7e0edcadb09d55d5319fdc862041fd1b874476f5:
> > > > > > >
> > > > > > >   Merge branch 'master' of https://source.denx.de/u-boot/custodians/u-boot-sunxi (2022-05-24 23:29:00 -0400)
> > > > > > >
> > > > > > > are available in the Git repository at:
> > > > > > >
> > > > > > >   https://source.denx.de/u-boot/custodians/u-boot-riscv.git
> > > > > > >
> > > > > > > for you to fetch changes up to c544b281cd3e549a4fcbf4ba9a05a5d72c9557dd:
> > > > > > >
> > > > > > >   riscv: qemu: Set kernel_comp_addr_r for compressed kernel (2022-05-26 18:42:34 +0800)
> > > > > > >
> > > > > > > CI result shows no issue: https://source.denx.de/u-boot/custodians/u-boot-riscv/-/pipelines/12131
> > > > > >
> > > > > > First, I've applied this to u-boot/master now.  Second, will
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/PH7PR14MB5594FD11D1BE74284F554BEBCED49@PH7PR14MB5594.namprd14.prod.outlook.com/
> > > > > > be coming soon?  Thanks!
> > > > >
> > > > > Hi Tom,
> > > > >
> > > > > This patch you mentioned will not pass CI, and the reason for that
> > > > > is the toolchain used for RISC-V in CI does not have corresponding
> > > > > settings for zifencei and zicsr.
> > > > > (detailed disscussion: https://patchwork.ozlabs.org/project/uboot/patch/20220128134713.2322800-1-alexandre.ghiti@canonical.com/)
> > > > > (CI result: https://source.denx.de/u-boot/custodians/u-boot-riscv/-/jobs/440735)
> > > > >
> > > > > The patch looks valid, but will fail CI on 32-bit configs.
> > > > > If we use 32-bit toolchain to test 32-bit configs, then
> > > > > problems solved.
> > > > >
> > > > > Do you have any comments?
> > > >
> > > > I guess I'm OK with saying we should use a 32bit toolchain for 32bit
> > > > riscv, if  that's how things should be handled moving forward for
> > > > everyone else.
> > > >
> > > > --
> > > > Tom
> > >
> > > Hi Tom,
> > >
> > > Sorry for taking such a long time to reply.
> > >
> > > Recap:
> > > All the "riscv: fix compitible with binutils 2.38" patches that
> > > try to support new RISC-V ISA extension will cause U-Boot CI to fail
> > > because the toolchain used in U-Boot CI do not support the new multilib settings.
> > > (original discussion:
> > > https://patchwork.ozlabs.org/project/uboot/patch/20220128134713.2322800-1-alexandre.ghiti@canonical.com/)
> > >
> > > We found that current RISC-V toolchains from kernel.org do not
> > > support zifencei and zicsr extensions' multilib settings,
> > > regardless of the toolchain version.
> > > (Both gcc 11.1.0, 12.1.0 do not support the needed settings.
> > > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
> > > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/12.1.0/)
> > >
> > > But we also found that if we use recent upstream riscv-gnu-toolchain,
> > > we could build an gcc-12.1.0 toolchain that does support multilib
> > > settings and could fix this issue.
> > >
> > > We have provided a Dockerfile as a reference build script[1] and
> > > a prebuilt toolchain[2] for U-Boot CI to use.
> > >
> > > We have also verified the CI process could execute successfully
> > > with your base image and the provided riscv64-linux toolchain[3].
> > >
> > > I guess the coming update of the toolchain in kernel.org should
> > > contain the new multilib settings, so I was wondering if we could
> > > replace the riscv64-linux toolchain from kernel.org with this prebuilt
> > > toolchain we've provided on github[2] temporarily?
> > >
> > > After studying a bit of the buildman tool, the earlier idea that
> > > "use different toolchains for different board configs" would require
> > > an amount of modification, thus we think its best to replace the toolchain
> > > temporarily to fix this issue, then the patch could be applied without CI failure.
> > >
> > > [1] https://github.com/ycliang-andes/riscv-toolchain/blob/master/Dockerfile
> > > [2] https://github.com/ycliang-andes/riscv-toolchain/releases/download/v1.0/x86_64-gcc-12.1.0-nolibc-riscv64-linux.tar.xz
> > > [3] https://source.denx.de/u-boot/custodians/u-boot-riscv/-/pipelines/13129
> >
> > Hi Tom,
> >
> > A gentle ping.
> > Do you have any concern on changing the toolchain source for now?
> 
> It is easy enough for people to download the toolchain and update the
> .buildman file to point to it. It would also be possible to patch
> 'buildman fetch-arch riscv64' to add a toolchain-prefix to the
> .buildman file, as a temporary measure. This would need a later
> buildman patch to remove the line.
> 

Hi Simon,

Thank you for pointing this out!

> I'm not sure if this sort of thing is a good idea though, when we can
> just tell people to download the toolchain and update the .buildman
> file.
> 

But agreed that this might not be a good idea.

Furthermore, I think this patch (https://patchwork.ozlabs.org/project/uboot/patch/BYAPR04MB48242371E5FBE6AC36069334A4769@BYAPR04MB4824.namprd04.prod.outlook.com/)
should be able to support new RISC-V extension without breaking the CI process as long as we do not update the toolchain CI uses.

I will review this patch as soon as possible.

Thanks for your suggestion!

Best regards,
Leo

> Regards,
> Simon