[U-Boot] [PATCH 03/12] riscv: bootm: Correct the 1st kernel argument to hart id

[Auer, Lukas]

Hi Bin,

On Thu, 2018-09-06 at 10:57 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Tue, Sep 4, 2018 at 5:41 AM Auer, Lukas
> <lukas.auer at aisec.fraunhofer.de> wrote:
> > 
> > On Thu, 2018-08-30 at 00:54 -0700, Bin Meng wrote:
> > > The first argument of Linux kernel is the risc-v core hart id,
> > > from which the kernel is booted from. It is not the mach_id,
> > > which seems to be copied from arm.
> > > 
> > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > ---
> > > 
> > >  arch/riscv/lib/bootm.c | 18 +++++-------------
> > >  1 file changed, 5 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c
> > > index 6662aff..754bbff 100644
> > > --- a/arch/riscv/lib/bootm.c
> > > +++ b/arch/riscv/lib/bootm.c
> > > @@ -25,10 +25,7 @@ int arch_fixup_fdt(void *blob)
> > > 
> > >  int do_bootm_linux(int flag, int argc, char *argv[],
> > > bootm_headers_t
> > > *images)
> > >  {
> > > -     bd_t    *bd = gd->bd;
> > > -     char    *s;
> > > -     int     machid = bd->bi_arch_number;
> > > -     void    (*theKernel)(int arch, uint params);
> > > +     void    (*kernel)(int hart, uint dtb);
> > 
> > This probably does not cause any issues in u-boot, but the second
> > parameter should be 32 bit or 64 bit depending on the architecture.
> > Since you are already changing the kernel arguments it would make
> > sense
> > to also change dtb from uint to ulong or void *.
> > 
> 
> Yes. Will address this in v2.
> 
> > > 
> > >       /*
> > >        * allow the PREP bootm subcommand, it is required for
> > > bootm to
> > > work
> > > @@ -39,18 +36,12 @@ int do_bootm_linux(int flag, int argc, char
> > > *argv[], bootm_headers_t *images)
> > >       if ((flag != 0) && (flag != BOOTM_STATE_OS_GO))
> > >               return 1;
> > > 
> > > -     theKernel = (void (*)(int, uint))images->ep;
> > > -
> > > -     s = env_get("machid");
> > > -     if (s) {
> > > -             machid = simple_strtoul(s, NULL, 16);
> > > -             printf("Using machid 0x%x from environment\n",
> > > machid);
> > > -     }
> > > +     kernel = (void (*)(int, uint))images->ep;
> > > 
> > >       bootstage_mark(BOOTSTAGE_ID_RUN_OS);
> > > 
> > >       debug("## Transferring control to Linux (at address %08lx)
> > > ...\n",
> > > -            (ulong)theKernel);
> > > +            (ulong)kernel);
> > > 
> > >       if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
> > >  #ifdef CONFIG_OF_LIBFDT
> > > @@ -66,8 +57,9 @@ int do_bootm_linux(int flag, int argc, char
> > > *argv[], bootm_headers_t *images)
> > >       printf("\nStarting kernel ...\n\n");
> > > 
> > >       cleanup_before_linux();
> > > +     /* TODO: hardcode the hart id to zero for now */
> > >       if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len)
> > > -             theKernel(machid, (unsigned long)images->ft_addr);
> > > +             kernel(0, (unsigned long)images->ft_addr);
> > > 
> > 
> > You can use the mhartid CSR to get the hart id. This will limit u-
> > boot
> > to running in machine mode however. Alternatively you can also use
> > the
> > hart id, which is passed in a0 by the bootloader.
> > 
> 
> If the goal is to use U-Boot to directly boot Linux, I think we need
> more changes. I believe we should allow only one hart to execute this
> function. For other harts, they should be waked up and jump to kernel
> directly.
> 

Yes, that's true. We should be able to pick which hart executes it
though. This is useful for chips like SiFive's U54, where hart 0 is a
smaller hart for monitoring tasks. Here it would make sense to run   
u-boot on one of the other harts.

Thanks,
Lukas