diff mbox series

[6/6] spi: tegra210-quad: combined sequence mode

Message ID 1643970576-31503-7-git-send-email-kyarlagadda@nvidia.com (mailing list archive)
State Superseded
Headers show
Series Tegra QUAD SPI combined sequence mode | expand

Commit Message

Krishna Yarlagadda Feb. 4, 2022, 10:29 a.m. UTC
Add combined sequence mode supported by Tegra QSPI controller.
For commands which contain cmd, addr, data parts to it,controller
can accept all 3 transfers at once and xfer avoiding interrupt for each
transfer. This would improve read & write performance.

Signed-off-by: Krishna Yarlagadda <kyarlagadda@nvidia.com>
---
 drivers/spi/spi-tegra210-quad.c | 216 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 212 insertions(+), 4 deletions(-)

Comments

Mark Brown Feb. 4, 2022, 2:09 p.m. UTC | #1
On Fri, Feb 04, 2022 at 03:59:36PM +0530, Krishna Yarlagadda wrote:

> +	/* Process individual transfer list */
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if (transfer_phase == CMD_TRANSFER) {

> +		} else if (transfer_phase == ADDR_TRANSFER) {

> +		} else {

Looks like you're writing a switch statement here...

> +			/* X1 SDR mode */
> +			cmd_config = tegra_qspi_cmd_config(false, 0,
> +							   xfer->len);
> +			cmd_value = *((const u8 *)(xfer->tx_buf));
> +
> +			len = xfer->len;

> +			/* X1 SDR mode */
> +			addr_config = tegra_qspi_addr_config(false, 0,
> +							     xfer->len);
> +			address_value = *((const u32 *)(xfer->tx_buf));

> +			/* Program Command, Address value in register */
> +			tegra_qspi_writel(tqspi, cmd_value, QSPI_CMB_SEQ_CMD);
> +			tegra_qspi_writel(tqspi, address_value,
> +					  QSPI_CMB_SEQ_ADDR);
> +			/* Program Command and Address config in register */
> +			tegra_qspi_writel(tqspi, cmd_config,
> +					  QSPI_CMB_SEQ_CMD_CFG);
> +			tegra_qspi_writel(tqspi, addr_config,
> +					  QSPI_CMB_SEQ_ADDR_CFG);

It looks like the command and address have to be specific lengths?  If
that's the case then

> +	if (cdata->is_cmb_xfer && transfer_count == 3)
> +		ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> +	else
> +		ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);

This check needs to be more specific.  But like I said in reply to the
binding patch I don't see why we can't just pattern match on the data
without requiring a property here, we'd need to check that the message
is suitable no matter what.
Krishna Yarlagadda Feb. 7, 2022, 2:54 p.m. UTC | #2
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 04 February 2022 19:39
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>;
> linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; Sowjanya Komatineni
> <skomatineni@nvidia.com>; Laxman Dewangan <ldewangan@nvidia.com>;
> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; p.zabel@pengutronix.de
> Subject: Re: [PATCH 6/6] spi: tegra210-quad: combined sequence mode
> 
> On Fri, Feb 04, 2022 at 03:59:36PM +0530, Krishna Yarlagadda wrote:
> 
> > +	/* Process individual transfer list */
> > +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > +		if (transfer_phase == CMD_TRANSFER) {
> 
> > +		} else if (transfer_phase == ADDR_TRANSFER) {
> 
> > +		} else {
> 
> Looks like you're writing a switch statement here...
Yes. This can be switch statement.
> 
> > +			/* X1 SDR mode */
> > +			cmd_config = tegra_qspi_cmd_config(false, 0,
> > +							   xfer->len);
> > +			cmd_value = *((const u8 *)(xfer->tx_buf));
> > +
> > +			len = xfer->len;
> 
> > +			/* X1 SDR mode */
> > +			addr_config = tegra_qspi_addr_config(false, 0,
> > +							     xfer->len);
> > +			address_value = *((const u32 *)(xfer->tx_buf));
> 
> > +			/* Program Command, Address value in register */
> > +			tegra_qspi_writel(tqspi, cmd_value,
> QSPI_CMB_SEQ_CMD);
> > +			tegra_qspi_writel(tqspi, address_value,
> > +					  QSPI_CMB_SEQ_ADDR);
> > +			/* Program Command and Address config in register
> */
> > +			tegra_qspi_writel(tqspi, cmd_config,
> > +					  QSPI_CMB_SEQ_CMD_CFG);
> > +			tegra_qspi_writel(tqspi, addr_config,
> > +					  QSPI_CMB_SEQ_ADDR_CFG);
> 
> It looks like the command and address have to be specific lengths?  If that's the
> case then
Cmd  and address are configurable to a limit. Will add min and max check.
> 
> > +	if (cdata->is_cmb_xfer && transfer_count == 3)
> > +		ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> > +	else
> > +		ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);
> 
> This check needs to be more specific.  But like I said in reply to the binding
> patch I don't see why we can't just pattern match on the data without requiring
> a property here, we'd need to check that the message is suitable no matter
> what.
There is no real-world use case we encountered so far preventing us stick to pattern.
But this was to avoid any corner case where there could be 3 different transfers sent in single msg.
Mark Brown Feb. 7, 2022, 3:05 p.m. UTC | #3
On Mon, Feb 07, 2022 at 02:54:00PM +0000, Krishna Yarlagadda wrote:

