| Message ID | 20210111151651.1616813-6-vkoul@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add and enable GPI DMA users | expand |
On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > This adds capability to use GSI DMA for I2C transfers > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 246 ++++++++++++++++++++++++++++- > 1 file changed, 244 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 046d241183c5..6978480fb4d1 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -12,7 +12,9 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/dmaengine.h> > #include <linux/qcom-geni-se.h> > +#include <linux/dma/qcom-gpi-dma.h> > #include <linux/spinlock.h> > > #define SE_I2C_TX_TRANS_LEN 0x26c > @@ -48,6 +50,8 @@ > #define LOW_COUNTER_SHFT 10 > #define CYCLE_COUNTER_MSK GENMASK(9, 0) > > +#define I2C_PACK_EN (BIT(0) | BIT(1)) > + > enum geni_i2c_err_code { > GP_IRQ0, > NACK, > @@ -72,6 +76,12 @@ enum geni_i2c_err_code { > #define XFER_TIMEOUT HZ > #define RST_TIMEOUT HZ > > +enum i2c_se_mode { > + UNINITIALIZED, > + FIFO_SE_DMA, > + GSI_ONLY, > +}; > + > struct geni_i2c_dev { > struct geni_se se; > u32 tx_wm; > @@ -86,6 +96,17 @@ struct geni_i2c_dev { > u32 clk_freq_out; > const struct geni_i2c_clk_fld *clk_fld; > int suspended; > + struct dma_chan *tx_c; > + struct dma_chan *rx_c; > + dma_cookie_t rx_cookie, tx_cookie; > + dma_addr_t tx_ph; > + dma_addr_t rx_ph; > + int cfg_sent; bool? > + struct dma_async_tx_descriptor *tx_desc; > + struct dma_async_tx_descriptor *rx_desc; > + enum i2c_se_mode se_mode; bool gsi_only; > + bool cmd_done; Unused? > + bool is_shared; Used but meaningless? > }; > > struct geni_i2c_err_log { > @@ -429,6 +450,183 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > return gi2c->err; > } > > +static void i2c_gsi_cb_result(void *cb, const struct dmaengine_result *result) > +{ > + struct geni_i2c_dev *gi2c = cb; > + > + if (result->result != DMA_TRANS_NOERROR) { > + dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result); > + return; > + } > + > + if (result->residue) > + dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue); > + > + complete(&gi2c->done); > +} > + > +static int geni_i2c_gsi_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], > + int num) > +{ > + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); > + struct dma_slave_config config; > + struct gpi_i2c_config peripheral; > + int i, ret = 0, timeout = 0; > + > + memset(&config, 0, sizeof(config)); Assign {} to config during declaration. > + memset(&peripheral, 0, sizeof(peripheral)); > + config.peripheral_config = &peripheral; > + config.peripheral_size = sizeof(peripheral); > + > + if (!gi2c->tx_c) { > + gi2c->tx_c = dma_request_slave_channel(gi2c->se.dev, "tx"); So object is reused for all future transfers as well? Seems reasonable, but it should be released on driver removal? Could it be requested at probe time instead? > + if (!gi2c->tx_c) { > + dev_err(gi2c->se.dev, "tx dma_request_slave_channel fail\n"); > + ret = -EIO; > + goto geni_i2c_gsi_xfer_out; > + } > + } > + > + if (!gi2c->rx_c) { > + gi2c->rx_c = dma_request_slave_channel(gi2c->se.dev, "rx"); > + if (!gi2c->rx_c) { > + dev_err(gi2c->se.dev, "rx dma_request_slave_channel fail\n"); > + ret = -EIO; > + goto geni_i2c_gsi_xfer_out; > + } > + } > + > + if (!gi2c->cfg_sent) { > + const struct geni_i2c_clk_fld *itr = gi2c->clk_fld; > + > + peripheral.pack_enable = I2C_PACK_EN; > + peripheral.cycle_count = itr->t_cycle_cnt; > + peripheral.high_count = itr->t_high_cnt; > + peripheral.low_count = itr->t_low_cnt; > + peripheral.clk_div = itr->clk_div; > + gi2c->cfg_sent = true; Is this a bool or an int? > + peripheral.set_config = true; I find this somewhat ugly, you will always dmaengine_slave_config(&config), but in the case of cfg_sent this will point to an all-zero peripheral and hence will have set_config = false, which will cause the skipping of setting up a configuration TRE. I would prefer that the value of peripheral.set_config related to cfg_sent in a more explicit fashion. > + } > + > + peripheral.multi_msg = false; > + for (i = 0; i < num; i++) { > + struct device *rx_dev = gi2c->se.wrapper->dev; > + struct device *tx_dev = gi2c->se.wrapper->dev; > + int stretch = (i < (num - 1)); > + u8 *dma_buf = NULL; No need to initialize this, first use is an assignment. > + unsigned int flags; > + > + gi2c->cur = &msgs[i]; > + > + peripheral.addr = msgs[i].addr; > + peripheral.stretch = stretch; > + if (msgs[i].flags & I2C_M_RD) > + peripheral.op = I2C_READ; > + else > + peripheral.op = I2C_WRITE; > + > + dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); > + if (!dma_buf) { > + ret = -ENOMEM; > + goto geni_i2c_gsi_xfer_out; > + } > + > + if (msgs[i].flags & I2C_M_RD) { > + gi2c->rx_ph = dma_map_single(rx_dev, dma_buf, > + msgs[i].len, DMA_FROM_DEVICE); > + if (dma_mapping_error(rx_dev, gi2c->rx_ph)) { > + dev_err(gi2c->se.dev, "dma_map_single for rx failed :%d\n", ret); > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); > + goto geni_i2c_gsi_xfer_out; > + } > + > + peripheral.op = I2C_READ; > + peripheral.stretch = stretch; > + ret = dmaengine_slave_config(gi2c->rx_c, &config); > + if (ret) { > + dev_err(gi2c->se.dev, "rx dma config error:%d\n", ret); > + goto geni_i2c_gsi_xfer_out; Need to unmap rx_ph? > + } > + peripheral.set_config = false; > + peripheral.multi_msg = true; > + peripheral.rx_len = msgs[i].len; > + > + flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; > + gi2c->rx_desc = dmaengine_prep_slave_single(gi2c->rx_c, gi2c->rx_ph, > + msgs[i].len, > + DMA_DEV_TO_MEM, flags); Is the rx_desc freed by the dmaengine core when dma_async_issue_pending() finishes it's job? If so, why do you need to keep this pointer in gi2c? Wouldn't a local variable suffice? > + if (!gi2c->rx_desc) { > + dev_err(gi2c->se.dev, "prep_slave_sg for rx failed\n"); > + gi2c->err = -EIO; > + goto geni_i2c_err_prep_sg; > + } > + > + gi2c->rx_desc->callback_result = i2c_gsi_cb_result; > + gi2c->rx_desc->callback_param = gi2c; > + > + /* Issue RX */ > + gi2c->rx_cookie = dmaengine_submit(gi2c->rx_desc); > + dma_async_issue_pending(gi2c->rx_c); > + } > + > + dev_dbg(gi2c->se.dev, "msg[%d].len:%d W\n", i, gi2c->cur->len); > + gi2c->tx_ph = dma_map_single(tx_dev, dma_buf, msgs[i].len, DMA_TO_DEVICE); Maybe I've forgotten something important about I2C, but why do we always TX (even if it's a RX transfer)? > + if (dma_mapping_error(tx_dev, gi2c->tx_ph)) { > + dev_err(gi2c->se.dev, "dma_map_single for tx failed :%d\n", ret); > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); Need to unmap rx_ph? > + goto geni_i2c_gsi_xfer_out; > + } > + > + peripheral.stretch = stretch; > + peripheral.op = I2C_WRITE; > + ret = dmaengine_slave_config(gi2c->tx_c, &config); > + if (ret) { > + dev_err(gi2c->se.dev, "tx dma config error:%d\n", ret); Need to unmap rx_ph and tx_ph? > + goto geni_i2c_gsi_xfer_out; > + } > + peripheral.set_config = false; > + peripheral.multi_msg = true; > + gi2c->tx_desc = dmaengine_prep_slave_single(gi2c->tx_c, gi2c->tx_ph, msgs[i].len, > + DMA_MEM_TO_DEV, > + (DMA_PREP_INTERRUPT | DMA_CTRL_ACK)); > + if (!gi2c->tx_desc) { > + dev_err(gi2c->se.dev, "prep_slave_sg for tx failed\n"); > + gi2c->err = -ENOMEM; > + goto geni_i2c_err_prep_sg; > + } > + gi2c->tx_desc->callback_result = i2c_gsi_cb_result; > + gi2c->tx_desc->callback_param = gi2c; > + > + /* Issue TX */ > + gi2c->tx_cookie = dmaengine_submit(gi2c->tx_desc); > + dma_async_issue_pending(gi2c->tx_c); > + > + timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > + if (!timeout) { > + dev_err(gi2c->se.dev, "I2C timeout gsi flags:%d addr:0x%x\n", > + gi2c->cur->flags, gi2c->cur->addr); > + gi2c->err = -ETIMEDOUT; > + } > +geni_i2c_err_prep_sg: Perhaps you can break the body of this loop out to a separate function and thereby avoid the goto within the block? > + if (gi2c->err) { > + dmaengine_terminate_all(gi2c->tx_c); > + gi2c->cfg_sent = 0; Is this a bool or an int? > + } > + if (msgs[i].flags & I2C_M_RD) > + dma_unmap_single(rx_dev, gi2c->rx_ph, msgs[i].len, DMA_FROM_DEVICE); You unconditionally map tx_ph, but you only unmap it on ~I2C_M_RD. This fits better with my expectation, but would mean that the whole tx block above should be in an else. > + else > + dma_unmap_single(tx_dev, gi2c->tx_ph, msgs[i].len, DMA_TO_DEVICE); > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], !gi2c->err); > + if (gi2c->err) > + goto geni_i2c_gsi_xfer_out; This goto is just a "break" in disguise. > + } > + > +geni_i2c_gsi_xfer_out: > + if (!ret && gi2c->err) > + ret = gi2c->err; > + return ret; > +} > + > static int geni_i2c_xfer(struct i2c_adapter *adap, > struct i2c_msg msgs[], > int num) > @@ -448,6 +646,15 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > } > > qcom_geni_i2c_conf(gi2c); > + > + if (gi2c->se_mode == GSI_ONLY) { > + ret = geni_i2c_gsi_xfer(adap, msgs, num); > + goto geni_i2c_txn_ret; Rather than goto skip_non_gsi_code; I think you should move the non-gsi part of this function into a separate fifo function and make this if (GSI_ONLY) ret = geni_i2c_gsi_xfer(); else ret = geni_i2c_fifo_xfer(); > + } else { > + /* Don't set shared flag in non-GSI mode */ > + gi2c->is_shared = false; I don't see this flag being looked at elsewhere. > + } > + > for (i = 0; i < num; i++) { > u32 m_param = i < (num - 1) ? STOP_STRETCH : 0; > > @@ -462,6 +669,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > if (ret) > break; > } > +geni_i2c_txn_ret: > if (ret == 0) > ret = num; > > @@ -628,7 +836,8 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) > int ret; > struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); > > - disable_irq(gi2c->irq); > + if (gi2c->se_mode == FIFO_SE_DMA) > + disable_irq(gi2c->irq); > ret = geni_se_resources_off(&gi2c->se); > if (ret) { > enable_irq(gi2c->irq); > @@ -653,8 +862,41 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev) > ret = geni_se_resources_on(&gi2c->se); > if (ret) > return ret; > + if (gi2c->se_mode == UNINITIALIZED) { > + int proto = geni_se_read_proto(&gi2c->se); > + u32 se_mode; Please declare your variables at the top of the function. > + > + if (unlikely(proto != GENI_SE_I2C)) { If this was the case at probe time the driver would never have probed, why has it changed? This is not a fastpath, so skip the unlikely() > + dev_err(gi2c->se.dev, "Invalid proto %d\n", proto); > + geni_se_resources_off(&gi2c->se); > + return -ENXIO; > + } > + > + se_mode = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & > + FIFO_IF_DISABLE; se_mode would better be called "fifo_disabled" or perhaps logically suited "gsi_only"? Please skip the _relaxed > + if (se_mode) { > + gi2c->se_mode = GSI_ONLY; > + geni_se_select_mode(&gi2c->se, GENI_GPI_DMA); > + dev_dbg(gi2c->se.dev, "i2c GSI mode\n"); > + } else { > + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se); This variable has an unnecessarily long name. > + > + gi2c->se_mode = FIFO_SE_DMA; > + gi2c->tx_wm = gi2c_tx_depth - 1; > + geni_se_init(&gi2c->se, gi2c->tx_wm, gi2c_tx_depth); > + geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, > + PACKING_BYTES_PW, true, true, true); > + qcom_geni_i2c_conf(gi2c); > + dev_dbg(gi2c->se.dev, > + "i2c fifo/se-dma mode. fifo depth:%d\n", gi2c_tx_depth); > + } > + dev_dbg(gi2c->se.dev, "i2c-%d: %s\n", > + gi2c->adap.nr, dev_name(gi2c->se.dev)); dev_dbg() already provides dev_name. What information does this debug print actually try to communicate? Regards, Bjorn > + } > + > + if (gi2c->se_mode == FIFO_SE_DMA) > + enable_irq(gi2c->irq); > > - enable_irq(gi2c->irq); > gi2c->suspended = 0; > return 0; > } > -- > 2.26.2 >
On 11-01-21, 12:14, Bjorn Andersson wrote: > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > > > This adds capability to use GSI DMA for I2C transfers > > > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > drivers/i2c/busses/i2c-qcom-geni.c | 246 ++++++++++++++++++++++++++++- > > 1 file changed, 244 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > > index 046d241183c5..6978480fb4d1 100644 > > --- a/drivers/i2c/busses/i2c-qcom-geni.c > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > > @@ -12,7 +12,9 @@ > > #include <linux/of.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > +#include <linux/dmaengine.h> > > #include <linux/qcom-geni-se.h> > > +#include <linux/dma/qcom-gpi-dma.h> > > #include <linux/spinlock.h> > > > > #define SE_I2C_TX_TRANS_LEN 0x26c > > @@ -48,6 +50,8 @@ > > #define LOW_COUNTER_SHFT 10 > > #define CYCLE_COUNTER_MSK GENMASK(9, 0) > > > > +#define I2C_PACK_EN (BIT(0) | BIT(1)) > > + > > enum geni_i2c_err_code { > > GP_IRQ0, > > NACK, > > @@ -72,6 +76,12 @@ enum geni_i2c_err_code { > > #define XFER_TIMEOUT HZ > > #define RST_TIMEOUT HZ > > > > +enum i2c_se_mode { > > + UNINITIALIZED, > > + FIFO_SE_DMA, > > + GSI_ONLY, > > +}; > > + > > struct geni_i2c_dev { > > struct geni_se se; > > u32 tx_wm; > > @@ -86,6 +96,17 @@ struct geni_i2c_dev { > > u32 clk_freq_out; > > const struct geni_i2c_clk_fld *clk_fld; > > int suspended; > > + struct dma_chan *tx_c; > > + struct dma_chan *rx_c; > > + dma_cookie_t rx_cookie, tx_cookie; > > + dma_addr_t tx_ph; > > + dma_addr_t rx_ph; > > + int cfg_sent; > > bool? ok > > > + struct dma_async_tx_descriptor *tx_desc; > > + struct dma_async_tx_descriptor *rx_desc; > > + enum i2c_se_mode se_mode; > > bool gsi_only; I think fifo_mode would be more apt... since we check for other modes in the code > > > + bool cmd_done; > > Unused? heh, will remove.. > > + bool is_shared; > > Used but meaningless? Will drop > > > }; > > > > struct geni_i2c_err_log { > > @@ -429,6 +450,183 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > > return gi2c->err; > > } > > > > +static void i2c_gsi_cb_result(void *cb, const struct dmaengine_result *result) > > +{ > > + struct geni_i2c_dev *gi2c = cb; > > + > > + if (result->result != DMA_TRANS_NOERROR) { > > + dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result); > > + return; > > + } > > + > > + if (result->residue) > > + dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue); > > + > > + complete(&gi2c->done); > > +} > > + > > +static int geni_i2c_gsi_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], > > + int num) > > +{ > > + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); > > + struct dma_slave_config config; > > + struct gpi_i2c_config peripheral; > > + int i, ret = 0, timeout = 0; > > + > > + memset(&config, 0, sizeof(config)); > > Assign {} to config during declaration. ok > > > + memset(&peripheral, 0, sizeof(peripheral)); > > + config.peripheral_config = &peripheral; > > + config.peripheral_size = sizeof(peripheral); > > + > > + if (!gi2c->tx_c) { > > + gi2c->tx_c = dma_request_slave_channel(gi2c->se.dev, "tx"); > > So object is reused for all future transfers as well? > Seems reasonable, but it should be released on driver removal? > > Could it be requested at probe time instead? yes it can be done, i would move it.. > > > + if (!gi2c->tx_c) { > > + dev_err(gi2c->se.dev, "tx dma_request_slave_channel fail\n"); > > + ret = -EIO; > > + goto geni_i2c_gsi_xfer_out; > > + } > > + } > > + > > + if (!gi2c->rx_c) { > > + gi2c->rx_c = dma_request_slave_channel(gi2c->se.dev, "rx"); > > + if (!gi2c->rx_c) { > > + dev_err(gi2c->se.dev, "rx dma_request_slave_channel fail\n"); > > + ret = -EIO; > > + goto geni_i2c_gsi_xfer_out; > > + } > > + } > > + > > + if (!gi2c->cfg_sent) { > > + const struct geni_i2c_clk_fld *itr = gi2c->clk_fld; > > + > > + peripheral.pack_enable = I2C_PACK_EN; > > + peripheral.cycle_count = itr->t_cycle_cnt; > > + peripheral.high_count = itr->t_high_cnt; > > + peripheral.low_count = itr->t_low_cnt; > > + peripheral.clk_div = itr->clk_div; > > + gi2c->cfg_sent = true; > > Is this a bool or an int? Now would be a bool :) > > > + peripheral.set_config = true; > > I find this somewhat ugly, you will always > dmaengine_slave_config(&config), but in the case of cfg_sent this will > point to an all-zero peripheral and hence will have set_config = false, > which will cause the skipping of setting up a configuration TRE. > > I would prefer that the value of peripheral.set_config related to > cfg_sent in a more explicit fashion. Sure, i think I can use a single value to do this, will update this > > > + } > > + > > + peripheral.multi_msg = false; > > + for (i = 0; i < num; i++) { > > + struct device *rx_dev = gi2c->se.wrapper->dev; > > + struct device *tx_dev = gi2c->se.wrapper->dev; > > + int stretch = (i < (num - 1)); > > + u8 *dma_buf = NULL; > > No need to initialize this, first use is an assignment. ok > > > + unsigned int flags; > > + > > + gi2c->cur = &msgs[i]; > > + > > + peripheral.addr = msgs[i].addr; > > + peripheral.stretch = stretch; > > + if (msgs[i].flags & I2C_M_RD) > > + peripheral.op = I2C_READ; > > + else > > + peripheral.op = I2C_WRITE; > > + > > + dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); > > + if (!dma_buf) { > > + ret = -ENOMEM; > > + goto geni_i2c_gsi_xfer_out; > > + } > > + > > + if (msgs[i].flags & I2C_M_RD) { > > + gi2c->rx_ph = dma_map_single(rx_dev, dma_buf, > > + msgs[i].len, DMA_FROM_DEVICE); > > + if (dma_mapping_error(rx_dev, gi2c->rx_ph)) { > > + dev_err(gi2c->se.dev, "dma_map_single for rx failed :%d\n", ret); > > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); > > + goto geni_i2c_gsi_xfer_out; > > + } > > + > > + peripheral.op = I2C_READ; > > + peripheral.stretch = stretch; > > + ret = dmaengine_slave_config(gi2c->rx_c, &config); > > + if (ret) { > > + dev_err(gi2c->se.dev, "rx dma config error:%d\n", ret); > > + goto geni_i2c_gsi_xfer_out; > > Need to unmap rx_ph? yes will update > > > + } > > + peripheral.set_config = false; > > + peripheral.multi_msg = true; > > + peripheral.rx_len = msgs[i].len; > > + > > + flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; > > + gi2c->rx_desc = dmaengine_prep_slave_single(gi2c->rx_c, gi2c->rx_ph, > > + msgs[i].len, > > + DMA_DEV_TO_MEM, flags); > > Is the rx_desc freed by the dmaengine core when > dma_async_issue_pending() finishes it's job? Yes > If so, why do you need to keep this pointer in gi2c? Wouldn't a local > variable suffice? Yes local should suffice, will update > > > + if (!gi2c->rx_desc) { > > + dev_err(gi2c->se.dev, "prep_slave_sg for rx failed\n"); > > + gi2c->err = -EIO; > > + goto geni_i2c_err_prep_sg; > > + } > > + > > + gi2c->rx_desc->callback_result = i2c_gsi_cb_result; > > + gi2c->rx_desc->callback_param = gi2c; > > + > > + /* Issue RX */ > > + gi2c->rx_cookie = dmaengine_submit(gi2c->rx_desc); > > + dma_async_issue_pending(gi2c->rx_c); > > + } > > + > > + dev_dbg(gi2c->se.dev, "msg[%d].len:%d W\n", i, gi2c->cur->len); > > + gi2c->tx_ph = dma_map_single(tx_dev, dma_buf, msgs[i].len, DMA_TO_DEVICE); > > Maybe I've forgotten something important about I2C, but why do we always > TX (even if it's a RX transfer)? I think we need to send the device address for i2c, so even if we want to do RX, that will always involve a TX txn as well > > > + if (dma_mapping_error(tx_dev, gi2c->tx_ph)) { > > + dev_err(gi2c->se.dev, "dma_map_single for tx failed :%d\n", ret); > > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); > > Need to unmap rx_ph? > > > + goto geni_i2c_gsi_xfer_out; > > + } > > + > > + peripheral.stretch = stretch; > > + peripheral.op = I2C_WRITE; > > + ret = dmaengine_slave_config(gi2c->tx_c, &config); > > + if (ret) { > > + dev_err(gi2c->se.dev, "tx dma config error:%d\n", ret); > > Need to unmap rx_ph and tx_ph? Yeah looks like I missed unrolling, will check and update all these > > > + goto geni_i2c_gsi_xfer_out; > > + } > > + peripheral.set_config = false; > > + peripheral.multi_msg = true; > > + gi2c->tx_desc = dmaengine_prep_slave_single(gi2c->tx_c, gi2c->tx_ph, msgs[i].len, > > + DMA_MEM_TO_DEV, > > + (DMA_PREP_INTERRUPT | DMA_CTRL_ACK)); > > + if (!gi2c->tx_desc) { > > + dev_err(gi2c->se.dev, "prep_slave_sg for tx failed\n"); > > + gi2c->err = -ENOMEM; > > + goto geni_i2c_err_prep_sg; > > + } > > + gi2c->tx_desc->callback_result = i2c_gsi_cb_result; > > + gi2c->tx_desc->callback_param = gi2c; > > + > > + /* Issue TX */ > > + gi2c->tx_cookie = dmaengine_submit(gi2c->tx_desc); > > + dma_async_issue_pending(gi2c->tx_c); > > + > > + timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > > + if (!timeout) { > > + dev_err(gi2c->se.dev, "I2C timeout gsi flags:%d addr:0x%x\n", > > + gi2c->cur->flags, gi2c->cur->addr); > > + gi2c->err = -ETIMEDOUT; > > + } > > +geni_i2c_err_prep_sg: > > Perhaps you can break the body of this loop out to a separate function > and thereby avoid the goto within the block? > > > + if (gi2c->err) { > > + dmaengine_terminate_all(gi2c->tx_c); > > + gi2c->cfg_sent = 0; > > Is this a bool or an int? > > > + } > > + if (msgs[i].flags & I2C_M_RD) > > + dma_unmap_single(rx_dev, gi2c->rx_ph, msgs[i].len, DMA_FROM_DEVICE); > > You unconditionally map tx_ph, but you only unmap it on ~I2C_M_RD. This > fits better with my expectation, but would mean that the whole tx block > above should be in an else. > > > + else > > + dma_unmap_single(tx_dev, gi2c->tx_ph, msgs[i].len, DMA_TO_DEVICE); > > + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], !gi2c->err); > > + if (gi2c->err) > > + goto geni_i2c_gsi_xfer_out; > > This goto is just a "break" in disguise. > > > + } > > + > > +geni_i2c_gsi_xfer_out: > > + if (!ret && gi2c->err) > > + ret = gi2c->err; > > + return ret; > > +} > > + > > static int geni_i2c_xfer(struct i2c_adapter *adap, > > struct i2c_msg msgs[], > > int num) > > @@ -448,6 +646,15 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > > } > > > > qcom_geni_i2c_conf(gi2c); > > + > > + if (gi2c->se_mode == GSI_ONLY) { > > + ret = geni_i2c_gsi_xfer(adap, msgs, num); > > + goto geni_i2c_txn_ret; > > Rather than goto skip_non_gsi_code; I think you should move the non-gsi > part of this function into a separate fifo function and make this Okay let me take a relook at this whole blob and refactor it.. > > if (GSI_ONLY) > ret = geni_i2c_gsi_xfer(); > else > ret = geni_i2c_fifo_xfer(); > > > + } else { > > + /* Don't set shared flag in non-GSI mode */ > > + gi2c->is_shared = false; > > I don't see this flag being looked at elsewhere. > > > + } > > + > > for (i = 0; i < num; i++) { > > u32 m_param = i < (num - 1) ? STOP_STRETCH : 0; > > > > @@ -462,6 +669,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, > > if (ret) > > break; > > } > > +geni_i2c_txn_ret: > > if (ret == 0) > > ret = num; > > > > @@ -628,7 +836,8 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) > > int ret; > > struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); > > > > - disable_irq(gi2c->irq); > > + if (gi2c->se_mode == FIFO_SE_DMA) > > + disable_irq(gi2c->irq); > > ret = geni_se_resources_off(&gi2c->se); > > if (ret) { > > enable_irq(gi2c->irq); > > @@ -653,8 +862,41 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev) > > ret = geni_se_resources_on(&gi2c->se); > > if (ret) > > return ret; > > + if (gi2c->se_mode == UNINITIALIZED) { > > + int proto = geni_se_read_proto(&gi2c->se); > > + u32 se_mode; > > Please declare your variables at the top of the function. > > > + > > + if (unlikely(proto != GENI_SE_I2C)) { > > If this was the case at probe time the driver would never have probed, > why has it changed? > > This is not a fastpath, so skip the unlikely() > > > + dev_err(gi2c->se.dev, "Invalid proto %d\n", proto); > > + geni_se_resources_off(&gi2c->se); > > + return -ENXIO; > > + } > > + > > + se_mode = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & > > + FIFO_IF_DISABLE; > > se_mode would better be called "fifo_disabled" or perhaps logically > suited "gsi_only"? O think fifo_mode or just mode might be apt > > Please skip the _relaxed yes > > > + if (se_mode) { > > + gi2c->se_mode = GSI_ONLY; > > + geni_se_select_mode(&gi2c->se, GENI_GPI_DMA); > > + dev_dbg(gi2c->se.dev, "i2c GSI mode\n"); > > + } else { > > + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se); > > This variable has an unnecessarily long name. will shorten > > > + > > + gi2c->se_mode = FIFO_SE_DMA; > > + gi2c->tx_wm = gi2c_tx_depth - 1; > > + geni_se_init(&gi2c->se, gi2c->tx_wm, gi2c_tx_depth); > > + geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, > > + PACKING_BYTES_PW, true, true, true); > > + qcom_geni_i2c_conf(gi2c); > > + dev_dbg(gi2c->se.dev, > > + "i2c fifo/se-dma mode. fifo depth:%d\n", gi2c_tx_depth); > > + } > > + dev_dbg(gi2c->se.dev, "i2c-%d: %s\n", > > + gi2c->adap.nr, dev_name(gi2c->se.dev)); > > dev_dbg() already provides dev_name. What information does this debug > print actually try to communicate? not much am afraid, will update
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 046d241183c5..6978480fb4d1 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -12,7 +12,9 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/dmaengine.h> #include <linux/qcom-geni-se.h> +#include <linux/dma/qcom-gpi-dma.h> #include <linux/spinlock.h> #define SE_I2C_TX_TRANS_LEN 0x26c @@ -48,6 +50,8 @@ #define LOW_COUNTER_SHFT 10 #define CYCLE_COUNTER_MSK GENMASK(9, 0) +#define I2C_PACK_EN (BIT(0) | BIT(1)) + enum geni_i2c_err_code { GP_IRQ0, NACK, @@ -72,6 +76,12 @@ enum geni_i2c_err_code { #define XFER_TIMEOUT HZ #define RST_TIMEOUT HZ +enum i2c_se_mode { + UNINITIALIZED, + FIFO_SE_DMA, + GSI_ONLY, +}; + struct geni_i2c_dev { struct geni_se se; u32 tx_wm; @@ -86,6 +96,17 @@ struct geni_i2c_dev { u32 clk_freq_out; const struct geni_i2c_clk_fld *clk_fld; int suspended; + struct dma_chan *tx_c; + struct dma_chan *rx_c; + dma_cookie_t rx_cookie, tx_cookie; + dma_addr_t tx_ph; + dma_addr_t rx_ph; + int cfg_sent; + struct dma_async_tx_descriptor *tx_desc; + struct dma_async_tx_descriptor *rx_desc; + enum i2c_se_mode se_mode; + bool cmd_done; + bool is_shared; }; struct geni_i2c_err_log { @@ -429,6 +450,183 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, return gi2c->err; } +static void i2c_gsi_cb_result(void *cb, const struct dmaengine_result *result) +{ + struct geni_i2c_dev *gi2c = cb; + + if (result->result != DMA_TRANS_NOERROR) { + dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result); + return; + } + + if (result->residue) + dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue); + + complete(&gi2c->done); +} + +static int geni_i2c_gsi_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], + int num) +{ + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap); + struct dma_slave_config config; + struct gpi_i2c_config peripheral; + int i, ret = 0, timeout = 0; + + memset(&config, 0, sizeof(config)); + memset(&peripheral, 0, sizeof(peripheral)); + config.peripheral_config = &peripheral; + config.peripheral_size = sizeof(peripheral); + + if (!gi2c->tx_c) { + gi2c->tx_c = dma_request_slave_channel(gi2c->se.dev, "tx"); + if (!gi2c->tx_c) { + dev_err(gi2c->se.dev, "tx dma_request_slave_channel fail\n"); + ret = -EIO; + goto geni_i2c_gsi_xfer_out; + } + } + + if (!gi2c->rx_c) { + gi2c->rx_c = dma_request_slave_channel(gi2c->se.dev, "rx"); + if (!gi2c->rx_c) { + dev_err(gi2c->se.dev, "rx dma_request_slave_channel fail\n"); + ret = -EIO; + goto geni_i2c_gsi_xfer_out; + } + } + + if (!gi2c->cfg_sent) { + const struct geni_i2c_clk_fld *itr = gi2c->clk_fld; + + peripheral.pack_enable = I2C_PACK_EN; + peripheral.cycle_count = itr->t_cycle_cnt; + peripheral.high_count = itr->t_high_cnt; + peripheral.low_count = itr->t_low_cnt; + peripheral.clk_div = itr->clk_div; + gi2c->cfg_sent = true; + peripheral.set_config = true; + } + + peripheral.multi_msg = false; + for (i = 0; i < num; i++) { + struct device *rx_dev = gi2c->se.wrapper->dev; + struct device *tx_dev = gi2c->se.wrapper->dev; + int stretch = (i < (num - 1)); + u8 *dma_buf = NULL; + unsigned int flags; + + gi2c->cur = &msgs[i]; + + peripheral.addr = msgs[i].addr; + peripheral.stretch = stretch; + if (msgs[i].flags & I2C_M_RD) + peripheral.op = I2C_READ; + else + peripheral.op = I2C_WRITE; + + dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); + if (!dma_buf) { + ret = -ENOMEM; + goto geni_i2c_gsi_xfer_out; + } + + if (msgs[i].flags & I2C_M_RD) { + gi2c->rx_ph = dma_map_single(rx_dev, dma_buf, + msgs[i].len, DMA_FROM_DEVICE); + if (dma_mapping_error(rx_dev, gi2c->rx_ph)) { + dev_err(gi2c->se.dev, "dma_map_single for rx failed :%d\n", ret); + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); + goto geni_i2c_gsi_xfer_out; + } + + peripheral.op = I2C_READ; + peripheral.stretch = stretch; + ret = dmaengine_slave_config(gi2c->rx_c, &config); + if (ret) { + dev_err(gi2c->se.dev, "rx dma config error:%d\n", ret); + goto geni_i2c_gsi_xfer_out; + } + peripheral.set_config = false; + peripheral.multi_msg = true; + peripheral.rx_len = msgs[i].len; + + flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; + gi2c->rx_desc = dmaengine_prep_slave_single(gi2c->rx_c, gi2c->rx_ph, + msgs[i].len, + DMA_DEV_TO_MEM, flags); + if (!gi2c->rx_desc) { + dev_err(gi2c->se.dev, "prep_slave_sg for rx failed\n"); + gi2c->err = -EIO; + goto geni_i2c_err_prep_sg; + } + + gi2c->rx_desc->callback_result = i2c_gsi_cb_result; + gi2c->rx_desc->callback_param = gi2c; + + /* Issue RX */ + gi2c->rx_cookie = dmaengine_submit(gi2c->rx_desc); + dma_async_issue_pending(gi2c->rx_c); + } + + dev_dbg(gi2c->se.dev, "msg[%d].len:%d W\n", i, gi2c->cur->len); + gi2c->tx_ph = dma_map_single(tx_dev, dma_buf, msgs[i].len, DMA_TO_DEVICE); + if (dma_mapping_error(tx_dev, gi2c->tx_ph)) { + dev_err(gi2c->se.dev, "dma_map_single for tx failed :%d\n", ret); + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false); + goto geni_i2c_gsi_xfer_out; + } + + peripheral.stretch = stretch; + peripheral.op = I2C_WRITE; + ret = dmaengine_slave_config(gi2c->tx_c, &config); + if (ret) { + dev_err(gi2c->se.dev, "tx dma config error:%d\n", ret); + goto geni_i2c_gsi_xfer_out; + } + peripheral.set_config = false; + peripheral.multi_msg = true; + gi2c->tx_desc = dmaengine_prep_slave_single(gi2c->tx_c, gi2c->tx_ph, msgs[i].len, + DMA_MEM_TO_DEV, + (DMA_PREP_INTERRUPT | DMA_CTRL_ACK)); + if (!gi2c->tx_desc) { + dev_err(gi2c->se.dev, "prep_slave_sg for tx failed\n"); + gi2c->err = -ENOMEM; + goto geni_i2c_err_prep_sg; + } + gi2c->tx_desc->callback_result = i2c_gsi_cb_result; + gi2c->tx_desc->callback_param = gi2c; + + /* Issue TX */ + gi2c->tx_cookie = dmaengine_submit(gi2c->tx_desc); + dma_async_issue_pending(gi2c->tx_c); + + timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); + if (!timeout) { + dev_err(gi2c->se.dev, "I2C timeout gsi flags:%d addr:0x%x\n", + gi2c->cur->flags, gi2c->cur->addr); + gi2c->err = -ETIMEDOUT; + } +geni_i2c_err_prep_sg: + if (gi2c->err) { + dmaengine_terminate_all(gi2c->tx_c); + gi2c->cfg_sent = 0; + } + if (msgs[i].flags & I2C_M_RD) + dma_unmap_single(rx_dev, gi2c->rx_ph, msgs[i].len, DMA_FROM_DEVICE); + else + dma_unmap_single(tx_dev, gi2c->tx_ph, msgs[i].len, DMA_TO_DEVICE); + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], !gi2c->err); + if (gi2c->err) + goto geni_i2c_gsi_xfer_out; + } + +geni_i2c_gsi_xfer_out: + if (!ret && gi2c->err) + ret = gi2c->err; + return ret; +} + static int geni_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) @@ -448,6 +646,15 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, } qcom_geni_i2c_conf(gi2c); + + if (gi2c->se_mode == GSI_ONLY) { + ret = geni_i2c_gsi_xfer(adap, msgs, num); + goto geni_i2c_txn_ret; + } else { + /* Don't set shared flag in non-GSI mode */ + gi2c->is_shared = false; + } + for (i = 0; i < num; i++) { u32 m_param = i < (num - 1) ? STOP_STRETCH : 0; @@ -462,6 +669,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap, if (ret) break; } +geni_i2c_txn_ret: if (ret == 0) ret = num; @@ -628,7 +836,8 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) int ret; struct geni_i2c_dev *gi2c = dev_get_drvdata(dev); - disable_irq(gi2c->irq); + if (gi2c->se_mode == FIFO_SE_DMA) + disable_irq(gi2c->irq); ret = geni_se_resources_off(&gi2c->se); if (ret) { enable_irq(gi2c->irq); @@ -653,8 +862,41 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev) ret = geni_se_resources_on(&gi2c->se); if (ret) return ret; + if (gi2c->se_mode == UNINITIALIZED) { + int proto = geni_se_read_proto(&gi2c->se); + u32 se_mode; + + if (unlikely(proto != GENI_SE_I2C)) { + dev_err(gi2c->se.dev, "Invalid proto %d\n", proto); + geni_se_resources_off(&gi2c->se); + return -ENXIO; + } + + se_mode = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & + FIFO_IF_DISABLE; + if (se_mode) { + gi2c->se_mode = GSI_ONLY; + geni_se_select_mode(&gi2c->se, GENI_GPI_DMA); + dev_dbg(gi2c->se.dev, "i2c GSI mode\n"); + } else { + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se); + + gi2c->se_mode = FIFO_SE_DMA; + gi2c->tx_wm = gi2c_tx_depth - 1; + geni_se_init(&gi2c->se, gi2c->tx_wm, gi2c_tx_depth); + geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, + PACKING_BYTES_PW, true, true, true); + qcom_geni_i2c_conf(gi2c); + dev_dbg(gi2c->se.dev, + "i2c fifo/se-dma mode. fifo depth:%d\n", gi2c_tx_depth); + } + dev_dbg(gi2c->se.dev, "i2c-%d: %s\n", + gi2c->adap.nr, dev_name(gi2c->se.dev)); + } + + if (gi2c->se_mode == FIFO_SE_DMA) + enable_irq(gi2c->irq); - enable_irq(gi2c->irq); gi2c->suspended = 0; return 0; }
This adds capability to use GSI DMA for I2C transfers Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 246 ++++++++++++++++++++++++++++- 1 file changed, 244 insertions(+), 2 deletions(-) -- 2.26.2