Hi Alan,
On Thu, Oct 31, 2019 at 3:49 PM Alan Kao alankao@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 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
- get hart ID.
- 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?
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