Re: [PATCH v4 3/7] riscv: errata: Add Andes alternative ports

[Lad, Prabhakar]

Hi Conor,

Thank you for the review.

On Thu, Nov 24, 2022 at 8:22 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Thu, Nov 24, 2022 at 05:22:03PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add required ports of the Alternative scheme for Andes CPU cores.
>
> You've got a lot of nice info in your cover letter that would be nice in
> the git history. Could you add some of the commentary about why the
> Andes cache needs special handling from there to this commit message
> please?
>
Sure, I'll update the commit message here.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > RFC v3 -> v4
> > * New patch
> > ---
> >  arch/riscv/Kconfig.erratas           | 22 +++++++++
> >  arch/riscv/errata/Makefile           |  1 +
> >  arch/riscv/errata/andes/Makefile     |  1 +
> >  arch/riscv/errata/andes/errata.c     | 68 ++++++++++++++++++++++++++++
> >  arch/riscv/include/asm/alternative.h |  3 ++
> >  arch/riscv/include/asm/errata_list.h |  5 ++
> >  arch/riscv/kernel/alternative.c      |  5 ++
> >  7 files changed, 105 insertions(+)
> >  create mode 100644 arch/riscv/errata/andes/Makefile
> >  create mode 100644 arch/riscv/errata/andes/errata.c
>
> > diff --git a/arch/riscv/errata/andes/errata.c b/arch/riscv/errata/andes/errata.c
> > new file mode 100644
> > index 000000000000..ec3e052ca8c7
> > --- /dev/null
> > +++ b/arch/riscv/errata/andes/errata.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Erratas to be applied for Andes CPU cores
> > + *
> > + *  Copyright (C) 2022 Renesas Electronics Corporation.
> > + *
> > + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include <asm/alternative.h>
> > +#include <asm/cacheflush.h>
> > +#include <asm/errata_list.h>
> > +#include <asm/patch.h>
> > +#include <asm/vendorid_list.h>
> > +
> > +static bool errata_probe_iocp(unsigned int stage, unsigned long arch_id, unsigned long impid)
>
> To the lay reader, what's an "iocp" when it's at home? "I/O coherency
> port"? Again, commit message would be a good place for the introduction
> of that term :)
>
Agree, I'll update that.

> > +{
> > +     if (!IS_ENABLED(CONFIG_ERRATA_ANDES_CMO))
> > +             return false;
> > +
> > +     if (arch_id != 0x8000000000008a45 || impid != 0x500)
>
> Can you #define these?
>
> > +             return false;
> > +
> > +     riscv_cbom_block_size = 1;
> > +     riscv_noncoherent_supported();
> > +
> > +     return true;
> > +}
> > +
> > +static u32 andes_errata_probe(unsigned int stage, unsigned long archid, unsigned long impid)
> > +{
> > +     u32 cpu_req_errata = 0;
> > +
>
> I read some code and when it does the opposite of what I'd expect, I
> feel inclined to add a comment. In this case, you're probing for the
> presence of the port `probe_iocp()`, but the interesting case is when
> you don't find it. You can leave it uncommented if you like, but even
> something like the below I think fits.
>
>         /*
>          * In the absence of the I/O Coherency Port, access to certain peripherals
>          * requires vendor specific DMA handling.
>          */
Makes sense, I'll include the above.

> > +     if (errata_probe_iocp(stage, archid, impid))
> > +             cpu_req_errata |= BIT(ERRATA_ANDESTECH_NO_IOCP);
> > +
> > +     return cpu_req_errata;
> > +}
> > +
> > +void __init_or_module andes_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
> > +                                           unsigned long archid, unsigned long impid,
> > +                                           unsigned int stage)
> > +{
> > +     u32 cpu_req_errata = andes_errata_probe(stage, archid, impid);
> > +     struct alt_entry *alt;
> > +     u32 tmp;
> > +
> > +     if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> > +             return;
> > +
> > +     for (alt = begin; alt < end; alt++) {
> > +             if (alt->vendor_id != ANDESTECH_VENDOR_ID)
> > +                     continue;
> > +             if (alt->errata_id >= ERRATA_ANDESTECH_NUMBER)
> > +                     continue;
> > +
> > +             tmp = (1U << alt->errata_id);
>
> Is this not BIT(alt->errata_id)?
>
Yep, I will switch to BIT().

Cheers,
Prabhakar