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

[Rob Herring]

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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> +M:	Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> +S:	Maintained
> +F:	include/linux/litex.h
> +
>  LIVE PATCHING
>  M:	Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
>  M:	Jiri Kosina <jikos@xxxxxxxxxx>
> 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.

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).

> +#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.

> +
> +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