[PATCH 0/9] Improve support for AMD SPI controllers

[PATCH 0/9] Improve support for AMD SPI controllers

[Lucas Tanure]

Add support for AMDI0062 and overcome the fact that the
controller can't hold the chip select activated between
transfers.

AMD SPI controller starts the SPI transfer by copying a
special byte called opcode into the bus, followed by the
TX register bytes from the FIFO into the bus.
If the RX register is not zero, it will copy RX bytes
from the bus to the FIFO.

Rules:
 - It must have an opcode set, which can be the first
byte from the writing part
 - The writing part of the FIFO always goes first
 - It's not full duplex, it writes TX bytes and then
reads RX bytes into the FIFO
 - Write and Read share the same FIFO. If the transfer
needs to write N bytes, it will only be able to read
(70 - N) bytes.
 - The chip select can only be activated during that
transaction. If a second transfer rely on the address
written during a previous transfer, it needs to write
an updated address, or it will fail, as the device in
the SPI bus will not understand a read without an
address as the chip select was not held between transfers.

So, when the regmap splits a write to an address or read from
an address into 2 separated transfers inside one message the
AMD SPI driver needs to merge them back into a single one.
Also it needs to be sure that the of bytes to read|write is
a little less so the address can fit into the FIFO.

Lucas Tanure (9):
  regmap: spi: Set regmap max raw r/w from max_transfer_size
  spi: core: Add flag for controllers that can't hold cs between
    transfers
  regmap: spi: SPI_CONTROLLER_CS_PER_TRANSFER affects max read/write
  spi: amd: Refactor code to use less spi_master_get_devdata
  spi: amd: Refactor amd_spi_busy_wait to use readl_poll_timeout
  spi: amd: Remove uneeded variable
  spi: amd: Check for idle bus before execute opcode
  spi: amd: Refactor to overcome 70 bytes per CS limitation
  spi: amd: Add support for latest platform

 drivers/base/regmap/regmap-spi.c |  44 +++-
 drivers/base/regmap/regmap.c     |   9 +
 drivers/spi/spi-amd.c            | 415 ++++++++++++++++++++-----------
 include/linux/regmap.h           |   2 +
 include/linux/spi/spi.h          |   1 +
 5 files changed, 315 insertions(+), 156 deletions(-)

-- 
2.33.0

[PATCH 1/9] regmap: spi: Set regmap max raw r/w from max_transfer_size

[Lucas Tanure]

Set regmap raw read/write from spi max_transfer_size
so regmap_raw_read/write can split the access into chunks

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/base/regmap/regmap-spi.c | 34 ++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
index c1894e93c378..603a4c1c2066 100644
--- a/drivers/base/regmap/regmap-spi.c
+++ b/drivers/base/regmap/regmap-spi.c
@@ -109,13 +109,35 @@ static const struct regmap_bus regmap_spi = {
 	.val_format_endian_default = REGMAP_ENDIAN_BIG,
 };
 
