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

Conor Dooley
2023-01-07T00:09:54Z

[-- Attachment #1: Type: text/plain, Size: 4977 bytes --]

On Fri, Jan 06, 2023 at 06:55:25PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> 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@bp.renesas.com>

> 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?

> +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.

> +
> + 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.

With those two, I'm happy with this I guess..
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> + 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@bp.renesas.com>");
> +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.

If you resurrect the directory level maintainers entry from my v5.1, you
can also add:
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]