Add SPI driver support for GENI based QUP

Hi,

From: Girish Mahadevan <[email protected]>

This driver supports GENI based SPI Controller in the Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable module supporting a wide range of serial interfaces including SPI. This driver supports SPI operations using FIFO mode of transfer.

Change-Id Ib2c7feba5a2fc4015bc83319b233b4356f7d3190

Signed-off-by Girish Mahadevan <[email protected]>

Signed-off-by Karthikeyan Ramasubramanian <[email protected]>

Signed-off-by Sagar Dharia <[email protected]>

Signed-off-by Girish Mahadevan <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

Signed-off-by Dilip Kota <[email protected]>

---

drivers/spi/Kconfig +2 -0

+config SPI_QCOM_GENI
+       tristate "Qualcomm SPI controller with QUP interface"

This is the same help text as the SPI_QUP config up above. Please make it different somehow by adding GENI or something like that instead of QUP?

drivers/spi/Kconfig +5 -0

+       depends on QCOM_GENI_SE
+       help
+         This driver supports GENI serial engine based SPI controller in
+         master mode on the Qualcomm Technologies Inc.'s SoCs. If you say
+         yes to this option, support will be included for the built-in SPI

Drop "built-in"?

drivers/spi/spi-geni-qcom.c new file +8 -0

+struct spi_geni_master {
+       struct geni_se se;
+       int irq;
+       struct device *dev;
+       int rx_fifo_depth;
+       int tx_fifo_depth;
+       int tx_fifo_width;
+       int tx_wm;

All of these can be unsigned ints?

drivers/spi/spi-geni-qcom.c new file +3 -0

+       bool setup;
+       u32 cur_speed_hz;
+       int cur_word_len;

unsigned?

drivers/spi/spi-geni-qcom.c new file +5 -0

+       unsigned int tx_rem_bytes;
+       unsigned int rx_rem_bytes;
+       struct spi_transfer *cur_xfer;
+       struct completion xfer_done;
+       int oversampling;

unsigned?

+static int get_spi_clk_cfg(u32 speed_hz, struct spi_geni_master *mas,

Why a u32? And not unsigned int?

drivers/spi/spi-geni-qcom.c new file +3 -0

+       if (ret) {
+               dev_err(mas->dev, "%s: Failed(%d) to find src clk for 0x%x\n",
+                                               __func__, ret, speed_hz);

Frequency Hz printed in hex? I am not a hex computer! I hope!

+static void spi_setup_word_len(struct spi_geni_master *mas, u32 mode,

mode is only u16 in spi_device struct. Maybe it would be better to pass the spi_device struct here and then pick out the mode from there.

drivers/spi/spi-geni-qcom.c new file +14 -0

+static int setup_fifo_params(struct spi_device *spi_slv,
+                                       struct spi_master *spi)
+{
+       struct spi_geni_master *mas = spi_master_get_devdata(spi);
+       struct geni_se *se = &mas->se;
+       u16 mode = spi_slv->mode;
+       u32 loopback_cfg = readl_relaxed(se->base + SE_SPI_LOOPBACK);
+       u32 cpol = readl_relaxed(se->base + SE_SPI_CPOL);
+       u32 cpha = readl_relaxed(se->base + SE_SPI_CPHA);
+       u32 demux_sel = 0;
+       u32 demux_output_inv = 0;
+       u32 clk_sel = 0;
+       u32 m_clk_cfg = 0;
+       int ret = 0;

Don't initialize variables and then overwrite them.

drivers/spi/spi-geni-qcom.c new file +2 -0

+       demux_sel = spi_slv->chip_select;
+       mas->cur_speed_hz = spi_slv->max_speed_hz;

Why can't you use clk_get_rate() instead? Or call clk_set_rate() with the rate you want the master clk to run at and then divide that down from there?

drivers/spi/spi-geni-qcom.c new file +10 -0

+       mas->cur_word_len = spi_slv->bits_per_word;
+
+       ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, &idx, &div);
+       if (ret) {
+               dev_err(mas->dev, "Err setting clks ret(%d) for %d\n",
+                                                       ret, mas->cur_speed_hz);
+               return ret;
+       }
+
+       clk_sel |= (idx & CLK_SEL_MSK);

Just assign clk_sel instead of ORing it because it's initialized to 0 up above.

+ m_clk_cfg |= ((div << CLK_DIV_SHFT) | SER_CLK_EN);

Same story.

drivers/spi/spi-geni-qcom.c new file +4 -0

+               mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
+               mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se);
+               mas->tx_fifo_width = geni_se_get_tx_fifo_width(se);
+               geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));

Why 2? Drop extra parenthesis please.

drivers/spi/spi-geni-qcom.c new file +4 -0

+               mas->oversampling = 1;
+               /* Transmit an entire FIFO worth of data per IRQ */
+               mas->tx_wm = 1;
+               geni_se_get_wrapper_version(se, major, minor, step);

Drop extra parenthesis.

drivers/spi/spi-geni-qcom.c new file +13 -0

+       /*
+        * If CS change flag is set, then toggle the CS line in between
+        * transfers and keep CS asserted after the last transfer.
+        * Else if keep CS flag asserted in between transfers and de-assert
+        * CS after the last message.
+        */
+       if (xfer->cs_change) {
+               if (list_is_last(&xfer->transfer_list,
+                               &spi->cur_msg->transfers))
+                       m_param |= FRAGMENTATION;
+       } else {
+               if (!list_is_last(&xfer->transfer_list,
+                               &spi->cur_msg->transfers))

Combine else and if here into an else if.

drivers/spi/spi-geni-qcom.c new file +14 -0

+       mas->cur_xfer = xfer;
+       if (m_cmd & SPI_TX_ONLY) {
+               mas->tx_rem_bytes = xfer->len;
+               writel_relaxed(trans_len, se->base + SE_SPI_TX_TRANS_LEN);
+       }
+
+       if (m_cmd & SPI_RX_ONLY) {
+               writel_relaxed(trans_len, se->base + SE_SPI_RX_TRANS_LEN);
+               mas->rx_rem_bytes = xfer->len;
+       }
+       writel_relaxed(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
+       geni_se_setup_m_cmd(se, m_cmd, m_param);
+       if (m_cmd & SPI_TX_ONLY)
+               writel_relaxed(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);

This can't be combined with above m_cmd & SPI_TX_ONLY statement?

drivers/spi/spi-geni-qcom.c new file +14 -0

+static void geni_spi_handle_tx(struct spi_geni_master *mas)
+{
+       int i = 0;
+       int tx_fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
+       int max_bytes = 0;
+       const u8 *tx_buf;
+       struct geni_se *se = &mas->se;
+
+       if (!mas->cur_xfer)
+               return;
+
+       /*
+        * For non-byte aligned bits-per-word values. (e.g 9)
+        * The FIFO packing is set to 1 SPI word per FIFO word.

Decapitalize "The"

drivers/spi/spi-geni-qcom.c new file +7 -0

+        * Assumption is that each SPI word will be accomodated in
+        * ceil (bits_per_word / bits_per_byte)
+        * and the next SPI word starts at the next byte.
+        * In such cases, we can fit 1 SPI word per FIFO word so adjust the
+        * max byte that can be sent per IRQ accordingly.
+        */
+       if ((mas->tx_fifo_width % mas->cur_word_len))

Drop extra parenthesis please.

drivers/spi/spi-geni-qcom.c new file +4 -0

+               max_bytes = (mas->tx_fifo_depth - mas->tx_wm) *
+                               ((mas->cur_word_len / BITS_PER_BYTE) + 1);
+       else
+               max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * tx_fifo_width;

The above if else needs braces, or it could be written as:

  max_bytes = mas->tx_fifo_depth - mas->tx_wm;
  if (mas->tx_fifo_width % mas->cur_word_len) {
    max_bytes *= (mas->cur_word_len / BITS_PER_BYTE) + 1;
  else
    max_bytes *= tx_fifo_width;

drivers/spi/spi-geni-qcom.c new file +2 -0

+       tx_buf = mas->cur_xfer->tx_buf;
+       tx_buf += (mas->cur_xfer->len - mas->tx_rem_bytes);

Drop parenthesis.

+ max_bytes = min_t(int, mas->tx_rem_bytes, max_bytes);

Why isn't max_bytes unsigned?

drivers/spi/spi-geni-qcom.c new file +8 -0

+       while (i < max_bytes) {
+               int j;
+               u32 fifo_word = 0;
+               u8 *fifo_byte;
+               int bytes_per_fifo = tx_fifo_width;
+               int bytes_to_write = 0;
+
+               if ((mas->tx_fifo_width % mas->cur_word_len))

Drop parenthesis.

drivers/spi/spi-geni-qcom.c new file +3 -0

+                       bytes_per_fifo =
+                               (mas->cur_word_len / BITS_PER_BYTE) + 1;
+               bytes_to_write = min_t(int, max_bytes - i, bytes_per_fifo);

More things can be unsigned?

drivers/spi/spi-geni-qcom.c new file +3 -0

+               fifo_byte = (u8 *)&fifo_word;
+               for (j = 0; j < bytes_to_write; j++)
+                       fifo_byte[j] = tx_buf[i++];

Why are we doing all this work to fill in a fifo byte at a time? tx_buf should be a buffer of bytes that we can iowrite32_rep() with directly. And then we could just run iowrite32_rep() with some count of bytes to write? I suppose we may run into problems with unaligned size buffers, but it sounds like that doesn't happen?

Have you tested out non-byte aligned word size transfers?

drivers/spi/spi-geni-qcom.c new file +12 -0

+static void geni_spi_handle_rx(struct spi_geni_master *mas)
+{
+       int i = 0;
+       struct geni_se *se = &mas->se;
+       int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;
+       u32 rx_fifo_status = readl_relaxed(se->base + SE_GENI_RX_FIFO_STATUS);
+       int rx_bytes = 0;
+       int rx_wc = 0;
+       u8 *rx_buf;
+
+       if (!mas->cur_xfer)
+               return;

This is an error condition? We should return IRQ_NONE in such a case in the irq handler? Or somehow indicate this up that we actually handled an rx or not so the irqhandler can do the right thing.

drivers/spi/spi-geni-qcom.c new file +13 -0

+       /*
+        * For non-byte aligned bits-per-word values. (e.g 9)
+        * The FIFO packing is set to 1 SPI word per FIFO word.
+        * Assumption is that each SPI word will be accomodated in
+        * ceil (bits_per_word / bits_per_byte)
+        * and the next SPI word starts at the next byte.
+        */
+       if (!(mas->tx_fifo_width % mas->cur_word_len))
+               rx_bytes += rx_wc * fifo_width;
+       else
+               rx_bytes += rx_wc *
+                       ((mas->cur_word_len / BITS_PER_BYTE) + 1);
+       rx_bytes = min_t(int, mas->rx_rem_bytes, rx_bytes);

min_t is preferably avoided.

+ rx_buf += (mas->cur_xfer->len - mas->rx_rem_bytes);

Drop parenthesis.

drivers/spi/spi-geni-qcom.c new file +8 -0

+       while (i < rx_bytes) {
+               u32 fifo_word = 0;
+               u8 *fifo_byte;
+               int bytes_per_fifo = fifo_width;
+               int read_bytes = 0;
+               int j;
+
+               if ((mas->tx_fifo_width % mas->cur_word_len))

Drop parenthesis.

drivers/spi/spi-geni-qcom.c new file +12 -0

+static int spi_geni_probe(struct platform_device *pdev)
+{
+       int ret;
+       struct spi_master *spi;
+       struct spi_geni_master *spi_geni;
+       struct resource *res;
+       struct geni_se *se;
+
+       spi = spi_alloc_master(&pdev->dev, sizeof(struct spi_geni_master));
+       if (!spi) {
+               ret = -ENOMEM;
+               dev_err(&pdev->dev, "Failed to alloc spi struct\n");

We don't need allocation error messages.

drivers/spi/spi-geni-qcom.c new file +9 -0

+       platform_set_drvdata(pdev, spi);
+       spi_geni = spi_master_get_devdata(spi);
+       spi_geni->dev = &pdev->dev;
+       spi_geni->se.dev = &pdev->dev;
+       spi_geni->se.wrapper = dev_get_drvdata(pdev->dev.parent);
+       se = &spi_geni->se;
+
+       spi->bus_num = -1;
+       spi->dev.of_node = pdev->dev.of_node;

This is broken, the virtual SPI device does not exist in DT and this might break things.

drivers/spi/spi-geni-qcom.c new file +9 -0

+       spi_geni->se.clk = devm_clk_get(&pdev->dev, "se");
+       if (IS_ERR(spi_geni->se.clk)) {
+               ret = PTR_ERR(spi_geni->se.clk);
+               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
+               goto spi_geni_probe_err;
+       }
+
+       if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
+                               &spi->max_speed_hz)) {

Why does this need to come from DT?

drivers/spi/spi-geni-qcom.c new file +5 -0

+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       se->base = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(se->base)) {
+               ret = -ENOMEM;
+               dev_err(&pdev->dev, "Err IO mapping iomem\n");

We don't need these error messages. devm_ioremap_resource() already does it.

drivers/spi/spi-geni-qcom.c new file +5 -0

+       init_completion(&spi_geni->xfer_done);
+       pm_runtime_enable(&pdev->dev);
+       ret = devm_spi_register_master(&pdev->dev, spi);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed to register SPI master\n");

+ pm_runtime_put_noidle(&pdev->dev);

include/linux/spi/spi-geni-qcom.h new file +1 -0

new file mode 100644
index 0000000..dc95764
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */

Why?

include/linux/spi/spi-geni-qcom.h new file +8 -0

+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SPI_GENI_QCOM_HEADER___
+#define __SPI_GENI_QCOM_HEADER___
+
+struct spi_geni_qcom_ctrl_data {

What's the point of this header file and structure? This driver supports board files? Isn't everything DT now?

drivers/spi/spi-geni-qcom.c line 256

Quoted 4 lines

drivers/spi/spi-geni-qcom.c new file +4 -0

+               mas->oversampling = 1;
+               /* Transmit an entire FIFO worth of data per IRQ */
+               mas->tx_wm = 1;
+               geni_se_get_wrapper_version(se, major, minor, step);

Quoted 1 line · In v1 Stephen said:

Drop extra parenthesis.

I'll further add that I'd prefer it if you based your series atop
<https://patchwork.kernel.org/patch/10412417/> which changes the prototype here.

Hi Girish,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.18-rc5 next-20180720]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:
https://github.com/0day-ci/linux/commits/Dilip-Kota/spi-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0 reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree GCC_VERSION=8.1.0 make.cross ARCH=ia64

All errors (new ones prefixed by >>):

In file included from drivers/spi/spi-geni-qcom.c:11:
drivers/spi/spi-geni-qcom.c: In function
'spi_geni_prepare_transfer_hardware':

Quoted 1 line

Quoted 1 line

include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'?

     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro
'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/qcom-geni-se.h:238:9: note: each undeclared
identifier is reported only once for each function it appears in
     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro
'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

--
In file included from drivers//spi/spi-geni-qcom.c:11:
drivers//spi/spi-geni-qcom.c: In function
'spi_geni_prepare_transfer_hardware':

Quoted 1 line

Quoted 1 line

include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'?

     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro
'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/qcom-geni-se.h:238:9: note: each undeclared
identifier is reported only once for each function it appears in
     step = version & HW_VER_STEP_MASK; \
            ^~~~~~~
   drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro
'geni_se_get_wrapper_version'
      geni_se_get_wrapper_version(se, major, minor, step);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +238 include/linux/qcom-geni-se.h

eddac5af Karthikeyan Ramasubramanian 2018-03-30 231 eddac5af Karthikeyan Ramasubramanian 2018-03-30 232 #define geni_se_get_wrapper_version(se, major, minor, step) do { \

eddac5af Karthikeyan Ramasubramanian 2018-03-30  233  	u32 ver; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  234  \
eddac5af Karthikeyan Ramasubramanian 2018-03-30  235  	ver =

geni_se_get_qup_hw_version(se); \
eddac5af Karthikeyan Ramasubramanian 2018-03-30 236 major = (ver &
HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30 237 minor = (ver &
HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30 @238 step = version
& HW_VER_STEP_MASK; \
eddac5af Karthikeyan Ramasubramanian 2018-03-30 239 } while (0) eddac5af Karthikeyan Ramasubramanian 2018-03-30 240

:::::: The code at line 238 was first introduced by commit :::::: eddac5af06546d2e7a0730e3dc02dde3dc91098a soc: qcom: Add GENI based QUP Wrapper driver

:::::: TO: Karthikeyan Ramasubramanian <[email protected]> :::::: CC: Andy Gross <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

From: Karthikeyan Ramasubramanian <[email protected]>

Add device tree binding support for the QCOM GENI SE driver.

Signed-off-by Karthikeyan Ramasubramanian <[email protected]>

Signed-off-by Sagar Dharia <[email protected]>

Signed-off-by Girish Mahadevan <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

Signed-off-by Andy Gross <[email protected]>

---
 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 119 +++++++++++++++++++++
 1 file changed, 119 insertions(+)

This patch series adds SPI driver support for GENI based QUP. Also adding the device tree binding document for reference as per reviewers request which was merged in Linus tree.

Girish Mahadevan (1):
spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

 drivers/spi/Kconfig               |  12 +
 drivers/spi/Makefile              |   1 +
 drivers/spi/spi-geni-qcom.c       | 685 ++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi-geni-qcom.h |  14 +
 4 files changed, 712 insertions(+)
 create mode 100644 drivers/spi/spi-geni-qcom.c
 create mode 100644 include/linux/spi/spi-geni-qcom.h