lkml.org 
[lkml]   [2021]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drivers/soc/litex: remove 8-bit subregister option
On Fri, May 21, 2021 at 02:36:21PM -0400, Gabriel Somlo 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@gmail.com>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Joel Stanley <joel@jms.id.au>
>
> ---
> drivers/soc/litex/Kconfig | 12 -----
> include/linux/litex.h | 100 +++++++-------------------------------
> 2 files changed, 17 insertions(+), 95 deletions(-)

...

> static inline void litex_write64(void __iomem *reg, u64 val)
> {
> - _litex_set_reg(reg, sizeof(u64), val);
> + _write_litex_subregister(val >> LITEX_SUBREG_SIZE_BIT, reg);
> + _write_litex_subregister(val, reg + LITEX_SUBREG_ALIGN);
> }

I wonder if it would be more clear to remove the macros and just write as:

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 + 0x4);
}

> 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) << LITEX_SUBREG_SIZE_BIT) |
> + _read_litex_subregister(reg + LITEX_SUBREG_ALIGN);
> }

Same here.

This all looks good to me. Just a bit of style preference/question for
discussion, for me it's easier to read without the macro's but it just may be
me. The macro's make sense when they could change, but now it's just something
to double check when reading the code.

Though they are used here in the init code which we could remove too now:

pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d",
LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN);


-Stafford

> #endif /* _LINUX_LITEX_H */
> --
> 2.31.1
>

\
 
 \ /
  Last update: 2021-05-22 01:44    [W:1.531 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site