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

[Yash Shah]

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.

- Yash

>
> Andrew
>

-- 
The information transmitted is intended only for the person or entity to 
which it is addressed and may contain confidential and/or privileged 
material. If you are not the intended recipient of this message please do 
not read, copy, use or disclose this communication and notify the sender 
immediately. It should be noted that any review, retransmission, 
dissemination or other use of, or taking action or reliance upon, this 
information by persons or entities other than the intended recipient is 
prohibited.