[PATCH v2 00/10] Improve support for AMD SPI controllers

[PATCH v2 00/10] Improve support for AMD SPI controllers

[Lucas Tanure]

Add support for AMDI0062 and correctly fill the FIFO buffer with
the whole message.

With a message of AMD_SPI_FIFO_SIZE bytes or less, copying all
transfers to the FIFO guarantees that the message is sent over
one CS. Because the controller has an automatic CS that is only
activated during the transmission of the FIFO.

And the controller is half-duplex in that it cannot read data
while it is sending data. But the FIFO is full-duplex, the writes
and reads must be queued and executed together, where it can only
have one write and one read per FIFO, and the writing part is
executed first. Therefore transfers can be put together in the
FIFO unless there is a write after read, which will need to be
executed in another CS.

v2 changes:
Replace flag SPI_CONTROLLER_CS_PER_TRANSFER by checking
spi_max_message_size
Add flag for controllers that can't TX after RX in the same
message
SPI controller now expects a message that always can fit in
FIFO
Add a new patch for configuring the SPI speed


Lucas Tanure (9):
  regmap: spi: Set regmap max raw r/w from max_transfer_size
  regmap: spi: Check raw_[read|write] against max message size
  spi: Add flag for no TX after a RX in the same Chip Select
  spi: amd: Refactor code to use less spi_master_get_devdata
  spi: amd: Refactor amd_spi_busy_wait
  spi: amd: Remove unneeded variable
  spi: amd: Check for idle bus before execute opcode
  spi: amd: Fill FIFO buffer with the whole message
  spi: amd: Configure the SPI speed

Nehal Bakulchandra Shah (1):
  spi: amd: Add support for latest platform

 drivers/base/regmap/regmap-spi.c |  40 ++-
 drivers/base/regmap/regmap.c     |  15 +
 drivers/spi/spi-amd.c            | 498 ++++++++++++++++++++++---------
 drivers/spi/spi.c                |  11 +
 include/linux/regmap.h           |   3 +
 include/linux/spi/spi.h          |   1 +
 6 files changed, 421 insertions(+), 147 deletions(-)

--
2.33.0

[PATCH 01/10] 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 | 36 ++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
index c1894e93c378..0e6552e57ecf 100644
--- a/drivers/base/regmap/regmap-spi.c
+++ b/drivers/base/regmap/regmap-spi.c
@@ -109,13 +109,37 @@ 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;
+	size_t max_size = spi_max_transfer_size(spi);
+
+	if (max_size != SIZE_MAX) {
+		bus = kmemdup(&regmap_spi, sizeof(*bus), GFP_KERNEL);
+		if (!bus)
+			return ERR_PTR(-ENOMEM);
+		bus->free_on_exit = true;
+		bus->max_raw_read = max_size;
+		bus->max_raw_write = max_size;
+		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 +148,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

[PATCH 02/10] regmap: spi: Check raw_[read|write] against max message size

[Lucas Tanure]

regmap-spi will split data and address between two transfers
in the same message, so max_[read|write] must include space
for the address and padding

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

diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
index 0e6552e57ecf..1434c502e340 100644
--- a/drivers/base/regmap/regmap-spi.c
+++ b/drivers/base/regmap/regmap-spi.c
@@ -123,6 +123,10 @@ static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi,
 		bus->free_on_exit = true;
 		bus->max_raw_read = max_size;
 		bus->max_raw_write = max_size;
+
+		if (spi_max_message_size(spi) != SIZE_MAX)
+			bus->max_combined_rw = spi_max_message_size(spi);
+
 		return bus;
 	}
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index fe3e38dd5324..1cd936e097b0 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -718,6 +718,7 @@ struct regmap *__regmap_init(struct device *dev,
 	struct regmap *map;
 	int ret = -EINVAL;
 	enum regmap_endian reg_endian, val_endian;
+	size_t reg_pad_size;
 	int i, j;
 
 	if (!config)
@@ -815,6 +816,20 @@ 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->max_combined_rw) {
+			reg_pad_size = map->format.reg_bytes + map->format.pad_bytes;
+
+			if (map->max_raw_read + reg_pad_size > bus->max_combined_rw)
+				map->max_raw_read -= reg_pad_size;
+			if (map->max_raw_write + reg_pad_size > bus->max_combined_rw)
+				map->max_raw_write -= reg_pad_size;
+
+			if (map->max_raw_read  < map->format.buf_size ||
+			    map->max_raw_write < map->format.buf_size) {
+				ret = -EINVAL;
+				goto err_hwlock;
+			}
+		}
 	}
 	map->dev = dev;
 	map->bus = bus;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f5f08dd0a116..53620c70ae5e 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -504,6 +504,8 @@ 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