+static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi,
+						   const struct regmap_config *config)
+{
+	struct spi_master *master = spi->master;
+	struct regmap_bus *bus = NULL;
+
+	if (master->max_transfer_size) {
+		bus = kmemdup(&regmap_spi, sizeof(*bus), GFP_KERNEL);
+		if (!bus)
+			return ERR_PTR(-ENOMEM);
+		bus->free_on_exit = true;
+		bus->max_raw_read = bus->max_raw_write = master->max_transfer_size(spi);
+		return bus;
+	}
+
+	return &regmap_spi;
+}
+
 struct regmap *__regmap_init_spi(struct spi_device *spi,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name)
 {
-	return __regmap_init(&spi->dev, &regmap_spi, &spi->dev, config,
-			     lock_key, lock_name);
+	const struct regmap_bus *bus = regmap_get_spi_bus(spi, config);
+
+	if (IS_ERR(bus))
+		return ERR_CAST(bus);
+
+	return __regmap_init(&spi->dev, bus, &spi->dev, config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__regmap_init_spi);
 
@@ -124,8 +146,12 @@ struct regmap *__devm_regmap_init_spi(struct spi_device *spi,
 				      struct lock_class_key *lock_key,
 				      const char *lock_name)
 {
-	return __devm_regmap_init(&spi->dev, &regmap_spi, &spi->dev, config,
-				  lock_key, lock_name);
+	const struct regmap_bus *bus = regmap_get_spi_bus(spi, config);
+
+	if (IS_ERR(bus))
+		return ERR_CAST(bus);
+
+	return __devm_regmap_init(&spi->dev, bus, &spi->dev, config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__devm_regmap_init_spi);
 
-- 
2.33.0

Re: [PATCH 1/9] regmap: spi: Set regmap max raw r/w from max_transfer_size

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:33AM +0100, Lucas Tanure wrote:

> +	if (master->max_transfer_size) {
> +		bus = kmemdup(&regmap_spi, sizeof(*bus), GFP_KERNEL);

We shouldn't be peering into the controller structure, use
spi_max_transfer_size() instead.

> +		bus->max_raw_read = bus->max_raw_write = master->max_transfer_size(spi);

Just write two assignment statements, it's more legible all round.

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

[PATCH 2/9] spi: core: Add flag for controllers that can't hold cs between transfers

[Lucas Tanure]

Create a flag for a controller that has an automatic cs selection and
can't hold cs activated between transfers
Some messages send address and data split between two transfers, see
regmap-spi, and without the cs held the data loses it`s meaning

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 include/linux/spi/spi.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 8371bca13729..f5b55c237634 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -514,6 +514,7 @@ struct spi_controller {
 #define SPI_CONTROLLER_MUST_TX		BIT(4)	/* requires tx */
 
 #define SPI_MASTER_GPIO_SS		BIT(5)	/* GPIO CS must select slave */
+#define SPI_CONTROLLER_CS_PER_TRANSFER	BIT(6)	/* SPI controller can not hold CS between transfers */
 
 	/* flag indicating if the allocation of this struct is devres-managed */
 	bool			devm_allocated;
-- 
2.33.0

Re: [PATCH 2/9] spi: core: Add flag for controllers that can't hold cs between transfers

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:34AM +0100, Lucas Tanure wrote:
> Create a flag for a controller that has an automatic cs selection and
> can't hold cs activated between transfers
> Some messages send address and data split between two transfers, see
> regmap-spi, and without the cs held the data loses it`s meaning

These controllers just plain can't support multiple transfers in any
useful fashion, the flag name should reflect that as well as being a bit
less long for legibility reasons.

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

[PATCH 3/9] regmap: spi: SPI_CONTROLLER_CS_PER_TRANSFER affects max read/write

[Lucas Tanure]

regmap-spi will split data and address between two transfers in the
same message so use addr_affects_max_raw_rw to flag that the number
bytes to read or write should be a little less (address + padding size),
so that the SPI controller can merge the entire message into a single
CS period

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/base/regmap/regmap-spi.c | 14 ++++++++++++--
 drivers/base/regmap/regmap.c     |  9 +++++++++
 include/linux/regmap.h           |  2 ++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
index 603a4c1c2066..0c1f2e51c0c7 100644
--- a/drivers/base/regmap/regmap-spi.c
+++ b/drivers/base/regmap/regmap-spi.c
@@ -115,12 +115,22 @@ static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi,
 	struct spi_master *master = spi->master;
 	struct regmap_bus *bus = NULL;
 
-	if (master->max_transfer_size) {
+	if (master->max_transfer_size || (master->flags & SPI_CONTROLLER_CS_PER_TRANSFER)) {
 		bus = kmemdup(&regmap_spi, sizeof(*bus), GFP_KERNEL);
 		if (!bus)
 			return ERR_PTR(-ENOMEM);
 		bus->free_on_exit = true;
-		bus->max_raw_read = bus->max_raw_write = master->max_transfer_size(spi);
+
+		/* regmap-spi will split data and address between two transfers in the same message
+		 * so use addr_affects_max_raw_rw to flag that the number bytes to read or write
+		 * should be a little less (address + padding size), so the controller can
+		 * fit both transfers in a single CS period
+		 */
+		bus->addr_affects_max_raw_rw = master->flags & SPI_CONTROLLER_CS_PER_TRANSFER;
+
+		if (master->max_transfer_size)
+			bus->max_raw_read = bus->max_raw_write = master->max_transfer_size(spi);
+
 		return bus;
 	}
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6ad41d0720ba..31d0949b6c2f 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -815,6 +815,15 @@ struct regmap *__regmap_init(struct device *dev,
 	if (bus) {
 		map->max_raw_read = bus->max_raw_read;
 		map->max_raw_write = bus->max_raw_write;
+		if (bus->addr_affects_max_raw_rw) {
+			if (map->max_raw_read  < map->format.buf_size ||
+			    map->max_raw_write < map->format.buf_size) {
+				ret = -EINVAL;
+				goto err_name;
+			}
+			map->max_raw_read -= (map->format.reg_bytes + map->format.pad_bytes);
+			map->max_raw_write -= (map->format.reg_bytes + map->format.pad_bytes);
+		}
 	}
 	map->dev = dev;
 	map->bus = bus;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 77755196277c..a90d1e270b1f 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -504,6 +504,7 @@ typedef void (*regmap_hw_free_context)(void *context);
  * @max_raw_read: Max raw read size that can be used on the bus.
  * @max_raw_write: Max raw write size that can be used on the bus.
  * @free_on_exit: kfree this on exit of regmap
+ * @addr_affects_max_raw_rw: max_raw_[read|write] must include the address and padding preamble
  */
 struct regmap_bus {
 	bool fast_io;
@@ -522,6 +523,7 @@ struct regmap_bus {
 	size_t max_raw_read;
 	size_t max_raw_write;
 	bool free_on_exit;
+	bool addr_affects_max_raw_rw;
 };
 
 /*
-- 
2.33.0

Re: [PATCH 3/9] regmap: spi: SPI_CONTROLLER_CS_PER_TRANSFER affects max read/write

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:35AM +0100, Lucas Tanure wrote:
> regmap-spi will split data and address between two transfers in the
> same message so use addr_affects_max_raw_rw to flag that the number
> bytes to read or write should be a little less (address + padding size),
> so that the SPI controller can merge the entire message into a single
> CS period

This should be handled by the SPI core, it's already relying on being
able to do multiple transfers to handle message size limits and in any
case this is a super standard thing to do so many clients would require
special code.  The core should transparently coalesce things where it
can, or error out if it can't, like it currently does when splitting
transfers up.

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

Re: [PATCH 3/9] regmap: spi: SPI_CONTROLLER_CS_PER_TRANSFER affects max read/write

[Lucas tanure]

On 8/24/21 5:37 PM, Mark Brown wrote:
> On Tue, Aug 24, 2021 at 11:40:35AM +0100, Lucas Tanure wrote:
>> regmap-spi will split data and address between two transfers in the
>> same message so use addr_affects_max_raw_rw to flag that the number
>> bytes to read or write should be a little less (address + padding size),
>> so that the SPI controller can merge the entire message into a single
>> CS period
> 
> This should be handled by the SPI core, it's already relying on being
> able to do multiple transfers to handle message size limits and in any
> case this is a super standard thing to do so many clients would require
> special code.  The core should transparently coalesce things where it
> can, or error out if it can't, like it currently does when splitting
> transfers up.
> 
__spi_validate seems a good candidate, but I don't think spi have enough
information to merge two transfers into a single one.

For a message with N transfers how can spi core decide what to merge or 
what not merge. If mergers everything and is less than max_transfer_size 
success, but if bigger will need to stop merging and add an address in 
front of the next not merged transfer, but spi core is not aware of 
addresses
And in the case of multiple addresses and data transfers, how it will 
know doesn't need to be merged?

For me seems more reasonable for the regmap-spi stop splitting address
and data. Or at least if the controller has some flag change the bus for
one where it uses different functions for gather_write, async_write etc

Can you point which way you think the code should go? Investigate more 
spi core to coalesce transfers or change regmap-spi to not split address 
and data anymore?

Thanks
Lucas

Re: [PATCH 3/9] regmap: spi: SPI_CONTROLLER_CS_PER_TRANSFER affects max read/write

[Mark Brown]

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

On Wed, Aug 25, 2021 at 06:13:01PM +0100, Lucas tanure wrote:
> On 8/24/21 5:37 PM, Mark Brown wrote:

> > This should be handled by the SPI core, it's already relying on being
> > able to do multiple transfers to handle message size limits and in any
> > case this is a super standard thing to do so many clients would require

> For a message with N transfers how can spi core decide what to merge or what
> not merge. If mergers everything and is less than max_transfer_size success,

In the same way it does for transfers that are too long.  If the
controller has a property saying that it can't handle more than one
transfer then the core needs to either combine multiple transfers in a
single message into a single transfer or return an error to the caller
(modulo handling of cs_change).  If the controller can handle the
message it should just get passed straight through.

> but if bigger will need to stop merging and add an address in front of the
> next not merged transfer, but spi core is not aware of addresses
> And in the case of multiple addresses and data transfers, how it will know
> doesn't need to be merged?

The spi_message says what the message should look like on the bus.  The
semantics of what's in the message don't matter.  

> For me seems more reasonable for the regmap-spi stop splitting address
> and data. Or at least if the controller has some flag change the bus for
> one where it uses different functions for gather_write, async_write etc

This would force us to marshall the data in memory prior to sending
which adds overhead.

> Can you point which way you think the code should go? Investigate more spi
> core to coalesce transfers or change regmap-spi to not split address and
> data anymore?

Like I said in reply to your driver patch it looks like this
fundamentally doesn't do what you want in the first place.

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

[PATCH 4/9] spi: amd: Refactor code to use less spi_master_get_devdata

[Lucas Tanure]

Get master data in the start and then just use struct amd_spi
as it has the needed variable

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 94 ++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 60 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 3cf76096a76d..f23467cf6acd 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -41,85 +41,66 @@ struct amd_spi {
 	u8 chip_select;
 };
 
-static inline u8 amd_spi_readreg8(struct spi_master *master, int idx)
+static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-
 	return ioread8((u8 __iomem *)amd_spi->io_remap_addr + idx);
 }
 
-static inline void amd_spi_writereg8(struct spi_master *master, int idx,
-				     u8 val)
+static inline void amd_spi_writereg8(struct amd_spi *amd_spi, int idx, u8 val)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-
 	iowrite8(val, ((u8 __iomem *)amd_spi->io_remap_addr + idx));
 }
 
-static inline void amd_spi_setclear_reg8(struct spi_master *master, int idx,
-					 u8 set, u8 clear)
+static void amd_spi_setclear_reg8(struct amd_spi *amd_spi, int idx, u8 set, u8 clear)
 {
-	u8 tmp = amd_spi_readreg8(master, idx);
+	u8 tmp = amd_spi_readreg8(amd_spi, idx);
 
 	tmp = (tmp & ~clear) | set;
-	amd_spi_writereg8(master, idx, tmp);
+	amd_spi_writereg8(amd_spi, idx, tmp);
 }
 
-static inline u32 amd_spi_readreg32(struct spi_master *master, int idx)
+static inline u32 amd_spi_readreg32(struct amd_spi *amd_spi, int idx)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-
 	return ioread32((u8 __iomem *)amd_spi->io_remap_addr + idx);
 }
 
-static inline void amd_spi_writereg32(struct spi_master *master, int idx,
-				      u32 val)
+static inline void amd_spi_writereg32(struct amd_spi *amd_spi, int idx, u32 val)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-
 	iowrite32(val, ((u8 __iomem *)amd_spi->io_remap_addr + idx));
 }
 
-static inline void amd_spi_setclear_reg32(struct spi_master *master, int idx,
-					  u32 set, u32 clear)
+static inline void amd_spi_setclear_reg32(struct amd_spi *amd_spi, int idx, u32 set, u32 clear)
 {
-	u32 tmp = amd_spi_readreg32(master, idx);
+	u32 tmp = amd_spi_readreg32(amd_spi, idx);
 
 	tmp = (tmp & ~clear) | set;
-	amd_spi_writereg32(master, idx, tmp);
+	amd_spi_writereg32(amd_spi, idx, tmp);
 }
 
-static void amd_spi_select_chip(struct spi_master *master)
+static void amd_spi_select_chip(struct amd_spi *amd_spi)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-	u8 chip_select = amd_spi->chip_select;
-
-	amd_spi_setclear_reg8(master, AMD_SPI_ALT_CS_REG, chip_select,
+	amd_spi_setclear_reg8(amd_spi, AMD_SPI_ALT_CS_REG, amd_spi->chip_select,
 			      AMD_SPI_ALT_CS_MASK);
 }
 
-static void amd_spi_clear_fifo_ptr(struct spi_master *master)
+static void amd_spi_clear_fifo_ptr(struct amd_spi *amd_spi)
 {
-	amd_spi_setclear_reg32(master, AMD_SPI_CTRL0_REG, AMD_SPI_FIFO_CLEAR,
-			       AMD_SPI_FIFO_CLEAR);
+	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_FIFO_CLEAR, AMD_SPI_FIFO_CLEAR);
 }
 
-static void amd_spi_set_opcode(struct spi_master *master, u8 cmd_opcode)
+static void amd_spi_set_opcode(struct amd_spi *amd_spi, u8 cmd_opcode)
 {
-	amd_spi_setclear_reg32(master, AMD_SPI_CTRL0_REG, cmd_opcode,
-			       AMD_SPI_OPCODE_MASK);
+	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, cmd_opcode, AMD_SPI_OPCODE_MASK);
 }
 
-static inline void amd_spi_set_rx_count(struct spi_master *master,
-					u8 rx_count)
+static inline void amd_spi_set_rx_count(struct amd_spi *amd_spi, u8 rx_count)
 {
-	amd_spi_setclear_reg8(master, AMD_SPI_RX_COUNT_REG, rx_count, 0xff);
+	amd_spi_setclear_reg8(amd_spi, AMD_SPI_RX_COUNT_REG, rx_count, 0xff);
 }
 
-static inline void amd_spi_set_tx_count(struct spi_master *master,
-					u8 tx_count)
+static inline void amd_spi_set_tx_count(struct amd_spi *amd_spi, u8 tx_count)
 {
-	amd_spi_setclear_reg8(master, AMD_SPI_TX_COUNT_REG, tx_count, 0xff);
+	amd_spi_setclear_reg8(amd_spi, AMD_SPI_TX_COUNT_REG, tx_count, 0xff);
 }
 
 static inline int amd_spi_busy_wait(struct amd_spi *amd_spi)
@@ -142,22 +123,18 @@ static inline int amd_spi_busy_wait(struct amd_spi *amd_spi)
 	return 0;
 }
 
-static void amd_spi_execute_opcode(struct spi_master *master)
+static void amd_spi_execute_opcode(struct amd_spi *amd_spi)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-
 	/* Set ExecuteOpCode bit in the CTRL0 register */
-	amd_spi_setclear_reg32(master, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD,
-			       AMD_SPI_EXEC_CMD);
-
+	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD);
 	amd_spi_busy_wait(amd_spi);
 }
 
 static int amd_spi_master_setup(struct spi_device *spi)
 {
-	struct spi_master *master = spi->master;
+	struct amd_spi *amd_spi = spi_master_get_devdata(spi->master);
 
-	amd_spi_clear_fifo_ptr(master);
+	amd_spi_clear_fifo_ptr(amd_spi);
 
 	return 0;
 }
@@ -185,19 +162,18 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
 			tx_len = xfer->len - 1;
 			cmd_opcode = *(u8 *)xfer->tx_buf;
 			buf++;
-			amd_spi_set_opcode(master, cmd_opcode);
+			amd_spi_set_opcode(amd_spi, cmd_opcode);
 
 			/* Write data into the FIFO. */
 			for (i = 0; i < tx_len; i++) {
-				iowrite8(buf[i],
-					 ((u8 __iomem *)amd_spi->io_remap_addr +
+				iowrite8(buf[i], ((u8 __iomem *)amd_spi->io_remap_addr +
 					 AMD_SPI_FIFO_BASE + i));
 			}
 
-			amd_spi_set_tx_count(master, tx_len);
-			amd_spi_clear_fifo_ptr(master);
+			amd_spi_set_tx_count(amd_spi, tx_len);
+			amd_spi_clear_fifo_ptr(amd_spi);
 			/* Execute command */
-			amd_spi_execute_opcode(master);
+			amd_spi_execute_opcode(amd_spi);
 		}
 		if (m_cmd & AMD_SPI_XFER_RX) {
 			/*
@@ -206,15 +182,13 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
 			 */
 			rx_len = xfer->len;
 			buf = (u8 *)xfer->rx_buf;
-			amd_spi_set_rx_count(master, rx_len);
-			amd_spi_clear_fifo_ptr(master);
+			amd_spi_set_rx_count(amd_spi, rx_len);
+			amd_spi_clear_fifo_ptr(amd_spi);
 			/* Execute command */
-			amd_spi_execute_opcode(master);
+			amd_spi_execute_opcode(amd_spi);
 			/* Read data from FIFO to receive buffer  */
 			for (i = 0; i < rx_len; i++)
-				buf[i] = amd_spi_readreg8(master,
-							  AMD_SPI_FIFO_BASE +
-							  tx_len + i);
+				buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i);
 		}
 	}
 
@@ -234,7 +208,7 @@ static int amd_spi_master_transfer(struct spi_master *master,
 	struct spi_device *spi = msg->spi;
 
 	amd_spi->chip_select = spi->chip_select;
-	amd_spi_select_chip(master);
+	amd_spi_select_chip(amd_spi);
 
 	/*
 	 * Extract spi_transfers from the spi message and
-- 
2.33.0

[PATCH 5/9] spi: amd: Refactor amd_spi_busy_wait to use readl_poll_timeout

[Lucas Tanure]

This function can be replaced by readl_poll_timeout

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index f23467cf6acd..9476b283840b 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/delay.h>
 #include <linux/spi/spi.h>
+#include <linux/iopoll.h>
 
 #define AMD_SPI_CTRL0_REG	0x00
 #define AMD_SPI_EXEC_CMD	BIT(16)
@@ -103,24 +104,12 @@ static inline void amd_spi_set_tx_count(struct amd_spi *amd_spi, u8 tx_count)
 	amd_spi_setclear_reg8(amd_spi, AMD_SPI_TX_COUNT_REG, tx_count, 0xff);
 }
 
-static inline int amd_spi_busy_wait(struct amd_spi *amd_spi)
+static int amd_spi_busy_wait(struct amd_spi *amd_spi)
 {
-	bool spi_busy;
-	int timeout = 100000;
-
-	/* poll for SPI bus to become idle */
-	spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr +
-		    AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY;
-	while (spi_busy) {
-		usleep_range(10, 20);
-		if (timeout-- < 0)
-			return -ETIMEDOUT;
-
-		spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr +
-			    AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY;
-	}
+	u32 val;
 
-	return 0;
+	return readl_poll_timeout(amd_spi->io_remap_addr + AMD_SPI_CTRL0_REG, val,
+				 !(val & AMD_SPI_BUSY), 10, 100000);
 }
 
 static void amd_spi_execute_opcode(struct amd_spi *amd_spi)
-- 
2.33.0

Re: [PATCH 5/9] spi: amd: Refactor amd_spi_busy_wait to use readl_poll_timeout

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:37AM +0100, Lucas Tanure wrote:

> -	spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr +
> -		    AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY;

This does ioread32() while readl_poll_timeout() does a readl().  These
are almost but not quite equivalent and the differences apply to PCI
devices like this one, see device-io.rst.

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

[PATCH 6/9] spi: amd: Remove uneeded variable

[Lucas Tanure]

Remove internal cs from amd_spi

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 9476b283840b..ebbc64a9fa7b 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -39,7 +39,6 @@ struct amd_spi {
 	void __iomem *io_remap_addr;
 	unsigned long io_base_addr;
 	u32 rom_addr;
-	u8 chip_select;
 };
 
 static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
@@ -78,10 +77,9 @@ static inline void amd_spi_setclear_reg32(struct amd_spi *amd_spi, int idx, u32
 	amd_spi_writereg32(amd_spi, idx, tmp);
 }
 
-static void amd_spi_select_chip(struct amd_spi *amd_spi)
+static void amd_spi_select_chip(struct amd_spi *amd_spi, u8 cs)
 {
-	amd_spi_setclear_reg8(amd_spi, AMD_SPI_ALT_CS_REG, amd_spi->chip_select,
-			      AMD_SPI_ALT_CS_MASK);
+	amd_spi_setclear_reg8(amd_spi, AMD_SPI_ALT_CS_REG, cs, AMD_SPI_ALT_CS_MASK);
 }
 
 static void amd_spi_clear_fifo_ptr(struct amd_spi *amd_spi)
@@ -196,8 +194,7 @@ static int amd_spi_master_transfer(struct spi_master *master,
 	struct amd_spi *amd_spi = spi_master_get_devdata(master);
 	struct spi_device *spi = msg->spi;
 
-	amd_spi->chip_select = spi->chip_select;
-	amd_spi_select_chip(amd_spi);
+	amd_spi_select_chip(amd_spi, spi->chip_select);
 
 	/*
 	 * Extract spi_transfers from the spi message and
-- 
2.33.0

[PATCH 7/9] spi: amd: Check for idle bus before execute opcode

[Lucas Tanure]

Check if the bus is not in use before starting the transfer
Also wait after so the READ bytes in the FIFO are ready to
be copied

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index ebbc64a9fa7b..75390fcb0481 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -110,11 +110,17 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi)
 				 !(val & AMD_SPI_BUSY), 10, 100000);
 }
 
-static void amd_spi_execute_opcode(struct amd_spi *amd_spi)
+static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
 {
+	int ret = amd_spi_busy_wait(amd_spi);
+
+	if (ret)
+		return ret;
+
 	/* Set ExecuteOpCode bit in the CTRL0 register */
 	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD);
-	amd_spi_busy_wait(amd_spi);
+
+	return amd_spi_busy_wait(amd_spi);
 }
 
 static int amd_spi_master_setup(struct spi_device *spi)
-- 
2.33.0

Re: [PATCH 7/9] spi: amd: Check for idle bus before execute opcode

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:39AM +0100, Lucas Tanure wrote:
> Check if the bus is not in use before starting the transfer
> Also wait after so the READ bytes in the FIFO are ready to
> be copied

This means that we will wait for read to be ready even for write only
operations, as opposed to potentially just absorbing the delay while the
CPU does other stuff.  If we need to wait prior to reading we should do
that in the relevant code.

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

[PATCH 8/9] spi: amd: Refactor to overcome 70 bytes per CS limitation

[Lucas Tanure]

AMD SPI controller has 70 bytes for its FIFO and it has an
automatic way of controlling it`s internal CS, which can
only be activated during the time that the FIFO is being
transfered.

SPI_MASTER_HALF_DUPLEX here means that it can only read
RX bytes after TX bytes were written, and RX+TX must be
less than 70. If you write 4 bytes the first byte of read
is in position 5 of the FIFO.

All of that means that for devices that require an address
for reads and writes, the 2 transfers must be put in the same
FIFO so the CS can be hold for address and data, otherwise
the data would lose it`s meaning.

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 208 ++++++++++++++++++++++++++++--------------
 1 file changed, 140 insertions(+), 68 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 75390fcb0481..b6308733265e 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -4,7 +4,8 @@
 //
 // Copyright (c) 2020, Advanced Micro Devices, Inc.
 //
-// Author: Sanjay R Mehta <sanju.mehta@amd.com>
+// Authors: Sanjay R Mehta <sanju.mehta@amd.com>
+//          Lucas Tanure <tanureal@opensource.cirrus.com>
 
 #include <linux/acpi.h>
 #include <linux/init.h>
@@ -29,6 +30,7 @@
 #define AMD_SPI_RX_COUNT_REG	0x4B
 #define AMD_SPI_STATUS_REG	0x4C
 
+#define AMD_SPI_FIFO_SIZE	70
 #define AMD_SPI_MEM_SIZE	200
 
 /* M_CMD OP codes for SPI */
@@ -132,83 +134,152 @@ static int amd_spi_master_setup(struct spi_device *spi)
 	return 0;
 }
 
-static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
-				    struct spi_master *master,
-				    struct spi_message *message)
+static int amd_spi_double_write(struct spi_master *mst, u8 *tx1_buf, u8 tx1_len, u8 *tx2_buf,
+				u8 tx2_len)
 {
-	struct spi_transfer *xfer = NULL;
-	u8 cmd_opcode;
-	u8 *buf = NULL;
-	u32 m_cmd = 0;
-	u32 i = 0;
-	u32 tx_len = 0, rx_len = 0;
-
-	list_for_each_entry(xfer, &message->transfers,
-			    transfer_list) {
-		if (xfer->rx_buf)
-			m_cmd = AMD_SPI_XFER_RX;
-		if (xfer->tx_buf)
-			m_cmd = AMD_SPI_XFER_TX;
-
-		if (m_cmd & AMD_SPI_XFER_TX) {
-			buf = (u8 *)xfer->tx_buf;
-			tx_len = xfer->len - 1;
-			cmd_opcode = *(u8 *)xfer->tx_buf;
-			buf++;
-			amd_spi_set_opcode(amd_spi, cmd_opcode);
-
-			/* Write data into the FIFO. */
-			for (i = 0; i < tx_len; i++) {
-				iowrite8(buf[i], ((u8 __iomem *)amd_spi->io_remap_addr +
-					 AMD_SPI_FIFO_BASE + i));
-			}
+	struct amd_spi *amd_spi = spi_master_get_devdata(mst);
+	int i, ret;
 
-			amd_spi_set_tx_count(amd_spi, tx_len);
-			amd_spi_clear_fifo_ptr(amd_spi);
-			/* Execute command */
-			amd_spi_execute_opcode(amd_spi);
-		}
-		if (m_cmd & AMD_SPI_XFER_RX) {
-			/*
-			 * Store no. of bytes to be received from
-			 * FIFO
-			 */
-			rx_len = xfer->len;
-			buf = (u8 *)xfer->rx_buf;
-			amd_spi_set_rx_count(amd_spi, rx_len);
-			amd_spi_clear_fifo_ptr(amd_spi);
-			/* Execute command */
-			amd_spi_execute_opcode(amd_spi);
-			/* Read data from FIFO to receive buffer  */
-			for (i = 0; i < rx_len; i++)
-				buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i);
-		}
-	}
+	if (tx1_len + tx2_len > AMD_SPI_FIFO_SIZE)
+		return -EINVAL;
 
