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
