spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

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.

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

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

---
Addressing all the reviewer commets given in Patchset1. Summerizing all the comments below:

MAKEFILE: Arrange SPI-GENI driver in alphabetical order
Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE
Enable SPI core auto runtime pm, and remove runtime pm calls.
Remove spi_geni_unprepare_message(), spi_geni_unprepare_transfer_hardware()
Remove likely/unlikely keywords.
Remove get_spi_master() and use dev_get_drvdata()
Move request_irq to probe()
Mark bus number assignment to -1 as SPI core framework will assign dynamically
Use devm_spi_register_master()
Include platform_device.h instead of of_platform.h
Removing macros which are used only once:
#define SPI_NUM_CHIPSELECT 4
#define SPI_XFER_TIMEOUT_MS 250
Place Register field definitions next to respective Register definitions.
Replace int and u32 declerations to unsigned int.
Remove Hex numbers in debug prints.
Declare mode as u16 in spi_setup_word_len()
Remove the labels: setup_fifo_params_exit: exit_prepare_transfer_hardware:
Declaring struct spi_master as spi everywhere in the file.
Calling spi_finalize_current_transfer() for end of transfer.
Hard code the SPI controller max frequency instead of reading from DTSI node.
Spinlock not required, removed it.
Removed unrequired error prints.
Fix KASAN error in geni_spi_isr().
Remove spi-geni-qcom.h
Remove inter words delay and CS to Clock toggle delay logic in the driver, as of now no clients are using it. Will submit this logic in the next patchset.
Use major, minor and step macros to read from hardware version register.

 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -

 drivers/spi/Kconfig                                |  12 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-geni-qcom.c                        | 678 +++++++++++++++++++++
 4 files changed, 691 insertions(+), 2 deletions(-)
 create mode 100644 drivers/spi/spi-geni-qcom.c

Quoted 49 lines · Dilip Kota

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.

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

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

---
Addressing all the reviewer commets given in Patchset1. Summerizing all the comments below:

MAKEFILE: Arrange SPI-GENI driver in alphabetical order
Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE Enable SPI core auto runtime pm, and remove runtime pm calls. Remove spi_geni_unprepare_message(),
spi_geni_unprepare_transfer_hardware()
Remove likely/unlikely keywords.
Remove get_spi_master() and use dev_get_drvdata()
Move request_irq to probe()
Mark bus number assignment to -1 as SPI core framework will assign
dynamically
Use devm_spi_register_master()
Include platform_device.h instead of of_platform.h Removing macros which are used only once:
#define SPI_NUM_CHIPSELECT 4
#define SPI_XFER_TIMEOUT_MS 250
Place Register field definitions next to respective Register
definitions.
Replace int and u32 declerations to unsigned int. Remove Hex numbers in debug prints.
Declare mode as u16 in spi_setup_word_len()
Remove the labels: setup_fifo_params_exit:
exit_prepare_transfer_hardware:
Declaring struct spi_master as spi everywhere in the file. Calling spi_finalize_current_transfer() for end of transfer. Hard code the SPI controller max frequency instead of reading from
DTSI node.
Spinlock not required, removed it.
Removed unrequired error prints.
Fix KASAN error in geni_spi_isr().
Remove spi-geni-qcom.h
Remove inter words delay and CS to Clock toggle delay logic in the
driver, as of now no clients are using it.
Will submit this logic in the next patchset.
Use major, minor and step macros to read from hardware version
register.

Quoted 1 line

 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -

Please split to a separate patch and explain why you are removing spi-max-frequency?

Quoted 7 lines

 drivers/spi/Kconfig                                |  12 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-geni-qcom.c                        | 678 
+++++++++++++++++++++
 4 files changed, 691 insertions(+), 2 deletions(-)
 create mode 100644 drivers/spi/spi-geni-qcom.c

Quoted 53 lines · Rob Herring

Quoted 52 lines · Dilip Kota

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.

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

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