-	/* Update statistics */
-	message->actual_length = tx_len + rx_len + 1;
-	/* complete the transaction */
-	message->status = 0;
-	spi_finalize_current_message(master);
+	amd_spi_clear_fifo_ptr(amd_spi);
+	amd_spi_set_rx_count(amd_spi, 0);
 
-	return 0;
+	amd_spi_set_opcode(amd_spi, tx1_buf[0]);
+	tx1_len--;
+	tx1_buf++;
+
+	for (i = 0; i < tx1_len; i++)
+		amd_spi_writereg8(amd_spi, (u8)(AMD_SPI_FIFO_BASE + i), tx1_buf[i]);
+
+	for (i = 0; i < tx2_len; i++)
+		amd_spi_writereg8(amd_spi, (u8)(AMD_SPI_FIFO_BASE + tx1_len + i), tx2_buf[i]);
+
+	amd_spi_set_tx_count(amd_spi, tx1_len + tx2_len);
+	ret = amd_spi_execute_opcode(amd_spi);
+
+	return ret ? ret : tx1_len + 1 + tx2_len;
 }
 
-static int amd_spi_master_transfer(struct spi_master *master,
-				   struct spi_message *msg)
+static int amd_spi_write_read(struct spi_master *mst, u8 *tx_buf, u8 tx_len, u8 *rx_buf, u8 rx_len)
 {
-	struct amd_spi *amd_spi = spi_master_get_devdata(master);
-	struct spi_device *spi = msg->spi;
+	struct amd_spi *amd_spi = spi_master_get_devdata(mst);
+	int i, ret;
+
+	if (tx_len + rx_len > AMD_SPI_FIFO_SIZE)
+		return -EINVAL;
+
+	amd_spi_clear_fifo_ptr(amd_spi);
+
+	if (tx_buf) {
+		/* Take the first byte to be written and set as opcode */
+		amd_spi_set_opcode(amd_spi, tx_buf[0]);
+		/* Set TX count as the number of bytes to be written less one (opcode byte) */
+		tx_len--;
+		tx_buf++;
 
-	amd_spi_select_chip(amd_spi, spi->chip_select);
+		/* Copy to the FIFO the remaining bytes */
+		for (i = 0; i < tx_len; i++)
+			amd_spi_writereg8(amd_spi, (AMD_SPI_FIFO_BASE + i), tx_buf[i]);
 
-	/*
-	 * Extract spi_transfers from the spi message and
-	 * program the controller.
+		amd_spi_set_tx_count(amd_spi, tx_len);
+	}
+	/* Set RX count as the number of bytes that will be read AFTER the TX bytes are sent
+	 * Or set to zero to avoid extra bytes after the write cycle
 	 */
-	amd_spi_fifo_xfer(amd_spi, master, msg);
+	amd_spi_set_rx_count(amd_spi, rx_buf ? rx_len : 0);
 
-	return 0;
+	/* Trigger the transfer by executing the opcode */
+	ret = amd_spi_execute_opcode(amd_spi);
+	if (ret)
+		return ret;
+
+	/* Wait for the SPI bus to be idle and copy the RX bytes from the FIFO from the starting
+	 * position of TX bytes
+	 */
+	if (rx_buf) {
+		for (i = 0; i < rx_len; i++)
+			rx_buf[i] = amd_spi_readreg8(amd_spi, AMD_SPI_FIFO_BASE + tx_len + i);
+	}
+
+	return tx_len + 1 + rx_len;
+}
+
+/* amd_spi_master_transfer expects a spi_message with one or two transfers only
+ * Where a message with one transfer is a single write or read to a device
+ * And a message with two transfer is an address write followed by a read or
+ * write data into that address
+ */
+static int amd_spi_master_transfer(struct spi_master *mst, struct spi_message *msg)
+{
+	struct amd_spi *amd_spi = spi_master_get_devdata(mst);
+	struct spi_transfer *xfer, *xfer_n;
+	struct list_head *pos;
+	int ret, count = 0;
+
+	list_for_each(pos, &msg->transfers)
+		count++;
+
+	amd_spi_select_chip(amd_spi, msg->spi->chip_select);
+
+	xfer = list_first_entry(&msg->transfers, struct spi_transfer, transfer_list);
+	switch (count) {
+	case 1:
+		/* This controller can't write and read simultaneously
+		 * It can only write data first and read afterwards
+		 */
+		if (xfer->tx_buf && xfer->rx_buf) {
+			ret = -EINVAL;
+			dev_err(&mst->dev, "Error. Can't write and read simultaneously\n");
+			goto complete;
+		}
+
+		ret = amd_spi_write_read(mst, (u8 *)xfer->tx_buf, xfer->len,
+					      (u8 *)xfer->rx_buf, xfer->len);
+		if (ret < 0)
+			goto complete;
+		break;
+	case 2:
+		xfer_n = list_last_entry(&msg->transfers, struct spi_transfer, transfer_list);
+		if (xfer->tx_buf && !xfer->rx_buf) {
+			if (xfer_n->rx_buf && !xfer_n->tx_buf) {
+				ret = amd_spi_write_read(mst, (u8 *)xfer->tx_buf, xfer->len,
+							      (u8 *)xfer_n->rx_buf, xfer_n->len);
+				if (ret < 0)
+					goto complete;
+				break;
+			} else if (xfer_n->tx_buf && !xfer_n->rx_buf) {
+				ret = amd_spi_double_write(mst, (u8 *)xfer->tx_buf, xfer->len,
+								(u8 *)xfer_n->tx_buf, xfer_n->len);
+				if (ret < 0)
+					goto complete;
+				break;
+			}
+		}
+		ret = -EINVAL;
+		dev_err(&mst->dev, "Error. Message not supported\n");
+		goto complete;
+	default:
+		ret = -EINVAL;
+		dev_err(&mst->dev, "Message with %d transfers is not supported\n", count);
+		goto complete;
+	}
+
+	msg->actual_length += ret;
+	ret = 0;
+
+complete:
+	/* complete the transaction */
+	msg->status = ret;
+	spi_finalize_current_message(mst);
+
+	return ret;
+}
+
+static size_t amd_spi_max_transfer_size(struct spi_device *spi)
+{
+	return AMD_SPI_FIFO_SIZE;
 }
 
 static int amd_spi_probe(struct platform_device *pdev)
@@ -238,8 +309,9 @@ static int amd_spi_probe(struct platform_device *pdev)
 	master->bus_num = 0;
 	master->num_chipselect = 4;
 	master->mode_bits = 0;
-	master->flags = SPI_MASTER_HALF_DUPLEX;
+	master->flags = SPI_MASTER_HALF_DUPLEX | SPI_CONTROLLER_CS_PER_TRANSFER;
 	master->setup = amd_spi_master_setup;
+	master->max_transfer_size = amd_spi_max_transfer_size;
 	master->transfer_one_message = amd_spi_master_transfer;
 
 	/* Register the controller with SPI framework */
-- 
2.33.0

Re: [PATCH 8/9] spi: amd: Refactor to overcome 70 bytes per CS limitation

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:40AM +0100, Lucas Tanure wrote:
> AMD SPI controller has 70 bytes for its FIFO and it has an
> automatic way of controlling it`s internal CS, which can
> only be activated during the time that the FIFO is being
> transfered.

> SPI_MASTER_HALF_DUPLEX here means that it can only read
> RX bytes after TX bytes were written, and RX+TX must be
> less than 70. If you write 4 bytes the first byte of read
> is in position 5 of the FIFO.
> 
> All of that means that for devices that require an address
> for reads and writes, the 2 transfers must be put in the same
> FIFO so the CS can be hold for address and data, otherwise
> the data would lose it`s meaning.

This commit message is confusing, it's hard to tell what the refactoring
is.  It also doesn't seem at all joined up with the rest of the series
or the contents of the patch.  The rest of this series adds a new
interface which says that the controller is only capable of doing a
single transfer which means that the core should ensure that the
controller never sees a message with more than one transfer in it but
this patch appears to be attempting to parse multiple transfers and pack
them together due to controller limitations in some way.  That is never
going to do anything useful, if anything is paying attention to the flag
the controller will never see messages like that.  Indeed code that pays
attention to the flag and needs this is likely to refuse to work with
the hardware at all since the device is half duplex so anything that
needs messages mixing reads and writes just won't work at all.

It also looks like the code is adding support for a new revision of the
hardware which isn't mentioned anywhere in the commit message at all and
really should be, it should most likely be a separate commit.

As far as I can tell what you're trying to do here is better expressed
as saying that the controller has a limit on the maximum message size
then having the transfer_one_message() pack those down into whatever the
controller needs so it can send them without bouncing chip select.  The
new flag is not needed for this device and indeed looks like it will
actively work against what's needed to make the controller useful in
these applications.

Please also use normal apostrophies.

> +	amd_spi_set_tx_count(amd_spi, tx1_len + tx2_len);
> +	ret = amd_spi_execute_opcode(amd_spi);
> +
> +	return ret ? ret : tx1_len + 1 + tx2_len;

Please write normal conditional statements so people can read the code
more easily.

> +static const struct amd_spi_devtype_data spi_v1 = {
> +       .exec_op        = amd_spi_execute_opcode_v1,
> +       .set_op         = amd_spi_set_opcode_v1,
> +};
> +
> +static const struct amd_spi_devtype_data spi_v2 = {
> +       .version        = 1,

v2 sets the version to 1 and v1 doesn't set the version at all and the
only use of the version field AFAICT is that we should try to call
amd_spi_clear_chip() after a transfer.  This isn't entirely clear.  If
it's just a flag for the need to do that clear make it an explicit flag
for that.

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

Re: [PATCH 8/9] spi: amd: Refactor to overcome 70 bytes per CS limitation

[Mark Brown]

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

On Tue, Aug 24, 2021 at 06:16:19PM +0100, Mark Brown wrote:

> It also looks like the code is adding support for a new revision of the
> hardware which isn't mentioned anywhere in the commit message at all and
> really should be, it should most likely be a separate commit.

Sorry, I managed to move on to the next message while writing the reply
without noticing - this is actually a separate commit so that bit is
fine (modulo the confusion with the version field).

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

[PATCH 9/9] spi: amd: Add support for latest platform

[Lucas Tanure]

Add support for AMDI0062 controllers

Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi-amd.c | 127 +++++++++++++++++++++++++++++++++---------
 1 file changed, 100 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index b6308733265e..cd0b38ead150 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -2,9 +2,10 @@
 //
 // AMD SPI controller driver
 //
-// Copyright (c) 2020, Advanced Micro Devices, Inc.
+// Copyright (c) 2020-2021, Advanced Micro Devices, Inc.
 //
 // Authors: Sanjay R Mehta <sanju.mehta@amd.com>
+//          Nehal Bakulchandra Shah <nehal-bakulchandra.shah@amd.com>
 //          Lucas Tanure <tanureal@opensource.cirrus.com>
 
 #include <linux/acpi.h>
@@ -15,32 +16,46 @@
 #include <linux/spi/spi.h>
 #include <linux/iopoll.h>
 
-#define AMD_SPI_CTRL0_REG	0x00
-#define AMD_SPI_EXEC_CMD	BIT(16)
-#define AMD_SPI_FIFO_CLEAR	BIT(20)
-#define AMD_SPI_BUSY		BIT(31)
+#define AMD_SPI_CTRL0_REG		0x00
+#define AMD_SPI_EXEC_CMD		BIT(16)
+#define AMD_SPI_FIFO_CLEAR		BIT(20)
+#define AMD_SPI_BUSY			BIT(31)
+#define AMD_SPI_ENABLE_REG		0x20
 
-#define AMD_SPI_OPCODE_MASK	0xFF
+#define AMD_SPI_DUMMY_CYCL_REG		0x32
+#define AMD_SPI_OPCODE_REG		0x45
+#define AMD_SPI_CMD_TRIGGER_REG		0x47
+#define AMD_SPI_TRIGGER_CMD		BIT(7)
+#define AMD_SPI_OPCODE_MASK		0xFF
 
-#define AMD_SPI_ALT_CS_REG	0x1D
-#define AMD_SPI_ALT_CS_MASK	0x3
+#define AMD_SPI_ALT_CS_REG		0x1D
+#define AMD_SPI_ALT_CS_MASK		GENMASK(1, 0)
 
-#define AMD_SPI_FIFO_BASE	0x80
-#define AMD_SPI_TX_COUNT_REG	0x48
-#define AMD_SPI_RX_COUNT_REG	0x4B
-#define AMD_SPI_STATUS_REG	0x4C
+#define AMD_SPI_FIFO_BASE		0x80
+#define AMD_SPI_TX_COUNT_REG		0x48
+#define AMD_SPI_RX_COUNT_REG		0x4B
+#define AMD_SPI_STATUS_REG		0x4C
 
-#define AMD_SPI_FIFO_SIZE	70
-#define AMD_SPI_MEM_SIZE	200
+#define AMD_SPI_FIFO_SIZE		70
+#define AMD_SPI_MEM_SIZE		200
 
 /* M_CMD OP codes for SPI */
-#define AMD_SPI_XFER_TX		1
-#define AMD_SPI_XFER_RX		2
+#define AMD_SPI_XFER_TX			1
+#define AMD_SPI_XFER_RX			2
 
 struct amd_spi {
 	void __iomem *io_remap_addr;
 	unsigned long io_base_addr;
 	u32 rom_addr;
+	const struct amd_spi_devtype_data *devtype_data;
+	struct spi_device *spi_dev;
+	struct spi_master *master;
+};
+
+struct amd_spi_devtype_data {
+	u8 version;
+	int (*exec_op)(struct amd_spi *amd_spi);
+	void (*set_op)(struct amd_spi *amd_spi, u8 cmd_opcode);
 };
 
 static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
@@ -84,16 +99,26 @@ static void amd_spi_select_chip(struct amd_spi *amd_spi, u8 cs)
 	amd_spi_setclear_reg8(amd_spi, AMD_SPI_ALT_CS_REG, cs, AMD_SPI_ALT_CS_MASK);
 }
 
+static inline void amd_spi_clear_chip(struct amd_spi *amd_spi, u8 chip_select)
+{
+	amd_spi_writereg8(amd_spi, AMD_SPI_ALT_CS_REG, chip_select & ~AMD_SPI_ALT_CS_MASK);
+}
+
 static void amd_spi_clear_fifo_ptr(struct amd_spi *amd_spi)
 {
 	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_FIFO_CLEAR, AMD_SPI_FIFO_CLEAR);
 }
 
