Re: [PATCH v9 6/9] i3c: master: Add driver for Cadence IP

[Arnd Bergmann]

On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon
<boris.brezillon@xxxxxxxxxxx> wrote:
>
> On Thu, 25 Oct 2018 17:30:26 +0200
> Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> > On 10/24/18, Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:
> > > Hi Arnd,
> > >
> > > On Mon, 22 Oct 2018 15:34:01 +0200
> > > Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:
> > >
> > >
> > >> +
> > >> +static void cdns_i3c_master_rd_from_rx_fifo(struct cdns_i3c_master
> > >> *master,
> > >> +                                      u8 *bytes, int nbytes)
> > >> +{
> > >> +  readsl(master->regs + RX_FIFO, bytes, nbytes / 4);
> > >
> > > Vitor reported a problem with readsl(): this function expects the 2nd
> > > argument to be aligned on 32-bit, which is not guaranteed here. Unless
> > > you see a better solution, I'll switch back to a loop doing:
> > >
> > >     for (i = 0; i < nbytes; i += 4) {
> > >             u32 tmp = __raw_readl(...);
> > >             memcpy(bytes + i, &tmp,
> > >                    nbytes - i  > 4 ? 4 : nbytes - i);
> > >     }
> >
> > Could we maybe mandate that the buffer itself must be aligned here?
> > What would be a reason why we see an unaligned target buffer?
>
> Well, the buffers we pass to i3c_send_ccc_cmd() are not necessarily
> aligned because they're not dynamically allocated (allocated on the
> stack) and are not naturally aligned on 32-bits (either because they
> are smaller than 32bits or because the struct is declared __packed).
>
> I guess I could dynamically allocate the payload, but that requires
> going over all users of i3c_send_ccc_cmd() to patch them.

This reminds me that Wolfram mentioned in his ELC talk that the
buffers on i3c should all be DMA capable to make life easier for
i3c master drivers that want to implement DMA transfers.

If we have buffers here that are not aligned to cache lines
(or even just 32 bit words), doesn't that also mean that the
same buffers are not DMA capable either?

        Arnd