+ * @max_combined_rw: Max size for raw_read + raw_write, when they are issued
+ *                   together as part of the same message
  */
 struct regmap_bus {
 	bool fast_io;
@@ -521,6 +523,7 @@ struct regmap_bus {
 	enum regmap_endian val_format_endian_default;
 	size_t max_raw_read;
 	size_t max_raw_write;
+	size_t max_combined_rw;
 	bool free_on_exit;
 };
 
-- 
2.33.0

Re: [PATCH 02/10] regmap: spi: Check raw_[read|write] against max message size

[Charles Keepax]

On Wed, Sep 08, 2021 at 12:34:43PM +0100, Lucas Tanure wrote:
> regmap-spi will split data and address between two transfers
> in the same message, so max_[read|write] must include space
> for the address and padding
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---
>  drivers/base/regmap/regmap-spi.c |  4 ++++
>  drivers/base/regmap/regmap.c     | 15 +++++++++++++++
>  include/linux/regmap.h           |  3 +++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
> index 0e6552e57ecf..1434c502e340 100644
> --- a/drivers/base/regmap/regmap-spi.c
> +++ b/drivers/base/regmap/regmap-spi.c
> @@ -123,6 +123,10 @@ static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi,
>  		bus->free_on_exit = true;
>  		bus->max_raw_read = max_size;
>  		bus->max_raw_write = max_size;
> +
> +		if (spi_max_message_size(spi) != SIZE_MAX)
> +			bus->max_combined_rw = spi_max_message_size(spi);

I am not sure max_combined_rw is the best name here, it makes
sense in a SPI context where reads are a write followed by a
read. But does it really make sense for all buses? Like an MMIO
this no longer seems a very meaningful name.

Perhaps max_transaction? But I am often not the best at thinking
of names myself.

> +
>  		return bus;
>  	}
>  
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index fe3e38dd5324..1cd936e097b0 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -718,6 +718,7 @@ struct regmap *__regmap_init(struct device *dev,
>  	struct regmap *map;
>  	int ret = -EINVAL;
>  	enum regmap_endian reg_endian, val_endian;
> +	size_t reg_pad_size;
>  	int i, j;
>  
>  	if (!config)
> @@ -815,6 +816,20 @@ 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->max_combined_rw) {
> +			reg_pad_size = map->format.reg_bytes + map->format.pad_bytes;

Maybe reg_overhead_size or something? This line uses pad to both
mean the actual padding in pad_bytes and to mean address +
padding in reg_pad_size.

Thanks,
Charles

Re: [PATCH 02/10] regmap: spi: Check raw_[read|write] against max message size

[Charles Keepax]

On Wed, Sep 08, 2021 at 01:09:29PM +0000, Charles Keepax wrote:
> On Wed, Sep 08, 2021 at 12:34:43PM +0100, Lucas Tanure wrote:
> > regmap-spi will split data and address between two transfers
> > in the same message, so max_[read|write] must include space
> > for the address and padding
> > 
> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> > ---
> >  drivers/base/regmap/regmap-spi.c |  4 ++++
> >  drivers/base/regmap/regmap.c     | 15 +++++++++++++++
> >  include/linux/regmap.h           |  3 +++
> >  3 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
> > index 0e6552e57ecf..1434c502e340 100644
> > --- a/drivers/base/regmap/regmap-spi.c
> > +++ b/drivers/base/regmap/regmap-spi.c
> > @@ -123,6 +123,10 @@ static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi,
> >  		bus->free_on_exit = true;
> >  		bus->max_raw_read = max_size;
> >  		bus->max_raw_write = max_size;
> > +
> > +		if (spi_max_message_size(spi) != SIZE_MAX)
> > +			bus->max_combined_rw = spi_max_message_size(spi);
> 
> I am not sure max_combined_rw is the best name here, it makes
> sense in a SPI context where reads are a write followed by a
> read. But does it really make sense for all buses? Like an MMIO
> this no longer seems a very meaningful name.
> 
> Perhaps max_transaction? But I am often not the best at thinking
> of names myself.
> 

Although thinking about this more are we sure this wouldn't just
be better as a flag to include the address in the max_raw_read/write?
I am not sure what extra use-cases the extra max_combined_rw
opens up and it feels like the field is doing two things, 1)
saying that the address needs to be included in the max size and
2) specifying a new max size.