-static void amd_spi_set_opcode(struct amd_spi *amd_spi, u8 cmd_opcode)
+static void amd_spi_set_opcode_v1(struct amd_spi *amd_spi, u8 cmd_opcode)
 {
 	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, cmd_opcode, AMD_SPI_OPCODE_MASK);
 }
 
+static void amd_spi_set_opcode_v2(struct amd_spi *amd_spi, u8 cmd_opcode)
+{
+	amd_spi_writereg8(amd_spi, AMD_SPI_OPCODE_REG, cmd_opcode);
+}
+
 static inline void amd_spi_set_rx_count(struct amd_spi *amd_spi, u8 rx_count)
 {
 	amd_spi_setclear_reg8(amd_spi, AMD_SPI_RX_COUNT_REG, rx_count, 0xff);
@@ -104,7 +129,7 @@ static inline void amd_spi_set_tx_count(struct amd_spi *amd_spi, u8 tx_count)
 	amd_spi_setclear_reg8(amd_spi, AMD_SPI_TX_COUNT_REG, tx_count, 0xff);
 }
 
-static int amd_spi_busy_wait(struct amd_spi *amd_spi)
+static int amd_spi_busy_wait_v1(struct amd_spi *amd_spi)
 {
 	u32 val;
 
@@ -112,17 +137,39 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi)
 				 !(val & AMD_SPI_BUSY), 10, 100000);
 }
 
