From: Amisha Patel <[email protected]>
In some situations like, chip wake-up with powersave enabled, SPI
commands are failing temporarily. Reissuing commands after reset helps
to overcome the failure. So, add the retry limit and reset command
sequence API for read/write SPI commands.
Signed-off-by: Amisha Patel <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/spi.c | 96 ++++++++++++++++---
1 file changed, 83 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index b0fc5e68feec..35856cdbfb08 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -74,6 +74,7 @@ static int wilc_spi_reset(struct wilc *wilc);
#define CMD_SINGLE_READ 0xca
#define CMD_RESET 0xcf
+#define SPI_RETRY_MAX_LIMIT 10
#define SPI_ENABLE_VMM_RETRY_LIMIT 2
/* SPI response fields (section 11.1.2 in ATWILC1000 User Guide): */
@@ -830,23 +831,45 @@ static int wilc_spi_special_cmd(struct wilc *wilc, u8 cmd)
return 0;
}
+static void wilc_spi_reset_cmd_sequence(struct wilc *wl, u8 attempt, u32 addr)
+{
+ struct spi_device *spi = to_spi_device(wl->dev);
+ struct wilc_spi *spi_priv = wl->bus_data;
+
+ if (!spi_priv->probing_crc)
+ dev_err(&spi->dev, "Reset and retry %d %x\n", attempt, addr);
+
+ usleep_range(1000, 1100);
+ wilc_spi_reset(wl);
+ usleep_range(1000, 1100);
+}
+
static int wilc_spi_read_reg(struct wilc *wilc, u32 addr, u32 *data)
{
struct spi_device *spi = to_spi_device(wilc->dev);
int result;
u8 cmd = CMD_SINGLE_READ;
u8 clockless = 0;
+ u8 retry_limit = SPI_RETRY_MAX_LIMIT;
- if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
+ if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
/* Clockless register */
cmd = CMD_INTERNAL_READ;
clockless = 1;
}
-
+
+retry:
result = wilc_spi_single_read(wilc, cmd, addr, data, clockless);
if (result) {
dev_err(&spi->dev, "Failed cmd, read reg (%08x)...\n", addr);
- return result;
+
+ /* retry is not applicable for clockless registers */
+ if (clockless || !retry_limit)
+ return result;
+
+ wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
+ retry_limit--;
+ goto retry;
}
le32_to_cpus(data);
@@ -858,14 +881,22 @@ static int wilc_spi_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
{
struct spi_device *spi = to_spi_device(wilc->dev);
int result;
+ u8 retry_limit = SPI_RETRY_MAX_LIMIT;
if (size <= 4)
return -EINVAL;
+retry:
result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_READ, addr, buf, size);
if (result) {
dev_err(&spi->dev, "Failed cmd, read block (%08x)...\n", addr);
- return result;
+
+ if (!retry_limit)
+ return result;
+
+ wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
+ retry_limit--;
+ goto retry;
}
return 0;
@@ -875,11 +906,19 @@ static int spi_internal_write(struct wilc *wilc, u32 adr, u32 dat)
{
struct spi_device *spi = to_spi_device(wilc->dev);
int result;
+ u8 retry_limit = SPI_RETRY_MAX_LIMIT;
+retry:
result = wilc_spi_write_cmd(wilc, CMD_INTERNAL_WRITE, adr, dat, 0);
if (result) {
dev_err(&spi->dev, "Failed internal write cmd...\n");
- return result;
+
+ if (!retry_limit)
+ return result;
+
+ wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
+ retry_limit--;
+ goto retry;
}
return 0;
@@ -890,12 +929,20 @@ static int spi_internal_read(struct wilc *wilc, u32 adr, u32 *data)
struct spi_device *spi = to_spi_device(wilc->dev);
struct wilc_spi *spi_priv = wilc->bus_data;
int result;
+ u8 retry_limit = SPI_RETRY_MAX_LIMIT;
+retry:
result = wilc_spi_single_read(wilc, CMD_INTERNAL_READ, adr, data, 0);
if (result) {
if (!spi_priv->probing_crc)
dev_err(&spi->dev, "Failed internal read cmd...\n");
- return result;
+
+ if (!retry_limit)
+ return result;
+
+ wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
+ retry_limit--;
+ goto retry;
}
le32_to_cpus(data);
@@ -915,19 +962,26 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
int result;
u8 cmd = CMD_SINGLE_WRITE;
u8 clockless = 0;
-
- if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
+ u8 retry_limit = SPI_RETRY_MAX_LIMIT;
+
+ if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
/* Clockless register */
cmd = CMD_INTERNAL_WRITE;
clockless = 1;
}
+retry:
result = wilc_spi_write_cmd(wilc, cmd, addr, data, clockless);
if (result) {
dev_err(&spi->dev, "Failed cmd, write reg (%08x)...\n", addr);
- return result;
- }
+
+ if (clockless || !retry_limit)
+ return result;
+ wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
+ retry_limit--;
+ goto retry;
+ }
return 0;
}
@@ -981,6 +1035,7 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
{
struct spi_device *spi = to_spi_device(wilc->dev);
int result;
+ u8 retry_limit = SPI_RETRY_MAX_LIMIT;
/*
* has to be greated than 4
@@ -988,11 +1043,12 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
if (size <= 4)
return -EINVAL;
+retry:
result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_WRITE, addr, NULL, size);
if (result) {
dev_err(&spi->dev,
"Failed cmd, write block (%08x)...\n", addr);
- return result;
+ goto fail;
}
/*
@@ -1001,13 +1057,27 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
result = spi_data_write(wilc, buf, size);
if (result) {
dev_err(&spi->dev, "Failed block data write...\n");
- return result;
+ goto fail;
}
/*
* Data response
*/
- return spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
+ result = spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
+ if (result) {
+ dev_err(&spi->dev, "Failed block data rsp...\n");
+ goto fail;
+ }
+
+ return 0;
+
+fail:
+ if (result && retry_limit) {
+ wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
+ retry_limit--;
+ goto retry;
+ }
+ return result;
}
/********************************************
--
2.25.1
On Thu, May 25, 2023 at 06:17:48PM +0000, [email protected] wrote:
> From: Amisha Patel <[email protected]>
>
> In some situations like, chip wake-up with powersave enabled, SPI
> commands are failing temporarily. Reissuing commands after reset helps
> to overcome the failure. So, add the retry limit and reset command
> sequence API for read/write SPI commands.
>
> Signed-off-by: Amisha Patel <[email protected]>
Hi Amisha,
thanks for your patch.
Some minor nits from my side.
...
> static int wilc_spi_read_reg(struct wilc *wilc, u32 addr, u32 *data)
> {
> struct spi_device *spi = to_spi_device(wilc->dev);
> int result;
> u8 cmd = CMD_SINGLE_READ;
> u8 clockless = 0;
> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>
> - if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
> + if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
> /* Clockless register */
> cmd = CMD_INTERNAL_READ;
> clockless = 1;
> }
> -
> +
nit: trailing tabs on the line above
> +retry:
> result = wilc_spi_single_read(wilc, cmd, addr, data, clockless);
> if (result) {
> dev_err(&spi->dev, "Failed cmd, read reg (%08x)...\n", addr);
> - return result;
> +
Ditto
> + /* retry is not applicable for clockless registers */
> + if (clockless || !retry_limit)
> + return result;
> +
> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> + retry_limit--;
> + goto retry;
> }
>
> le32_to_cpus(data);
> @@ -858,14 +881,22 @@ static int wilc_spi_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
> {
> struct spi_device *spi = to_spi_device(wilc->dev);
> int result;
> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>
> if (size <= 4)
> return -EINVAL;
>
> +retry:
> result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_READ, addr, buf, size);
> if (result) {
> dev_err(&spi->dev, "Failed cmd, read block (%08x)...\n", addr);
> - return result;
> +
Ditto
> + if (!retry_limit)
> + return result;
> +
> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> + retry_limit--;
> + goto retry;
> }
>
> return 0;
> @@ -875,11 +906,19 @@ static int spi_internal_write(struct wilc *wilc, u32 adr, u32 dat)
> {
> struct spi_device *spi = to_spi_device(wilc->dev);
> int result;
> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>
> +retry:
> result = wilc_spi_write_cmd(wilc, CMD_INTERNAL_WRITE, adr, dat, 0);
> if (result) {
> dev_err(&spi->dev, "Failed internal write cmd...\n");
> - return result;
> +
Ditto
> + if (!retry_limit)
> + return result;
> +
> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
> + retry_limit--;
> + goto retry;
> }
>
> return 0;
> @@ -890,12 +929,20 @@ static int spi_internal_read(struct wilc *wilc, u32 adr, u32 *data)
> struct spi_device *spi = to_spi_device(wilc->dev);
> struct wilc_spi *spi_priv = wilc->bus_data;
> int result;
> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>
> +retry:
> result = wilc_spi_single_read(wilc, CMD_INTERNAL_READ, adr, data, 0);
> if (result) {
> if (!spi_priv->probing_crc)
> dev_err(&spi->dev, "Failed internal read cmd...\n");
> - return result;
> +
Ditto
> + if (!retry_limit)
> + return result;
> +
> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
> + retry_limit--;
> + goto retry;
> }
>
> le32_to_cpus(data);
> @@ -915,19 +962,26 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
> int result;
> u8 cmd = CMD_SINGLE_WRITE;
> u8 clockless = 0;
> -
> - if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
> +
Ditto
> + if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
> /* Clockless register */
> cmd = CMD_INTERNAL_WRITE;
> clockless = 1;
> }
>
> +retry:
> result = wilc_spi_write_cmd(wilc, cmd, addr, data, clockless);
> if (result) {
> dev_err(&spi->dev, "Failed cmd, write reg (%08x)...\n", addr);
> - return result;
> - }
> +
Ditto
> + if (clockless || !retry_limit)
> + return result;
>
> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> + retry_limit--;
> + goto retry;
> + }
> return 0;
> }
>
> @@ -981,6 +1035,7 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
> {
> struct spi_device *spi = to_spi_device(wilc->dev);
> int result;
> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>
> /*
> * has to be greated than 4
> @@ -988,11 +1043,12 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
> if (size <= 4)
> return -EINVAL;
>
> +retry:
> result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_WRITE, addr, NULL, size);
> if (result) {
> dev_err(&spi->dev,
> "Failed cmd, write block (%08x)...\n", addr);
> - return result;
> + goto fail;
> }
>
> /*
> @@ -1001,13 +1057,27 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
> result = spi_data_write(wilc, buf, size);
> if (result) {
> dev_err(&spi->dev, "Failed block data write...\n");
> - return result;
> + goto fail;
> }
>
> /*
> * Data response
> */
> - return spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
> + result = spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
> + if (result) {
> + dev_err(&spi->dev, "Failed block data rsp...\n");
> + goto fail;
> + }
> +
> + return 0;
> +
> +fail:
> + if (result && retry_limit) {
nit: is result always non-zero here?
> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
> + retry_limit--;
> + goto retry;
> + }
> + return result;
> }
>
> /********************************************
Hi Simon,
On 5/27/23 10:45, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, May 25, 2023 at 06:17:48PM +0000, [email protected] wrote:
>> From: Amisha Patel <[email protected]>
>>
>> In some situations like, chip wake-up with powersave enabled, SPI
>> commands are failing temporarily. Reissuing commands after reset helps
>> to overcome the failure. So, add the retry limit and reset command
>> sequence API for read/write SPI commands.
>>
>> Signed-off-by: Amisha Patel <[email protected]>
>
> Hi Amisha,
>
> thanks for your patch.
> Some minor nits from my side.
>
> ...
>
>> static int wilc_spi_read_reg(struct wilc *wilc, u32 addr, u32 *data)
>> {
>> struct spi_device *spi = to_spi_device(wilc->dev);
>> int result;
>> u8 cmd = CMD_SINGLE_READ;
>> u8 clockless = 0;
>> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>>
>> - if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
>> + if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
>> /* Clockless register */
>> cmd = CMD_INTERNAL_READ;
>> clockless = 1;
>> }
>> -
>> +
>
> nit: trailing tabs on the line above
>
Thanks for letting me know. I'll remove all false indentations.
>> +retry:
>> result = wilc_spi_single_read(wilc, cmd, addr, data, clockless);
>> if (result) {
>> dev_err(&spi->dev, "Failed cmd, read reg (%08x)...\n", addr);
>> - return result;
>> +
>
> Ditto
>
>> + /* retry is not applicable for clockless registers */
>> + if (clockless || !retry_limit)
>> + return result;
>> +
>> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
>> + retry_limit--;
>> + goto retry;
>> }
>>
>> le32_to_cpus(data);
>> @@ -858,14 +881,22 @@ static int wilc_spi_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>> {
>> struct spi_device *spi = to_spi_device(wilc->dev);
>> int result;
>> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>>
>> if (size <= 4)
>> return -EINVAL;
>>
>> +retry:
>> result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_READ, addr, buf, size);
>> if (result) {
>> dev_err(&spi->dev, "Failed cmd, read block (%08x)...\n", addr);
>> - return result;
>> +
>
> Ditto
>
>> + if (!retry_limit)
>> + return result;
>> +
>> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
>> + retry_limit--;
>> + goto retry;
>> }
>>
>> return 0;
>> @@ -875,11 +906,19 @@ static int spi_internal_write(struct wilc *wilc, u32 adr, u32 dat)
>> {
>> struct spi_device *spi = to_spi_device(wilc->dev);
>> int result;
>> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>>
>> +retry:
>> result = wilc_spi_write_cmd(wilc, CMD_INTERNAL_WRITE, adr, dat, 0);
>> if (result) {
>> dev_err(&spi->dev, "Failed internal write cmd...\n");
>> - return result;
>> +
>
> Ditto
>
>> + if (!retry_limit)
>> + return result;
>> +
>> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
>> + retry_limit--;
>> + goto retry;
>> }
>>
>> return 0;
>> @@ -890,12 +929,20 @@ static int spi_internal_read(struct wilc *wilc, u32 adr, u32 *data)
>> struct spi_device *spi = to_spi_device(wilc->dev);
>> struct wilc_spi *spi_priv = wilc->bus_data;
>> int result;
>> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>>
>> +retry:
>> result = wilc_spi_single_read(wilc, CMD_INTERNAL_READ, adr, data, 0);
>> if (result) {
>> if (!spi_priv->probing_crc)
>> dev_err(&spi->dev, "Failed internal read cmd...\n");
>> - return result;
>> +
>
> Ditto
>
>> + if (!retry_limit)
>> + return result;
>> +
>> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, adr);
>> + retry_limit--;
>> + goto retry;
>> }
>>
>> le32_to_cpus(data);
>> @@ -915,19 +962,26 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
>> int result;
>> u8 cmd = CMD_SINGLE_WRITE;
>> u8 clockless = 0;
>> -
>> - if (addr < WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
>> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>> +
>
> Ditto
>
>> + if (addr <= WILC_SPI_CLOCKLESS_ADDR_LIMIT) {
>> /* Clockless register */
>> cmd = CMD_INTERNAL_WRITE;
>> clockless = 1;
>> }
>>
>> +retry:
>> result = wilc_spi_write_cmd(wilc, cmd, addr, data, clockless);
>> if (result) {
>> dev_err(&spi->dev, "Failed cmd, write reg (%08x)...\n", addr);
>> - return result;
>> - }
>> +
>
> Ditto
>
>> + if (clockless || !retry_limit)
>> + return result;
>>
>> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
>> + retry_limit--;
>> + goto retry;
>> + }
>> return 0;
>> }
>>
>> @@ -981,6 +1035,7 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>> {
>> struct spi_device *spi = to_spi_device(wilc->dev);
>> int result;
>> + u8 retry_limit = SPI_RETRY_MAX_LIMIT;
>>
>> /*
>> * has to be greated than 4
>> @@ -988,11 +1043,12 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>> if (size <= 4)
>> return -EINVAL;
>>
>> +retry:
>> result = wilc_spi_dma_rw(wilc, CMD_DMA_EXT_WRITE, addr, NULL, size);
>> if (result) {
>> dev_err(&spi->dev,
>> "Failed cmd, write block (%08x)...\n", addr);
>> - return result;
>> + goto fail;
>> }
>>
>> /*
>> @@ -1001,13 +1057,27 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
>> result = spi_data_write(wilc, buf, size);
>> if (result) {
>> dev_err(&spi->dev, "Failed block data write...\n");
>> - return result;
>> + goto fail;
>> }
>>
>> /*
>> * Data response
>> */
>> - return spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
>> + result = spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
>> + if (result) {
>> + dev_err(&spi->dev, "Failed block data rsp...\n");
>> + goto fail;
>> + }
>> +
>> + return 0;
>> +
>> +fail:
>> + if (result && retry_limit) {
>
> nit: is result always non-zero here?
>
Yes, result is always non-zero here as it reaches fail only in case of
unsuccessful spi data response function. I realize that result check is
not needed in fail as its checked beforehand. I'll edit it out too in
v2. Thank you.
>> + wilc_spi_reset_cmd_sequence(wilc, retry_limit, addr);
>> + retry_limit--;
>> + goto retry;
>> + }
>> + return result;
>> }
>>
>> /********************************************