Re: [PATCH v2] drivers/soc/litex: remove 8-bit subregister option

[Joel Stanley]

On Wed, 26 May 2021 at 10:55, Gabriel Somlo <gsomlo@xxxxxxxxx> wrote:
>
> Since upstream LiteX recommends that Linux support be limited to
> designs configured with 32-bit CSR subregisters (see commit a2b71fde
> in upstream LiteX, https://github.com/enjoy-digital/litex), remove
> the option to select 8-bit subregisters, significantly reducing the
> complexity of LiteX CSR (MMIO register) accessor methods.
>
> NOTE: for details on the underlying mechanics of LiteX CSR registers,
> see https://github.com/enjoy-digital/litex/wiki/CSR-Bus or the original
> LiteX accessors (litex/soc/software/include/hw/common.h in the upstream
> repository).
>
> Signed-off-by: Gabriel Somlo <gsomlo@xxxxxxxxx>

I like how this simplifies things.

Reviewed-by: Joel Stanley <joel@xxxxxxxxx>

With this change, is there any need to keep the litex acessors around?

There's the 64 bit case, but we don't have any 64 bit CSR registers do we?



> Cc: Stafford Horne <shorne@xxxxxxxxx>
> Cc: Florent Kermarrec <florent@xxxxxxxxxxxxxxxx>
> Cc: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> Cc: Joel Stanley <joel@xxxxxxxxx>
> ---
> Changes since v1:
>  - remove LITEX_SUBREG_* macros as suggested by Stafford Horne
>
>  drivers/soc/litex/Kconfig          |  12 ----
>  drivers/soc/litex/litex_soc_ctrl.c |   3 +-
>  include/linux/litex.h              | 103 +++++------------------------
>  3 files changed, 16 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> index e7011d665b15..e6ba3573a772 100644
> --- a/drivers/soc/litex/Kconfig
> +++ b/drivers/soc/litex/Kconfig
> @@ -17,16 +17,4 @@ config LITEX_SOC_CONTROLLER
>           All drivers that use functions from litex.h must depend on
>           LITEX.
>
> -config LITEX_SUBREG_SIZE
> -       int "Size of a LiteX CSR subregister, in bytes"
> -       depends on LITEX
> -       range 1 4
> -       default 4
> -       help
> -       LiteX MMIO registers (referred to as Configuration and Status
> -       registers, or CSRs) are spread across adjacent 8- or 32-bit
> -       subregisters, located at 32-bit aligned MMIO addresses. Use
> -       this to select the appropriate size (1 or 4 bytes) matching
> -       your particular LiteX build.
> -
>  endmenu
> diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> index c3e379a990f2..f75790091d38 100644
> --- a/drivers/soc/litex/litex_soc_ctrl.c
> +++ b/drivers/soc/litex/litex_soc_ctrl.c
> @@ -62,8 +62,7 @@ static int litex_check_csr_access(void __iomem *reg_addr)
>         /* restore original value of the SCRATCH register */
>         litex_write32(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_VALUE);
>
> -       pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d",
> -               LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN);
> +       pr_info("LiteX SoC Controller driver initialized");
>
>         return 0;
>  }
> diff --git a/include/linux/litex.h b/include/linux/litex.h
> index 5ea9ccf5cce4..f2edb86d5f44 100644
> --- a/include/linux/litex.h
> +++ b/include/linux/litex.h
> @@ -11,18 +11,6 @@
>
>  #include <linux/io.h>
>
> -/* LiteX SoCs support 8- or 32-bit CSR Bus data width (i.e., subreg. size) */
> -#if defined(CONFIG_LITEX_SUBREG_SIZE) && \
> -       (CONFIG_LITEX_SUBREG_SIZE == 1 || CONFIG_LITEX_SUBREG_SIZE == 4)
> -#define LITEX_SUBREG_SIZE      CONFIG_LITEX_SUBREG_SIZE
> -#else
> -#error LiteX subregister size (LITEX_SUBREG_SIZE) must be 4 or 1!
> -#endif
> -#define LITEX_SUBREG_SIZE_BIT   (LITEX_SUBREG_SIZE * 8)
> -
> -/* LiteX subregisters of any width are always aligned on a 4-byte boundary */
> -#define LITEX_SUBREG_ALIGN       0x4
> -
>  static inline void _write_litex_subregister(u32 val, void __iomem *addr)
>  {
>         writel((u32 __force)cpu_to_le32(val), addr);
> @@ -42,115 +30,54 @@ static inline u32 _read_litex_subregister(void __iomem *addr)
>   * 32-bit wide logical CSR will be laid out as four 32-bit physical
>   * subregisters, each one containing one byte of meaningful data.
>   *
> + * For Linux support, upstream LiteX enforces a 32-bit wide CSR bus, which
> + * means that only larger-than-32-bit CSRs will be split across multiple
> + * subregisters (e.g., a 64-bit CSR will be spread across two consecutive
> + * 32-bit subregisters).
> + *
>   * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
>   */
>
> -/* number of LiteX subregisters needed to store a register of given reg_size */
> -#define _litex_num_subregs(reg_size) \
> -       (((reg_size) - 1) / LITEX_SUBREG_SIZE + 1)
> -
> -/*
> - * since the number of 4-byte aligned subregisters required to store a single
> - * LiteX CSR (MMIO) register varies with LITEX_SUBREG_SIZE, the offset of the
> - * next adjacent LiteX CSR register w.r.t. the offset of the current one also
> - * depends on how many subregisters the latter is spread across
> - */
> -#define _next_reg_off(off, size) \
> -       ((off) + _litex_num_subregs(size) * LITEX_SUBREG_ALIGN)
> -
> -/*
> - * The purpose of `_litex_[set|get]_reg()` is to implement the logic of
> - * writing to/reading from the LiteX CSR in a single place that can be then
> - * reused by all LiteX drivers via the `litex_[write|read][8|16|32|64]()`
> - * accessors for the appropriate data width.
> - * NOTE: direct use of `_litex_[set|get]_reg()` by LiteX drivers is strongly
> - * discouraged, as they perform no error checking on the requested data width!
> - */
> -
> -/**
> - * _litex_set_reg() - Writes a value to the LiteX CSR (Control&Status Register)
> - * @reg: Address of the CSR
> - * @reg_size: The width of the CSR expressed in the number of bytes
> - * @val: Value to be written to the CSR
> - *
> - * This function splits a single (possibly multi-byte) LiteX CSR write into
> - * a series of subregister writes with a proper offset.
> - * NOTE: caller is responsible for ensuring (0 < reg_size <= sizeof(u64)).
> - */
> -static inline void _litex_set_reg(void __iomem *reg, size_t reg_size, u64 val)
> -{
> -       u8 shift = _litex_num_subregs(reg_size) * LITEX_SUBREG_SIZE_BIT;
> -
> -       while (shift > 0) {
> -               shift -= LITEX_SUBREG_SIZE_BIT;
> -               _write_litex_subregister(val >> shift, reg);
> -               reg += LITEX_SUBREG_ALIGN;
> -       }
> -}
> -
> -/**
> - * _litex_get_reg() - Reads a value of the LiteX CSR (Control&Status Register)
> - * @reg: Address of the CSR
> - * @reg_size: The width of the CSR expressed in the number of bytes
> - *
> - * Return: Value read from the CSR
> - *
> - * This function generates a series of subregister reads with a proper offset
> - * and joins their results into a single (possibly multi-byte) LiteX CSR value.
> - * NOTE: caller is responsible for ensuring (0 < reg_size <= sizeof(u64)).
> - */
> -static inline u64 _litex_get_reg(void __iomem *reg, size_t reg_size)
> -{
> -       u64 r;
> -       u8 i;
> -
> -       r = _read_litex_subregister(reg);
> -       for (i = 1; i < _litex_num_subregs(reg_size); i++) {
> -               r <<= LITEX_SUBREG_SIZE_BIT;
> -               reg += LITEX_SUBREG_ALIGN;
> -               r |= _read_litex_subregister(reg);
> -       }
> -       return r;
> -}
> -
>  static inline void litex_write8(void __iomem *reg, u8 val)
>  {
> -       _litex_set_reg(reg, sizeof(u8), val);
> +       _write_litex_subregister(val, reg);
>  }
>
>  static inline void litex_write16(void __iomem *reg, u16 val)
>  {
> -       _litex_set_reg(reg, sizeof(u16), val);
> +       _write_litex_subregister(val, reg);
>  }
>
>  static inline void litex_write32(void __iomem *reg, u32 val)
>  {
> -       _litex_set_reg(reg, sizeof(u32), val);
> +       _write_litex_subregister(val, reg);
>  }
>
>  static inline void litex_write64(void __iomem *reg, u64 val)
>  {
> -       _litex_set_reg(reg, sizeof(u64), val);
> +       _write_litex_subregister(val >> 32, reg);
> +       _write_litex_subregister(val, reg + 4);
>  }
>
>  static inline u8 litex_read8(void __iomem *reg)
>  {
> -       return _litex_get_reg(reg, sizeof(u8));
> +       return _read_litex_subregister(reg);
>  }
>
>  static inline u16 litex_read16(void __iomem *reg)
>  {
> -       return _litex_get_reg(reg, sizeof(u16));
> +       return _read_litex_subregister(reg);
>  }
>
>  static inline u32 litex_read32(void __iomem *reg)
>  {
> -       return _litex_get_reg(reg, sizeof(u32));
> +       return _read_litex_subregister(reg);
>  }
>
>  static inline u64 litex_read64(void __iomem *reg)
>  {
> -       return _litex_get_reg(reg, sizeof(u64));
> +       return ((u64)_read_litex_subregister(reg) << 32) |
> +               _read_litex_subregister(reg + 4);
>  }
>
>  #endif /* _LINUX_LITEX_H */
> --
> 2.31.1
>