Thanks,
Charles

[PATCH 03/10] spi: Add flag for no TX after a RX in the same Chip Select

[Lucas Tanure]

Some controllers can't write to the bus after a read without
releasing the chip select, so add flag and a check in spi core

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
---
 drivers/spi/spi.c       | 11 +++++++++++
 include/linux/spi/spi.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 65d14af9c015..1dbc8b6f1398 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3724,6 +3724,17 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 			return -EINVAL;
 	}
 
+	if (ctlr->flags & SPI_CONTROLLER_NO_TX_RX_CS) {
+		bool read = false;
+
+		list_for_each_entry(xfer, &message->transfers, transfer_list) {
+			if (read && xfer->tx_buf)
+				return -EINVAL;
+			if (xfer->rx_buf && !xfer->cs_change)
+				read = true;
+		}
+	}
+
 	message->status = -EINPROGRESS;
 
 	return 0;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 8371bca13729..916b982dc2a1 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_NO_TX_RX_CS	BIT(6)	/* can't write after a read in the same CS */
 
 	/* flag indicating if the allocation of this struct is devres-managed */
 	bool			devm_allocated;
-- 
2.33.0

Re: [PATCH 03/10] spi: Add flag for no TX after a RX in the same Chip Select

[Mark Brown]

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

On Wed, Sep 08, 2021 at 12:34:44PM +0100, Lucas Tanure wrote:
> Some controllers can't write to the bus after a read without
> releasing the chip select, so add flag and a check in spi core

Nothing you've added ever reads this flag and I'm not sure what anything
would be able to constructively do with it so why add the flag?  I don't
understand what the use case is.

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

Re: [PATCH 03/10] spi: Add flag for no TX after a RX in the same Chip Select

[Lucas tanure]

On 9/8/21 1:37 PM, Mark Brown wrote:
> On Wed, Sep 08, 2021 at 12:34:44PM +0100, Lucas Tanure wrote:
>> Some controllers can't write to the bus after a read without
>> releasing the chip select, so add flag and a check in spi core
> 
> Nothing you've added ever reads this flag and I'm not sure what anything
> would be able to constructively do with it so why add the flag?  I don't
> understand what the use case is.
> 
__spi_validate checks this flag and makes sure the message can be 
received by the controller.
__spi_validate can't fix the message, so it only rejects the message.

Re: [PATCH 03/10] spi: Add flag for no TX after a RX in the same Chip Select

[Mark Brown]

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

On Thu, Sep 09, 2021 at 11:51:21AM +0100, Lucas tanure wrote:
> On 9/8/21 1:37 PM, Mark Brown wrote:
> > On Wed, Sep 08, 2021 at 12:34:44PM +0100, Lucas Tanure wrote:
> > > Some controllers can't write to the bus after a read without
> > > releasing the chip select, so add flag and a check in spi core

