2021-08-24 10:41:55

by Lucas Tanure

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

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

2021-08-24 10:41:58

by Lucas Tanure

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

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 <[email protected]>
---
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

2021-08-24 10:42:11

by Lucas Tanure

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

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 <[email protected]>
---
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

2021-08-24 10:42:40

by Lucas Tanure

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

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 <[email protected]>
---
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

2021-08-24 10:42:55

by Lucas Tanure

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

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

Signed-off-by: Lucas Tanure <[email protected]>
---
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

2021-08-24 10:42:55

by Lucas Tanure

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

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 <[email protected]>
---
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 <[email protected]>
+// Authors: Sanjay R Mehta <[email protected]>
+// Lucas Tanure <[email protected]>

#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

2021-08-24 10:42:59

by Lucas Tanure

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

This function can be replaced by readl_poll_timeout

Signed-off-by: Lucas Tanure <[email protected]>
---
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

2021-08-24 10:43:01

by Lucas Tanure

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

Add support for AMDI0062 controllers

Signed-off-by: Nehal Bakulchandra Shah <[email protected]>
Signed-off-by: Lucas Tanure <[email protected]>
---
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 <[email protected]>
+// Nehal Bakulchandra Shah <[email protected]>
// Lucas Tanure <[email protected]>

#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 <[email protected]>");
+MODULE_AUTHOR("Nehal Bakulchandra Shah <[email protected]>");
MODULE_DESCRIPTION("AMD SPI Master Controller Driver");
--
2.33.0

2021-08-24 10:43:25

by Lucas Tanure

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

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 <[email protected]>
---
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

2021-08-24 10:43:57

by Lucas Tanure

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

Remove internal cs from amd_spi

Signed-off-by: Lucas Tanure <[email protected]>
---
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

2021-08-24 15:47:27

by kernel test robot

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

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 <[email protected]>

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/[email protected]

Attachments:

2021-08-24 16:39:50

by Mark Brown

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

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.

Attachments:

2021-08-24 16:41:47

by Mark Brown

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

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.

Attachments:

2021-08-24 16:46:00

by Mark Brown

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

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.

Attachments:

2021-08-24 16:49:29

by Mark Brown

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

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.

Attachments:

2021-08-24 16:52:46

by Mark Brown

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

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.

Attachments:

2021-08-24 17:42:39

by Mark Brown

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

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.

Attachments:

2021-08-24 17:42:49

by Mark Brown

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

On Tue, Aug 24, 2021 at 11:40:41AM +0100, Lucas Tanure wrote:
> Add support for AMDI0062 controllers
>
> Signed-off-by: Nehal Bakulchandra Shah <[email protected]>
> Signed-off-by: Lucas Tanure <[email protected]>

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.

Attachments:

2021-08-24 17:43:36

by Mark Brown

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

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).

Attachments:

2021-08-25 17:15:45

by Lucas Tanure

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

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

2021-08-25 21:02:04

by Mark Brown

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

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.

Attachments: