Re: [PATCH v4 2/3] openrisc: Introduce new utility functions to flush and invalidate caches

[Sahil Siddiq]

Hi,

On 3/29/25 3:18 AM, Stafford Horne wrote:

On Sat, Mar 29, 2025 at 01:56:31AM +0530, Sahil Siddiq wrote:

[...]
diff --git a/arch/openrisc/include/asm/cpuinfo.h 
b/arch/openrisc/include/asm/cpuinfo.h
index 82f5d4c06314..e46afbfe9b5a 100644
--- a/arch/openrisc/include/asm/cpuinfo.h
+++ b/arch/openrisc/include/asm/cpuinfo.h
@@ -15,6 +15,9 @@
  #ifndef __ASM_OPENRISC_CPUINFO_H
  #define __ASM_OPENRISC_CPUINFO_H

+#include <asm/spr.h>

+#include <asm/spr_defs.h>
+
  struct cache_desc {
        u32 size;
        u32 sets;
@@ -34,4 +37,16 @@ struct cpuinfo_or1k {
  extern struct cpuinfo_or1k cpuinfo_or1k[NR_CPUS];
  extern void setup_cpuinfo(void);

+/*

+ * Check if the cache component exists.
+ */
+extern bool cpu_cache_is_present(const unsigned int cache_type);

This is used in cacheinfo.  OK.

+/*
+ * Check if the cache block flush/invalidate register is implemented for the
+ * cache component.
+ */
+extern bool cb_inv_flush_is_implemented(const unsigned int reg,
+                                       const unsigned int cache_type);

But this function doesnt seem to be used anywhere but in cache.c. Does it need
to be public?

Right, this doesn't have to be public. However, based on the description below, 
it's
better to remove this function.

  #endif /* __ASM_OPENRISC_CPUINFO_H */
diff --git a/arch/openrisc/kernel/dma.c b/arch/openrisc/kernel/dma.c
index b3edbb33b621..3a7b5baaa450 100644
--- a/arch/openrisc/kernel/dma.c
+++ b/arch/openrisc/kernel/dma.c
@@ -17,6 +17,7 @@
  #include <linux/pagewalk.h>

#include <asm/cpuinfo.h>

+#include <asm/cacheflush.h>
  #include <asm/spr_defs.h>
  #include <asm/tlbflush.h>

@@ -24,9 +25,6 @@ static int

  page_set_nocache(pte_t *pte, unsigned long addr,
                 unsigned long next, struct mm_walk *walk)
  {
-       unsigned long cl;
-       struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
-
        pte_val(*pte) |= _PAGE_CI;

/*

@@ -36,8 +34,7 @@ page_set_nocache(pte_t *pte, unsigned long addr,
        flush_tlb_kernel_range(addr, addr + PAGE_SIZE);

/* Flush page out of dcache */

-       for (cl = __pa(addr); cl < __pa(next); cl += cpuinfo->dcache_block_size)
-               mtspr(SPR_DCBFR, cl);
+       local_dcache_range_flush(__pa(addr), __pa(next));

return 0;

  }
@@ -98,21 +95,14 @@ void arch_dma_clear_uncached(void *cpu_addr, size_t size)
  void arch_sync_dma_for_device(phys_addr_t addr, size_t size,
                enum dma_data_direction dir)
  {
-       unsigned long cl;
-       struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
-
        switch (dir) {
        case DMA_TO_DEVICE:
                /* Flush the dcache for the requested range */
-               for (cl = addr; cl < addr + size;
-                    cl += cpuinfo->dcache_block_size)
-                       mtspr(SPR_DCBFR, cl);
+               local_dcache_range_flush(addr, addr + size);
                break;
        case DMA_FROM_DEVICE:
                /* Invalidate the dcache for the requested range */
-               for (cl = addr; cl < addr + size;
-                    cl += cpuinfo->dcache_block_size)
-                       mtspr(SPR_DCBIR, cl);
+               local_dcache_range_inv(addr, addr + size);
                break;
        default:
                /*
diff --git a/arch/openrisc/mm/cache.c b/arch/openrisc/mm/cache.c
index eb43b73f3855..3bf6d728d2d2 100644
--- a/arch/openrisc/mm/cache.c
+++ b/arch/openrisc/mm/cache.c
@@ -14,31 +14,63 @@
  #include <asm/spr_defs.h>
  #include <asm/cache.h>
  #include <asm/cacheflush.h>
+#include <asm/cpuinfo.h>
  #include <asm/tlbflush.h>

-static __always_inline void cache_loop(struct page *page, const unsigned int reg)

+static __always_inline void cache_loop(unsigned long paddr, unsigned long end,
+                                      const unsigned int reg, const unsigned 
int cache_type)
  {
-       unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
-       unsigned long line = paddr & ~(L1_CACHE_BYTES - 1);
+       if (!cpu_cache_is_present(cache_type))
+               return;
+
+       if (!cb_inv_flush_is_implemented(reg, cache_type))
+               return;

- while (line < paddr + PAGE_SIZE) {

-               mtspr(reg, line);
-               line += L1_CACHE_BYTES;
+       while (paddr < end) {
+               mtspr(reg, paddr);
+               paddr += L1_CACHE_BYTES;
        }
  }

+static void cache_loop_page(struct page *page, const unsigned int reg,

+                                           const unsigned int cache_type)
+{
+       unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
+       unsigned long end = paddr + PAGE_SIZE;
+
+       paddr &= ~(L1_CACHE_BYTES - 1);
+
+       cache_loop(paddr, end, reg, cache_type);
+}
+
  void local_dcache_page_flush(struct page *page)
  {
-       cache_loop(page, SPR_DCBFR);
+       cache_loop_page(page, SPR_DCBFR, SPR_UPR_DCP);
  }
  EXPORT_SYMBOL(local_dcache_page_flush);

void local_icache_page_inv(struct page *page)

  {
-       cache_loop(page, SPR_ICBIR);
+       cache_loop_page(page, SPR_ICBIR, SPR_UPR_ICP);
  }
  EXPORT_SYMBOL(local_icache_page_inv);

+void local_dcache_range_flush(unsigned long start, unsigned long end)

+{
+       cache_loop(start, end, SPR_DCBFR, SPR_UPR_DCP);
+}
+
+void local_dcache_range_inv(unsigned long start, unsigned long end)
+{
+       cache_loop(start, end, SPR_DCBIR, SPR_UPR_DCP);
+}
+
+void local_icache_range_inv(unsigned long start, unsigned long end)
+{
+       cache_loop(start, end, SPR_ICBIR, SPR_UPR_ICP);
+}
+
+

There is an extra newline here.

Oh, I missed this. I'll fix it.

  void update_cache(struct vm_area_struct *vma, unsigned long address,
        pte_t *pte)
  {
@@ -59,3 +91,42 @@ void update_cache(struct vm_area_struct *vma, unsigned long 
address,
        }
  }

+/*

+ * Check if the cache component exists.
+ */
+bool cpu_cache_is_present(const unsigned int cache_type)
+{
+       unsigned long upr = mfspr(SPR_UPR);
+
+       return !!(upr & (SPR_UPR_UP | cache_type));
+}
+
+ /*
+  * Check if the cache block flush/invalidate register is implemented for the
+  * cache component.
+  */
+bool cb_inv_flush_is_implemented(const unsigned int reg,
+                                const unsigned int cache_type)
+{
+       unsigned long cfgr;
+
+       if (cache_type == SPR_UPR_DCP) {
+               cfgr = mfspr(SPR_DCCFGR);
+               if (reg == SPR_DCBFR)
+                       return !!(cfgr & SPR_DCCFGR_CBFRI);
+
+               if (reg == SPR_DCBIR)
+                       return !!(cfgr & SPR_DCCFGR_CBIRI);
+       }
+
+       /*
+        * The cache block flush register is not implemented for the instruction
+        * cache.
+        */
+       if (cache_type == SPR_UPR_ICP) {
+               cfgr = mfspr(SPR_ICCFGR);
+               return !!(cfgr & SPR_ICCFGR_CBIRI);
+       }
+
+       return false;
+}

Usually we try to define functions before they are used.  This and
cpu_cache_is_present should be above cache_loop.

Got it. I'll shift this.

As I mentioned, this may be a bit overkill, OpenRISC completely ignore writes to
SPR registers that are not implemented.  They are basically nops.  It may help
to avoid running cache loops, but really there are no implementations I know of
that have caches (SPR_UPR_UP) but no cache invalidation/flush registers.

Understood. I'll remove this function.

Thanks,
Sahil