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

On Thu, 25 Oct 2018 18:13:51 +0200
Arnd Bergmann [off-list ref] wrote:

On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon
[off-list ref] wrote:

quoted

On Thu, 25 Oct 2018 17:30:26 +0200
Arnd Bergmann [off-list ref] wrote:
 

quoted

On 10/24/18, Boris Brezillon [off-list ref] wrote:  

quoted

Hi Arnd,

On Mon, 22 Oct 2018 15:34:01 +0200
Boris Brezillon [off-list ref] wrote:

 

quoted

+
+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.

And this is the case for all buffers passed to
i3c_device_do_priv_xfers() (and soon i3c_device_send_hdr_cmd()),
but I did not enforce that for the internal
i3c_master_send_ccc_cmd_locked() helper, maybe I should...
It was just convenient to place the object to be transmitted/received on
the stack.

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?

Yep, if it's not cache-line-aligned (and on the stack), it's not
DMA-able.