> > Nothing you've added ever reads this flag and I'm not sure what anything
> > would be able to constructively do with it so why add the flag?  I don't
> > understand what the use case is.

> __spi_validate checks this flag and makes sure the message can be received
> by the controller.
> __spi_validate can't fix the message, so it only rejects the message.

Given that this is hardware that can't possibly work how useful is that
validation?  It's a fairly unusual thing for devices to do in the first
place, only applies if using the native chip select (which your patch
doesn't check for) and I am not sure that this is a general enough
pattern in controllers to have generic support for.  I suspect that a
lot of controllers with similar restrictions will be even more limited
than this, for example only supporting one or two transfers with limits
on the data, so it's not clear to me how useful this capability would
be.

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

[PATCH 04/10] 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 05/10] spi: amd: Refactor amd_spi_busy_wait

[Lucas Tanure]

Use amd_spi_readreg32 to read 32 bits registers

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

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index f23467cf6acd..f2dd8d432aff 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -103,21 +103,15 @@ 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) {
+	while (amd_spi_readreg32(amd_spi, AMD_SPI_CTRL0_REG) & AMD_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;
 	}
 
 	return 0;
-- 
2.33.0

[PATCH 06/10] spi: amd: Remove unneeded 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 f2dd8d432aff..97838b57871c 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -38,7 +38,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)
@@ -77,10 +76,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)
@@ -201,8 +199,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 07/10] spi: amd: Check for idle bus before execute opcode

[Lucas Tanure]

Check if the bus is not in use before starting the transfer

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

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 97838b57871c..99b2b0ccff08 100644
--- a/drivers/spi/spi-amd.c
+++ b/drivers/spi/spi-amd.c
@@ -115,11 +115,18 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi)
 	return 0;
 }
 
-static void amd_spi_execute_opcode(struct amd_spi *amd_spi)
+static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
 {
+	int ret;
+
+	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 0;
 }
 
 static int amd_spi_master_setup(struct spi_device *spi)
-- 
2.33.0

[PATCH 08/10] spi: amd: Fill FIFO buffer with the whole message

[Lucas Tanure]

The controller is half-duplex, in that it cannot
read data while it is sending data. But the FIFO
is full-duplex, the writes and reads must be
queued and executed together, and the read data
will be offset in the FIFO by the length of the
initial write data (as it would in a full-duplex
SPI).

And the controller has an automatic CS which can
only be activated during the transmission of the
FIFO, which can make read|write data lose meaning
as the CS will be toggle after the required
read|write address.
To avoid that set the max transfer and message
size as AMD_SPI_FIFO_SIZE ensuring that incoming
messages always fit inside a FIFO buffer

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

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 99b2b0ccff08..0face11740ea 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>
@@ -28,6 +29,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 */
@@ -38,6 +40,13 @@ struct amd_spi {
 	void __iomem *io_remap_addr;
 	unsigned long io_base_addr;
 	u32 rom_addr;
+	struct list_head rbuf_head;
+};
+
+struct amd_spi_read_buffer {
+	struct list_head node;
+	u8 *buf;
+	u8 len;
 };
 
 static inline u8 amd_spi_readreg8(struct amd_spi *amd_spi, int idx)
@@ -138,83 +147,127 @@ 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 void amd_spi_clear_list(struct amd_spi *amd_spi)
 {
-	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_read_buffer *rbuf, *tmp;
 
-			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);
-		}
+	list_for_each_entry_safe(rbuf, tmp, &amd_spi->rbuf_head, node) {
+		list_del(&rbuf->node);
+		kfree(rbuf);
 	}
+}
 
