Re: [PATCH v5 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Mateusz Holenko]

On Wed, Apr 29, 2020 at 5:12 AM Benjamin Herrenschmidt
<benh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sat, 2020-04-25 at 13:42 +0200, Mateusz Holenko wrote:
> > From: Pawel Czarnecki <pczarnecki@xxxxxxxxxxxxxxxxxxxxxxxx>
> >
> > This commit adds driver for the FPGA-based LiteX SoC
> > Controller from LiteX SoC builder.
>
> Sorry for jumping in late, Joel only just pointed me to this :)
>
> > + * 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;
> > +
> > +             __raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +}
> > +
> > +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> > +{
> > +     unsigned long shifted_data, shift, i;
> > +     unsigned long result = 0;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&csr_lock, flags);
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shifted_data = __raw_readl(reg + (LITEX_REG_SIZE * i));
> > +
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             result |= (shifted_data << shift);
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +
> > +     return result;
> > +}
>
> I really don't like the fact that the register sizes & sub sizes are
> #defined. As your comment explains, this makes it harder to support
> other configurations. This geometry should come from the device-tree
> instead.

This is a valid point - putting those parameters into DT would indeed allow
for more flexibility. Currently we are focusing on supporting a single LiteX
configuration (32-bit/8-bit bus) and that's why those parameters got fixed.

Adding support for other configurations however is not just changing those
two parameters (which should be fairly easy by itself), but also
handling different
registers layout for a single peripheral (in different configurations
CSRs offsets and sizes may differ).

Since this adds another layer of complexity we want to start with a simpler
version that can be extended in the future.

> Also this while thing is rather gross (and the lock will not help
> performance). Why can't CSRs be normally memory mapped always instead ?

Using a different LiteX configuration 32-bit/32-bit bus would solve
the problem -
a single LiteX CSR would map nicely to a single 32-bit memory pointer and no
loop/locks would be needed.

In the default configuration (32-bit/8-bit bus) there are gaps between bytes
(as Gabriel Somlo already explained in his mail) which need to be handled
"manually".

> Even when transporting them on a HW bus that's smaller, the HW bus
> conversion should be able to do the break-down into a multi-breat
> transfer rather than doing that in SW.
>
> Or at least have a fast-path if the register size is no larger than the
> sub size, so you can use a normal ioread32/iowrite32.

Again - this is possible, but using a non-default 32-bit/32-bit bus LiteX
configuration.

> Also I wonder ... last I played with LiteX, it would re-generate the
> register layout (including the bit layout inside registers potentially)
> rather enthousiastically, making it pretty hard to have a fixed
> register layout for use by a kernel driver. Was this addressed ?

TBH I never experienced bit layout inside a register to change by itself,
but I agree that using different bus width configurations causes CSRs
to be splitted into 4/2/1 32-bit registers (changing de facto the layout
from the SW perspective) - that's why we provide helper functions
in this file.

It is possible to have different configurations of a peripheral
in LiteX that e.g, turns features on/off - this might cause some CSRs
to shift and result in incompatibilities. There are ways in LiteX
to avoid such problems if the model is properly designed, though.

Another aspect of LiteX is that the order in which separate peripherals
(modules) are created results in a different memory map of the whole SoC.
This, however, is easily addressed by using a dynamically generated DT
and do not require the code of drivers to be altered in any way.

> Cheers,
> Ben.
>
>

Thanks for your comments!


-- 
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland