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

[Damien Le Moal]

On 2020/08/14 15:08, Anup Patel wrote:
>> The following works for me. However, the sbi_ecall_console_puts() call in
>> test_main() for the test FW_PAYLOAD does not work. Individual calls to
>> sbi_ecall_console_putc() do work, but not the full string. It looks like the
>> string is broken, so the data section may be broken... Digging into that.
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c index a7fb848..9692cd9 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -84,69 +84,18 @@ static void sbi_boot_prints(struct sbi_scratch
>> *scratch, u32 hartid)
>>         sbi_hart_pmp_dump(scratch);
>>  }
>>
>> -static spinlock_t coldboot_lock = SPIN_LOCK_INITIALIZER; -static unsigned
>> long coldboot_done = 0; -static struct sbi_hartmask coldboot_wait_hmask = {
>> 0 };
>> +static atomic_t coldboot_done = ATOMIC_INITIALIZER(0);
>>
>> -static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid)
>> +static inline void wait_for_coldboot(struct sbi_scratch *scratch, u32
>> hartid)
>>  {
>> -       unsigned long saved_mie, cmip;
>> -       const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>> -
>> -       /* Save MIE CSR */
>> -       saved_mie = csr_read(CSR_MIE);
>> -
>> -       /* Set MSIE bit to receive IPI */
>> -       csr_set(CSR_MIE, MIP_MSIP);
>> -
>> -       /* Acquire coldboot lock */
>> -       spin_lock(&coldboot_lock);
>> -
>> -       /* Mark current HART as waiting */
>> -       sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask);
>> -
>> -       /* Wait for coldboot to finish using WFI */
>> -       while (!coldboot_done) {
>> -               spin_unlock(&coldboot_lock);
>> -               do {
>> -                       wfi();
>> -                       cmip = csr_read(CSR_MIP);
>> -                } while (!(cmip & MIP_MSIP));
>> -               spin_lock(&coldboot_lock);
>> -       };
>> -
>> -       /* Unmark current HART as waiting */
>> -       sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask);
>> -
>> -       /* Release coldboot lock */
>> -       spin_unlock(&coldboot_lock);
>> -
>> -       /* Restore MIE CSR */
>> -       csr_write(CSR_MIE, saved_mie);
>> -
>> -       /* Clear current HART IPI */
>> -       sbi_platform_ipi_clear(plat, hartid);
>> +       /* Wait for coldboot to finish */
>> +       while (!atomic_read(&coldboot_done)) {};
> 
> This is over-simplified and will slow down the booting in cold boot path.
> 
> Imagine, boot CPU in cold boot path and all secondary CPUs bombarding
> the interconnect with atomic operations.

That was just a quick test. With this, the kendryte goes to S mode and start
running the fw payload, which does not execute correctly still. So there is
another problem somewhere with kendryte platform. Not sure what.

> 
> It has to be a combination of atomic coldboot_done and WFI to
> avoid unnecessary traffic in interconnect. 
> 
> We just need to make coldboot_done atomic and don't use coldboot_lock
> for coldboot_done. Rest of the things should stay as-is.

Without the spinlock, there will be problems with coldboot_wait_hmask if it is
used as is. But I do not think that one is necessary. The boot hart can just
send an IPI to all waiting harts. The waiting harts do not need to clear that mask.


-- 
Damien Le Moal
Western Digital Research