> > > +	if (cdata->is_cmb_xfer && transfer_count == 3)
> > > +		ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> > > +	else
> > > +		ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);

> > This check needs to be more specific.  But like I said in reply to the binding
> > patch I don't see why we can't just pattern match on the data without requiring
> > a property here, we'd need to check that the message is suitable no matter
> > what.

> There is no real-world use case we encountered so far preventing us stick to pattern.
> But this was to avoid any corner case where there could be 3 different transfers sent in single msg.

So you'll remove the property and just pattern match on the message?

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.
Krishna Yarlagadda Feb. 7, 2022, 3:40 p.m. UTC | #4
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 07 February 2022 20:36
> To: Krishna Yarlagadda <kyarlagadda@nvidia.com>
> Cc: thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>;
> linux-spi@vger.kernel.org; linux-tegra@vger.kernel.org; Sowjanya
> Komatineni <skomatineni@nvidia.com>; Laxman Dewangan
> <ldewangan@nvidia.com>; robh+dt@kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> p.zabel@pengutronix.de
> Subject: Re: [PATCH 6/6] spi: tegra210-quad: combined sequence mode
> 
> On Mon, Feb 07, 2022 at 02:54:00PM +0000, Krishna Yarlagadda wrote:
> 
> > > > +	if (cdata->is_cmb_xfer && transfer_count == 3)
> > > > +		ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
> > > > +	else
> > > > +		ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);
> 
> > > This check needs to be more specific.  But like I said in reply to
> > > the binding patch I don't see why we can't just pattern match on the
> > > data without requiring a property here, we'd need to check that the
> > > message is suitable no matter what.
> 
> > There is no real-world use case we encountered so far preventing us stick
> to pattern.
> > But this was to avoid any corner case where there could be 3 different
> transfers sent in single msg.
> 
> So you'll remove the property and just pattern match on the message?
Yes. I will send out V2 without property.
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.
Sorry. Fixed mail client now.
diff mbox series

Patch

diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index c83701b..1c6cec8 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -121,19 +121,45 @@ 
 #define QSPI_NUM_DUMMY_CYCLE(x)			(((x) & 0xff) << 0)
 #define QSPI_DUMMY_CYCLES_MAX			0xff
 
