1. Patchew
  2. linux
  3. View series

[PATCH] spi: spi_amd: Add HIDDMA basic write support

Raju Rangoju posted 1 patch 7 months, 1 week ago 
drivers/spi/spi-amd.c | 130 ++++++++++++++++++++++++++++++++++--------
1 file changed, 107 insertions(+), 23 deletions(-)
[PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by Raju Rangoju 7 months, 1 week ago 
SPI index mode has hardware limitation of transferring only 64 bytes per
transaction due to fixed number of FIFO registers. This constraint leads to
performance issues when reading/writing data to/from NAND/NOR flash
devices, as the controller must issue multiple requests to read/write
64-byte chunks, even if the slave can transfer up to 2 or 4 KB in a single
transaction.

The AMD HID2 SPI controller supports DMA mode, allowing for reading/writing
up to 4 KB of data in a single transaction. The existing spi_amd driver
already supports HID2 DMA read operations.

This patch introduces changes to implement HID2 DMA single mode basic write
support for the HID2 SPI controller.

Co-developed-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
Signed-off-by: Krishnamoorthi M <krishnamoorthi.m@amd.com>
Co-developed-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
 drivers/spi/spi-amd.c | 130 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 107 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 17fc0b17e756..8567465d2282 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -52,10 +52,13 @@
 #define AMD_SPI_SPD7_MASK	GENMASK(13, AMD_SPI_SPD7_SHIFT)
 
 #define AMD_SPI_HID2_INPUT_RING_BUF0	0X100
+#define AMD_SPI_HID2_OUTPUT_BUF0	0x140
 #define AMD_SPI_HID2_CNTRL		0x150
 #define AMD_SPI_HID2_INT_STATUS		0x154
 #define AMD_SPI_HID2_CMD_START		0x156
 #define AMD_SPI_HID2_INT_MASK		0x158
+#define AMD_SPI_HID2_WRITE_CNTRL0	0x160
+#define AMD_SPI_HID2_WRITE_CNTRL1	0x164
 #define AMD_SPI_HID2_READ_CNTRL0	0x170
 #define AMD_SPI_HID2_READ_CNTRL1	0x174
 #define AMD_SPI_HID2_READ_CNTRL2	0x180
@@ -93,6 +96,10 @@ enum amd_spi_versions {
 	AMD_HID2_SPI,
 };
 
+/* SPINAND write command opcodes */
+#define AMD_SPI_OP_PP			0x02	/* Page program */
+#define AMD_SPI_OP_PP_RANDOM		0x84	/* Page program */
+
 enum amd_spi_speed {
 	F_66_66MHz,
 	F_33_33MHz,
@@ -445,6 +452,17 @@ static inline bool amd_is_spi_read_cmd(const u16 op)
 	}
 }
 
