Hi Gabriel, On Fri, Sep 25, 2020 at 5:06 PM Gabriel L. Somlo <gsomlo@xxxxxxxxx> wrote: > On Fri, Sep 25, 2020 at 03:16:02PM +0200, Geert Uytterhoeven wrote: > > On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@xxxxxxxxxxxx> wrote: > > > + */ > > > +#define LITEX_REG_SIZE 0x4 > > > +#define LITEX_SUBREG_SIZE 0x1 > > > +#define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8) > > > + > > > +static DEFINE_SPINLOCK(csr_lock); > > > + > > > +/* > > > + * LiteX SoC Generator, depending on the configuration, > > > + * can split a single logical CSR (Control & Status Register) > > > + * into a series of consecutive physical registers. > > > + * > > > + * For example, in the configuration with 8-bit CSR Bus, > > > + * 32-bit aligned (the default one for 32-bit CPUs) a 32-bit > > > + * logical CSR will be generated as four 32-bit physical registers, > > > + * each one containing one byte of meaningful data. > > > + * > > > + * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus > > > + * > > > + * The purpose of `litex_set_reg`/`litex_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. > > > + */ > > > +void litex_set_reg(void __iomem *reg, unsigned long reg_size, > > > + unsigned long val) > > > +{ > > > + unsigned long shifted_data, shift, i; > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&csr_lock, flags); > > > + > > > + for (i = 0; i < reg_size; ++i) { > > > + shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT); > > > + shifted_data = val >> shift; > > > + > > > + writel((u32 __force)cpu_to_le32(shifted_data), reg + (LITEX_REG_SIZE * i)); > > > + } > > > + > > > + spin_unlock_irqrestore(&csr_lock, flags); > > > +} > > > +EXPORT_SYMBOL_GPL(litex_set_reg); > > > > I'm still wondering about the overhead of loops and multiple accesses, > > and the need for them (see also BenH's earlier comment). > > If e.g. the register widths change for LiteUART (currently they're > > hardcoded to one), would you still consider it using the same > > programming interface, and thus compatible with "litex,liteuart"? > > There's been talk within the LiteX dev community to standardize on a > LITEX_SUBREG_SIZE of 0x4 (i.e., using all 32 bits of a 32-bit > (LITEX_REG_SIZE) aligned MMIO location). Early 32-bit (vexriscv based) > Linux capable LiteX designs started out with only the 8 LSBits used > within a 32-bit MMIO location, but 64-bit (Rocket chip) based LiteX SoCs > use 4-byte aligned, fully populated MMIO registers (i.e., both > LITEX_SUBREG_SIZE *and* LITEX_REG_SIZE are 4). There's also been talk of > deprecating LITEX_SUBREG_SIZE == 0x1 for "linux-capable LiteX builds", > but nothing definitive yet AFAIK. That sounds like a good idea to me. Having 8-bit accesses may be worthwhile on a small microcontroller, but a full-fledge Linux system can use more and wider MMIO. > Geert: note that LiteX has wider-than-32-bit registers spread across > multiple 32-bit aligned, 8- or 32-bit wide "subregisters", so looping > and shifting will still be necessary, even with LITEX_SUBREG_SIZE 0x4. Can these be different than 64-bit (and 128-bit)? That's not unlike accessors on other 32-bit platforms. Still, no loop needed, just doing two (or four) 32-bit accesses in a row is fine (but requires using inlines instead of your current single out-of-line function). > > > --- /dev/null > > > +++ b/include/linux/litex.h > > > +void litex_set_reg(void __iomem *reg, unsigned long reg_sz, unsigned long val); > > > + > > > +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_sz); > > > > Perhaps you can add static inline litex_{read,write}{8,16,32}() wrappers, > > so drivers don't have to pass the reg_sz parameter explicitly, > > and to make it look more like accessors of other bus types? > > Seconded -- perhaps simply cut'n'paste and/or adapt from > https://github.com/litex-hub/linux/blob/litex-rocket-rebase/include/linux/litex.h#L78 > (from the 64-bit port of the LiteX linux patch set) Yes, you definitely want the 32-bit and 64-bit ports to agree ;-) Note that these are using the "old" "bwlq" convention (with "l" predating 64-bit long on 64-bit platforms) instead of the more modern explicit {8,16,32,64}, but that's a minor detail. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
