Date:   Sat, 18 Dec 2021 13:35:28 +0900
From:   Hector Martin <marcan@...can.st>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Sven Peter <sven@...npeter.dev>, Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller

On 14/12/2021 08.42, Andy Shevchenko wrote:
> On Sunday, December 12, 2021, Hector Martin <marcan@...can.st
> <mailto:marcan@...can.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@...can.st)
Public Key: https://mrcn.st/pub

Powered by blists - more mailing lists