-	/* Update statistics */
-	message->actual_length = tx_len + rx_len + 1;
-	/* complete the transaction */
-	message->status = 0;
-	spi_finalize_current_message(master);
+static int amd_spi_transfer(struct amd_spi *amd_spi, u8 opcode, u8 tx_len, u8 rx_len, u8 fifo_pos)
+{
+	struct amd_spi_read_buffer *rbuf;
+	struct list_head *p;
+	int ret, i;
+
+	amd_spi_set_opcode(amd_spi, opcode);
+	amd_spi_set_tx_count(amd_spi, tx_len);
+	amd_spi_set_rx_count(amd_spi, rx_len);
+
+	ret = amd_spi_execute_opcode(amd_spi);
+	if (ret)
+		return ret;
+
+	if (!list_empty(&amd_spi->rbuf_head)) {
+		ret = amd_spi_busy_wait(amd_spi);
+		if (ret)
+			return ret;
+		list_for_each(p, &amd_spi->rbuf_head) {
+			rbuf = list_entry(p, struct amd_spi_read_buffer, node);
+			for (i = 0; i < rbuf->len; i++)
+				rbuf->buf[i] = amd_spi_readreg8(amd_spi, fifo_pos++);
+		}
+		amd_spi_clear_list(amd_spi);
+	}
 
 	return 0;
 }
 
-static int amd_spi_master_transfer(struct spi_master *master,
-				   struct spi_message *msg)
+/* amd_spi_master_transfer expects a spi_message with no more than AMD_SPI_FIFO_SIZE and no TX after
+ * a RX in the same CS
+ * The CS can not be held between two amd_spi_execute_opcode so fill the FIFO with all transfers
+ * until the first RX transfer
+ */
+static int amd_spi_transfer_one_message(struct spi_controller *ctrl, struct spi_message *msg)
 {
-	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(ctrl);
+	u8 tx_len = 0, rx_len = 0, opcode = 0, fifo_pos = AMD_SPI_FIFO_BASE;
+	struct amd_spi_read_buffer *rbuf;
+	struct spi_transfer *xfer;
+	u8 *tx_buf;
+	int ret, i;
+
+	amd_spi_select_chip(amd_spi, msg->spi->chip_select);
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (xfer->tx_buf) {
+			tx_buf = (u8 *)xfer->tx_buf;
+			if (!tx_len) {
+				opcode = tx_buf[0];
+				xfer->len--;
+				tx_buf++;
+			}
+			tx_len += xfer->len;
+			for (i = 0; i < xfer->len; i++)
+				amd_spi_writereg8(amd_spi, fifo_pos++, tx_buf[i]);
+		}
 
-	amd_spi_select_chip(amd_spi, spi->chip_select);
+		if (xfer->rx_buf) {
+			rx_len += xfer->len;
+			rbuf = kmalloc(sizeof(*rbuf), GFP_KERNEL);
+			if (!rbuf) {
+				ret = -ENOMEM;
+				goto complete;
+			}
 
-	/*
-	 * Extract spi_transfers from the spi message and
-	 * program the controller.
-	 */
-	amd_spi_fifo_xfer(amd_spi, master, msg);
+			rbuf->buf = (u8 *)xfer->rx_buf;
+			rbuf->len = xfer->len;
+			list_add(&rbuf->node, &amd_spi->rbuf_head);
+		}
 
-	return 0;
+		if (xfer->cs_change) {
+			ret = amd_spi_transfer(amd_spi, opcode, tx_len, rx_len, fifo_pos);
+			if (ret)
+				goto complete;
+
+			msg->actual_length += rx_len;
+			if (tx_len)
+				msg->actual_length += tx_len + 1;
+
+			fifo_pos = AMD_SPI_FIFO_BASE;
+			opcode = 0;
+			tx_len = 0;
+			rx_len = 0;
+		}
+	}
+
+	if (tx_len || rx_len) {
+		ret = amd_spi_transfer(amd_spi, opcode, tx_len, rx_len, fifo_pos);
+		if (ret)
+			goto complete;
+
+		msg->actual_length += rx_len;
+		if (tx_len)
+			msg->actual_length += tx_len + 1;
+	}
+	ret = 0;
+
+complete:
+	if (!list_empty(&amd_spi->rbuf_head))
+		amd_spi_clear_list(amd_spi);
+	/* complete the transaction */
+	msg->status = ret;
+	spi_finalize_current_message(ctrl);
+
+	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)
@@ -244,9 +297,13 @@ 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_CONTROLLER_HALF_DUPLEX | SPI_CONTROLLER_NO_TX_RX_CS;
 	master->setup = amd_spi_master_setup;
