[PATCH 1/1] lib: sbi: relax scoldboot_lock spinlock

[Sean Anderson]

On 8/13/20 8:32 PM, Atish Patra wrote:
> On Thu, Aug 13, 2020 at 3:52 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> On 14.08.20 00:25, Atish Patra wrote:
>>>>> Probably, the hart executing warmboot invokes sbi_platform_ipi_clear
>>>>> even before sbi_ipi_init is being called from the hart running coldboot.
>>>> There are two calls to sbi_ipi_init(). Is only the one in coldboot relevant?
>>>>
>>> Yes. That actually setups the clint base address.
>>>
>>>> Do you mean something like:
>>>>
>>> No. I meant like this.
>>>
>>> +#define UART_BASE 0x38000000ULL
>>> +static void sbi_print_early(char ch) {
>>> +    int32_t r;
>>> +    do {
>>> +      __asm__ __volatile__ (
>>> +        "amoor.w %0, %2, %1\n"
>>> +        : "=r" (r), "+A" (*(uint32_t*)(UART_BASE))
>>> +        : "r" (ch));
>>> +    } while (r < 0);
>>> +}
>>> +
>>>  static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid)
>>>  {
>>>         int xlen;
>>> @@ -112,6 +123,8 @@ static void wait_for_coldboot(struct sbi_scratch
>>> *scratch, u32 hartid)
>>>                         wfi();
>>>                         cmip = csr_read(CSR_MIP);
>>>                  } while (!(cmip & MIP_MSIP));
>>> +               sbi_print_early('T');
>>> +               sbi_printf("mip value %lx\n", cmip);
>>>                 spin_lock(&coldboot_lock);
>>>         };
>>>
>>>
>>> Note: sbi_printf may not work if sbi_console_init from the cold boot
>>> path is not invoked.
>>> That's why I added a "sbi_print_early".  If sbi_printf doesn't work,
>>> please remove it.
>>>
>>> Thanks for your time in figuring out the issue with Kendryte.
>>> Unfortunately, I can't test these changes as I don't have
>>> a kendryte board at home.
>>>
>>>
>>
>> This now booted to U-Boot. The extra time for printing was enough to
>> break the livelock:
>>
> Thanks again for verification.
> 
>>
>> TTTTTTTTTT
> 
> This confirms that MSIP bit is set even if there was no IPI sent and MIP was
> cleared in fw_base.S [1]. I guess there is a hardware bug with Kendryte where
> MSIP is not getting reset properly.

Hm, perhaps MIP is level-based, so the CLINT itself needs to be
acknowledged before MIP will be disabled.

> 
> [1] https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L370
> 
> If that's the case, we can convert coldboot_done to an atomic variable
> as suggested by

I think this is the best course of action in general. All the harts are
just waiting around, so there's no need for an interrupt here.

--Sean