[PATCH v4] riscv: Make SiFive HiFive Unleashed board boot again

[Rick Chen]

Hi Bin

> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> Sent: Friday, July 24, 2020 11:26 AM
> To: Leo Yu-Chi Liang(梁育齊); Rick Jian-Zhi Chen(陳建志)
> Cc: U-Boot Mailing List
> Subject: Re: [PATCH v4] riscv: Make SiFive HiFive Unleashed board boot again
>
> Hi Rick,
>
> On Tue, Jul 21, 2020 at 4:30 PM Leo Liang <ycliang at andestech.com> wrote:
> >
> > On Sun, Jul 19, 2020 at 11:17:07PM -0700, Bin Meng wrote:
> > > From: Bin Meng <bin.meng at windriver.com>
> > >
> > > Commit 40686c394e53 ("riscv: Clean up IPI initialization code")
> > > caused U-Boot failed to boot on SiFive HiFive Unleashed board.
> > >
> > > The codes inside arch_cpu_init_dm() may call U-Boot timer APIs
> > > before the call to riscv_init_ipi(). At that time the timer register
> > > base (e.g.: the SiFive CLINT device in this case) is unknown yet.
> > >
> > > It might be the name riscv_init_ipi() that misleads people to only
> > > consider it is related to IPI, but in fact the timer capability is
> > > provided by the same SiFive CLINT device that provides the IPI.
> > > Timer capability is needed for both UP and SMP.
> > >
> > > Considering that the original refactor does have benefits, that it
> > > makes the IPI code more similar to U-Boot initialization idioms.
> > > It also removes some quite ugly macros. Let's do the minimal revert
> > > instead of a complete revert, plus a fixes to arch_cpu_init_dm() to
> > > consider the SPL case.
> > >
> > > Fixes: 40686c394e53 ("riscv: Clean up IPI initialization code")
> > > Signed-off-by: Bin Meng <bin.meng at windriver.com>
> > > Reviewed-by: Sean Anderson <seanga2 at gmail.com>
> > > ---
> > >
> > > Changes in v4:
> > > - Include Sean's "Reviewed-by" tag
> > >
> > > Changes in v3:
> > > - Simply call riscv_init_ipi() in clint timer functions to avoid
> > >   some duplications
> > >
> > > Changes in v2:
> > > - Do the minimal partial revert instead of a complete revert, enough
> > >   to make HiFive Unleashed board boot again.
> > >
> > >  arch/riscv/cpu/cpu.c          |  2 +-
> > >  arch/riscv/lib/sifive_clint.c | 16 ++++++++++++----
> > >  common/spl/spl_opensbi.c      |  5 -----
> > >  3 files changed, 13 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c index
> > > bbd6c15..bfa2d4a 100644
> > > --- a/arch/riscv/cpu/cpu.c
> > > +++ b/arch/riscv/cpu/cpu.c
> > > @@ -107,7 +107,7 @@ int arch_cpu_init_dm(void)  #endif
> > >       }
> > >
> > > -#ifdef CONFIG_SMP
> > > +#if CONFIG_IS_ENABLED(SMP)
> > >       ret = riscv_init_ipi();
> > >       if (ret)
> > >               return ret;
> > > diff --git a/arch/riscv/lib/sifive_clint.c
> > > b/arch/riscv/lib/sifive_clint.c index 78fc6c8..b9a2c64 100644
> > > --- a/arch/riscv/lib/sifive_clint.c
> > > +++ b/arch/riscv/lib/sifive_clint.c
> > > @@ -26,6 +26,9 @@ DECLARE_GLOBAL_DATA_PTR;
> > >
> > >  int riscv_get_time(u64 *time)
> > >  {
> > > +     /* ensure timer register base has a sane value */
> > > +     riscv_init_ipi();
> > > +
> > >       *time = readq((void __iomem *)MTIME_REG(gd->arch.clint));
> > >
> > >       return 0;
> > > @@ -33,6 +36,9 @@ int riscv_get_time(u64 *time)
> > >
> > >  int riscv_set_timecmp(int hart, u64 cmp)  {
> > > +     /* ensure timer register base has a sane value */
> > > +     riscv_init_ipi();
> > > +
> > >       writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint,
> > > hart));
> > >
> > >       return 0;
> > > @@ -40,11 +46,13 @@ int riscv_set_timecmp(int hart, u64 cmp)
> > >
> > >  int riscv_init_ipi(void)
> > >  {
> > > -     long *ret = syscon_get_first_range(RISCV_SYSCON_CLINT);
> > > +     if (!gd->arch.clint) {
> > > +             long *ret =
> > > + syscon_get_first_range(RISCV_SYSCON_CLINT);
> > >
> > > -     if (IS_ERR(ret))
> > > -             return PTR_ERR(ret);
> > > -     gd->arch.clint = ret;
> > > +             if (IS_ERR(ret))
> > > +                     return PTR_ERR(ret);
> > > +             gd->arch.clint = ret;
> > > +     }
> > >
> > >       return 0;
> > >  }
> > > diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> > > index 3440bc0..14f335f 100644
> > > --- a/common/spl/spl_opensbi.c
> > > +++ b/common/spl/spl_opensbi.c
> > > @@ -79,11 +79,6 @@ void spl_invoke_opensbi(struct spl_image_info *spl_image)
> > >       invalidate_icache_all();
> > >
> > >  #ifdef CONFIG_SPL_SMP
> > > -     /* Initialize the IPI before we use it */
> > > -     ret = riscv_init_ipi();
> > > -     if (ret)
> > > -             hang();
> > > -
> > >       /*
> > >        * Start OpenSBI on all secondary harts and wait for acknowledgment.
> > >        *
> > > --
> > > 2.7.4
> > >
> >
> > Tested-by: Leo Liang <ycliang at andestech.com>
>
> Could we get this applied soon, since it's broken in u-boot/master?

OK. I am preparing to send a PR later today.

Thanks,
Rick

>
> Regards,
> Bin