On Mon 31 Jan 04:04 PST 2022, Vinod Koul wrote:
> QUP Serial engines supports data transfers thru FIFO mode, SE DMA mode and
> lastly GPI DMA mode. Former two are already supported and this adds supports the
> last mode.
>
> In GPI DMA mode, the firmware is issued commands by driver to perform DMA
> and setup the serial port.
>
> Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> ---
>
> Changes since v4:
> - Fix buildbot warn
> - Fix flase warn reported by Alexey
> - Fix feedback from Bjorn and cleanup the probe code and add more details
> in changelog
>
> Changes since v3:
> - remove separate tx and rx function for gsi dma and make a common one
> - remove global structs and use local variables instead
>
> drivers/i2c/busses/i2c-qcom-geni.c | 300 ++++++++++++++++++++++++++---
> 1 file changed, 273 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 6d635a7c104c..696253d178a6 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -3,7 +3,9 @@
>
> #include <linux/acpi.h>
> #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> #include <linux/dma-mapping.h>
> +#include <linux/dma/qcom-gpi-dma.h>
> #include <linux/err.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> @@ -48,6 +50,9 @@
> #define LOW_COUNTER_SHFT 10
> #define CYCLE_COUNTER_MSK GENMASK(9, 0)
>
> +#define I2C_PACK_TX BIT(0)
> +#define I2C_PACK_RX BIT(1)
> +
> enum geni_i2c_err_code {
> GP_IRQ0,
> NACK,
> @@ -89,6 +94,9 @@ struct geni_i2c_dev {
> void *dma_buf;
> size_t xfer_len;
> dma_addr_t dma_addr;
> + struct dma_chan *tx_c;
> + struct dma_chan *rx_c;
> + bool gpi_mode;
> };
>
> struct geni_i2c_err_log {
> @@ -456,12 +464,199 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> return gi2c->err;
> }
>
> +static void i2c_gpi_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);
Iiuc the API the expectation is that if we get !NOERROR we shouldn't
expect to get NOERROR after that.
If so we're just returning here and leaving geni_i2c_gpi_xfer() to just
timeout in a HZ or so. Given that xfer happens under the adaptor lock,
how about carrying an error in geni_i2c_dev and complete(&done) here as
well?
> + return;
> + }
> +
> + if (result->residue)
> + dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue);
> +
> + complete(&gi2c->done);
> +}
> +
[..]
> +
> +static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int num)
> +{
> + struct dma_slave_config config = {};
> + struct gpi_i2c_config peripheral = {};
> + int i, ret = 0, timeout;
> + dma_addr_t tx_addr, rx_addr;
> + void *tx_buf = NULL, *rx_buf = NULL;
> + const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
> +
> + config.peripheral_config = &peripheral;
> + config.peripheral_size = sizeof(peripheral);
> +
> + peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
> + 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;
> + peripheral.set_config = 1;
> + peripheral.multi_msg = false;
> +
> + for (i = 0; i < num; i++) {
> + gi2c->cur = &msgs[i];
> + dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
> +
> + peripheral.stretch = 0;
> + if (i < num - 1)
> + peripheral.stretch = 1;
> +
> + peripheral.addr = msgs[i].addr;
> +
> + if (msgs[i].flags & I2C_M_RD) {
> + ret = geni_i2c_gpi(gi2c, &msgs[i], &config, &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
> + if (ret)
> + goto err;
> + }
> +
> + ret = geni_i2c_gpi(gi2c, &msgs[i], &config, &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
> + if (ret)
> + goto err;
> +
> + if (msgs[i].flags & I2C_M_RD)
> + dma_async_issue_pending(gi2c->rx_c);
> + 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 gpi flags:%d addr:0x%x\n",
> + gi2c->cur->flags, gi2c->cur->addr);
> + ret = gi2c->err = -ETIMEDOUT;
> + goto err;
> + }
> +
> + geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
> + }
> +
> + return 0;
> +
> +err:
> + dmaengine_terminate_sync(gi2c->rx_c);
> + dmaengine_terminate_sync(gi2c->tx_c);
> + geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
> + return ret;
> +}
[..]
> +static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
> +{
> + int ret;
> +
> + geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);
> + gi2c->tx_c = dma_request_chan(gi2c->se.dev, "tx");
> + if (IS_ERR(gi2c->tx_c)) {
> + ret = dev_err_probe(gi2c->se.dev, PTR_ERR(gi2c->tx_c),
> + "Failed to get tx DMA ch\n");
> + if (ret < 0)
> + goto err_tx;
> + }
> +
> + gi2c->rx_c = dma_request_chan(gi2c->se.dev, "rx");
> + if (IS_ERR(gi2c->rx_c)) {
> + ret = dev_err_probe(gi2c->se.dev, PTR_ERR(gi2c->rx_c),
> + "Failed to get rx DMA ch\n");
> + if (ret < 0)
> + goto err_rx;
> + }
> +
> + dev_dbg(gi2c->se.dev, "Grabbed GPI dma channels\n");
> + return 0;
> +
> +err_rx:
> + dma_release_channel(gi2c->tx_c);
> + gi2c->tx_c = NULL;
You're not accessing tx_c or rx_c again when returning an error here. So
I don't think there's a reason to clear them.
> +err_tx:
> + gi2c->rx_c = NULL;
> + return ret;
> +}
> +
[..]
> static int geni_i2c_remove(struct platform_device *pdev)
> {
> struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
>
> + release_gpi_dma(gi2c);
Your i2c devices aren't torn down until i2c_del_adapter(), so you might
still end up trying to use the two channels here, after releasing them.
In other words, I think you should reorder these.
Regards,
Bjorn
