Message-ID: <Yg/ueBiOAgBOI8BX@ripper>
Date:   Fri, 18 Feb 2022 11:07:36 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Wolfram Sang <wsa@...nel.org>, linux-arm-msm@...r.kernel.org,
        Andy Gross <agross@...nel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alexey Minnekhanov <alexeymin@...tmarketos.org>,
        jorcrous@...zon.com, dmitry.baryshkov@...aro.org
Subject: Re: [PATCH v6] i2c: qcom-geni: Add support for GPI DMA

On Fri 18 Feb 10:55 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 for 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@...nel.org>
> ---
> 
> Changes since v5:
>  - Fix comments by Bjorn, Dmitry and Jordan

Thanks, but it's useful to know what those things where. Please list the
actual changes.

> 
> 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 | 303 ++++++++++++++++++++++++++---
>  1 file changed, 275 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 6d635a7c104c..e03544ba827c 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,202 @@ 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);
> +		goto err_trans;

Maybe I'm just missing it, but aren't you completing done now to avoid
the timeout, which causes the xfer to return success?

Note that, as I mentioned in previous review, we're under the adaptor
lock, so I don't think you need any additional locking to pass a value
back to xfer().

> +	}
> +
> +	if (result->residue)

You can avoid the goto, and make it clear from the overall structure that
only one (or none) of these paths will be taken.

	if (NOERROR) {

	} else if (result->residue) {

	}

Regards,
Bjorn

