[U-Boot] [PATCH 4/8] riscv: andes_plic: Fix some wrong configurations

[Alan Kao <alankao@andestech.com>]

On Thu, Oct 31, 2019 at 11:36:48AM +0800, Bin Meng wrote:
> Hi Alan,
> 
> On Thu, Oct 31, 2019 at 9:00 AM Alan Kao <alankao@andestech.com> wrote:
> >
> > Hi Bin,
> >
> > Thanks for the critics.  Comments below.
> > On Wed, Oct 30, 2019 at 06:38:00PM +0800, Bin Meng wrote:
> > > Hi Rick,
> > >
> > > On Wed, Oct 30, 2019 at 10:50 AM Rick Chen <rickchen36@gmail.com> wrote:
> > > >
> > > > Hi Bin
> > > >
> > > > >
> > > > > Hi Rick,
> > > > >
> > > > > On Fri, Oct 25, 2019 at 2:18 PM Andes <uboot@andestech.com> wrote:
> > > > > >
> > > > > > From: Rick Chen <rick@andestech.com>
> > > > > >
> > > > > > It will work fine due to hart 0 always will be main
> > > > > > hart coincidentally. When develop SPL flow, I try to
> > > > > > force other harts to be main hart. And it will go
> > > > > > wrong in sending IPI flow. So fix it.
> > > > >
> > > > > Fix what? Does this commit contain 2 fixes, or just 1 fix?
> > > >
> > > > Yes, it include two fixs. But they will cause one negative result
> > > > that only hart 0 can send ipi to other harts.
> > > >
> > > > >
> > > > > >
> > > > > > Having this fix, any hart can be main hart in U-Boot SPL
> > > > > > theoretically, but it still fail somewhere. After dig in
> > > > > > and found there is an assumption that hart 0 shall be
> > > > > > main hart in OpenSbi.
> > > > >
> > > > > So does this mean there is a bug in OpenSBI too?
> > > >
> > > > I am not sure if it is a bug. Maybe it is a compatible issue.
> > > > There is a limitation that only hart 0 can be main hart in OpenSBI.
> > >
> > > I don't think OpenSBI has such limitation.
> > >
> >
> > Please check the source.
> > https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L54
> >
> > Apparently, the FIRST TWO LINEs of the initialization are the
> > 1. get hart ID.
> > 2. determine which route to take based on their ID respectively.
> >
> 
> This is true only for the very first a few instructions when OpenSBI
> boots. Later OpenSBI main initialization does not require hart to be
> zero.
> 
> > So, I do think OpenSBI has this signature, if you are not willing to call it
> > a limitation.
> >
> > > > But any hart can be main hart in U-Boot.
> > > >
> > > > In general case, hart 0 will be main and it is fine when U-Boot jump ot OpenSBI.
> > > > But if we force hart 1 to be main hart, when hart 0 jump to OPenSBI from U-Boot,
> > > > It will do relocation flow in OpenSBI which willcorrupt U-Boot SPL,
> > > > but hart 0 still in U-Boot SPL.
> > > >
> > > > >
> > > > > >
> > > > > > After some work-arounds, it can pass the verifications
> > > > > > that any hart can be main hart and boots U-Boot SPL ->
> > > > > > OpenSbi -> U-Boot proper -> Linux Kernel successfully.
> > > > > >
> > > > >
> > > > > It's a bit hard for me to understand what was described here in the
> > > > > commit message. Maybe you need rewrite something.
> > > >
> > > > OK. I will rewrite this commit message.
> > > >
> > > > >
> > > > > > Signed-off-by: Rick Chen <rick@andestech.com>
> > > > > > Cc: KC Lin <kclin@andestech.com>
> > > > > > Cc: Alan Kao <alankao@andestech.com>
> > > > > > ---
> > > > > >  arch/riscv/lib/andes_plic.c | 11 +++++++----
> > > > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/riscv/lib/andes_plic.c b/arch/riscv/lib/andes_plic.c
> > > > > > index 28568e4..42394b9 100644
> > > > > > --- a/arch/riscv/lib/andes_plic.c
> > > > > > +++ b/arch/riscv/lib/andes_plic.c
> > > > > > @@ -19,7 +19,7 @@
> > > > > >  #include <cpu.h>
> > > > > >
> > > > > >  /* pending register */
> > > > > > -#define PENDING_REG(base, hart)        ((ulong)(base) + 0x1000 + (hart) * 8)
> > > > > > +#define PENDING_REG(base, hart)        ((ulong)(base) + 0x1000 + ((hart) / 4) * 4)
> > > > > >  /* enable register */
> > > > > >  #define ENABLE_REG(base, hart) ((ulong)(base) + 0x2000 + (hart) * 0x80)
> > > > > >  /* claim register */
> > > > > > @@ -46,7 +46,7 @@ static int init_plic(void);
> > > > > >
> > > > > >  static int enable_ipi(int hart)
> > > > > >  {
> > > > > > -       int en;
> > > > > > +       unsigned int en;
> > > > >
> > > > > Is this for some compiler warning fix?
> > > >
> > > > No, it is not a warning fix. It is a bug fix.
> > > > I hope en can be 0x0000000080808080 instead of 0xffffffff80808080,
> > >
> > > But it is int, which is only 32-bit. The example you gave was a 64-bit number.
> > >
> >
> > Please consider the following simple program:
> >
> > > #define MASK 0x80808080
> > >int main(){
> > >        int en;
> > >        en = MASK;
> > >        printf("%x, shifted %x\n", en, en >> 1);
> > >        return 0;
> > >}
> >
> > Would you mind sharing what you get after running this on your x86_64
> > (if you have one) computer?  Really appreiciate that.
> >
> > The almost identical episode is in the patch, specifically,
> > > en = ENABLE_HART_IPI >> hart
> 
> Yes, this is a bug. ...

Wait, what do you mean but "this"?  What is a bug here?
If you want to be helpful, please also be specific or anyone else reviewing
this patch will be confused.

> ... But I was confused by Rick's comments as he was
> using a 64-bit number as int is never to be a 64-bit for both 32-bit
> and 64-bit.

It was just an example.  Nothing to do with bit width, but just a sign-
extension issue.

> 
> Regards,
> Bin

Thanks,
Alan