linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/9] Some fixes for spi-s3c64xx
       [not found] <CGME20200821161404eucas1p20577160d1bff2e8f5cae7403e93716ab@eucas1p2.samsung.com>
@ 2020-08-21 16:13 ` Łukasz Stelmach
       [not found]   ` <CGME20200821161405eucas1p1d43a5970c6a26389cd506aab5f986bc8@eucas1p1.samsung.com>
                     ` (9 more replies)
  0 siblings, 10 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

This is a series of fixes created during porting a device driver (these
patches will be released soon too) for an SPI device to the current kernel.

The two most important are 

  spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
  spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

Without them DMA transfers larger than 512 bytes from the SPI controller
would fail.

Łukasz Stelmach (9):
  spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and
    s3c64xx_enable_datapath()
  spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
  spi: spi-s3c64xx: Report more information when errors occur
  spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*
  spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data
  spi: spi-s3c64xx: Check return values
  spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  spi: spi-s3c64xx: Increase transfer timeout
  spi: spi-s3c64xx: Turn on interrupts upon resume

 drivers/spi/spi-s3c64xx.c | 111 +++++++++++++++++++++++++++-----------
 1 file changed, 79 insertions(+), 32 deletions(-)

Changes in v2:
  - added missing commit descriptions
  - added spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  - implemented error propagation in
      spi: spi-s3c64xx: Check return values
  - rebased onto v5.9-rc1 which contains
      spi: spi-s3c64xx: Add missing entries for structs 's3c64xx_spi_dma_data' and 's3c64xx_spi_dma_data'
-- 
2.26.2


^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
       [not found]   ` <CGME20200821161405eucas1p1d43a5970c6a26389cd506aab5f986bc8@eucas1p1.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  2020-08-22 12:01       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

This and the next patch fix issues with DMA transfers bigger than
512 bytes on Exynos3250. Without the patches such transfers fail
to complete. This solution to the problem is found in the vendor kernel
for ARTIK5 boards based on Exynos3250.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 924b24441789..26c7cb79cd78 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -685,11 +685,11 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		sdd->state &= ~RXBUSY;
 		sdd->state &= ~TXBUSY;
 
-		s3c64xx_enable_datapath(sdd, xfer, use_dma);
-
 		/* Start the signals */
 		s3c64xx_spi_set_cs(spi, true);
 
+		s3c64xx_enable_datapath(sdd, xfer, use_dma);
+
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
 		if (use_dma)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
       [not found]   ` <CGME20200821161405eucas1p20aad659cd41bc4f56d5123d3c63394f0@eucas1p2.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  2020-08-22 12:02       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

This and the previous patch fix issues with DMA transfers bigger than
512 bytes on Exynos3250. Without the patches such transfers fail.

The vendor kernel for ARTIK5 handles CS in a simmilar way.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 26c7cb79cd78..22bf8c75580a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1379,6 +1379,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
 	.tx_st_done	= 25,
 	.high_speed	= true,
 	.clk_from_cmu	= true,
+	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
 static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 3/9] spi: spi-s3c64xx: Report more information when errors occur
       [not found]   ` <CGME20200821161405eucas1p19280babcd73926b5c22a48830f5fecd7@eucas1p1.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  2020-08-22 12:03       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Report amount of pending data when a transfer stops due to errors.

Report if DMA was used to transfer data and print the status code.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 22bf8c75580a..3364d362ed21 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -122,6 +122,7 @@
 
 struct s3c64xx_spi_dma_data {
 	struct dma_chan *ch;
+	dma_cookie_t cookie;
 	enum dma_transfer_direction direction;
 };
 
@@ -304,7 +305,7 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 	desc->callback = s3c64xx_spi_dmacb;
 	desc->callback_param = dma;
 
-	dmaengine_submit(desc);
+	dma->cookie = dmaengine_submit(desc);
 	dma_async_issue_pending(dma->ch);
 }
 
