Re: [PATCH v6 5/6] cache: Add L2 cache management for Andes AX45MP RISC-V core

[Lad, Prabhakar]

Hi Conor,

Thank you for the review.

On Sat, Jan 7, 2023 at 12:09 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Fri, Jan 06, 2023 at 06:55:25PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > I/O Coherence Port (IOCP) provides an AXI interface for connecting
> > external non-caching masters, such as DMA controllers. The accesses
> > from IOCP are coherent with D-Caches and L2 Cache.
> >
> > IOCP is a specification option and is disabled on the Renesas RZ/Five
> > SoC due to this reason IP blocks using DMA will fail.
> >
> > 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
> >
> > More info about PMA (section 10.3):
> > Link: http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> >
> > As a workaround for SoCs with IOCP disabled CMO needs to be handled by
> > software. Firstly OpenSBI configures the memory region as
> > "Memory, Non-cacheable, Bufferable" and passes this region as a global
> > shared dma pool as a DT node. With DMA_GLOBAL_POOL enabled all DMA
> > allocations happen from this region and synchronization callbacks are
> > implemented to synchronize when doing DMA transactions.
> >
> > Example PMA region passes as a DT node from OpenSBI:
> >     reserved-memory {
> >         #address-cells = <2>;
> >         #size-cells = <2>;
> >         ranges;
> >
> >         pma_resv0@58000000 {
> >             compatible = "shared-dma-pool";
> >             reg = <0x0 0x58000000 0x0 0x08000000>;
> >             no-map;
> >             linux,dma-default;
> >         };
> >     };
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> > diff --git a/drivers/cache/ax45mp_cache.c b/drivers/cache/ax45mp_cache.c
> > new file mode 100644
> > index 000000000000..556e6875627c
> > --- /dev/null
> > +++ b/drivers/cache/ax45mp_cache.c
> > @@ -0,0 +1,279 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * non-coherent cache functions for Andes AX45MP
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corp.
> > + */
> > +
> > +#include <asm/dma-noncoherent.h>
> > +#include <linux/cacheflush.h>
> > +#include <linux/cacheinfo.h>
> > +#include <linux/dma-direction.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +
> > +#include <asm/cacheflush.h>
> > +#include <asm/sbi.h>
>
> You don't actually need this anymore, do you?
>
Agreed these can be dropped now.

> > +static int ax45mp_l2c_probe(struct platform_device *pdev)
> > +{
> > +     struct riscv_cache_ops ax45mp_cmo_ops;
> > +
> > +     /*
> > +      * riscv_cbom_block_size is set very much earlier so we can
> > +      * definitely rely on it and only if its being set we continue
> > +      * further in the probe path.
> > +      */
> > +     if (!riscv_cbom_block_size)
> > +             return 0;
>
> Return 0? That's because we may actually have the IOCP & do not want to
> install ops, right?
> If so, please add that to the comment.
>
OK, I will add that.

> > +
> > +     ax45mp_priv = devm_kzalloc(&pdev->dev, sizeof(*ax45mp_priv), GFP_KERNEL);
> > +     if (!ax45mp_priv)
> > +             return -ENOMEM;
> > +
> > +     ax45mp_priv->l2c_base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(ax45mp_priv->l2c_base))
> > +             return PTR_ERR(ax45mp_priv->l2c_base);
> > +
> > +     ax45mp_get_l2_line_size(pdev);
> > +
> > +     memset(&ax45mp_cmo_ops, 0x0, sizeof(ax45mp_cmo_ops));
> > +     ax45mp_cmo_ops.riscv_dma_noncoherent_cmo_ops = &ax45mp_no_iocp_cmo;
>
> Yah, drop this dance and use a static struct foo_ops construct please.
>
Will do.

> With those two, I'm happy with this I guess..
> Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>
> > +     riscv_noncoherent_register_cache_ops(&ax45mp_cmo_ops);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id ax45mp_cache_ids[] = {
> > +     { .compatible = "andestech,ax45mp-cache" },
> > +     { /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver ax45mp_l2c_driver = {
> > +     .driver = {
> > +             .name = "ax45mp-l2c",
> > +             .of_match_table = ax45mp_cache_ids,
> > +     },
> > +     .probe = ax45mp_l2c_probe,
> > +};
> > +
> > +static int __init ax45mp_cache_init(void)
> > +{
> > +     return platform_driver_register(&ax45mp_l2c_driver);
> > +}
> > +arch_initcall(ax45mp_cache_init);
> > +
> > +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Andes AX45MP L2 cache driver");
> > +MODULE_LICENSE("GPL");
>
> BTW, I think these are surplus-to-requirements since this is never going
> to be built as a module.
>
Agreed, I will drop it.

> If you resurrect the directory level maintainers entry from my v5.1, you
> can also add:
Sure I'll add it in the next version..

> Acked-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>
... and also include your ACK.

Cheers,
Prabhakar