-static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
+static int amd_spi_execute_opcode_v1(struct amd_spi *amd_spi)
 {
-	int ret = amd_spi_busy_wait(amd_spi);
+	int ret;
 
+	ret = amd_spi_busy_wait_v1(amd_spi);
 	if (ret)
 		return ret;
 
-	/* Set ExecuteOpCode bit in the CTRL0 register */
 	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD, AMD_SPI_EXEC_CMD);
 
-	return amd_spi_busy_wait(amd_spi);
+	return amd_spi_busy_wait_v1(amd_spi);
+}
+
+static int amd_spi_busy_wait_v2(struct amd_spi *amd_spi)
+{
+	u32 val;
+
+	return readl_poll_timeout(amd_spi->io_remap_addr + AMD_SPI_STATUS_REG, val,
+				 !(val & AMD_SPI_BUSY), 10, 1000);
+}
+
+static int amd_spi_execute_opcode_v2(struct amd_spi *amd_spi)
+{
+	int ret;
+
+	ret = amd_spi_busy_wait_v2(amd_spi);
+	if (ret)
+		return ret;
+
+	amd_spi_setclear_reg8(amd_spi, AMD_SPI_CMD_TRIGGER_REG, AMD_SPI_TRIGGER_CMD,
+			      AMD_SPI_TRIGGER_CMD);
+
+	return amd_spi_busy_wait_v2(amd_spi);
 }
 
 static int amd_spi_master_setup(struct spi_device *spi)