---
Addressing all the reviewer commets given in Patchset1. Summerizing all the comments below:

MAKEFILE: Arrange SPI-GENI driver in alphabetical order
Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE Enable SPI core auto runtime pm, and remove runtime pm calls. Remove spi_geni_unprepare_message(),
spi_geni_unprepare_transfer_hardware()
Remove likely/unlikely keywords.
Remove get_spi_master() and use dev_get_drvdata()
Move request_irq to probe()
Mark bus number assignment to -1 as SPI core framework will assign
dynamically
Use devm_spi_register_master()
Include platform_device.h instead of of_platform.h Removing macros which are used only once:
#define SPI_NUM_CHIPSELECT 4
#define SPI_XFER_TIMEOUT_MS 250
Place Register field definitions next to respective Register
definitions.
Replace int and u32 declerations to unsigned int. Remove Hex numbers in debug prints.
Declare mode as u16 in spi_setup_word_len()
Remove the labels: setup_fifo_params_exit:
exit_prepare_transfer_hardware:
Declaring struct spi_master as spi everywhere in the file. Calling spi_finalize_current_transfer() for end of transfer. Hard code the SPI controller max frequency instead of reading from
DTSI node.
Spinlock not required, removed it.
Removed unrequired error prints.
Fix KASAN error in geni_spi_isr().
Remove spi-geni-qcom.h
Remove inter words delay and CS to Clock toggle delay logic in the
driver, as of now no clients are using it.
Will submit this logic in the next patchset.
Use major, minor and step macros to read from hardware version
register.

Quoted 4 lines

Quoted 1 line

 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -

Please split to a separate patch and explain why you are removing spi-max-frequency?

Hi Rob Herring,

In this patch, added changes for Driver not to read the SPI controller Maximum frequency from the device tree. Accordingly I removed it in the device tree documentation file. As both the files need to updated so did in the same patch.

Could you please let me know the reason for making a separate patch.

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

+       return 0;
+}
+
+static const struct dev_pm_ops spi_geni_pm_ops = {
+       SET_RUNTIME_PM_OPS(spi_geni_runtime_suspend,
+                                       spi_geni_runtime_resume, NULL)
+       SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, NULL)
+};
+

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.

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

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

---
Addressing all the reviewer commets given in Patchset1. Summerizing all the comments below:

MAKEFILE: Arrange SPI-GENI driver in alphabetical order
Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE
Enable SPI core auto runtime pm, and remove runtime pm calls.
Remove spi_geni_unprepare_message(), spi_geni_unprepare_transfer_hardware()
Remove likely/unlikely keywords.
Remove get_spi_master() and use dev_get_drvdata()
Move request_irq to probe()
Mark bus number assignment to -1 as SPI core framework will assign dynamically
Use devm_spi_register_master()
Include platform_device.h instead of of_platform.h
Removing macros which are used only once:
#define SPI_NUM_CHIPSELECT 4
#define SPI_XFER_TIMEOUT_MS 250
Place Register field definitions next to respective Register definitions.
Replace int and u32 declerations to unsigned int.
Remove Hex numbers in debug prints.
Declare mode as u16 in spi_setup_word_len()
Remove the labels: setup_fifo_params_exit: exit_prepare_transfer_hardware:
Declaring struct spi_master as spi everywhere in the file.
Calling spi_finalize_current_transfer() for end of transfer.
Hard code the SPI controller max frequency instead of reading from DTSI node.
Spinlock not required, removed it.
Removed unrequired error prints.
Fix KASAN error in geni_spi_isr().
Remove spi-geni-qcom.h
Remove inter words delay and CS to Clock toggle delay logic in the driver, as of now no clients are using it. Will submit this logic in the next patchset.
Use major, minor and step macros to read from hardware version register.

 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -
 drivers/spi/Kconfig                                |  12 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-geni-qcom.c                        | 678 +++++++++++++++++++++
 4 files changed, 691 insertions(+), 2 deletions(-)