+#define QSPI_CMB_SEQ_CMD			0x19c
+#define QSPI_COMMAND_VALUE_SET(X)		(((x) & 0xFF) << 0)
+
+#define QSPI_CMB_SEQ_CMD_CFG			0x1a0
+#define QSPI_COMMAND_X1_X2_X4(x)		(((x) & 0x3) << 13)
+#define QSPI_COMMAND_X1_X2_X4_MASK		(0x03 << 13)
+#define QSPI_COMMAND_SDR_DDR			BIT(12)
+#define QSPI_COMMAND_SIZE_SET(x)		(((x) & 0xFF) << 0)
+
+#define QSPI_GLOBAL_CONFIG			0X1a4
+#define QSPI_CMB_SEQ_EN				BIT(0)
+
+#define QSPI_CMB_SEQ_ADDR			0x1a8
+#define QSPI_ADDRESS_VALUE_SET(X)		(((x) & 0xFFFF) << 0)
+
+#define QSPI_CMB_SEQ_ADDR_CFG			0x1ac
+#define QSPI_ADDRESS_X1_X2_X4(x)		(((x) & 0x3) << 13)
+#define QSPI_ADDRESS_X1_X2_X4_MASK		(0x03 << 13)
+#define QSPI_ADDRESS_SDR_DDR			BIT(12)
+#define QSPI_ADDRESS_SIZE_SET(x)		(((x) & 0xFF) << 0)
+
 #define DATA_DIR_TX				BIT(0)
 #define DATA_DIR_RX				BIT(1)
 
 #define QSPI_DMA_TIMEOUT			(msecs_to_jiffies(1000))
 #define DEFAULT_QSPI_DMA_BUF_LEN		(64 * 1024)
+#define CMD_TRANSFER				0
+#define ADDR_TRANSFER				1
+#define DATA_TRANSFER				2
 
 struct tegra_qspi_soc_data {
 	bool has_dma;
+	bool cmb_xfer_capable;
 };
 
 struct tegra_qspi_client_data {
 	int tx_clk_tap_delay;
 	int rx_clk_tap_delay;
+	bool is_cmb_xfer;
 };
 
 struct tegra_qspi {
@@ -880,6 +906,7 @@  static int tegra_qspi_start_transfer_one(struct spi_device *spi,
 
 static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
 {
+	struct tegra_qspi *tqspi = spi_master_get_devdata(spi->master);
 	struct tegra_qspi_client_data *cdata;
 
 	cdata = devm_kzalloc(&spi->dev, sizeof(*cdata), GFP_KERNEL);
@@ -890,6 +917,12 @@  static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_devic
 				 &cdata->tx_clk_tap_delay);
 	device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
 				 &cdata->rx_clk_tap_delay);
+	if (tqspi->soc_data->cmb_xfer_capable)
+		cdata->is_cmb_xfer = device_property_read_bool
+					(&spi->dev,
+					 "nvidia,cmb-xfer");
+	else
+		cdata->is_cmb_xfer = false;
 
 	return cdata;
 }
@@ -912,7 +945,6 @@  static int tegra_qspi_setup(struct spi_device *spi)
 		cdata = tegra_qspi_parse_cdata_dt(spi);
 		spi->controller_data = cdata;
 	}
-
 	spin_lock_irqsave(&tqspi->lock, flags);
 
 	/* keep default cs state to inactive */
@@ -970,9 +1002,160 @@  static void tegra_qspi_transfer_end(struct spi_device *spi)
 	tegra_qspi_writel(tqspi, tqspi->def_command1_reg, QSPI_COMMAND1);
 }
 
