Re: [PATCH v3 2/2] RISC-V: sifive_l2_cache: Add L2 cache controller driver for SiFive SoCs

[Yash Shah]

On Tue, May 7, 2019 at 7:15 PM Andrew F. Davis <afd@xxxxxx> wrote:
>
> On 5/7/19 2:48 AM, Yash Shah wrote:
> > On Mon, May 6, 2019 at 5:48 PM Andrew F. Davis <afd@xxxxxx> wrote:
> >>
> >> On 5/6/19 6:48 AM, Yash Shah wrote:
> >>> The driver currently supports only SiFive FU540-C000 platform.
> >>>
> >>> The initial version of L2 cache controller driver includes:
> >>> - Initial configuration reporting at boot up.
> >>> - Support for ECC related functionality.
> >>>
> >>> Signed-off-by: Yash Shah <yash.shah@xxxxxxxxxx>
> >>> ---
> >>>  arch/riscv/include/asm/sifive_l2_cache.h |  16 +++
> >>>  arch/riscv/mm/Makefile                   |   1 +
> >>>  arch/riscv/mm/sifive_l2_cache.c          | 175 +++++++++++++++++++++++++++++++
> >>>  3 files changed, 192 insertions(+)
> >>>  create mode 100644 arch/riscv/include/asm/sifive_l2_cache.h
> >>>  create mode 100644 arch/riscv/mm/sifive_l2_cache.c
> >>>
> >>> diff --git a/arch/riscv/include/asm/sifive_l2_cache.h b/arch/riscv/include/asm/sifive_l2_cache.h
> >>> new file mode 100644
> >>> index 0000000..04f6748
> >>> --- /dev/null
> >>> +++ b/arch/riscv/include/asm/sifive_l2_cache.h
> >>> @@ -0,0 +1,16 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>> +/*
> >>> + * SiFive L2 Cache Controller header file
> >>> + *
> >>> + */
> >>> +
> >>> +#ifndef _ASM_RISCV_SIFIVE_L2_CACHE_H
> >>> +#define _ASM_RISCV_SIFIVE_L2_CACHE_H
> >>> +
> >>> +extern int register_sifive_l2_error_notifier(struct notifier_block *nb);
> >>> +extern int unregister_sifive_l2_error_notifier(struct notifier_block *nb);
> >>> +
> >>> +#define SIFIVE_L2_ERR_TYPE_CE 0
> >>> +#define SIFIVE_L2_ERR_TYPE_UE 1
> >>> +
> >>> +#endif /* _ASM_RISCV_SIFIVE_L2_CACHE_H */
> >>> diff --git a/arch/riscv/mm/Makefile b/arch/riscv/mm/Makefile
> >>> index eb22ab4..1523ee5 100644
> >>> --- a/arch/riscv/mm/Makefile
> >>> +++ b/arch/riscv/mm/Makefile
> >>> @@ -3,3 +3,4 @@ obj-y += fault.o
> >>>  obj-y += extable.o
> >>>  obj-y += ioremap.o
> >>>  obj-y += cacheflush.o
> >>> +obj-y += sifive_l2_cache.o
> >>> diff --git a/arch/riscv/mm/sifive_l2_cache.c b/arch/riscv/mm/sifive_l2_cache.c
> >>> new file mode 100644
> >>> index 0000000..4eb6461
> >>> --- /dev/null
> >>> +++ b/arch/riscv/mm/sifive_l2_cache.c
> >>> @@ -0,0 +1,175 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * SiFive L2 cache controller Driver
> >>> + *
> >>> + * Copyright (C) 2018-2019 SiFive, Inc.
> >>> + *
> >>> + */
> > [...]
> >>> +
> >>> +#ifdef CONFIG_DEBUG_FS
> >>> +static struct dentry *sifive_test;
> >>> +
> >>> +static ssize_t l2_write(struct file *file, const char __user *data,
> >>> +                     size_t count, loff_t *ppos)
> >>> +{
> >>> +     unsigned int val;
> >>> +
> >>> +     if (kstrtouint_from_user(data, count, 0, &val))
> >>> +             return -EINVAL;
> >>> +     if ((val >= 0 && val < 0xFF) || (val >= 0x10000 && val < 0x100FF))
> >>
> >> I'm guessing bit 16 is the enable and the lower 8 are some kind of
> >> region to enable the error? This is probably a bad interface, it looks
> >> useful for testing but doesn't provide any debugging info useful for
> >> running systems. Do you really want userspace to be able to do this?
> >
> > Bit 16 selects the type of ECC error (0=data or 1=directory error).
> > The lower 8 bits toggles (corrupt) that bit index.
> > Are you suggesting to remove this debug interface altogether or you
> > want me to improve the current interface?
> > Something like providing 2 separate debugfs files for data and
> > directory errors. And create a separate 8-bit debugfs variable to
> > select the bit index to toggle.
> >
>
> I was suggesting to remove the whole thing. I don't see it being all
> that useful, but it is up to you.

Thanks for the suggestion, but I will keep it as we do need it for our testing.

- Yash