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: <20230330204217.47666-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20230330204217.47666-6-prabhakar.mahadev-lad.rj@bp.renesas.com> <5468019d-e688-4019-882f-6f9611443408@spud>
In-Reply-To: <5468019d-e688-4019-882f-6f9611443408@spud>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Fri, 31 Mar 2023 20:17:39 +0000
Message-ID: <CA+V-a8uzk7VmYRgHgXpN9Hz3VUoHU5hN7OUaLzB1KX6m=nkBzQ@mail.gmail.com>
Subject: Re: [PATCH v7 5/6] cache: Add L2 cache management for Andes AX45MP
 RISC-V core
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        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>,
        Samuel Holland <samuel@sholland.org>,
        linux-riscv@lists.infradead.org, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org,
        Biju Das <biju.das.jz@bp.renesas.com>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Conor,

Thank you for the review.

On Fri, Mar 31, 2023 at 1:45=E2=80=AFPM Conor Dooley <conor.dooley@microchi=
p.com> wrote:
>
> On Thu, Mar 30, 2023 at 09:42:16PM +0100, Prabhakar wrote:
>
> > +STANDALONE CACHE CONTROLLER DRIVERS
>
> > +F:   include/cache
>
> This can go since the file no longer exists.
>
Agreed I will drop this.

> > +config AX45MP_L2_CACHE
> > +     bool "Andes Technology AX45MP L2 Cache controller"
> > +     depends on RISCV && RISCV_DMA_NONCOHERENT
>
> This can just be depends on RISCV_DMA_NONCOHERENT, since that's only
> defined on RISC-V.
>
Agreed.

> > +static void ax45mp_get_l2_line_size(struct platform_device *pdev)
> > +{
> > +     struct device_node *np =3D pdev->dev.of_node;
> > +     struct device *dev =3D &pdev->dev;
> > +     int ret;
> > +
> > +     ret =3D of_property_read_u32(np, "cache-line-size", &ax45mp_priv-=
>ax45mp_cache_line_size);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to get cache-line-size, defaulting t=
o 64 bytes\n");
> > +             ax45mp_priv->ax45mp_cache_line_size =3D AX45MP_CACHE_LINE=
_SIZE;
> > +     }
> > +
> > +     if (ax45mp_priv->ax45mp_cache_line_size !=3D AX45MP_CACHE_LINE_SI=
ZE) {
> > +             dev_err(dev, "Expected cache-line-size to be 64 bytes (fo=
und:%u). Defaulting to 64 bytes\n",
> > +                     ax45mp_priv->ax45mp_cache_line_size);
> > +             ax45mp_priv->ax45mp_cache_line_size =3D AX45MP_CACHE_LINE=
_SIZE;
> > +     }
>
> I forget, why are we doing this defaulting rather than falling over
> immediately if we detect the property is missing or wrong?
>
No reason as such on not failing on property not existing/Invalid. I
will bail out in an error case now.

> > +}
>
> > +static const struct riscv_cache_ops ax45mp_cmo_ops =3D {
> > +     .clean_range =3D &ax45mp_cpu_dma_wb_range,
> > +     .inv_range =3D &ax45mp_cpu_dma_inval_range,
> > +     .flush_range =3D &ax45mp_cpu_dma_flush_range,
> > +};
>
> I think it would be nice if your driver functions matched the names used
> by the ops. (and as I said on the other patch, I think the ops should
> match the cross-arch naming.
>
Agreed, will do.

> Otherwise, looks grand - although I think I was mostly happy with the
> last revision too.a
>
I know you had provided the RB for the last version ;)

Cheers,
Prabhakar