Re: [PATCH v2 2/4] litex: add common LiteX header

[Mateusz Holenko]

wt., 26 lis 2019 o 10:19 Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> napisaÅ(a):
>
> On Tue, Nov 26, 2019 at 10:02:18AM +0100, Mateusz Holenko wrote:
> > År., 20 lis 2019 o 20:26 Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> napisaÅ(a):
> > >
> > > On Wed, Oct 23, 2019 at 11:47:04AM +0200, Mateusz Holenko wrote:
> > > > +#ifdef __LITTLE_ENDIAN
> > > > +# define LITEX_READ_REG(addr)                  ioread32(addr)
> > > > +# define LITEX_READ_REG_OFF(addr, off)         ioread32(addr + off)
> > > > +# define LITEX_WRITE_REG(val, addr)            iowrite32(val, addr)
> > > > +# define LITEX_WRITE_REG_OFF(val, addr, off)   iowrite32(val, addr + off)
> > > > +#else
> > > > +# define LITEX_READ_REG(addr)                  ioread32be(addr)
> > > > +# define LITEX_READ_REG_OFF(addr, off)         ioread32be(addr + off)
> > > > +# define LITEX_WRITE_REG(val, addr)            iowrite32be(val, addr)
> > > > +# define LITEX_WRITE_REG_OFF(val, addr, off)   iowrite32be(val, addr + off)
> > > > +#endif
> > >
> > > I just noticed this.
> > >
> > > Ick, this is not good.  You will run into problems in the future with
> > > this, I can guarantee it.  What about systems where the CPU is one
> > > endian and the hardware in the other?  It will happen trust us.
> >
> > As mentioned in the previous comment, LiteX CSRs are guaranteed to be
> > always little-endian - this includes configurations with both
> > big-endian and little-endian CPUs.
>
> What enforces that guarantee?

liteuart is na IP core that comes as a part of a design generated and
configured by LiteX SoC builder
(https://github.com/enjoy-digital/litex). Current implementation of
LiteX generates systems in such a way that CSRs in peripherals are
little-endian regardless of a softcore CPU used (available options
cover both little-endian and big-endian CPU cores). Liteuart is a part
of the LiteX project and is probably not usable outside of it.

I was digging through the code and documentation to verify that the
always-little-endian-CSR behaviour is "guaranteed", but - to my
surprise - couldn't find anything. It seems to be simply an
implementation detail that might (maybe?) change in the future.

In this context your suggestion about probing things dynamically seems
to be even more relevant.

> > The aim of including the ifdef section was exactly to target situation
> > where endianness is different for CPU and devices. As such this
> > approach *should* work.
>
> "should" :)
>
> We have seen it happen all the time that some hardware team hooks this
> up backwards, no matter what the "spec" required.  So be careful here.
>
> good luck!
>
> greg k-h



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