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