mbox series

[RFC,v3,0/2] AX45MP: Add support to non-coherent DMA

Message ID 20221019220242.4746-1-prabhakar.mahadev-lad.rj@bp.renesas.com
Headers show
Series AX45MP: Add support to non-coherent DMA | expand

Message

Prabhakar Oct. 19, 2022, 10:02 p.m. UTC
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

On the Andes AX45MP core, cache coherency is a specification option so it
may not be supported. In this case DMA will fail. To get around with this
issue this patch series does the below:

1] Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
block that allows dynamic adjustment of memory attributes in the runtime.
It contains a configurable amount of PMA entries implemented as CSR
registers to control the attributes of memory locations in interest. PMA
regions are passed from the l2 node which are configured as
non-cacheable + bufferable with the SBI call.

        l2cache: cache-controller@13400000 {
                ....
                andestech,pma-regions = <0x0 0x58000000 0x0 0x08000000 0x0
                                         (AX45MP_PMACFG_ETYP_NAPOT |
                                          AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF)>;
                ....
        };

2] We provide callbacks to synchronize specific content between memory and
cache.

        - arch_sync_dma_for_device()
        - arch_sync_dma_for_cpu()

Below are the configs that are enabled:

        - DMA_GLOBAL_POOL
        - RISCV_DMA_NONCOHERENT

3] We reserve the shared DMA pool, so the DMA memory requests go through
   this pool:

        reserved-memory {
                #address-cells = <2>;
                #size-cells = <2>;
                ranges;

                reserved: linux,cma@58000000 {
                        compatible = "shared-dma-pool";
                        no-map;
                        linux,dma-default;
                        reg = <0x0 0x58000000 0x0 0x08000000>;
                };
        };


Below is the L2 cache DT node:

        l2cache: cache-controller@13400000 {
                compatible = "andestech,ax45mp-cache", "cache";
                cache-size = <0x40000>;
                cache-line-size = <64>;
                cache-sets = <1024>;
                cache-unified;
                reg = <0x0 0x13400000 0x0 0x100000>;
                andestech,pma-regions = <0x0 0x58000000 0x0 0x08000000 0x0
                                         (AX45MP_PMACFG_ETYP_NAPOT |
                                          AX45MP_PMACFG_MTYP_MEM_NON_CACHE_BUF)>;
                andestech,inst-prefetch = <0x3>;
                andestech,data-prefetch = <0x3>;
                andestech,tag-ram-ctl = /bits/ 8 <0x1 0x0>;
                andestech,data-ram-ctl = /bits/ 8 <0x1 0x0>;
                interrupts = <SOC_PERIPHERAL_IRQ(476, IRQ_TYPE_LEVEL_HIGH)>;
        };

Due to the above approach custom SBI calls have been implemented. The
above implementation is in preparation for adding support for Renesas
RZ/Five SoC which uses the AX45MP core. As with the above approach the
kernel image might not be generic so that it can be used on other
platforms, so sending it as an RFC (without DT binding patches).

OpenSBI implementation isn't upstreamed yet, public repo for access is
available at [0].

[0] https://github.com/renesas-rz/rz_opensbi/tree/work/OpenSBI-PMA

RFC v2-> RFC v3
* Fixed review comments pointed by Conor
* Move DT binding into cache folder
* Fixed DT binding check issue
* Added andestech,ax45mp-cache.h header file
* Now passing the flags for the PMA setup as part of andestech,pma-regions
  property.
* Added andestech,inst/data-prefetch and andestech,tag/data-ram-ctl
  properties to configure the L2 cache.
* Registered the cache driver as platform driver

RFC v1-> RFC v2
* Moved out the code from arc/riscv to drivers/soc/renesas
* Now handling the PMA setup as part of the L2 cache
* Now making use of dma-noncoherent.c instead SoC specific implementation.
* Dropped arch_dma_alloc() and arch_dma_free()
* Switched to RISCV_DMA_NONCOHERENT
* Included DT binding doc

RFC v2: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20221003223222.448551-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
RFC v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20220906102154.32526-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Sending this as an RFC as CONFIG_ERRATA_THEAD_CMO/CONFIG_AX45MP_L2_CACHE
is used for determining the CMO to call it would better if we could do
this runtime instead.

Cheers,
Prabhakar

Lad Prabhakar (2):
  dt-bindings: cache: r9a07g043f-l2-cache: Add DT binding documentation
    for L2 cache controller
  soc: renesas: Add L2 cache management for RZ/Five SoC

 .../cache/andestech,ax45mp-cache.yaml         | 125 +++++
 arch/riscv/include/asm/cacheflush.h           |   8 +
 arch/riscv/include/asm/errata_list.h          |   2 +
 arch/riscv/mm/dma-noncoherent.c               |  20 +
 drivers/soc/renesas/Kconfig                   |   5 +
 drivers/soc/renesas/Makefile                  |   4 +
 drivers/soc/renesas/rzf/Kconfig               |   6 +
 drivers/soc/renesas/rzf/Makefile              |   3 +
 drivers/soc/renesas/rzf/ax45mp_cache.c        | 431 ++++++++++++++++++
 drivers/soc/renesas/rzf/ax45mp_sbi.h          |  29 ++
 .../cache/andestech,ax45mp-cache.h            |  38 ++
 11 files changed, 671 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cache/andestech,ax45mp-cache.yaml
 create mode 100644 drivers/soc/renesas/rzf/Kconfig
 create mode 100644 drivers/soc/renesas/rzf/Makefile
 create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
 create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
 create mode 100644 include/dt-bindings/cache/andestech,ax45mp-cache.h

Comments