@@ -699,17 +700,28 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 
 		if (status) {
 			dev_err(&spi->dev,
-				"I/O Error: rx-%d tx-%d res:rx-%c tx-%c len-%d\n",
+				"I/O Error: rx-%d tx-%d rx-%c tx-%c len-%d dma-%d res-(%d)\n",
 				xfer->rx_buf ? 1 : 0, xfer->tx_buf ? 1 : 0,
 				(sdd->state & RXBUSY) ? 'f' : 'p',
 				(sdd->state & TXBUSY) ? 'f' : 'p',
-				xfer->len);
+				xfer->len, use_dma ? 1 : 0, status);
 
 			if (use_dma) {
-				if (xfer->tx_buf && (sdd->state & TXBUSY))
+				struct dma_tx_state s;
+
+				if (xfer->tx_buf && (sdd->state & TXBUSY)) {
+					dmaengine_pause(sdd->tx_dma.ch);
+					dmaengine_tx_status(sdd->tx_dma.ch, sdd->tx_dma.cookie, &s);
 					dmaengine_terminate_all(sdd->tx_dma.ch);
-				if (xfer->rx_buf && (sdd->state & RXBUSY))
+					dev_err(&spi->dev, "TX residue: %d\n", s.residue);
+
+				}
+				if (xfer->rx_buf && (sdd->state & RXBUSY)) {
+					dmaengine_pause(sdd->rx_dma.ch);
+					dmaengine_tx_status(sdd->rx_dma.ch, sdd->rx_dma.cookie, &s);
 					dmaengine_terminate_all(sdd->rx_dma.ch);
+					dev_err(&spi->dev, "RX residue: %d\n", s.residue);
+				}
 			}
 		} else {
 			s3c64xx_flush_fifo(sdd);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 4/9] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*
       [not found]   ` <CGME20200821161406eucas1p2be3221183a855afd0213f8ce58bd8942@eucas1p2.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* to match documentation.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/spi/spi-s3c64xx.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 3364d362ed21..433b9d77b914 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -29,7 +29,7 @@
 #define S3C64XX_SPI_CH_CFG		0x00
 #define S3C64XX_SPI_CLK_CFG		0x04
 #define S3C64XX_SPI_MODE_CFG		0x08
-#define S3C64XX_SPI_SLAVE_SEL		0x0C
+#define S3C64XX_SPI_CS_REG		0x0C
 #define S3C64XX_SPI_INT_EN		0x10
 #define S3C64XX_SPI_STATUS		0x14
 #define S3C64XX_SPI_TX_DATA		0x18
@@ -64,9 +64,9 @@
 #define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
 #define S3C64XX_SPI_MODE_4BURST			(1<<0)
 
-#define S3C64XX_SPI_SLAVE_AUTO			(1<<1)
-#define S3C64XX_SPI_SLAVE_SIG_INACT		(1<<0)
-#define S3C64XX_SPI_SLAVE_NSC_CNT_2		(2<<4)
+#define S3C64XX_SPI_CS_NSC_CNT_2		(2<<4)
+#define S3C64XX_SPI_CS_AUTO			(1<<1)
+#define S3C64XX_SPI_CS_SIG_INACT		(1<<0)
 
 #define S3C64XX_SPI_INT_TRAILING_EN		(1<<6)
 #define S3C64XX_SPI_INT_RX_OVERRUN_EN		(1<<5)
@@ -319,18 +319,18 @@ static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (enable) {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
-			writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			writel(0, sdd->regs + S3C64XX_SPI_CS_REG);
 		} else {
-			u32 ssel = readl(sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			u32 ssel = readl(sdd->regs + S3C64XX_SPI_CS_REG);
 
-			ssel |= (S3C64XX_SPI_SLAVE_AUTO |
-						S3C64XX_SPI_SLAVE_NSC_CNT_2);
-			writel(ssel, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			ssel |= (S3C64XX_SPI_CS_AUTO |
+						S3C64XX_SPI_CS_NSC_CNT_2);
+			writel(ssel, sdd->regs + S3C64XX_SPI_CS_REG);
 		}
 	} else {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-			writel(S3C64XX_SPI_SLAVE_SIG_INACT,
-			       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			writel(S3C64XX_SPI_CS_SIG_INACT,
+			       sdd->regs + S3C64XX_SPI_CS_REG);
 	}
 }
 
@@ -951,9 +951,9 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 	sdd->cur_speed = 0;
 
 	if (sci->no_cs)
-		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		writel(0, sdd->regs + S3C64XX_SPI_CS_REG);
 	else if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		writel(S3C64XX_SPI_CS_SIG_INACT, sdd->regs + S3C64XX_SPI_CS_REG);
 
 	/* Disable Interrupts - we use Polling if not DMA mode */
 	writel(0, regs + S3C64XX_SPI_INT_EN);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 5/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data
       [not found]   ` <CGME20200821161406eucas1p121553719d4e9cc020d2c557a69000f0c@eucas1p1.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  2020-08-22 12:26       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Remove descriptions for non-existent fields and fix indentation.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 433b9d77b914..6381a7557def 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -162,11 +162,8 @@ struct s3c64xx_spi_port_config {
  * @cntrlr_info: Platform specific data for the controller this driver manages.
  * @lock: Controller specific lock.
  * @state: Set of FLAGS to indicate status.
- * @rx_dmach: Controller's DMA channel for Rx.
- * @tx_dmach: Controller's DMA channel for Tx.
  * @sfr_start: BUS address of SPI controller regs.
  * @regs: Pointer to ioremap'ed controller registers.
- * @irq: interrupt
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.
@@ -183,7 +180,7 @@ struct s3c64xx_spi_driver_data {
 	struct clk                      *ioclk;
 	struct platform_device          *pdev;
 	struct spi_master               *master;
-	struct s3c64xx_spi_info  *cntrlr_info;
+	struct s3c64xx_spi_info         *cntrlr_info;
 	spinlock_t                      lock;
 	unsigned long                   sfr_start;
 	struct completion               xfer_completion;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
       [not found]   ` <CGME20200821161407eucas1p116af63a668bdbb75fa974589e5f6139f@eucas1p1.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  2020-08-22 12:37       ` Krzysztof Kozlowski
  2020-08-25 19:06       ` Sylwester Nawrocki
  0 siblings, 2 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Check return values in prepare_dma() and s3c64xx_spi_config() and
propagate errors upwards.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6381a7557def..02de734b8ab1 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -269,12 +269,13 @@ static void s3c64xx_spi_dmacb(void *data)
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
+static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
 			struct sg_table *sgt)
 {
 	struct s3c64xx_spi_driver_data *sdd;
 	struct dma_slave_config config;
 	struct dma_async_tx_descriptor *desc;
+	int ret;
 
 	memset(&config, 0, sizeof(config));
 
@@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 
 	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
 				       dma->direction, DMA_PREP_INTERRUPT);
+	if (!desc) {
+		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
+			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
+		return -ENOMEM;
+	}
 
 	desc->callback = s3c64xx_spi_dmacb;
 	desc->callback_param = dma;
 
 	dma->cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(dma->cookie);
+	if (ret) {
+		dev_err(&sdd->pdev->dev, "DMA submission failed");
+		return -EIO;
+	}
+
 	dma_async_issue_pending(dma->ch);
+	return 0;
 }
 
 static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
@@ -353,11 +366,12 @@ static bool s3c64xx_spi_can_dma(struct spi_master *master,
 	return xfer->len > (FIFO_LVL_MASK(sdd) >> 1) + 1;
 }
 
-static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
+static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 				    struct spi_transfer *xfer, int dma_mode)
 {
 	void __iomem *regs = sdd->regs;
 	u32 modecfg, chcfg;
+	int ret = 0;
 
 	modecfg = readl(regs + S3C64XX_SPI_MODE_CFG);
 	modecfg &= ~(S3C64XX_SPI_MODE_TXDMA_ON | S3C64XX_SPI_MODE_RXDMA_ON);
@@ -383,7 +397,7 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 		chcfg |= S3C64XX_SPI_CH_TXCH_ON;
 		if (dma_mode) {
 			modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
-			prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
+			ret = prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
 		} else {
 			switch (sdd->cur_bpw) {
 			case 32:
@@ -415,12 +429,17 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
 					| S3C64XX_SPI_PACKET_CNT_EN,
 					regs + S3C64XX_SPI_PACKET_CNT);
-			prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
+			ret = prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
 		}
 	}
 
+	if (ret)
+		return ret;
+
 	writel(modecfg, regs + S3C64XX_SPI_MODE_CFG);
 	writel(chcfg, regs + S3C64XX_SPI_CH_CFG);
+
+	return 0;
 }
 
 static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
@@ -553,9 +572,10 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 	return 0;
 }
 
-static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
+static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 {
 	void __iomem *regs = sdd->regs;
+	int ret;
 	u32 val;
 
 	/* Disable Clock */
@@ -603,7 +623,9 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 
 	if (sdd->port_conf->clk_from_cmu) {
 		/* The src_clk clock is divided internally by 2 */
-		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
+		ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
+		if (ret)
+			return ret;
 	} else {
 		/* Configure Clock */
 		val = readl(regs + S3C64XX_SPI_CLK_CFG);
@@ -617,6 +639,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 		val |= S3C64XX_SPI_ENCLK_ENABLE;
 		writel(val, regs + S3C64XX_SPI_CLK_CFG);
 	}
+
+	return 0;
 }
 
 #define XFER_DMAADDR_INVALID DMA_BIT_MASK(32)
@@ -659,7 +683,9 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		sdd->cur_bpw = bpw;
 		sdd->cur_speed = speed;
 		sdd->cur_mode = spi->mode;
-		s3c64xx_spi_config(sdd);
+		status = s3c64xx_spi_config(sdd);
+		if (status)
+			return status;
 	}
 
 	if (!is_polling(sdd) && (xfer->len > fifo_len) &&
@@ -686,10 +712,15 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		/* Start the signals */
 		s3c64xx_spi_set_cs(spi, true);
 
-		s3c64xx_enable_datapath(sdd, xfer, use_dma);
+		status = s3c64xx_enable_datapath(sdd, xfer, use_dma);
 
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
+		if (status) {
+			dev_err(&spi->dev, "failed to enable data path for transfer: %d\n", status);
+			break;
+		}
+
 		if (use_dma)
 			status = s3c64xx_wait_for_dma(sdd, xfer);
 		else
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
       [not found]   ` <CGME20200821161407eucas1p249e4833b8839f717cc2a496ab43bb8a2@eucas1p2.samsung.com>
@ 2020-08-21 16:13     ` Łukasz Stelmach
  2020-08-22 12:43       ` Krzysztof Kozlowski
  2020-08-22 14:54       ` Tomasz Figa
  0 siblings, 2 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:13 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach, Tomasz Figa

cur_speed is used to calculate transfer timeout and needs to be
set to the actual value of (half) the clock speed for precise
calculations.

Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 02de734b8ab1..89c162efe355 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 		ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 		if (ret)
 			return ret;
+		sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
 	} else {
 		/* Configure Clock */
 		val = readl(regs + S3C64XX_SPI_CLK_CFG);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 8/9] spi: spi-s3c64xx: Increase transfer timeout
       [not found]   ` <CGME20200821161407eucas1p23a283ac117d4381e087e9bacec537665@eucas1p2.samsung.com>
@ 2020-08-21 16:14     ` Łukasz Stelmach
  2020-08-22 12:43       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:14 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Increase timeout by 30 ms for some wiggle and set the minimum value to
100 ms. This ensures a non-zero value for short transfers which
may take less than 1 ms. The timeout value does not affect
performance because it is used with a completion.

Similar formula is used in other drivers e.g. sun4i, sun6i.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 89c162efe355..ea5a22dec53d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -473,7 +473,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 
 	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
 	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
-	ms += 10; /* some tolerance */
+	ms += 30;               /* some tolerance */
+	ms = max(ms, 100);      /* minimum timeout */
 
 	val = msecs_to_jiffies(ms) + 10;
 	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume
       [not found]   ` <CGME20200821161408eucas1p196aa4b954b3d19ad1b89a48dbbe41fbc@eucas1p1.samsung.com>
@ 2020-08-21 16:14     ` Łukasz Stelmach
  2020-08-22 12:44       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-08-21 16:14 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

s3c64xx_spi_hwinit() disables interrupts. In s3c64xx_spi_probe() after
calling s3c64xx_spi_hwinit() they are enabled with the following call.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ea5a22dec53d..53e3581bcd7b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1378,6 +1378,10 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
 
 	s3c64xx_spi_hwinit(sdd);
 
+	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
+	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
+	       sdd->regs + S3C64XX_SPI_INT_EN);
+
 	return 0;
 
 err_disable_src_clk:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
  2020-08-21 16:13     ` [PATCH v2 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath() Łukasz Stelmach
@ 2020-08-22 12:01       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:01 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:13:53PM +0200, Łukasz Stelmach wrote:
> This and the next patch fix issues with DMA transfers bigger than

Just:
"Fix issues with DMA transfers..."

The order of patches could change when applying and backporting.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


> 512 bytes on Exynos3250. Without the patches such transfers fail
> to complete. This solution to the problem is found in the vendor kernel
> for ARTIK5 boards based on Exynos3250.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 924b24441789..26c7cb79cd78 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -685,11 +685,11 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
>  		sdd->state &= ~RXBUSY;
>  		sdd->state &= ~TXBUSY;
>  
> -		s3c64xx_enable_datapath(sdd, xfer, use_dma);
> -
>  		/* Start the signals */
>  		s3c64xx_spi_set_cs(spi, true);
>  
> +		s3c64xx_enable_datapath(sdd, xfer, use_dma);
> +
>  		spin_unlock_irqrestore(&sdd->lock, flags);
>  
>  		if (use_dma)
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
  2020-08-21 16:13     ` [PATCH v2 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250 Łukasz Stelmach
@ 2020-08-22 12:02       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:02 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:13:54PM +0200, Łukasz Stelmach wrote:
> This and the previous patch fix issues with DMA transfers bigger than

"Fix issues with DMA..."

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


> 512 bytes on Exynos3250. Without the patches such transfers fail.
> 
> The vendor kernel for ARTIK5 handles CS in a simmilar way.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 26c7cb79cd78..22bf8c75580a 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1379,6 +1379,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
>  	.tx_st_done	= 25,
>  	.high_speed	= true,
>  	.clk_from_cmu	= true,
> +	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
>  };
>  
>  static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 3/9] spi: spi-s3c64xx: Report more information when errors occur
  2020-08-21 16:13     ` [PATCH v2 3/9] spi: spi-s3c64xx: Report more information when errors occur Łukasz Stelmach
@ 2020-08-22 12:03       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:03 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:13:55PM +0200, Łukasz Stelmach wrote:
> Report amount of pending data when a transfer stops due to errors.
> 
> Report if DMA was used to transfer data and print the status code.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>

I already reviewed v1. Include tags you receive on resubmissions.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 5/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data
  2020-08-21 16:13     ` [PATCH v2 5/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data Łukasz Stelmach
@ 2020-08-22 12:26       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:26 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:13:57PM +0200, Łukasz Stelmach wrote:
> Remove descriptions for non-existent fields and fix indentation.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
  2020-08-21 16:13     ` [PATCH v2 6/9] spi: spi-s3c64xx: Check return values Łukasz Stelmach
@ 2020-08-22 12:37       ` Krzysztof Kozlowski
  2020-08-25 19:06       ` Sylwester Nawrocki
  1 sibling, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:37 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:13:58PM +0200, Łukasz Stelmach wrote:
> Check return values in prepare_dma() and s3c64xx_spi_config() and
> propagate errors upwards.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 39 insertions(+), 8 deletions(-)

This should be a third patch - backportable fixes should go at
beginning.

Fixes: 788437273fa8 ("spi: s3c64xx: move to generic dmaengine API")
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-21 16:13     ` [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
@ 2020-08-22 12:43       ` Krzysztof Kozlowski
  2020-08-22 14:52         ` Tomasz Figa
       [not found]         ` <CGME20200824131716eucas1p16a3fde52aa765e7cd6584d4733762047@eucas1p1.samsung.com>
  2020-08-22 14:54       ` Tomasz Figa
  1 sibling, 2 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:43 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie,
	Tomasz Figa

On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> cur_speed is used to calculate transfer timeout and needs to be
> set to the actual value of (half) the clock speed for precise
> calculations.

If you need this only for timeout calculation just divide it in
s3c64xx_wait_for_dma(). Otherwise why only if (cmu) case is updated?

You are also affecting here not only timeout but
s3c64xx_enable_datapath() which is not mentioned in commit log. In other
words, this looks wrong.

Best regards,
Krzysztof

> 
> Cc: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 02de734b8ab1..89c162efe355 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>  		ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
>  		if (ret)
>  			return ret;
> +		sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>  	} else {
>  		/* Configure Clock */
>  		val = readl(regs + S3C64XX_SPI_CLK_CFG);
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 8/9] spi: spi-s3c64xx: Increase transfer timeout
  2020-08-21 16:14     ` [PATCH v2 8/9] spi: spi-s3c64xx: Increase transfer timeout Łukasz Stelmach
@ 2020-08-22 12:43       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:43 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:14:00PM +0200, Łukasz Stelmach wrote:
> Increase timeout by 30 ms for some wiggle and set the minimum value to
> 100 ms. This ensures a non-zero value for short transfers which
> may take less than 1 ms. The timeout value does not affect
> performance because it is used with a completion.
> 
> Similar formula is used in other drivers e.g. sun4i, sun6i.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume
  2020-08-21 16:14     ` [PATCH v2 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume Łukasz Stelmach
@ 2020-08-22 12:44       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 12:44 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

On Fri, Aug 21, 2020 at 06:14:01PM +0200, Łukasz Stelmach wrote:
> s3c64xx_spi_hwinit() disables interrupts. In s3c64xx_spi_probe() after
> calling s3c64xx_spi_hwinit() they are enabled with the following call.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-22 12:43       ` Krzysztof Kozlowski
@ 2020-08-22 14:52         ` Tomasz Figa
  2020-08-22 15:18           ` Krzysztof Kozlowski
       [not found]         ` <CGME20200824131716eucas1p16a3fde52aa765e7cd6584d4733762047@eucas1p1.samsung.com>
  1 sibling, 1 reply; 50+ messages in thread
From: Tomasz Figa @ 2020-08-22 14:52 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Łukasz Stelmach, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

On Sat, Aug 22, 2020 at 2:43 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> > cur_speed is used to calculate transfer timeout and needs to be
> > set to the actual value of (half) the clock speed for precise
> > calculations.
>
> If you need this only for timeout calculation just divide it in
> s3c64xx_wait_for_dma().

Division is not the point of the patch. The point is that
clk_set_rate() that was originally there doesn't guarantee that the
rate is set exactly. The rate directly determines the SPI transfer
speed and thus the driver needs to use the rate that was actually set
for further calculations.

> Otherwise why only if (cmu) case is updated?

Right, the !cmu case actually suffers from the same problem. The code
divides the parent clock rate with the requested speed to obtain the
divider to program into the register. This is subject to integer
rounding, so (parent / (parent / speed)) doesn't always equal (speed).

>
> You are also affecting here not only timeout but
> s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> words, this looks wrong.

Actually this is right and fixes one more problem, which I didn't spot
when looking at this code when I suggested the change (I only spotted
the effects on timeout calculation). The rounding error might have
caused wrong configuration there, because e.g. 30000000 Hz could be
requested and rounded to 28000000 Hz. The former is a threshold for
setting the S3C64XX_SPI_CH_HS_EN bit, but the real frequency wouldn't
actually require setting it.

Best regards,
Tomasz

>
> Best regards,
> Krzysztof
>
> >
> > Cc: Tomasz Figa <tfiga@chromium.org>
> > Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> > ---
> >  drivers/spi/spi-s3c64xx.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> > index 02de734b8ab1..89c162efe355 100644
> > --- a/drivers/spi/spi-s3c64xx.c
> > +++ b/drivers/spi/spi-s3c64xx.c
> > @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
> >               ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> >               if (ret)
> >                       return ret;
> > +             sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
> >       } else {
> >               /* Configure Clock */
> >               val = readl(regs + S3C64XX_SPI_CLK_CFG);
> > --
> > 2.26.2
> >

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-21 16:13     ` [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
  2020-08-22 12:43       ` Krzysztof Kozlowski
@ 2020-08-22 14:54       ` Tomasz Figa
  1 sibling, 0 replies; 50+ messages in thread
From: Tomasz Figa @ 2020-08-22 14:54 UTC (permalink / raw)
  To: linux-spi
  Cc: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Łukasz Stelmach,
	Marek Szyprowski, Bartlomiej Zolnierkiewicz

On Fri, Aug 21, 2020 at 6:14 PM Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>
> cur_speed is used to calculate transfer timeout and needs to be
> set to the actual value of (half) the clock speed for precise
> calculations.
>
> Cc: Tomasz Figa <tfiga@chromium.org>

As we talked on IRC, Lukasz forgot to add:

Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>

Would be nice if it could be added when (and if) applying.

> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 02de734b8ab1..89c162efe355 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>                 ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
>                 if (ret)
>                         return ret;
> +               sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>         } else {
>                 /* Configure Clock */
>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-22 14:52         ` Tomasz Figa
@ 2020-08-22 15:18           ` Krzysztof Kozlowski
  2020-08-22 15:20             ` Tomasz Figa
  0 siblings, 1 reply; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-08-22 15:18 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Łukasz Stelmach, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

On Sat, Aug 22, 2020 at 04:52:40PM +0200, Tomasz Figa wrote:
> On Sat, Aug 22, 2020 at 2:43 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> > > cur_speed is used to calculate transfer timeout and needs to be
> > > set to the actual value of (half) the clock speed for precise
> > > calculations.
> >
> > If you need this only for timeout calculation just divide it in
> > s3c64xx_wait_for_dma().
> 
> Division is not the point of the patch. The point is that
> clk_set_rate() that was originally there doesn't guarantee that the
> rate is set exactly.

Unfortunately onlt that point of timeout is mentioned in commit msg. If
the correction of timeout was not the point of the patch, then describe
the real point...

> The rate directly determines the SPI transfer
> speed and thus the driver needs to use the rate that was actually set
> for further calculations.

Yep, makes sense.

> 
> > Otherwise why only if (cmu) case is updated?
> 
> Right, the !cmu case actually suffers from the same problem. The code
> divides the parent clock rate with the requested speed to obtain the
> divider to program into the register. This is subject to integer
> rounding, so (parent / (parent / speed)) doesn't always equal (speed).

It is not only this problem. The meaning of cur_speed is now changed.
For !cmu_case this the requested in trasnfer SPI bus clock frequency,
for cmu_case this is now real src_clk frequency. It's getting slightly
messier.

> 
> >
> > You are also affecting here not only timeout but
> > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> > words, this looks wrong.
> 
> Actually this is right and fixes one more problem, which I didn't spot
> when looking at this code when I suggested the change (I only spotted
> the effects on timeout calculation). The rounding error might have
> caused wrong configuration there, because e.g. 30000000 Hz could be
> requested and rounded to 28000000 Hz. The former is a threshold for
> setting the S3C64XX_SPI_CH_HS_EN bit, but the real frequency wouldn't
> actually require setting it.

Wrong is here describing one thing and doing much more.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-22 15:18           ` Krzysztof Kozlowski
@ 2020-08-22 15:20             ` Tomasz Figa
  0 siblings, 0 replies; 50+ messages in thread
From: Tomasz Figa @ 2020-08-22 15:20 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Łukasz Stelmach, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

On Sat, Aug 22, 2020 at 5:18 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Sat, Aug 22, 2020 at 04:52:40PM +0200, Tomasz Figa wrote:
> > On Sat, Aug 22, 2020 at 2:43 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> > > > cur_speed is used to calculate transfer timeout and needs to be
> > > > set to the actual value of (half) the clock speed for precise
> > > > calculations.
> > >
> > > If you need this only for timeout calculation just divide it in
> > > s3c64xx_wait_for_dma().
> >
> > Division is not the point of the patch. The point is that
> > clk_set_rate() that was originally there doesn't guarantee that the
> > rate is set exactly.
>
> Unfortunately onlt that point of timeout is mentioned in commit msg. If
> the correction of timeout was not the point of the patch, then describe
> the real point...
>
> > The rate directly determines the SPI transfer
> > speed and thus the driver needs to use the rate that was actually set
> > for further calculations.
>
> Yep, makes sense.
>
> >
> > > Otherwise why only if (cmu) case is updated?
> >
> > Right, the !cmu case actually suffers from the same problem. The code
> > divides the parent clock rate with the requested speed to obtain the
> > divider to program into the register. This is subject to integer
> > rounding, so (parent / (parent / speed)) doesn't always equal (speed).
>
> It is not only this problem. The meaning of cur_speed is now changed.
> For !cmu_case this the requested in trasnfer SPI bus clock frequency,
> for cmu_case this is now real src_clk frequency. It's getting slightly
> messier.
>
> >
> > >
> > > You are also affecting here not only timeout but
> > > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> > > words, this looks wrong.
> >
> > Actually this is right and fixes one more problem, which I didn't spot
> > when looking at this code when I suggested the change (I only spotted
> > the effects on timeout calculation). The rounding error might have
> > caused wrong configuration there, because e.g. 30000000 Hz could be
> > requested and rounded to 28000000 Hz. The former is a threshold for
> > setting the S3C64XX_SPI_CH_HS_EN bit, but the real frequency wouldn't
> > actually require setting it.
>
> Wrong is here describing one thing and doing much more.

Yeah, agreed that the description could be improved. :)

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
       [not found]         ` <CGME20200824131716eucas1p16a3fde52aa765e7cd6584d4733762047@eucas1p1.samsung.com>
@ 2020-08-24 13:17           ` Lukasz Stelmach
  2020-08-24 13:21             ` Tomasz Figa
  0 siblings, 1 reply; 50+ messages in thread
From: Lukasz Stelmach @ 2020-08-24 13:17 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Kukjin Kim, Andi Shyti, Mark Brown, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie,
	Tomasz Figa

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:
> On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
>> cur_speed is used to calculate transfer timeout and needs to be
>> set to the actual value of (half) the clock speed for precise
>> calculations.
>
> If you need this only for timeout calculation just divide it in
> s3c64xx_wait_for_dma().

I divide it here to keep the relationship between the value the variable
holds and the one that is inside clk_* (See? It's multiplied 3 lines
above). If you look around every single clk_get_rate() call in the file is
divided by two.

> Otherwise why only if (cmu) case is updated?

You are righ I will update that too.

However, I wonder if it is even possible that the value read from
S3C64XX_SPI_CLK_CFG would be different than the one written to it?

> You are also affecting here not only timeout but
> s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> words, this looks wrong.

Indeed, there is a reference too. I've corrected the message.

>> 
>> Cc: Tomasz Figa <tfiga@chromium.org>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 02de734b8ab1..89c162efe355 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>>  		ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
>>  		if (ret)
>>  			return ret;
>> +		sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>>  	} else {
>>  		/* Configure Clock */
>>  		val = readl(regs + S3C64XX_SPI_CLK_CFG);
>> -- 
>> 2.26.2
>> 
>
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-24 13:17           ` Lukasz Stelmach
@ 2020-08-24 13:21             ` Tomasz Figa
       [not found]               ` <CGME20200825090211eucas1p1b63191fa778a775e33169ba2c1d3b74b@eucas1p1.samsung.com>
  0 siblings, 1 reply; 50+ messages in thread
From: Tomasz Figa @ 2020-08-24 13:21 UTC (permalink / raw)
  To: Lukasz Stelmach
  Cc: Krzysztof Kozlowski, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>
> It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:
> > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> >> cur_speed is used to calculate transfer timeout and needs to be
> >> set to the actual value of (half) the clock speed for precise
> >> calculations.
> >
> > If you need this only for timeout calculation just divide it in
> > s3c64xx_wait_for_dma().
>
> I divide it here to keep the relationship between the value the variable
> holds and the one that is inside clk_* (See? It's multiplied 3 lines
> above). If you look around every single clk_get_rate() call in the file is
> divided by two.
>
> > Otherwise why only if (cmu) case is updated?
>
> You are righ I will update that too.
>
> However, I wonder if it is even possible that the value read from
> S3C64XX_SPI_CLK_CFG would be different than the one written to it?
>

It is not possible for the register itself, but please see my other
reply, where I explained the integer rounding error which can happen
when calculating the value to write to the register.

> > You are also affecting here not only timeout but
> > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> > words, this looks wrong.
>
> Indeed, there is a reference too. I've corrected the message.
>

Thanks!

Best regards,
Tomasz

> >>
> >> Cc: Tomasz Figa <tfiga@chromium.org>
> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> >> ---
> >>  drivers/spi/spi-s3c64xx.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> index 02de734b8ab1..89c162efe355 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
> >>              ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> >>              if (ret)
> >>                      return ret;
> >> +            sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
> >>      } else {
> >>              /* Configure Clock */
> >>              val = readl(regs + S3C64XX_SPI_CLK_CFG);
> >> --
> >> 2.26.2
> >>
> >
> >
>
> --
> Łukasz Stelmach
> Samsung R&D Institute Poland
> Samsung Electronics

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
       [not found]               ` <CGME20200825090211eucas1p1b63191fa778a775e33169ba2c1d3b74b@eucas1p1.samsung.com>
@ 2020-08-25  9:01                 ` Lukasz Stelmach
  2020-08-25 15:11                   ` Tomasz Figa
  0 siblings, 1 reply; 50+ messages in thread
From: Lukasz Stelmach @ 2020-08-25  9:01 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Krzysztof Kozlowski, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

[-- Attachment #1: Type: text/plain, Size: 4102 bytes --]

It was <2020-08-24 pon 15:21>, when Tomasz Figa wrote:
> On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>>
>> It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:
>> > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
>> >> cur_speed is used to calculate transfer timeout and needs to be
>> >> set to the actual value of (half) the clock speed for precise
>> >> calculations.
>> >
>> > If you need this only for timeout calculation just divide it in
>> > s3c64xx_wait_for_dma().
>>
>> I divide it here to keep the relationship between the value the variable
>> holds and the one that is inside clk_* (See? It's multiplied 3 lines
>> above). If you look around every single clk_get_rate() call in the file is
>> divided by two.
>>
>> > Otherwise why only if (cmu) case is updated?
>>
>> You are righ I will update that too.
>>
>> However, I wonder if it is even possible that the value read from
>> S3C64XX_SPI_CLK_CFG would be different than the one written to it?
>>
>
> It is not possible for the register itself, but please see my other
> reply, where I explained the integer rounding error which can happen
> when calculating the value to write to the register.

I don't have any board to test it and Marek says there is only one that
doesn't use cmu *and* has an SPI device attached.

Here is what I think should work for the !cmu case.

--8<---------------cut here---------------start------------->8---
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 18b89e53ceda..5ebb1caade4d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -655,13 +655,18 @@ static int s3c64xx_spi_config(struct
s3c64xx_spi_driver_data *sdd)
                        return ret;
                sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
        } else {
+               int src_clk_rate = clk_get_rate(sdd->src_clk);
+               int clk_val = (src_clk_rate / sdd->cur_speed / 2 - 1);
+
                /* Configure Clock */
                val = readl(regs + S3C64XX_SPI_CLK_CFG);
                val &= ~S3C64XX_SPI_PSR_MASK;
-               val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1)
-                               & S3C64XX_SPI_PSR_MASK);
+               val |= (clk_val & S3C64XX_SPI_PSR_MASK);
                writel(val, regs + S3C64XX_SPI_CLK_CFG);

+               /* Keep the actual value */
+               sdd->cur_speed = src_clk_rate / (2 * (clk_val + 1));
+
                /* Enable Clock */
                val = readl(regs + S3C64XX_SPI_CLK_CFG);
                val |= S3C64XX_SPI_ENCLK_ENABLE;
--8<---------------cut here---------------end--------------->8---


>> > You are also affecting here not only timeout but
>> > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
>> > words, this looks wrong.
>>
>> Indeed, there is a reference too. I've corrected the message.
>>
>
> Thanks!
>
> Best regards,
> Tomasz
>
>> >>
>> >> Cc: Tomasz Figa <tfiga@chromium.org>
>> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> >> ---
>> >>  drivers/spi/spi-s3c64xx.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> >> index 02de734b8ab1..89c162efe355 100644
>> >> --- a/drivers/spi/spi-s3c64xx.c
>> >> +++ b/drivers/spi/spi-s3c64xx.c
>> >> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>> >>              ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
>> >>              if (ret)
>> >>                      return ret;
>> >> +            sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>> >>      } else {
>> >>              /* Configure Clock */
>> >>              val = readl(regs + S3C64XX_SPI_CLK_CFG);
>> >> --
>> >> 2.26.2
>> >>
>> >
>> >
>>
>> --
>> Łukasz Stelmach
>> Samsung R&D Institute Poland
>> Samsung Electronics
>
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-08-25  9:01                 ` Lukasz Stelmach
@ 2020-08-25 15:11                   ` Tomasz Figa
       [not found]                     ` <CGME20200825154611eucas1p284be8779ab484e675af071afef28376b@eucas1p2.samsung.com>
  0 siblings, 1 reply; 50+ messages in thread
From: Tomasz Figa @ 2020-08-25 15:11 UTC (permalink / raw)
  To: Lukasz Stelmach
  Cc: Krzysztof Kozlowski, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

On Tue, Aug 25, 2020 at 11:02 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>
> It was <2020-08-24 pon 15:21>, when Tomasz Figa wrote:
> > On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
> >>
> >> It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:
> >> > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
> >> >> cur_speed is used to calculate transfer timeout and needs to be
> >> >> set to the actual value of (half) the clock speed for precise
> >> >> calculations.
> >> >
> >> > If you need this only for timeout calculation just divide it in
> >> > s3c64xx_wait_for_dma().
> >>
> >> I divide it here to keep the relationship between the value the variable
> >> holds and the one that is inside clk_* (See? It's multiplied 3 lines
> >> above). If you look around every single clk_get_rate() call in the file is
> >> divided by two.
> >>
> >> > Otherwise why only if (cmu) case is updated?
> >>
> >> You are righ I will update that too.
> >>
> >> However, I wonder if it is even possible that the value read from
> >> S3C64XX_SPI_CLK_CFG would be different than the one written to it?
> >>
> >
> > It is not possible for the register itself, but please see my other
> > reply, where I explained the integer rounding error which can happen
> > when calculating the value to write to the register.
>
> I don't have any board to test it and Marek says there is only one that
> doesn't use cmu *and* has an SPI device attached.
>
> Here is what I think should work for the !cmu case.
>
> --8<---------------cut here---------------start------------->8---
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 18b89e53ceda..5ebb1caade4d 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -655,13 +655,18 @@ static int s3c64xx_spi_config(struct
> s3c64xx_spi_driver_data *sdd)
>                         return ret;
>                 sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>         } else {
> +               int src_clk_rate = clk_get_rate(sdd->src_clk);

The return value of clk_get_rate() is unsigned long.

> +               int clk_val = (src_clk_rate / sdd->cur_speed / 2 - 1);

Perhaps u32, since this is a value to be written to a 32-bit register.
Also if you could add a comment explaining that a negative overflow is
impossible:

/* s3c64xx_spi_setup() ensures that sdd->cur_speed <= src_clk_rate / 2. */

But actually, unless my lack of sleep is badly affecting my brain
processes, the original computation was completely wrong. Let's
consider the scenario below:

src_clk_rate = 8000000
sdd->cur_speed = 2500000

clk_val = 8000000 / 2500000 / 2 - 1 = 3 / 2 - 1 = 1 - 1 = 0
[...]
sdd->cur_speed = 8000000 / (2 * (0 + 1)) = 8000000 / (2 * 1) = 8000000
/ 2 = 4000000

So a request for 2.5 MHz ends up with 4 MHz, which could actually be
above the client device or link spec.

I believe the right thing to do would be DIV_ROUND_UP(src_clk_rate /
2, sdd->cur_speed) - 1. It's safe to divide src_clk_rate directly,
because those are normally high rates divisible by two without much
precision loss.

> +
>                 /* Configure Clock */
>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
>                 val &= ~S3C64XX_SPI_PSR_MASK;
> -               val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1)
> -                               & S3C64XX_SPI_PSR_MASK);
> +               val |= (clk_val & S3C64XX_SPI_PSR_MASK);
>                 writel(val, regs + S3C64XX_SPI_CLK_CFG);
>
> +               /* Keep the actual value */
> +               sdd->cur_speed = src_clk_rate / (2 * (clk_val + 1));

Also need to consider S3C64XX_SPI_PSR_MASK here, because clk_val could
actually be > S3C64XX_SPI_PSR_MASK.

Best regards,
Tomasz

> +
>                 /* Enable Clock */
>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
>                 val |= S3C64XX_SPI_ENCLK_ENABLE;
> --8<---------------cut here---------------end--------------->8---
>
>
> >> > You are also affecting here not only timeout but
> >> > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
> >> > words, this looks wrong.
> >>
> >> Indeed, there is a reference too. I've corrected the message.
> >>
> >
> > Thanks!
> >
> > Best regards,
> > Tomasz
> >
> >> >>
> >> >> Cc: Tomasz Figa <tfiga@chromium.org>
> >> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> >> >> ---
> >> >>  drivers/spi/spi-s3c64xx.c | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >>
> >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> >> index 02de734b8ab1..89c162efe355 100644
> >> >> --- a/drivers/spi/spi-s3c64xx.c
> >> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> >> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
> >> >>              ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> >> >>              if (ret)
> >> >>                      return ret;
> >> >> +            sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
> >> >>      } else {
> >> >>              /* Configure Clock */
> >> >>              val = readl(regs + S3C64XX_SPI_CLK_CFG);
> >> >> --
> >> >> 2.26.2
> >> >>
> >> >
> >> >
> >>
> >> --
> >> Łukasz Stelmach
> >> Samsung R&D Institute Poland
> >> Samsung Electronics
> >
> >
>
> --
> Łukasz Stelmach
> Samsung R&D Institute Poland
> Samsung Electronics

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
       [not found]                     ` <CGME20200825154611eucas1p284be8779ab484e675af071afef28376b@eucas1p2.samsung.com>
@ 2020-08-25 15:45                       ` Lukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Lukasz Stelmach @ 2020-08-25 15:45 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Krzysztof Kozlowski, Kukjin Kim, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc,
	list@263.net:IOMMU DRIVERS
	<iommu@lists.linux-foundation.org>,
	Joerg Roedel <joro@8bytes.org>,,
	Linux Kernel Mailing List, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz

[-- Attachment #1: Type: text/plain, Size: 6235 bytes --]

It was <2020-08-25 wto 17:11>, when Tomasz Figa wrote:
> On Tue, Aug 25, 2020 at 11:02 AM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>>
>> It was <2020-08-24 pon 15:21>, when Tomasz Figa wrote:
>> > On Mon, Aug 24, 2020 at 3:17 PM Lukasz Stelmach <l.stelmach@samsung.com> wrote:
>> >>
>> >> It was <2020-08-22 sob 14:43>, when Krzysztof Kozlowski wrote:
>> >> > On Fri, Aug 21, 2020 at 06:13:59PM +0200, Łukasz Stelmach wrote:
>> >> >> cur_speed is used to calculate transfer timeout and needs to be
>> >> >> set to the actual value of (half) the clock speed for precise
>> >> >> calculations.
>> >> >
>> >> > If you need this only for timeout calculation just divide it in
>> >> > s3c64xx_wait_for_dma().
>> >>
>> >> I divide it here to keep the relationship between the value the variable
>> >> holds and the one that is inside clk_* (See? It's multiplied 3 lines
>> >> above). If you look around every single clk_get_rate() call in the file is
>> >> divided by two.
>> >>
>> >> > Otherwise why only if (cmu) case is updated?
>> >>
>> >> You are righ I will update that too.
>> >>
>> >> However, I wonder if it is even possible that the value read from
>> >> S3C64XX_SPI_CLK_CFG would be different than the one written to it?
>> >>
>> >
>> > It is not possible for the register itself, but please see my other
>> > reply, where I explained the integer rounding error which can happen
>> > when calculating the value to write to the register.
>>
>> I don't have any board to test it and Marek says there is only one that
>> doesn't use cmu *and* has an SPI device attached.
>>
>> Here is what I think should work for the !cmu case.
>>
>> --8<---------------cut here---------------start------------->8---
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 18b89e53ceda..5ebb1caade4d 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -655,13 +655,18 @@ static int s3c64xx_spi_config(struct
>> s3c64xx_spi_driver_data *sdd)
>>                         return ret;
>>                 sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>>         } else {
>> +               int src_clk_rate = clk_get_rate(sdd->src_clk);
>
> The return value of clk_get_rate() is unsigned long.
>
>> +               int clk_val = (src_clk_rate / sdd->cur_speed / 2 - 1);
>
> Perhaps u32, since this is a value to be written to a 32-bit register.
> Also if you could add a comment explaining that a negative overflow is
> impossible:
>
> /* s3c64xx_spi_setup() ensures that sdd->cur_speed <= src_clk_rate / 2. */

OK.

> But actually, unless my lack of sleep is badly affecting my brain
> processes, the original computation was completely wrong. Let's
> consider the scenario below:
>
> src_clk_rate = 8000000
> sdd->cur_speed = 2500000
>
> clk_val = 8000000 / 2500000 / 2 - 1 = 3 / 2 - 1 = 1 - 1 = 0
> [...]
> sdd->cur_speed = 8000000 / (2 * (0 + 1)) = 8000000 / (2 * 1) = 8000000
> / 2 = 4000000
>
> So a request for 2.5 MHz ends up with 4 MHz, which could actually be
> above the client device or link spec.
>
> I believe the right thing to do would be DIV_ROUND_UP(src_clk_rate /
> 2, sdd->cur_speed) - 1. It's safe to divide src_clk_rate directly,
> because those are normally high rates divisible by two without much
> precision loss.

This makes sense.

>> +
>>                 /* Configure Clock */
>>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
>>                 val &= ~S3C64XX_SPI_PSR_MASK;
>> -               val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1)
>> -                               & S3C64XX_SPI_PSR_MASK);
>> +               val |= (clk_val & S3C64XX_SPI_PSR_MASK);
>>                 writel(val, regs + S3C64XX_SPI_CLK_CFG);
>>
>> +               /* Keep the actual value */
>> +               sdd->cur_speed = src_clk_rate / (2 * (clk_val + 1));
>
> Also need to consider S3C64XX_SPI_PSR_MASK here, because clk_val could
> actually be > S3C64XX_SPI_PSR_MASK.

Good point, but this

    clk_val = clk_val < 127 ? clk_val : 127;

seems more appropriate since masking may give very bogus value. Eg.

    src_clk_rate = 80000000 // 80 MHz
    cur_speed = 300000 // 300 kHz

    clk_val = 80000000/300000/2-1        => 132
    clk_val &= S3C64XX_SPI_PSR_MASK      => 4
    cur_speed = 80000000 / (2 * (4 + 1)) => 8000000 // 8 MHz


>> +
>>                 /* Enable Clock */
>>                 val = readl(regs + S3C64XX_SPI_CLK_CFG);
>>                 val |= S3C64XX_SPI_ENCLK_ENABLE;
>> --8<---------------cut here---------------end--------------->8---
>>
>>
>> >> > You are also affecting here not only timeout but
>> >> > s3c64xx_enable_datapath() which is not mentioned in commit log. In other
>> >> > words, this looks wrong.
>> >>
>> >> Indeed, there is a reference too. I've corrected the message.
>> >>
>> >
>> > Thanks!
>> >
>> > Best regards,
>> > Tomasz
>> >
>> >> >>
>> >> >> Cc: Tomasz Figa <tfiga@chromium.org>
>> >> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> >> >> ---
>> >> >>  drivers/spi/spi-s3c64xx.c | 1 +
>> >> >>  1 file changed, 1 insertion(+)
>> >> >>
>> >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> >> >> index 02de734b8ab1..89c162efe355 100644
>> >> >> --- a/drivers/spi/spi-s3c64xx.c
>> >> >> +++ b/drivers/spi/spi-s3c64xx.c
>> >> >> @@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>> >> >>              ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
>> >> >>              if (ret)
>> >> >>                      return ret;
>> >> >> +            sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
>> >> >>      } else {
>> >> >>              /* Configure Clock */
>> >> >>              val = readl(regs + S3C64XX_SPI_CLK_CFG);
>> >> >> --
>> >> >> 2.26.2
>> >> >>
>> >> >
>> >> >
>> >>
>> >> --
>> >> Łukasz Stelmach
>> >> Samsung R&D Institute Poland
>> >> Samsung Electronics
>> >
>> >
>>
>> --
>> Łukasz Stelmach
>> Samsung R&D Institute Poland
>> Samsung Electronics
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
  2020-08-21 16:13     ` [PATCH v2 6/9] spi: spi-s3c64xx: Check return values Łukasz Stelmach
  2020-08-22 12:37       ` Krzysztof Kozlowski
@ 2020-08-25 19:06       ` Sylwester Nawrocki
       [not found]         ` <CGME20200901152113eucas1p2977046b7a5b4c5a519f88870d658698a@eucas1p2.samsung.com>
  1 sibling, 1 reply; 50+ messages in thread
From: Sylwester Nawrocki @ 2020-08-25 19:06 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel,
	m.szyprowski, b.zolnierkie

On 8/21/20 18:13, Łukasz Stelmach wrote:
> Check return values in prepare_dma() and s3c64xx_spi_config() and
> propagate errors upwards.
> 
> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com>
> ---
>   drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 39 insertions(+), 8 deletions(-)

> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>   
>   	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>   				       dma->direction, DMA_PREP_INTERRUPT);
> +	if (!desc) {
> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
> +		return -ENOMEM;
> +	}
>   
>   	desc->callback = s3c64xx_spi_dmacb;
>   	desc->callback_param = dma;
>   
>   	dma->cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(dma->cookie);
> +	if (ret) {
> +		dev_err(&sdd->pdev->dev, "DMA submission failed");
> +		return -EIO;

Just return the error value from dma_submit_error() here?



^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
       [not found]         ` <CGME20200901152113eucas1p2977046b7a5b4c5a519f88870d658698a@eucas1p2.samsung.com>
@ 2020-09-01 15:21           ` Lukasz Stelmach
  2020-09-02  8:14             ` Sylwester Nawrocki
  0 siblings, 1 reply; 50+ messages in thread
From: Lukasz Stelmach @ 2020-09-01 15:21 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel,
	m.szyprowski, b.zolnierkie

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote:
> On 8/21/20 18:13, Łukasz Stelmach wrote:
>> Check return values in prepare_dma() and s3c64xx_spi_config() and
>> propagate errors upwards.
>>
>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com>
>> ---
>>   drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
>>   1 file changed, 39 insertions(+), 8 deletions(-)
>
>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>>     	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>>   				       dma->direction, DMA_PREP_INTERRUPT);
>> +	if (!desc) {
>> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
>> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
>> +		return -ENOMEM;
>> +	}
>>     	desc->callback = s3c64xx_spi_dmacb;
>>   	desc->callback_param = dma;
>>     	dma->cookie = dmaengine_submit(desc);
>> +	ret = dma_submit_error(dma->cookie);
>> +	if (ret) {
>> +		dev_err(&sdd->pdev->dev, "DMA submission failed");
>> +		return -EIO;
>
> Just return the error value from dma_submit_error() here?
>

--8<---------------cut here---------------start------------->8---
static inline int dma_submit_error(dma_cookie_t cookie)
{
        return cookie < 0 ? cookie : 0;

}
--8<---------------cut here---------------end--------------->8---

Not quite meaningful IMHO, is it?

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
  2020-09-01 15:21           ` Lukasz Stelmach
@ 2020-09-02  8:14             ` Sylwester Nawrocki
       [not found]               ` <CGME20200903084555eucas1p2f40375edb325107b68966fd52198b220@eucas1p2.samsung.com>
  0 siblings, 1 reply; 50+ messages in thread
From: Sylwester Nawrocki @ 2020-09-02  8:14 UTC (permalink / raw)
  To: Lukasz Stelmach
  Cc: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel,
	m.szyprowski, b.zolnierkie

On 9/1/20 17:21, Lukasz Stelmach wrote:
> It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote:
>> On 8/21/20 18:13, Łukasz Stelmach wrote:
>>> Check return values in prepare_dma() and s3c64xx_spi_config() and
>>> propagate errors upwards.
>>>
>>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com>
>>> ---
>>>    drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
>>>    1 file changed, 39 insertions(+), 8 deletions(-)
>>
>>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>>>      	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>>>    				       dma->direction, DMA_PREP_INTERRUPT);
>>> +	if (!desc) {
>>> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
>>> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
>>> +		return -ENOMEM;
>>> +	}
>>>      	desc->callback = s3c64xx_spi_dmacb;
>>>    	desc->callback_param = dma;
>>>      	dma->cookie = dmaengine_submit(desc);
>>> +	ret = dma_submit_error(dma->cookie);
>>> +	if (ret) {
>>> +		dev_err(&sdd->pdev->dev, "DMA submission failed");
>>> +		return -EIO;
>>
>> Just return the error value from dma_submit_error() here?
>>
> 
> --8<---------------cut here---------------start------------->8---
> static inline int dma_submit_error(dma_cookie_t cookie)
> {
>          return cookie < 0 ? cookie : 0;
> 
> }
> --8<---------------cut here---------------end--------------->8---
> 
> Not quite meaningful IMHO, is it?

dma_submit_error() returns 0 or an error code, I think it makes sense
to propagate that error code rather than replacing it with -EIO.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
       [not found]               ` <CGME20200903084555eucas1p2f40375edb325107b68966fd52198b220@eucas1p2.samsung.com>
@ 2020-09-03  8:45                 ` Lukasz Stelmach
  2020-09-03 11:18                   ` Sylwester Nawrocki
  0 siblings, 1 reply; 50+ messages in thread
From: Lukasz Stelmach @ 2020-09-03  8:45 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel,
	m.szyprowski, b.zolnierkie

[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]

It was <2020-09-02 śro 10:14>, when Sylwester Nawrocki wrote:
> On 9/1/20 17:21, Lukasz Stelmach wrote:
>> It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote:
>>> On 8/21/20 18:13, Łukasz Stelmach wrote:
>>>> Check return values in prepare_dma() and s3c64xx_spi_config() and
>>>> propagate errors upwards.
>>>>
>>>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com>
>>>> ---
>>>>    drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
>>>>    1 file changed, 39 insertions(+), 8 deletions(-)
>>>
>>>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>>>>      	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>>>>    				       dma->direction, DMA_PREP_INTERRUPT);
>>>> +	if (!desc) {
>>>> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
>>>> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
>>>> +		return -ENOMEM;
>>>> +	}
>>>>      	desc->callback = s3c64xx_spi_dmacb;
>>>>    	desc->callback_param = dma;
>>>>      	dma->cookie = dmaengine_submit(desc);
>>>> +	ret = dma_submit_error(dma->cookie);
>>>> +	if (ret) {
>>>> +		dev_err(&sdd->pdev->dev, "DMA submission failed");
>>>> +		return -EIO;
>>>
>>> Just return the error value from dma_submit_error() here?
>>>
>>
>> --8<---------------cut here---------------start------------->8---
>> static inline int dma_submit_error(dma_cookie_t cookie)
>> {
>>          return cookie < 0 ? cookie : 0;
>>
>> }
>> --8<---------------cut here---------------end--------------->8---
>>
>> Not quite meaningful IMHO, is it?
>
> dma_submit_error() returns 0 or an error code, I think it makes sense
> to propagate that error code rather than replacing it with -EIO.

It is not an error code that d_s_e() returns it is a value returned by
dma_cookie_assigned() called from within the tx_submit() operaton of a
DMA driver.

--8<---------------cut here---------------start------------->8---
static inline dma_cookie_t dma_cookie_assign(struct
dma_async_tx_descriptor *tx)
{
        struct dma_chan *chan = tx->chan;
        dma_cookie_t cookie;

        cookie = chan->cookie + 1;
        if (cookie < DMA_MIN_COOKIE)
                cookie = DMA_MIN_COOKIE;
        tx->cookie = chan->cookie = cookie;

        return cookie;
}
--8<---------------cut here---------------end--------------->8---

Yes, a non-zero value returned by d_s_e() indicates an error but it
definitely isn't one of error codes from errno*.h.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
  2020-09-03  8:45                 ` Lukasz Stelmach
@ 2020-09-03 11:18                   ` Sylwester Nawrocki
  0 siblings, 0 replies; 50+ messages in thread
From: Sylwester Nawrocki @ 2020-09-03 11:18 UTC (permalink / raw)
  To: Lukasz Stelmach
  Cc: Kukjin Kim, Krzysztof Kozlowski, Andi Shyti, Mark Brown,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel,
	m.szyprowski, b.zolnierkie

On 9/3/20 10:45, Lukasz Stelmach wrote:
> It was <2020-09-02 śro 10:14>, when Sylwester Nawrocki wrote:
>> On 9/1/20 17:21, Lukasz Stelmach wrote:
>>> It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote:
>>>> On 8/21/20 18:13, Łukasz Stelmach wrote:
>>>>> Check return values in prepare_dma() and s3c64xx_spi_config() and
>>>>> propagate errors upwards.
>>>>>
>>>>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com>
>>>>> ---
>>>>>     drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++-------
>>>>>     1 file changed, 39 insertions(+), 8 deletions(-)
>>>>
>>>>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>>>>>       	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>>>>>     				       dma->direction, DMA_PREP_INTERRUPT);
>>>>> +	if (!desc) {
>>>>> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
>>>>> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
>>>>> +		return -ENOMEM;
>>>>> +	}
>>>>>       	desc->callback = s3c64xx_spi_dmacb;
>>>>>     	desc->callback_param = dma;
>>>>>       	dma->cookie = dmaengine_submit(desc);
>>>>> +	ret = dma_submit_error(dma->cookie);
>>>>> +	if (ret) {
>>>>> +		dev_err(&sdd->pdev->dev, "DMA submission failed");
>>>>> +		return -EIO;
>>>>
>>>> Just return the error value from dma_submit_error() here?
>>>>
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> static inline int dma_submit_error(dma_cookie_t cookie)
>>> {
>>>           return cookie < 0 ? cookie : 0;
>>>
>>> }
>>> --8<---------------cut here---------------end--------------->8---
>>>
>>> Not quite meaningful IMHO, is it?
>>
>> dma_submit_error() returns 0 or an error code, I think it makes sense
>> to propagate that error code rather than replacing it with -EIO.
> 
> It is not an error code that d_s_e() returns it is a value returned by
> dma_cookie_assign() called from within the tx_submit() operation of a
> DMA driver.
> 
> --8<---------------cut here---------------start------------->8---
> static inline dma_cookie_t dma_cookie_assign(struct
> dma_async_tx_descriptor *tx)
> {
>          struct dma_chan *chan = tx->chan;
>          dma_cookie_t cookie;
> 
>          cookie = chan->cookie + 1;
>          if (cookie < DMA_MIN_COOKIE)
>                  cookie = DMA_MIN_COOKIE;
>          tx->cookie = chan->cookie = cookie;
> 
>          return cookie;
> }
> --8<---------------cut here---------------end--------------->8---
> 
> Yes, a non-zero value returned by d_s_e() indicates an error but it
> definitely isn't one of error codes from errno*.h.

I guess we can end that discussion at this point and keep your patch
as is, I just followed comment at the dma_submit_error() function:

"if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code"
 
AFAICS dma_cookie_assign() always returns value > 0 and dma_submit_error()
only returns the cookie if its value is < 0 so in consequence d_s_e() will 
be always returning 0 in your case (PL330 DMAC)?

The below commit, likely a result of static code analysis, might be 
a confirmation. It could also explain why some drivers overwrite the return
value of d_s_e() and some just pass it up to the callers.

--------------------------------8<------------------------------------
commit 71ea148370f8b6c745a8a42f6fd983cf5ebade18
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date:   Sat Aug 10 10:46:50 2013 +0300

    dmaengine: make dma_submit_error() return an error code
    
    The problem here is that the dma_xfer() functions in
    drivers/ata/pata_arasan_cf.c and drivers/mtd/nand/fsmc_nand.c expect
    dma_submit_error() to return an error code so they return 1 when they
    intended to return a negative.
    
    So far as I can tell, none of the ->tx_submit() functions ever do
    return error codes so this patch should have no effect in the current
    code.
    
    I also changed it from a define to an inline.
    
    Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
    Signed-off-by: Dan Williams <djbw@fb.com>

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cb286b1a..b3ba7e4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -38,7 +38,10 @@ typedef s32 dma_cookie_t;
 #define DMA_MIN_COOKIE 1
 #define DMA_MAX_COOKIE INT_MAX
 
-#define dma_submit_error(cookie) ((cookie) < 0 ? 1 : 0)
+static inline int dma_submit_error(dma_cookie_t cookie)
+{
+       return cookie < 0 ? cookie : 0;
+}
--------------------------------8<------------------------------------




^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx
       [not found]   ` <CGME20201001152246eucas1p1ee289b8a85b707f7496355c081623796@eucas1p1.samsung.com>
@ 2020-10-01 15:21     ` Łukasz Stelmach
       [not found]       ` <CGME20201001152246eucas1p1b4155ab4f06a39cc88f205b4a7cd47f9@eucas1p1.samsung.com>
                         ` (9 more replies)
  0 siblings, 10 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

This is a series of fixes created during porting a device driver (these
patches will be released soon too) for an SPI device to the current kernel.

The two most important are

  spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
  spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

Without them DMA transfers larger than 512 bytes from the SPI controller
would fail.

Łukasz Stelmach (9):
  spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and
    s3c64xx_enable_datapath()
  spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
  spi: spi-s3c64xx: Check return values
  spi: spi-s3c64xx: Report more information when errors occur
  spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*
  spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data
  spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  spi: spi-s3c64xx: Increase transfer timeout
  spi: spi-s3c64xx: Turn on interrupts upon resume

 drivers/spi/spi-s3c64xx.c | 111 +++++++++++++++++++++++++++-----------
 1 file changed, 79 insertions(+), 32 deletions(-)

Changes in v2:
  - added missing commit descriptions
  - added spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  - implemented error propagation in
      spi: spi-s3c64xx: Check return values
  - rebased onto v5.9-rc1 which contains
      spi: spi-s3c64xx: Add missing entries for structs 's3c64xx_spi_dma_data' and 's3c64xx_spi_dma_data'
-- 
2.26.2

^ permalink raw reply	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
       [not found]       ` <CGME20201001152246eucas1p1b4155ab4f06a39cc88f205b4a7cd47f9@eucas1p1.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Fix issues with DMA transfers bigger than 512 bytes on Exynos3250. Without
the patches such transfers fail to complete. This solution to the problem
is found in the vendor kernel for ARTIK5 boards based on Exynos3250.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 924b24441789..26c7cb79cd78 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -685,11 +685,11 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		sdd->state &= ~RXBUSY;
 		sdd->state &= ~TXBUSY;
 
-		s3c64xx_enable_datapath(sdd, xfer, use_dma);
-
 		/* Start the signals */
 		s3c64xx_spi_set_cs(spi, true);
 
+		s3c64xx_enable_datapath(sdd, xfer, use_dma);
+
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
 		if (use_dma)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
       [not found]       ` <CGME20201001152246eucas1p2fb22ab55c276d76c4508142842c90ab8@eucas1p2.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  2020-10-01 19:04           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Fix issues with DMA transfers bigger than 512 bytes on Exynos3250. Without
the patches such transfers fail.

The vendor kernel for ARTIK5 handles CS in a simmilar way.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 26c7cb79cd78..22bf8c75580a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1379,6 +1379,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
 	.tx_st_done	= 25,
 	.high_speed	= true,
 	.clk_from_cmu	= true,
+	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
 static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 3/9] spi: spi-s3c64xx: Check return values
       [not found]       ` <CGME20201001152247eucas1p2b6b1cc61b9b175b0a621609cd58effbd@eucas1p2.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Check return values in prepare_dma() and s3c64xx_spi_config() and
propagate errors upwards.

Fixes: 788437273fa8 ("spi: s3c64xx: move to generic dmaengine API")
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 50 ++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 22bf8c75580a..f7482f2f1e13 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -122,6 +122,7 @@
 
 struct s3c64xx_spi_dma_data {
 	struct dma_chan *ch;
+	dma_cookie_t cookie;
 	enum dma_transfer_direction direction;
 };
 
@@ -271,12 +272,13 @@ static void s3c64xx_spi_dmacb(void *data)
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
+static int prepare_dma(struct s3c64xx_spi_dma_data *dma,
 			struct sg_table *sgt)
 {
 	struct s3c64xx_spi_driver_data *sdd;
 	struct dma_slave_config config;
 	struct dma_async_tx_descriptor *desc;
+	int ret;
 
 	memset(&config, 0, sizeof(config));
 
@@ -300,12 +302,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 
 	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
 				       dma->direction, DMA_PREP_INTERRUPT);
+	if (!desc) {
+		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
+			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
+		return -ENOMEM;
+	}
 
 	desc->callback = s3c64xx_spi_dmacb;
 	desc->callback_param = dma;
 
-	dmaengine_submit(desc);
+	dma->cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(dma->cookie);
+	if (ret) {
+		dev_err(&sdd->pdev->dev, "DMA submission failed");
+		return -EIO;
+	}
+
 	dma_async_issue_pending(dma->ch);
+	return 0;
 }
 
 static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
@@ -355,11 +369,12 @@ static bool s3c64xx_spi_can_dma(struct spi_master *master,
 	return xfer->len > (FIFO_LVL_MASK(sdd) >> 1) + 1;
 }
 
-static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
+static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 				    struct spi_transfer *xfer, int dma_mode)
 {
 	void __iomem *regs = sdd->regs;
 	u32 modecfg, chcfg;
+	int ret = 0;
 
 	modecfg = readl(regs + S3C64XX_SPI_MODE_CFG);
 	modecfg &= ~(S3C64XX_SPI_MODE_TXDMA_ON | S3C64XX_SPI_MODE_RXDMA_ON);
@@ -385,7 +400,7 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 		chcfg |= S3C64XX_SPI_CH_TXCH_ON;
 		if (dma_mode) {
 			modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
-			prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
+			ret = prepare_dma(&sdd->tx_dma, &xfer->tx_sg);
 		} else {
 			switch (sdd->cur_bpw) {
 			case 32:
@@ -417,12 +432,17 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
 					| S3C64XX_SPI_PACKET_CNT_EN,
 					regs + S3C64XX_SPI_PACKET_CNT);
-			prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
+			ret = prepare_dma(&sdd->rx_dma, &xfer->rx_sg);
 		}
 	}
 
+	if (ret)
+		return ret;
+
 	writel(modecfg, regs + S3C64XX_SPI_MODE_CFG);
 	writel(chcfg, regs + S3C64XX_SPI_CH_CFG);
+
+	return 0;
 }
 
 static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
@@ -555,9 +575,10 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
 	return 0;
 }
 
-static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
+static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 {
 	void __iomem *regs = sdd->regs;
+	int ret;
 	u32 val;
 
 	/* Disable Clock */
@@ -605,7 +626,9 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 
 	if (sdd->port_conf->clk_from_cmu) {
 		/* The src_clk clock is divided internally by 2 */
-		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
+		ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
+		if (ret)
+			return ret;
 	} else {
 		/* Configure Clock */
 		val = readl(regs + S3C64XX_SPI_CLK_CFG);
@@ -619,6 +642,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 		val |= S3C64XX_SPI_ENCLK_ENABLE;
 		writel(val, regs + S3C64XX_SPI_CLK_CFG);
 	}
+
+	return 0;
 }
 
 #define XFER_DMAADDR_INVALID DMA_BIT_MASK(32)
@@ -661,7 +686,9 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		sdd->cur_bpw = bpw;
 		sdd->cur_speed = speed;
 		sdd->cur_mode = spi->mode;
-		s3c64xx_spi_config(sdd);
+		status = s3c64xx_spi_config(sdd);
+		if (status)
+			return status;
 	}
 
 	if (!is_polling(sdd) && (xfer->len > fifo_len) &&
@@ -688,10 +715,15 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		/* Start the signals */
 		s3c64xx_spi_set_cs(spi, true);
 
-		s3c64xx_enable_datapath(sdd, xfer, use_dma);
+		status = s3c64xx_enable_datapath(sdd, xfer, use_dma);
 
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
+		if (status) {
+			dev_err(&spi->dev, "failed to enable data path for transfer: %d\n", status);
+			break;
+		}
+
 		if (use_dma)
 			status = s3c64xx_wait_for_dma(sdd, xfer);
 		else
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 4/9] spi: spi-s3c64xx: Report more information when errors occur
       [not found]       ` <CGME20201001152247eucas1p2afff5b5b73f78d7c5111ac1c800e718a@eucas1p2.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Report amount of pending data when a transfer stops due to errors.

Report if DMA was used to transfer data and print the status code.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index f7482f2f1e13..5be6f2484e86 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -731,17 +731,28 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 
 		if (status) {
 			dev_err(&spi->dev,
-				"I/O Error: rx-%d tx-%d res:rx-%c tx-%c len-%d\n",
+				"I/O Error: rx-%d tx-%d rx-%c tx-%c len-%d dma-%d res-(%d)\n",
 				xfer->rx_buf ? 1 : 0, xfer->tx_buf ? 1 : 0,
 				(sdd->state & RXBUSY) ? 'f' : 'p',
 				(sdd->state & TXBUSY) ? 'f' : 'p',
-				xfer->len);
+				xfer->len, use_dma ? 1 : 0, status);
 
 			if (use_dma) {
-				if (xfer->tx_buf && (sdd->state & TXBUSY))
+				struct dma_tx_state s;
+
+				if (xfer->tx_buf && (sdd->state & TXBUSY)) {
+					dmaengine_pause(sdd->tx_dma.ch);
+					dmaengine_tx_status(sdd->tx_dma.ch, sdd->tx_dma.cookie, &s);
 					dmaengine_terminate_all(sdd->tx_dma.ch);
-				if (xfer->rx_buf && (sdd->state & RXBUSY))
+					dev_err(&spi->dev, "TX residue: %d\n", s.residue);
+
+				}
+				if (xfer->rx_buf && (sdd->state & RXBUSY)) {
+					dmaengine_pause(sdd->rx_dma.ch);
+					dmaengine_tx_status(sdd->rx_dma.ch, sdd->rx_dma.cookie, &s);
 					dmaengine_terminate_all(sdd->rx_dma.ch);
+					dev_err(&spi->dev, "RX residue: %d\n", s.residue);
+				}
 			}
 		} else {
 			s3c64xx_flush_fifo(sdd);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 5/9] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*
       [not found]       ` <CGME20201001152248eucas1p10219600aaa0df6e030d2493b2aefbe92@eucas1p1.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* to match documentation.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/spi/spi-s3c64xx.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 5be6f2484e86..adc5593ca2ca 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -29,7 +29,7 @@
 #define S3C64XX_SPI_CH_CFG		0x00
 #define S3C64XX_SPI_CLK_CFG		0x04
 #define S3C64XX_SPI_MODE_CFG		0x08
-#define S3C64XX_SPI_SLAVE_SEL		0x0C
+#define S3C64XX_SPI_CS_REG		0x0C
 #define S3C64XX_SPI_INT_EN		0x10
 #define S3C64XX_SPI_STATUS		0x14
 #define S3C64XX_SPI_TX_DATA		0x18
@@ -64,9 +64,9 @@
 #define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
 #define S3C64XX_SPI_MODE_4BURST			(1<<0)
 
-#define S3C64XX_SPI_SLAVE_AUTO			(1<<1)
-#define S3C64XX_SPI_SLAVE_SIG_INACT		(1<<0)
-#define S3C64XX_SPI_SLAVE_NSC_CNT_2		(2<<4)
+#define S3C64XX_SPI_CS_NSC_CNT_2		(2<<4)
+#define S3C64XX_SPI_CS_AUTO			(1<<1)
+#define S3C64XX_SPI_CS_SIG_INACT		(1<<0)
 
 #define S3C64XX_SPI_INT_TRAILING_EN		(1<<6)
 #define S3C64XX_SPI_INT_RX_OVERRUN_EN		(1<<5)
@@ -332,18 +332,18 @@ static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (enable) {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
-			writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			writel(0, sdd->regs + S3C64XX_SPI_CS_REG);
 		} else {
-			u32 ssel = readl(sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			u32 ssel = readl(sdd->regs + S3C64XX_SPI_CS_REG);
 
-			ssel |= (S3C64XX_SPI_SLAVE_AUTO |
-						S3C64XX_SPI_SLAVE_NSC_CNT_2);
-			writel(ssel, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			ssel |= (S3C64XX_SPI_CS_AUTO |
+						S3C64XX_SPI_CS_NSC_CNT_2);
+			writel(ssel, sdd->regs + S3C64XX_SPI_CS_REG);
 		}
 	} else {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-			writel(S3C64XX_SPI_SLAVE_SIG_INACT,
-			       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			writel(S3C64XX_SPI_CS_SIG_INACT,
+			       sdd->regs + S3C64XX_SPI_CS_REG);
 	}
 }
 
@@ -982,9 +982,9 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 	sdd->cur_speed = 0;
 
 	if (sci->no_cs)
-		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		writel(0, sdd->regs + S3C64XX_SPI_CS_REG);
 	else if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		writel(S3C64XX_SPI_CS_SIG_INACT, sdd->regs + S3C64XX_SPI_CS_REG);
 
 	/* Disable Interrupts - we use Polling if not DMA mode */
 	writel(0, regs + S3C64XX_SPI_INT_EN);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 6/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data
       [not found]       ` <CGME20201001152248eucas1p12f71c21a5873b6360e4b38efebb50271@eucas1p1.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Remove descriptions for non-existent fields and fix indentation.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index adc5593ca2ca..02de734b8ab1 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -162,11 +162,8 @@ struct s3c64xx_spi_port_config {
  * @cntrlr_info: Platform specific data for the controller this driver manages.
  * @lock: Controller specific lock.
  * @state: Set of FLAGS to indicate status.
- * @rx_dmach: Controller's DMA channel for Rx.
- * @tx_dmach: Controller's DMA channel for Tx.
  * @sfr_start: BUS address of SPI controller regs.
  * @regs: Pointer to ioremap'ed controller registers.
- * @irq: interrupt
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.
@@ -183,7 +180,7 @@ struct s3c64xx_spi_driver_data {
 	struct clk                      *ioclk;
 	struct platform_device          *pdev;
 	struct spi_master               *master;
-	struct s3c64xx_spi_info  *cntrlr_info;
+	struct s3c64xx_spi_info         *cntrlr_info;
 	spinlock_t                      lock;
 	unsigned long                   sfr_start;
 	struct completion               xfer_completion;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
       [not found]       ` <CGME20201001152248eucas1p132a63f588f62d902879322ebadd67173@eucas1p1.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  2020-10-01 19:12           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Make sure the cur_speed value used in s3c64xx_enable_datapath()
to configure DMA channel and in s3c64xx_wait_for_*() to calculate the
transfer timeout is set to the actual value of (half) the clock speed.

Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 02de734b8ab1..89c162efe355 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -626,6 +626,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 		ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 		if (ret)
 			return ret;
+		sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2;
 	} else {
 		/* Configure Clock */
 		val = readl(regs + S3C64XX_SPI_CLK_CFG);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 8/9] spi: spi-s3c64xx: Increase transfer timeout
       [not found]       ` <CGME20201001152249eucas1p1c3bbe7b2a677affe4e1a1e4d469f94c8@eucas1p1.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

Increase timeout by 30 ms for some wiggle room and set the minimum value
to 100 ms. This ensures a non-zero value for short transfers which
may take less than 1 ms. The timeout value does not affect
performance because it is used with a completion.

Similar formula is used in other drivers e.g. sun4i, sun6i.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 89c162efe355..ea5a22dec53d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -473,7 +473,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 
 	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
 	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
-	ms += 10; /* some tolerance */
+	ms += 30;               /* some tolerance */
+	ms = max(ms, 100);      /* minimum timeout */
 
 	val = msecs_to_jiffies(ms) + 10;
 	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH v2 RESEND 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume
       [not found]       ` <CGME20201001152249eucas1p165b78adf542a48b38b49cb5e00500e26@eucas1p1.samsung.com>
@ 2020-10-01 15:21         ` Łukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Łukasz Stelmach @ 2020-10-01 15:21 UTC (permalink / raw)
  To: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	Mark Brown, linux-spi, linux-samsung-soc, linux-arm-kernel,
	linux-kernel
  Cc: m.szyprowski, b.zolnierkie, Łukasz Stelmach

s3c64xx_spi_hwinit() disables interrupts. In s3c64xx_spi_probe() after
calling s3c64xx_spi_hwinit() they are enabled with the following call.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ea5a22dec53d..53e3581bcd7b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1378,6 +1378,10 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
 
 	s3c64xx_spi_hwinit(sdd);
 
+	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
+	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
+	       sdd->regs + S3C64XX_SPI_INT_EN);
+
 	return 0;
 
 err_disable_src_clk:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx
  2020-10-01 15:21     ` [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx Łukasz Stelmach
                         ` (8 preceding siblings ...)
       [not found]       ` <CGME20201001152249eucas1p165b78adf542a48b38b49cb5e00500e26@eucas1p1.samsung.com>
@ 2020-10-01 16:13       ` Mark Brown
       [not found]         ` <CGME20201001182315eucas1p1b1fc9d5d0ea91db6e56e92d5cf2583e5@eucas1p1.samsung.com>
  9 siblings, 1 reply; 50+ messages in thread
From: Mark Brown @ 2020-10-01 16:13 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Krzysztof Kozlowski, Tomasz Figa, Andi Shyti,
	linux-spi, linux-samsung-soc, linux-arm-kernel, linux-kernel,
	m.szyprowski, b.zolnierkie

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On Thu, Oct 01, 2020 at 05:21:39PM +0200, Łukasz Stelmach wrote:
> This is a series of fixes created during porting a device driver (these
> patches will be released soon too) for an SPI device to the current kernel.

There appeared to be a number of outstanding review comments (misleading
commit message on patch 7, some concerns about the non-CMU case), please
address those.

Please don't send new patches in reply to old ones, it buries them in
threads and can make it hard to follow what's going on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx
       [not found]         ` <CGME20201001182315eucas1p1b1fc9d5d0ea91db6e56e92d5cf2583e5@eucas1p1.samsung.com>
@ 2020-10-01 18:23           ` Lukasz Stelmach
  2020-10-01 19:02             ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Lukasz Stelmach @ 2020-10-01 18:23 UTC (permalink / raw)
  To: Mark Brown, Tomasz Figa, Krzysztof Kozlowski
  Cc: Kukjin Kim, Andi Shyti, linux-spi, linux-samsung-soc,
	linux-arm-kernel, linux-kernel, m.szyprowski, b.zolnierkie

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

It was <2020-10-01 czw 17:13>, when Mark Brown wrote:
> On Thu, Oct 01, 2020 at 05:21:39PM +0200, Łukasz Stelmach wrote:
>> This is a series of fixes created during porting a device driver (these
>> patches will be released soon too) for an SPI device to the current kernel.
>
> There appeared to be a number of outstanding review comments (misleading
> commit message on patch 7, some concerns about the non-CMU case), please
> address those.

We discussed with Tomasz Figa and Krzysztof Kozłowski off the list that
this is practically unused. Tomasz, Krzysztof, would you be so kind to
share the details?

Kind regards,
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx
  2020-10-01 18:23           ` Lukasz Stelmach
@ 2020-10-01 19:02             ` Krzysztof Kozlowski
  2020-10-01 19:43               ` Mark Brown
  0 siblings, 1 reply; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-01 19:02 UTC (permalink / raw)
  To: Lukasz Stelmach
  Cc: Mark Brown, Tomasz Figa, Kukjin Kim, Andi Shyti, linux-spi,
	linux-samsung-soc, linux-arm-kernel, linux-kernel, m.szyprowski,
	b.zolnierkie

On Thu, Oct 01, 2020 at 08:23:00PM +0200, Lukasz Stelmach wrote:
> It was <2020-10-01 czw 17:13>, when Mark Brown wrote:
> > On Thu, Oct 01, 2020 at 05:21:39PM +0200, Łukasz Stelmach wrote:
> >> This is a series of fixes created during porting a device driver (these
> >> patches will be released soon too) for an SPI device to the current kernel.
> >
> > There appeared to be a number of outstanding review comments (misleading
> > commit message on patch 7, some concerns about the non-CMU case), please
> > address those.
> 
> We discussed with Tomasz Figa and Krzysztof Kozłowski off the list that
> this is practically unused. Tomasz, Krzysztof, would you be so kind to
> share the details?

That is correct. We did not provide final comments on the list so they
could be added here - in change log. This would also be an explanation
why there is a resend. Another solution would be to extend the commit #7
description - why only CMU case is covered.

About patch #7: The decision was not to correct non-CMU case because
there were not actual reports about clock rounding poblems, it would not
be trivial change and we do not have the HW to test.

Thanks Mark for looking into it.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
  2020-10-01 15:21         ` [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250 Łukasz Stelmach
@ 2020-10-01 19:04           ` Krzysztof Kozlowski
       [not found]             ` <CGME20201002101408eucas1p121c21cde5e644992078978d9bf1c5194@eucas1p1.samsung.com>
  0 siblings, 1 reply; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-01 19:04 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Tomasz Figa, Andi Shyti, Mark Brown, linux-spi,
	linux-samsung-soc, linux-arm-kernel, linux-kernel, m.szyprowski,
	b.zolnierkie

On Thu, Oct 01, 2020 at 05:21:41PM +0200, Łukasz Stelmach wrote:
> Fix issues with DMA transfers bigger than 512 bytes on Exynos3250. Without
> the patches such transfers fail.
> 
> The vendor kernel for ARTIK5 handles CS in a simmilar way.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 26c7cb79cd78..22bf8c75580a 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1379,6 +1379,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
>  	.tx_st_done	= 25,
>  	.high_speed	= true,
>  	.clk_from_cmu	= true,
> +	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,

I spotted now: you have here double space after '='.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value
  2020-10-01 15:21         ` [PATCH v2 RESEND 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
@ 2020-10-01 19:12           ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-01 19:12 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: Kukjin Kim, Tomasz Figa, Andi Shyti, Mark Brown, linux-spi,
	linux-samsung-soc, linux-arm-kernel, linux-kernel, m.szyprowski,
	b.zolnierkie

On Thu, Oct 01, 2020 at 05:21:46PM +0200, Łukasz Stelmach wrote:
> Make sure the cur_speed value used in s3c64xx_enable_datapath()
> to configure DMA channel and in s3c64xx_wait_for_*() to calculate the
> transfer timeout is set to the actual value of (half) the clock speed.
> 
> Suggested-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx
  2020-10-01 19:02             ` Krzysztof Kozlowski
@ 2020-10-01 19:43               ` Mark Brown
  2020-10-02  6:18                 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 50+ messages in thread
From: Mark Brown @ 2020-10-01 19:43 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Lukasz Stelmach, Tomasz Figa, Kukjin Kim, Andi Shyti, linux-spi,
	linux-samsung-soc, linux-arm-kernel, linux-kernel, m.szyprowski,
	b.zolnierkie

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

On Thu, Oct 01, 2020 at 09:02:57PM +0200, Krzysztof Kozlowski wrote:

> That is correct. We did not provide final comments on the list so they
> could be added here - in change log. This would also be an explanation
> why there is a resend. Another solution would be to extend the commit #7
> description - why only CMU case is covered.

If there's a new changelog then it's not a resend, the changelog is part
of the content so I'd expect a version bump for that alone.  IIRC the
changelog needed some clarification anyway?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx
  2020-10-01 19:43               ` Mark Brown
@ 2020-10-02  6:18                 ` Krzysztof Kozlowski
  0 siblings, 0 replies; 50+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-02  6:18 UTC (permalink / raw)
  To: Mark Brown
  Cc: Lukasz Stelmach, Tomasz Figa, Kukjin Kim, Andi Shyti, linux-spi,
	linux-samsung-soc, linux-arm-kernel, linux-kernel,
	Marek Szyprowski, Bartłomiej Żołnierkiewicz

On Thu, 1 Oct 2020 at 21:44, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Oct 01, 2020 at 09:02:57PM +0200, Krzysztof Kozlowski wrote:
>
> > That is correct. We did not provide final comments on the list so they
> > could be added here - in change log. This would also be an explanation
> > why there is a resend. Another solution would be to extend the commit #7
> > description - why only CMU case is covered.
>
> If there's a new changelog then it's not a resend, the changelog is part
> of the content so I'd expect a version bump for that alone.  IIRC the
> changelog needed some clarification anyway?

Yes, documenting the non-CMU case in changeloge would be good. It
should be also v3 because of another reason: two patches got reordered
to a more meaningful position in patchset, which brought minor
differences in them.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
       [not found]             ` <CGME20201002101408eucas1p121c21cde5e644992078978d9bf1c5194@eucas1p1.samsung.com>
@ 2020-10-02 10:13               ` Lukasz Stelmach
  0 siblings, 0 replies; 50+ messages in thread
From: Lukasz Stelmach @ 2020-10-02 10:13 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Kukjin Kim, Tomasz Figa, Andi Shyti, Mark Brown, linux-spi,
	linux-samsung-soc, linux-arm-kernel, linux-kernel, m.szyprowski,
	b.zolnierkie

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

It was <2020-10-01 czw 21:04>, when Krzysztof Kozlowski wrote:
> On Thu, Oct 01, 2020 at 05:21:41PM +0200, Łukasz Stelmach wrote:
>> Fix issues with DMA transfers bigger than 512 bytes on Exynos3250. Without
>> the patches such transfers fail.
>> 
>> The vendor kernel for ARTIK5 handles CS in a simmilar way.
>> 
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 26c7cb79cd78..22bf8c75580a 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -1379,6 +1379,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
>>  	.tx_st_done	= 25,
>>  	.high_speed	= true,
>>  	.clk_from_cmu	= true,
>> +	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
>
> I spotted now: you have here double space after '='.
>

Fixed, thanks. v3 is coming.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2020-10-02 10:14 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200821161404eucas1p20577160d1bff2e8f5cae7403e93716ab@eucas1p2.samsung.com>
2020-08-21 16:13 ` [PATCH v2 0/9] Some fixes for spi-s3c64xx Łukasz Stelmach
     [not found]   ` <CGME20200821161405eucas1p1d43a5970c6a26389cd506aab5f986bc8@eucas1p1.samsung.com>
2020-08-21 16:13     ` [PATCH v2 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath() Łukasz Stelmach
2020-08-22 12:01       ` Krzysztof Kozlowski
     [not found]   ` <CGME20200821161405eucas1p20aad659cd41bc4f56d5123d3c63394f0@eucas1p2.samsung.com>
2020-08-21 16:13     ` [PATCH v2 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250 Łukasz Stelmach
2020-08-22 12:02       ` Krzysztof Kozlowski
     [not found]   ` <CGME20200821161405eucas1p19280babcd73926b5c22a48830f5fecd7@eucas1p1.samsung.com>
2020-08-21 16:13     ` [PATCH v2 3/9] spi: spi-s3c64xx: Report more information when errors occur Łukasz Stelmach
2020-08-22 12:03       ` Krzysztof Kozlowski
     [not found]   ` <CGME20200821161406eucas1p2be3221183a855afd0213f8ce58bd8942@eucas1p2.samsung.com>
2020-08-21 16:13     ` [PATCH v2 4/9] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* Łukasz Stelmach
     [not found]   ` <CGME20200821161406eucas1p121553719d4e9cc020d2c557a69000f0c@eucas1p1.samsung.com>
2020-08-21 16:13     ` [PATCH v2 5/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data Łukasz Stelmach
2020-08-22 12:26       ` Krzysztof Kozlowski
     [not found]   ` <CGME20200821161407eucas1p116af63a668bdbb75fa974589e5f6139f@eucas1p1.samsung.com>
2020-08-21 16:13     ` [PATCH v2 6/9] spi: spi-s3c64xx: Check return values Łukasz Stelmach
2020-08-22 12:37       ` Krzysztof Kozlowski
2020-08-25 19:06       ` Sylwester Nawrocki
     [not found]         ` <CGME20200901152113eucas1p2977046b7a5b4c5a519f88870d658698a@eucas1p2.samsung.com>
2020-09-01 15:21           ` Lukasz Stelmach
2020-09-02  8:14             ` Sylwester Nawrocki
     [not found]               ` <CGME20200903084555eucas1p2f40375edb325107b68966fd52198b220@eucas1p2.samsung.com>
2020-09-03  8:45                 ` Lukasz Stelmach
2020-09-03 11:18                   ` Sylwester Nawrocki
     [not found]   ` <CGME20200821161407eucas1p249e4833b8839f717cc2a496ab43bb8a2@eucas1p2.samsung.com>
2020-08-21 16:13     ` [PATCH v2 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
2020-08-22 12:43       ` Krzysztof Kozlowski
2020-08-22 14:52         ` Tomasz Figa
2020-08-22 15:18           ` Krzysztof Kozlowski
2020-08-22 15:20             ` Tomasz Figa
     [not found]         ` <CGME20200824131716eucas1p16a3fde52aa765e7cd6584d4733762047@eucas1p1.samsung.com>
2020-08-24 13:17           ` Lukasz Stelmach
2020-08-24 13:21             ` Tomasz Figa
     [not found]               ` <CGME20200825090211eucas1p1b63191fa778a775e33169ba2c1d3b74b@eucas1p1.samsung.com>
2020-08-25  9:01                 ` Lukasz Stelmach
2020-08-25 15:11                   ` Tomasz Figa
     [not found]                     ` <CGME20200825154611eucas1p284be8779ab484e675af071afef28376b@eucas1p2.samsung.com>
2020-08-25 15:45                       ` Lukasz Stelmach
2020-08-22 14:54       ` Tomasz Figa
     [not found]   ` <CGME20200821161407eucas1p23a283ac117d4381e087e9bacec537665@eucas1p2.samsung.com>
2020-08-21 16:14     ` [PATCH v2 8/9] spi: spi-s3c64xx: Increase transfer timeout Łukasz Stelmach
2020-08-22 12:43       ` Krzysztof Kozlowski
     [not found]   ` <CGME20200821161408eucas1p196aa4b954b3d19ad1b89a48dbbe41fbc@eucas1p1.samsung.com>
2020-08-21 16:14     ` [PATCH v2 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume Łukasz Stelmach
2020-08-22 12:44       ` Krzysztof Kozlowski
     [not found]   ` <CGME20201001152246eucas1p1ee289b8a85b707f7496355c081623796@eucas1p1.samsung.com>
2020-10-01 15:21     ` [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx Łukasz Stelmach
     [not found]       ` <CGME20201001152246eucas1p1b4155ab4f06a39cc88f205b4a7cd47f9@eucas1p1.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 1/9] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath() Łukasz Stelmach
     [not found]       ` <CGME20201001152246eucas1p2fb22ab55c276d76c4508142842c90ab8@eucas1p2.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250 Łukasz Stelmach
2020-10-01 19:04           ` Krzysztof Kozlowski
     [not found]             ` <CGME20201002101408eucas1p121c21cde5e644992078978d9bf1c5194@eucas1p1.samsung.com>
2020-10-02 10:13               ` Lukasz Stelmach
     [not found]       ` <CGME20201001152247eucas1p2b6b1cc61b9b175b0a621609cd58effbd@eucas1p2.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 3/9] spi: spi-s3c64xx: Check return values Łukasz Stelmach
     [not found]       ` <CGME20201001152247eucas1p2afff5b5b73f78d7c5111ac1c800e718a@eucas1p2.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 4/9] spi: spi-s3c64xx: Report more information when errors occur Łukasz Stelmach
     [not found]       ` <CGME20201001152248eucas1p10219600aaa0df6e030d2493b2aefbe92@eucas1p1.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 5/9] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* Łukasz Stelmach
     [not found]       ` <CGME20201001152248eucas1p12f71c21a5873b6360e4b38efebb50271@eucas1p1.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 6/9] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data Łukasz Stelmach
     [not found]       ` <CGME20201001152248eucas1p132a63f588f62d902879322ebadd67173@eucas1p1.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 7/9] spi: spi-s3c64xx: Ensure cur_speed holds actual clock value Łukasz Stelmach
2020-10-01 19:12           ` Krzysztof Kozlowski
     [not found]       ` <CGME20201001152249eucas1p1c3bbe7b2a677affe4e1a1e4d469f94c8@eucas1p1.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 8/9] spi: spi-s3c64xx: Increase transfer timeout Łukasz Stelmach
     [not found]       ` <CGME20201001152249eucas1p165b78adf542a48b38b49cb5e00500e26@eucas1p1.samsung.com>
2020-10-01 15:21         ` [PATCH v2 RESEND 9/9] spi: spi-s3c64xx: Turn on interrupts upon resume Łukasz Stelmach
2020-10-01 16:13       ` [PATCH v2 RESEND 0/9] Some fixes for spi-s3c64xx Mark Brown
     [not found]         ` <CGME20201001182315eucas1p1b1fc9d5d0ea91db6e56e92d5cf2583e5@eucas1p1.samsung.com>
2020-10-01 18:23           ` Lukasz Stelmach
2020-10-01 19:02             ` Krzysztof Kozlowski
2020-10-01 19:43               ` Mark Brown
2020-10-02  6:18                 ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).