From there you can download patch files by using the "DOWNLOAD" link

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +11 -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;
+       unsigned int mode = spi_slv->mode;
+       unsigned int loopback_cfg = readl_relaxed(se->base + SE_SPI_LOOPBACK);
+       unsigned int cpol = readl_relaxed(se->base + SE_SPI_CPOL);
+       unsigned int cpha = readl_relaxed(se->base + SE_SPI_CPHA);
+       unsigned int demux_sel, clk_sel, m_clk_cfg, idx, div;
+       int ret = 0;

Don't initialize variables and then overwrite them.

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +21 -0

+       unsigned int demux_output_inv = 0;
+
+       loopback_cfg &= ~LOOPBACK_MSK;
+       cpol &= ~CPOL;
+       cpha &= ~CPHA;
+
+       if (mode & SPI_LOOP)
+               loopback_cfg |= LOOPBACK_ENABLE;
+
+       if (mode & SPI_CPOL)
+               cpol |= CPOL;
+
+       if (mode & SPI_CPHA)
+               cpha |= CPHA;
+
+       if (spi_slv->mode & SPI_CS_HIGH)
+               demux_output_inv = BIT(spi_slv->chip_select);
+
+       demux_sel = spi_slv->chip_select;
+       mas->cur_speed_hz = spi_slv->max_speed_hz;
+       mas->cur_word_len = spi_slv->bits_per_word;

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +19 -0