@@ -138,6 +185,7 @@ static int amd_spi_double_write(struct spi_master *mst, u8 *tx1_buf, u8 tx1_len,
 				u8 tx2_len)
 {
 	struct amd_spi *amd_spi = spi_master_get_devdata(mst);
+	const struct amd_spi_devtype_data *priv = amd_spi->devtype_data;
 	int i, ret;
 
 	if (tx1_len + tx2_len > AMD_SPI_FIFO_SIZE)
@@ -146,7 +194,7 @@ static int amd_spi_double_write(struct spi_master *mst, u8 *tx1_buf, u8 tx1_len,
 	amd_spi_clear_fifo_ptr(amd_spi);
 	amd_spi_set_rx_count(amd_spi, 0);
 
-	amd_spi_set_opcode(amd_spi, tx1_buf[0]);
+	priv->set_op(amd_spi, tx1_buf[0]);
 	tx1_len--;
 	tx1_buf++;
 
@@ -157,7 +205,7 @@ static int amd_spi_double_write(struct spi_master *mst, u8 *tx1_buf, u8 tx1_len,
 		amd_spi_writereg8(amd_spi, (u8)(AMD_SPI_FIFO_BASE + tx1_len + i), tx2_buf[i]);
 
 	amd_spi_set_tx_count(amd_spi, tx1_len + tx2_len);
-	ret = amd_spi_execute_opcode(amd_spi);
+	ret = priv->exec_op(amd_spi);
 
 	return ret ? ret : tx1_len + 1 + tx2_len;
 }
@@ -165,6 +213,7 @@ static int amd_spi_double_write(struct spi_master *mst, u8 *tx1_buf, u8 tx1_len,
 static int amd_spi_write_read(struct spi_master *mst, u8 *tx_buf, u8 tx_len, u8 *rx_buf, u8 rx_len)
 {
 	struct amd_spi *amd_spi = spi_master_get_devdata(mst);
+	const struct amd_spi_devtype_data *priv = amd_spi->devtype_data;
 	int i, ret;
 
 	if (tx_len + rx_len > AMD_SPI_FIFO_SIZE)
@@ -174,7 +223,7 @@ static int amd_spi_write_read(struct spi_master *mst, u8 *tx_buf, u8 tx_len, u8
 
 	if (tx_buf) {
 		/* Take the first byte to be written and set as opcode */
-		amd_spi_set_opcode(amd_spi, tx_buf[0]);
+		priv->set_op(amd_spi, tx_buf[0]);
 		/* Set TX count as the number of bytes to be written less one (opcode byte) */
 		tx_len--;
 		tx_buf++;
@@ -191,7 +240,7 @@ static int amd_spi_write_read(struct spi_master *mst, u8 *tx_buf, u8 tx_len, u8
 	amd_spi_set_rx_count(amd_spi, rx_buf ? rx_len : 0);
 
 	/* Trigger the transfer by executing the opcode */
-	ret = amd_spi_execute_opcode(amd_spi);
+	ret = priv->exec_op(amd_spi);
 	if (ret)
 		return ret;
 
@@ -274,6 +323,9 @@ static int amd_spi_master_transfer(struct spi_master *mst, struct spi_message *m
 	msg->status = ret;
 	spi_finalize_current_message(mst);
 
+	if (amd_spi->devtype_data->version)
+		amd_spi_clear_chip(amd_spi, msg->spi->chip_select);
+
 	return ret;
 }
 
@@ -305,6 +357,12 @@ static int amd_spi_probe(struct platform_device *pdev)
 	}
 	dev_dbg(dev, "io_remap_address: %p\n", amd_spi->io_remap_addr);
 
+	amd_spi->devtype_data = device_get_match_data(dev);
+	if (!amd_spi->devtype_data) {
+		err = -ENODEV;
+		goto err_free_master;
+	}
+
 	/* Initialize the spi_master fields */
 	master->bus_num = 0;
 	master->num_chipselect = 4;
@@ -329,9 +387,23 @@ static int amd_spi_probe(struct platform_device *pdev)
 	return err;
 }
 
+static const struct amd_spi_devtype_data spi_v1 = {
+	.exec_op	= amd_spi_execute_opcode_v1,
+	.set_op		= amd_spi_set_opcode_v1,
+};
+
+static const struct amd_spi_devtype_data spi_v2 = {
+	.version	= 1,
+	.exec_op	= amd_spi_execute_opcode_v2,
+	.set_op		= amd_spi_set_opcode_v2,
+};
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id spi_acpi_match[] = {
-	{ "AMDI0061", 0 },
+	{ "AMDI0061",
+	.driver_data = (kernel_ulong_t)&spi_v1 },
+	{ "AMDI0062",
+	.driver_data = (kernel_ulong_t)&spi_v2 },
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, spi_acpi_match);
@@ -349,4 +421,5 @@ module_platform_driver(amd_spi_driver);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Sanjay Mehta <sanju.mehta@amd.com>");
+MODULE_AUTHOR("Nehal Bakulchandra Shah <nehal-bakulchandra.shah@amd.com>");
 MODULE_DESCRIPTION("AMD SPI Master Controller Driver");
-- 
2.33.0

Re: [PATCH 9/9] spi: amd: Add support for latest platform

[kernel test robot]

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

Hi Lucas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on regmap/for-next driver-core/driver-core-testing v5.14-rc7 next-20210824]
[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]

url:    https://github.com/0day-ci/linux/commits/Lucas-Tanure/Improve-support-for-AMD-SPI-controllers/20210824-184257
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: riscv-randconfig-c006-20210824 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d26000e4cc2bc65e207a84fa26cb6e374d60aa12)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/ac75baf94633bf8c290e7fc90ecb84957cb602a9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Lucas-Tanure/Improve-support-for-AMD-SPI-controllers/20210824-184257
        git checkout ac75baf94633bf8c290e7fc90ecb84957cb602a9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/spi/spi-amd.c:390:42: warning: unused variable 'spi_v1' [-Wunused-const-variable]
   static const struct amd_spi_devtype_data spi_v1 = {
                                            ^
>> drivers/spi/spi-amd.c:395:42: warning: unused variable 'spi_v2' [-Wunused-const-variable]
   static const struct amd_spi_devtype_data spi_v2 = {
                                            ^
   2 warnings generated.


vim +/spi_v1 +390 drivers/spi/spi-amd.c

   389	
 > 390	static const struct amd_spi_devtype_data spi_v1 = {
   391		.exec_op	= amd_spi_execute_opcode_v1,
   392		.set_op		= amd_spi_set_opcode_v1,
   393	};
   394	
 > 395	static const struct amd_spi_devtype_data spi_v2 = {
   396		.version	= 1,
   397		.exec_op	= amd_spi_execute_opcode_v2,
   398		.set_op		= amd_spi_set_opcode_v2,
   399	};
   400	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38987 bytes --]

Re: [PATCH 9/9] spi: amd: Add support for latest platform

[Mark Brown]

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

On Tue, Aug 24, 2021 at 11:40:41AM +0100, Lucas Tanure wrote:
> Add support for AMDI0062 controllers
> 
> Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>

Did Nehal write this commit?  If so it there should be a From: saying so
- git commit --amend --author will fix things up if you're using git.

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