Hi Alan, On Thu, Oct 31, 2019 at 3:49 PM Alan Kao <alan...@andestech.com> wrote: > > > 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 <alan...@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 <rickche...@gmail.com> wrote: > > > > > > > > > > Hi Bin > > > > > > > > > > > > > > > > > Hi Rick, > > > > > > > > > > > > On Fri, Oct 25, 2019 at 2:18 PM Andes <ub...@andestech.com> wrote: > > > > > > > > > > > > > > From: Rick Chen <r...@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 <r...@andestech.com> > > > > > > > Cc: KC Lin <kc...@andestech.com> > > > > > > > Cc: Alan Kao <alan...@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?
The bug you mentioned. > If you want to be helpful, please also be specific or anyone else reviewing > this patch will be confused. It's just the explanation that Rick gave confuses people like me. > > > ... 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
