Re: [PATCH v1 3/3] board: qemu-riscv: Override enable_caches

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 21.08.24 11:11, Mayuresh Chitale wrote:
> Hi Heinrich,
> 
> On Tue, Aug 20, 2024 at 5:43 PM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 20.08.24 11:37, Mayuresh Chitale wrote:
>>> Define enable_caches function for the qemu-riscv board which probes for
>>> the cbom-block-size dt property when RISCV_ISA_ZICBOM is enabled. Also
>>> add flush_dcache_range and invalidate_dcache_range functions which use
>>> the corresponding CBO ops.
>>>
>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>> ---
>>>    board/emulation/qemu-riscv/qemu-riscv.c | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c
>>> index e5193e31e3..1795d2f831 100644
>>> --- a/board/emulation/qemu-riscv/qemu-riscv.c
>>> +++ b/board/emulation/qemu-riscv/qemu-riscv.c
>>> @@ -14,6 +14,7 @@
>>>    #include <usb.h>
>>>    #include <virtio_types.h>
>>>    #include <virtio.h>
>>> +#include <asm/cache.h>
>>>
>>>    DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -70,3 +71,18 @@ void *board_fdt_blob_setup(int *err)
>>>        /* Stored the DTB address there during our init */
>>>        return (void *)(ulong)gd->arch.firmware_fdt_addr;
>>>    }
>>> +
>>> +void enable_caches(void)
>>> +{
>>> +     riscv_zicbom_init();
>>> +}
>>
>> Enable caches may be called multiple times. But riscv_zicbom_init() only
>> needs to be called once.
> Ok.
>>
>>> +
>>> +void flush_dcache_range(unsigned long start, unsigned long end)
>>> +{
>>> +     cbo_flush(start, end);
>>> +}
>>> +
>>> +void invalidate_dcache_range(unsigned long start, unsigned long end)
>>> +{
>>> +     cbo_inval(start, end);
>>> +}
>>
>> The ZICBOM extension is not specific to a single board. We should not
>> add this code to board files but into the central place for cache
>> operations, i.e. arch/riscv/lib/cache.c.
> Most of the code is already cache.c. I didn't know if it would suffice
> for all platforms so I decided to override the weak functions like in
> ARM.


With the suggestion below boards needing a board specific solution can 
still override the weak functions. But I would expect that in future new 
boards converge on zicbom.

Best regards

Heinrich

> 
>> E.g.
>>
>> static int get_zicbom_block_size()
>> {
>>          if (!CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM))
>>                  return 0;
>>          if (zicbom_block_size)
>>                  return zicbom_block_size;
>>
>>          uclass_first_device(UCLASS_CPU, &dev);
>>          if (!dev) {
>>                  log_err("Failed to get CPU device!\n");
>>                  return 0;
>>          }
>>
>>          if (dev_read_u32(dev, "riscv,cbom-block-size", &zicbom_block_size))
>>                  return 0;
>>
>>          return zicbom_block_size
>> }
>>
>> __weak void invalidate_icache_range(unsigned long start, unsigned long end)
>> {
>>          if (CONFIG_IS_ENABLED(RISCV_ISA_ZICBOM) && get_zicbom_block_size())
>>                  cbo_inval(start, end);
>>          else
>>                  invalidate_icache_all();
>> }
>>
>> Cc: Rick as one of the RISC-V maintainers.
> Ok.
>>
>> Best regards
>>
>> Heinrich
>>