+static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
+{
+       struct spi_geni_master *mas = spi_master_get_devdata(spi);
+       struct geni_se *se = &mas->se;
+
+       if (!mas->setup) {
+               unsigned int proto = geni_se_read_proto(se);
+               unsigned int major, minor, step, ver;
+
+               if (proto != GENI_SE_SPI) {
+                       dev_err(mas->dev, "Invalid proto %d\n", proto);
+                       return -ENXIO;
+               }
+               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);
+
+               /*
+                * Hardware programming guide suggests to configure

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +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.

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +13 -0

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

Girish said it can't. ...but IMO if it really can't then please add a comment in the code so someone doesn't later go back and try to combine.

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +10 -0

+       unsigned int max_bytes;
+       const u8 *tx_buf;
+       struct geni_se *se = &mas->se;
+
+       if (!mas->cur_xfer)
+               return IRQ_NONE;
+
+       /*
+        * 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"

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +9 -0

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

+ spi->prepare_transfer_hardware = spi_geni_prepare_transfer_hardware;

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +6 -0

+static int spi_geni_remove(struct platform_device *pdev)
+{
+       struct spi_master *spi = platform_get_drvdata(pdev);
+       struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
+
+       geni_se_resources_off(&spi_geni->se);

Why would you need to call geni_se_resources_off()? Isn't it handled by pm_runtime?

+ pm_runtime_put_noidle(&pdev->dev);

I'm not sure where to comment about this, so adding it to the end:

Between v1 and v2 you totally removed all the locking. Presumably this is because you didn't want to hold the lock in handle_fifo_timeout() while waiting for the completion. IMO taking the lock out was the wrong thing to do. You should keep it, but just drop the lock before wait_for_completion_timeout() and add it back afterwards. Specifically you _don't_ want the IRQ and timeout code stomping on each other.

Hi,

Quoted 49 lines · Dilip Kota

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.

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

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

---
Addressing all the reviewer commets given in Patchset1. Summerizing all the comments below:

MAKEFILE: Arrange SPI-GENI driver in alphabetical order
Kconfig: Mark SPI_GENI driver dependent on QCOM_GENI_SE Enable SPI core auto runtime pm, and remove runtime pm calls. Remove spi_geni_unprepare_message(),
spi_geni_unprepare_transfer_hardware()
Remove likely/unlikely keywords.
Remove get_spi_master() and use dev_get_drvdata() Move request_irq to probe()
Mark bus number assignment to -1 as SPI core framework will
assign dynamically
Use devm_spi_register_master()
Include platform_device.h instead of of_platform.h Removing macros which are used only once:
#define SPI_NUM_CHIPSELECT 4
#define SPI_XFER_TIMEOUT_MS 250
Place Register field definitions next to respective Register
definitions.
Replace int and u32 declerations to unsigned int. Remove Hex numbers in debug prints.
Declare mode as u16 in spi_setup_word_len()
Remove the labels: setup_fifo_params_exit:
exit_prepare_transfer_hardware:
Declaring struct spi_master as spi everywhere in the file. Calling spi_finalize_current_transfer() for end of transfer. Hard code the SPI controller max frequency instead of reading
from DTSI node.
Spinlock not required, removed it.
Removed unrequired error prints.
Fix KASAN error in geni_spi_isr().
Remove spi-geni-qcom.h
Remove inter words delay and CS to Clock toggle delay logic in
the driver, as of now no clients are using it.
Will submit this logic in the next patchset.
Use major, minor and step macros to read from hardware version
register.

Quoted 6 lines

 .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  |   2 -
 drivers/spi/Kconfig                                |  12 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-geni-qcom.c                        | 678 
+++++++++++++++++++++
 4 files changed, 691 insertions(+), 2 deletions(-)

See below for comments. In general I've tried to post patches to address my own comments. See the series ending at
<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142/1>.
From there you can download patch files by using the "DOWNLOAD" link at the bottom. Yell if you have problems. Hopefully that's useful. I expect that you can squash many of these into your patch to give you a leg up on v3.

NOTE: I won't promise that I made no mistakes on my fixup patches nor that I caught everything or did everything right. I'll plan to take a fresh look at the whole patch when I see your v3.

Quoted 8 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +0 -1

@@ -60,7 +60,6 @@ Required properties:
 - interrupts:          Must contain SPI controller interrupts.
 - clock-names:         Must contain "se".
 - clocks:              Serial engine core clock needed by the device.
-- spi-max-frequency:   Specifies maximum SPI clock frequency, units - 

Hz.

As per Rob's feedback, please split the device tree change into a separate patch and justify it. Perhaps the commit message could be something like:

---

dt-bindings: spi: Remove spi-max-frequency from qcom,geni-se controller

No other SPI controllers have a "spi-max-frequency" at the controller level. The normal "spi-max-frequency" property is something that is used when defining the nodes for SPI slaves. While it is possible that someone might want to define a controller-level max frequency it should be done in other ways (perhaps by keying off a compatible string?)

---

I think in the past Mark Brown has also requested that the bindings actually live under "Documentation/devicetree/bindings/spi/", so perhaps you should also add a patch to your series that moves this documentation there and changes the "soc/qcom/qcom,geni-se.txt" to reference that.

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +5 -0

+static irqreturn_t geni_spi_isr(int irq, void *data);
+
+struct spi_geni_master {
+       struct geni_se se;
+       unsigned int irq;

In v1 Stephen requested that many things in this struct become "unsigned int", but he didn't mean the "irq". Please change this back to an int. As you have things right now the code "if (spi_geni->irq < 0)" you have below is a no-op. :(

...oh, and as Stephen pointed out to me offline you don't even need to store irq.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201065

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +5 -0

+       struct device *dev;
+       unsigned int rx_fifo_depth;
+       unsigned int tx_fifo_depth;
+       unsigned int tx_fifo_width;
+       unsigned int tx_wm;

Anything that is effectively the contents of a hardware register should be of type 'u32'.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201066

Quoted 11 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +11 -0

+static int get_spi_clk_cfg(unsigned int speed_hz,
+                       struct spi_geni_master *mas,
+                       unsigned int *clk_idx,
+                       unsigned int *clk_div)
+{
+       unsigned long sclk_freq;
+       struct geni_se *se = &mas->se;
+       int ret;
+
+       ret = geni_se_clk_freq_match(&mas->se,
+                               (speed_hz * mas->oversampling), 

Quoted 2 lines

clk_idx,
+ &sclk_freq, true);

Pass "false" as the last parameter after picking my geni patch to make this work right. For my geni patch see
<https://lkml.kernel.org/r/[email protected]>.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064
Agree. Will mark it to False.

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+       if (ret) {
+               dev_err(mas->dev, "%s: Failed(%d) to find src clk for 

%dHz\n",
+ __func__, ret,
speed_hz);

As I said in previous reviews: printing __func__ is generally discouraged for dev_xx printouts. They've already got the device name and that together with the string should be enough. Remove it here and elsewhere in this file.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201067

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 4 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -0

+               return ret;
+       }
+
+       *clk_div = ((sclk_freq / mas->oversampling) / speed_hz);

After changing exact to false, you'll want:

  *clk_div = DIV_ROUND_UP(sclk_freq, mas->oversampling * speed_hz);
  actual_hz = sclk_freq / (mas->oversampling * *clk_div);

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 8 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+       if (!(*clk_div)) {
+               dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d 

speed:%u\n",
+ __func__, sclk_freq, mas->oversampling,
speed_hz);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+               return -EINVAL;
+       }

After change above to use DIV_ROUND_UP() you no longer need to check for a zero clk_div.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201064

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 8 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +8 -0

+static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
+                                       unsigned int bits_per_word)
+{
+       unsigned int word_len, pack_words;
+       bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
+       struct geni_se *se = &mas->se;
+
+       word_len = readl_relaxed(se->base + SE_SPI_WORD_LEN);

Gratuitously using the __relaxed versions of readl() and writel() is discouraged in Linux because it gives very little performance gain in 99.9% of the cases and it makes the code significantly harder to reason about it. Remove it in spi-geni-qcom. If it is shown that a particular read or write is in a performance critical section then that one instance can be added back in after carefully analysis and documentation.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201068

Quoted 10 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +7 -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;
+       unsigned int mode = spi_slv->mode;
+       unsigned int loopback_cfg = readl_relaxed(se->base + 

SE_SPI_LOOPBACK);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 4 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -0

+       unsigned int cpol = readl_relaxed(se->base + SE_SPI_CPOL);
+       unsigned int cpha = readl_relaxed(se->base + SE_SPI_CPHA);
+       unsigned int demux_sel, clk_sel, m_clk_cfg, idx, div;
+       int ret = 0;

In v1 Stephen said:

Quoted 1 line

Don't initialize variables and then overwrite them.

This is in lots of places in your code. I've found and fixed them for you:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201070

Quoted 29 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +26 -0

+       unsigned int demux_output_inv = 0;
+
+       loopback_cfg &= ~LOOPBACK_MSK;
+       cpol &= ~CPOL;
+       cpha &= ~CPHA;
+
+       if (mode & SPI_LOOP)
+               loopback_cfg |= LOOPBACK_ENABLE;
+
+       if (mode & SPI_CPOL)
+               cpol |= CPOL;
+
+       if (mode & SPI_CPHA)
+               cpha |= CPHA;
+
+       if (spi_slv->mode & SPI_CS_HIGH)
+               demux_output_inv = BIT(spi_slv->chip_select);
+
+       demux_sel = spi_slv->chip_select;
+       mas->cur_speed_hz = spi_slv->max_speed_hz;
+       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);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +5 -0

+               return ret;
+       }
+
+       clk_sel = (idx & CLK_SEL_MSK);
+       m_clk_cfg = ((div << CLK_DIV_SHFT) | SER_CLK_EN);

Please avoid useless parenthesis everywhere in this file. Stephen pointed out many of these in v1 but the correct thing to do would have been to search for all instances and fix them, not just fix the ones that Stephen pointed out. I've now done it for you:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201071

Quoted 24 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +11 -0

+static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
+{
+       struct spi_geni_master *mas = spi_master_get_devdata(spi);
+       struct geni_se *se = &mas->se;
+
+       if (!mas->setup) {
+               unsigned int proto = geni_se_read_proto(se);
+               unsigned int major, minor, step, ver;
+
+               if (proto != GENI_SE_SPI) {
+                       dev_err(mas->dev, "Invalid proto %d\n", 

proto);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +12 -0

+                       return -ENXIO;
+               }
+               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);
+
+               /*
+                * Hardware programming guide suggests to configure
+                * RX FIFO RFR level to fifo_depth-2.
+                */
+               geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
+               mas->oversampling = 1;

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 6 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +6 -0

+               /* Transmit an entire FIFO worth of data per IRQ */
+               mas->tx_wm = 1;
+               ver = geni_se_get_qup_hw_version(se);
+               major = GENI_SE_VERSION_MAJOR(ver);
+               minor = GENI_SE_VERSION_MINOR(ver);
+               step = GENI_SE_VERSION_STEP(ver);

You don't use "step". Kill it.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201069

Quoted 15 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+       /*
+        * If CS change flag is set, then toggle the CS line in 

between

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+        * transfers and keep CS asserted after the last transfer.
+        * Else if keep CS flag asserted in between transfers and 

de-assert

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +9 -0

+        * 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))

Quoted 1 line · In v1 Stephen said:

Combine else and if here into an else if.

...but really I think you could just do:

/*
* Keep the CS asserted in either of the two cases:
* - It's not the last transfer (and cs_change is not set)
* - It is the last transfer (and cs_change is set)
*
* ...AKA keep it asserted whenever the "last transfer" and cs_change
* booleans agree.
*/
if (!!xfer->cs_change ==

    !!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
        m_param |= FRAGMENTATION;

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201072

Quoted 16 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +3 -0

+       if (m_cmd & SPI_TX_ONLY) {
+               mas->tx_rem_bytes = xfer->len;
+               writel_relaxed(trans_len, se->base + 

SE_SPI_TX_TRANS_LEN);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -0

+       }
+
+       if (m_cmd & SPI_RX_ONLY) {
+               writel_relaxed(trans_len, se->base + 

SE_SPI_RX_TRANS_LEN);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +6 -0

+               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);

Quoted 1 line · In v1 Stephen said:

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

In v2 I said:

Quoted 3 lines

Girish said it can't. ...but IMO if it really can't then please add a comment in the code so someone doesn't later go back and try to combine.

I don't see a comment. An example of an appropriate comment (assuming that the geni hardware really does need the services of a dark wizard) would be:

/*
* NOTE: we have to write the SE_GENI_TX_WATERMARK_REG _after_ the
* write to SE_SPI_TRANS_CFG and the call to geni_se_setup_m_cmd()
* because these two things perform a special incantation to summon a
* dark wizard and we're not allowed to set the watermark until the
* dark wizard arrives.
*/

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201073

Presumably writing to the watermark kicks off the interrupt happening so it has to be last. That would be something to say instead of talking about magic.

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -0

+static irqreturn_t geni_spi_handle_tx(struct spi_geni_master *mas)
+{
+       unsigned int i = 0;
+       unsigned int tx_fifo_width = mas->tx_fifo_width / 

BITS_PER_BYTE;

This is super confusing. You have two things with the same name "tx_fifo_width" one that's in terms of bits and one that's in terms of bytes. ...in general it's really hard to figure out the units of all your variables here. For things that it's not clear you should rename variables.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 10 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +10 -0

+       unsigned int max_bytes;
+       const u8 *tx_buf;
+       struct geni_se *se = &mas->se;
+
+       if (!mas->cur_xfer)
+               return IRQ_NONE;
+
+       /*
+        * For non-byte aligned bits-per-word values(e.g 9).
+        * The FIFO packing is set to 1 SPI word per FIFO word.

In v1 Stephen said:

Quoted 1 line

Decapitalize "The"

You added a "." at the end of the first line so I guess it's not quite as wrong as it was but it's still better grammar to just have one sentence.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201074

Quoted 11 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -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

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +5 -0

+        * max byte that can be sent per IRQ accordingly.
+        */
+       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);

Even though the math should be the same (since you should you'll never divide cur_word_len evenly), the above is clearer as:

  max_bytes *= DIV_ROUND_UP(mas->cur_word_len, BITS_PER_BYTE);

...oh, but even that is wrong. SPI framework documents that we need the nearest power of 2, so a 20-bit transfer will not take up 3 bytes but will actually take up 4.

...and of course this same assumption is replicated twice in both the tx and rx functions. I'm just going to rewrite these two functions.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201076

Quoted 11 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +10 -0

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

1;

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 9 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +9 -0

+
+               if (bytes_per_fifo < (max_bytes - i))
+                       bytes_to_write = bytes_per_fifo;
+               else
+                       bytes_to_write = max_bytes - i;
+
+               fifo_byte = (u8 *)&fifo_word;
+               for (j = 0; j < bytes_to_write; j++)
+                       fifo_byte[j] = tx_buf[i++];

In previous reviews Stephen pointed out that he's not super keen on this manual copy of bytes, but IMO it's not a huge deal and we can find a better way to do it later. ...but if someone wants to propose a patch that makes it nicer then I won't object.

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +5 -0

+static irqreturn_t geni_spi_handle_rx(struct spi_geni_master *mas)
+{
+       unsigned int i = 0;
+       struct geni_se *se = &mas->se;
+       unsigned int fifo_width = mas->tx_fifo_width / BITS_PER_BYTE;

You're assuming TX and RX FIFO widths are the same. Let's make that explicit.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201075

Quoted 11 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +9 -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));

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 3 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +3 -0

+       if (!spi) {
+               ret = -ENOMEM;
+               goto spi_geni_probe_err;

When spi_alloc_master() fails you don't want spi_master_put().

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078

Quoted 2 lines

+ spi->prepare_transfer_hardware =
spi_geni_prepare_transfer_hardware;

IMO this is wrong. You're using prepare_transfer_hardware() to do one-time init. Just do it at probe.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201079

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 6 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +5 -0

+       init_completion(&spi_geni->xfer_done);
+       pm_runtime_enable(&pdev->dev);
+       ret = devm_spi_register_master(&pdev->dev, spi);
+       if (ret)
+               goto spi_geni_probe_unmap;

In this error case I believe you need pm_runtime_disable().

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 3 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +3 -0

+       return ret;
+spi_geni_probe_unmap:
+       devm_iounmap(&pdev->dev, se->base);

The whole point of devm is that you don't need this kind of the error handling.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201078

Quoted 7 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -0

+static int spi_geni_remove(struct platform_device *pdev)
+{
+       struct spi_master *spi = platform_get_drvdata(pdev);
+       struct spi_geni_master *spi_geni = 

spi_master_get_devdata(spi);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+
+       geni_se_resources_off(&spi_geni->se);

Quoted 2 lines · In v2 I said:

Why would you need to call geni_se_resources_off()? Isn't it handled by pm_runtime?

Quoted 3 lines

+ pm_runtime_put_noidle(&pdev->dev);

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 6 lines · In v2 I said:

I'm curious: why would you have a "put" here? Won't that result in an imbalance since you don't exit probe with "get"? ...or did pm_runtime throw me for a loop again?

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +2 -0

+       pm_runtime_disable(&pdev->dev);
+       return 0;

So testing:

      cd /sys/bus/platform/drivers/geni_spi/
      echo 880000.spi > unbind
      sleep 2
      echo 880000.spi > bind

...in fact it yells a lot. To me this means that you didn't test spi_geni_remove() at all.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201077

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt +4 -0

+static int __maybe_unused spi_geni_suspend(struct device *dev)
+{
+       if (!pm_runtime_status_suspended(dev))
+               return -EBUSY;

I didn't have time to dig into Stephen Boyd's feedback, but please make sure you address it here. I agree that something seems fishy here.

---

Quoted 9 lines · In v2, I said:

I'm not sure where to comment about this, so adding it to the end:

Between v1 and v2 you totally removed all the locking. Presumably this is because you didn't want to hold the lock in handle_fifo_timeout() while waiting for the completion. IMO taking the lock out was the wrong thing to do. You should keep it, but just drop the lock before wait_for_completion_timeout() and add it back afterwards. Specifically you _don't_ want the IRQ and timeout code stomping on each other.

...but still no spinlock?

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081

---

As we've discussed offline, your current driver doesn't deal with chip select in a correct enough way to make cros_ec_spi work. I've applied and fixed up your WIP patch to fix this.

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201142

-Doug

When spi_alloc_master() fails you don't want spi_master_put().

Quoted 13 lines

Quoted 9 lines · In v2, I said:

I'm not sure where to comment about this, so adding it to the end:

Between v1 and v2 you totally removed all the locking. Presumably this is because you didn't want to hold the lock in handle_fifo_timeout() while waiting for the completion. IMO taking the lock out was the wrong thing to do. You should keep it, but just drop the lock before wait_for_completion_timeout() and add it back afterwards. Specifically you _don't_ want the IRQ and timeout code stomping on each other.

Quoted 1 line

...but still no spinlock?

I see there is no need of taking the spinlock as timeout will be handled after the calculated time as per data size and speed.
There is 99.9% less chances of interrupt during the timeout handler.

Quoted 4 lines

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081

Quoted 13 lines

Quoted 9 lines · In v2, I said:

I'm not sure where to comment about this, so adding it to the end:

Between v1 and v2 you totally removed all the locking. Presumably this is because you didn't want to hold the lock in handle_fifo_timeout() while waiting for the completion. IMO taking the lock out was the wrong thing to do. You should keep it, but just drop the lock before wait_for_completion_timeout() and add it back afterwards. Specifically you _don't_ want the IRQ and timeout code stomping on each other.

Quoted 1 line

...but still no spinlock?

I see there is no need of taking the spinlock as timeout will be handled after the calculated time as per data size and speed.
There is 99.9% less chances of interrupt during the timeout handler.

Quoted 4 lines

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081

Quoted 7 lines

I see there is no need of taking the spinlock as timeout will be handled after the calculated time as per data size and speed.
There is 99.9% less chances of interrupt during the timeout handler.

Quoted 3 lines

Quoted 1 line

Quoted 1 line

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1201081

The thing is, we want it to be 100% reliable, not 99.9% reliable. Is it somehow wrong to add the spinlock? ...or are you noticing performance problems with the spinlock there? It's just nice not to have to think about it.

Quoted 4 lines

The thing is, we want it to be 100% reliable, not 99.9% reliable. Is it somehow wrong to add the spinlock? ...or are you noticing performance problems with the spinlock there? It's just nice not to have to think about it.

As I said, timeout will be handled after the calculated time as per data size and speed. Enough time is given for interrupt, there is no chance of interrupt occurrence during the handle_fifo_timeout(). So there is no need of spinlock.

Quoted 9 lines · On Mon, Sep 10, 2018 at 09:27:09AM +0530, [email protected] wrote:

Quoted 4 lines

The thing is, we want it to be 100% reliable, not 99.9% reliable. Is it somehow wrong to add the spinlock? ...or are you noticing performance problems with the spinlock there? It's just nice not to have to think about it.

As I said, timeout will be handled after the calculated time as per data size and speed. Enough time is given for interrupt, there is no chance of interrupt occurrence during the handle_fifo_timeout(). So there is no need of spinlock.

Assuming nothing goes wrong - the system isn't under unusually heavy load for example, there's some oversight in the code, there's no impact from power management causing things to run more slowly than you were expecting, someone uses the driver on a new bit of hardware where there are extra considerations or whatever else might go wrong. Like Doug says unless we're in some performance critical situation where it's worth thinking *really* hard about how things really are actually safe even though they might not look it it's both easier and more maintainable to just write software that's obviously safe to inspection.