* [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
@ 2018-05-03 21:34 Girish Mahadevan
2018-05-03 23:38 ` Mark Brown
` (2 more replies)
0 siblings, 3 replies; 33+ messages in thread
From: Girish Mahadevan @ 2018-05-03 21:34 UTC (permalink / raw)
To: broonie, linux-kernel, linux-spi
Cc: sdharia, kramasub, dianders, linux-arm-msm, swboyd, Girish Mahadevan
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 <girishm@codeaurora.org>
---
drivers/spi/Kconfig | 12 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-geni-qcom.c | 766 ++++++++++++++++++++++++++++++++++++++
include/linux/spi/spi-geni-qcom.h | 14 +
4 files changed, 793 insertions(+)
create mode 100644 drivers/spi/spi-geni-qcom.c
create mode 100644 include/linux/spi/spi-geni-qcom.h
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9b31351..358d60a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -564,6 +564,18 @@ config SPI_QUP
This driver can also be built as a module. If so, the module
will be called spi_qup.
+config SPI_QCOM_GENI
+ tristate "Qualcomm SPI controller with QUP interface"
+ depends on ARCH_QCOM || (ARM && COMPILE_TEST)
+ 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
+ interface on the Qualcomm Technologies Inc.'s SoCs.
+
+ This driver can also be built as a module. If so, the module
+ will be called spi-geni-qcom.
+
config SPI_S3C24XX
tristate "Samsung S3C24XX series SPI"
depends on ARCH_S3C24XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index a3ae2b7..cc90d6e 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -77,6 +77,7 @@ spi-pxa2xx-platform-objs := spi-pxa2xx.o spi-pxa2xx-dma.o
obj-$(CONFIG_SPI_PXA2XX) += spi-pxa2xx-platform.o
obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o
obj-$(CONFIG_SPI_QUP) += spi-qup.o
+obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o
obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o
obj-$(CONFIG_SPI_RSPI) += spi-rspi.o
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
new file mode 100644
index 0000000..eecc634
--- /dev/null
+++ b/drivers/spi/spi-geni-qcom.c
@@ -0,0 +1,766 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/spinlock.h>
+#include <linux/qcom-geni-se.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-geni-qcom.h>
+
+#define SPI_NUM_CHIPSELECT 4
+#define SPI_XFER_TIMEOUT_MS 250
+/* SPI SE specific registers */
+#define SE_SPI_CPHA 0x224
+#define SE_SPI_LOOPBACK 0x22c
+#define SE_SPI_CPOL 0x230
+#define SE_SPI_DEMUX_OUTPUT_INV 0x24c
+#define SE_SPI_DEMUX_SEL 0x250
+#define SE_SPI_TRANS_CFG 0x25c
+#define SE_SPI_WORD_LEN 0x268
+#define SE_SPI_TX_TRANS_LEN 0x26c
+#define SE_SPI_RX_TRANS_LEN 0x270
+#define SE_SPI_PRE_POST_CMD_DLY 0x274
+#define SE_SPI_DELAY_COUNTERS 0x278
+
+/* SE_SPI_CPHA register fields */
+#define CPHA BIT(0)
+
+/* SE_SPI_LOOPBACK register fields */
+#define LOOPBACK_ENABLE 0x1
+#define NORMAL_MODE 0x0
+#define LOOPBACK_MSK GENMASK(1, 0)
+
+/* SE_SPI_CPOL register fields */
+#define CPOL BIT(2)
+
+/* SE_SPI_DEMUX_OUTPUT_INV register fields */
+#define CS_DEMUX_OUTPUT_INV_MSK GENMASK(3, 0)
+
+/* SE_SPI_DEMUX_SEL register fields */
+#define CS_DEMUX_OUTPUT_SEL GENMASK(3, 0)
+
+/* SE_SPI_TX_TRANS_CFG register fields */
+#define CS_TOGGLE BIT(0)
+
+/* SE_SPI_WORD_LEN register fields */
+#define WORD_LEN_MSK GENMASK(9, 0)
+#define MIN_WORD_LEN 4
+
+/* SPI_TX/SPI_RX_TRANS_LEN fields */
+#define TRANS_LEN_MSK GENMASK(23, 0)
+
+/* SE_SPI_DELAY_COUNTERS */
+#define SPI_INTER_WORDS_DELAY_MSK GENMASK(9, 0)
+#define SPI_CS_CLK_DELAY_MSK GENMASK(19, 10)
+#define SPI_CS_CLK_DELAY_SHFT 10
+
+/* M_CMD OP codes for SPI */
+#define SPI_TX_ONLY 1
+#define SPI_RX_ONLY 2
+#define SPI_FULL_DUPLEX 3
+#define SPI_TX_RX 7
+#define SPI_CS_ASSERT 8
+#define SPI_CS_DEASSERT 9
+#define SPI_SCK_ONLY 10
+/* M_CMD params for SPI */
+#define SPI_PRE_CMD_DELAY BIT(0)
+#define TIMESTAMP_BEFORE BIT(1)
+#define FRAGMENTATION BIT(2)
+#define TIMESTAMP_AFTER BIT(3)
+#define POST_CMD_DELAY BIT(4)
+
+static irqreturn_t geni_spi_isr(int irq, void *dev);
+
+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;
+ bool setup;
+ u32 cur_speed_hz;
+ int cur_word_len;
+ spinlock_t lock;
+ unsigned int tx_rem_bytes;
+ unsigned int rx_rem_bytes;
+ struct spi_transfer *cur_xfer;
+ struct completion xfer_done;
+ int oversampling;
+};
+
+static struct spi_master *get_spi_master(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct spi_master *spi = platform_get_drvdata(pdev);
+
+ return spi;
+}
+
+static int get_spi_clk_cfg(u32 speed_hz, struct spi_geni_master *mas,
+ int *clk_idx, 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), clk_idx,
+ &sclk_freq, true);
+ if (ret) {
+ dev_err(mas->dev, "%s: Failed(%d) to find src clk for 0x%x\n",
+ __func__, ret, speed_hz);
+ return ret;
+ }
+
+ *clk_div = ((sclk_freq / mas->oversampling) / speed_hz);
+ if (!(*clk_div)) {
+ dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d speed:%u\n",
+ __func__, sclk_freq, mas->oversampling, speed_hz);
+ return -EINVAL;
+ }
+
+ dev_dbg(mas->dev, "%s: req %u sclk %lu, idx %d, div %d\n", __func__,
+ speed_hz, sclk_freq, *clk_idx, *clk_div);
+ ret = clk_set_rate(se->clk, sclk_freq);
+ if (ret)
+ dev_err(mas->dev, "%s: clk_set_rate failed %d\n",
+ __func__, ret);
+ return ret;
+}
+
+static void spi_setup_word_len(struct spi_geni_master *mas, u32 mode,
+ int bits_per_word)
+{
+ int pack_words = 1;
+ bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
+ struct geni_se *se = &mas->se;
+ u32 word_len;
+
+ word_len = readl_relaxed(se->base + SE_SPI_WORD_LEN);
+
+ /*
+ * If bits_per_word isn't a byte aligned value, set the packing to be
+ * 1 SPI word per FIFO word.
+ */
+ if (!(mas->tx_fifo_width % bits_per_word))
+ pack_words = mas->tx_fifo_width / bits_per_word;
+ word_len &= ~WORD_LEN_MSK;
+ word_len |= ((bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK);
+ geni_se_config_packing(&mas->se, bits_per_word, pack_words, msb_first,
+ true, true);
+ writel_relaxed(word_len, se->base + SE_SPI_WORD_LEN);
+}
+
+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;
+ int idx;
+ int div;
+ struct spi_geni_qcom_ctrl_data *delay_params = NULL;
+ u32 spi_delay_params = 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);
+
+ if (spi_slv->controller_data) {
+ u32 cs_clk_delay = 0;
+ u32 inter_words_delay = 0;
+
+ delay_params =
+ (struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
+ cs_clk_delay =
+ (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
+ & SPI_CS_CLK_DELAY_MSK;
+ inter_words_delay =
+ delay_params->spi_inter_words_delay &
+ SPI_INTER_WORDS_DELAY_MSK;
+ spi_delay_params =
+ (inter_words_delay | cs_clk_delay);
+ }
+
+ 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);
+ goto setup_fifo_params_exit;
+ }
+
+ clk_sel |= (idx & CLK_SEL_MSK);
+ m_clk_cfg |= ((div << CLK_DIV_SHFT) | SER_CLK_EN);
+ spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
+ writel_relaxed(loopback_cfg, se->base + SE_SPI_LOOPBACK);
+ writel_relaxed(demux_sel, se->base + SE_SPI_DEMUX_SEL);
+ writel_relaxed(cpha, se->base + SE_SPI_CPHA);
+ writel_relaxed(cpol, se->base + SE_SPI_CPOL);
+ writel_relaxed(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
+ writel_relaxed(clk_sel, se->base + SE_GENI_CLK_SEL);
+ writel_relaxed(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
+ writel_relaxed(spi_delay_params, se->base + SE_SPI_DELAY_COUNTERS);
+setup_fifo_params_exit:
+ return ret;
+}
+
+static int spi_geni_prepare_message(struct spi_master *spi,
+ struct spi_message *spi_msg)
+{
+ int ret = 0;
+ struct spi_geni_master *mas = spi_master_get_devdata(spi);
+ struct geni_se *se = &mas->se;
+
+ geni_se_select_mode(se, GENI_SE_FIFO);
+ reinit_completion(&mas->xfer_done);
+ ret = setup_fifo_params(spi_msg->spi, spi);
+ if (ret) {
+ dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, ret);
+ ret = -EINVAL;
+ }
+ return ret;
+}
+
+static int spi_geni_unprepare_message(struct spi_master *spi_mas,
+ struct spi_message *spi_msg)
+{
+ struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
+
+ mas->cur_speed_hz = 0;
+ mas->cur_word_len = 0;
+ return 0;
+}
+
+static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
+{
+ struct spi_geni_master *mas = spi_master_get_devdata(spi);
+ int ret = 0;
+ struct geni_se *se = &mas->se;
+
+ ret = pm_runtime_get_sync(mas->dev);
+ if (ret < 0) {
+ dev_err(mas->dev, "Error enabling SE resources\n");
+ pm_runtime_put_noidle(mas->dev);
+ goto exit_prepare_transfer_hardware;
+ } else {
+ ret = 0;
+ }
+
+ if (unlikely(!mas->setup)) {
+ int proto = geni_se_read_proto(se);
+ unsigned int major;
+ unsigned int minor;
+ unsigned int step;
+
+ if (unlikely(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);
+ geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
+ 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);
+ if ((major == 1) && (minor == 0))
+ mas->oversampling = 2;
+ mas->setup = 1;
+ ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
+ IRQF_TRIGGER_HIGH, "spi_geni", mas);
+ if (ret) {
+ dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",
+ mas->irq, ret);
+ goto exit_prepare_transfer_hardware;
+ }
+ }
+exit_prepare_transfer_hardware:
+ return ret;
+}
+
+static int spi_geni_unprepare_transfer_hardware(struct spi_master *spi)
+{
+ struct spi_geni_master *mas = spi_master_get_devdata(spi);
+
+ pm_runtime_put_sync(mas->dev);
+ return 0;
+}
+
+static void setup_fifo_xfer(struct spi_transfer *xfer,
+ struct spi_geni_master *mas, u16 mode,
+ struct spi_master *spi)
+{
+ u32 m_cmd = 0;
+ u32 m_param = 0;
+ struct geni_se *se = &mas->se;
+ u32 spi_tx_cfg = readl_relaxed(se->base + SE_SPI_TRANS_CFG);
+ u32 trans_len = 0;
+
+ if (xfer->bits_per_word != mas->cur_word_len) {
+ spi_setup_word_len(mas, mode, xfer->bits_per_word);
+ mas->cur_word_len = xfer->bits_per_word;
+ }
+
+ /* Speed and bits per word can be overridden per transfer */
+ if (xfer->speed_hz != mas->cur_speed_hz) {
+ int ret = 0;
+ u32 clk_sel = 0;
+ u32 m_clk_cfg = 0;
+ int idx = 0;
+ int div = 0;
+
+ ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, &div);
+ if (ret) {
+ dev_err(mas->dev, "%s:Err setting clks:%d\n",
+ __func__, ret);
+ return;
+ }
+ mas->cur_speed_hz = xfer->speed_hz;
+ clk_sel |= (idx & CLK_SEL_MSK);
+ m_clk_cfg |= ((div << CLK_DIV_SHFT) | SER_CLK_EN);
+ writel_relaxed(clk_sel, se->base + SE_GENI_CLK_SEL);
+ writel_relaxed(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
+ }
+
+ mas->tx_rem_bytes = 0;
+ mas->rx_rem_bytes = 0;
+ if (xfer->tx_buf && xfer->rx_buf)
+ m_cmd = SPI_FULL_DUPLEX;
+ else if (xfer->tx_buf)
+ m_cmd = SPI_TX_ONLY;
+ else if (xfer->rx_buf)
+ m_cmd = SPI_RX_ONLY;
+
+ spi_tx_cfg &= ~CS_TOGGLE;
+ if (!(mas->cur_word_len % MIN_WORD_LEN)) {
+ trans_len =
+ ((xfer->len * BITS_PER_BYTE) /
+ mas->cur_word_len) & TRANS_LEN_MSK;
+ } else {
+ int bytes_per_word = (mas->cur_word_len / BITS_PER_BYTE) + 1;
+
+ trans_len = (xfer->len / bytes_per_word) & TRANS_LEN_MSK;
+ }
+
+ /*
+ * 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))
+ m_param |= FRAGMENTATION;
+ }
+
+ 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);
+}
+
+static void handle_fifo_timeout(struct spi_geni_master *mas)
+{
+ unsigned long timeout;
+ struct geni_se *se = &mas->se;
+ unsigned long flags;
+
+ reinit_completion(&mas->xfer_done);
+ spin_lock_irqsave(&mas->lock, flags);
+ geni_se_cancel_m_cmd(se);
+ writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
+ timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
+ if (!timeout) {
+ reinit_completion(&mas->xfer_done);
+ geni_se_abort_m_cmd(se);
+ timeout = wait_for_completion_timeout(&mas->xfer_done,
+ HZ);
+ if (!timeout)
+ dev_err(mas->dev,
+ "Failed to cancel/abort m_cmd\n");
+ }
+ spin_unlock_irqrestore(&mas->lock, flags);
+}
+
+static int spi_geni_transfer_one(struct spi_master *spi,
+ struct spi_device *slv,
+ struct spi_transfer *xfer)
+{
+ int ret = 0;
+ struct spi_geni_master *mas = spi_master_get_devdata(spi);
+ unsigned long timeout;
+
+ if ((xfer->tx_buf == NULL) && (xfer->rx_buf == NULL)) {
+ dev_err(mas->dev, "Invalid xfer both tx rx are NULL\n");
+ return -EINVAL;
+ }
+
+ setup_fifo_xfer(xfer, mas, slv->mode, spi);
+ timeout = wait_for_completion_timeout(&mas->xfer_done,
+ msecs_to_jiffies(SPI_XFER_TIMEOUT_MS));
+ if (!timeout) {
+ dev_err(mas->dev,
+ "Xfer[len %d tx %pK rx %pK n %d] timed out.\n",
+ xfer->len, xfer->tx_buf,
+ xfer->rx_buf,
+ xfer->bits_per_word);
+ mas->cur_xfer = NULL;
+ ret = -ETIMEDOUT;
+ goto err_fifo_geni_transfer_one;
+ }
+ return ret;
+err_fifo_geni_transfer_one:
+ handle_fifo_timeout(mas);
+ return ret;
+}
+
+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.
+ * 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))
+ 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;
+ tx_buf = mas->cur_xfer->tx_buf;
+ tx_buf += (mas->cur_xfer->len - mas->tx_rem_bytes);
+ max_bytes = min_t(int, mas->tx_rem_bytes, max_bytes);
+ 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))
+ bytes_per_fifo =
+ (mas->cur_word_len / BITS_PER_BYTE) + 1;
+ bytes_to_write = min_t(int, max_bytes - i, bytes_per_fifo);
+ fifo_byte = (u8 *)&fifo_word;
+ for (j = 0; j < bytes_to_write; j++)
+ fifo_byte[j] = tx_buf[i++];
+ iowrite32_rep(se->base + SE_GENI_TX_FIFOn, &fifo_word, 1);
+ }
+ mas->tx_rem_bytes -= max_bytes;
+ if (!mas->tx_rem_bytes)
+ writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
+}
+
+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;
+
+ rx_buf = mas->cur_xfer->rx_buf;
+ rx_wc = rx_fifo_status & RX_FIFO_WC_MSK;
+ if (rx_fifo_status & RX_LAST) {
+ int rx_last_byte_valid =
+ (rx_fifo_status & RX_LAST_BYTE_VALID_MSK)
+ >> RX_LAST_BYTE_VALID_SHFT;
+ if (rx_last_byte_valid && (rx_last_byte_valid < 4)) {
+ rx_wc -= 1;
+ rx_bytes += rx_last_byte_valid;
+ }
+ }
+
+ /*
+ * 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);
+ rx_buf += (mas->cur_xfer->len - mas->rx_rem_bytes);
+ 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))
+ bytes_per_fifo =
+ (mas->cur_word_len / BITS_PER_BYTE) + 1;
+ read_bytes = min_t(int, rx_bytes - i, bytes_per_fifo);
+ ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 1);
+ fifo_byte = (u8 *)&fifo_word;
+ for (j = 0; j < read_bytes; j++)
+ rx_buf[i++] = fifo_byte[j];
+ }
+ mas->rx_rem_bytes -= rx_bytes;
+}
+
+static irqreturn_t geni_spi_isr(int irq, void *dev)
+{
+ struct spi_geni_master *mas = dev;
+ struct geni_se *se = &mas->se;
+ u32 m_irq = 0;
+ irqreturn_t ret = IRQ_HANDLED;
+ unsigned long flags;
+
+ spin_lock_irqsave(&mas->lock, flags);
+ if (pm_runtime_status_suspended(dev)) {
+ ret = IRQ_NONE;
+ goto exit_geni_spi_irq;
+ }
+ m_irq = readl_relaxed(se->base + SE_GENI_M_IRQ_STATUS);
+ if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
+ geni_spi_handle_rx(mas);
+
+ if ((m_irq & M_TX_FIFO_WATERMARK_EN))
+ geni_spi_handle_tx(mas);
+
+ if ((m_irq & M_CMD_DONE_EN) || (m_irq & M_CMD_CANCEL_EN) ||
+ (m_irq & M_CMD_ABORT_EN)) {
+ complete(&mas->xfer_done);
+ /*
+ * If this happens, then a CMD_DONE came before all the Tx
+ * buffer bytes were sent out. This is unusual, log this
+ * condition and disable the WM interrupt to prevent the
+ * system from stalling due an interrupt storm.
+ * If this happens when all Rx bytes haven't been received, log
+ * the condition.
+ * The only known time this can happen is if bits_per_word != 8
+ * and some registers that expect xfer lengths in num spi_words
+ * weren't written correctly.
+ */
+ if (mas->tx_rem_bytes) {
+ writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
+ dev_err(mas->dev,
+ "%s:Premature Done.tx_rem%d bpw%d\n",
+ __func__, mas->tx_rem_bytes, mas->cur_word_len);
+ }
+ if (mas->rx_rem_bytes)
+ dev_err(mas->dev,
+ "%s:Premature Done.rx_rem%d bpw%d\n",
+ __func__, mas->rx_rem_bytes, mas->cur_word_len);
+ }
+exit_geni_spi_irq:
+ writel_relaxed(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
+ spin_unlock_irqrestore(&mas->lock, flags);
+ return ret;
+}
+
+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");
+ goto spi_geni_probe_err;
+ }
+
+ 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 = of_alias_get_id(pdev->dev.of_node, "spi");
+ spi->dev.of_node = pdev->dev.of_node;
+ 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)) {
+ dev_err(&pdev->dev, "Max frequency not specified.\n");
+ ret = -ENXIO;
+ goto spi_geni_probe_err;
+ }
+
+ 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");
+ goto spi_geni_probe_err;
+ }
+
+ spi_geni->irq = platform_get_irq(pdev, 0);
+ if (spi_geni->irq < 0) {
+ dev_err(&pdev->dev, "Err getting IRQ\n");
+ ret = spi_geni->irq;
+ goto spi_geni_probe_unmap;
+ }
+
+ spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
+ spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
+ spi->num_chipselect = SPI_NUM_CHIPSELECT;
+ spi->prepare_transfer_hardware = spi_geni_prepare_transfer_hardware;
+ spi->prepare_message = spi_geni_prepare_message;
+ spi->unprepare_message = spi_geni_unprepare_message;
+ spi->transfer_one = spi_geni_transfer_one;
+ spi->unprepare_transfer_hardware
+ = spi_geni_unprepare_transfer_hardware;
+ spi->auto_runtime_pm = false;
+
+ init_completion(&spi_geni->xfer_done);
+ spin_lock_init(&spi_geni->lock);
+ pm_runtime_enable(&pdev->dev);
+ ret = spi_register_master(spi);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register SPI master\n");
+ goto spi_geni_probe_unmap;
+ }
+ dev_dbg(&pdev->dev, "%s Probed\n", __func__);
+ return ret;
+spi_geni_probe_unmap:
+ devm_iounmap(&pdev->dev, se->base);
+spi_geni_probe_err:
+ spi_master_put(spi);
+ return ret;
+}
+
+static int spi_geni_remove(struct platform_device *pdev)
+{
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct spi_geni_master *spi_geni = spi_master_get_devdata(master);
+
+ spi_unregister_master(master);
+ geni_se_resources_off(&spi_geni->se);
+ pm_runtime_put_noidle(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ return 0;
+}
+
+static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
+{
+ int ret;
+ struct spi_master *spi = get_spi_master(dev);
+ struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
+
+ ret = geni_se_resources_off(&spi_geni->se);
+ return ret;
+}
+
+static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
+{
+ int ret;
+ struct spi_master *spi = get_spi_master(dev);
+ struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
+
+ ret = geni_se_resources_on(&spi_geni->se);
+ return ret;
+}
+
+static int __maybe_unused spi_geni_suspend(struct device *dev)
+{
+ if (!pm_runtime_status_suspended(dev))
+ return -EBUSY;
+ 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)
+};
+
+static const struct of_device_id spi_geni_dt_match[] = {
+ { .compatible = "qcom,geni-spi" },
+ {}
+};
+
+static struct platform_driver spi_geni_driver = {
+ .probe = spi_geni_probe,
+ .remove = spi_geni_remove,
+ .driver = {
+ .name = "geni_spi",
+ .pm = &spi_geni_pm_ops,
+ .of_match_table = spi_geni_dt_match,
+ },
+};
+module_platform_driver(spi_geni_driver);
+
+MODULE_DESCRIPTION("SPI driver for GENI based QUP cores");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/spi/spi-geni-qcom.h b/include/linux/spi/spi-geni-qcom.h
new file mode 100644
index 0000000..dc95764
--- /dev/null
+++ b/include/linux/spi/spi-geni-qcom.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * 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 {
+ u32 spi_cs_clk_delay;
+ u32 spi_inter_words_delay;
+};
+
+#endif /*__SPI_GENI_QCOM_HEADER___*/
--
1.9.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-03 21:34 [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Girish Mahadevan
@ 2018-05-03 23:38 ` Mark Brown
2018-05-07 21:40 ` Mahadevan, Girish
[not found] ` <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>
2018-05-11 22:30 ` Stephen Boyd
2018-06-08 23:34 ` Matthias Kaehlcke
2 siblings, 2 replies; 33+ messages in thread
From: Mark Brown @ 2018-05-03 23:38 UTC (permalink / raw)
To: Girish Mahadevan
Cc: linux-kernel, linux-spi, sdharia, kramasub, dianders,
linux-arm-msm, swboyd
[-- Attachment #1: Type: text/plain, Size: 3086 bytes --]
On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
> 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.
This is a DT based driver but there is no binding documentation.
Binding documentation is required for any new DT stuff.
> + depends on ARCH_QCOM || (ARM && COMPILE_TEST)
Why the ARM dependency? There's no architecture specific headers
included...
> obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o
> obj-$(CONFIG_SPI_QUP) += spi-qup.o
> +obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o
> obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
Please keep Kconfig and Makefile alphabetically sorted to reduce
conflicts.
> +static struct spi_master *get_spi_master(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct spi_master *spi = platform_get_drvdata(pdev);
> +
> + return spi;
> +}
This doesn't look at all driver specific with the current naming but it
actually is given that other drivers may use other driver data so it
should be renamed. I'm also not clear why it's bouncing through the
platform device, dev_get_drvdata() exists.
> +static int spi_geni_unprepare_message(struct spi_master *spi_mas,
> + struct spi_message *spi_msg)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> + mas->cur_speed_hz = 0;
> + mas->cur_word_len = 0;
> + return 0;
> +}
Is this really useful? If the driver needs to reconfigure for every
message then it should just do that and not care about the state. If it
might end up caring about the state anyway that suggests there's some
kind of bug somewhere that's being masked.
> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + int ret = 0;
> + struct geni_se *se = &mas->se;
> +
> + ret = pm_runtime_get_sync(mas->dev);
> + if (ret < 0) {
Use auto_runtime_pm.
> + if (unlikely(!mas->setup)) {
> + int proto = geni_se_read_proto(se);
Does this really need a likely/unlikely annotation - it shouldn't be any
kind of hot path... There's a lot of these annotations in the code.
> + ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
> + IRQF_TRIGGER_HIGH, "spi_geni", mas);
> + if (ret) {
> + dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",
Why are we dynamically requesting the IRQ outside of probe? Normally an
interrupt is requested on startup and held through the life of the
device. I'm also not seeing any sign that it's freed except via devm...
> + spi->bus_num = of_alias_get_id(pdev->dev.of_node, "spi");
Don't do this, just set bus_num to -1 and let the core assign an ID.
> + 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.
> + pm_runtime_enable(&pdev->dev);
> + ret = spi_register_master(spi);
No devm?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-03 23:38 ` Mark Brown
@ 2018-05-07 21:40 ` Mahadevan, Girish
[not found] ` <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>
1 sibling, 0 replies; 33+ messages in thread
From: Mahadevan, Girish @ 2018-05-07 21:40 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, linux-spi, sdharia, kramasub, dianders,
linux-arm-msm, swboyd
Hi Mark
On 5/3/2018 5:38 PM, Mark Brown wrote:
> On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
>> 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.
>
> This is a DT based driver but there is no binding documentation.
> Binding documentation is required for any new DT stuff.
>
The DT documentation for the SPI driver was done as part of this patch
series
https://patchwork.kernel.org/patch/10318125/
>> + depends on ARCH_QCOM || (ARM && COMPILE_TEST)
> > Why the ARM dependency? There's no architecture specific headers
> included...
Agree, I will remove it. I will add the dependency on QCOM_GENI_SE(to be
consistent with the other GENI_QUP protocol drivers (I2C and UART))
>> obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o
>> obj-$(CONFIG_SPI_QUP) += spi-qup.o
>> +obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o
>> obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
>
> Please keep Kconfig and Makefile alphabetically sorted to reduce
> conflicts.
>
Ok.
>> +static struct spi_master *get_spi_master(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct spi_master *spi = platform_get_drvdata(pdev);
>> +
>> + return spi;
>> +}
>
> This doesn't look at all driver specific with the current naming but it
> actually is given that other drivers may use other driver data so it
> should be renamed. I'm also not clear why it's bouncing through the
> platform device, dev_get_drvdata() exists.
>
Agree, this function isn't needed, dev_get_drvdata() should be sufficient.
>> +static int spi_geni_unprepare_message(struct spi_master *spi_mas,
>> + struct spi_message *spi_msg)
>> +{
>> + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
>> +
>> + mas->cur_speed_hz = 0;
>> + mas->cur_word_len = 0;
>> + return 0;
>> +}
>
> Is this really useful? If the driver needs to reconfigure for every
> message then it should just do that and not care about the state. If it
> might end up caring about the state anyway that suggests there's some
> kind of bug somewhere that's being masked.
>
Agree, it can be removed.
>> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
>> +{
>> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> + int ret = 0;
>> + struct geni_se *se = &mas->se;
>> +
>> + ret = pm_runtime_get_sync(mas->dev);
>> + if (ret < 0) {
>
> Use auto_runtime_pm.
>
Ok
>> + if (unlikely(!mas->setup)) {
>> + int proto = geni_se_read_proto(se);
>
> Does this really need a likely/unlikely annotation - it shouldn't be any
> kind of hot path... There's a lot of these annotations in the code.
>
Ok
>> + ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
>> + IRQF_TRIGGER_HIGH, "spi_geni", mas);
>> + if (ret) {
>> + dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",
>
> Why are we dynamically requesting the IRQ outside of probe? Normally an
> interrupt is requested on startup and held through the life of the
> device. I'm also not seeing any sign that it's freed except via devm...
>
Ok, will move this to probe.
>> + spi->bus_num = of_alias_get_id(pdev->dev.of_node, "spi");
>
> Don't do this, just set bus_num to -1 and let the core assign an ID.
>
Ok.
>> + 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.
>
I don't follow, if I don't do this the framework won't be able to probe
the slave devices of the controller.
>> + pm_runtime_enable(&pdev->dev);
>> + ret = spi_register_master(spi);
>
> No devm?
>
Agree, I will change this to use devm_spi_register_master()
Best Regards
Girish
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 33+ messages in thread
[parent not found: <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>]
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
[not found] ` <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>
@ 2018-05-17 7:21 ` Mark Brown
2018-05-21 21:45 ` Mahadevan, Girish
0 siblings, 1 reply; 33+ messages in thread
From: Mark Brown @ 2018-05-17 7:21 UTC (permalink / raw)
To: Mahadevan, Girish
Cc: linux-kernel, linux-spi, sdharia, kramasub, dianders,
linux-arm-msm, swboyd
[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]
On Mon, May 07, 2018 at 02:29:45PM -0600, Mahadevan, Girish wrote:
> On 5/3/2018 5:38 PM, Mark Brown wrote:
> > This is a DT based driver but there is no binding documentation.
> > Binding documentation is required for any new DT stuff.
> The DT documentation for the SPI driver was done as part of this patch series
> https://patchwork.kernel.org/patch/10318125/
I can't follow the link as I'm working offline but since I've no record
of having seen a copy of any bindings for review and I'm fairly sure I'd
have remembered any bindings without code I'm very disappointed -
bindings should be being reviewed by the relevant maintainers just like
code.
Fortunately as far as I can tell whereever you sent that to it doesn't
seem to have been applied but that makes it even more disappointing that
they're not being sent.
...
Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-17 7:21 ` Mark Brown
@ 2018-05-21 21:45 ` Mahadevan, Girish
2018-05-22 17:32 ` Mark Brown
0 siblings, 1 reply; 33+ messages in thread
From: Mahadevan, Girish @ 2018-05-21 21:45 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, linux-spi, sdharia, kramasub, dianders,
linux-arm-msm, swboyd
[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]
Hi Mark,
On 5/17/2018 1:21 AM, Mark Brown wrote:
> On Mon, May 07, 2018 at 02:29:45PM -0600, Mahadevan, Girish wrote:
>> On 5/3/2018 5:38 PM, Mark Brown wrote:
>
>>> This is a DT based driver but there is no binding documentation.
>>> Binding documentation is required for any new DT stuff.
>
>> The DT documentation for the SPI driver was done as part of this patch series
>> https://patchwork.kernel.org/patch/10318125/
>
> I can't follow the link as I'm working offline but since I've no record
> of having seen a copy of any bindings for review and I'm fairly sure I'd
> have remembered any bindings without code I'm very disappointed -
> bindings should be being reviewed by the relevant maintainers just like
> code.
>
> Fortunately as far as I can tell whereever you sent that to it doesn't
> seem to have been applied but that makes it even more disappointing that
> they're not being sent.
>
https://patchwork.kernel.org/patch/10318125/
[
Add device tree binding support for the QCOM GENI SE driver.
Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
]
is a patch train for Generic Interface (GENI) based Qualcomm Universal
Peripheral (QUP) wrapper. The wrapper can contain one or more mini cores
that can be used to implement different serial protocols (I2C/SPI/UART).
We'd submitted the DT bindings for that wrapper core and for UART/I2C
drivers which were part of that patch train; but there was a comment to
add the SPI binding document even without the SPI driver (attaching that
email thread).
I can resubmit the SPI binding documentation as part of this patch series.
Best Regards
Girish
> ...
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.
[-- Attachment #2: Fwd Re [PATCH v3 1_4] dt-bindings soc qcom Add device tree binding for GENI SE.txt --]
[-- Type: text/plain, Size: 4521 bytes --]
Subject:
Fwd: Re: [PATCH v3 1/4] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From:
Karthik Ramasubramanian <kramasub@codeaurora.org>
Date:
5/21/2018 10:44 AM
To:
"Mahadevan, Girish" <girishm@codeaurora.org>
-------- Forwarded Message --------
Subject: Re: [PATCH v3 1/4] dt-bindings: soc: qcom: Add device tree
binding for GENI SE
Date: Tue, 6 Mar 2018 10:13:09 -0700
From: Karthik Ramasubramanian <kramasub@codeaurora.org>
To: Rob Herring <robh@kernel.org>
CC: Jonathan Corbet <corbet@lwn.net>, Andy Gross
<andy.gross@linaro.org>, David Brown <david.brown@linaro.org>, Mark
Rutland <mark.rutland@arm.com>, Wolfram Sang <wsa@the-dreams.de>, Greg
Kroah-Hartman <gregkh@linuxfoundation.org>, linux-doc@vger.kernel.org,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
devicetree@vger.kernel.org, Linux I2C <linux-i2c@vger.kernel.org>,
linux-serial@vger.kernel.org, Jiri Slaby <jslaby@suse.com>,
evgreen@chromium.org, acourbot@chromium.org, Sagar Dharia
<sdharia@codeaurora.org>, Girish Mahadevan <girishm@codeaurora.org>
On 3/6/2018 6:22 AM, Rob Herring wrote:
> > On Mon, Mar 5, 2018 at 6:55 PM, Karthik Ramasubramanian
> > <kramasub@codeaurora.org> wrote:
>> >>
>> >>
>> >> On 3/5/2018 4:58 PM, Rob Herring wrote:
>>> >>>
>>> >>> On Tue, Feb 27, 2018 at 06:38:06PM -0700, Karthikeyan Ramasubramanian
>>> >>> wrote:
>>>> >>>>
>>>> >>>> Add device tree binding support for the QCOM GENI SE driver.
>>>> >>>>
>>>> >>>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>>>> >>>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>>>> >>>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>>>> >>>> ---
>>>> >>>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 89
>>>> >>>> ++++++++++++++++++++++
>>>> >>>> 1 file changed, 89 insertions(+)
>>>> >>>> create mode 100644
>>>> >>>> Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> >>>>
>>>> >>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> >>>> b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> >>>> new file mode 100644
>>>> >>>> index 0000000..fe6a0c0
>>>> >>>> --- /dev/null
>>>> >>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> >>>> @@ -0,0 +1,89 @@
>>>> >>>> +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
>>>> >>>> +
>>>> >>>> +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP)
>>>> >>>> wrapper
>>>> >>>> +is a programmable module for supporting a wide range of serial
>>>> >>>> interfaces
>>>> >>>> +like UART, SPI, I2C, I3C, etc. A single QUP module can provide upto 8
>>>> >>>> Serial
>>>> >>>> +Interfaces, using its internal Serial Engines. The GENI Serial Engine
>>>> >>>> QUP
>>>> >>>> +Wrapper controller is modeled as a node with zero or more child nodes
>>>> >>>> each
>>>> >>>> +representing a serial engine.
>>>> >>>> +
>>>> >>>> +Required properties:
>>>> >>>> +- compatible: Must be "qcom,geni-se-qup".
>>>> >>>> +- reg: Must contain QUP register address and length.
>>>> >>>> +- clock-names: Must contain "m-ahb" and "s-ahb".
>>>> >>>> +- clocks: AHB clocks needed by the device.
>>>> >>>> +
>>>> >>>> +Required properties if child node exists:
>>>> >>>> +- #address-cells: Must be <1> for Serial Engine Address
>>>> >>>> +- #size-cells: Must be <1> for Serial Engine Address
>>>> >>>> Size
>>>> >>>> +- ranges: Must be present
>>>> >>>> +
>>>> >>>> +Properties for children:
>>>> >>>> +
>>>> >>>> +A GENI based QUP wrapper controller node can contain 0 or more child
>>>> >>>> nodes
>>>> >>>> +representing serial devices. These serial devices can be a QCOM UART,
>>>> >>>> I2C
>>>> >>>> +controller, spi controller, or some combination of aforementioned
>>>> >>>> devices.
>>> >>>
>>> >>>
>>> >>> s/spi/SPI/
>>> >>>
>>> >>> Where's the SPI binding?
>> >>
>> >> Since the patch series introduces UART and I2C drivers, I added the bindings
>> >> only for them. I thought about adding the SPI binding when the SPI
>> >> controller driver is introduced. Please let me know if you want me to add
>> >> the bindings for SPI in this patch series itself.
> >
> > There's no requirement to have the driver and I prefer bindings be as
> > complete as possible.
Ok, I will add the bindings for SPI controller in the next posting.
> >
> > Rob
> >
Regards,
Karthik.
-- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-21 21:45 ` Mahadevan, Girish
@ 2018-05-22 17:32 ` Mark Brown
0 siblings, 0 replies; 33+ messages in thread
From: Mark Brown @ 2018-05-22 17:32 UTC (permalink / raw)
To: Mahadevan, Girish
Cc: linux-kernel, linux-spi, sdharia, kramasub, dianders,
linux-arm-msm, swboyd
[-- Attachment #1: Type: text/plain, Size: 453 bytes --]
On Mon, May 21, 2018 at 03:45:09PM -0600, Mahadevan, Girish wrote:
> I can resubmit the SPI binding documentation as part of this patch series.
Yes, and realy I'd expect to see the SPI subdevices being documented in
a separate SPI binding document rather than just lumped in with the
parent documentation. The same thing probably applies to the rest of
the bindings in there which I'm guessing have also not been reviewed in
the relevant subsystems.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-03 21:34 [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Girish Mahadevan
2018-05-03 23:38 ` Mark Brown
@ 2018-05-11 22:30 ` Stephen Boyd
2018-05-21 15:52 ` Mahadevan, Girish
2018-06-08 23:34 ` Matthias Kaehlcke
2 siblings, 1 reply; 33+ messages in thread
From: Stephen Boyd @ 2018-05-11 22:30 UTC (permalink / raw)
To: Girish Mahadevan, broonie, linux-kernel, linux-spi
Cc: sdharia, kramasub, dianders, linux-arm-msm, Girish Mahadevan
Quoting Girish Mahadevan (2018-05-03 14:34:43)
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 9b31351..358d60a 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -564,6 +564,18 @@ config SPI_QUP
> This driver can also be built as a module. If so, the module
> will be called spi_qup.
>
> +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?
> + depends on ARCH_QCOM || (ARM && COMPILE_TEST)
This driver uses the GENI wrapper code so it may need to have a better
Kconfig dependency than this.
> + 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"?
> + interface on the Qualcomm Technologies Inc.'s SoCs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called spi-geni-qcom.
> +
> config SPI_S3C24XX
> tristate "Samsung S3C24XX series SPI"
> depends on ARCH_S3C24XX
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index a3ae2b7..cc90d6e 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -77,6 +77,7 @@ spi-pxa2xx-platform-objs := spi-pxa2xx.o spi-pxa2xx-dma.o
> obj-$(CONFIG_SPI_PXA2XX) += spi-pxa2xx-platform.o
> obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o
> obj-$(CONFIG_SPI_QUP) += spi-qup.o
> +obj-$(CONFIG_SPI_QCOM_GENI) += spi-geni-qcom.o
This should come before QUP.
> obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o
> obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o
> obj-$(CONFIG_SPI_RSPI) += spi-rspi.o
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> new file mode 100644
> index 0000000..eecc634
> --- /dev/null
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -0,0 +1,766 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
include platform_device.h instead of of_platform.h?
> +#include <linux/pm_runtime.h>
> +#include <linux/spinlock.h>
> +#include <linux/qcom-geni-se.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-geni-qcom.h>
> +
> +#define SPI_NUM_CHIPSELECT 4
Why do we need the define? It's used one place.
> +#define SPI_XFER_TIMEOUT_MS 250
Same comment.
> +/* SPI SE specific registers */
> +#define SE_SPI_CPHA 0x224
> +#define SE_SPI_LOOPBACK 0x22c
> +#define SE_SPI_CPOL 0x230
> +#define SE_SPI_DEMUX_OUTPUT_INV 0x24c
> +#define SE_SPI_DEMUX_SEL 0x250
> +#define SE_SPI_TRANS_CFG 0x25c
> +#define SE_SPI_WORD_LEN 0x268
> +#define SE_SPI_TX_TRANS_LEN 0x26c
> +#define SE_SPI_RX_TRANS_LEN 0x270
> +#define SE_SPI_PRE_POST_CMD_DLY 0x274
> +#define SE_SPI_DELAY_COUNTERS 0x278
> +
> +/* SE_SPI_CPHA register fields */
> +#define CPHA BIT(0)
Can you put these defines next to the register that they correspond to?
Then we don't need the duplicate comment to indicate what registers they
are used with.
> +
> +/* SE_SPI_LOOPBACK register fields */
> +#define LOOPBACK_ENABLE 0x1
> +#define NORMAL_MODE 0x0
> +#define LOOPBACK_MSK GENMASK(1, 0)
> +
> +/* SE_SPI_CPOL register fields */
> +#define CPOL BIT(2)
> +
> +/* SE_SPI_DEMUX_OUTPUT_INV register fields */
> +#define CS_DEMUX_OUTPUT_INV_MSK GENMASK(3, 0)
> +
> +/* SE_SPI_DEMUX_SEL register fields */
> +#define CS_DEMUX_OUTPUT_SEL GENMASK(3, 0)
> +
> +/* SE_SPI_TX_TRANS_CFG register fields */
> +#define CS_TOGGLE BIT(0)
> +
> +/* SE_SPI_WORD_LEN register fields */
> +#define WORD_LEN_MSK GENMASK(9, 0)
> +#define MIN_WORD_LEN 4
> +
> +/* SPI_TX/SPI_RX_TRANS_LEN fields */
> +#define TRANS_LEN_MSK GENMASK(23, 0)
> +
> +/* SE_SPI_DELAY_COUNTERS */
> +#define SPI_INTER_WORDS_DELAY_MSK GENMASK(9, 0)
> +#define SPI_CS_CLK_DELAY_MSK GENMASK(19, 10)
> +#define SPI_CS_CLK_DELAY_SHFT 10
> +
> +/* M_CMD OP codes for SPI */
> +#define SPI_TX_ONLY 1
> +#define SPI_RX_ONLY 2
> +#define SPI_FULL_DUPLEX 3
> +#define SPI_TX_RX 7
> +#define SPI_CS_ASSERT 8
> +#define SPI_CS_DEASSERT 9
> +#define SPI_SCK_ONLY 10
> +/* M_CMD params for SPI */
> +#define SPI_PRE_CMD_DELAY BIT(0)
> +#define TIMESTAMP_BEFORE BIT(1)
> +#define FRAGMENTATION BIT(2)
> +#define TIMESTAMP_AFTER BIT(3)
> +#define POST_CMD_DELAY BIT(4)
> +
> +static irqreturn_t geni_spi_isr(int irq, void *dev);
> +
> +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?
> + bool setup;
> + u32 cur_speed_hz;
> + int cur_word_len;
unsigned?
> + spinlock_t lock;
> + unsigned int tx_rem_bytes;
> + unsigned int rx_rem_bytes;
> + struct spi_transfer *cur_xfer;
> + struct completion xfer_done;
> + int oversampling;
unsigned?
> +};
> +
> +static struct spi_master *get_spi_master(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct spi_master *spi = platform_get_drvdata(pdev);
> +
> + return spi;
> +}
This function is used in two places, and then it's followed immediately
by:
struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
so perhaps the function should be get_spi_geni_master() instead?
> +
> +static int get_spi_clk_cfg(u32 speed_hz, struct spi_geni_master *mas,
Why a u32? And not unsigned int?
> + int *clk_idx, 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), clk_idx,
> + &sclk_freq, true);
> + 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!
> + return ret;
> + }
> +
> + *clk_div = ((sclk_freq / mas->oversampling) / speed_hz);
> + if (!(*clk_div)) {
> + dev_err(mas->dev, "%s:Err:sclk:%lu oversampling:%d speed:%u\n",
> + __func__, sclk_freq, mas->oversampling, speed_hz);
> + return -EINVAL;
> + }
> +
> + dev_dbg(mas->dev, "%s: req %u sclk %lu, idx %d, div %d\n", __func__,
> + speed_hz, sclk_freq, *clk_idx, *clk_div);
> + ret = clk_set_rate(se->clk, sclk_freq);
> + if (ret)
> + dev_err(mas->dev, "%s: clk_set_rate failed %d\n",
> + __func__, ret);
> + return ret;
> +}
> +
> +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.
> + int bits_per_word)
> +{
> + int pack_words = 1;
> + bool msb_first = (mode & SPI_LSB_FIRST) ? false : true;
> + struct geni_se *se = &mas->se;
> + u32 word_len;
> +
> + word_len = readl_relaxed(se->base + SE_SPI_WORD_LEN);
> +
> + /*
> + * If bits_per_word isn't a byte aligned value, set the packing to be
> + * 1 SPI word per FIFO word.
> + */
> + if (!(mas->tx_fifo_width % bits_per_word))
> + pack_words = mas->tx_fifo_width / bits_per_word;
> + word_len &= ~WORD_LEN_MSK;
> + word_len |= ((bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK);
> + geni_se_config_packing(&mas->se, bits_per_word, pack_words, msb_first,
> + true, true);
> + writel_relaxed(word_len, se->base + SE_SPI_WORD_LEN);
> +}
> +
> +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.
> + int idx;
> + int div;
> + struct spi_geni_qcom_ctrl_data *delay_params = NULL;
> + u32 spi_delay_params = 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);
> +
> + if (spi_slv->controller_data) {
> + u32 cs_clk_delay = 0;
> + u32 inter_words_delay = 0;
> +
> + delay_params =
> + (struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
> + cs_clk_delay =
> + (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
> + & SPI_CS_CLK_DELAY_MSK;
> + inter_words_delay =
> + delay_params->spi_inter_words_delay &
> + SPI_INTER_WORDS_DELAY_MSK;
> + spi_delay_params =
> + (inter_words_delay | cs_clk_delay);
> + }
> +
> + 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?
> + 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);
> + goto setup_fifo_params_exit;
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.
> + spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
> + writel_relaxed(loopback_cfg, se->base + SE_SPI_LOOPBACK);
> + writel_relaxed(demux_sel, se->base + SE_SPI_DEMUX_SEL);
> + writel_relaxed(cpha, se->base + SE_SPI_CPHA);
> + writel_relaxed(cpol, se->base + SE_SPI_CPOL);
> + writel_relaxed(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
> + writel_relaxed(clk_sel, se->base + SE_GENI_CLK_SEL);
> + writel_relaxed(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
> + writel_relaxed(spi_delay_params, se->base + SE_SPI_DELAY_COUNTERS);
> +setup_fifo_params_exit:
Drop this label.
> + return ret;
return 0.
> +}
> +
> +static int spi_geni_prepare_message(struct spi_master *spi,
> + struct spi_message *spi_msg)
> +{
> + int ret = 0;
> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + struct geni_se *se = &mas->se;
> +
> + geni_se_select_mode(se, GENI_SE_FIFO);
> + reinit_completion(&mas->xfer_done);
> + ret = setup_fifo_params(spi_msg->spi, spi);
> + if (ret) {
> + dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, ret);
> + ret = -EINVAL;
> + }
> + return ret;
> +}
> +
> +static int spi_geni_unprepare_message(struct spi_master *spi_mas,
> + struct spi_message *spi_msg)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> + mas->cur_speed_hz = 0;
> + mas->cur_word_len = 0;
> + return 0;
> +}
> +
> +static int spi_geni_prepare_transfer_hardware(struct spi_master *spi)
Sometimes spi_master is called spi, other tims it's called spi_mas. Can
it be called spi everywhere? or spim?
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + int ret = 0;
> + struct geni_se *se = &mas->se;
> +
> + ret = pm_runtime_get_sync(mas->dev);
> + if (ret < 0) {
> + dev_err(mas->dev, "Error enabling SE resources\n");
> + pm_runtime_put_noidle(mas->dev);
> + goto exit_prepare_transfer_hardware;
> + } else {
> + ret = 0;
Does pm_runtime_get_sync() return anything besides 0 on success?
> + }
> +
> + if (unlikely(!mas->setup)) {
> + int proto = geni_se_read_proto(se);
> + unsigned int major;
> + unsigned int minor;
> + unsigned int step;
> +
> + if (unlikely(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);
> + geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
Why 2? Drop extra parenthesis please.
> + 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);
> + if ((major == 1) && (minor == 0))
Drop extra parenthesis.
> + mas->oversampling = 2;
> + mas->setup = 1;
> + ret = devm_request_irq(mas->dev, mas->irq, geni_spi_isr,
> + IRQF_TRIGGER_HIGH, "spi_geni", mas);
> + if (ret) {
> + dev_err(mas->dev, "Request_irq failed:%d: err:%d\n",
> + mas->irq, ret);
> + goto exit_prepare_transfer_hardware;
> + }
> + }
> +exit_prepare_transfer_hardware:
Drop label, just return ret at goto sites and return 0 otherwise.
> + return ret;
> +}
> +
> +static int spi_geni_unprepare_transfer_hardware(struct spi_master *spi)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> + pm_runtime_put_sync(mas->dev);
> + return 0;
> +}
> +
> +static void setup_fifo_xfer(struct spi_transfer *xfer,
> + struct spi_geni_master *mas, u16 mode,
> + struct spi_master *spi)
> +{
> + u32 m_cmd = 0;
> + u32 m_param = 0;
> + struct geni_se *se = &mas->se;
> + u32 spi_tx_cfg = readl_relaxed(se->base + SE_SPI_TRANS_CFG);
> + u32 trans_len = 0;
> +
> + if (xfer->bits_per_word != mas->cur_word_len) {
> + spi_setup_word_len(mas, mode, xfer->bits_per_word);
> + mas->cur_word_len = xfer->bits_per_word;
> + }
> +
> + /* Speed and bits per word can be overridden per transfer */
> + if (xfer->speed_hz != mas->cur_speed_hz) {
> + int ret = 0;
> + u32 clk_sel = 0;
> + u32 m_clk_cfg = 0;
> + int idx = 0;
> + int div = 0;
> +
> + ret = get_spi_clk_cfg(xfer->speed_hz, mas, &idx, &div);
> + if (ret) {
> + dev_err(mas->dev, "%s:Err setting clks:%d\n",
> + __func__, ret);
> + return;
> + }
> + mas->cur_speed_hz = xfer->speed_hz;
> + clk_sel |= (idx & CLK_SEL_MSK);
> + m_clk_cfg |= ((div << CLK_DIV_SHFT) | SER_CLK_EN);
> + writel_relaxed(clk_sel, se->base + SE_GENI_CLK_SEL);
> + writel_relaxed(m_clk_cfg, se->base + GENI_SER_M_CLK_CFG);
> + }
> +
> + mas->tx_rem_bytes = 0;
> + mas->rx_rem_bytes = 0;
> + if (xfer->tx_buf && xfer->rx_buf)
> + m_cmd = SPI_FULL_DUPLEX;
> + else if (xfer->tx_buf)
> + m_cmd = SPI_TX_ONLY;
> + else if (xfer->rx_buf)
> + m_cmd = SPI_RX_ONLY;
> +
> + spi_tx_cfg &= ~CS_TOGGLE;
> + if (!(mas->cur_word_len % MIN_WORD_LEN)) {
> + trans_len =
> + ((xfer->len * BITS_PER_BYTE) /
> + mas->cur_word_len) & TRANS_LEN_MSK;
> + } else {
> + int bytes_per_word = (mas->cur_word_len / BITS_PER_BYTE) + 1;
> +
> + trans_len = (xfer->len / bytes_per_word) & TRANS_LEN_MSK;
> + }
> +
> + /*
> + * 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.
> + m_param |= FRAGMENTATION;
> + }
> +
> + 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?
> +}
> +
> +static void handle_fifo_timeout(struct spi_geni_master *mas)
> +{
> + unsigned long timeout;
> + struct geni_se *se = &mas->se;
> + unsigned long flags;
> +
> + reinit_completion(&mas->xfer_done);
> + spin_lock_irqsave(&mas->lock, flags);
> + geni_se_cancel_m_cmd(se);
> + writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
> + timeout = wait_for_completion_timeout(&mas->xfer_done, HZ);
We can't wait_for_anything() inside of a spinlock.
> + if (!timeout) {
> + reinit_completion(&mas->xfer_done);
> + geni_se_abort_m_cmd(se);
> + timeout = wait_for_completion_timeout(&mas->xfer_done,
> + HZ);
> + if (!timeout)
> + dev_err(mas->dev,
> + "Failed to cancel/abort m_cmd\n");
> + }
> + spin_unlock_irqrestore(&mas->lock, flags);
> +}
> +
> +static int spi_geni_transfer_one(struct spi_master *spi,
> + struct spi_device *slv,
> + struct spi_transfer *xfer)
> +{
> + int ret = 0;
> + struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + unsigned long timeout;
> +
> + if ((xfer->tx_buf == NULL) && (xfer->rx_buf == NULL)) {
if (!xfer->tx_buf && !xfer->rx_buf)
is more normal style, but shouldn't the framework never call this if
this is the case? Looks like a useless check.
> + dev_err(mas->dev, "Invalid xfer both tx rx are NULL\n");
> + return -EINVAL;
> + }
> +
> + setup_fifo_xfer(xfer, mas, slv->mode, spi);
> + timeout = wait_for_completion_timeout(&mas->xfer_done,
> + msecs_to_jiffies(SPI_XFER_TIMEOUT_MS));
Can you implement the 'handle_err' for the controller and call
spi_finalize_current_transfer() from the interrupt handler when the
transfer completes? The completion variable stuff and timeout code in
this driver can hopefully all go away.
> + if (!timeout) {
> + dev_err(mas->dev,
> + "Xfer[len %d tx %pK rx %pK n %d] timed out.\n",
> + xfer->len, xfer->tx_buf,
> + xfer->rx_buf,
> + xfer->bits_per_word);
> + mas->cur_xfer = NULL;
> + ret = -ETIMEDOUT;
> + goto err_fifo_geni_transfer_one;
> + }
> + return ret;
> +err_fifo_geni_transfer_one:
> + handle_fifo_timeout(mas);
> + return ret;
> +}
> +
> +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"
> + * 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.
> + 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;
> + 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?
> + 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.
> + 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?
> + 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?
> + iowrite32_rep(se->base + SE_GENI_TX_FIFOn, &fifo_word, 1);
> + }
> + mas->tx_rem_bytes -= max_bytes;
> + if (!mas->tx_rem_bytes)
> + writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +}
> +
> +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.
> +
> + rx_buf = mas->cur_xfer->rx_buf;
> + rx_wc = rx_fifo_status & RX_FIFO_WC_MSK;
> + if (rx_fifo_status & RX_LAST) {
> + int rx_last_byte_valid =
> + (rx_fifo_status & RX_LAST_BYTE_VALID_MSK)
> + >> RX_LAST_BYTE_VALID_SHFT;
> + if (rx_last_byte_valid && (rx_last_byte_valid < 4)) {
> + rx_wc -= 1;
> + rx_bytes += rx_last_byte_valid;
> + }
> + }
> +
> + /*
> + * 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.
> + 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.
> + bytes_per_fifo =
> + (mas->cur_word_len / BITS_PER_BYTE) + 1;
> + read_bytes = min_t(int, rx_bytes - i, bytes_per_fifo);
> + ioread32_rep(se->base + SE_GENI_RX_FIFOn, &fifo_word, 1);
> + fifo_byte = (u8 *)&fifo_word;
> + for (j = 0; j < read_bytes; j++)
> + rx_buf[i++] = fifo_byte[j];
> + }
> + mas->rx_rem_bytes -= rx_bytes;
> +}
> +
> +static irqreturn_t geni_spi_isr(int irq, void *dev)
> +{
> + struct spi_geni_master *mas = dev;
> + struct geni_se *se = &mas->se;
> + u32 m_irq = 0;
> + irqreturn_t ret = IRQ_HANDLED;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mas->lock, flags);
> + if (pm_runtime_status_suspended(dev)) {
Why does the lock need to be held while checking runtime PM status?
> + ret = IRQ_NONE;
> + goto exit_geni_spi_irq;
> + }
> + m_irq = readl_relaxed(se->base + SE_GENI_M_IRQ_STATUS);
> + if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
> + geni_spi_handle_rx(mas);
> +
> + if ((m_irq & M_TX_FIFO_WATERMARK_EN))
> + geni_spi_handle_tx(mas);
> +
> + if ((m_irq & M_CMD_DONE_EN) || (m_irq & M_CMD_CANCEL_EN) ||
> + (m_irq & M_CMD_ABORT_EN)) {
> + complete(&mas->xfer_done);
> + /*
> + * If this happens, then a CMD_DONE came before all the Tx
> + * buffer bytes were sent out. This is unusual, log this
> + * condition and disable the WM interrupt to prevent the
> + * system from stalling due an interrupt storm.
> + * If this happens when all Rx bytes haven't been received, log
> + * the condition.
> + * The only known time this can happen is if bits_per_word != 8
> + * and some registers that expect xfer lengths in num spi_words
> + * weren't written correctly.
> + */
> + if (mas->tx_rem_bytes) {
> + writel_relaxed(0, se->base + SE_GENI_TX_WATERMARK_REG);
> + dev_err(mas->dev,
> + "%s:Premature Done.tx_rem%d bpw%d\n",
> + __func__, mas->tx_rem_bytes, mas->cur_word_len);
> + }
> + if (mas->rx_rem_bytes)
> + dev_err(mas->dev,
> + "%s:Premature Done.rx_rem%d bpw%d\n",
> + __func__, mas->rx_rem_bytes, mas->cur_word_len);
> + }
> +exit_geni_spi_irq:
> + writel_relaxed(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
> + spin_unlock_irqrestore(&mas->lock, flags);
> + return ret;
> +}
> +
> +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.
> + goto spi_geni_probe_err;
> + }
> +
> + 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 = of_alias_get_id(pdev->dev.of_node, "spi");
> + spi->dev.of_node = pdev->dev.of_node;
> + 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?
> + dev_err(&pdev->dev, "Max frequency not specified.\n");
> + ret = -ENXIO;
> + goto spi_geni_probe_err;
> + }
> +
> + 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.
> + goto spi_geni_probe_err;
> + }
> +
> + spi_geni->irq = platform_get_irq(pdev, 0);
> + if (spi_geni->irq < 0) {
> + dev_err(&pdev->dev, "Err getting IRQ\n");
> + ret = spi_geni->irq;
> + goto spi_geni_probe_unmap;
> + }
> +
> + spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
> + spi->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
> + spi->num_chipselect = SPI_NUM_CHIPSELECT;
> + spi->prepare_transfer_hardware = spi_geni_prepare_transfer_hardware;
> + spi->prepare_message = spi_geni_prepare_message;
> + spi->unprepare_message = spi_geni_unprepare_message;
> + spi->transfer_one = spi_geni_transfer_one;
> + spi->unprepare_transfer_hardware
> + = spi_geni_unprepare_transfer_hardware;
> + spi->auto_runtime_pm = false;
> +
> + init_completion(&spi_geni->xfer_done);
> + spin_lock_init(&spi_geni->lock);
> + pm_runtime_enable(&pdev->dev);
> + ret = spi_register_master(spi);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register SPI master\n");
Most likely spi_register_master() is going to print what went wrong so
this print is not useful.
> + goto spi_geni_probe_unmap;
> + }
> + dev_dbg(&pdev->dev, "%s Probed\n", __func__);
> + return ret;
> +spi_geni_probe_unmap:
> + devm_iounmap(&pdev->dev, se->base);
> +spi_geni_probe_err:
> + spi_master_put(spi);
> + return ret;
> +}
> +
[...]
> +
> +static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
> +{
> + int ret;
> + struct spi_master *spi = get_spi_master(dev);
> + struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
> +
> + ret = geni_se_resources_off(&spi_geni->se);
> + return ret;
return geni_se_resources_off();
> +}
> +
> +static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct spi_master *spi = get_spi_master(dev);
> + struct spi_geni_master *spi_geni = spi_master_get_devdata(spi);
> +
> + ret = geni_se_resources_on(&spi_geni->se);
> + return ret;
return geni_se_resources_on();
> +}
> +
> +static int __maybe_unused spi_geni_suspend(struct device *dev)
> +{
> + if (!pm_runtime_status_suspended(dev))
> + return -EBUSY;
> + return 0;
> +}
> diff --git a/include/linux/spi/spi-geni-qcom.h b/include/linux/spi/spi-geni-qcom.h
> new file mode 100644
> index 0000000..dc95764
> --- /dev/null
> +++ b/include/linux/spi/spi-geni-qcom.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
Why?
> +/*
> + * 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?
> + u32 spi_cs_clk_delay;
> + u32 spi_inter_words_delay;
> +};
> +
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-11 22:30 ` Stephen Boyd
@ 2018-05-21 15:52 ` Mahadevan, Girish
2018-05-22 16:46 ` Stephen Boyd
0 siblings, 1 reply; 33+ messages in thread
From: Mahadevan, Girish @ 2018-05-21 15:52 UTC (permalink / raw)
To: Stephen Boyd, broonie, linux-kernel, linux-spi
Cc: sdharia, kramasub, dianders, linux-arm-msm
Hi Stephen
On 5/11/2018 4:30 PM, Stephen Boyd wrote:
>> + if (mode & SPI_CPHA)
>> + cpha |= CPHA;
>> +
>> + if (spi_slv->mode & SPI_CS_HIGH)
>> + demux_output_inv |= BIT(spi_slv->chip_select);
>> +
>> + if (spi_slv->controller_data) {
>> + u32 cs_clk_delay = 0;
>> + u32 inter_words_delay = 0;
>> +
>> + delay_params =
>> + (struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
>> + cs_clk_delay =
>> + (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
>> + & SPI_CS_CLK_DELAY_MSK;
>> + inter_words_delay =
>> + delay_params->spi_inter_words_delay &
>> + SPI_INTER_WORDS_DELAY_MSK;
>> + spi_delay_params =
>> + (inter_words_delay | cs_clk_delay);
>> + }
>> +
>> + 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?
Not sure I follow, the intention is to run the controller clock based on
the slave's max frequency.
>> +
>> + ret = pm_runtime_get_sync(mas->dev);
>> + if (ret < 0) {
>> + dev_err(mas->dev, "Error enabling SE resources\n");
>> + pm_runtime_put_noidle(mas->dev);
>> + goto exit_prepare_transfer_hardware;
>> + } else {
>> + ret = 0;
>
> Does pm_runtime_get_sync() return anything besides 0 on success?
This will go away, since I will switch to using auto-runtime option
provided by the framework.
>
>> + }
>> +
>> + if (unlikely(!mas->setup)) {
>> + int proto = geni_se_read_proto(se);
>> + unsigned int major;
>> + unsigned int minor;
>> + unsigned int step;
>> +
>> + if (unlikely(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);
>> + geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
>
> Why 2? Drop extra parenthesis please.
This is what the hardware programming doc recommends, the parameter is
actually the rx_rfr_watermark, something that doesn't apply to non-UART
protocols.
(I will add a detailed comment)
an else if.
>
>> + m_param |= FRAGMENTATION;
>> + }
>> +
>> + 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?
No, writing to this register should be done after we enqueue the command
to the GENI engine(the geni_se_setup_m_cmd), but some of the transaction
properties (length etc) should be setup before we enqueue the GENI command.
setup_fifo_xfer(xfer, mas, slv->mode, spi);
>> + timeout = wait_for_completion_timeout(&mas->xfer_done,
>> + msecs_to_jiffies(SPI_XFER_TIMEOUT_MS));
>
> Can you implement the 'handle_err' for the controller and call
> spi_finalize_current_transfer() from the interrupt handler when the
> transfer completes? The completion variable stuff and timeout code in
> this driver can hopefully all go away.
Will do (thanks for the suggestion).
>
> More things can be unsigned?
>
>> + 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?
I did this for 2 reasons.
1.The core can handle different byte order transmissions (e.g MSB
first), I am quite honestly not sure how the client's will setup the
data buffer in these cases.(for bigger word sizes , 16/32)
[we plan to support these]
2. For non-byte aligned word sizes (e,g 9), I am not enabling FIFO word
packing (meaning 1 SPI-WORD/FIFO-WORD in these cases).
>> +++ b/include/linux/spi/spi-geni-qcom.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>
> Why?
>
>> +/*
>> + * 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?
The intention was to allow a client to specify slave specific timing
requirements, e.g CS-CLK delay (based on the slave's data sheet).
So that the client drivers could setup these delays and pass it in part
of the controller_data member of the spi_device data structure.
The header file was meant to expose these timing params that the client
could specify. I honestly didn't know how else a client could specify
these to the controller driver.
>
>> + u32 spi_cs_clk_delay;
>> + u32 spi_inter_words_delay;
>> +};
>> +
Best Regards
Girish
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-21 15:52 ` Mahadevan, Girish
@ 2018-05-22 16:46 ` Stephen Boyd
2018-05-22 17:30 ` Mark Brown
0 siblings, 1 reply; 33+ messages in thread
From: Stephen Boyd @ 2018-05-22 16:46 UTC (permalink / raw)
To: Mahadevan, Girish, broonie, linux-kernel, linux-spi
Cc: sdharia, kramasub, dianders, linux-arm-msm
Quoting Mahadevan, Girish (2018-05-21 08:52:47)
> Hi Stephen
>
> On 5/11/2018 4:30 PM, Stephen Boyd wrote:
>
> >> + if (mode & SPI_CPHA)
> >> + cpha |= CPHA;
> >> +
> >> + if (spi_slv->mode & SPI_CS_HIGH)
> >> + demux_output_inv |= BIT(spi_slv->chip_select);
> >> +
> >> + if (spi_slv->controller_data) {
> >> + u32 cs_clk_delay = 0;
> >> + u32 inter_words_delay = 0;
> >> +
> >> + delay_params =
> >> + (struct spi_geni_qcom_ctrl_data *) spi_slv->controller_data;
> >> + cs_clk_delay =
> >> + (delay_params->spi_cs_clk_delay << SPI_CS_CLK_DELAY_SHFT)
> >> + & SPI_CS_CLK_DELAY_MSK;
> >> + inter_words_delay =
> >> + delay_params->spi_inter_words_delay &
> >> + SPI_INTER_WORDS_DELAY_MSK;
> >> + spi_delay_params =
> >> + (inter_words_delay | cs_clk_delay);
> >> + }
> >> +
> >> + 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?
>
> Not sure I follow, the intention is to run the controller clock based on
> the slave's max frequency.
That's good. The problem I see is that we have to specify the max
frequency in the controller/bus node, and also in the child/slave node.
It should only need to be specified in the slave node, so making the
cur_speed_hz equal the max_speed_hz is problematic. The current speed of
the master should be determined by calling clk_get_rate().
> >
> >> + }
> >> +
> >> + if (unlikely(!mas->setup)) {
> >> + int proto = geni_se_read_proto(se);
> >> + unsigned int major;
> >> + unsigned int minor;
> >> + unsigned int step;
> >> +
> >> + if (unlikely(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);
> >> + geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2));
> >
> > Why 2? Drop extra parenthesis please.
>
> This is what the hardware programming doc recommends, the parameter is
> actually the rx_rfr_watermark, something that doesn't apply to non-UART
> protocols.
> (I will add a detailed comment)
Thanks. Just wanted to make sure we don't forget what the magic number
means.
> an else if.
> >
> >> + m_param |= FRAGMENTATION;
> >> + }
> >> +
> >> + 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?
>
> No, writing to this register should be done after we enqueue the command
> to the GENI engine(the geni_se_setup_m_cmd), but some of the transaction
> properties (length etc) should be setup before we enqueue the GENI command.
Ok.
>
> >
> > More things can be unsigned?
> >
> >> + 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?
>
> I did this for 2 reasons.
> 1.The core can handle different byte order transmissions (e.g MSB
> first), I am quite honestly not sure how the client's will setup the
> data buffer in these cases.(for bigger word sizes , 16/32)
> [we plan to support these]
> 2. For non-byte aligned word sizes (e,g 9), I am not enabling FIFO word
> packing (meaning 1 SPI-WORD/FIFO-WORD in these cases).
Alright. I'm not certain how the incoming buffer looks when clients
request certain modes. Have you tested out non-byte aligned word size
transfers?
>
>
> >> +++ b/include/linux/spi/spi-geni-qcom.h
> >> @@ -0,0 +1,14 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >
> > Why?
> >
> >> +/*
> >> + * 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?
>
> The intention was to allow a client to specify slave specific timing
> requirements, e.g CS-CLK delay (based on the slave's data sheet).
> So that the client drivers could setup these delays and pass it in part
> of the controller_data member of the spi_device data structure.
> The header file was meant to expose these timing params that the client
> could specify. I honestly didn't know how else a client could specify
> these to the controller driver.
Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
documented but don't seem to be used. There's also the delay_usecs part
of the spi_transfer structure, which may be what you're talking about.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-22 16:46 ` Stephen Boyd
@ 2018-05-22 17:30 ` Mark Brown
2018-05-24 16:25 ` Mahadevan, Girish
0 siblings, 1 reply; 33+ messages in thread
From: Mark Brown @ 2018-05-22 17:30 UTC (permalink / raw)
To: Stephen Boyd
Cc: Mahadevan, Girish, linux-kernel, linux-spi, sdharia, kramasub,
dianders, linux-arm-msm
[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]
On Tue, May 22, 2018 at 09:46:39AM -0700, Stephen Boyd wrote:
> Quoting Mahadevan, Girish (2018-05-21 08:52:47)
> > Not sure I follow, the intention is to run the controller clock based on
> > the slave's max frequency.
> That's good. The problem I see is that we have to specify the max
> frequency in the controller/bus node, and also in the child/slave node.
> It should only need to be specified in the slave node, so making the
> cur_speed_hz equal the max_speed_hz is problematic. The current speed of
> the master should be determined by calling clk_get_rate().
We don't require that the slaves all individually set a speed since it
gets a bit redundant, it should be enough to just use the default the
controller provides. A bigger problem with this is that the driver will
never see a transfer which doesn't explicitly have a speed set as the
core will ensure something is set, open coding this logic in every
driver would obviously be tiresome.
> > The intention was to allow a client to specify slave specific timing
> > requirements, e.g CS-CLK delay (based on the slave's data sheet).
> > So that the client drivers could setup these delays and pass it in part
> > of the controller_data member of the spi_device data structure.
> > The header file was meant to expose these timing params that the client
> > could specify. I honestly didn't know how else a client could specify
> > these to the controller driver.
> Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
> documented but don't seem to be used. There's also the delay_usecs part
> of the spi_transfer structure, which may be what you're talking about.
delay_usecs is for inter-transfer delays within a message rather than
after the initial chip select assert (it can be used to keep chip select
asserted for longer after the final transfer too). Obviously this is
also something that shouldn't be configured in a driver specific
fashion.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-22 17:30 ` Mark Brown
@ 2018-05-24 16:25 ` Mahadevan, Girish
2018-05-24 16:29 ` Mark Brown
0 siblings, 1 reply; 33+ messages in thread
From: Mahadevan, Girish @ 2018-05-24 16:25 UTC (permalink / raw)
To: Mark Brown, Stephen Boyd
Cc: linux-kernel, linux-spi, sdharia, kramasub, dianders, linux-arm-msm
Hi Mark, Stephen
On 5/22/2018 11:30 AM, Mark Brown wrote:
>> That's good. The problem I see is that we have to specify the max
>> frequency in the controller/bus node, and also in the child/slave node.
>> It should only need to be specified in the slave node, so making the
>> cur_speed_hz equal the max_speed_hz is problematic. The current speed of
>> the master should be determined by calling clk_get_rate().
>
> We don't require that the slaves all individually set a speed since it
> gets a bit redundant, it should be enough to just use the default the
> controller provides. A bigger problem with this is that the driver will
> never see a transfer which doesn't explicitly have a speed set as the
> core will ensure something is set, open coding this logic in every
> driver would obviously be tiresome.
Sorry , I need some more clarification.
When I register the master, I specify the max rate the core can support
(50 Mhz). So any transaction that comes to the driver is going to be
requesting at-most 50 Mhz.
The reason I have the cur_speed_hz is that there is a max_speed_hz which
is the max frequency the slave can do; but every transfer can also
specify a speed_hz and override this.
So my point is we can do upto 4 slaves on a given controller, each slave
can have a different max speed and each slave's transfer can override
the max_frequency of that slave device.
(the default frequency is the master's max frequency)
>> Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
>> documented but don't seem to be used. There's also the delay_usecs part
>> of the spi_transfer structure, which may be what you're talking about.
>
> delay_usecs is for inter-transfer delays within a message rather than
> after the initial chip select assert (it can be used to keep chip select
> asserted for longer after the final transfer too). Obviously this is
> also something that shouldn't be configured in a driver specific
> fashion.
>
Hmmm ok, so you mean don't send these as controller_data, rather add new
members to the spi_device struct ?
Best Regards
Girish
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-24 16:25 ` Mahadevan, Girish
@ 2018-05-24 16:29 ` Mark Brown
[not found] ` <28d8ab5fdeb34e52eba7ca771a17bc06@codeaurora.org>
0 siblings, 1 reply; 33+ messages in thread
From: Mark Brown @ 2018-05-24 16:29 UTC (permalink / raw)
To: Mahadevan, Girish
Cc: Stephen Boyd, linux-kernel, linux-spi, sdharia, kramasub,
dianders, linux-arm-msm
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
On Thu, May 24, 2018 at 10:25:58AM -0600, Mahadevan, Girish wrote:
> The reason I have the cur_speed_hz is that there is a max_speed_hz which
> is the max frequency the slave can do; but every transfer can also
> specify a speed_hz and override this.
Every transfer *will* specify a speed, you should never see a transfer
that doesn't specify a speed.
> > delay_usecs is for inter-transfer delays within a message rather than
> > after the initial chip select assert (it can be used to keep chip select
> > asserted for longer after the final transfer too). Obviously this is
> > also something that shouldn't be configured in a driver specific
> > fashion.
> Hmmm ok, so you mean don't send these as controller_data, rather add new
> members to the spi_device struct ?
Yes, that'd be one way to do it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
2018-05-03 21:34 [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Girish Mahadevan
2018-05-03 23:38 ` Mark Brown
2018-05-11 22:30 ` Stephen Boyd
@ 2018-06-08 23:34 ` Matthias Kaehlcke
2 siblings, 0 replies; 33+ messages in thread
From: Matthias Kaehlcke @ 2018-06-08 23:34 UTC (permalink / raw)
To: Girish Mahadevan
Cc: broonie, linux-kernel, linux-spi, sdharia, kramasub, dianders,
linux-arm-msm, swboyd, amstan
On Thu, May 03, 2018 at 03:34:43PM -0600, Girish Mahadevan wrote:
> 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 <girishm@codeaurora.org>
> ---
> drivers/spi/Kconfig | 12 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-geni-qcom.c | 766 ++++++++++++++++++++++++++++++++++++++
> include/linux/spi/spi-geni-qcom.h | 14 +
> 4 files changed, 793 insertions(+)
> create mode 100644 drivers/spi/spi-geni-qcom.c
> create mode 100644 include/linux/spi/spi-geni-qcom.h
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> new file mode 100644
> index 0000000..eecc634
> --- /dev/null
> +++ b/drivers/spi/spi-geni-qcom.c
>
> ...
>
> +static irqreturn_t geni_spi_isr(int irq, void *dev)
> +{
> + struct spi_geni_master *mas = dev;
> + struct geni_se *se = &mas->se;
> + u32 m_irq = 0;
> + irqreturn_t ret = IRQ_HANDLED;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mas->lock, flags);
> + if (pm_runtime_status_suspended(dev)) {
kasan is unhappy about geni_spi_isr:
[ 3.206593] BUG: KASAN: slab-out-of-bounds in geni_spi_isr+0x978/0xbf4
[ 3.213310] Read of size 4 at addr ffffffc0da803b04 by task swapper/0/1
[ 3.221664] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.47 #20
[ 3.227936] Hardware name: Google Cheza (DT)
[ 3.232341] Call trace:
[ 3.234884] [<ffffff90080925a4>] dump_backtrace+0x0/0x6d0
[ 3.240441] [<ffffff9008092cc8>] show_stack+0x20/0x2c
[ 3.245649] [<ffffff90094b53f8>] __dump_stack+0x20/0x28
[ 3.251034] [<ffffff90094b53b0>] dump_stack+0xcc/0xf4
[ 3.256240] [<ffffff90083cfb58>] print_address_description+0x70/0x238
[ 3.262868] [<ffffff90083d00ec>] kasan_report+0x1cc/0x260
[ 3.268425] [<ffffff90083d021c>] __asan_report_load4_noabort+0x2c/0x38
[ 3.275142] [<ffffff9008ca820c>] geni_spi_isr+0x978/0xbf4
...
[ 3.662568] Allocated by task 1:
[ 3.665908] kasan_kmalloc+0xb4/0x174
[ 3.669693] __kmalloc+0x260/0x2f4
[ 3.673201] __spi_alloc_controller+0x38/0x180
[ 3.677781] spi_geni_probe+0x38/0x574
[ 3.681647] platform_drv_probe+0xac/0x134
[ 3.685865] driver_probe_device+0x470/0x4f4
[ 3.690268] __driver_attach+0xe8/0x128
[ 3.694228] bus_for_each_dev+0x104/0x16c
[ 3.698356] driver_attach+0x48/0x54
[ 3.702052] bus_add_driver+0x258/0x3d0
[ 3.706010] driver_register+0x1ac/0x230
[ 3.710056] __platform_driver_register+0xcc/0xdc
[ 3.714906] spi_geni_driver_init+0x1c/0x24
[ 3.719220] do_one_initcall+0x22c/0x3c4
[ 3.723266] kernel_init_freeable+0x31c/0x40c
[ 3.727753] kernel_init+0x14/0x10c
[ 3.731349] ret_from_fork+0x10/0x18
Reason is that 'dev' is passed to pm_runtime_status_suspended(), when
it should be 'mas->dev'.
As this bug indicates kernel developers have strong expectations what
a variable called 'dev' represents, I suggest to change it to
something like 'data'.
Thanks
Matthias
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-08-24 11:00 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-03 21:34 [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Girish Mahadevan
2018-05-03 23:38 ` Mark Brown
2018-05-07 21:40 ` Mahadevan, Girish
[not found] ` <0c26e96c-85ad-c2a2-9abd-33096d76008b@codeaurora.org>
2018-05-17 7:21 ` Mark Brown
2018-05-21 21:45 ` Mahadevan, Girish
2018-05-22 17:32 ` Mark Brown
2018-05-11 22:30 ` Stephen Boyd
2018-05-21 15:52 ` Mahadevan, Girish
2018-05-22 16:46 ` Stephen Boyd
2018-05-22 17:30 ` Mark Brown
2018-05-24 16:25 ` Mahadevan, Girish
2018-05-24 16:29 ` Mark Brown
[not found] ` <28d8ab5fdeb34e52eba7ca771a17bc06@codeaurora.org>
2018-08-03 12:18 ` dkota
2018-08-09 18:03 ` Doug Anderson
2018-08-09 18:24 ` Trent Piepho
2018-08-09 19:37 ` Doug Anderson
2018-08-10 18:43 ` Trent Piepho
2018-08-10 10:52 ` Mark Brown
2018-08-10 15:40 ` Doug Anderson
2018-08-10 16:13 ` Mark Brown
2018-08-10 16:27 ` Doug Anderson
2018-08-10 16:43 ` Mark Brown
2018-08-10 16:47 ` Doug Anderson
2018-08-10 16:29 ` dkota
2018-08-10 16:46 ` Mark Brown
2018-08-14 9:00 ` dkota
2018-08-14 15:03 ` Mark Brown
2018-08-17 10:36 ` dkota
2018-08-17 14:59 ` Mark Brown
2018-08-24 11:00 ` dkota
2018-08-10 16:49 ` Doug Anderson
2018-08-10 17:55 ` Trent Piepho
2018-06-08 23:34 ` Matthias Kaehlcke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).