Re: [PATCH v2 2/4] litex: add common LiteX header

[Mateusz Holenko <mholenko@antmicro.com>]

sob., 26 paź 2019 o 02:03 Rob Herring <robh@kernel.org> napisał(a):
>
> On Wed, Oct 23, 2019 at 11:47:04AM +0200, Mateusz Holenko wrote:
> > It provides helper CSR access functions used by all
> > LiteX drivers.
> >
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > ---
> > This commit has been introduced in v2 of the patchset.
> >
> >  MAINTAINERS           |  6 +++++
> >  include/linux/litex.h | 59 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >  create mode 100644 include/linux/litex.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 296de2b51c83..eaa51209bfb2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9493,6 +9493,12 @@ F:     Documentation/misc-devices/lis3lv02d.rst
> >  F:   drivers/misc/lis3lv02d/
> >  F:   drivers/platform/x86/hp_accel.c
> >
> > +LITEX PLATFORM
> > +M:   Karol Gugala <kgugala@antmicro.com>
> > +M:   Mateusz Holenko <mholenko@antmicro.com>
> > +S:   Maintained
> > +F:   include/linux/litex.h
> > +
> >  LIVE PATCHING
> >  M:   Josh Poimboeuf <jpoimboe@redhat.com>
> >  M:   Jiri Kosina <jikos@kernel.org>
> > diff --git a/include/linux/litex.h b/include/linux/litex.h
> > new file mode 100644
> > index 000000000000..e793d2d7c881
> > --- /dev/null
> > +++ b/include/linux/litex.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Common LiteX header providing
> > + * helper functions for accessing CSRs.
> > + *
> > + * Copyright (C) 2019 Antmicro <www.antmicro.com>
> > + */
> > +
> > +#ifndef _LINUX_LITEX_H
> > +#define _LINUX_LITEX_H
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/compiler_types.h>
> > +
> > +#define LITEX_REG_SIZE             0x4
> > +#define LITEX_SUBREG_SIZE          0x1
> > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > +
> > +#ifdef __LITTLE_ENDIAN
> > +# define LITEX_READ_REG(addr)                  ioread32(addr)
> > +# define LITEX_READ_REG_OFF(addr, off)         ioread32(addr + off)
> > +# define LITEX_WRITE_REG(val, addr)            iowrite32(val, addr)
> > +# define LITEX_WRITE_REG_OFF(val, addr, off)   iowrite32(val, addr + off)
> > +#else
> > +# define LITEX_READ_REG(addr)                  ioread32be(addr)
> > +# define LITEX_READ_REG_OFF(addr, off)         ioread32be(addr + off)
> > +# define LITEX_WRITE_REG(val, addr)            iowrite32be(val, addr)
> > +# define LITEX_WRITE_REG_OFF(val, addr, off)   iowrite32be(val, addr + off)
>
> Defining custom accessors makes it harder for others to understand
> the code. The __raw_readl/writel accessors are native endian, so use
> those. One difference though is they don't have a memory barrier, but
> based on the below functions, you may want to do your own barrier
> anyways. And if DMA is not involved you don't need the barriers either.

LiteX CSRs are always little endian (even when combined with a big endian CPU),
so using just __raw_readl/writel won't work in all cases. This is the reason why
we proposed a custom accessors defined based on host CPU endianness.
Would adding a comment explaining this be enough or do you have other ideas?

> The _OFF variants don't add anything. LITEX_READ_REG(addr + off) is just
> as easy to read if not easier than LITEX_READ_REG_OFF(addr, off).

I agree, LITEX_READ_REG/LITEX_WRITE_REG is enough.

> > +#endif
> > +
> > +/* Helper functions for manipulating LiteX registers */
> > +
> > +static inline void litex_set_reg(void __iomem *reg, u32 reg_size, u32 val)
> > +{
> > +     u32 shifted_data, shift, i;
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             shifted_data = val >> shift;
> > +             LITEX_WRITE_REG(shifted_data, reg + (LITEX_REG_SIZE * i));
> > +     }
> > +}
>
> The problem with this is it hides whether you need to do any locking.
> Normally, you would assume a register access is atomic when it's not
> here. You could add locking in this function, but then you're doing
> locking even when not needed.
>
> It doesn't look like you actually use this in your series, so maybe
> remove until you do.

That's right. I added those functions in advance, having in mind
further drivers,
but maybe it will be better to drop them for now and re-introduce later.

> > +
> > +static inline u32 litex_get_reg(void __iomem *reg, u32 reg_size)
> > +{
> > +     u32 shifted_data, shift, i;
> > +     u32 result = 0;
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shifted_data = LITEX_READ_REG(reg + (LITEX_REG_SIZE * i));
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             result |= (shifted_data << shift);
> > +     }
> > +
> > +     return result;
> > +}
> > +
> > +#endif /* _LINUX_LITEX_H */
> > --
> > 2.23.0



--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland