| Message ID | 1515805547-22816-6-git-send-email-kramasub@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote: > This bus driver supports the GENI based i2c hardware controller in the > Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable > module supporting a wide range of serial interfaces including I2C. The > driver supports FIFO mode and DMA mode of transfer and switches modes > dynamically depending on the size of the transfer. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > drivers/i2c/busses/Kconfig | 10 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 667 insertions(+) > create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 009345d..caef309 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -838,6 +838,16 @@ config I2C_PXA_SLAVE > is necessary for systems where the PXA may be a target on the > I2C bus. > > +config I2C_QCOM_GENI > + tristate "Qualcomm Technologies Inc.'s GENI based I2C controller" > + depends on ARCH_QCOM Just depend on the GENI wrapper as well. > + help > + If you say yes to this option, support will be included for the > + built-in I2C interface on the Qualcomm Technologies Inc.'s SoCs. > + > + This driver can also be built as a module. If so, the module > + will be called i2c-qcom-geni. > + > config I2C_QUP > tristate "Qualcomm QUP based I2C controller" > depends on ARCH_QCOM > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 2ce8576..201fce1 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o > obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o > obj-$(CONFIG_I2C_PXA) += i2c-pxa.o > obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o > +obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o > obj-$(CONFIG_I2C_QUP) += i2c-qup.o > obj-$(CONFIG_I2C_RIIC) += i2c-riic.o > obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > new file mode 100644 > index 0000000..59ad4da > --- /dev/null > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -0,0 +1,656 @@ > +/* > + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ Use SPDX license header. > + > +#include <linux/clk.h> > +#include <linux/delay.h> Unused? > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/dma-mapping.h> > +#include <linux/qcom-geni-se.h> > + > +#define SE_I2C_TX_TRANS_LEN (0x26C) Drop the parenthesis, when not needed. > +#define SE_I2C_RX_TRANS_LEN (0x270) > +#define SE_I2C_SCL_COUNTERS (0x278) > +#define SE_GENI_IOS (0x908) > + > +#define SE_I2C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\ > + M_GP_IRQ_1_EN | M_GP_IRQ_3_EN | M_GP_IRQ_4_EN) > +#define SE_I2C_ABORT (1U << 1) > +/* M_CMD OP codes for I2C */ > +#define I2C_WRITE (0x1) > +#define I2C_READ (0x2) > +#define I2C_WRITE_READ (0x3) > +#define I2C_ADDR_ONLY (0x4) > +#define I2C_BUS_CLEAR (0x6) > +#define I2C_STOP_ON_BUS (0x7) > +/* M_CMD params for I2C */ > +#define PRE_CMD_DELAY (BIT(0)) > +#define TIMESTAMP_BEFORE (BIT(1)) > +#define STOP_STRETCH (BIT(2)) > +#define TIMESTAMP_AFTER (BIT(3)) > +#define POST_COMMAND_DELAY (BIT(4)) > +#define IGNORE_ADD_NACK (BIT(6)) > +#define READ_FINISHED_WITH_ACK (BIT(7)) > +#define BYPASS_ADDR_PHASE (BIT(8)) > +#define SLV_ADDR_MSK (GENMASK(15, 9)) > +#define SLV_ADDR_SHFT (9) > + > +#define I2C_CORE2X_VOTE (10000) > +#define GP_IRQ0 0 > +#define GP_IRQ1 1 > +#define GP_IRQ2 2 > +#define GP_IRQ3 3 > +#define GP_IRQ4 4 > +#define GP_IRQ5 5 > +#define GENI_OVERRUN 6 > +#define GENI_ILLEGAL_CMD 7 > +#define GENI_ABORT_DONE 8 > +#define GENI_TIMEOUT 9 > + > +#define I2C_NACK GP_IRQ1 > +#define I2C_BUS_PROTO GP_IRQ3 > +#define I2C_ARB_LOST GP_IRQ4 > +#define DM_I2C_CB_ERR ((BIT(GP_IRQ1) | BIT(GP_IRQ3) | BIT(GP_IRQ4)) \ > + << 5) > + > +#define I2C_AUTO_SUSPEND_DELAY 250 > +#define KHz(freq) (1000 * freq) > + > +struct geni_i2c_dev { > + struct device *dev; > + void __iomem *base; > + unsigned int tx_wm; > + int irq; > + int err; > + struct i2c_adapter adap; > + struct completion xfer; How about naming this "done" or something like that, gi2c->xfer doesn't really give the sense of being a "we're done with the operation"-event. > + struct i2c_msg *cur; > + struct geni_se_rsc i2c_rsc; > + int cur_wr; > + int cur_rd; > + struct device *wrapper_dev; This is already availabe in i2c_rsc, and in particular if you pass the i2c_rsc down to the wrapper in the 2 cases you use the wrapper_dev you don't need this duplication. > + u32 clk_freq_out; > + int clk_fld_idx; Keep track of the const struct geni_i2c_clk_fld * here instead. > +}; > + > +struct geni_i2c_err_log { > + int err; > + const char *msg; > +}; > + > +static struct geni_i2c_err_log gi2c_log[] = { > + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"}, > + [I2C_NACK] = {-ENOTCONN, > + "NACK: slv unresponsive, check its power/reset-ln"}, Break the 80-char rule, to improve readability. > + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"}, > + [I2C_BUS_PROTO] = {-EPROTO, > + "Bus proto err, noisy/unepxected start/stop"}, > + [I2C_ARB_LOST] = {-EBUSY, > + "Bus arbitration lost, clock line undriveable"}, > + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"}, > + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"}, > + [GENI_ILLEGAL_CMD] = {-EILSEQ, > + "Illegal cmd, check GENI cmd-state machine"}, > + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"}, > + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"}, > +}; > + > +struct geni_i2c_clk_fld { > + u32 clk_freq_out; > + u8 clk_div; > + u8 t_high; > + u8 t_low; > + u8 t_cycle; > +}; > + > +static struct geni_i2c_clk_fld geni_i2c_clk_map[] = { const > + {KHz(100), 7, 10, 11, 26}, > + {KHz(400), 2, 5, 12, 24}, > + {KHz(1000), 1, 3, 9, 18}, > +}; > + > +static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) > +{ > + int i; > + int ret = 0; > + bool clk_map_present = false; > + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; > + > + for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { > + if (itr->clk_freq_out == gi2c->clk_freq_out) { > + clk_map_present = true; > + break; Make this: gi2c->clk_fld = geni_i2c_clk_map + i; return 0; > + } > + } > + ...then you can drop ret and clk_map_present and just return -EINVAL here. > + if (clk_map_present) > + gi2c->clk_fld_idx = i; > + else > + ret = -EINVAL; > + > + return ret; > +} > + > +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs) Drop the "inline", if it makes sense the compiler will inline it, if not it knows better than we do. dfs is always 0, so drop this parameter and hard code the value below. > +{ > + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx; > + > + geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL); > + > + geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG); > + geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) | > + itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS); > + > + /* Ensure Clk config completes before return */ That's not what "mb" does, it ensures that later memory operations aren't reordered beyond the barrier. > + mb(); > +} > + > +static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err) Looking at the code in this function it's just a bunch of logic to print various debug messages...except for the last line that uses the gi2c_log lookup table to convert the error to a errno. Sneaky...and not very nice. > +{ > + u32 m_cmd = readl_relaxed(gi2c->base + SE_GENI_M_CMD0); Unless you have a really good reason, just use readl/writel in favour of their _relaxed versions. > + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); > + u32 geni_s = readl_relaxed(gi2c->base + SE_GENI_STATUS); > + u32 geni_ios = readl_relaxed(gi2c->base + SE_GENI_IOS); > + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); > + u32 rx_st, tx_st; > + > + if (gi2c->cur) > + dev_dbg(gi2c->dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n", > + gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags); > + > + if (err == I2C_NACK || err == GENI_ABORT_DONE) { > + dev_dbg(gi2c->dev, "%s\n", gi2c_log[err].msg); > + goto err_ret; > + } else { > + dev_err(gi2c->dev, "%s\n", gi2c_log[err].msg); > + } > + > + if (dma) { > + rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); > + tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); > + } else { > + rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); > + tx_st = readl_relaxed(gi2c->base + SE_GENI_TX_FIFO_STATUS); > + } > + dev_dbg(gi2c->dev, "DMA:%d tx_stat:0x%x, rx_stat:0x%x, irq-stat:0x%x\n", > + dma, tx_st, rx_st, m_stat); > + dev_dbg(gi2c->dev, "m_cmd:0x%x, geni_status:0x%x, geni_ios:0x%x\n", > + m_cmd, geni_s, geni_ios); > +err_ret: > + gi2c->err = gi2c_log[err].err; > +} > + > +static irqreturn_t geni_i2c_irq(int irq, void *dev) > +{ > + struct geni_i2c_dev *gi2c = dev; > + int i, j; > + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); > + u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); > + u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); > + u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); > + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); > + struct i2c_msg *cur = gi2c->cur; > + > + if (!cur || (m_stat & M_CMD_FAILURE_EN) || > + (dm_rx_st & (DM_I2C_CB_ERR)) || > + (m_stat & M_CMD_ABORT_EN)) { > + > + if (m_stat & M_GP_IRQ_1_EN) > + geni_i2c_err(gi2c, I2C_NACK); > + if (m_stat & M_GP_IRQ_3_EN) > + geni_i2c_err(gi2c, I2C_BUS_PROTO); > + if (m_stat & M_GP_IRQ_4_EN) > + geni_i2c_err(gi2c, I2C_ARB_LOST); > + if (m_stat & M_CMD_OVERRUN_EN) > + geni_i2c_err(gi2c, GENI_OVERRUN); > + if (m_stat & M_ILLEGAL_CMD_EN) > + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD); > + if (m_stat & M_CMD_ABORT_EN) > + geni_i2c_err(gi2c, GENI_ABORT_DONE); > + if (m_stat & M_GP_IRQ_0_EN) > + geni_i2c_err(gi2c, GP_IRQ0); > + > + if (!dma) > + writel_relaxed(0, (gi2c->base + > + SE_GENI_TX_WATERMARK_REG)); Drop the extra parenthesis. And using writel() instead keeps you under 80 chars. > + goto irqret; > + } > + > + if (dma) { > + dev_dbg(gi2c->dev, "i2c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, > + dm_rx_st); > + goto irqret; > + } > + > + if (((m_stat & M_RX_FIFO_WATERMARK_EN) || > + (m_stat & M_RX_FIFO_LAST_EN)) && (cur->flags & I2C_M_RD)) { Some of these parenthesis are unnecessary, but more importantly the way you wrap this line is misleading; the wrapping indicates that the two latter conditionals are related, but the parenthesis says the first two are. The most significant part of this conditional is "is this a read operation", so put this first. > + u32 rxcnt = rx_st & RX_FIFO_WC_MSK; > + > + for (j = 0; j < rxcnt; j++) { > + u32 temp; > + int p; > + > + temp = readl_relaxed(gi2c->base + SE_GENI_RX_FIFOn); > + for (i = gi2c->cur_rd, p = 0; (i < cur->len && p < 4); > + i++, p++) > + cur->buf[i] = (u8) ((temp >> (p * 8)) & 0xff); > + gi2c->cur_rd = i; The two-range for loop with a line wrap isn't very clean and the wrap calls for a set of braces - which will look ugly. How about something like this instead? p = 0; while (p < 4 && gi2c->cur_rd < cur->len) { cur->buf[gi2c->cur_rd++] = temp & 0xff; temp >>= 8; p++; } > + if (gi2c->cur_rd == cur->len) { > + dev_dbg(gi2c->dev, "FIFO i:%d,read 0x%x\n", > + i, temp); Who's the consumer of this debug line? > + break; > + } > + } > + } else if ((m_stat & M_TX_FIFO_WATERMARK_EN) && > + !(cur->flags & I2C_M_RD)) { > + for (j = 0; j < gi2c->tx_wm; j++) { > + u32 temp = 0; > + int p; > + > + for (i = gi2c->cur_wr, p = 0; (i < cur->len && p < 4); > + i++, p++) > + temp |= (((u32)(cur->buf[i]) << (p * 8))); > + writel_relaxed(temp, gi2c->base + SE_GENI_TX_FIFOn); Same concern as above. > + gi2c->cur_wr = i; > + dev_dbg(gi2c->dev, "FIFO i:%d,wrote 0x%x\n", i, temp); > + if (gi2c->cur_wr == cur->len) { > + dev_dbg(gi2c->dev, "FIFO i2c bytes done writing\n"); > + writel_relaxed(0, > + (gi2c->base + SE_GENI_TX_WATERMARK_REG)); The line break here is bad. checkpatch.pl --strict will help you find these. Also using writel() and dropping the parenthesis keeps you below 80 chars. > + break; > + } > + } > + } > +irqret: > + if (m_stat) > + writel_relaxed(m_stat, gi2c->base + SE_GENI_M_IRQ_CLEAR); Is it okay for this to be re-ordered wrt above writes? > + > + if (dma) { > + if (dm_tx_st) > + writel_relaxed(dm_tx_st, gi2c->base + > + SE_DMA_TX_IRQ_CLR); Use writel() and you don't have to wrap the line. > + if (dm_rx_st) > + writel_relaxed(dm_rx_st, gi2c->base + > + SE_DMA_RX_IRQ_CLR); > + /* Ensure all writes are done before returning from ISR. */ That's not what wmb does. > + wmb(); > + } > + /* if this is err with done-bit not set, handle that thr' timeout. */ Is "thr'" should for "through"? > + if (m_stat & M_CMD_DONE_EN) > + complete(&gi2c->xfer); > + else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & RX_DMA_DONE)) > + complete(&gi2c->xfer); > + > + return IRQ_HANDLED; > +} > + > +static int geni_i2c_xfer(struct i2c_adapter *adap, > + struct i2c_msg msgs[], > + int num) > +{ > + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); > + int i, ret = 0, timeout = 0; No need to initialize these, first use is an assignment. > + > + gi2c->err = 0; > + gi2c->cur = &msgs[0]; You're assigning cur in each iteration of the loop below, why do you need to do it here as well? > + reinit_completion(&gi2c->xfer); > + ret = pm_runtime_get_sync(gi2c->dev); > + if (ret < 0) { > + dev_err(gi2c->dev, "error turning SE resources:%d\n", ret); > + pm_runtime_put_noidle(gi2c->dev); > + /* Set device in suspended since resume failed */ > + pm_runtime_set_suspended(gi2c->dev); > + return ret; With the questionable assignment above you're leaving the function with gi2c->cur pointing to an object that will soon be invalid. > + } > + > + qcom_geni_i2c_conf(gi2c, 0); > + dev_dbg(gi2c->dev, "i2c xfer:num:%d, msgs:len:%d,flg:%d\n", > + num, msgs[0].len, msgs[0].flags); Use dynamic function tracing instead of debug prints for this. > + for (i = 0; i < num; i++) { I suggest that you split out two functions here, one for rx-one-message and one for tx-one-message. There are some duplication between the two, but not too bad. > + int stretch = (i < (num - 1)); > + u32 m_param = 0; > + u32 m_cmd = 0; > + dma_addr_t tx_dma = 0; > + dma_addr_t rx_dma = 0; > + enum geni_se_xfer_mode mode = FIFO_MODE; No need to initialize mode, as the first use is an assignment. > + > + m_param |= (stretch ? STOP_STRETCH : 0); > + m_param |= ((msgs[i].addr & 0x7F) << SLV_ADDR_SHFT); Drop some parenthesis. > + > + gi2c->cur = &msgs[i]; > + mode = msgs[i].len > 32 ? SE_DMA : FIFO_MODE; > + ret = geni_se_select_mode(gi2c->base, mode); > + if (ret) { > + dev_err(gi2c->dev, "%s: Error mode init %d:%d:%d\n", > + __func__, mode, i, msgs[i].len); > + break; > + } > + if (msgs[i].flags & I2C_M_RD) { > + dev_dbg(gi2c->dev, > + "READ,n:%d,i:%d len:%d, stretch:%d\n", > + num, i, msgs[i].len, stretch); > + geni_write_reg(msgs[i].len, > + gi2c->base, SE_I2C_RX_TRANS_LEN); > + m_cmd = I2C_READ; > + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param); Drop m_cmd and just write I2C_READ in the function call. > + if (mode == SE_DMA) { > + ret = geni_se_rx_dma_prep(gi2c->wrapper_dev, > + gi2c->base, msgs[i].buf, > + msgs[i].len, &rx_dma); > + if (ret) { > + mode = FIFO_MODE; > + ret = geni_se_select_mode(gi2c->base, > + mode); > + } > + } > + } else { > + dev_dbg(gi2c->dev, > + "WRITE:n:%d,i:%d len:%d, stretch:%d, m_param:0x%x\n", > + num, i, msgs[i].len, stretch, m_param); > + geni_write_reg(msgs[i].len, gi2c->base, > + SE_I2C_TX_TRANS_LEN); > + m_cmd = I2C_WRITE; > + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param); > + if (mode == SE_DMA) { > + ret = geni_se_tx_dma_prep(gi2c->wrapper_dev, > + gi2c->base, msgs[i].buf, > + msgs[i].len, &tx_dma); > + if (ret) { > + mode = FIFO_MODE; > + ret = geni_se_select_mode(gi2c->base, > + mode); > + } > + } > + if (mode == FIFO_MODE) /* Get FIFO IRQ */ > + geni_write_reg(1, gi2c->base, > + SE_GENI_TX_WATERMARK_REG); > + } > + /* Ensure FIFO write go through before waiting for Done evet */ That's not what mb does, drop it. > + mb(); > + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ); As written you should just use "ret", but the return value of wait_for_completion_timeout() is unsigned long, so change the type of timeout instead. > + if (!timeout) { > + geni_i2c_err(gi2c, GENI_TIMEOUT); > + gi2c->cur = NULL; > + geni_se_abort_m_cmd(gi2c->base); > + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ); What if it takes a fraction above HZ time to complete the previous operation, then you might end up here with the completion completed, but from the wrong operation. > + } > + gi2c->cur_wr = 0; > + gi2c->cur_rd = 0; > + if (mode == SE_DMA) { > + if (gi2c->err) { > + if (msgs[i].flags != I2C_M_RD) > + writel_relaxed(1, gi2c->base + > + SE_DMA_TX_FSM_RST); > + else > + writel_relaxed(1, gi2c->base + > + SE_DMA_RX_FSM_RST); > + wait_for_completion_timeout(&gi2c->xfer, HZ); > + } > + geni_se_rx_dma_unprep(gi2c->wrapper_dev, rx_dma, > + msgs[i].len); Reduce the length of gi2c->wrapper_dev here by using a local variable, so that you get below (or close to) 80 chars. Also, in either rx or tx cases above I see only that you prep one of thse, and then you're unprepping both. > + geni_se_tx_dma_unprep(gi2c->wrapper_dev, tx_dma, > + msgs[i].len); > + } > + ret = gi2c->err; > + if (gi2c->err) { > + dev_err(gi2c->dev, "i2c error :%d\n", gi2c->err); > + break; > + } > + } > + if (ret == 0) > + ret = num; > + > + pm_runtime_mark_last_busy(gi2c->dev); > + pm_runtime_put_autosuspend(gi2c->dev); > + gi2c->cur = NULL; > + gi2c->err = 0; > + dev_dbg(gi2c->dev, "i2c txn ret:%d\n", ret); > + return ret; > +} [..] > +static int geni_i2c_probe(struct platform_device *pdev) > +{ > + struct geni_i2c_dev *gi2c; > + struct resource *res; > + int ret; > + > + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL); > + if (!gi2c) > + return -ENOMEM; > + > + gi2c->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -EINVAL; Put this right before devm_ioremap_resource() and drop the error check. > + > + gi2c->wrapper_dev = pdev->dev.parent; > + gi2c->i2c_rsc.wrapper_dev = pdev->dev.parent; As suggested in the other patch, if you have an helper function in the wrapper driver that initializes the i2c_rsc then this could fill out the actual wrapper, instead of just the device. > + gi2c->i2c_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk"); > + if (IS_ERR(gi2c->i2c_rsc.se_clk)) { > + ret = PTR_ERR(gi2c->i2c_rsc.se_clk); > + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); > + return ret; > + } > + > + gi2c->base = devm_ioremap_resource(gi2c->dev, res); > + if (IS_ERR(gi2c->base)) > + return PTR_ERR(gi2c->base); > + > + gi2c->i2c_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev); Drop all the pinctrl stuff. You might still want to call pinctrl_pm_select_{idle,default,sleep}_state(), in the various stages of PM state though. > + if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_pinctrl)) { > + dev_err(&pdev->dev, "No pinctrl config specified\n"); > + ret = PTR_ERR(gi2c->i2c_rsc.geni_pinctrl); > + return ret; > + } [..] > +static int geni_i2c_runtime_resume(struct device *dev) > +{ > + int ret; > + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); > + > + ret = geni_se_resources_on(&gi2c->i2c_rsc); > + if (ret) > + return ret; > + > + if (!unlikely(gi2c->tx_wm)) { Drop the unlikely() > + int proto = geni_se_get_proto(gi2c->base); > + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(gi2c->base); > + > + if (unlikely(proto != I2C)) { Has this changes since probe? Can't we verify that the proto is correct during probe and then just trust that the hardware doesn't change function? > + dev_err(gi2c->dev, "Invalid proto %d\n", proto); > + geni_se_resources_off(&gi2c->i2c_rsc); > + return -ENXIO; > + } > + > + gi2c->tx_wm = gi2c_tx_depth - 1; > + geni_se_init(gi2c->base, gi2c->tx_wm, gi2c_tx_depth); > + geni_se_config_packing(gi2c->base, 8, 4, true); > + dev_dbg(gi2c->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", > + gi2c_tx_depth); > + } > + enable_irq(gi2c->irq); > + > + return 0; > +} [..] > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:i2c_geni"); What will reference the kernel module by this alias? > -- > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> wrote: > This bus driver supports the GENI based i2c hardware controller in the > Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable > module supporting a wide range of serial interfaces including I2C. The > driver supports FIFO mode and DMA mode of transfer and switches modes > dynamically depending on the size of the transfer. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > drivers/i2c/busses/Kconfig | 10 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 667 insertions(+) > create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c Newbie again, throwing in my two cents. > +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs) > +{ > + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx; > + > + geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL); > + > + geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG); > + geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) | > + itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS); It would be great to get these register field shifts defined, as they're otherwise fairly opaque. > +static irqreturn_t geni_i2c_irq(int irq, void *dev) > +{ > + struct geni_i2c_dev *gi2c = dev; > + int i, j; > + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); > + u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); > + u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); > + u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); > + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); > + struct i2c_msg *cur = gi2c->cur; > + > + if (!cur || (m_stat & M_CMD_FAILURE_EN) || > + (dm_rx_st & (DM_I2C_CB_ERR)) || > + (m_stat & M_CMD_ABORT_EN)) { > + > + if (m_stat & M_GP_IRQ_1_EN) > + geni_i2c_err(gi2c, I2C_NACK); > + if (m_stat & M_GP_IRQ_3_EN) > + geni_i2c_err(gi2c, I2C_BUS_PROTO); > + if (m_stat & M_GP_IRQ_4_EN) > + geni_i2c_err(gi2c, I2C_ARB_LOST); > + if (m_stat & M_CMD_OVERRUN_EN) > + geni_i2c_err(gi2c, GENI_OVERRUN); > + if (m_stat & M_ILLEGAL_CMD_EN) > + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD); > + if (m_stat & M_CMD_ABORT_EN) > + geni_i2c_err(gi2c, GENI_ABORT_DONE); > + if (m_stat & M_GP_IRQ_0_EN) > + geni_i2c_err(gi2c, GP_IRQ0); > + > + if (!dma) > + writel_relaxed(0, (gi2c->base + > + SE_GENI_TX_WATERMARK_REG)); The writes to the TX_WATERMARK_REG here and a little further down in the irq handler stick out to me. A comment as to why poking at the watermark register is necessary here would be very helpful. -Evan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/17/2018 10:23 PM, Bjorn Andersson wrote: > On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote: > >> This bus driver supports the GENI based i2c hardware controller in the >> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable >> module supporting a wide range of serial interfaces including I2C. The >> driver supports FIFO mode and DMA mode of transfer and switches modes >> dynamically depending on the size of the transfer. >> >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> >> --- >> drivers/i2c/busses/Kconfig | 10 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 667 insertions(+) >> create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 009345d..caef309 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -838,6 +838,16 @@ config I2C_PXA_SLAVE >> is necessary for systems where the PXA may be a target on the >> I2C bus. >> >> +config I2C_QCOM_GENI >> + tristate "Qualcomm Technologies Inc.'s GENI based I2C controller" >> + depends on ARCH_QCOM > > Just depend on the GENI wrapper as well. Ok. > >> + help >> + If you say yes to this option, support will be included for the >> + built-in I2C interface on the Qualcomm Technologies Inc.'s SoCs. >> + >> + This driver can also be built as a module. If so, the module >> + will be called i2c-qcom-geni. >> + >> config I2C_QUP >> tristate "Qualcomm QUP based I2C controller" >> depends on ARCH_QCOM >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 2ce8576..201fce1 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o >> obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o >> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o >> obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o >> +obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o >> obj-$(CONFIG_I2C_QUP) += i2c-qup.o >> obj-$(CONFIG_I2C_RIIC) += i2c-riic.o >> obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >> new file mode 100644 >> index 0000000..59ad4da >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -0,0 +1,656 @@ >> +/* >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ > > Use SPDX license header. Ok. > >> + >> +#include <linux/clk.h> >> +#include <linux/delay.h> > > Unused? Ok, I will remove unused header files. > >> +#include <linux/err.h> >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/dma-mapping.h> >> +#include <linux/qcom-geni-se.h> >> + >> +#define SE_I2C_TX_TRANS_LEN (0x26C) > > Drop the parenthesis, when not needed. Ok. > >> +#define SE_I2C_RX_TRANS_LEN (0x270) >> +#define SE_I2C_SCL_COUNTERS (0x278) >> +#define SE_GENI_IOS (0x908) >> + >> +#define SE_I2C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\ >> + M_GP_IRQ_1_EN | M_GP_IRQ_3_EN | M_GP_IRQ_4_EN) >> +#define SE_I2C_ABORT (1U << 1) >> +/* M_CMD OP codes for I2C */ >> +#define I2C_WRITE (0x1) >> +#define I2C_READ (0x2) >> +#define I2C_WRITE_READ (0x3) >> +#define I2C_ADDR_ONLY (0x4) >> +#define I2C_BUS_CLEAR (0x6) >> +#define I2C_STOP_ON_BUS (0x7) >> +/* M_CMD params for I2C */ >> +#define PRE_CMD_DELAY (BIT(0)) >> +#define TIMESTAMP_BEFORE (BIT(1)) >> +#define STOP_STRETCH (BIT(2)) >> +#define TIMESTAMP_AFTER (BIT(3)) >> +#define POST_COMMAND_DELAY (BIT(4)) >> +#define IGNORE_ADD_NACK (BIT(6)) >> +#define READ_FINISHED_WITH_ACK (BIT(7)) >> +#define BYPASS_ADDR_PHASE (BIT(8)) >> +#define SLV_ADDR_MSK (GENMASK(15, 9)) >> +#define SLV_ADDR_SHFT (9) >> + >> +#define I2C_CORE2X_VOTE (10000) >> +#define GP_IRQ0 0 >> +#define GP_IRQ1 1 >> +#define GP_IRQ2 2 >> +#define GP_IRQ3 3 >> +#define GP_IRQ4 4 >> +#define GP_IRQ5 5 >> +#define GENI_OVERRUN 6 >> +#define GENI_ILLEGAL_CMD 7 >> +#define GENI_ABORT_DONE 8 >> +#define GENI_TIMEOUT 9 >> + >> +#define I2C_NACK GP_IRQ1 >> +#define I2C_BUS_PROTO GP_IRQ3 >> +#define I2C_ARB_LOST GP_IRQ4 >> +#define DM_I2C_CB_ERR ((BIT(GP_IRQ1) | BIT(GP_IRQ3) | BIT(GP_IRQ4)) \ >> + << 5) >> + >> +#define I2C_AUTO_SUSPEND_DELAY 250 >> +#define KHz(freq) (1000 * freq) >> + >> +struct geni_i2c_dev { >> + struct device *dev; >> + void __iomem *base; >> + unsigned int tx_wm; >> + int irq; >> + int err; >> + struct i2c_adapter adap; >> + struct completion xfer; > > How about naming this "done" or something like that, gi2c->xfer doesn't > really give the sense of being a "we're done with the operation"-event. Ok. > >> + struct i2c_msg *cur; >> + struct geni_se_rsc i2c_rsc; >> + int cur_wr; >> + int cur_rd; >> + struct device *wrapper_dev; > > This is already availabe in i2c_rsc, and in particular if you pass the > i2c_rsc down to the wrapper in the 2 cases you use the wrapper_dev you > don't need this duplication. Ok. > >> + u32 clk_freq_out; >> + int clk_fld_idx; > > Keep track of the const struct geni_i2c_clk_fld * here instead. Ok. > >> +}; >> + >> +struct geni_i2c_err_log { >> + int err; >> + const char *msg; >> +}; >> + >> +static struct geni_i2c_err_log gi2c_log[] = { >> + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"}, >> + [I2C_NACK] = {-ENOTCONN, >> + "NACK: slv unresponsive, check its power/reset-ln"}, > > Break the 80-char rule, to improve readability. Ok. > >> + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"}, >> + [I2C_BUS_PROTO] = {-EPROTO, >> + "Bus proto err, noisy/unepxected start/stop"}, >> + [I2C_ARB_LOST] = {-EBUSY, >> + "Bus arbitration lost, clock line undriveable"}, >> + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"}, >> + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"}, >> + [GENI_ILLEGAL_CMD] = {-EILSEQ, >> + "Illegal cmd, check GENI cmd-state machine"}, >> + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"}, >> + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"}, >> +}; >> + >> +struct geni_i2c_clk_fld { >> + u32 clk_freq_out; >> + u8 clk_div; >> + u8 t_high; >> + u8 t_low; >> + u8 t_cycle; >> +}; >> + >> +static struct geni_i2c_clk_fld geni_i2c_clk_map[] = { > > const Ok. > >> + {KHz(100), 7, 10, 11, 26}, >> + {KHz(400), 2, 5, 12, 24}, >> + {KHz(1000), 1, 3, 9, 18}, >> +}; >> + >> +static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) >> +{ >> + int i; >> + int ret = 0; >> + bool clk_map_present = false; >> + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; >> + >> + for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { >> + if (itr->clk_freq_out == gi2c->clk_freq_out) { >> + clk_map_present = true; >> + break; > > Make this: > gi2c->clk_fld = geni_i2c_clk_map + i; > return 0; Ok. > >> + } >> + } >> + > > ...then you can drop ret and clk_map_present and just return -EINVAL > here. Ok. > >> + if (clk_map_present) >> + gi2c->clk_fld_idx = i; >> + else >> + ret = -EINVAL; >> + >> + return ret; >> +} >> + >> +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs) > > Drop the "inline", if it makes sense the compiler will inline it, if not > it knows better than we do. > > dfs is always 0, so drop this parameter and hard code the value below. Ok. > >> +{ >> + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx; >> + >> + geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL); >> + >> + geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG); >> + geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) | >> + itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS); >> + >> + /* Ensure Clk config completes before return */ > > That's not what "mb" does, it ensures that later memory operations > aren't reordered beyond the barrier. Our intention also is for ordering purposes so that any later register writes/reads does not get re-ordered until the writes to clock configuration register is complete. I will update the comment. > >> + mb(); >> +} >> + >> +static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err) > > Looking at the code in this function it's just a bunch of logic to print > various debug messages...except for the last line that uses the gi2c_log > lookup table to convert the error to a errno. Sneaky...and not very > nice. I will update this function so that it is more obvious. > >> +{ >> + u32 m_cmd = readl_relaxed(gi2c->base + SE_GENI_M_CMD0); > > Unless you have a really good reason, just use readl/writel in favour of > their _relaxed versions. The goal is to maintain the order of execution to the Serial Engine register block. I believe _relaxed help enusre that order with a little less overhead compared to non_relaxed function. Please correct me otherwise. > >> + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); >> + u32 geni_s = readl_relaxed(gi2c->base + SE_GENI_STATUS); >> + u32 geni_ios = readl_relaxed(gi2c->base + SE_GENI_IOS); >> + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); >> + u32 rx_st, tx_st; >> + >> + if (gi2c->cur) >> + dev_dbg(gi2c->dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n", >> + gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags); >> + >> + if (err == I2C_NACK || err == GENI_ABORT_DONE) { >> + dev_dbg(gi2c->dev, "%s\n", gi2c_log[err].msg); >> + goto err_ret; >> + } else { >> + dev_err(gi2c->dev, "%s\n", gi2c_log[err].msg); >> + } >> + >> + if (dma) { >> + rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); >> + tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); >> + } else { >> + rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); >> + tx_st = readl_relaxed(gi2c->base + SE_GENI_TX_FIFO_STATUS); >> + } >> + dev_dbg(gi2c->dev, "DMA:%d tx_stat:0x%x, rx_stat:0x%x, irq-stat:0x%x\n", >> + dma, tx_st, rx_st, m_stat); >> + dev_dbg(gi2c->dev, "m_cmd:0x%x, geni_status:0x%x, geni_ios:0x%x\n", >> + m_cmd, geni_s, geni_ios); >> +err_ret: >> + gi2c->err = gi2c_log[err].err; >> +} >> + >> +static irqreturn_t geni_i2c_irq(int irq, void *dev) >> +{ >> + struct geni_i2c_dev *gi2c = dev; >> + int i, j; >> + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); >> + u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); >> + u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); >> + u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); >> + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); >> + struct i2c_msg *cur = gi2c->cur; >> + >> + if (!cur || (m_stat & M_CMD_FAILURE_EN) || >> + (dm_rx_st & (DM_I2C_CB_ERR)) || >> + (m_stat & M_CMD_ABORT_EN)) { >> + >> + if (m_stat & M_GP_IRQ_1_EN) >> + geni_i2c_err(gi2c, I2C_NACK); >> + if (m_stat & M_GP_IRQ_3_EN) >> + geni_i2c_err(gi2c, I2C_BUS_PROTO); >> + if (m_stat & M_GP_IRQ_4_EN) >> + geni_i2c_err(gi2c, I2C_ARB_LOST); >> + if (m_stat & M_CMD_OVERRUN_EN) >> + geni_i2c_err(gi2c, GENI_OVERRUN); >> + if (m_stat & M_ILLEGAL_CMD_EN) >> + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD); >> + if (m_stat & M_CMD_ABORT_EN) >> + geni_i2c_err(gi2c, GENI_ABORT_DONE); >> + if (m_stat & M_GP_IRQ_0_EN) >> + geni_i2c_err(gi2c, GP_IRQ0); >> + >> + if (!dma) >> + writel_relaxed(0, (gi2c->base + >> + SE_GENI_TX_WATERMARK_REG)); > > Drop the extra parenthesis. And using writel() instead keeps you under > 80 chars. Ok, to dropping the parenthesis. > >> + goto irqret; >> + } >> + >> + if (dma) { >> + dev_dbg(gi2c->dev, "i2c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, >> + dm_rx_st); >> + goto irqret; >> + } >> + >> + if (((m_stat & M_RX_FIFO_WATERMARK_EN) || >> + (m_stat & M_RX_FIFO_LAST_EN)) && (cur->flags & I2C_M_RD)) { > > Some of these parenthesis are unnecessary, but more importantly the way > you wrap this line is misleading; the wrapping indicates that the two > latter conditionals are related, but the parenthesis says the first two > are. > > The most significant part of this conditional is "is this a read > operation", so put this first. Ok. > > >> + u32 rxcnt = rx_st & RX_FIFO_WC_MSK; >> + >> + for (j = 0; j < rxcnt; j++) { >> + u32 temp; >> + int p; >> + >> + temp = readl_relaxed(gi2c->base + SE_GENI_RX_FIFOn); >> + for (i = gi2c->cur_rd, p = 0; (i < cur->len && p < 4); >> + i++, p++) >> + cur->buf[i] = (u8) ((temp >> (p * 8)) & 0xff); >> + gi2c->cur_rd = i; > > The two-range for loop with a line wrap isn't very clean and the wrap > calls for a set of braces - which will look ugly. > > How about something like this instead? > > p = 0; > while (p < 4 && gi2c->cur_rd < cur->len) { > cur->buf[gi2c->cur_rd++] = temp & 0xff; > temp >>= 8; > p++; > } Ok. > >> + if (gi2c->cur_rd == cur->len) { >> + dev_dbg(gi2c->dev, "FIFO i:%d,read 0x%x\n", >> + i, temp); > > Who's the consumer of this debug line? I will make the debug message more comprehensible, if that is what you mean. > >> + break; >> + } >> + } >> + } else if ((m_stat & M_TX_FIFO_WATERMARK_EN) && >> + !(cur->flags & I2C_M_RD)) { >> + for (j = 0; j < gi2c->tx_wm; j++) { >> + u32 temp = 0; >> + int p; >> + >> + for (i = gi2c->cur_wr, p = 0; (i < cur->len && p < 4); >> + i++, p++) >> + temp |= (((u32)(cur->buf[i]) << (p * 8))); >> + writel_relaxed(temp, gi2c->base + SE_GENI_TX_FIFOn); > > Same concern as above. Ok. > >> + gi2c->cur_wr = i; >> + dev_dbg(gi2c->dev, "FIFO i:%d,wrote 0x%x\n", i, temp); >> + if (gi2c->cur_wr == cur->len) { >> + dev_dbg(gi2c->dev, "FIFO i2c bytes done writing\n"); >> + writel_relaxed(0, >> + (gi2c->base + SE_GENI_TX_WATERMARK_REG)); > > The line break here is bad. checkpatch.pl --strict will help you find > these. Ok/ > > Also using writel() and dropping the parenthesis keeps you below 80 > chars. Ok, to dropping the parenthesis. > >> + break; >> + } >> + } >> + } >> +irqret: >> + if (m_stat) >> + writel_relaxed(m_stat, gi2c->base + SE_GENI_M_IRQ_CLEAR); > > Is it okay for this to be re-ordered wrt above writes? It is okay. The interrupt bitmask can be cleared anytime while handling the IRQ. > >> + >> + if (dma) { >> + if (dm_tx_st) >> + writel_relaxed(dm_tx_st, gi2c->base + >> + SE_DMA_TX_IRQ_CLR); > > Use writel() and you don't have to wrap the line. Please refer to my earlier response regarding using _relaxed() variants. > >> + if (dm_rx_st) >> + writel_relaxed(dm_rx_st, gi2c->base + >> + SE_DMA_RX_IRQ_CLR); >> + /* Ensure all writes are done before returning from ISR. */ > > That's not what wmb does. I will drop it. > >> + wmb(); >> + } >> + /* if this is err with done-bit not set, handle that thr' timeout. */ > > Is "thr'" should for "through"? Yes. I will make it clear. > >> + if (m_stat & M_CMD_DONE_EN) >> + complete(&gi2c->xfer); >> + else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & RX_DMA_DONE)) >> + complete(&gi2c->xfer); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int geni_i2c_xfer(struct i2c_adapter *adap, >> + struct i2c_msg msgs[], >> + int num) >> +{ >> + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); >> + int i, ret = 0, timeout = 0; > > No need to initialize these, first use is an assignment. Ok. > >> + >> + gi2c->err = 0; >> + gi2c->cur = &msgs[0]; > > You're assigning cur in each iteration of the loop below, why do you > need to do it here as well? I will remove. > >> + reinit_completion(&gi2c->xfer); >> + ret = pm_runtime_get_sync(gi2c->dev); >> + if (ret < 0) { >> + dev_err(gi2c->dev, "error turning SE resources:%d\n", ret); >> + pm_runtime_put_noidle(gi2c->dev); >> + /* Set device in suspended since resume failed */ >> + pm_runtime_set_suspended(gi2c->dev); >> + return ret; > > With the questionable assignment above you're leaving the function with > gi2c->cur pointing to an object that will soon be invalid. I will fix the assignment. > >> + } >> + >> + qcom_geni_i2c_conf(gi2c, 0); >> + dev_dbg(gi2c->dev, "i2c xfer:num:%d, msgs:len:%d,flg:%d\n", >> + num, msgs[0].len, msgs[0].flags); > > Use dynamic function tracing instead of debug prints for this. Ok. I will remove the debug statements for now and use dynamic function tracing in a different patch. > >> + for (i = 0; i < num; i++) { > > I suggest that you split out two functions here, one for rx-one-message > and one for tx-one-message. There are some duplication between the two, > but not too bad. Ok. > >> + int stretch = (i < (num - 1)); >> + u32 m_param = 0; >> + u32 m_cmd = 0; >> + dma_addr_t tx_dma = 0; >> + dma_addr_t rx_dma = 0; >> + enum geni_se_xfer_mode mode = FIFO_MODE; > > No need to initialize mode, as the first use is an assignment. Ok. > >> + >> + m_param |= (stretch ? STOP_STRETCH : 0); >> + m_param |= ((msgs[i].addr & 0x7F) << SLV_ADDR_SHFT); > > Drop some parenthesis. Ok. > >> + >> + gi2c->cur = &msgs[i]; >> + mode = msgs[i].len > 32 ? SE_DMA : FIFO_MODE; >> + ret = geni_se_select_mode(gi2c->base, mode); >> + if (ret) { >> + dev_err(gi2c->dev, "%s: Error mode init %d:%d:%d\n", >> + __func__, mode, i, msgs[i].len); >> + break; >> + } >> + if (msgs[i].flags & I2C_M_RD) { >> + dev_dbg(gi2c->dev, >> + "READ,n:%d,i:%d len:%d, stretch:%d\n", >> + num, i, msgs[i].len, stretch); >> + geni_write_reg(msgs[i].len, >> + gi2c->base, SE_I2C_RX_TRANS_LEN); >> + m_cmd = I2C_READ; >> + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param); > > Drop m_cmd and just write I2C_READ in the function call. Ok. > >> + if (mode == SE_DMA) { >> + ret = geni_se_rx_dma_prep(gi2c->wrapper_dev, >> + gi2c->base, msgs[i].buf, >> + msgs[i].len, &rx_dma); >> + if (ret) { >> + mode = FIFO_MODE; >> + ret = geni_se_select_mode(gi2c->base, >> + mode); >> + } >> + } >> + } else { >> + dev_dbg(gi2c->dev, >> + "WRITE:n:%d,i:%d len:%d, stretch:%d, m_param:0x%x\n", >> + num, i, msgs[i].len, stretch, m_param); >> + geni_write_reg(msgs[i].len, gi2c->base, >> + SE_I2C_TX_TRANS_LEN); >> + m_cmd = I2C_WRITE; >> + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param); >> + if (mode == SE_DMA) { >> + ret = geni_se_tx_dma_prep(gi2c->wrapper_dev, >> + gi2c->base, msgs[i].buf, >> + msgs[i].len, &tx_dma); >> + if (ret) { >> + mode = FIFO_MODE; >> + ret = geni_se_select_mode(gi2c->base, >> + mode); >> + } >> + } >> + if (mode == FIFO_MODE) /* Get FIFO IRQ */ >> + geni_write_reg(1, gi2c->base, >> + SE_GENI_TX_WATERMARK_REG); >> + } >> + /* Ensure FIFO write go through before waiting for Done evet */ > > That's not what mb does, drop it. Ok. > >> + mb(); >> + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ); > > As written you should just use "ret", but the return value of > wait_for_completion_timeout() is unsigned long, so change the type of > timeout instead. Ok. > >> + if (!timeout) { >> + geni_i2c_err(gi2c, GENI_TIMEOUT); >> + gi2c->cur = NULL; >> + geni_se_abort_m_cmd(gi2c->base); >> + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ); > > What if it takes a fraction above HZ time to complete the previous > operation, then you might end up here with the completion completed, but > from the wrong operation. I will update it to checking the specific "abort" flag using readl_poll_timeout. > >> + } >> + gi2c->cur_wr = 0; >> + gi2c->cur_rd = 0; >> + if (mode == SE_DMA) { >> + if (gi2c->err) { >> + if (msgs[i].flags != I2C_M_RD) >> + writel_relaxed(1, gi2c->base + >> + SE_DMA_TX_FSM_RST); >> + else >> + writel_relaxed(1, gi2c->base + >> + SE_DMA_RX_FSM_RST); >> + wait_for_completion_timeout(&gi2c->xfer, HZ); >> + } >> + geni_se_rx_dma_unprep(gi2c->wrapper_dev, rx_dma, >> + msgs[i].len); > > Reduce the length of gi2c->wrapper_dev here by using a local variable, > so that you get below (or close to) 80 chars. > > Also, in either rx or tx cases above I see only that you prep one of > thse, and then you're unprepping both. I will re-organize the tx and rx into 2 separate functions. That way all the comments will be taken care of. > >> + geni_se_tx_dma_unprep(gi2c->wrapper_dev, tx_dma, >> + msgs[i].len); >> + } >> + ret = gi2c->err; >> + if (gi2c->err) { >> + dev_err(gi2c->dev, "i2c error :%d\n", gi2c->err); >> + break; >> + } >> + } >> + if (ret == 0) >> + ret = num; >> + >> + pm_runtime_mark_last_busy(gi2c->dev); >> + pm_runtime_put_autosuspend(gi2c->dev); >> + gi2c->cur = NULL; >> + gi2c->err = 0; >> + dev_dbg(gi2c->dev, "i2c txn ret:%d\n", ret); >> + return ret; >> +} > [..] >> +static int geni_i2c_probe(struct platform_device *pdev) >> +{ >> + struct geni_i2c_dev *gi2c; >> + struct resource *res; >> + int ret; >> + >> + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL); >> + if (!gi2c) >> + return -ENOMEM; >> + >> + gi2c->dev = &pdev->dev; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return -EINVAL; > > Put this right before devm_ioremap_resource() and drop the error check. Ok. > >> + >> + gi2c->wrapper_dev = pdev->dev.parent; >> + gi2c->i2c_rsc.wrapper_dev = pdev->dev.parent; > > As suggested in the other patch, if you have an helper function in the > wrapper driver that initializes the i2c_rsc then this could fill out the > actual wrapper, instead of just the device. Ok. > >> + gi2c->i2c_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk"); >> + if (IS_ERR(gi2c->i2c_rsc.se_clk)) { >> + ret = PTR_ERR(gi2c->i2c_rsc.se_clk); >> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); >> + return ret; >> + } >> + >> + gi2c->base = devm_ioremap_resource(gi2c->dev, res); >> + if (IS_ERR(gi2c->base)) >> + return PTR_ERR(gi2c->base); >> + >> + gi2c->i2c_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev); > > Drop all the pinctrl stuff. You might still want to call > pinctrl_pm_select_{idle,default,sleep}_state(), in the various stages of > PM state though. Ok. > >> + if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_pinctrl)) { >> + dev_err(&pdev->dev, "No pinctrl config specified\n"); >> + ret = PTR_ERR(gi2c->i2c_rsc.geni_pinctrl); >> + return ret; >> + } > [..] >> +static int geni_i2c_runtime_resume(struct device *dev) >> +{ >> + int ret; >> + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); >> + >> + ret = geni_se_resources_on(&gi2c->i2c_rsc); >> + if (ret) >> + return ret; >> + >> + if (!unlikely(gi2c->tx_wm)) { > > Drop the unlikely() Ok. > >> + int proto = geni_se_get_proto(gi2c->base); >> + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(gi2c->base); >> + >> + if (unlikely(proto != I2C)) { > > Has this changes since probe? Can't we verify that the proto is correct > during probe and then just trust that the hardware doesn't change > function? Yes we can do that. I will move the check to the probe. > >> + dev_err(gi2c->dev, "Invalid proto %d\n", proto); >> + geni_se_resources_off(&gi2c->i2c_rsc); >> + return -ENXIO; >> + } >> + >> + gi2c->tx_wm = gi2c_tx_depth - 1; >> + geni_se_init(gi2c->base, gi2c->tx_wm, gi2c_tx_depth); >> + geni_se_config_packing(gi2c->base, 8, 4, true); >> + dev_dbg(gi2c->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", >> + gi2c_tx_depth); >> + } >> + enable_irq(gi2c->irq); >> + >> + return 0; >> +} > [..] >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:i2c_geni"); > > What will reference the kernel module by this alias? I will remove it. > >> -- >> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 009345d..caef309 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -838,6 +838,16 @@ config I2C_PXA_SLAVE is necessary for systems where the PXA may be a target on the I2C bus. +config I2C_QCOM_GENI + tristate "Qualcomm Technologies Inc.'s GENI based I2C controller" + depends on ARCH_QCOM + help + If you say yes to this option, support will be included for the + built-in I2C interface on the Qualcomm Technologies Inc.'s SoCs. + + This driver can also be built as a module. If so, the module + will be called i2c-qcom-geni. + config I2C_QUP tristate "Qualcomm QUP based I2C controller" depends on ARCH_QCOM diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 2ce8576..201fce1 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o obj-$(CONFIG_I2C_PXA) += i2c-pxa.o obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o +obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o obj-$(CONFIG_I2C_QUP) += i2c-qup.o obj-$(CONFIG_I2C_RIIC) += i2c-riic.o obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c new file mode 100644 index 0000000..59ad4da --- /dev/null +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -0,0 +1,656 @@ +/* + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/dma-mapping.h> +#include <linux/qcom-geni-se.h> + +#define SE_I2C_TX_TRANS_LEN (0x26C) +#define SE_I2C_RX_TRANS_LEN (0x270) +#define SE_I2C_SCL_COUNTERS (0x278) +#define SE_GENI_IOS (0x908) + +#define SE_I2C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\ + M_GP_IRQ_1_EN | M_GP_IRQ_3_EN | M_GP_IRQ_4_EN) +#define SE_I2C_ABORT (1U << 1) +/* M_CMD OP codes for I2C */ +#define I2C_WRITE (0x1) +#define I2C_READ (0x2) +#define I2C_WRITE_READ (0x3) +#define I2C_ADDR_ONLY (0x4) +#define I2C_BUS_CLEAR (0x6) +#define I2C_STOP_ON_BUS (0x7) +/* M_CMD params for I2C */ +#define PRE_CMD_DELAY (BIT(0)) +#define TIMESTAMP_BEFORE (BIT(1)) +#define STOP_STRETCH (BIT(2)) +#define TIMESTAMP_AFTER (BIT(3)) +#define POST_COMMAND_DELAY (BIT(4)) +#define IGNORE_ADD_NACK (BIT(6)) +#define READ_FINISHED_WITH_ACK (BIT(7)) +#define BYPASS_ADDR_PHASE (BIT(8)) +#define SLV_ADDR_MSK (GENMASK(15, 9)) +#define SLV_ADDR_SHFT (9) + +#define I2C_CORE2X_VOTE (10000) +#define GP_IRQ0 0 +#define GP_IRQ1 1 +#define GP_IRQ2 2 +#define GP_IRQ3 3 +#define GP_IRQ4 4 +#define GP_IRQ5 5 +#define GENI_OVERRUN 6 +#define GENI_ILLEGAL_CMD 7 +#define GENI_ABORT_DONE 8 +#define GENI_TIMEOUT 9 + +#define I2C_NACK GP_IRQ1 +#define I2C_BUS_PROTO GP_IRQ3 +#define I2C_ARB_LOST GP_IRQ4 +#define DM_I2C_CB_ERR ((BIT(GP_IRQ1) | BIT(GP_IRQ3) | BIT(GP_IRQ4)) \ + << 5) + +#define I2C_AUTO_SUSPEND_DELAY 250 +#define KHz(freq) (1000 * freq) + +struct geni_i2c_dev { + struct device *dev; + void __iomem *base; + unsigned int tx_wm; + int irq; + int err; + struct i2c_adapter adap; + struct completion xfer; + struct i2c_msg *cur; + struct geni_se_rsc i2c_rsc; + int cur_wr; + int cur_rd; + struct device *wrapper_dev; + u32 clk_freq_out; + int clk_fld_idx; +}; + +struct geni_i2c_err_log { + int err; + const char *msg; +}; + +static struct geni_i2c_err_log gi2c_log[] = { + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"}, + [I2C_NACK] = {-ENOTCONN, + "NACK: slv unresponsive, check its power/reset-ln"}, + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"}, + [I2C_BUS_PROTO] = {-EPROTO, + "Bus proto err, noisy/unepxected start/stop"}, + [I2C_ARB_LOST] = {-EBUSY, + "Bus arbitration lost, clock line undriveable"}, + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"}, + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"}, + [GENI_ILLEGAL_CMD] = {-EILSEQ, + "Illegal cmd, check GENI cmd-state machine"}, + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"}, + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"}, +}; + +struct geni_i2c_clk_fld { + u32 clk_freq_out; + u8 clk_div; + u8 t_high; + u8 t_low; + u8 t_cycle; +}; + +static struct geni_i2c_clk_fld geni_i2c_clk_map[] = { + {KHz(100), 7, 10, 11, 26}, + {KHz(400), 2, 5, 12, 24}, + {KHz(1000), 1, 3, 9, 18}, +}; + +static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c) +{ + int i; + int ret = 0; + bool clk_map_present = false; + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map; + + for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) { + if (itr->clk_freq_out == gi2c->clk_freq_out) { + clk_map_present = true; + break; + } + } + + if (clk_map_present) + gi2c->clk_fld_idx = i; + else + ret = -EINVAL; + + return ret; +} + +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs) +{ + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx; + + geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL); + + geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG); + geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) | + itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS); + + /* Ensure Clk config completes before return */ + mb(); +} + +static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err) +{ + u32 m_cmd = readl_relaxed(gi2c->base + SE_GENI_M_CMD0); + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); + u32 geni_s = readl_relaxed(gi2c->base + SE_GENI_STATUS); + u32 geni_ios = readl_relaxed(gi2c->base + SE_GENI_IOS); + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); + u32 rx_st, tx_st; + + if (gi2c->cur) + dev_dbg(gi2c->dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n", + gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags); + + if (err == I2C_NACK || err == GENI_ABORT_DONE) { + dev_dbg(gi2c->dev, "%s\n", gi2c_log[err].msg); + goto err_ret; + } else { + dev_err(gi2c->dev, "%s\n", gi2c_log[err].msg); + } + + if (dma) { + rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); + tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); + } else { + rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); + tx_st = readl_relaxed(gi2c->base + SE_GENI_TX_FIFO_STATUS); + } + dev_dbg(gi2c->dev, "DMA:%d tx_stat:0x%x, rx_stat:0x%x, irq-stat:0x%x\n", + dma, tx_st, rx_st, m_stat); + dev_dbg(gi2c->dev, "m_cmd:0x%x, geni_status:0x%x, geni_ios:0x%x\n", + m_cmd, geni_s, geni_ios); +err_ret: + gi2c->err = gi2c_log[err].err; +} + +static irqreturn_t geni_i2c_irq(int irq, void *dev) +{ + struct geni_i2c_dev *gi2c = dev; + int i, j; + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS); + u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS); + u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT); + u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT); + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN); + struct i2c_msg *cur = gi2c->cur; + + if (!cur || (m_stat & M_CMD_FAILURE_EN) || + (dm_rx_st & (DM_I2C_CB_ERR)) || + (m_stat & M_CMD_ABORT_EN)) { + + if (m_stat & M_GP_IRQ_1_EN) + geni_i2c_err(gi2c, I2C_NACK); + if (m_stat & M_GP_IRQ_3_EN) + geni_i2c_err(gi2c, I2C_BUS_PROTO); + if (m_stat & M_GP_IRQ_4_EN) + geni_i2c_err(gi2c, I2C_ARB_LOST); + if (m_stat & M_CMD_OVERRUN_EN) + geni_i2c_err(gi2c, GENI_OVERRUN); + if (m_stat & M_ILLEGAL_CMD_EN) + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD); + if (m_stat & M_CMD_ABORT_EN) + geni_i2c_err(gi2c, GENI_ABORT_DONE); + if (m_stat & M_GP_IRQ_0_EN) + geni_i2c_err(gi2c, GP_IRQ0); + + if (!dma) + writel_relaxed(0, (gi2c->base + + SE_GENI_TX_WATERMARK_REG)); + goto irqret; + } + + if (dma) { + dev_dbg(gi2c->dev, "i2c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, + dm_rx_st); + goto irqret; + } + + if (((m_stat & M_RX_FIFO_WATERMARK_EN) || + (m_stat & M_RX_FIFO_LAST_EN)) && (cur->flags & I2C_M_RD)) { + u32 rxcnt = rx_st & RX_FIFO_WC_MSK; + + for (j = 0; j < rxcnt; j++) { + u32 temp; + int p; + + temp = readl_relaxed(gi2c->base + SE_GENI_RX_FIFOn); + for (i = gi2c->cur_rd, p = 0; (i < cur->len && p < 4); + i++, p++) + cur->buf[i] = (u8) ((temp >> (p * 8)) & 0xff); + gi2c->cur_rd = i; + if (gi2c->cur_rd == cur->len) { + dev_dbg(gi2c->dev, "FIFO i:%d,read 0x%x\n", + i, temp); + break; + } + } + } else if ((m_stat & M_TX_FIFO_WATERMARK_EN) && + !(cur->flags & I2C_M_RD)) { + for (j = 0; j < gi2c->tx_wm; j++) { + u32 temp = 0; + int p; + + for (i = gi2c->cur_wr, p = 0; (i < cur->len && p < 4); + i++, p++) + temp |= (((u32)(cur->buf[i]) << (p * 8))); + writel_relaxed(temp, gi2c->base + SE_GENI_TX_FIFOn); + gi2c->cur_wr = i; + dev_dbg(gi2c->dev, "FIFO i:%d,wrote 0x%x\n", i, temp); + if (gi2c->cur_wr == cur->len) { + dev_dbg(gi2c->dev, "FIFO i2c bytes done writing\n"); + writel_relaxed(0, + (gi2c->base + SE_GENI_TX_WATERMARK_REG)); + break; + } + } + } +irqret: + if (m_stat) + writel_relaxed(m_stat, gi2c->base + SE_GENI_M_IRQ_CLEAR); + + if (dma) { + if (dm_tx_st) + writel_relaxed(dm_tx_st, gi2c->base + + SE_DMA_TX_IRQ_CLR); + if (dm_rx_st) + writel_relaxed(dm_rx_st, gi2c->base + + SE_DMA_RX_IRQ_CLR); + /* Ensure all writes are done before returning from ISR. */ + wmb(); + } + /* if this is err with done-bit not set, handle that thr' timeout. */ + if (m_stat & M_CMD_DONE_EN) + complete(&gi2c->xfer); + else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & RX_DMA_DONE)) + complete(&gi2c->xfer); + + return IRQ_HANDLED; +} + +static int geni_i2c_xfer(struct i2c_adapter *adap, + struct i2c_msg msgs[], + int num) +{ + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); + int i, ret = 0, timeout = 0; + + gi2c->err = 0; + gi2c->cur = &msgs[0]; + reinit_completion(&gi2c->xfer); + ret = pm_runtime_get_sync(gi2c->dev); + if (ret < 0) { + dev_err(gi2c->dev, "error turning SE resources:%d\n", ret); + pm_runtime_put_noidle(gi2c->dev); + /* Set device in suspended since resume failed */ + pm_runtime_set_suspended(gi2c->dev); + return ret; + } + + qcom_geni_i2c_conf(gi2c, 0); + dev_dbg(gi2c->dev, "i2c xfer:num:%d, msgs:len:%d,flg:%d\n", + num, msgs[0].len, msgs[0].flags); + for (i = 0; i < num; i++) { + int stretch = (i < (num - 1)); + u32 m_param = 0; + u32 m_cmd = 0; + dma_addr_t tx_dma = 0; + dma_addr_t rx_dma = 0; + enum geni_se_xfer_mode mode = FIFO_MODE; + + m_param |= (stretch ? STOP_STRETCH : 0); + m_param |= ((msgs[i].addr & 0x7F) << SLV_ADDR_SHFT); + + gi2c->cur = &msgs[i]; + mode = msgs[i].len > 32 ? SE_DMA : FIFO_MODE; + ret = geni_se_select_mode(gi2c->base, mode); + if (ret) { + dev_err(gi2c->dev, "%s: Error mode init %d:%d:%d\n", + __func__, mode, i, msgs[i].len); + break; + } + if (msgs[i].flags & I2C_M_RD) { + dev_dbg(gi2c->dev, + "READ,n:%d,i:%d len:%d, stretch:%d\n", + num, i, msgs[i].len, stretch); + geni_write_reg(msgs[i].len, + gi2c->base, SE_I2C_RX_TRANS_LEN); + m_cmd = I2C_READ; + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param); + if (mode == SE_DMA) { + ret = geni_se_rx_dma_prep(gi2c->wrapper_dev, + gi2c->base, msgs[i].buf, + msgs[i].len, &rx_dma); + if (ret) { + mode = FIFO_MODE; + ret = geni_se_select_mode(gi2c->base, + mode); + } + } + } else { + dev_dbg(gi2c->dev, + "WRITE:n:%d,i:%d len:%d, stretch:%d, m_param:0x%x\n", + num, i, msgs[i].len, stretch, m_param); + geni_write_reg(msgs[i].len, gi2c->base, + SE_I2C_TX_TRANS_LEN); + m_cmd = I2C_WRITE; + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param); + if (mode == SE_DMA) { + ret = geni_se_tx_dma_prep(gi2c->wrapper_dev, + gi2c->base, msgs[i].buf, + msgs[i].len, &tx_dma); + if (ret) { + mode = FIFO_MODE; + ret = geni_se_select_mode(gi2c->base, + mode); + } + } + if (mode == FIFO_MODE) /* Get FIFO IRQ */ + geni_write_reg(1, gi2c->base, + SE_GENI_TX_WATERMARK_REG); + } + /* Ensure FIFO write go through before waiting for Done evet */ + mb(); + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ); + if (!timeout) { + geni_i2c_err(gi2c, GENI_TIMEOUT); + gi2c->cur = NULL; + geni_se_abort_m_cmd(gi2c->base); + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ); + } + gi2c->cur_wr = 0; + gi2c->cur_rd = 0; + if (mode == SE_DMA) { + if (gi2c->err) { + if (msgs[i].flags != I2C_M_RD) + writel_relaxed(1, gi2c->base + + SE_DMA_TX_FSM_RST); + else + writel_relaxed(1, gi2c->base + + SE_DMA_RX_FSM_RST); + wait_for_completion_timeout(&gi2c->xfer, HZ); + } + geni_se_rx_dma_unprep(gi2c->wrapper_dev, rx_dma, + msgs[i].len); + geni_se_tx_dma_unprep(gi2c->wrapper_dev, tx_dma, + msgs[i].len); + } + ret = gi2c->err; + if (gi2c->err) { + dev_err(gi2c->dev, "i2c error :%d\n", gi2c->err); + break; + } + } + if (ret == 0) + ret = num; + + pm_runtime_mark_last_busy(gi2c->dev); + pm_runtime_put_autosuspend(gi2c->dev); + gi2c->cur = NULL; + gi2c->err = 0; + dev_dbg(gi2c->dev, "i2c txn ret:%d\n", ret); + return ret; +} + +static u32 geni_i2c_func(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); +} + +static const struct i2c_algorithm geni_i2c_algo = { + .master_xfer = geni_i2c_xfer, + .functionality = geni_i2c_func, +}; + +static int geni_i2c_probe(struct platform_device *pdev) +{ + struct geni_i2c_dev *gi2c; + struct resource *res; + int ret; + + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL); + if (!gi2c) + return -ENOMEM; + + gi2c->dev = &pdev->dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -EINVAL; + + gi2c->wrapper_dev = pdev->dev.parent; + gi2c->i2c_rsc.wrapper_dev = pdev->dev.parent; + gi2c->i2c_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk"); + if (IS_ERR(gi2c->i2c_rsc.se_clk)) { + ret = PTR_ERR(gi2c->i2c_rsc.se_clk); + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); + return ret; + } + + gi2c->base = devm_ioremap_resource(gi2c->dev, res); + if (IS_ERR(gi2c->base)) + return PTR_ERR(gi2c->base); + + gi2c->i2c_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev); + if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_pinctrl)) { + dev_err(&pdev->dev, "No pinctrl config specified\n"); + ret = PTR_ERR(gi2c->i2c_rsc.geni_pinctrl); + return ret; + } + gi2c->i2c_rsc.geni_gpio_active = + pinctrl_lookup_state(gi2c->i2c_rsc.geni_pinctrl, + PINCTRL_DEFAULT); + if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_gpio_active)) { + dev_err(&pdev->dev, "No default config specified\n"); + ret = PTR_ERR(gi2c->i2c_rsc.geni_gpio_active); + return ret; + } + gi2c->i2c_rsc.geni_gpio_sleep = + pinctrl_lookup_state(gi2c->i2c_rsc.geni_pinctrl, + PINCTRL_SLEEP); + if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_gpio_sleep)) { + dev_err(&pdev->dev, "No sleep config specified\n"); + ret = PTR_ERR(gi2c->i2c_rsc.geni_gpio_sleep); + return ret; + } + + ret = device_property_read_u32(&pdev->dev, "clock-frequency", + &gi2c->clk_freq_out); + if (ret) { + dev_info(&pdev->dev, + "Bus frequency not specified, default to 400KHz.\n"); + gi2c->clk_freq_out = KHz(400); + } + + gi2c->irq = platform_get_irq(pdev, 0); + if (gi2c->irq < 0) { + dev_err(gi2c->dev, "IRQ error for i2c-geni\n"); + return gi2c->irq; + } + + ret = geni_i2c_clk_map_idx(gi2c); + if (ret) { + dev_err(gi2c->dev, "Invalid clk frequency %d KHz: %d\n", + gi2c->clk_freq_out, ret); + return ret; + } + + gi2c->adap.algo = &geni_i2c_algo; + init_completion(&gi2c->xfer); + platform_set_drvdata(pdev, gi2c); + ret = devm_request_irq(gi2c->dev, gi2c->irq, geni_i2c_irq, + IRQF_TRIGGER_HIGH, "i2c_geni", gi2c); + if (ret) { + dev_err(gi2c->dev, "Request_irq failed:%d: err:%d\n", + gi2c->irq, ret); + return ret; + } + disable_irq(gi2c->irq); + i2c_set_adapdata(&gi2c->adap, gi2c); + gi2c->adap.dev.parent = gi2c->dev; + gi2c->adap.dev.of_node = pdev->dev.of_node; + + strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name)); + + pm_runtime_set_suspended(gi2c->dev); + pm_runtime_set_autosuspend_delay(gi2c->dev, I2C_AUTO_SUSPEND_DELAY); + pm_runtime_use_autosuspend(gi2c->dev); + pm_runtime_enable(gi2c->dev); + i2c_add_adapter(&gi2c->adap); + + dev_dbg(gi2c->dev, "I2C probed\n"); + return 0; +} + +static int geni_i2c_remove(struct platform_device *pdev) +{ + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev); + + pm_runtime_disable(gi2c->dev); + i2c_del_adapter(&gi2c->adap); + return 0; +} + +static int geni_i2c_resume_noirq(struct device *device) +{ + return 0; +} + +#ifdef CONFIG_PM +static int geni_i2c_runtime_suspend(struct device *dev) +{ + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); + + disable_irq(gi2c->irq); + geni_se_resources_off(&gi2c->i2c_rsc); + return 0; +} + +static int geni_i2c_runtime_resume(struct device *dev) +{ + int ret; + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); + + ret = geni_se_resources_on(&gi2c->i2c_rsc); + if (ret) + return ret; + + if (!unlikely(gi2c->tx_wm)) { + int proto = geni_se_get_proto(gi2c->base); + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(gi2c->base); + + if (unlikely(proto != I2C)) { + dev_err(gi2c->dev, "Invalid proto %d\n", proto); + geni_se_resources_off(&gi2c->i2c_rsc); + return -ENXIO; + } + + gi2c->tx_wm = gi2c_tx_depth - 1; + geni_se_init(gi2c->base, gi2c->tx_wm, gi2c_tx_depth); + geni_se_config_packing(gi2c->base, 8, 4, true); + dev_dbg(gi2c->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", + gi2c_tx_depth); + } + enable_irq(gi2c->irq); + + return 0; +} + +static int geni_i2c_suspend_noirq(struct device *device) +{ + struct geni_i2c_dev *gi2c = dev_get_drvdata(device); + int ret; + + /* Make sure no transactions are pending */ + ret = i2c_trylock_bus(&gi2c->adap, I2C_LOCK_SEGMENT); + if (!ret) { + dev_err(gi2c->dev, "late I2C transaction request\n"); + return -EBUSY; + } + if (!pm_runtime_status_suspended(device)) { + geni_i2c_runtime_suspend(device); + pm_runtime_disable(device); + pm_runtime_set_suspended(device); + pm_runtime_enable(device); + } + i2c_unlock_bus(&gi2c->adap, I2C_LOCK_SEGMENT); + return 0; +} +#else +static int geni_i2c_runtime_suspend(struct device *dev) +{ + return 0; +} + +static int geni_i2c_runtime_resume(struct device *dev) +{ + return 0; +} + +static int geni_i2c_suspend_noirq(struct device *device) +{ + return 0; +} +#endif + +static const struct dev_pm_ops geni_i2c_pm_ops = { + .suspend_noirq = geni_i2c_suspend_noirq, + .resume_noirq = geni_i2c_resume_noirq, + .runtime_suspend = geni_i2c_runtime_suspend, + .runtime_resume = geni_i2c_runtime_resume, +}; + +static const struct of_device_id geni_i2c_dt_match[] = { + { .compatible = "qcom,i2c-geni" }, + {} +}; +MODULE_DEVICE_TABLE(of, geni_i2c_dt_match); + +static struct platform_driver geni_i2c_driver = { + .probe = geni_i2c_probe, + .remove = geni_i2c_remove, + .driver = { + .name = "i2c_geni", + .pm = &geni_i2c_pm_ops, + .of_match_table = geni_i2c_dt_match, + }, +}; + +module_platform_driver(geni_i2c_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:i2c_geni");