* [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP [not found] <1532087461-3474-1-git-send-email-dkota@codeaurora.org> @ 2018-07-20 11:50 ` Dilip Kota 2018-07-20 22:45 ` kbuild test robot ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Dilip Kota @ 2018-07-20 11:50 UTC (permalink / raw) To: timur, swboyd, broonie, mka, linux-kernel, linux-spi Cc: linux-arm-msm, Girish Mahadevan, Karthikeyan Ramasubramanian, Sagar Dharia, Dilip Kota From: Girish Mahadevan <girishm@codeaurora.org> This driver supports GENI based SPI Controller in the Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable module supporting a wide range of serial interfaces including SPI. This driver supports SPI operations using FIFO mode of transfer. Change-Id: Ib2c7feba5a2fc4015bc83319b233b4356f7d3190 Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> 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> Signed-off-by: Dilip Kota <dkota@codeaurora.org> --- drivers/spi/Kconfig | 12 + drivers/spi/Makefile | 1 + drivers/spi/spi-geni-qcom.c | 685 ++++++++++++++++++++++++++++++++++++++ include/linux/spi/spi-geni-qcom.h | 14 + 4 files changed, 712 insertions(+) create mode 100644 drivers/spi/spi-geni-qcom.c create mode 100644 include/linux/spi/spi-geni-qcom.h diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index ad5d68e..d4aa755 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -533,6 +533,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 QCOM_GENI_SE + help + This driver supports GENI serial engine based SPI controller in + master mode on the Qualcomm Technologies Inc.'s SoCs. If you say + yes to this option, support will be included for the built-in SPI + 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 cb1f437..98337cf 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -74,6 +74,7 @@ obj-$(CONFIG_SPI_PPC4xx) += spi-ppc4xx.o 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_QCOM_GENI) += spi-geni-qcom.o obj-$(CONFIG_SPI_QUP) += spi-qup.o obj-$(CONFIG_SPI_ROCKCHIP) += spi-rockchip.o obj-$(CONFIG_SPI_RB4XX) += spi-rb4xx.o diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c new file mode 100644 index 0000000..95381b8 --- /dev/null +++ b/drivers/spi/spi-geni-qcom.c @@ -0,0 +1,685 @@ +// 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/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/qcom-geni-se.h> +#include <linux/spi/spi.h> +#include <linux/spi/spi-geni-qcom.h> + +/* SPI SE specific registers and respective register fields */ +#define SE_SPI_CPHA 0x224 +#define CPHA BIT(0) + +#define SE_SPI_LOOPBACK 0x22c +#define LOOPBACK_ENABLE 0x1 +#define NORMAL_MODE 0x0 +#define LOOPBACK_MSK GENMASK(1, 0) + +#define SE_SPI_CPOL 0x230 +#define CPOL BIT(2) + +#define SE_SPI_DEMUX_OUTPUT_INV 0x24c +#define CS_DEMUX_OUTPUT_INV_MSK GENMASK(3, 0) + +#define SE_SPI_DEMUX_SEL 0x250 +#define CS_DEMUX_OUTPUT_SEL GENMASK(3, 0) + +#define SE_SPI_TRANS_CFG 0x25c +#define CS_TOGGLE BIT(0) + +#define SE_SPI_WORD_LEN 0x268 +#define WORD_LEN_MSK GENMASK(9, 0) +#define MIN_WORD_LEN 4 + +#define SE_SPI_TX_TRANS_LEN 0x26c +#define SE_SPI_RX_TRANS_LEN 0x270 +#define TRANS_LEN_MSK GENMASK(23, 0) + +#define SE_SPI_PRE_POST_CMD_DLY 0x274 + +#define SE_SPI_DELAY_COUNTERS 0x278 +#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 *data); + +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; + unsigned int tx_rem_bytes; + unsigned int rx_rem_bytes; + struct spi_transfer *cur_xfer; + struct completion xfer_done; + int oversampling; +}; + +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); + return ret; + } + + 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); + 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_prepare_transfer_hardware(struct spi_master *spi) +{ + struct spi_geni_master *mas = spi_master_get_devdata(spi); + struct geni_se *se = &mas->se; + + if (!mas->setup) { + int proto = geni_se_read_proto(se); + unsigned int major; + unsigned int minor; + unsigned int step; + + if (proto != GENI_SE_SPI) { + dev_err(mas->dev, "Invalid proto %d\n", proto); + return -ENXIO; + } + mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se); + mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se); + mas->tx_fifo_width = geni_se_get_tx_fifo_width(se); + 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; + } + 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_master *spi, + struct spi_message *msg) +{ + struct spi_geni_master *mas = spi_master_get_devdata(spi); + unsigned long timeout; + struct geni_se *se = &mas->se; + + reinit_completion(&mas->xfer_done); + 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"); + } +} + +static int spi_geni_transfer_one(struct spi_master *spi, + struct spi_device *slv, + struct spi_transfer *xfer) +{ + struct spi_geni_master *mas = spi_master_get_devdata(spi); + + setup_fifo_xfer(xfer, mas, slv->mode, spi); + return 1; +} + +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 *data) +{ + struct spi_master *spi = data; + struct spi_geni_master *mas = spi_master_get_devdata(spi); + struct geni_se *se = &mas->se; + u32 m_irq = 0; + irqreturn_t ret = IRQ_HANDLED; + + if (pm_runtime_status_suspended(mas->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) { + spi_finalize_current_transfer(spi); + /* + * 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); + } + + if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN)) + complete(&mas->xfer_done); +exit_geni_spi_irq: + writel_relaxed(m_irq, se->base + SE_GENI_M_IRQ_CLEAR); + 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 = -1; + 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; + } + ret = devm_request_irq(&pdev->dev, spi_geni->irq, geni_spi_isr, + IRQF_TRIGGER_HIGH, "spi_geni", spi); + if (ret) { + dev_err(&pdev->dev, "Request_irq failed:%d: err:%d\n", + spi_geni->irq, ret); + 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 = 4; + spi->prepare_transfer_hardware = spi_geni_prepare_transfer_hardware; + spi->prepare_message = spi_geni_prepare_message; + spi->transfer_one = spi_geni_transfer_one; + spi->auto_runtime_pm = true; + spi->handle_err = handle_fifo_timeout; + + init_completion(&spi_geni->xfer_done); + pm_runtime_enable(&pdev->dev); + ret = devm_spi_register_master(&pdev->dev, spi); + if (ret) { + dev_err(&pdev->dev, "Failed to register SPI master\n"); + 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); + + 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) +{ + struct spi_master *spi = dev_get_drvdata(dev); + struct spi_geni_master *spi_geni = spi_master_get_devdata(spi); + + return geni_se_resources_off(&spi_geni->se); +} + +static int __maybe_unused spi_geni_runtime_resume(struct device *dev) +{ + struct spi_master *spi = dev_get_drvdata(dev); + struct spi_geni_master *spi_geni = spi_master_get_devdata(spi); + + return geni_se_resources_on(&spi_geni->se); +} + +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___*/ -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP 2018-07-20 11:50 ` [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Dilip Kota @ 2018-07-20 22:45 ` kbuild test robot 2018-07-20 22:53 ` Doug Anderson 2018-07-20 23:37 ` kbuild test robot 2 siblings, 0 replies; 6+ messages in thread From: kbuild test robot @ 2018-07-20 22:45 UTC (permalink / raw) To: Dilip Kota Cc: kbuild-all, timur, swboyd, broonie, mka, linux-kernel, linux-spi, linux-arm-msm, Girish Mahadevan, Karthikeyan Ramasubramanian, Sagar Dharia, Dilip Kota [-- Attachment #1: Type: text/plain, Size: 2883 bytes --] Hi Girish, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on spi/for-next] [also build test WARNING on v4.18-rc5 next-20180720] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Dilip-Kota/spi-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next config: i386-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): In file included from drivers/spi/spi-geni-qcom.c:11:0: drivers/spi/spi-geni-qcom.c: In function 'spi_geni_prepare_transfer_hardware': include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'? step = version & HW_VER_STEP_MASK; \ ^ >> drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version' geni_se_get_wrapper_version(se, major, minor, step); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/qcom-geni-se.h:238:9: note: each undeclared identifier is reported only once for each function it appears in step = version & HW_VER_STEP_MASK; \ ^ >> drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version' geni_se_get_wrapper_version(se, major, minor, step); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/geni_se_get_wrapper_version +256 drivers/spi/spi-geni-qcom.c 233 234 static int spi_geni_prepare_transfer_hardware(struct spi_master *spi) 235 { 236 struct spi_geni_master *mas = spi_master_get_devdata(spi); 237 struct geni_se *se = &mas->se; 238 239 if (!mas->setup) { 240 int proto = geni_se_read_proto(se); 241 unsigned int major; 242 unsigned int minor; 243 unsigned int step; 244 245 if (proto != GENI_SE_SPI) { 246 dev_err(mas->dev, "Invalid proto %d\n", proto); 247 return -ENXIO; 248 } 249 mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se); 250 mas->rx_fifo_depth = geni_se_get_rx_fifo_depth(se); 251 mas->tx_fifo_width = geni_se_get_tx_fifo_width(se); 252 geni_se_init(se, 0x0, (mas->tx_fifo_depth - 2)); 253 mas->oversampling = 1; 254 /* Transmit an entire FIFO worth of data per IRQ */ 255 mas->tx_wm = 1; > 256 geni_se_get_wrapper_version(se, major, minor, step); 257 if ((major == 1) && (minor == 0)) 258 mas->oversampling = 2; 259 mas->setup = 1; 260 } 261 return 0; 262 } 263 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 64141 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP 2018-07-20 11:50 ` [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Dilip Kota 2018-07-20 22:45 ` kbuild test robot @ 2018-07-20 22:53 ` Doug Anderson 2018-07-20 23:32 ` Doug Anderson 2018-07-20 23:37 ` kbuild test robot 2 siblings, 1 reply; 6+ messages in thread From: Doug Anderson @ 2018-07-20 22:53 UTC (permalink / raw) To: Dilip Kota Cc: Timur Tabi, Stephen Boyd, Mark Brown, Matthias Kaehlcke, LKML, linux-spi, linux-arm-msm, Girish Mahadevan, Karthikeyan Ramasubramanian, Sagar Dharia >Hi, On Fri, Jul 20, 2018 at 4:50 AM, Dilip Kota <dkota@codeaurora.org> wrote: > From: Girish Mahadevan <girishm@codeaurora.org> > > This driver supports GENI based SPI Controller in the Qualcomm SOCs. The > Qualcomm Generic Interface (GENI) is a programmable module supporting a > wide range of serial interfaces including SPI. This driver supports SPI > operations using FIFO mode of transfer. > > Change-Id: Ib2c7feba5a2fc4015bc83319b233b4356f7d3190 > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> Why is Girish here twice? That doesn't seem right. This is also an awful lot of Signed-off-by tags. Did they all contribute code here? v1 of this patch only had girish. > Reviewed-by: Rob Herring <robh@kernel.org> Rob may have reviewed the bindings, but that doesn't mean he's happy with the code. I'd suggest removing his Reviewed-by > Reviewed-by: Stephen Boyd <swboyd@chromium.org> Are you certain that Stephen provided his Reviewed-by? I don't see it. ...and, as you can see below, it seems like you ignored most of Stephen's comments on v1. > Signed-off-by: Dilip Kota <dkota@codeaurora.org> > --- I would have expected some version history here to say what changed from v1 to v2. See my comments on the cover letter and consider using patman <https://github.com/u-boot/u-boot/tree/master/tools/patman> to help you. > +config SPI_QCOM_GENI > + tristate "Qualcomm SPI controller with QUP interface" On v1 Stephen Boyd said: > 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? More explicitly, please change the line to: tristate "Qualcomm SPI controller with GENI interface" > + depends on QCOM_GENI_SE > + help > + This driver supports GENI serial engine based SPI controller in > + master mode on the Qualcomm Technologies Inc.'s SoCs. If you say > + yes to this option, support will be included for the built-in SPI On v1 Stephen Boyd said: > Drop "built-in"? > +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; In v1 Stephen said: > All of these can be unsigned ints? > + bool setup; > + u32 cur_speed_hz; > + int cur_word_len; In v1 Stephen said: > unsigned? > + unsigned int tx_rem_bytes; > + unsigned int rx_rem_bytes; > + struct spi_transfer *cur_xfer; > + struct completion xfer_done; > + int oversampling; In v1 Stephen said: > unsigned? > +static int get_spi_clk_cfg(u32 speed_hz, struct spi_geni_master *mas, In v1 Stephen said: > Why a u32? And not unsigned int? > + if (ret) { > + dev_err(mas->dev, "%s: Failed(%d) to find src clk for 0x%x\n", > + __func__, ret, speed_hz); In v1 Stephen said: > Frequency Hz printed in hex? I am not a hex computer! I hope! ...I'll further add that printing __func__ is generally discouraged for dev_xx printouts. They've already got the device name and that together with the string should be enough. Remove it. > +static void spi_setup_word_len(struct spi_geni_master *mas, u32 mode, In v1 Stephen said: > 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. > +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; In v1 Stephen said: > Don't initialize variables and then overwrite them. > + demux_sel = spi_slv->chip_select; > + mas->cur_speed_hz = spi_slv->max_speed_hz; In v1 Stephen said: > 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? ...there was a bunch of arguments here and I'm not sure I followed all of them, but the net-net is that there should be no rate at the controller level. Each SPI slave should request its rate and that should take effect for each transfer. Basically just honor the rate of the transfer in setup_fifo_xfer and be done with it. > + mas->cur_word_len = spi_slv->bits_per_word; > + > + ret = get_spi_clk_cfg(mas->cur_speed_hz, mas, &idx, &div); > + if (ret) { > + dev_err(mas->dev, "Err setting clks ret(%d) for %d\n", > + ret, mas->cur_speed_hz); > + return ret; > + } > + > + clk_sel |= (idx & CLK_SEL_MSK); In v1 Stephen said: > 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); In v1 Stephen said: > Same story. > + 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)); In v1 Stephen said: > Why 2? Drop extra parenthesis please. Girish responded that the 2 is important and that he would add a detailed comment, which I don't see. I also still see parenthesis. > + 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); In v1 Stephen said: > Drop extra parenthesis. I'll further add that I'd prefer it if you based your series atop <https://patchwork.kernel.org/patch/10412417/> which changes the prototype here. > + /* > + * 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)) In v1 Stephen said: > Combine else and if here into an else if. > + 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); In v1 Stephen said: > This can't be combined with above m_cmd & SPI_TX_ONLY statement? Girish said it can't. ...but IMO if it really can't then please add a comment in the code so someone doesn't later go back and try to combine. > +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. In v1 Stephen said: > 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)) In v1 Stephen said: > 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; In v1 Stephen said: > 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); In v1 Stephen said: > Drop parenthesis. > + max_bytes = min_t(int, mas->tx_rem_bytes, max_bytes); In v1 Stephen said: > 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)) In v1 Stephen said: > 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); In v1 Stephen said: > More things can be unsigned? > + fifo_byte = (u8 *)&fifo_word; > + for (j = 0; j < bytes_to_write; j++) > + fifo_byte[j] = tx_buf[i++]; In v1 Stephen said: > 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? Girish had a response to this and Stephen had another question which was never answered. Specifically he asked: > Have you tested out non-byte aligned word size transfers? Please answer and also add a comment justifying why we're doing all this work to fill a fifo a byte at a time. > +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; In v1 Stephen said: > 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. > + /* > + * 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); In v1 Stephen said: > min_t is preferably avoided. > + rx_buf += (mas->cur_xfer->len - mas->rx_rem_bytes); In v1 Stephen said: > 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)) In v1 Stephen said: > Drop parenthesis. > +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"); In v1 Stephen said: > We don't need allocation error messages. > + platform_set_drvdata(pdev, spi); > + spi_geni = spi_master_get_devdata(spi); > + spi_geni->dev = &pdev->dev; > + spi_geni->se.dev = &pdev->dev; > + spi_geni->se.wrapper = dev_get_drvdata(pdev->dev.parent); > + se = &spi_geni->se; > + > + spi->bus_num = -1; > + spi->dev.of_node = pdev->dev.of_node; In v1 Mark Brown said: > This is broken, the virtual SPI device does not exist in DT and this > might break things. ...though I'm as confused as Girish was. It seems like all the drivers do what you're doing... I didn't dig into the code though. > + 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)) { In v1 Stephen said: > Why does this need to come from DT? Please remove this concept. No other SPI driver has it. > + 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"); In v1 Stephen said: > We don't need these error messages. devm_ioremap_resource() already does > it. > + init_completion(&spi_geni->xfer_done); > + pm_runtime_enable(&pdev->dev); > + ret = devm_spi_register_master(&pdev->dev, spi); > + if (ret) { > + dev_err(&pdev->dev, "Failed to register SPI master\n"); In v1 Stephen said: > Most likely spi_register_master() is going to print what went wrong so > this print is not useful. > +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); > + > + geni_se_resources_off(&spi_geni->se); Why would you need to call geni_se_resources_off()? Isn't it handled by pm_runtime? > + pm_runtime_put_noidle(&pdev->dev); I'm curious: why would you have a "put" here? Won't that result in an imbalance since you don't exit probe with "get"? ...or did pm_runtime throw me for a loop again? > 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 */ In v1 Stephen said: > 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 { In v1 Stephen said: > What's the point of this header file and structure? This driver supports > board files? Isn't everything DT now? There was a bunch of discussion in followup threads but no closure and I still don't understand why we need this. Please remove it. One note is that Girish seemed to justify this by saying it's useful for "inter-transfer delays within a message". Isn't this just the "delay_usecs" within a transfer? I think the answer here is to remove the header file and structure and if you can later show where it's used then we can always add it back in. > + u32 spi_cs_clk_delay; > + u32 spi_inter_words_delay; > +}; > + > +#endif /*__SPI_GENI_QCOM_HEADER___*/ > -- I'm not sure where to comment about this, so adding it to the end: Between v1 and v2 you totally removed all the locking. Presumably this is because you didn't want to hold the lock in handle_fifo_timeout() while waiting for the completion. IMO taking the lock out was the wrong thing to do. You should keep it, but just drop the lock before wait_for_completion_timeout() and add it back afterwards. Specifically you _don't_ want the IRQ and timeout code stomping on each other. --- I'll also mention that you just wasted quite a bit of reviewer resources for me to repeat the refrain "you didn't address comments from v1". Reviewer time is precious. It's better that you wasted my time than Mark's, but there are still better things for me to do. Please be more careful. I realize that this is your first patch posted but ignoring previous feedback is not a good start. If you want to see the v1 discussion, you can easily find it at: https://patchwork.kernel.org/patch/10379299/ ...since I spent so much time checking over what you did vs. v1, I didn't actually have a chance to review v2 on its own merits. -Doug ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP 2018-07-20 22:53 ` Doug Anderson @ 2018-07-20 23:32 ` Doug Anderson 0 siblings, 0 replies; 6+ messages in thread From: Doug Anderson @ 2018-07-20 23:32 UTC (permalink / raw) To: Dilip Kota Cc: Timur Tabi, Stephen Boyd, Mark Brown, Matthias Kaehlcke, LKML, linux-spi, linux-arm-msm, Girish Mahadevan, Karthikeyan Ramasubramanian, Sagar Dharia Hi, On Fri, Jul 20, 2018 at 3:53 PM, Doug Anderson <dianders@chromium.org> wrote: >> + 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); > > In v1 Stephen said: > >> Drop extra parenthesis. > > I'll further add that I'd prefer it if you based your series atop > <https://patchwork.kernel.org/patch/10412417/> which changes the > prototype here. Whoops. Extra parenthesis was actually referring to the line below this one, AKA: > + if ((major == 1) && (minor == 0)) > + mas->oversampling = 2; ...that still applies. ...but then ignore my comment suggesting that you base your series atop Stephen's. You already did that. Certainly you should mention somewhere in your email that your patch is based on his. -Doug ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP 2018-07-20 11:50 ` [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Dilip Kota 2018-07-20 22:45 ` kbuild test robot 2018-07-20 22:53 ` Doug Anderson @ 2018-07-20 23:37 ` kbuild test robot 2018-07-23 10:27 ` dkota 2 siblings, 1 reply; 6+ messages in thread From: kbuild test robot @ 2018-07-20 23:37 UTC (permalink / raw) To: Dilip Kota Cc: kbuild-all, timur, swboyd, broonie, mka, linux-kernel, linux-spi, linux-arm-msm, Girish Mahadevan, Karthikeyan Ramasubramanian, Sagar Dharia, Dilip Kota [-- Attachment #1: Type: text/plain, Size: 3937 bytes --] Hi Girish, Thank you for the patch! Yet something to improve: [auto build test ERROR on spi/for-next] [also build test ERROR on v4.18-rc5 next-20180720] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Dilip-Kota/spi-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 8.1.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=8.1.0 make.cross ARCH=ia64 All errors (new ones prefixed by >>): In file included from drivers/spi/spi-geni-qcom.c:11: drivers/spi/spi-geni-qcom.c: In function 'spi_geni_prepare_transfer_hardware': >> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'? step = version & HW_VER_STEP_MASK; \ ^~~~~~~ drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version' geni_se_get_wrapper_version(se, major, minor, step); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/qcom-geni-se.h:238:9: note: each undeclared identifier is reported only once for each function it appears in step = version & HW_VER_STEP_MASK; \ ^~~~~~~ drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version' geni_se_get_wrapper_version(se, major, minor, step); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ -- In file included from drivers//spi/spi-geni-qcom.c:11: drivers//spi/spi-geni-qcom.c: In function 'spi_geni_prepare_transfer_hardware': >> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared (first use in this function); did you mean 'vm_region'? step = version & HW_VER_STEP_MASK; \ ^~~~~~~ drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version' geni_se_get_wrapper_version(se, major, minor, step); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/qcom-geni-se.h:238:9: note: each undeclared identifier is reported only once for each function it appears in step = version & HW_VER_STEP_MASK; \ ^~~~~~~ drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro 'geni_se_get_wrapper_version' geni_se_get_wrapper_version(se, major, minor, step); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +238 include/linux/qcom-geni-se.h eddac5af Karthikeyan Ramasubramanian 2018-03-30 231 eddac5af Karthikeyan Ramasubramanian 2018-03-30 232 #define geni_se_get_wrapper_version(se, major, minor, step) do { \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 233 u32 ver; \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 234 \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 235 ver = geni_se_get_qup_hw_version(se); \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 236 major = (ver & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 237 minor = (ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 @238 step = version & HW_VER_STEP_MASK; \ eddac5af Karthikeyan Ramasubramanian 2018-03-30 239 } while (0) eddac5af Karthikeyan Ramasubramanian 2018-03-30 240 :::::: The code at line 238 was first introduced by commit :::::: eddac5af06546d2e7a0730e3dc02dde3dc91098a soc: qcom: Add GENI based QUP Wrapper driver :::::: TO: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> :::::: CC: Andy Gross <andy.gross@linaro.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 50909 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP 2018-07-20 23:37 ` kbuild test robot @ 2018-07-23 10:27 ` dkota 0 siblings, 0 replies; 6+ messages in thread From: dkota @ 2018-07-23 10:27 UTC (permalink / raw) To: kbuild-all, timur, swboyd, broonie, mka, linux-kernel, linux-spi, linux-arm-msm, Doug Anderson, alokc On 2018-07-21 05:07, kbuild test robot wrote: > Hi Girish, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on spi/for-next] > [also build test ERROR on v4.18-rc5 next-20180720] > [if your patch is applied to the wrong git tree, please drop us a note > to help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Dilip-Kota/spi-spi-geni-qcom-Add-SPI-driver-support-for-GENI-based-QUP/20180721-034916 > base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git > for-next > config: ia64-allmodconfig (attached as .config) > compiler: ia64-linux-gcc (GCC) 8.1.0 > reproduce: > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross > -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=8.1.0 make.cross ARCH=ia64 > > All errors (new ones prefixed by >>): > > In file included from drivers/spi/spi-geni-qcom.c:11: > drivers/spi/spi-geni-qcom.c: In function > 'spi_geni_prepare_transfer_hardware': >>> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared >>> (first use in this function); did you mean 'vm_region'? > step = version & HW_VER_STEP_MASK; \ > ^~~~~~~ > drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro > 'geni_se_get_wrapper_version' > geni_se_get_wrapper_version(se, major, minor, step); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/qcom-geni-se.h:238:9: note: each undeclared > identifier is reported only once for each function it appears in > step = version & HW_VER_STEP_MASK; \ > ^~~~~~~ > drivers/spi/spi-geni-qcom.c:256:3: note: in expansion of macro > 'geni_se_get_wrapper_version' > geni_se_get_wrapper_version(se, major, minor, step); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > -- > In file included from drivers//spi/spi-geni-qcom.c:11: > drivers//spi/spi-geni-qcom.c: In function > 'spi_geni_prepare_transfer_hardware': >>> include/linux/qcom-geni-se.h:238:9: error: 'version' undeclared >>> (first use in this function); did you mean 'vm_region'? > step = version & HW_VER_STEP_MASK; \ > ^~~~~~~ > drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro > 'geni_se_get_wrapper_version' > geni_se_get_wrapper_version(se, major, minor, step); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/qcom-geni-se.h:238:9: note: each undeclared > identifier is reported only once for each function it appears in > step = version & HW_VER_STEP_MASK; \ > ^~~~~~~ > drivers//spi/spi-geni-qcom.c:256:3: note: in expansion of macro > 'geni_se_get_wrapper_version' > geni_se_get_wrapper_version(se, major, minor, step); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > vim +238 include/linux/qcom-geni-se.h > > eddac5af Karthikeyan Ramasubramanian 2018-03-30 231 > eddac5af Karthikeyan Ramasubramanian 2018-03-30 232 #define > geni_se_get_wrapper_version(se, major, minor, step) do { \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 233 u32 ver; \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 234 \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 235 ver = > geni_se_get_qup_hw_version(se); \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 236 major = (ver & > HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT; \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 237 minor = (ver & > HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT; \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 @238 step = version > & HW_VER_STEP_MASK; \ > eddac5af Karthikeyan Ramasubramanian 2018-03-30 239 } while (0) > eddac5af Karthikeyan Ramasubramanian 2018-03-30 240 > > :::::: The code at line 238 was first introduced by commit > :::::: eddac5af06546d2e7a0730e3dc02dde3dc91098a soc: qcom: Add GENI > based QUP Wrapper driver > > :::::: TO: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > :::::: CC: Andy Gross <andy.gross@linaro.org> > > --- > 0-DAY kernel test infrastructure Open Source Technology > Center > https://lists.01.org/pipermail/kbuild-all Intel > Corporation HI, For now please ignore the PATCHSET 2. I will upload a new PATCHSET addressing all the comments of patchset 1 with detailed description. Sorry for the inconvenience. Regards, Dilip ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-23 10:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1532087461-3474-1-git-send-email-dkota@codeaurora.org>
2018-07-20 11:50 ` [PATCH V2] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP Dilip Kota
2018-07-20 22:45 ` kbuild test robot
2018-07-20 22:53 ` Doug Anderson
2018-07-20 23:32 ` Doug Anderson
2018-07-20 23:37 ` kbuild test robot
2018-07-23 10:27 ` dkota
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).