Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230106185526.260163-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20230106185526.260163-6-prabhakar.mahadev-lad.rj@bp.renesas.com> <Y7i4Q3CAbiCzvGS/@spud>
In-Reply-To: <Y7i4Q3CAbiCzvGS/@spud>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Sat, 7 Jan 2023 20:49:14 +0000
Message-ID: <CA+V-a8uEEDuN56roLKLiD-ryOH9khz1a5Wh=W+6WAsAYQVKNUw@mail.gmail.com>
Subject: Re: [PATCH v6 5/6] cache: Add L2 cache management for Andes AX45MP
 RISC-V core
To:     Conor Dooley <conor@kernel.org>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Conor Dooley <conor.dooley@microchip.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Heiko Stuebner <heiko@sntech.de>, Guo Ren <guoren@kernel.org>,
        Andrew Jones <ajones@ventanamicro.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:RISC-V ARCHITECTURE" <linux-riscv@lists.infradead.org>,
        devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Bart Van Assche <bvanassche@acm.org>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Dipen Patel <dipenp@nvidia.com>,
        Oded Gabbay <ogabbay@kernel.org>,
        Iwona Winiarska <iwona.winiarska@intel.com>,
        "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Jason M Bills <jason.m.bills@linux.intel.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        Dan Williams <dan.j.williams@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Conor,

Thank you for the review.

On Sat, Jan 7, 2023 at 12:09 AM Conor Dooley <conor@kernel.org> wrote:
>
> 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?
>
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@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.
>
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@microchip.com>
>
... and also include your ACK.

Cheers,
Prabhakar