-static int tegra_qspi_transfer_one_message(struct spi_master *master, struct spi_message *msg)
+static u32 tegra_qspi_cmd_config(bool is_ddr, u8 bus_width, u8 len)
+{
+	u32 cmd_config = 0;
+
+	/* Extract Command configuration and value */
+	if (is_ddr)
+		cmd_config |= QSPI_COMMAND_SDR_DDR;
+	else
+		cmd_config &= ~QSPI_COMMAND_SDR_DDR;
+
+	cmd_config |= QSPI_COMMAND_X1_X2_X4(bus_width);
+	cmd_config |= QSPI_COMMAND_SIZE_SET((len * 8) - 1);
+
+	return cmd_config;
+}
+
+static u32 tegra_qspi_addr_config(bool is_ddr, u8 bus_width, u8 len)
+{
+	u32 addr_config = 0;
+
+	/* Extract Address configuration and value */
+	is_ddr = 0; //Only SDR mode supported
+	bus_width = 0; //X1 mode
+
+	if (is_ddr)
+		addr_config |= QSPI_ADDRESS_SDR_DDR;
+	else
+		addr_config &= ~QSPI_ADDRESS_SDR_DDR;
+
+	addr_config |= QSPI_ADDRESS_X1_X2_X4(bus_width);
+	addr_config |= QSPI_ADDRESS_SIZE_SET((len * 8) - 1);
+
+	return addr_config;
+}
+
+static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
+					struct spi_message *msg)
+{
+	bool is_first_msg = true;
+	int single_xfer;
+	struct spi_transfer *xfer;
+	struct spi_device *spi = msg->spi;
+	u8 transfer_phase = 0;
+	u32 cmd1 = 0, dma_ctl = 0;
+	int ret;
+	u32 address_value = 0;
+	u32 cmd_config = 0, addr_config = 0;
+	u8 cmd_value = 0, len = 0, val = 0;
+
+	/* Enable Combined sequence mode */
+	val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
+	val |= QSPI_CMB_SEQ_EN;
+	tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
+	single_xfer = list_is_singular(&msg->transfers);
+	/* Process individual transfer list */
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (transfer_phase == CMD_TRANSFER) {
+			/* X1 SDR mode */
+			cmd_config = tegra_qspi_cmd_config(false, 0,
+							   xfer->len);
+			cmd_value = *((const u8 *)(xfer->tx_buf));
+
+		} else if (transfer_phase == ADDR_TRANSFER) {
+			len = xfer->len;
+			/* X1 SDR mode */
+			addr_config = tegra_qspi_addr_config(false, 0,
+							     xfer->len);
+			address_value = *((const u32 *)(xfer->tx_buf));
+		} else {
+			/* Program Command, Address value in register */
+			tegra_qspi_writel(tqspi, cmd_value, QSPI_CMB_SEQ_CMD);
+			tegra_qspi_writel(tqspi, address_value,
+					  QSPI_CMB_SEQ_ADDR);
+			/* Program Command and Address config in register */
+			tegra_qspi_writel(tqspi, cmd_config,
+					  QSPI_CMB_SEQ_CMD_CFG);
+			tegra_qspi_writel(tqspi, addr_config,
+					  QSPI_CMB_SEQ_ADDR_CFG);
+
+			reinit_completion(&tqspi->xfer_completion);
+			cmd1 = tegra_qspi_setup_transfer_one(spi, xfer,
+							     is_first_msg);
+			ret = tegra_qspi_start_transfer_one(spi, xfer,
+							    cmd1);
+
+			if (ret < 0) {
+				dev_err(tqspi->dev, "Failed to start transfer-one: %d\n",
+					ret);
+				return ret;
+			}
+
+			is_first_msg = false;
+			ret = wait_for_completion_timeout
+					(&tqspi->xfer_completion,
+					QSPI_DMA_TIMEOUT);
+
+			if (WARN_ON(ret == 0)) {
+				dev_err(tqspi->dev, "QSPI Transfer failed with timeout: %d\n",
+					ret);
+				if (tqspi->is_curr_dma_xfer &&
+				    (tqspi->cur_direction & DATA_DIR_TX))
+					dmaengine_terminate_all
+						(tqspi->tx_dma_chan);
+
+				if (tqspi->is_curr_dma_xfer &&
+				    (tqspi->cur_direction & DATA_DIR_RX))
+					dmaengine_terminate_all
+						(tqspi->rx_dma_chan);
+
+				/* Abort transfer by resetting pio/dma bit */
+				if (!tqspi->is_curr_dma_xfer) {
+					cmd1 = tegra_qspi_readl
+							(tqspi,
+							 QSPI_COMMAND1);
+					cmd1 &= ~QSPI_PIO;
+					tegra_qspi_writel
+							(tqspi, cmd1,
+							 QSPI_COMMAND1);
+				} else {
+					dma_ctl = tegra_qspi_readl
+							(tqspi,
+							 QSPI_DMA_CTL);
+					dma_ctl &= ~QSPI_DMA_EN;
+					tegra_qspi_writel(tqspi, dma_ctl,
+							  QSPI_DMA_CTL);
+				}
+
+				/* Reset controller if timeout happens */
+				device_reset(tqspi->dev);
+				ret = -EIO;
+				goto exit;
+			}
+
+			if (tqspi->tx_status ||  tqspi->rx_status) {
+				dev_err(tqspi->dev, "QSPI Transfer failed\n");
+				tqspi->tx_status = 0;
+				tqspi->rx_status = 0;
+				ret = -EIO;
+				goto exit;
+			}
+		}
+		msg->actual_length += xfer->len;
+		transfer_phase++;
+	}
+
+exit:
+	msg->status = ret;
+
+	return ret;
+}
+
+static int tegra_qspi_non_combined_seq_xfer(struct tegra_qspi *tqspi,
+					    struct spi_message *msg)
 {
-	struct tegra_qspi *tqspi = spi_master_get_devdata(master);
 	struct spi_device *spi = msg->spi;
 	struct spi_transfer *transfer;
 	bool is_first_msg = true;
@@ -1020,7 +1203,6 @@  static int tegra_qspi_transfer_one_message(struct spi_master *master, struct spi
 			goto complete_xfer;
 		}
 
-		is_first_msg = false;
 		ret = wait_for_completion_timeout(&tqspi->xfer_completion,
 						  QSPI_DMA_TIMEOUT);
 		if (WARN_ON(ret == 0)) {
@@ -1065,7 +1247,29 @@  static int tegra_qspi_transfer_one_message(struct spi_master *master, struct spi
 	ret = 0;
 exit:
 	msg->status = ret;
+
+	return ret;
+}
+
+static int tegra_qspi_transfer_one_message(struct spi_master *master,
+					   struct spi_message *msg)
+{
+	struct tegra_qspi *tqspi = spi_master_get_devdata(master);
+	struct tegra_qspi_client_data *cdata = msg->spi->controller_data;
+	int ret;
+	int transfer_count = 0;
+	struct spi_transfer *transfer;
+
+	list_for_each_entry(transfer, &msg->transfers, transfer_list) {
+		transfer_count++;
+	}
+	if (cdata->is_cmb_xfer && transfer_count == 3)
+		ret = tegra_qspi_combined_seq_xfer(tqspi, msg);
+	else
+		ret = tegra_qspi_non_combined_seq_xfer(tqspi, msg);
+
 	spi_finalize_current_message(master);
+
 	return ret;
 }
 
@@ -1199,14 +1403,17 @@  static irqreturn_t tegra_qspi_isr_thread(int irq, void *context_data)
 
 static struct tegra_qspi_soc_data tegra210_qspi_soc_data = {
 	.has_dma = true,
+	.cmb_xfer_capable = false,
 };
 
 static struct tegra_qspi_soc_data tegra186_qspi_soc_data = {
 	.has_dma = true,
+	.cmb_xfer_capable = true,
 };
 
 static struct tegra_qspi_soc_data tegra234_qspi_soc_data = {
 	.has_dma = false,
+	.cmb_xfer_capable = true,
 };
 
 static const struct of_device_id tegra_qspi_of_match[] = {
@@ -1277,6 +1484,7 @@  static int tegra_qspi_probe(struct platform_device *pdev)
 	tqspi->dev = &pdev->dev;
 	spin_lock_init(&tqspi->lock);
 
+	tqspi->soc_data = device_get_match_data(&pdev->dev);
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	tqspi->base = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(tqspi->base))