+static inline bool amd_is_spi_write_cmd(const u16 op)
+{
+	switch (op) {
+	case AMD_SPI_OP_PP:
+	case AMD_SPI_OP_PP_RANDOM:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static bool amd_spi_supports_op(struct spi_mem *mem,
 				const struct spi_mem_op *op)
 {
@@ -455,7 +473,7 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
 		return false;
 
 	/* AMD SPI controllers support quad mode only for read operations */
-	if (amd_is_spi_read_cmd(op->cmd.opcode)) {
+	if (amd_is_spi_read_cmd(op->cmd.opcode) || amd_is_spi_write_cmd(op->cmd.opcode)) {
 		if (op->data.buswidth > 4)
 			return false;
 
@@ -464,7 +482,8 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
 		 * doesn't support 4-byte address commands.
 		 */
 		if (amd_spi->version == AMD_HID2_SPI) {
-			if (amd_is_spi_read_cmd_4b(op->cmd.opcode) ||
+			if ((amd_is_spi_read_cmd_4b(op->cmd.opcode) ||
+			     amd_is_spi_write_cmd(op->cmd.opcode)) &&
 			    op->data.nbytes > AMD_SPI_HID2_DMA_SIZE)
 				return false;
 		} else if (op->data.nbytes > AMD_SPI_MAX_DATA) {
@@ -490,7 +509,8 @@ static int amd_spi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 	 * controller index mode supports maximum of 64 bytes in a single
 	 * transaction.
 	 */
-	if (amd_spi->version == AMD_HID2_SPI && amd_is_spi_read_cmd(op->cmd.opcode))
+	if (amd_spi->version == AMD_HID2_SPI && (amd_is_spi_read_cmd(op->cmd.opcode) ||
+						 amd_is_spi_write_cmd(op->cmd.opcode)))
 		op->data.nbytes = clamp_val(op->data.nbytes, 0, AMD_SPI_HID2_DMA_SIZE);
 	else
 		op->data.nbytes = clamp_val(op->data.nbytes, 0, AMD_SPI_MAX_DATA);
@@ -514,32 +534,91 @@ static void amd_spi_set_addr(struct amd_spi *amd_spi,
 	}
 }
 
+static void amd_spi_hiddma_write(struct amd_spi *amd_spi, const struct spi_mem_op *op)
+{
+	u16 hid_cmd_start, val;
+	u32 hid_regval;
+
+	/*
+	 * Program the HID2 output Buffer0. 4k aligned buf_memory_addr[31:12],
+	 * buf_size[2:0].
+	 */
+	hid_regval = amd_spi->phy_dma_buf | BIT(0);
+	amd_spi_writereg32(amd_spi, AMD_SPI_HID2_OUTPUT_BUF0, hid_regval);
+
+	/* Program max write length in hid2_write_control1 register */
+	hid_regval = amd_spi_readreg32(amd_spi, AMD_SPI_HID2_WRITE_CNTRL1);
+	hid_regval = (hid_regval & ~GENMASK(15, 0)) | ((op->data.nbytes) + 3);
+	amd_spi_writereg32(amd_spi, AMD_SPI_HID2_WRITE_CNTRL1, hid_regval);
+
+	/* Set cmd start bit in hid2_cmd_start register to trigger HID basic write operation */
+	hid_cmd_start = amd_spi_readreg16(amd_spi, AMD_SPI_HID2_CMD_START);
+	amd_spi_writereg16(amd_spi, AMD_SPI_HID2_CMD_START, (hid_cmd_start | BIT(2)));
+
+	/* Check interrupt status of HIDDMA basic write operation in hid2_int_status register */
+	readw_poll_timeout(amd_spi->io_remap_addr + AMD_SPI_HID2_INT_STATUS, val,
+			   (val & BIT(2)), AMD_SPI_IO_SLEEP_US, AMD_SPI_IO_TIMEOUT_US);
+
+	/* Clear the interrupts by writing to hid2_int_status register */
+	val = amd_spi_readreg16(amd_spi, AMD_SPI_HID2_INT_STATUS);
+	amd_spi_writereg16(amd_spi, AMD_SPI_HID2_INT_STATUS, val);
+}
+
 static void amd_spi_mem_data_out(struct amd_spi *amd_spi,
 				 const struct spi_mem_op *op)
 {
 	int base_addr = AMD_SPI_FIFO_BASE + op->addr.nbytes;
 	u64 *buf_64 = (u64 *)op->data.buf.out;
+	u64 addr_val = op->addr.val;
 	u32 nbytes = op->data.nbytes;
 	u32 left_data = nbytes;
 	u8 *buf;
 	int i;
 
-	amd_spi_set_opcode(amd_spi, op->cmd.opcode);
-	amd_spi_set_addr(amd_spi, op);
+	/*
+	 * Condition for using HID write mode. Only for writing complete page data, use HID write.
+	 * Use index mode otherwise.
+	 */
+	if (amd_spi->version == AMD_HID2_SPI && amd_is_spi_write_cmd(op->cmd.opcode)) {
+		void *hid_base_addr = amd_spi->dma_virt_addr + op->addr.nbytes + op->cmd.nbytes;
 
-	for (i = 0; left_data >= 8; i++, left_data -= 8)
-		amd_spi_writereg64(amd_spi, base_addr + op->dummy.nbytes + (i * 8), *buf_64++);
+		/* Write opcode and address in system memory */
+		writeb(op->cmd.opcode, amd_spi->dma_virt_addr);
 
-	buf = (u8 *)buf_64;
-	for (i = 0; i < left_data; i++) {
-		amd_spi_writereg8(amd_spi, base_addr + op->dummy.nbytes + nbytes + i - left_data,
-				  buf[i]);
-	}
+		for (i = 0; i < op->addr.nbytes; i++) {
+			writeb(addr_val & GENMASK(7, 0), hid_base_addr - i - op->cmd.nbytes);
+			addr_val >>= 8;
+		}
 
-	amd_spi_set_tx_count(amd_spi, op->addr.nbytes + op->data.nbytes);
-	amd_spi_set_rx_count(amd_spi, 0);
-	amd_spi_clear_fifo_ptr(amd_spi);
-	amd_spi_execute_opcode(amd_spi);
+		for (i = 0; left_data >= 8; i++, left_data -= 8)
+			writeq(*buf_64++, hid_base_addr + (i * 8));
+
+		buf = (u8 *)buf_64;
+
+		for (i = 0; i < left_data; i++)
+			writeb(buf[i], hid_base_addr + (nbytes - left_data + i));
+
+		amd_spi_hiddma_write(amd_spi, op);
+	} else {
+		amd_spi_set_opcode(amd_spi, op->cmd.opcode);
+		amd_spi_set_addr(amd_spi, op);
+
+		for (i = 0; left_data >= 8; i++, left_data -= 8)
+			amd_spi_writereg64(amd_spi, base_addr + op->dummy.nbytes + (i * 8),
+					   *buf_64++);
+
+		buf = (u8 *)buf_64;
+		for (i = 0; i < left_data; i++) {
+			amd_spi_writereg8(amd_spi,
+					  base_addr + op->dummy.nbytes + nbytes + i - left_data,
+					  buf[i]);
+		}
+
+		amd_spi_set_tx_count(amd_spi, op->addr.nbytes + op->data.nbytes);
+		amd_spi_set_rx_count(amd_spi, 0);
+		amd_spi_clear_fifo_ptr(amd_spi);
+		amd_spi_execute_opcode(amd_spi);
+	}
 }
 
 static void amd_spi_hiddma_read(struct amd_spi *amd_spi, const struct spi_mem_op *op)
@@ -728,23 +807,28 @@ static int amd_spi_setup_hiddma(struct amd_spi *amd_spi, struct device *dev)
 {
 	u32 hid_regval;
 
-	/* Allocate DMA buffer to use for HID basic read operation */
-	amd_spi->dma_virt_addr = dma_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE,
-						    &amd_spi->phy_dma_buf, GFP_KERNEL);
+	/* Allocate DMA buffer to use for HID basic read and write operations. For write
+	 * operations, the DMA buffer should include the opcode, address bytes and dummy
+	 * bytes(if any) in addition to the data bytes. Additionally, the hardware requires
+	 * that the buffer address be 4K aligned. So, allocate DMA buffer of size
+	 * 2 * AMD_SPI_HID2_DMA_SIZE.
+	 */
+	amd_spi->dma_virt_addr = dmam_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE * 2,
+						     &amd_spi->phy_dma_buf, GFP_KERNEL);
 	if (!amd_spi->dma_virt_addr)
 		return -ENOMEM;
 
 	/*
 	 * Enable interrupts and set mask bits in hid2_int_mask register to generate interrupt
-	 * properly for HIDDMA basic read operations.
+	 * properly for HIDDMA basic read and write operations.
 	 */
 	hid_regval = amd_spi_readreg32(amd_spi, AMD_SPI_HID2_INT_MASK);
-	hid_regval = (hid_regval & GENMASK(31, 8)) | BIT(19);
+	hid_regval = (hid_regval & GENMASK(31, 8)) | BIT(18) | BIT(19);
 	amd_spi_writereg32(amd_spi, AMD_SPI_HID2_INT_MASK, hid_regval);
 
-	/* Configure buffer unit(4k) in hid2_control register */
+	/* Configure buffer unit(4k) and write threshold in hid2_control register */
 	hid_regval = amd_spi_readreg32(amd_spi, AMD_SPI_HID2_CNTRL);
-	amd_spi_writereg32(amd_spi, AMD_SPI_HID2_CNTRL, hid_regval & ~BIT(3));
+	amd_spi_writereg32(amd_spi, AMD_SPI_HID2_CNTRL, (hid_regval | GENMASK(13, 12)) & ~BIT(3));
 
 	return 0;
 }
-- 
2.34.1
Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by kernel test robot 7 months, 1 week ago 
Hi Raju,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.15-rc5]
[also build test WARNING on linus/master]
[cannot apply to broonie-spi/for-next next-20250509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
base:   v6.15-rc5
patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505110641.zLT16Dv7-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/spi/spi-amd.c:594:57: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got void * @@
   drivers/spi/spi-amd.c:594:57: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/spi/spi-amd.c:594:57: sparse:     got void *

vim +594 drivers/spi/spi-amd.c

   566	
   567	static void amd_spi_mem_data_out(struct amd_spi *amd_spi,
   568					 const struct spi_mem_op *op)
   569	{
   570		int base_addr = AMD_SPI_FIFO_BASE + op->addr.nbytes;
   571		u64 *buf_64 = (u64 *)op->data.buf.out;
   572		u64 addr_val = op->addr.val;
   573		u32 nbytes = op->data.nbytes;
   574		u32 left_data = nbytes;
   575		u8 *buf;
   576		int i;
   577	
   578		/*
   579		 * Condition for using HID write mode. Only for writing complete page data, use HID write.
   580		 * Use index mode otherwise.
   581		 */
   582		if (amd_spi->version == AMD_HID2_SPI && amd_is_spi_write_cmd(op->cmd.opcode)) {
   583			void *hid_base_addr = amd_spi->dma_virt_addr + op->addr.nbytes + op->cmd.nbytes;
   584	
   585			/* Write opcode and address in system memory */
   586			writeb(op->cmd.opcode, amd_spi->dma_virt_addr);
   587	
   588			for (i = 0; i < op->addr.nbytes; i++) {
   589				writeb(addr_val & GENMASK(7, 0), hid_base_addr - i - op->cmd.nbytes);
   590				addr_val >>= 8;
   591			}
   592	
   593			for (i = 0; left_data >= 8; i++, left_data -= 8)
 > 594				writeq(*buf_64++, hid_base_addr + (i * 8));
   595	
   596			buf = (u8 *)buf_64;
   597	
   598			for (i = 0; i < left_data; i++)
   599				writeb(buf[i], hid_base_addr + (nbytes - left_data + i));
   600	
   601			amd_spi_hiddma_write(amd_spi, op);
   602		} else {
   603			amd_spi_set_opcode(amd_spi, op->cmd.opcode);
   604			amd_spi_set_addr(amd_spi, op);
   605	
   606			for (i = 0; left_data >= 8; i++, left_data -= 8)
   607				amd_spi_writereg64(amd_spi, base_addr + op->dummy.nbytes + (i * 8),
   608						   *buf_64++);
   609	
   610			buf = (u8 *)buf_64;
   611			for (i = 0; i < left_data; i++) {
   612				amd_spi_writereg8(amd_spi,
   613						  base_addr + op->dummy.nbytes + nbytes + i - left_data,
   614						  buf[i]);
   615			}
   616	
   617			amd_spi_set_tx_count(amd_spi, op->addr.nbytes + op->data.nbytes);
   618			amd_spi_set_rx_count(amd_spi, 0);
   619			amd_spi_clear_fifo_ptr(amd_spi);
   620			amd_spi_execute_opcode(amd_spi);
   621		}
   622	}
   623	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by Rangoju, Raju 7 months, 1 week ago 

On 5/11/2025 3:51 AM, kernel test robot wrote:
> Hi Raju,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on v6.15-rc5]
> [also build test WARNING on linus/master]
> [cannot apply to broonie-spi/for-next next-20250509]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
> base:   v6.15-rc5
> patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
> patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
> config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 14.2.0

Thanks for reporting this. We do not support m68k.
Will re-spin v2 with necessary changes in Kconfig.

> reproduce: (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202505110641.zLT16Dv7-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
>>> drivers/spi/spi-amd.c:594:57: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got void * @@
>     drivers/spi/spi-amd.c:594:57: sparse:     expected void volatile [noderef] __iomem *addr
>     drivers/spi/spi-amd.c:594:57: sparse:     got void *
Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by Geert Uytterhoeven 7 months, 1 week ago 
Hi Rangoju,

On Mon, 12 May 2025 at 09:29, Rangoju, Raju <raju.rangoju@amd.com> wrote:
> On 5/11/2025 3:51 AM, kernel test robot wrote:
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on v6.15-rc5]
> > [also build test WARNING on linus/master]
> > [cannot apply to broonie-spi/for-next next-20250509]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
> > base:   v6.15-rc5
> > patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
> > patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
> > config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
> > compiler: m68k-linux-gcc (GCC) 14.2.0
>
> Thanks for reporting this. We do not support m68k.

All write[bwlq]() functions take a volatile void __iomem pointer
(https://elixir.bootlin.com/linux/v6.14.6/source/include/asm-generic/io.h#L174)
while you are passing a void *, so sparse should complain about this
on all architectures.  And sparse is right, this driver is using MMIO
accessors on allocated DMA memory, which is just plain wrong:

    amd_spi->dma_virt_addr = dma_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE,
            &amd_spi->phy_dma_buf, GFP_KERNEL);

     for (i = 0; left_data >= 8; i++, left_data -= 8)
            *buf_64++ = readq((u8 __iomem *)amd_spi->dma_virt_addr + (i * 8));

> Will re-spin v2 with necessary changes in Kconfig.

Please fix the real issue instead ;-)

> > reproduce: (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202505110641.zLT16Dv7-lkp@intel.com/
> >
> > sparse warnings: (new ones prefixed by >>)
> >>> drivers/spi/spi-amd.c:594:57: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got void * @@
> >     drivers/spi/spi-amd.c:594:57: sparse:     expected void volatile [noderef] __iomem *addr
> >     drivers/spi/spi-amd.c:594:57: sparse:     got void *

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by Rangoju, Raju 7 months, 1 week ago 

On 5/12/2025 7:47 PM, Geert Uytterhoeven wrote:
> Hi Rangoju,
> 
> On Mon, 12 May 2025 at 09:29, Rangoju, Raju <raju.rangoju@amd.com> wrote:
>> On 5/11/2025 3:51 AM, kernel test robot wrote:
>>> kernel test robot noticed the following build warnings:
>>>
>>> [auto build test WARNING on v6.15-rc5]
>>> [also build test WARNING on linus/master]
>>> [cannot apply to broonie-spi/for-next next-20250509]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
>>> base:   v6.15-rc5
>>> patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
>>> patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
>>> config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
>>> compiler: m68k-linux-gcc (GCC) 14.2.0
>>
>> Thanks for reporting this. We do not support m68k.
> 
> All write[bwlq]() functions take a volatile void __iomem pointer
> (https://elixir.bootlin.com/linux/v6.14.6/source/include/asm-generic/io.h#L174)
> while you are passing a void *, so sparse should complain about this
> on all architectures.  

My bad, with the following flags included, sparse now complains this on 
all architectures.

-fmax-errors=unlimited -fmax-warnings=unlimited

And sparse is right, this driver is using MMIO
> accessors on allocated DMA memory, which is just plain wrong:
> 
>      amd_spi->dma_virt_addr = dma_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE,
>              &amd_spi->phy_dma_buf, GFP_KERNEL);
> 
>       for (i = 0; left_data >= 8; i++, left_data -= 8)
>              *buf_64++ = readq((u8 __iomem *)amd_spi->dma_virt_addr + (i * 8));
> 
>> Will re-spin v2 with necessary changes in Kconfig.
> 
> Please fix the real issue instead ;-)

We are using read*/write* calls instead of memcpy to copy data to/from 
DMA memory due to performance concerns, as we observed better throughput 
during continuous read/write compared to the memcpy functions.

Additionally, the DMA operations are entirely handled by the hardware, 
and the software's role is limited to copying data to the DMA buffer.

> 
>>> reproduce: (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202505110641.zLT16Dv7-lkp@intel.com/
>>>
>>> sparse warnings: (new ones prefixed by >>)
>>>>> drivers/spi/spi-amd.c:594:57: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got void * @@
>>>      drivers/spi/spi-amd.c:594:57: sparse:     expected void volatile [noderef] __iomem *addr
>>>      drivers/spi/spi-amd.c:594:57: sparse:     got void *
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by Geert Uytterhoeven 7 months, 1 week ago 
Hi Raju,

On Mon, 12 May 2025 at 19:55, Rangoju, Raju <raju.rangoju@amd.com> wrote:
> On 5/12/2025 7:47 PM, Geert Uytterhoeven wrote:
> > On Mon, 12 May 2025 at 09:29, Rangoju, Raju <raju.rangoju@amd.com> wrote:
> >> On 5/11/2025 3:51 AM, kernel test robot wrote:
> >>> kernel test robot noticed the following build warnings:
> >>>
> >>> [auto build test WARNING on v6.15-rc5]
> >>> [also build test WARNING on linus/master]
> >>> [cannot apply to broonie-spi/for-next next-20250509]
> >>> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >>> And when submitting patch, we suggest to use '--base' as documented in
> >>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>>
> >>> url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
> >>> base:   v6.15-rc5
> >>> patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
> >>> patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
> >>> config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
> >>> compiler: m68k-linux-gcc (GCC) 14.2.0
> >>
> >> Thanks for reporting this. We do not support m68k.
> >
> > All write[bwlq]() functions take a volatile void __iomem pointer
> > (https://elixir.bootlin.com/linux/v6.14.6/source/include/asm-generic/io.h#L174)
> > while you are passing a void *, so sparse should complain about this
> > on all architectures.
>
> My bad, with the following flags included, sparse now complains this on
> all architectures.
>
> -fmax-errors=unlimited -fmax-warnings=unlimited
>
> And sparse is right, this driver is using MMIO
> > accessors on allocated DMA memory, which is just plain wrong:
> >
> >      amd_spi->dma_virt_addr = dma_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE,
> >              &amd_spi->phy_dma_buf, GFP_KERNEL);
> >
> >       for (i = 0; left_data >= 8; i++, left_data -= 8)
> >              *buf_64++ = readq((u8 __iomem *)amd_spi->dma_virt_addr + (i * 8));
> >
> >> Will re-spin v2 with necessary changes in Kconfig.
> >
> > Please fix the real issue instead ;-)
>
> We are using read*/write* calls instead of memcpy to copy data to/from
> DMA memory due to performance concerns, as we observed better throughput
> during continuous read/write compared to the memcpy functions.

Perhaps your memcpy() copies backwards?
https://lwn.net/Articles/1016300/

There is no guarantee that read*/write* calls work on normal RAM on
all architectures. It may just crash, as some architectures return
cookies instead of real pointers when mapping MMIO.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Re: [PATCH] spi: spi_amd: Add HIDDMA basic write support
Posted by Rangoju, Raju 7 months ago 

On 5/13/2025 12:04 AM, Geert Uytterhoeven wrote:
> Hi Raju,
> 
> On Mon, 12 May 2025 at 19:55, Rangoju, Raju <raju.rangoju@amd.com> wrote:
>> On 5/12/2025 7:47 PM, Geert Uytterhoeven wrote:
>>> On Mon, 12 May 2025 at 09:29, Rangoju, Raju <raju.rangoju@amd.com> wrote:
>>>> On 5/11/2025 3:51 AM, kernel test robot wrote:
>>>>> kernel test robot noticed the following build warnings:
>>>>>
>>>>> [auto build test WARNING on v6.15-rc5]
>>>>> [also build test WARNING on linus/master]
>>>>> [cannot apply to broonie-spi/for-next next-20250509]
>>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>>>
>>>>> url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Rangoju/spi-spi_amd-Add-HIDDMA-basic-write-support/20250510-021954
>>>>> base:   v6.15-rc5
>>>>> patch link:    https://lore.kernel.org/r/20250509181737.997167-1-Raju.Rangoju%40amd.com
>>>>> patch subject: [PATCH] spi: spi_amd: Add HIDDMA basic write support
>>>>> config: m68k-randconfig-r111-20250511 (https://download.01.org/0day-ci/archive/20250511/202505110641.zLT16Dv7-lkp@intel.com/config)
>>>>> compiler: m68k-linux-gcc (GCC) 14.2.0
>>>>
>>>> Thanks for reporting this. We do not support m68k.
>>>
>>> All write[bwlq]() functions take a volatile void __iomem pointer
>>> (https://elixir.bootlin.com/linux/v6.14.6/source/include/asm-generic/io.h#L174)
>>> while you are passing a void *, so sparse should complain about this
>>> on all architectures.
>>
>> My bad, with the following flags included, sparse now complains this on
>> all architectures.
>>
>> -fmax-errors=unlimited -fmax-warnings=unlimited
>>
>> And sparse is right, this driver is using MMIO
>>> accessors on allocated DMA memory, which is just plain wrong:
>>>
>>>       amd_spi->dma_virt_addr = dma_alloc_coherent(dev, AMD_SPI_HID2_DMA_SIZE,
>>>               &amd_spi->phy_dma_buf, GFP_KERNEL);
>>>
>>>        for (i = 0; left_data >= 8; i++, left_data -= 8)
>>>               *buf_64++ = readq((u8 __iomem *)amd_spi->dma_virt_addr + (i * 8));
>>>
>>>> Will re-spin v2 with necessary changes in Kconfig.
>>>
>>> Please fix the real issue instead ;-)
>>
>> We are using read*/write* calls instead of memcpy to copy data to/from
>> DMA memory due to performance concerns, as we observed better throughput
>> during continuous read/write compared to the memcpy functions.
>
Hi Geert,

> Perhaps your memcpy() copies backwards?

Nope. The Source and destinations are in different address range, so we 
do not do backward copying.

> https://lwn.net/Articles/1016300/
> 
> There is no guarantee that read*/write* calls work on normal RAM on
> all architectures. It may just crash, as some architectures return
> cookies instead of real pointers when mapping MMIO.

Okay. We will copy the data to/from the DMA buffer by iterating 
(ensuring that memory access is safe) by avoiding MMIO accessor usage 
(`read*`/`write*`).

For example:

u64 *dma_buff = (u64 *)amd_spi->dma_virt_addr;
...
for (i = 0; i < nbytes / 8; i++)
     *dma_buff++ = *buf_64++;

We will re-spin V2 with these changes.

> Gr{oetje,eeting}s,
> 
>                          Geert
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.