-	master->transfer_one_message = amd_spi_master_transfer;
+	master->max_transfer_size = amd_spi_max_transfer_size;
+	master->max_message_size = amd_spi_max_transfer_size;
+	master->transfer_one_message = amd_spi_transfer_one_message;
+
+	INIT_LIST_HEAD(&amd_spi->rbuf_head);
 
 	/* Register the controller with SPI framework */
 	err = devm_spi_register_master(dev, master);
-- 
2.33.0

Re: [PATCH 08/10] spi: amd: Fill FIFO buffer with the whole message

[Charles Keepax]

On Wed, Sep 08, 2021 at 12:34:49PM +0100, Lucas Tanure wrote:
> The controller is half-duplex, in that it cannot
> read data while it is sending data. But the FIFO
> is full-duplex, the writes and reads must be
> queued and executed together, and the read data
> will be offset in the FIFO by the length of the
> initial write data (as it would in a full-duplex
> SPI).
> 
> And the controller has an automatic CS which can
> only be activated during the transmission of the
> FIFO, which can make read|write data lose meaning
> as the CS will be toggle after the required
> read|write address.
> To avoid that set the max transfer and message
> size as AMD_SPI_FIFO_SIZE ensuring that incoming
> messages always fit inside a FIFO buffer
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> ---

Its only really this change I think that depends relates to the regmap/SPI
changes, it might be worth doing a separate series with the trivial
improvements to the SPI driver. As that allow that to get merged
quickly, and makes the series more focused and easy to review on
the more complex part of supporting the SPI hardwares weird CS/message
length quirk.

Thanks,
Charles

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

[Lucas Tanure]

From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>

Add support for AMDI0062 controller

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 | 128 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 104 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
index 0face11740ea..788a5c42d811 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>
@@ -14,33 +15,48 @@
 #include <linux/delay.h>
 #include <linux/spi/spi.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;
 	struct list_head rbuf_head;
+	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);
+	int (*busy_wait)(struct amd_spi *amd_spi);
 };
 
 struct amd_spi_read_buffer {
@@ -90,16 +106,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);
@@ -110,7 +136,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)
 {
 	int timeout = 100000;
 
@@ -124,11 +150,11 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi)
 	return 0;
 }
 
-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;
 
-	ret = amd_spi_busy_wait(amd_spi);
+	ret = amd_spi_busy_wait_v1(amd_spi);
 	if (ret)
 		return ret;
 
@@ -138,6 +164,33 @@ static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
 	return 0;
 }
 