> +		dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue);
> +
> +err_trans:
> +	complete(&gi2c->done);
> +}
> +
> +static void geni_i2c_gpi_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> +			       void *tx_buf, dma_addr_t tx_addr,
> +			       void *rx_buf, dma_addr_t rx_addr)
> +{
> +	if (tx_buf) {
> +		dma_unmap_single(gi2c->se.dev->parent, tx_addr, msg->len, DMA_TO_DEVICE);
> +		i2c_put_dma_safe_msg_buf(tx_buf, msg, false);
> +	}
> +
> +	if (rx_buf) {
> +		dma_unmap_single(gi2c->se.dev->parent, rx_addr, msg->len, DMA_FROM_DEVICE);
> +		i2c_put_dma_safe_msg_buf(rx_buf, msg, false);
> +	}
> +}
> +
> +static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> +			   struct dma_slave_config *config, dma_addr_t *dma_addr_p,
> +			   void **buf, unsigned int op, struct dma_chan *dma_chan)
> +{
> +	struct gpi_i2c_config *peripheral;
> +	unsigned int flags;
> +	void *dma_buf;
> +	dma_addr_t addr;
> +	enum dma_data_direction map_dirn;
> +	enum dma_transfer_direction dma_dirn;
> +	struct dma_async_tx_descriptor *desc;
> +	int ret;
> +
> +	peripheral = config->peripheral_config;
> +
> +	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
> +	if (!dma_buf)
> +		return -ENOMEM;
> +
> +	if (op == I2C_WRITE)
> +		map_dirn = DMA_TO_DEVICE;
> +	else
> +		map_dirn = DMA_FROM_DEVICE;
> +
> +	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
> +	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
> +		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> +		return -ENOMEM;
> +	}
> +
> +	/* set the length as message for rx txn */
> +	peripheral->rx_len = msg->len;
> +	peripheral->op = op;
> +
> +	ret = dmaengine_slave_config(dma_chan, config);
> +	if (ret) {
> +		dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
> +		goto err_config;
> +	}
> +
> +	peripheral->set_config = 0;
> +	peripheral->multi_msg = true;
> +	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +
> +	if (op == I2C_WRITE)
> +		dma_dirn = DMA_MEM_TO_DEV;
> +	else
> +		dma_dirn = DMA_DEV_TO_MEM;
> +
> +	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
> +	if (!desc) {
> +		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
> +		ret = -EIO;
> +		goto err_config;
> +	}
> +
> +	desc->callback_result = i2c_gpi_cb_result;
> +	desc->callback_param = gi2c;
> +
> +	dmaengine_submit(desc);
> +	*dma_addr_p = addr;
> +
> +	return 0;
> +
> +err_config:
> +	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
> +	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> +	return ret;
> +}
> +
> +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 num;
> +
> +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 geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
> +			      struct i2c_msg msgs[], int num)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < num; i++) {
> +		u32 m_param = i < (num - 1) ? STOP_STRETCH : 0;
> +
> +		m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK);
> +
> +		gi2c->cur = &msgs[i];
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = geni_i2c_rx_one_msg(gi2c, &msgs[i], m_param);
> +		else
> +			ret = geni_i2c_tx_one_msg(gi2c, &msgs[i], m_param);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return num;
> +}
> +
>  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;
> +	int ret;
>  
>  	gi2c->err = 0;
>  	reinit_completion(&gi2c->done);
> @@ -475,28 +673,17 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  	}
>  
>  	qcom_geni_i2c_conf(gi2c);
> -	for (i = 0; i < num; i++) {
> -		u32 m_param = i < (num - 1) ? STOP_STRETCH : 0;
> -
> -		m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK);
> -
> -		gi2c->cur = &msgs[i];
> -		if (msgs[i].flags & I2C_M_RD)
> -			ret = geni_i2c_rx_one_msg(gi2c, &msgs[i], m_param);
> -		else
> -			ret = geni_i2c_tx_one_msg(gi2c, &msgs[i], m_param);
>  
> -		if (ret)
> -			break;
> -	}
> -	if (ret == 0)
> -		ret = num;
> +	if (gi2c->gpi_mode)
> +		ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
> +	else
> +		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
>  
>  	pm_runtime_mark_last_busy(gi2c->se.dev);
>  	pm_runtime_put_autosuspend(gi2c->se.dev);
>  	gi2c->cur = NULL;
>  	gi2c->err = 0;
> -	return ret;
> +	return num;
>  }
>  
>  static u32 geni_i2c_func(struct i2c_adapter *adap)
> @@ -517,11 +704,50 @@ static const struct acpi_device_id geni_i2c_acpi_match[] = {
>  MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
>  #endif
>  
> +static void release_gpi_dma(struct geni_i2c_dev *gi2c)
> +{
> +	if (gi2c->rx_c)
> +		dma_release_channel(gi2c->rx_c);
> +
> +	if (gi2c->tx_c)
> +		dma_release_channel(gi2c->tx_c);
> +}
> +
> +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);
> +err_tx:
> +	return ret;
> +}
> +
>  static int geni_i2c_probe(struct platform_device *pdev)
>  {
>  	struct geni_i2c_dev *gi2c;
>  	struct resource *res;
> -	u32 proto, tx_depth;
> +	u32 proto, tx_depth, fifo_disable;
>  	int ret;
>  	struct device *dev = &pdev->dev;
>  
> @@ -601,27 +827,43 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  	proto = geni_se_read_proto(&gi2c->se);
> -	tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>  	if (proto != GENI_SE_I2C) {
>  		dev_err(dev, "Invalid proto %d\n", proto);
>  		geni_se_resources_off(&gi2c->se);
>  		return -ENXIO;
>  	}
> -	gi2c->tx_wm = tx_depth - 1;
> -	geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> -	geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, PACKING_BYTES_PW,
> -							true, true, true);
> +
> +	fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +	if (fifo_disable) {
> +		/* FIFO is disabled, so we can only use GPI DMA */
> +		gi2c->gpi_mode = true;
> +		ret = setup_gpi_dma(gi2c);
> +		if (ret) {
> +			dev_err(dev, "Failed to setup GPI DMA mode:%d ret\n", ret);
> +			return ret;
> +		}
> +
> +		dev_dbg(dev, "Using GPI DMA mode for I2C\n");
> +	} else {
> +		gi2c->gpi_mode = false;
> +		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> +		gi2c->tx_wm = tx_depth - 1;
> +		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> +		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
> +				       PACKING_BYTES_PW, true, true, true);
> +
> +		dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> +	}
> +
>  	ret = geni_se_resources_off(&gi2c->se);
>  	if (ret) {
>  		dev_err(dev, "Error turning off resources %d\n", ret);
> -		return ret;
> +		goto err_dma;
>  	}
>  
>  	ret = geni_icc_disable(&gi2c->se);
>  	if (ret)
> -		return ret;
> -
> -	dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> +		goto err_dma;
>  
>  	gi2c->suspended = 1;
>  	pm_runtime_set_suspended(gi2c->se.dev);
> @@ -633,12 +875,16 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(dev, "Error adding i2c adapter %d\n", ret);
>  		pm_runtime_disable(gi2c->se.dev);
> -		return ret;
> +		goto err_dma;
>  	}
>  
>  	dev_dbg(dev, "Geni-I2C adaptor successfully added\n");
>  
>  	return 0;
> +
> +err_dma:
> +	release_gpi_dma(gi2c);
> +	return ret;
>  }
>  
>  static int geni_i2c_remove(struct platform_device *pdev)
> @@ -646,6 +892,7 @@ static int geni_i2c_remove(struct platform_device *pdev)
>  	struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
>  
>  	i2c_del_adapter(&gi2c->adap);
> +	release_gpi_dma(gi2c);
>  	pm_runtime_disable(gi2c->se.dev);
>  	return 0;
>  }
> -- 
> 2.31.1
> 

Powered by blists - more mailing lists