lkml.org 
[lkml]   [2021]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller
Date
On 14/12/2021 08.42, Andy Shevchenko wrote:
> On Sunday, December 12, 2021, Hector Martin <marcan@marcan.st
> <mailto:marcan@marcan.st>> wrote:
>
> This SPI controller is present in Apple SoCs such as the M1 (t8103) and
> M1 Pro/Max (t600x). It is a relatively straightforward design with two
> 16-entry FIFOs, arbitrary transfer sizes (up to 2**32 - 1) and fully
> configurable word size up to 32 bits. It supports one hardware CS line
> which can also be driven via the pinctrl/GPIO driver instead, if
> desired. TX and RX can be independently enabled.
>
> There are a surprising number of knobs for tweaking details of the
> transfer, most of which we do not use right now. Hardware CS control
> is available, but we haven't found a way to make it stay low across
> multiple logical transfers, so we just use software CS control for now.
>
>
> So, AFAIU there is no limitation to the software CS lines (you actually
> meant GPIO, right?).

No, this is software control over a single built-in CS line that is part
of the SPI peripheral. You can of course use the GPIO mechanism too,
which has no limit on the number of CS lines. That said, I don't think
Apple uses more than one CS per controller on any current machine, so we
just use internal CS.

> +struct apple_spi {
> +       void __iomem      *regs;        /* MMIO register address */
> +       struct clk        *clk;         /* bus clock */
> +       struct completion done;         /* wake-up from interrupt */
>
>
> Making it first member may save a few cycles on pointer arithmetic

The completion? The IRQ handler has to access regs more often than the
completion, so it sounds like the current order should be faster.

> +static int apple_spi_wait(struct apple_spi *spi, u32 fifo_bit, u32
> xfer_bit, int poll)
> +{
> +       int ret = 0;
> +
> +       if (poll) {
> +               u32 fifo, xfer;
> +               unsigned long timeout = jiffies +
> APPLE_SPI_TIMEOUT_MS * HZ / 1000;
> +
> +               do {
> +                       fifo = reg_read(spi, APPLE_SPI_IF_FIFO);
> +                       xfer = reg_read(spi, APPLE_SPI_IF_XFER);
> +                       if (time_after(jiffies, timeout)) {
> +                               ret = -ETIMEDOUT;
> +                               break;
> +                       }
> +               } while (!((fifo & fifo_bit) || (xfer & xfer_bit)));
>
>
> You may use read_poll_timeout() with a specific _read() function, but it
> ma be not worth doing that, just compare and choose the best.

Probably not worth it; I could have a function read both registers and
stuff it into a u64, but I think it'd end up being about the same amount
of code in the end with the extra scaffolding.

> +       case 4: {
> +               const u32 *p = *tx_ptr;
> +
> +               while (words--)
> +                       reg_write(spi, APPLE_SPI_TXDATA, *p++);
> +               break;
> +       }
> +       default:
> +               WARN_ON(1);
> +       }
> +
> +       *tx_ptr = ((u8 *)*tx_ptr) + bytes_per_word * wrote;
>
>
> Not sure if it’s good written code from endianness / alignment handling
> perspective (while it may still work), perhaps rewrite it a bit?

I'm not entirely sure what the alignment guarantees for SPI buffers are.
Some drivers use unaligned accessors (e.g. spi-uniphier.c), while others
don't (e.g. spi-xilinx.c). That makes me think they're aligned in the
general case (and they usually would be if drivers intend to use them in
16-bit or 32-bit mode; hopefully they're allocated as arrays of those
units in that case).

spi.h has this to say:

* In-memory data values are always in native CPU byte order, translated
* from the wire byte order (big-endian except with SPI_LSB_FIRST). So
* for example when bits_per_word is sixteen, buffers are 2N bytes long
* (@len = 2N) and hold N sixteen bit words in CPU byte order.

So endianness should be correct, at least for power of two word sizes. I
also believe it should work for non-POT word sizes, and assuming packing
doesn't change in SPI_LSB_FIRST, also for that case. It's kind of hard
to properly validate this without a real device that uses these modes,
so I think at this point we can just go with the current logic and if we
run into a problem in the future, we can fix it then :)

> +       ctlr = spi_alloc_master(&pdev->dev, sizeof(struct apple_spi));
> +       if (!ctlr) {
> +               dev_err(&pdev->dev, "out of memory\n");
> +               return -ENOMEM;
>
>
> It’s fine to use dev_err_probe() here as well

Yeah, I switched everything to dev_err_probe()

> +       pm_runtime_set_active(&pdev->dev);
> +       pm_runtime_enable(&pdev->dev);
> +
> +       pdev->dev.dma_mask = NULL;
>
>
> Why do you need this? It won’t work anyway in SPI case.

This was cargo-culted from spi-sifive and I noticed it actually causes a
warning to be printed on rebinding the driver to the same device;
already got rid of it :)

--
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

\
 
 \ /
  Last update: 2021-12-18 05:36    [W:1.822 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site