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

[Gabriel L. Somlo]

On Sat, May 22, 2021 at 08:44:35AM +0900, Stafford Horne wrote:
> 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@xxxxxxxxx>
> > Cc: Stafford Horne <shorne@xxxxxxxxx>
> > Cc: Florent Kermarrec <florent@xxxxxxxxxxxxxxxx>
> > Cc: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> > Cc: Joel Stanley <joel@xxxxxxxxx>
> > 
> > ---
> >  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);

Since nobody else seems to have any strong feelings on the topic, I'll
just send out a v2 with the changes suggested above in a few minutes.

Thanks,
--Gabriel