Rob Herring (Arm) Oct. 21, 2022, 2:05 a.m. UTC | #1
On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On the AX45MP core, cache coherency is a specification option so it may
> not be supported. In this case DMA will fail. As a workaround, firstly we
> allocate a global dma coherent pool from which DMA allocations are taken
> and marked as non-cacheable + bufferable using the PMA region as specified
> in the device tree. Synchronization callbacks are implemented to
> synchronize when doing DMA transactions.
> 
> The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> block that allows dynamic adjustment of memory attributes in the runtime.
> It contains a configurable amount of PMA entries implemented as CSR
> registers to control the attributes of memory locations in interest.
> 
> Below are the memory attributes supported:
> * Device, Non-bufferable
> * Device, bufferable
> * Memory, Non-cacheable, Non-bufferable
> * Memory, Non-cacheable, Bufferable
> * Memory, Write-back, No-allocate
> * Memory, Write-back, Read-allocate
> * Memory, Write-back, Write-allocate
> * Memory, Write-back, Read and Write-allocate
> 
> This patch adds support to configure the memory attributes of the memory
> regions as passed from the l2 cache node and exposes the cache management
> ops.
> 
> More info about PMA (section 10.3):
> http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> 
> This feature is based on the work posted [0] by Vincent Chen
> <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> 
> [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  arch/riscv/include/asm/cacheflush.h    |   8 +
>  arch/riscv/include/asm/errata_list.h   |   2 +
>  arch/riscv/mm/dma-noncoherent.c        |  20 ++
>  drivers/soc/renesas/Kconfig            |   5 +
>  drivers/soc/renesas/Makefile           |   4 +
>  drivers/soc/renesas/rzf/Kconfig        |   6 +
>  drivers/soc/renesas/rzf/Makefile       |   3 +
>  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++

How many cache drivers do we have around now? I've seen a few bindings 
go by. I'm guessing it is time to stop putting the drivers in the
drivers/soc/ dumping ground.
 
>  drivers/soc/renesas/rzf/ax45mp_sbi.h   |  29 ++
>  9 files changed, 508 insertions(+)
>  create mode 100644 drivers/soc/renesas/rzf/Kconfig
>  create mode 100644 drivers/soc/renesas/rzf/Makefile
>  create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
>  create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
> 
> diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> index 8a5c246b0a21..40aa790be9a3 100644
> --- a/arch/riscv/include/asm/cacheflush.h
> +++ b/arch/riscv/include/asm/cacheflush.h
> @@ -65,6 +65,14 @@ static inline void riscv_noncoherent_supported(void) {}
>  #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
>  #define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
>  
> +#ifdef CONFIG_AX45MP_L2_CACHE
> +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t end);
> +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t end);
> +
> +#define ALT_CMO_OP(_op, _start, _size, _cachesize)	\
> +		   _op(_start, _size)
> +#endif
> +
>  #include <asm-generic/cacheflush.h>
>  
>  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> index 19a771085781..d9cbf60c3b65 100644
> --- a/arch/riscv/include/asm/errata_list.h
> +++ b/arch/riscv/include/asm/errata_list.h
> @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(						\
>  #define ALT_THEAD_PMA(_val)
>  #endif
>  
> +#ifdef CONFIG_ERRATA_THEAD_CMO
>  /*
>   * dcache.ipa rs1 (invalidate, physical address)
>   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(						\
>  	: "a0")
>  
>  #endif /* __ASSEMBLY__ */
> +#endif
>  
>  #endif
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index b0add983530a..5270acca6766 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>  
>  	switch (dir) {
>  	case DMA_TO_DEVICE:
> +#ifdef CONFIG_ERRATA_THEAD_CMO
>  		ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> +#elif CONFIG_AX45MP_L2_CACHE
> +		ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> +#endif

How do you support more than one platform in a build?

Rob
Prabhakar Oct. 21, 2022, 10:05 p.m. UTC | #2
Hi Rob,

Thank you for the review.

On Fri, Oct 21, 2022 at 3:05 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the AX45MP core, cache coherency is a specification option so it may
> > not be supported. In this case DMA will fail. As a workaround, firstly we
> > allocate a global dma coherent pool from which DMA allocations are taken
> > and marked as non-cacheable + bufferable using the PMA region as specified
> > in the device tree. Synchronization callbacks are implemented to
> > synchronize when doing DMA transactions.
> >
> > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > block that allows dynamic adjustment of memory attributes in the runtime.
> > It contains a configurable amount of PMA entries implemented as CSR
> > registers to control the attributes of memory locations in interest.
> >
> > Below are the memory attributes supported:
> > * Device, Non-bufferable
> > * Device, bufferable
> > * Memory, Non-cacheable, Non-bufferable
> > * Memory, Non-cacheable, Bufferable
> > * Memory, Write-back, No-allocate
> > * Memory, Write-back, Read-allocate
> > * Memory, Write-back, Write-allocate
> > * Memory, Write-back, Read and Write-allocate
> >
> > This patch adds support to configure the memory attributes of the memory
> > regions as passed from the l2 cache node and exposes the cache management
> > ops.
> >
> > More info about PMA (section 10.3):
> > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> >
> > This feature is based on the work posted [0] by Vincent Chen
> > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> >
> > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  arch/riscv/include/asm/cacheflush.h    |   8 +
> >  arch/riscv/include/asm/errata_list.h   |   2 +
> >  arch/riscv/mm/dma-noncoherent.c        |  20 ++
> >  drivers/soc/renesas/Kconfig            |   5 +
> >  drivers/soc/renesas/Makefile           |   4 +
> >  drivers/soc/renesas/rzf/Kconfig        |   6 +
> >  drivers/soc/renesas/rzf/Makefile       |   3 +
> >  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++
>
> How many cache drivers do we have around now? I've seen a few bindings
> go by. I'm guessing it is time to stop putting the drivers in the
> drivers/soc/ dumping ground.
>
The main reason this driver is not in arch/riscv is that it has vendor
specific extensions. Due to this reason it was agreed during the LPC
that vendor specific extension should be maintained by SoC vendors and
was agreed that this can go into drivers/soc/renesas folder instead.

> >  drivers/soc/renesas/rzf/ax45mp_sbi.h   |  29 ++
> >  9 files changed, 508 insertions(+)
> >  create mode 100644 drivers/soc/renesas/rzf/Kconfig
> >  create mode 100644 drivers/soc/renesas/rzf/Makefile
> >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
> >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
> >
> > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> > index 8a5c246b0a21..40aa790be9a3 100644
> > --- a/arch/riscv/include/asm/cacheflush.h
> > +++ b/arch/riscv/include/asm/cacheflush.h
> > @@ -65,6 +65,14 @@ static inline void riscv_noncoherent_supported(void) {}
> >  #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
> >  #define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
> >
> > +#ifdef CONFIG_AX45MP_L2_CACHE
> > +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t end);
> > +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t end);
> > +
> > +#define ALT_CMO_OP(_op, _start, _size, _cachesize)   \
> > +                _op(_start, _size)
> > +#endif
> > +
> >  #include <asm-generic/cacheflush.h>
> >
> >  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > index 19a771085781..d9cbf60c3b65 100644
> > --- a/arch/riscv/include/asm/errata_list.h
> > +++ b/arch/riscv/include/asm/errata_list.h
> > @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                           \
> >  #define ALT_THEAD_PMA(_val)
> >  #endif
> >
> > +#ifdef CONFIG_ERRATA_THEAD_CMO
> >  /*
> >   * dcache.ipa rs1 (invalidate, physical address)
> >   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> > @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                               \
> >       : "a0")
> >
> >  #endif /* __ASSEMBLY__ */
> > +#endif
> >
> >  #endif
> > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> > index b0add983530a..5270acca6766 100644
> > --- a/arch/riscv/mm/dma-noncoherent.c
> > +++ b/arch/riscv/mm/dma-noncoherent.c
> > @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> >
> >       switch (dir) {
> >       case DMA_TO_DEVICE:
> > +#ifdef CONFIG_ERRATA_THEAD_CMO
> >               ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> > +#elif CONFIG_AX45MP_L2_CACHE
> > +             ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> > +#endif
>
> How do you support more than one platform in a build?
>
Yes, that's one concern which I have mentioned in the cover letter too
(At that moment it's just a single platform). Suggestions welcome!

Cheers,
Prabhakar
Conor Dooley Oct. 21, 2022, 10:32 p.m. UTC | #3
On Fri, Oct 21, 2022 at 11:05:40PM +0100, Lad, Prabhakar wrote:
> Hi Rob,
> 
> Thank you for the review.
> 
> On Fri, Oct 21, 2022 at 3:05 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > On the AX45MP core, cache coherency is a specification option so it may
> > > not be supported. In this case DMA will fail. As a workaround, firstly we
> > > allocate a global dma coherent pool from which DMA allocations are taken
> > > and marked as non-cacheable + bufferable using the PMA region as specified
> > > in the device tree. Synchronization callbacks are implemented to
> > > synchronize when doing DMA transactions.
> > >
> > > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > > block that allows dynamic adjustment of memory attributes in the runtime.
> > > It contains a configurable amount of PMA entries implemented as CSR
> > > registers to control the attributes of memory locations in interest.
> > >
> > > Below are the memory attributes supported:
> > > * Device, Non-bufferable
> > > * Device, bufferable
> > > * Memory, Non-cacheable, Non-bufferable
> > > * Memory, Non-cacheable, Bufferable
> > > * Memory, Write-back, No-allocate
> > > * Memory, Write-back, Read-allocate
> > > * Memory, Write-back, Write-allocate
> > > * Memory, Write-back, Read and Write-allocate
> > >
> > > This patch adds support to configure the memory attributes of the memory
> > > regions as passed from the l2 cache node and exposes the cache management
> > > ops.
> > >
> > > More info about PMA (section 10.3):
> > > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> > >
> > > This feature is based on the work posted [0] by Vincent Chen
> > > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> > >
> > > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  arch/riscv/include/asm/cacheflush.h    |   8 +
> > >  arch/riscv/include/asm/errata_list.h   |   2 +
> > >  arch/riscv/mm/dma-noncoherent.c        |  20 ++
> > >  drivers/soc/renesas/Kconfig            |   5 +
> > >  drivers/soc/renesas/Makefile           |   4 +
> > >  drivers/soc/renesas/rzf/Kconfig        |   6 +
> > >  drivers/soc/renesas/rzf/Makefile       |   3 +
> > >  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++
> >
> > How many cache drivers do we have around now? I've seen a few bindings
> > go by. I'm guessing it is time to stop putting the drivers in the
> > drivers/soc/ dumping ground.
> >
> The main reason this driver is not in arch/riscv is that it has vendor
> specific extensions. Due to this reason it was agreed during the LPC
> that vendor specific extension should be maintained by SoC vendors and
> was agreed that this can go into drivers/soc/renesas folder instead.

Does not in drivers/soc mean they need to go into arch/riscv?
The outcome of the chat at the LPC BoF was more that the cache drivers
themselves should not be be routed via the arch maintainers, no?

> 
> > >  drivers/soc/renesas/rzf/ax45mp_sbi.h   |  29 ++
> > >  9 files changed, 508 insertions(+)
> > >  create mode 100644 drivers/soc/renesas/rzf/Kconfig
> > >  create mode 100644 drivers/soc/renesas/rzf/Makefile
> > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
> > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
> > >
> > > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> > > index 8a5c246b0a21..40aa790be9a3 100644
> > > --- a/arch/riscv/include/asm/cacheflush.h
> > > +++ b/arch/riscv/include/asm/cacheflush.h
> > > @@ -65,6 +65,14 @@ static inline void riscv_noncoherent_supported(void) {}
> > >  #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
> > >  #define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
> > >
> > > +#ifdef CONFIG_AX45MP_L2_CACHE
> > > +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t end);
> > > +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t end);
> > > +
> > > +#define ALT_CMO_OP(_op, _start, _size, _cachesize)   \
> > > +                _op(_start, _size)
> > > +#endif
> > > +
> > >  #include <asm-generic/cacheflush.h>
> > >
> > >  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > > index 19a771085781..d9cbf60c3b65 100644
> > > --- a/arch/riscv/include/asm/errata_list.h
> > > +++ b/arch/riscv/include/asm/errata_list.h
> > > @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                           \
> > >  #define ALT_THEAD_PMA(_val)
> > >  #endif
> > >
> > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > >  /*
> > >   * dcache.ipa rs1 (invalidate, physical address)
> > >   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> > > @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                               \
> > >       : "a0")
> > >
> > >  #endif /* __ASSEMBLY__ */
> > > +#endif
> > >
> > >  #endif
> > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> > > index b0add983530a..5270acca6766 100644
> > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> > >
> > >       switch (dir) {
> > >       case DMA_TO_DEVICE:
> > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > >               ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> > > +#elif CONFIG_AX45MP_L2_CACHE
> > > +             ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> > > +#endif
> >
> > How do you support more than one platform in a build?
> >
> Yes, that's one concern which I have mentioned in the cover letter too
> (At that moment it's just a single platform). Suggestions welcome!

I think I said it on one of the earlier version, but it needs to be
implemented w/ runtime patching via alternatives just like the thead
stuff patches in their functions.
Prabhakar Oct. 24, 2022, 11:55 a.m. UTC | #4
Hi Conor,

On Fri, Oct 21, 2022 at 11:32 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Oct 21, 2022 at 11:05:40PM +0100, Lad, Prabhakar wrote:
> > Hi Rob,
> >
> > Thank you for the review.
> >
> > On Fri, Oct 21, 2022 at 3:05 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > On the AX45MP core, cache coherency is a specification option so it may
> > > > not be supported. In this case DMA will fail. As a workaround, firstly we
> > > > allocate a global dma coherent pool from which DMA allocations are taken
> > > > and marked as non-cacheable + bufferable using the PMA region as specified
> > > > in the device tree. Synchronization callbacks are implemented to
> > > > synchronize when doing DMA transactions.
> > > >
> > > > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > > > block that allows dynamic adjustment of memory attributes in the runtime.
> > > > It contains a configurable amount of PMA entries implemented as CSR
> > > > registers to control the attributes of memory locations in interest.
> > > >
> > > > Below are the memory attributes supported:
> > > > * Device, Non-bufferable
> > > > * Device, bufferable
> > > > * Memory, Non-cacheable, Non-bufferable
> > > > * Memory, Non-cacheable, Bufferable
> > > > * Memory, Write-back, No-allocate
> > > > * Memory, Write-back, Read-allocate
> > > > * Memory, Write-back, Write-allocate
> > > > * Memory, Write-back, Read and Write-allocate
> > > >
> > > > This patch adds support to configure the memory attributes of the memory
> > > > regions as passed from the l2 cache node and exposes the cache management
> > > > ops.
> > > >
> > > > More info about PMA (section 10.3):
> > > > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> > > >
> > > > This feature is based on the work posted [0] by Vincent Chen
> > > > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> > > >
> > > > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  arch/riscv/include/asm/cacheflush.h    |   8 +
> > > >  arch/riscv/include/asm/errata_list.h   |   2 +
> > > >  arch/riscv/mm/dma-noncoherent.c        |  20 ++
> > > >  drivers/soc/renesas/Kconfig            |   5 +
> > > >  drivers/soc/renesas/Makefile           |   4 +
> > > >  drivers/soc/renesas/rzf/Kconfig        |   6 +
> > > >  drivers/soc/renesas/rzf/Makefile       |   3 +
> > > >  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++
> > >
> > > How many cache drivers do we have around now? I've seen a few bindings
> > > go by. I'm guessing it is time to stop putting the drivers in the
> > > drivers/soc/ dumping ground.
> > >
> > The main reason this driver is not in arch/riscv is that it has vendor
> > specific extensions. Due to this reason it was agreed during the LPC
> > that vendor specific extension should be maintained by SoC vendors and
> > was agreed that this can go into drivers/soc/renesas folder instead.
>
> Does not in drivers/soc mean they need to go into arch/riscv?
I was under the impression Rob wanted them arch/riscv, sorry for the confusion.

> The outcome of the chat at the LPC BoF was more that the cache drivers
> themselves should not be be routed via the arch maintainers, no?
>
Indeed.

> >
> > > >  drivers/soc/renesas/rzf/ax45mp_sbi.h   |  29 ++
> > > >  9 files changed, 508 insertions(+)
> > > >  create mode 100644 drivers/soc/renesas/rzf/Kconfig
> > > >  create mode 100644 drivers/soc/renesas/rzf/Makefile
> > > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
> > > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
> > > >
> > > > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> > > > index 8a5c246b0a21..40aa790be9a3 100644
> > > > --- a/arch/riscv/include/asm/cacheflush.h
> > > > +++ b/arch/riscv/include/asm/cacheflush.h
> > > > @@ -65,6 +65,14 @@ static inline void riscv_noncoherent_supported(void) {}
> > > >  #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
> > > >  #define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
> > > >
> > > > +#ifdef CONFIG_AX45MP_L2_CACHE
> > > > +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t end);
> > > > +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t end);
> > > > +
> > > > +#define ALT_CMO_OP(_op, _start, _size, _cachesize)   \
> > > > +                _op(_start, _size)
> > > > +#endif
> > > > +
> > > >  #include <asm-generic/cacheflush.h>
> > > >
> > > >  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > > > index 19a771085781..d9cbf60c3b65 100644
> > > > --- a/arch/riscv/include/asm/errata_list.h
> > > > +++ b/arch/riscv/include/asm/errata_list.h
> > > > @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                           \
> > > >  #define ALT_THEAD_PMA(_val)
> > > >  #endif
> > > >
> > > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > > >  /*
> > > >   * dcache.ipa rs1 (invalidate, physical address)
> > > >   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> > > > @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                               \
> > > >       : "a0")
> > > >
> > > >  #endif /* __ASSEMBLY__ */
> > > > +#endif
> > > >
> > > >  #endif
> > > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> > > > index b0add983530a..5270acca6766 100644
> > > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > > @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> > > >
> > > >       switch (dir) {
> > > >       case DMA_TO_DEVICE:
> > > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > > >               ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> > > > +#elif CONFIG_AX45MP_L2_CACHE
> > > > +             ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> > > > +#endif
> > >
> > > How do you support more than one platform in a build?
> > >
> > Yes, that's one concern which I have mentioned in the cover letter too
> > (At that moment it's just a single platform). Suggestions welcome!
>
> I think I said it on one of the earlier version, but it needs to be
> implemented w/ runtime patching via alternatives just like the thead
> stuff patches in their functions.
>
I'm a bit stumped with alternatives() usage.

Currently I am just replacing the ALT_CMO_OP() macro if
CONFIG_AX45MP_L2_CACHE is enabled. For AX45MP currently we have two
exported functions ax45mp_cpu_dma_inval_range/ax45mp_cpu_dma_wb_range.
If I switch to
ALTERNATIVE() macro usage then I'll have to use the assembly version
of the above two mentioned functions?

Cheers,
Prabhakar
Heiko Stübner Oct. 24, 2022, 12:04 p.m. UTC | #5
Hi Prabhakar,

Am Montag, 24. Oktober 2022, 13:55:00 CEST schrieb Lad, Prabhakar:
> Hi Conor,
> 
> On Fri, Oct 21, 2022 at 11:32 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Fri, Oct 21, 2022 at 11:05:40PM +0100, Lad, Prabhakar wrote:
> > > Hi Rob,
> > >
> > > Thank you for the review.
> > >
> > > On Fri, Oct 21, 2022 at 3:05 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > On the AX45MP core, cache coherency is a specification option so it may
> > > > > not be supported. In this case DMA will fail. As a workaround, firstly we
> > > > > allocate a global dma coherent pool from which DMA allocations are taken
> > > > > and marked as non-cacheable + bufferable using the PMA region as specified
> > > > > in the device tree. Synchronization callbacks are implemented to
> > > > > synchronize when doing DMA transactions.
> > > > >
> > > > > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > > > > block that allows dynamic adjustment of memory attributes in the runtime.
> > > > > It contains a configurable amount of PMA entries implemented as CSR
> > > > > registers to control the attributes of memory locations in interest.
> > > > >
> > > > > Below are the memory attributes supported:
> > > > > * Device, Non-bufferable
> > > > > * Device, bufferable
> > > > > * Memory, Non-cacheable, Non-bufferable
> > > > > * Memory, Non-cacheable, Bufferable
> > > > > * Memory, Write-back, No-allocate
> > > > > * Memory, Write-back, Read-allocate
> > > > > * Memory, Write-back, Write-allocate
> > > > > * Memory, Write-back, Read and Write-allocate
> > > > >
> > > > > This patch adds support to configure the memory attributes of the memory
> > > > > regions as passed from the l2 cache node and exposes the cache management
> > > > > ops.
> > > > >
> > > > > More info about PMA (section 10.3):
> > > > > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> > > > >
> > > > > This feature is based on the work posted [0] by Vincent Chen
> > > > > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> > > > >
> > > > > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/cacheflush.h    |   8 +
> > > > >  arch/riscv/include/asm/errata_list.h   |   2 +
> > > > >  arch/riscv/mm/dma-noncoherent.c        |  20 ++
> > > > >  drivers/soc/renesas/Kconfig            |   5 +
> > > > >  drivers/soc/renesas/Makefile           |   4 +
> > > > >  drivers/soc/renesas/rzf/Kconfig        |   6 +
> > > > >  drivers/soc/renesas/rzf/Makefile       |   3 +
> > > > >  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++
> > > >
> > > > How many cache drivers do we have around now? I've seen a few bindings
> > > > go by. I'm guessing it is time to stop putting the drivers in the
> > > > drivers/soc/ dumping ground.
> > > >
> > > The main reason this driver is not in arch/riscv is that it has vendor
> > > specific extensions. Due to this reason it was agreed during the LPC
> > > that vendor specific extension should be maintained by SoC vendors and
> > > was agreed that this can go into drivers/soc/renesas folder instead.
> >
> > Does not in drivers/soc mean they need to go into arch/riscv?
> I was under the impression Rob wanted them arch/riscv, sorry for the confusion.
> 
> > The outcome of the chat at the LPC BoF was more that the cache drivers
> > themselves should not be be routed via the arch maintainers, no?
> >
> Indeed.
> 
> > >
> > > > >  drivers/soc/renesas/rzf/ax45mp_sbi.h   |  29 ++
> > > > >  9 files changed, 508 insertions(+)
> > > > >  create mode 100644 drivers/soc/renesas/rzf/Kconfig
> > > > >  create mode 100644 drivers/soc/renesas/rzf/Makefile
> > > > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
> > > > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> > > > > index 8a5c246b0a21..40aa790be9a3 100644
> > > > > --- a/arch/riscv/include/asm/cacheflush.h
> > > > > +++ b/arch/riscv/include/asm/cacheflush.h
> > > > > @@ -65,6 +65,14 @@ static inline void riscv_noncoherent_supported(void) {}
> > > > >  #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
> > > > >  #define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
> > > > >
> > > > > +#ifdef CONFIG_AX45MP_L2_CACHE
> > > > > +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t end);
> > > > > +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t end);
> > > > > +
> > > > > +#define ALT_CMO_OP(_op, _start, _size, _cachesize)   \
> > > > > +                _op(_start, _size)
> > > > > +#endif
> > > > > +
> > > > >  #include <asm-generic/cacheflush.h>
> > > > >
> > > > >  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > > > > index 19a771085781..d9cbf60c3b65 100644
> > > > > --- a/arch/riscv/include/asm/errata_list.h
> > > > > +++ b/arch/riscv/include/asm/errata_list.h
> > > > > @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                           \
> > > > >  #define ALT_THEAD_PMA(_val)
> > > > >  #endif
> > > > >
> > > > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > > > >  /*
> > > > >   * dcache.ipa rs1 (invalidate, physical address)
> > > > >   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> > > > > @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                               \
> > > > >       : "a0")
> > > > >
> > > > >  #endif /* __ASSEMBLY__ */
> > > > > +#endif
> > > > >
> > > > >  #endif
> > > > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> > > > > index b0add983530a..5270acca6766 100644
> > > > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > > > @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> > > > >
> > > > >       switch (dir) {
> > > > >       case DMA_TO_DEVICE:
> > > > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > > > >               ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> > > > > +#elif CONFIG_AX45MP_L2_CACHE
> > > > > +             ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> > > > > +#endif
> > > >
> > > > How do you support more than one platform in a build?
> > > >
> > > Yes, that's one concern which I have mentioned in the cover letter too
> > > (At that moment it's just a single platform). Suggestions welcome!
> >
> > I think I said it on one of the earlier version, but it needs to be
> > implemented w/ runtime patching via alternatives just like the thead
> > stuff patches in their functions.
> >
> I'm a bit stumped with alternatives() usage.
> 
> Currently I am just replacing the ALT_CMO_OP() macro if
> CONFIG_AX45MP_L2_CACHE is enabled. For AX45MP currently we have two
> exported functions ax45mp_cpu_dma_inval_range/ax45mp_cpu_dma_wb_range.
> If I switch to
> ALTERNATIVE() macro usage then I'll have to use the assembly version
> of the above two mentioned functions?

The overarching goal should always be the unified-kernel-image.
So hardware-specific compile-time #ifeefs are normally a no-no :-) .

So yes, it most likely should be assembly-based, and you'll "just" need
to introduce an ALTERNATIVE_3 macro, similar to what ALTERNAITVE_2 does.

That is actually the really nice part of alternatives, that you can have as
many variants as you like.

Heiko
Geert Uytterhoeven Oct. 24, 2022, 2:22 p.m. UTC | #6
Hi Prabhakar,

(fixed Palmer's address)

On Thu, Oct 20, 2022 at 12:02 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On the AX45MP core, cache coherency is a specification option so it may
> not be supported. In this case DMA will fail. As a workaround, firstly we
> allocate a global dma coherent pool from which DMA allocations are taken
> and marked as non-cacheable + bufferable using the PMA region as specified
> in the device tree. Synchronization callbacks are implemented to
> synchronize when doing DMA transactions.
>
> The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> block that allows dynamic adjustment of memory attributes in the runtime.
> It contains a configurable amount of PMA entries implemented as CSR
> registers to control the attributes of memory locations in interest.
>
> Below are the memory attributes supported:
> * Device, Non-bufferable
> * Device, bufferable
> * Memory, Non-cacheable, Non-bufferable
> * Memory, Non-cacheable, Bufferable
> * Memory, Write-back, No-allocate
> * Memory, Write-back, Read-allocate
> * Memory, Write-back, Write-allocate
> * Memory, Write-back, Read and Write-allocate
>
> This patch adds support to configure the memory attributes of the memory
> regions as passed from the l2 cache node and exposes the cache management
> ops.
>
> More info about PMA (section 10.3):
> http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
>
> This feature is based on the work posted [0] by Vincent Chen
> <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
>
> [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/soc/renesas/Kconfig
> +++ b/drivers/soc/renesas/Kconfig
> @@ -340,9 +340,14 @@ if RISCV
>  config ARCH_R9A07G043
>         bool "RISC-V Platform support for RZ/Five"
>         select ARCH_RZG2L
> +       select AX45MP_L2_CACHE
> +       select DMA_GLOBAL_POOL
> +       select RISCV_DMA_NONCOHERENT
>         help
>           This enables support for the Renesas RZ/Five SoC.
>
> +source "drivers/soc/renesas/rzf/Kconfig"

s/rzf/rzfive/? (or "rz5"? "rzv"?)

> +
>  endif # RISCV
>
>  config RST_RCAR
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
> index 535868c9c7e4..a20cc7ad5b12 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -31,6 +31,10 @@ ifdef CONFIG_SMP
>  obj-$(CONFIG_ARCH_R9A06G032)   += r9a06g032-smp.o
>  endif
>
> +ifdef CONFIG_RISCV
> +obj-y += rzf/
> +endif

obj-$(CONFIG_RISCV)

> --- /dev/null
> +++ b/drivers/soc/renesas/rzf/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config AX45MP_L2_CACHE
> +       bool "AX45MP L2 Cache controller"

Andes Technology ...

> +       help
> +         Support for the L2 cache controller on AX45MP platforms.

... Andes Technology ...

> --- /dev/null
> +++ b/drivers/soc/renesas/rzf/ax45mp_cache.c
> @@ -0,0 +1,431 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PMA setup and non-coherent cache functions for AX45MP
> + *
> + * Copyright (C) 2022 Renesas Electronics Corp.
> + */
> +
> +#include <linux/cacheflush.h>
> +#include <linux/cacheinfo.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/sbi.h>
> +
> +#include "ax45mp_sbi.h"
> +
> +/* L2 cache registers */
> +#define AX45MP_L2C_REG_CTL_OFFSET              0x8
> +#define AX45MP_L2C_IPREPETCH_OFF               3
> +#define AX45MP_L2C_DPREPETCH_OFF               5
> +#define AX45MP_L2C_IPREPETCH_MSK               (3 << AX45MP_L2C_IPREPETCH_OFF)
> +#define AX45MP_L2C_DPREPETCH_MSK               (3 << AX45MP_L2C_DPREPETCH_OFF)

#define AX45MP_L2C_IPREPETCH    GENMASK(4, 3)
etc., and then you can use the FIELD_PREP() macros.

> +#define AX45MP_L2C_TRAMOCTL_OFF                        8
> +#define AX45MP_L2C_TRAMICTL_OFF                        10
> +#define AX45MP_L2C_TRAMOCTL_MSK                        (3 << AX45MP_L2C_TRAMOCTL_OFF)
> +#define AX45MP_L2C_TRAMICTL_MSK                        BIT(AX45MP_L2C_TRAMICTL_OFF)
> +#define AX45MP_L2C_DRAMOCTL_OFF                        11
> +#define AX45MP_L2C_DRAMICTL_OFF                        13
> +#define AX45MP_L2C_DRAMOCTL_MSK                        (3 << AX45MP_L2C_DRAMOCTL_OFF)
> +#define AX45MP_L2C_DRAMICTL_MSK                        BIT(AX45MP_L2C_DRAMICTL_OFF)

> +
> +#define AX45MP_MAX_CACHE_LINE_SIZE             256
> +
> +#define AX45MP_MAX_PMA_REGIONS                 16
> +
> +struct ax45mp_priv {
> +       void __iomem *l2c_base;
> +       unsigned int ax45mp_cache_line_size;
> +       bool l2cache_enabled;
> +       bool ucctl_ok;
> +};
> +
> +static struct ax45mp_priv *ax45mp_priv;
> +static DEFINE_STATIC_KEY_FALSE(ax45mp_l2c_configured);
> +
> +/* PMA setup */
> +static long ax45mp_sbi_set_pma(unsigned long start,
> +                              unsigned long size,
> +                              unsigned long flags,
> +                              unsigned int entry_id)
> +{
> +       struct sbiret ret;
> +
> +       ret = sbi_ecall(SBI_EXT_ANDES, AX45MP_SBI_EXT_SET_PMA,
> +                       start, start + size, size, entry_id,
> +                       flags, 0);

Fits on two lines.

> +
> +       return ret.value;
> +}
> +
> +static int ax45mp_configure_pma_regions(struct device_node *np)
> +{
> +       const char *propname = "andestech,pma-regions";
> +       u64 start, size, flags;
> +       unsigned int entry_id;
> +       unsigned int i;
> +       int count;
> +       int ret;
> +
> +       count = of_property_count_elems_of_size(np, propname,
> +                                               sizeof(u32) * 6);

Fits on a single line.

> +static inline uint32_t ax45mp_cpu_l2c_get_cctl_status(void)
> +{
> +       return readl((void *)(ax45mp_priv->l2c_base + AX45MP_L2C_REG_STATUS_OFFSET));

Why the cast to "(void *)"?

> +}
> +
> +static inline uint32_t ax45mp_cpu_l2c_ctl_status(void)
> +{
> +       return readl((void *)(ax45mp_priv->l2c_base + AX45MP_L2C_REG_CTL_OFFSET));

Likewise.

> +}

> +static void ax45mp_cpu_dcache_wb_range(unsigned long start,
> +                                      unsigned long end,
> +                                      int line_size)
> +{
> +       void __iomem *base = ax45mp_priv->l2c_base;
> +       unsigned long pa;
> +       int mhartid = 0;
> +#ifdef CONFIG_SMP
> +       mhartid = smp_processor_id();
> +#endif
> +
> +       while (end > start) {
> +               if (ax45mp_priv->ucctl_ok) {
> +                       csr_write(AX45MP_CCTL_REG_UCCTLBEGINADDR_NUM, start);
> +                       csr_write(AX45MP_CCTL_REG_UCCTLCOMMAND_NUM, AX45MP_CCTL_L1D_VA_WB);
> +               }
> +
> +               if (ax45mp_priv->l2cache_enabled) {
> +                       pa = virt_to_phys((void *)start);

Looks like start and end should be "void *" instead of " unsigned long",
as they are virtual addresses. See also below...

> +                       writel(pa, (void *)(base + AX45MP_L2C_REG_CN_ACC_OFFSET(mhartid)));
> +                       writel(AX45MP_CCTL_L2_PA_WB,
> +                              (void *)(base + AX45MP_L2C_REG_CN_CMD_OFFSET(mhartid)));

Why the casts to "(void *)"?

> +                       while ((ax45mp_cpu_l2c_get_cctl_status() &
> +                               AX45MP_CCTL_L2_STATUS_CN_MASK(mhartid)) !=
> +                               AX45MP_CCTL_L2_STATUS_IDLE)
> +                               ;
> +               }
> +
> +               start += line_size;
> +       }
> +}
> +
> +static void ax45mp_cpu_dcache_inval_range(unsigned long start,
> +                                         unsigned long end,
> +                                         int line_size)
> +{
> +       void __iomem *base = ax45mp_priv->l2c_base;
> +       unsigned long pa;
> +       int mhartid = 0;
> +#ifdef CONFIG_SMP
> +       mhartid = smp_processor_id();
> +#endif
> +
> +       while (end > start) {
> +               if (ax45mp_priv->ucctl_ok) {
> +                       csr_write(AX45MP_CCTL_REG_UCCTLBEGINADDR_NUM, start);
> +                       csr_write(AX45MP_CCTL_REG_UCCTLCOMMAND_NUM, AX45MP_CCTL_L1D_VA_INVAL);
> +               }
> +
> +               if (ax45mp_priv->l2cache_enabled) {
> +                       pa = virt_to_phys((void *)start);

Looks like start and end should be "void *" instead of " unsigned long",
as they are virtual addresses. See also below...

> +                       writel(pa, (void *)(base + AX45MP_L2C_REG_CN_ACC_OFFSET(mhartid)));
> +                       writel(AX45MP_CCTL_L2_PA_INVAL,
> +                              (void *)(base + AX45MP_L2C_REG_CN_CMD_OFFSET(mhartid)));
> +                       while ((ax45mp_cpu_l2c_get_cctl_status() &
> +                               AX45MP_CCTL_L2_STATUS_CN_MASK(mhartid)) !=
> +                               AX45MP_CCTL_L2_STATUS_IDLE)
> +                               ;
> +               }
> +
> +               start += line_size;
> +       }
> +}
> +
> +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t size)
> +{
> +       char cache_buf[2][AX45MP_MAX_CACHE_LINE_SIZE] = { 0 };

AX45MP_MAX_CACHE_LINE_SIZE = 256, so 512 bytes of data on the stack,
auto-initialized by memset().

Please remove the { 0 }, ...

> +       unsigned long start = (unsigned long)vaddr;
> +       unsigned long end = start + size;
> +       unsigned long old_start = start;
> +       unsigned long old_end = end;
> +       unsigned long line_size;
> +       unsigned long flags;
> +
> +       if (static_branch_unlikely(&ax45mp_l2c_configured) && !ax45mp_priv)
> +               return;
> +
> +       if (unlikely(start == end))
> +               return;
> +
> +       line_size = ax45mp_priv->ax45mp_cache_line_size;

... and call memset() here, so the buffer is not initialized when unused.
Perhaps use two buffers, so you can easily memset() only the part that is
used?

> +
> +       start = start & (~(line_size - 1));
> +       end = ((end + line_size - 1) & (~(line_size - 1)));

These are the only calculations that need to use "unsigned long"
instead of "void *", but you can use PTR_ALIGN_DOWN() and PTR_ALIGN()
to avoid explicit casts.

> +
> +       local_irq_save(flags);
> +       if (unlikely(start != old_start))
> +               memcpy(&cache_buf[0][0], (void *)start, line_size);
> +
> +       if (unlikely(end != old_end))
> +               memcpy(&cache_buf[1][0], (void *)(old_end & (~(line_size - 1))), line_size);

PTR_ALIGN_DOWN()

> +
> +       ax45mp_cpu_dcache_inval_range(start, end, line_size);
> +
> +       if (unlikely(start != old_start))
> +               memcpy((void *)start, &cache_buf[0][0], (old_start & (line_size - 1)));
> +
> +       if (unlikely(end != old_end))
> +               memcpy((void *)(old_end + 1),
> +                      &cache_buf[1][(old_end & (line_size - 1)) + 1],
> +                      end - old_end - 1);
> +
> +       local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL(ax45mp_cpu_dma_inval_range);
> +
> +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t size)
> +{
> +       unsigned long start = (unsigned long)vaddr;
> +       unsigned long end = start + size;
> +       unsigned long line_size;
> +       unsigned long flags;
> +
> +       if (static_branch_unlikely(&ax45mp_l2c_configured) && !ax45mp_priv)
> +               return;
> +
> +       line_size = ax45mp_priv->ax45mp_cache_line_size;
> +       local_irq_save(flags);
> +       start = start & (~(line_size - 1));

PTR_ALIGN_DOWN() etc...

> +       ax45mp_cpu_dcache_wb_range(start, end, line_size);
> +       local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL(ax45mp_cpu_dma_wb_range);
> +
> +static int ax45mp_configure_l2_cache(struct device_node *np)
> +{
> +       u8 ram_ctl[2];
> +       u32 cache_ctl;
> +       u32 prefetch;
> +       int ret;
> +
> +       cache_ctl = ax45mp_cpu_l2c_ctl_status();
> +
> +       /* Instruction and data fetch prefetch depth */
> +       ret = of_property_read_u32(np, "andestech,inst-prefetch", &prefetch);
> +       if (!ret) {
> +               cache_ctl &= ~AX45MP_L2C_IPREPETCH_MSK;
> +               cache_ctl |= (prefetch << AX45MP_L2C_IPREPETCH_OFF);

FIELD_PREP(), also below

> +       }
> +
> +       ret = of_property_read_u32(np, "andestech,data-prefetch", &prefetch);
> +       if (!ret) {
> +               cache_ctl &= ~AX45MP_L2C_DPREPETCH_MSK;
> +               cache_ctl |= (prefetch << AX45MP_L2C_DPREPETCH_OFF);

prefect / 2

> +       }
> +
> +       /* tag RAM and data RAM setup and output cycle */
> +       ret = of_property_read_u8_array(np, "andestech,tag-ram-ctl", ram_ctl, 2);
> +       if (!ret) {
> +               cache_ctl &= ~(AX45MP_L2C_TRAMOCTL_MSK | AX45MP_L2C_TRAMICTL_MSK);
> +               cache_ctl |= ram_ctl[0] << AX45MP_L2C_TRAMOCTL_OFF;
> +               cache_ctl |= ram_ctl[1] << AX45MP_L2C_TRAMICTL_OFF;
> +       }
> +
> +       ret = of_property_read_u8_array(np, "andestech,data-ram-ctl", ram_ctl, 2);
> +       if (!ret) {
> +               cache_ctl &= ~(AX45MP_L2C_DRAMOCTL_MSK | AX45MP_L2C_DRAMICTL_MSK);
> +               cache_ctl |= ram_ctl[0] << AX45MP_L2C_DRAMOCTL_OFF;
> +               cache_ctl |= ram_ctl[1] << AX45MP_L2C_DRAMICTL_OFF;
> +       }
> +
> +       writel(cache_ctl, ax45mp_priv->l2c_base + AX45MP_L2C_REG_CTL_OFFSET);
> +
> +       ret = of_property_read_u32(np, "cache-line-size", &ax45mp_priv->ax45mp_cache_line_size);

According to the bindings, this must be 64?

> +       if (ret) {
> +               pr_err("Failed to get cache-line-size defaulting to 64 bytes\n");
> +               ax45mp_priv->ax45mp_cache_line_size = SZ_64;
> +       }
> +
> +       ax45mp_priv->ucctl_ok = ax45mp_cpu_cache_controlable();
> +       ax45mp_priv->l2cache_enabled = ax45mp_cpu_l2c_ctl_status() & AX45MP_L2_CACHE_CTL_CEN_MASK;
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Prabhakar Oct. 25, 2022, 11:07 p.m. UTC | #7
Hi Geert,

Thank you for the review.

On Mon, Oct 24, 2022 at 3:22 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> (fixed Palmer's address)
>
> On Thu, Oct 20, 2022 at 12:02 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the AX45MP core, cache coherency is a specification option so it may
> > not be supported. In this case DMA will fail. As a workaround, firstly we
> > allocate a global dma coherent pool from which DMA allocations are taken
> > and marked as non-cacheable + bufferable using the PMA region as specified
> > in the device tree. Synchronization callbacks are implemented to
> > synchronize when doing DMA transactions.
> >
> > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > block that allows dynamic adjustment of memory attributes in the runtime.
> > It contains a configurable amount of PMA entries implemented as CSR
> > registers to control the attributes of memory locations in interest.
> >
> > Below are the memory attributes supported:
> > * Device, Non-bufferable
> > * Device, bufferable
> > * Memory, Non-cacheable, Non-bufferable
> > * Memory, Non-cacheable, Bufferable
> > * Memory, Write-back, No-allocate
> > * Memory, Write-back, Read-allocate
> > * Memory, Write-back, Write-allocate
> > * Memory, Write-back, Read and Write-allocate
> >
> > This patch adds support to configure the memory attributes of the memory
> > regions as passed from the l2 cache node and exposes the cache management
> > ops.
> >
> > More info about PMA (section 10.3):
> > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> >
> > This feature is based on the work posted [0] by Vincent Chen
> > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> >
> > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/soc/renesas/Kconfig
> > +++ b/drivers/soc/renesas/Kconfig
> > @@ -340,9 +340,14 @@ if RISCV
> >  config ARCH_R9A07G043
> >         bool "RISC-V Platform support for RZ/Five"
> >         select ARCH_RZG2L
> > +       select AX45MP_L2_CACHE
> > +       select DMA_GLOBAL_POOL
> > +       select RISCV_DMA_NONCOHERENT
> >         help
> >           This enables support for the Renesas RZ/Five SoC.
> >
> > +source "drivers/soc/renesas/rzf/Kconfig"
>
> s/rzf/rzfive/? (or "rz5"? "rzv"?)
>
OK, I'll rename it to rzfive instead.

> > +
> >  endif # RISCV
> >
> >  config RST_RCAR
> > diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
> > index 535868c9c7e4..a20cc7ad5b12 100644
> > --- a/drivers/soc/renesas/Makefile
> > +++ b/drivers/soc/renesas/Makefile
> > @@ -31,6 +31,10 @@ ifdef CONFIG_SMP
> >  obj-$(CONFIG_ARCH_R9A06G032)   += r9a06g032-smp.o
> >  endif
> >
> > +ifdef CONFIG_RISCV
> > +obj-y += rzf/
> > +endif
>
> obj-$(CONFIG_RISCV)
>
Agreed.

> > --- /dev/null
> > +++ b/drivers/soc/renesas/rzf/Kconfig
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +config AX45MP_L2_CACHE
> > +       bool "AX45MP L2 Cache controller"
>
> Andes Technology ...
>
OK, "Andes Technology AX45MP L2 Cache controller"

> > +       help
> > +         Support for the L2 cache controller on AX45MP platforms.
>
> ... Andes Technology ...
>
OK.

> > --- /dev/null
> > +++ b/drivers/soc/renesas/rzf/ax45mp_cache.c
> > @@ -0,0 +1,431 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PMA setup and non-coherent cache functions for AX45MP
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/cacheflush.h>
> > +#include <linux/cacheinfo.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +
> > +#include <asm/sbi.h>
> > +
> > +#include "ax45mp_sbi.h"
> > +
> > +/* L2 cache registers */
> > +#define AX45MP_L2C_REG_CTL_OFFSET              0x8
> > +#define AX45MP_L2C_IPREPETCH_OFF               3
> > +#define AX45MP_L2C_DPREPETCH_OFF               5
> > +#define AX45MP_L2C_IPREPETCH_MSK               (3 << AX45MP_L2C_IPREPETCH_OFF)
> > +#define AX45MP_L2C_DPREPETCH_MSK               (3 << AX45MP_L2C_DPREPETCH_OFF)
>
> #define AX45MP_L2C_IPREPETCH    GENMASK(4, 3)
> etc., and then you can use the FIELD_PREP() macros.
>
Agreed, now that we have decided to drop the setup of l2c controls the
above macros can be dropped.

> > +#define AX45MP_L2C_TRAMOCTL_OFF                        8
> > +#define AX45MP_L2C_TRAMICTL_OFF                        10
> > +#define AX45MP_L2C_TRAMOCTL_MSK                        (3 << AX45MP_L2C_TRAMOCTL_OFF)
> > +#define AX45MP_L2C_TRAMICTL_MSK                        BIT(AX45MP_L2C_TRAMICTL_OFF)
> > +#define AX45MP_L2C_DRAMOCTL_OFF                        11
> > +#define AX45MP_L2C_DRAMICTL_OFF                        13
> > +#define AX45MP_L2C_DRAMOCTL_MSK                        (3 << AX45MP_L2C_DRAMOCTL_OFF)
> > +#define AX45MP_L2C_DRAMICTL_MSK                        BIT(AX45MP_L2C_DRAMICTL_OFF)
>
> > +
> > +#define AX45MP_MAX_CACHE_LINE_SIZE             256
> > +
> > +#define AX45MP_MAX_PMA_REGIONS                 16
> > +
> > +struct ax45mp_priv {
> > +       void __iomem *l2c_base;
> > +       unsigned int ax45mp_cache_line_size;
> > +       bool l2cache_enabled;
> > +       bool ucctl_ok;
> > +};
> > +
> > +static struct ax45mp_priv *ax45mp_priv;
> > +static DEFINE_STATIC_KEY_FALSE(ax45mp_l2c_configured);
> > +
> > +/* PMA setup */
> > +static long ax45mp_sbi_set_pma(unsigned long start,
> > +                              unsigned long size,
> > +                              unsigned long flags,
> > +                              unsigned int entry_id)
> > +{
> > +       struct sbiret ret;
> > +
> > +       ret = sbi_ecall(SBI_EXT_ANDES, AX45MP_SBI_EXT_SET_PMA,
> > +                       start, start + size, size, entry_id,
> > +                       flags, 0);
>
> Fits on two lines.
>
OK.

On a second look the "start + size" arg can be dropped as this can be
calculated in OpenSBI, so I'll fix it in OpenSBI and here too.

> > +
> > +       return ret.value;
> > +}
> > +
> > +static int ax45mp_configure_pma_regions(struct device_node *np)
> > +{
> > +       const char *propname = "andestech,pma-regions";
> > +       u64 start, size, flags;
> > +       unsigned int entry_id;
> > +       unsigned int i;
> > +       int count;
> > +       int ret;
> > +
> > +       count = of_property_count_elems_of_size(np, propname,
> > +                                               sizeof(u32) * 6);
>
> Fits on a single line.
>
OK.

> > +static inline uint32_t ax45mp_cpu_l2c_get_cctl_status(void)
> > +{
> > +       return readl((void *)(ax45mp_priv->l2c_base + AX45MP_L2C_REG_STATUS_OFFSET));
>
> Why the cast to "(void *)"?
>
Can be dropped.

> > +}
> > +
> > +static inline uint32_t ax45mp_cpu_l2c_ctl_status(void)
> > +{
> > +       return readl((void *)(ax45mp_priv->l2c_base + AX45MP_L2C_REG_CTL_OFFSET));
>
> Likewise.
>
Ditto.

> > +}
>
> > +static void ax45mp_cpu_dcache_wb_range(unsigned long start,
> > +                                      unsigned long end,
> > +                                      int line_size)
> > +{
> > +       void __iomem *base = ax45mp_priv->l2c_base;
> > +       unsigned long pa;
> > +       int mhartid = 0;
> > +#ifdef CONFIG_SMP
> > +       mhartid = smp_processor_id();
> > +#endif
> > +
> > +       while (end > start) {
> > +               if (ax45mp_priv->ucctl_ok) {
> > +                       csr_write(AX45MP_CCTL_REG_UCCTLBEGINADDR_NUM, start);
> > +                       csr_write(AX45MP_CCTL_REG_UCCTLCOMMAND_NUM, AX45MP_CCTL_L1D_VA_WB);
> > +               }
> > +
> > +               if (ax45mp_priv->l2cache_enabled) {
> > +                       pa = virt_to_phys((void *)start);
>
> Looks like start and end should be "void *" instead of " unsigned long",
> as they are virtual addresses. See also below...
>
OK.

> > +                       writel(pa, (void *)(base + AX45MP_L2C_REG_CN_ACC_OFFSET(mhartid)));
> > +                       writel(AX45MP_CCTL_L2_PA_WB,
> > +                              (void *)(base + AX45MP_L2C_REG_CN_CMD_OFFSET(mhartid)));
>
> Why the casts to "(void *)"?
>
Can be dropped.

> > +                       while ((ax45mp_cpu_l2c_get_cctl_status() &
> > +                               AX45MP_CCTL_L2_STATUS_CN_MASK(mhartid)) !=
> > +                               AX45MP_CCTL_L2_STATUS_IDLE)
> > +                               ;
> > +               }
> > +
> > +               start += line_size;
> > +       }
> > +}
> > +
> > +static void ax45mp_cpu_dcache_inval_range(unsigned long start,
> > +                                         unsigned long end,
> > +                                         int line_size)
> > +{
> > +       void __iomem *base = ax45mp_priv->l2c_base;
> > +       unsigned long pa;
> > +       int mhartid = 0;
> > +#ifdef CONFIG_SMP
> > +       mhartid = smp_processor_id();
> > +#endif
> > +
> > +       while (end > start) {
> > +               if (ax45mp_priv->ucctl_ok) {
> > +                       csr_write(AX45MP_CCTL_REG_UCCTLBEGINADDR_NUM, start);
> > +                       csr_write(AX45MP_CCTL_REG_UCCTLCOMMAND_NUM, AX45MP_CCTL_L1D_VA_INVAL);
> > +               }
> > +
> > +               if (ax45mp_priv->l2cache_enabled) {
> > +                       pa = virt_to_phys((void *)start);
>
> Looks like start and end should be "void *" instead of " unsigned long",
> as they are virtual addresses. See also below...
>
OK.

> > +                       writel(pa, (void *)(base + AX45MP_L2C_REG_CN_ACC_OFFSET(mhartid)));
> > +                       writel(AX45MP_CCTL_L2_PA_INVAL,
> > +                              (void *)(base + AX45MP_L2C_REG_CN_CMD_OFFSET(mhartid)));
> > +                       while ((ax45mp_cpu_l2c_get_cctl_status() &
> > +                               AX45MP_CCTL_L2_STATUS_CN_MASK(mhartid)) !=
> > +                               AX45MP_CCTL_L2_STATUS_IDLE)
> > +                               ;
> > +               }
> > +
> > +               start += line_size;
> > +       }
> > +}
> > +
> > +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t size)
> > +{
> > +       char cache_buf[2][AX45MP_MAX_CACHE_LINE_SIZE] = { 0 };
>
> AX45MP_MAX_CACHE_LINE_SIZE = 256, so 512 bytes of data on the stack,
> auto-initialized by memset().
>
> Please remove the { 0 }, ...
>
Ok will do.

> > +       unsigned long start = (unsigned long)vaddr;
> > +       unsigned long end = start + size;
> > +       unsigned long old_start = start;
> > +       unsigned long old_end = end;
> > +       unsigned long line_size;
> > +       unsigned long flags;
> > +
> > +       if (static_branch_unlikely(&ax45mp_l2c_configured) && !ax45mp_priv)
> > +               return;
> > +
> > +       if (unlikely(start == end))
> > +               return;
> > +
> > +       line_size = ax45mp_priv->ax45mp_cache_line_size;
>
> ... and call memset() here, so the buffer is not initialized when unused.
> Perhaps use two buffers, so you can easily memset() only the part that is
> used?
>
OK.

> > +
> > +       start = start & (~(line_size - 1));
> > +       end = ((end + line_size - 1) & (~(line_size - 1)));
>
> These are the only calculations that need to use "unsigned long"
> instead of "void *", but you can use PTR_ALIGN_DOWN() and PTR_ALIGN()
> to avoid explicit casts.
>
Good point.

> > +
> > +       local_irq_save(flags);
> > +       if (unlikely(start != old_start))
> > +               memcpy(&cache_buf[0][0], (void *)start, line_size);
> > +
> > +       if (unlikely(end != old_end))
> > +               memcpy(&cache_buf[1][0], (void *)(old_end & (~(line_size - 1))), line_size);
>
> PTR_ALIGN_DOWN()
>
OK.

> > +
> > +       ax45mp_cpu_dcache_inval_range(start, end, line_size);
> > +
> > +       if (unlikely(start != old_start))
> > +               memcpy((void *)start, &cache_buf[0][0], (old_start & (line_size - 1)));
> > +
> > +       if (unlikely(end != old_end))
> > +               memcpy((void *)(old_end + 1),
> > +                      &cache_buf[1][(old_end & (line_size - 1)) + 1],
> > +                      end - old_end - 1);
> > +
> > +       local_irq_restore(flags);
> > +}
> > +EXPORT_SYMBOL(ax45mp_cpu_dma_inval_range);
> > +
> > +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t size)
> > +{
> > +       unsigned long start = (unsigned long)vaddr;
> > +       unsigned long end = start + size;
> > +       unsigned long line_size;
> > +       unsigned long flags;
> > +
> > +       if (static_branch_unlikely(&ax45mp_l2c_configured) && !ax45mp_priv)
> > +               return;
> > +
> > +       line_size = ax45mp_priv->ax45mp_cache_line_size;
> > +       local_irq_save(flags);
> > +       start = start & (~(line_size - 1));
>
> PTR_ALIGN_DOWN() etc...
>
OK.

> > +       ax45mp_cpu_dcache_wb_range(start, end, line_size);
> > +       local_irq_restore(flags);
> > +}
> > +EXPORT_SYMBOL(ax45mp_cpu_dma_wb_range);
> > +
> > +static int ax45mp_configure_l2_cache(struct device_node *np)
> > +{
> > +       u8 ram_ctl[2];
> > +       u32 cache_ctl;
> > +       u32 prefetch;
> > +       int ret;
> > +
> > +       cache_ctl = ax45mp_cpu_l2c_ctl_status();
> > +
> > +       /* Instruction and data fetch prefetch depth */
> > +       ret = of_property_read_u32(np, "andestech,inst-prefetch", &prefetch);
> > +       if (!ret) {
> > +               cache_ctl &= ~AX45MP_L2C_IPREPETCH_MSK;
> > +               cache_ctl |= (prefetch << AX45MP_L2C_IPREPETCH_OFF);
>
> FIELD_PREP(), also below
>
This entire function will be dropped now...

> > +       }
> > +
> > +       ret = of_property_read_u32(np, "andestech,data-prefetch", &prefetch);
> > +       if (!ret) {
> > +               cache_ctl &= ~AX45MP_L2C_DPREPETCH_MSK;
> > +               cache_ctl |= (prefetch << AX45MP_L2C_DPREPETCH_OFF);
>
> prefect / 2
>
> > +       }
> > +
> > +       /* tag RAM and data RAM setup and output cycle */
> > +       ret = of_property_read_u8_array(np, "andestech,tag-ram-ctl", ram_ctl, 2);
> > +       if (!ret) {
> > +               cache_ctl &= ~(AX45MP_L2C_TRAMOCTL_MSK | AX45MP_L2C_TRAMICTL_MSK);
> > +               cache_ctl |= ram_ctl[0] << AX45MP_L2C_TRAMOCTL_OFF;
> > +               cache_ctl |= ram_ctl[1] << AX45MP_L2C_TRAMICTL_OFF;
> > +       }
> > +
> > +       ret = of_property_read_u8_array(np, "andestech,data-ram-ctl", ram_ctl, 2);
> > +       if (!ret) {
> > +               cache_ctl &= ~(AX45MP_L2C_DRAMOCTL_MSK | AX45MP_L2C_DRAMICTL_MSK);
> > +               cache_ctl |= ram_ctl[0] << AX45MP_L2C_DRAMOCTL_OFF;
> > +               cache_ctl |= ram_ctl[1] << AX45MP_L2C_DRAMICTL_OFF;
> > +       }
> > +
> > +       writel(cache_ctl, ax45mp_priv->l2c_base + AX45MP_L2C_REG_CTL_OFFSET);
> > +
> > +       ret = of_property_read_u32(np, "cache-line-size", &ax45mp_priv->ax45mp_cache_line_size);
>
> According to the bindings, this must be 64?
>
Agreed, I'll add a check to make sure it's always 64.

Cheers,
Prabhakar
Prabhakar Oct. 25, 2022, 11:21 p.m. UTC | #8
Hi Heiko,

On Mon, Oct 24, 2022 at 1:04 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Prabhakar,
>
> Am Montag, 24. Oktober 2022, 13:55:00 CEST schrieb Lad, Prabhakar:
> > Hi Conor,
> >
> > On Fri, Oct 21, 2022 at 11:32 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Fri, Oct 21, 2022 at 11:05:40PM +0100, Lad, Prabhakar wrote:
> > > > Hi Rob,
> > > >
> > > > Thank you for the review.
> > > >
> > > > On Fri, Oct 21, 2022 at 3:05 AM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > On the AX45MP core, cache coherency is a specification option so it may
> > > > > > not be supported. In this case DMA will fail. As a workaround, firstly we
> > > > > > allocate a global dma coherent pool from which DMA allocations are taken
> > > > > > and marked as non-cacheable + bufferable using the PMA region as specified
> > > > > > in the device tree. Synchronization callbacks are implemented to
> > > > > > synchronize when doing DMA transactions.
> > > > > >
> > > > > > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > > > > > block that allows dynamic adjustment of memory attributes in the runtime.
> > > > > > It contains a configurable amount of PMA entries implemented as CSR
> > > > > > registers to control the attributes of memory locations in interest.
> > > > > >
> > > > > > Below are the memory attributes supported:
> > > > > > * Device, Non-bufferable
> > > > > > * Device, bufferable
> > > > > > * Memory, Non-cacheable, Non-bufferable
> > > > > > * Memory, Non-cacheable, Bufferable
> > > > > > * Memory, Write-back, No-allocate
> > > > > > * Memory, Write-back, Read-allocate
> > > > > > * Memory, Write-back, Write-allocate
> > > > > > * Memory, Write-back, Read and Write-allocate
> > > > > >
> > > > > > This patch adds support to configure the memory attributes of the memory
> > > > > > regions as passed from the l2 cache node and exposes the cache management
> > > > > > ops.
> > > > > >
> > > > > > More info about PMA (section 10.3):
> > > > > > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> > > > > >
> > > > > > This feature is based on the work posted [0] by Vincent Chen
> > > > > > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> > > > > >
> > > > > > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > >  arch/riscv/include/asm/cacheflush.h    |   8 +
> > > > > >  arch/riscv/include/asm/errata_list.h   |   2 +
> > > > > >  arch/riscv/mm/dma-noncoherent.c        |  20 ++
> > > > > >  drivers/soc/renesas/Kconfig            |   5 +
> > > > > >  drivers/soc/renesas/Makefile           |   4 +
> > > > > >  drivers/soc/renesas/rzf/Kconfig        |   6 +
> > > > > >  drivers/soc/renesas/rzf/Makefile       |   3 +
> > > > > >  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++
> > > > >
> > > > > How many cache drivers do we have around now? I've seen a few bindings
> > > > > go by. I'm guessing it is time to stop putting the drivers in the
> > > > > drivers/soc/ dumping ground.
> > > > >
> > > > The main reason this driver is not in arch/riscv is that it has vendor
> > > > specific extensions. Due to this reason it was agreed during the LPC
> > > > that vendor specific extension should be maintained by SoC vendors and
> > > > was agreed that this can go into drivers/soc/renesas folder instead.
> > >
> > > Does not in drivers/soc mean they need to go into arch/riscv?
> > I was under the impression Rob wanted them arch/riscv, sorry for the confusion.
> >
> > > The outcome of the chat at the LPC BoF was more that the cache drivers
> > > themselves should not be be routed via the arch maintainers, no?
> > >
> > Indeed.
> >
> > > >
> > > > > >  drivers/soc/renesas/rzf/ax45mp_sbi.h   |  29 ++
> > > > > >  9 files changed, 508 insertions(+)
> > > > > >  create mode 100644 drivers/soc/renesas/rzf/Kconfig
> > > > > >  create mode 100644 drivers/soc/renesas/rzf/Makefile
> > > > > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_cache.c
> > > > > >  create mode 100644 drivers/soc/renesas/rzf/ax45mp_sbi.h
> > > > > >
> > > > > > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> > > > > > index 8a5c246b0a21..40aa790be9a3 100644
> > > > > > --- a/arch/riscv/include/asm/cacheflush.h
> > > > > > +++ b/arch/riscv/include/asm/cacheflush.h
> > > > > > @@ -65,6 +65,14 @@ static inline void riscv_noncoherent_supported(void) {}
> > > > > >  #define SYS_RISCV_FLUSH_ICACHE_LOCAL 1UL
> > > > > >  #define SYS_RISCV_FLUSH_ICACHE_ALL   (SYS_RISCV_FLUSH_ICACHE_LOCAL)
> > > > > >
> > > > > > +#ifdef CONFIG_AX45MP_L2_CACHE
> > > > > > +void ax45mp_cpu_dma_inval_range(void *vaddr, size_t end);
> > > > > > +void ax45mp_cpu_dma_wb_range(void *vaddr, size_t end);
> > > > > > +
> > > > > > +#define ALT_CMO_OP(_op, _start, _size, _cachesize)   \
> > > > > > +                _op(_start, _size)
> > > > > > +#endif
> > > > > > +
> > > > > >  #include <asm-generic/cacheflush.h>
> > > > > >
> > > > > >  #endif /* _ASM_RISCV_CACHEFLUSH_H */
> > > > > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > > > > > index 19a771085781..d9cbf60c3b65 100644
> > > > > > --- a/arch/riscv/include/asm/errata_list.h
> > > > > > +++ b/arch/riscv/include/asm/errata_list.h
> > > > > > @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                           \
> > > > > >  #define ALT_THEAD_PMA(_val)
> > > > > >  #endif
> > > > > >
> > > > > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > > > > >  /*
> > > > > >   * dcache.ipa rs1 (invalidate, physical address)
> > > > > >   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> > > > > > @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                               \
> > > > > >       : "a0")
> > > > > >
> > > > > >  #endif /* __ASSEMBLY__ */
> > > > > > +#endif
> > > > > >
> > > > > >  #endif
> > > > > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> > > > > > index b0add983530a..5270acca6766 100644
> > > > > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > > > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > > > > @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> > > > > >
> > > > > >       switch (dir) {
> > > > > >       case DMA_TO_DEVICE:
> > > > > > +#ifdef CONFIG_ERRATA_THEAD_CMO
> > > > > >               ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> > > > > > +#elif CONFIG_AX45MP_L2_CACHE
> > > > > > +             ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> > > > > > +#endif
> > > > >
> > > > > How do you support more than one platform in a build?
> > > > >
> > > > Yes, that's one concern which I have mentioned in the cover letter too
> > > > (At that moment it's just a single platform). Suggestions welcome!
> > >
> > > I think I said it on one of the earlier version, but it needs to be
> > > implemented w/ runtime patching via alternatives just like the thead
> > > stuff patches in their functions.
> > >
> > I'm a bit stumped with alternatives() usage.
> >
> > Currently I am just replacing the ALT_CMO_OP() macro if
> > CONFIG_AX45MP_L2_CACHE is enabled. For AX45MP currently we have two
> > exported functions ax45mp_cpu_dma_inval_range/ax45mp_cpu_dma_wb_range.
> > If I switch to
> > ALTERNATIVE() macro usage then I'll have to use the assembly version
> > of the above two mentioned functions?
>
> The overarching goal should always be the unified-kernel-image.
> So hardware-specific compile-time #ifeefs are normally a no-no :-) .
>
> So yes, it most likely should be assembly-based, and you'll "just" need
> to introduce an ALTERNATIVE_3 macro, similar to what ALTERNAITVE_2 does.
>
> That is actually the really nice part of alternatives, that you can have as
> many variants as you like.
>
Thank you for the pointer. I'm still going through the ALTERNATIVE()
macro implementation, do you think "call  <c_function>" would be an
acceptable approach (I haven't implemented/nor tested)? Or is it that
my understanding is completely invalid?

Cheers,
Prabhakar
Geert Uytterhoeven Nov. 1, 2022, 12:42 p.m. UTC | #9
Hi Prabhakar,

On Thu, Oct 20, 2022 at 12:02 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On the AX45MP core, cache coherency is a specification option so it may
> not be supported. In this case DMA will fail. As a workaround, firstly we
> allocate a global dma coherent pool from which DMA allocations are taken
> and marked as non-cacheable + bufferable using the PMA region as specified
> in the device tree. Synchronization callbacks are implemented to
> synchronize when doing DMA transactions.
>
> The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> block that allows dynamic adjustment of memory attributes in the runtime.
> It contains a configurable amount of PMA entries implemented as CSR
> registers to control the attributes of memory locations in interest.
>
> Below are the memory attributes supported:
> * Device, Non-bufferable
> * Device, bufferable
> * Memory, Non-cacheable, Non-bufferable
> * Memory, Non-cacheable, Bufferable
> * Memory, Write-back, No-allocate
> * Memory, Write-back, Read-allocate
> * Memory, Write-back, Write-allocate
> * Memory, Write-back, Read and Write-allocate
>
> This patch adds support to configure the memory attributes of the memory
> regions as passed from the l2 cache node and exposes the cache management
> ops.
>
> More info about PMA (section 10.3):
> http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
>
> This feature is based on the work posted [0] by Vincent Chen
> <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
>
> [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>
>         switch (dir) {
>         case DMA_TO_DEVICE:
> +#ifdef CONFIG_ERRATA_THEAD_CMO
>                 ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> +#elif CONFIG_AX45MP_L2_CACHE

"#elif defined(CONFIG_AX45MP_L2_CACHE)" (everywhere)

Else it may fail with:

    error: "CONFIG_AX45MP_L2_CACHE" is not defined, evaluates to 0
[-Werror=undef]


> +               ALT_CMO_OP(ax45mp_cpu_dma_wb_range, vaddr, size, 0x0);
> +#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven Nov. 1, 2022, 1:38 p.m. UTC | #10
Hi Prabhakar,

On Thu, Oct 20, 2022 at 12:02 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On the AX45MP core, cache coherency is a specification option so it may
> not be supported. In this case DMA will fail. As a workaround, firstly we
> allocate a global dma coherent pool from which DMA allocations are taken
> and marked as non-cacheable + bufferable using the PMA region as specified
> in the device tree. Synchronization callbacks are implemented to
> synchronize when doing DMA transactions.
>
> The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> block that allows dynamic adjustment of memory attributes in the runtime.
> It contains a configurable amount of PMA entries implemented as CSR
> registers to control the attributes of memory locations in interest.
>
> Below are the memory attributes supported:
> * Device, Non-bufferable
> * Device, bufferable
> * Memory, Non-cacheable, Non-bufferable
> * Memory, Non-cacheable, Bufferable
> * Memory, Write-back, No-allocate
> * Memory, Write-back, Read-allocate
> * Memory, Write-back, Write-allocate
> * Memory, Write-back, Read and Write-allocate
>
> This patch adds support to configure the memory attributes of the memory
> regions as passed from the l2 cache node and exposes the cache management
> ops.
>
> More info about PMA (section 10.3):
> http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
>
> This feature is based on the work posted [0] by Vincent Chen
> <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
>
> [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/arch/riscv/include/asm/errata_list.h
> +++ b/arch/riscv/include/asm/errata_list.h
> @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                             \
>  #define ALT_THEAD_PMA(_val)
>  #endif
>
> +#ifdef CONFIG_ERRATA_THEAD_CMO
>  /*
>   * dcache.ipa rs1 (invalidate, physical address)
>   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                         \
>         : "a0")
>
>  #endif /* __ASSEMBLY__ */
> +#endif

FTR, the new #endif should be above the old #endif.

I noticed because after rebasing on top of commit 65e9fb081877a18c
("drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head
C9xx cores") in riscv/for-next, the build failed because the new
ALT_SBI_PMU_OVERFLOW() definition ended up inside both #endifs,
instead of between.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Prabhakar Nov. 2, 2022, 12:59 a.m. UTC | #11
Hi Geert,

On Tue, Nov 1, 2022 at 1:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 20, 2022 at 12:02 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the AX45MP core, cache coherency is a specification option so it may
> > not be supported. In this case DMA will fail. As a workaround, firstly we
> > allocate a global dma coherent pool from which DMA allocations are taken
> > and marked as non-cacheable + bufferable using the PMA region as specified
> > in the device tree. Synchronization callbacks are implemented to
> > synchronize when doing DMA transactions.
> >
> > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > block that allows dynamic adjustment of memory attributes in the runtime.
> > It contains a configurable amount of PMA entries implemented as CSR
> > registers to control the attributes of memory locations in interest.
> >
> > Below are the memory attributes supported:
> > * Device, Non-bufferable
> > * Device, bufferable
> > * Memory, Non-cacheable, Non-bufferable
> > * Memory, Non-cacheable, Bufferable
> > * Memory, Write-back, No-allocate
> > * Memory, Write-back, Read-allocate
> > * Memory, Write-back, Write-allocate
> > * Memory, Write-back, Read and Write-allocate
> >
> > This patch adds support to configure the memory attributes of the memory
> > regions as passed from the l2 cache node and exposes the cache management
> > ops.
> >
> > More info about PMA (section 10.3):
> > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> >
> > This feature is based on the work posted [0] by Vincent Chen
> > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> >
> > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/arch/riscv/include/asm/errata_list.h
> > +++ b/arch/riscv/include/asm/errata_list.h
> > @@ -89,6 +89,7 @@ asm volatile(ALTERNATIVE(                                             \
> >  #define ALT_THEAD_PMA(_val)
> >  #endif
> >
> > +#ifdef CONFIG_ERRATA_THEAD_CMO
> >  /*
> >   * dcache.ipa rs1 (invalidate, physical address)
> >   * | 31 - 25 | 24 - 20 | 19 - 15 | 14 - 12 | 11 - 7 | 6 - 0 |
> > @@ -143,5 +144,6 @@ asm volatile(ALTERNATIVE_2(                                         \
> >         : "a0")
> >
> >  #endif /* __ASSEMBLY__ */
> > +#endif
>
> FTR, the new #endif should be above the old #endif.
>
> I noticed because after rebasing on top of commit 65e9fb081877a18c
> ("drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head
> C9xx cores") in riscv/for-next, the build failed because the new
> ALT_SBI_PMU_OVERFLOW() definition ended up inside both #endifs,
> instead of between.
>
Thanks for pointing this out.

Cheers,
Prabhakar
Prabhakar Nov. 2, 2022, 1:02 a.m. UTC | #12
Hi Geert,

On Tue, Nov 1, 2022 at 12:43 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Oct 20, 2022 at 12:02 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the AX45MP core, cache coherency is a specification option so it may
> > not be supported. In this case DMA will fail. As a workaround, firstly we
> > allocate a global dma coherent pool from which DMA allocations are taken
> > and marked as non-cacheable + bufferable using the PMA region as specified
> > in the device tree. Synchronization callbacks are implemented to
> > synchronize when doing DMA transactions.
> >
> > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > block that allows dynamic adjustment of memory attributes in the runtime.
> > It contains a configurable amount of PMA entries implemented as CSR
> > registers to control the attributes of memory locations in interest.
> >
> > Below are the memory attributes supported:
> > * Device, Non-bufferable
> > * Device, bufferable
> > * Memory, Non-cacheable, Non-bufferable
> > * Memory, Non-cacheable, Bufferable
> > * Memory, Write-back, No-allocate
> > * Memory, Write-back, Read-allocate
> > * Memory, Write-back, Write-allocate
> > * Memory, Write-back, Read and Write-allocate
> >
> > This patch adds support to configure the memory attributes of the memory
> > regions as passed from the l2 cache node and exposes the cache management
> > ops.
> >
> > More info about PMA (section 10.3):
> > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> >
> > This feature is based on the work posted [0] by Vincent Chen
> > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> >
> > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/arch/riscv/mm/dma-noncoherent.c
> > +++ b/arch/riscv/mm/dma-noncoherent.c
> > @@ -24,13 +24,25 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> >
> >         switch (dir) {
> >         case DMA_TO_DEVICE:
> > +#ifdef CONFIG_ERRATA_THEAD_CMO
> >                 ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size);
> > +#elif CONFIG_AX45MP_L2_CACHE
>
> "#elif defined(CONFIG_AX45MP_L2_CACHE)" (everywhere)
>
> Else it may fail with:
>
>     error: "CONFIG_AX45MP_L2_CACHE" is not defined, evaluates to 0
> [-Werror=undef]
>
Agreed, thanks for pointing this out. Said that I plan to get rid of
these checks in the next version (only after getting around the
ALTERNATIVE() macro).

Cheers,
Prabhakar
Rob Herring (Arm) Nov. 3, 2022, 3:20 a.m. UTC | #13
On Fri, Oct 21, 2022 at 11:32:01PM +0100, Conor Dooley wrote:
> On Fri, Oct 21, 2022 at 11:05:40PM +0100, Lad, Prabhakar wrote:
> > Hi Rob,
> > 
> > Thank you for the review.
> > 
> > On Fri, Oct 21, 2022 at 3:05 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 11:02:42PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > On the AX45MP core, cache coherency is a specification option so it may
> > > > not be supported. In this case DMA will fail. As a workaround, firstly we
> > > > allocate a global dma coherent pool from which DMA allocations are taken
> > > > and marked as non-cacheable + bufferable using the PMA region as specified
> > > > in the device tree. Synchronization callbacks are implemented to
> > > > synchronize when doing DMA transactions.
> > > >
> > > > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > > > block that allows dynamic adjustment of memory attributes in the runtime.
> > > > It contains a configurable amount of PMA entries implemented as CSR
> > > > registers to control the attributes of memory locations in interest.
> > > >
> > > > Below are the memory attributes supported:
> > > > * Device, Non-bufferable
> > > > * Device, bufferable
> > > > * Memory, Non-cacheable, Non-bufferable
> > > > * Memory, Non-cacheable, Bufferable
> > > > * Memory, Write-back, No-allocate
> > > > * Memory, Write-back, Read-allocate
> > > > * Memory, Write-back, Write-allocate
> > > > * Memory, Write-back, Read and Write-allocate
> > > >
> > > > This patch adds support to configure the memory attributes of the memory
> > > > regions as passed from the l2 cache node and exposes the cache management
> > > > ops.
> > > >
> > > > More info about PMA (section 10.3):
> > > > http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> > > >
> > > > This feature is based on the work posted [0] by Vincent Chen
> > > > <vincentc@andestech.com> for the Andes AndeStart RISC-V CPU.
> > > >
> > > > [0] https://lore.kernel.org/lkml/1540982130-28248-1-git-send-email-vincentc@andestech.com/
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  arch/riscv/include/asm/cacheflush.h    |   8 +
> > > >  arch/riscv/include/asm/errata_list.h   |   2 +
> > > >  arch/riscv/mm/dma-noncoherent.c        |  20 ++
> > > >  drivers/soc/renesas/Kconfig            |   5 +
> > > >  drivers/soc/renesas/Makefile           |   4 +
> > > >  drivers/soc/renesas/rzf/Kconfig        |   6 +
> > > >  drivers/soc/renesas/rzf/Makefile       |   3 +
> > > >  drivers/soc/renesas/rzf/ax45mp_cache.c | 431 +++++++++++++++++++++++++
> > >
> > > How many cache drivers do we have around now? I've seen a few bindings
> > > go by. I'm guessing it is time to stop putting the drivers in the
> > > drivers/soc/ dumping ground.
> > >
> > The main reason this driver is not in arch/riscv is that it has vendor
> > specific extensions. Due to this reason it was agreed during the LPC
> > that vendor specific extension should be maintained by SoC vendors and
> > was agreed that this can go into drivers/soc/renesas folder instead.
> 
> Does not in drivers/soc mean they need to go into arch/riscv?
> The outcome of the chat at the LPC BoF was more that the cache drivers
> themselves should not be be routed via the arch maintainers, no?

drivers/cache/ or something is what I'm suggesting starting. The first 
thing is probably making an inventory of how many we already have.

Rob