+static int amd_spi_busy_wait_v2(struct amd_spi *amd_spi)
+{
+	int timeout = 100000;
+
+	while (amd_spi_readreg32(amd_spi, AMD_SPI_STATUS_REG) & AMD_SPI_BUSY) {
+		usleep_range(10, 20);
+		if (timeout-- < 0)
+			return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+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 0;
+}
+
 static int amd_spi_master_setup(struct spi_device *spi)
 {
 	struct amd_spi *amd_spi = spi_master_get_devdata(spi->master);
@@ -159,20 +212,21 @@ static void amd_spi_clear_list(struct amd_spi *amd_spi)
 
 static int amd_spi_transfer(struct amd_spi *amd_spi, u8 opcode, u8 tx_len, u8 rx_len, u8 fifo_pos)
 {
+	const struct amd_spi_devtype_data *priv = amd_spi->devtype_data;
 	struct amd_spi_read_buffer *rbuf;
 	struct list_head *p;
 	int ret, i;
 
-	amd_spi_set_opcode(amd_spi, opcode);
+	priv->set_op(amd_spi, opcode);
 	amd_spi_set_tx_count(amd_spi, tx_len);
 	amd_spi_set_rx_count(amd_spi, rx_len);
 
-	ret = amd_spi_execute_opcode(amd_spi);
+	ret = priv->exec_op(amd_spi);
 	if (ret)
 		return ret;
 
 	if (!list_empty(&amd_spi->rbuf_head)) {
-		ret = amd_spi_busy_wait(amd_spi);
+		ret = priv->busy_wait(amd_spi);
 		if (ret)
 			return ret;
 		list_for_each(p, &amd_spi->rbuf_head) {
@@ -262,6 +316,9 @@ static int amd_spi_transfer_one_message(struct spi_controller *ctrl, struct spi_
 	msg->status = ret;
 	spi_finalize_current_message(ctrl);
 
+	if (amd_spi->devtype_data->version)
+		amd_spi_clear_chip(amd_spi, msg->spi->chip_select);
+
 	return ret;
 }
 
@@ -293,6 +350,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;
@@ -320,9 +383,25 @@ 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,
+	.busy_wait	= amd_spi_busy_wait_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,
+	.busy_wait	= amd_spi_busy_wait_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);
@@ -340,4 +419,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 09/10] spi: amd: Add support for latest platform

[Gabriel Krisman Bertazi]

Lucas Tanure <tanureal@opensource.cirrus.com> writes:

> From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
>
> Add support for AMDI0062 controller

Hi,

This patch is way more complex than it needs to be, making it much
harder to review.  A few comments inline.

I hope I didn't miss a newer version of this patch?

> Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>

Broken signoff chain, missing Co-developed-by tag.

> ---
>  drivers/spi/spi-amd.c | 128 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 104 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
> index 0face11740ea..788a5c42d811 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>
> @@ -14,33 +15,48 @@
>  #include <linux/delay.h>
>  #include <linux/spi/spi.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

If you drop the indentation changes, the real diff seems to be:

+#define AMD_SPI_ENABLE_REG     0x20

...

+#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_ALT_CS_MASK    0x3
+#define AMD_SPI_ALT_CS_MASK    GENMASK(1, 0)

Which is WAY simpler to review.  Even if it is nice to have the defines
aligned, I suggest not doing it if the patch becomes much harder to
review.

>
>  struct amd_spi {
>  	void __iomem *io_remap_addr;
>  	unsigned long io_base_addr;
>  	u32 rom_addr;
>  	struct list_head rbuf_head;
> +	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);
> +	int (*busy_wait)(struct amd_spi *amd_spi);
>  };
>  
>  struct amd_spi_read_buffer {
> @@ -90,16 +106,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);
> +}
> +

Instead of creating two copies of each function and having indirect
branches, it would be way simpler to just save the version in
amd_spi and:

static void amd_spi_set_opcode(struct amd_spi *amd_spi, u8 cmd_opcode)
{
	switch (amd_spi->version) {
        case AMD_SPI_V1:
        	amd_spi_setclear_reg32(amd_spi, AMD_SPI_CTRL0_REG,
				       cmd_opcode, AMD_SPI_OPCODE_MASK);
 		break;
	case AMD_SPI_V2:
		amd_spi_writereg8(amd_spi, AMD_SPI_OPCODE_REG, cmd_opcode);
                break;
	default:
        	WARN_ON(1);
        }
}

Likewise for the other amd_spi_devtype_data hooks.

It is simpler, faster (avoids the indirect branches), avoids code
duplication, and you get the benefit of not touching the callers, which
will eliminate several of the hunks below.

>  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);
> @@ -110,7 +136,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)
>  {
>  	int timeout = 100000;
>  
> @@ -124,11 +150,11 @@ static int amd_spi_busy_wait(struct amd_spi *amd_spi)
>  	return 0;
>  }
>  
> -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;
>  
> -	ret = amd_spi_busy_wait(amd_spi);
> +	ret = amd_spi_busy_wait_v1(amd_spi);
>  	if (ret)
>  		return ret;
>  
> @@ -138,6 +164,33 @@ static int amd_spi_execute_opcode(struct amd_spi *amd_spi)
>  	return 0;
>  }
>  
> +static int amd_spi_busy_wait_v2(struct amd_spi *amd_spi)
> +{
> +	int timeout = 100000;
> +
> +	while (amd_spi_readreg32(amd_spi, AMD_SPI_STATUS_REG) & AMD_SPI_BUSY) {
> +		usleep_range(10, 20);
> +		if (timeout-- < 0)
> +			return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

This duplicates amd_spi_busy_wait_v1, except for the readreg call.  If
you follow the suggestion above, you'll avoid all this code duplication.

> +
> +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 0;
> +}
> +
>  static int amd_spi_master_setup(struct spi_device *spi)
>  {
>  	struct amd_spi *amd_spi = spi_master_get_devdata(spi->master);
> @@ -159,20 +212,21 @@ static void amd_spi_clear_list(struct amd_spi *amd_spi)
>  
>  static int amd_spi_transfer(struct amd_spi *amd_spi, u8 opcode, u8 tx_len, u8 rx_len, u8 fifo_pos)
>  {
> +	const struct amd_spi_devtype_data *priv = amd_spi->devtype_data;
>  	struct amd_spi_read_buffer *rbuf;
>  	struct list_head *p;
>  	int ret, i;
>  
> -	amd_spi_set_opcode(amd_spi, opcode);
> +	priv->set_op(amd_spi, opcode);
>  	amd_spi_set_tx_count(amd_spi, tx_len);
>  	amd_spi_set_rx_count(amd_spi, rx_len);
>  
> -	ret = amd_spi_execute_opcode(amd_spi);
> +	ret = priv->exec_op(amd_spi);
>  	if (ret)
>  		return ret;
>  
>  	if (!list_empty(&amd_spi->rbuf_head)) {
> -		ret = amd_spi_busy_wait(amd_spi);
> +		ret = priv->busy_wait(amd_spi);
>  		if (ret)
>  			return ret;
>  		list_for_each(p, &amd_spi->rbuf_head) {
> @@ -262,6 +316,9 @@ static int amd_spi_transfer_one_message(struct spi_controller *ctrl, struct spi_
>  	msg->status = ret;
>  	spi_finalize_current_message(ctrl);
>

And the above hunk will disappear.

> +	if (amd_spi->devtype_data->version)
> +		amd_spi_clear_chip(amd_spi, msg->spi->chip_select);
> +

This should be explicit (amd_spi->devtype_data->version == AMD_SPI_V2)

>  	return ret;
>  }
>  
> @@ -293,6 +350,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;
> @@ -320,9 +383,25 @@ 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,
> +	.busy_wait	= amd_spi_busy_wait_v1,
> +};
> +
> +static const struct amd_spi_devtype_data spi_v2 = {
> +	.version	= 1,

This is confusing.  The structure is called _v2, but the version field
says 1.  Please, be explicit:

#define AMD_SPI_V1  1
#define AMD_SPI_V2  2

And use the constants here.

Also, a nit regarding the summary line:  Surely someday there will
be a new "latest platform". It would be more meaningful to say
"Add support for AMDI0062" or maybe "Add support for rev2", if that makes
sense.

Thanks,

-- 
Gabriel Krisman Bertazi

Re: [PATCH v2 00/10] Improve support for AMD SPI controllers

[Mark Brown]

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

On Wed, Sep 08, 2021 at 12:34:41PM +0100, Lucas Tanure wrote:

> Lucas Tanure (9):
>   regmap: spi: Set regmap max raw r/w from max_transfer_size
>   regmap: spi: Check raw_[read|write] against max message size

There's no interaction between these regmap patches and the rest of the
series, they have no API impact, so they should be a separate series to
avoid spurious dependencies.

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