* [PATCH 2/4] wilc1000: Introduce symbolic names for SPI protocol register
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
@ 2021-02-24 5:51 ` David Mosberger-Tang
2021-02-24 5:51 ` [PATCH 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
` (5 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-24 5:51 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
The WILC1000 protocol control register has bits for enabling the CRCs
(CRC7 for commands and CRC16 for data) and to set the data packet
size. Define symbolic names for those so the code is more easily
understood.
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
drivers/net/wireless/microchip/wilc1000/spi.c | 38 ++++++++++++++-----
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index d11e365eeee2..fca34d1999ec 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -46,12 +46,25 @@ static const struct wilc_hif_func wilc_hif_spi;
#define CMD_RESET 0xcf
#define SPI_ENABLE_VMM_RETRY_LIMIT 2
-#define DATA_PKT_SZ_256 256
-#define DATA_PKT_SZ_512 512
-#define DATA_PKT_SZ_1K 1024
-#define DATA_PKT_SZ_4K (4 * 1024)
-#define DATA_PKT_SZ_8K (8 * 1024)
-#define DATA_PKT_SZ DATA_PKT_SZ_8K
+
+#define PROTOCOL_REG_PKT_SZ_MASK GENMASK(6, 4)
+#define PROTOCOL_REG_CRC16_MASK GENMASK(3, 3)
+#define PROTOCOL_REG_CRC7_MASK GENMASK(2, 2)
+
+/*
+ * The SPI data packet size may be any integer power of two in the
+ * range from 256 to 8192 bytes.
+ */
+#define DATA_PKT_LOG_SZ_MIN 8 /* 256 B */
+#define DATA_PKT_LOG_SZ_MAX 13 /* 8 KiB */
+
+/*
+ * Select the data packet size (log2 of number of bytes): Use the
+ * maximum data packet size. We only retransmit complete packets, so
+ * there is no benefit from using smaller data packets.
+ */
+#define DATA_PKT_LOG_SZ DATA_PKT_LOG_SZ_MAX
+#define DATA_PKT_SZ (1 << DATA_PKT_LOG_SZ)
#define USE_SPI_DMA 0
@@ -827,9 +840,16 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
}
}
if (spi_priv->crc_off == 0) {
- reg &= ~0xc; /* disable crc checking */
- reg &= ~0x70;
- reg |= (0x5 << 4);
+ /* disable crc checking: */
+ reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
+
+ /* set the data packet size: */
+ BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
+ || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
+ reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
+ reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
+ DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
+
ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
if (ret) {
dev_err(&spi->dev,
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 3/4] wilc1000: Check for errors at end of DMA write
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
2021-02-24 5:51 ` [PATCH 2/4] wilc1000: Introduce symbolic names for SPI protocol register David Mosberger-Tang
@ 2021-02-24 5:51 ` David Mosberger-Tang
2021-02-25 8:27 ` Kalle Valo
2021-02-24 5:52 ` [PATCH 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
` (4 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-24 5:51 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
After a DMA write to the WILC chip, check for and report any errors.
This is based on code from the wilc driver in the linux-at91
repository.
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
drivers/net/wireless/microchip/wilc1000/spi.c | 50 ++++++++++++++++++-
1 file changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index fca34d1999ec..b0e096a03a28 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -750,6 +750,51 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
return 0;
}
+static int spi_data_rsp(struct wilc *wilc, u8 cmd)
+{
+ struct spi_device *spi = to_spi_device(wilc->dev);
+ int result, i;
+ u8 rsp[4];
+
+ /*
+ * The response to data packets is two bytes long. For
+ * efficiency's sake, wilc_spi_write() wisely ignores the
+ * responses for all packets but the final one. The downside
+ * of that optimization is that when the final data packet is
+ * short, we may receive (part of) the response to the
+ * second-to-last packet before the one for the final packet.
+ * To handle this, we always read 4 bytes and then search for
+ * the last byte that contains the "Response Start" code (0xc
+ * in the top 4 bits). We then know that this byte is the
+ * first response byte of the final data packet.
+ */
+ result = wilc_spi_rx(wilc, rsp, sizeof(rsp));
+ if (result) {
+ dev_err(&spi->dev, "Failed bus error...\n");
+ return result;
+ }
+
+ for (i = sizeof(rsp) - 2; i >= 0; --i)
+ if ((rsp[i] & 0xf0) == 0xc0)
+ break;
+
+ if (i < 0) {
+ dev_err(&spi->dev,
+ "Data packet response missing (%02x %02x %02x %02x)\n",
+ rsp[0], rsp[1], rsp[2], rsp[3]);
+ return -1;
+ }
+
+ /* rsp[i] is the last response start byte */
+
+ if (rsp[i] != 0xc3 || rsp[i + 1] != 0x00) {
+ dev_err(&spi->dev, "Data response error (%02x %02x)\n",
+ rsp[i], rsp[i + 1]);
+ return -1;
+ }
+ return 0;
+}
+
static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
{
struct spi_device *spi = to_spi_device(wilc->dev);
@@ -777,7 +822,10 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
return result;
}
- return 0;
+ /*
+ * Data response
+ */
+ return spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
}
/********************************************
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/4] wilc1000: Check for errors at end of DMA write
2021-02-24 5:51 ` [PATCH 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
@ 2021-02-25 8:27 ` Kalle Valo
2021-02-25 18:10 ` David Mosberger-Tang
0 siblings, 1 reply; 24+ messages in thread
From: Kalle Valo @ 2021-02-25 8:27 UTC (permalink / raw)
To: David Mosberger-Tang; +Cc: linux-wireless, Ajay Singh, Claudiu Beznea
David Mosberger-Tang <davidm@egauge.net> writes:
> After a DMA write to the WILC chip, check for and report any errors.
>
> This is based on code from the wilc driver in the linux-at91
> repository.
>
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
> drivers/net/wireless/microchip/wilc1000/spi.c | 50 ++++++++++++++++++-
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index fca34d1999ec..b0e096a03a28 100644
> --- a/drivers/net/wireless/microchip/wilc1000/spi.c
> +++ b/drivers/net/wireless/microchip/wilc1000/spi.c
> @@ -750,6 +750,51 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
> return 0;
> }
>
> +static int spi_data_rsp(struct wilc *wilc, u8 cmd)
> +{
> + struct spi_device *spi = to_spi_device(wilc->dev);
> + int result, i;
> + u8 rsp[4];
> +
> + /*
> + * The response to data packets is two bytes long. For
> + * efficiency's sake, wilc_spi_write() wisely ignores the
> + * responses for all packets but the final one. The downside
> + * of that optimization is that when the final data packet is
> + * short, we may receive (part of) the response to the
> + * second-to-last packet before the one for the final packet.
> + * To handle this, we always read 4 bytes and then search for
> + * the last byte that contains the "Response Start" code (0xc
> + * in the top 4 bits). We then know that this byte is the
> + * first response byte of the final data packet.
> + */
> + result = wilc_spi_rx(wilc, rsp, sizeof(rsp));
> + if (result) {
> + dev_err(&spi->dev, "Failed bus error...\n");
> + return result;
> + }
> +
> + for (i = sizeof(rsp) - 2; i >= 0; --i)
> + if ((rsp[i] & 0xf0) == 0xc0)
> + break;
No magic numbers. Please create proper defines for these.
> + if (i < 0) {
> + dev_err(&spi->dev,
> + "Data packet response missing (%02x %02x %02x %02x)\n",
> + rsp[0], rsp[1], rsp[2], rsp[3]);
> + return -1;
> + }
> +
> + /* rsp[i] is the last response start byte */
> +
> + if (rsp[i] != 0xc3 || rsp[i + 1] != 0x00) {
Same here.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3/4] wilc1000: Check for errors at end of DMA write
2021-02-25 8:27 ` Kalle Valo
@ 2021-02-25 18:10 ` David Mosberger-Tang
0 siblings, 0 replies; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-25 18:10 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Ajay Singh, Claudiu Beznea
On Thu, 2021-02-25 at 10:27 +0200, Kalle Valo wrote:
> David Mosberger-Tang <davidm@egauge.net> writes:
> > + for (i = sizeof(rsp) - 2; i >= 0; --i)
> > + if ((rsp[i] & 0xf0) == 0xc0)
> > + break;
>
> No magic numbers. Please create proper defines for these.]
>
> + if (rsp[i] != 0xc3 || rsp[i + 1] != 0x00) {
>
> Same here.
Good points. I'll change those, thanks.
--david
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
2021-02-24 5:51 ` [PATCH 2/4] wilc1000: Introduce symbolic names for SPI protocol register David Mosberger-Tang
2021-02-24 5:51 ` [PATCH 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
@ 2021-02-24 5:52 ` David Mosberger-Tang
2021-02-24 10:01 ` Ajay.Kathat
` (2 more replies)
2021-02-27 17:29 ` [PATCH v2 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
` (3 subsequent siblings)
6 siblings, 3 replies; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-24 5:52 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
The driver so far has always disabled CRC protection. This means any
data corruption that occurred during the SPI transfers could
potentially go unnoticed. This patch adds the macros ENABLE_CRC7 and
ENABLE_CRC16 to allow compile-time selection of whether or not CRC7
and CRC16, respectively, should be enabled.
The default configuration remains unchanged, with both CRC7 and CRC16
off.
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
.../net/wireless/microchip/wilc1000/Kconfig | 1 +
drivers/net/wireless/microchip/wilc1000/spi.c | 151 +++++++++++++-----
2 files changed, 108 insertions(+), 44 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig b/drivers/net/wireless/microchip/wilc1000/Kconfig
index 7f15e42602dd..62cfcdc9aacc 100644
--- a/drivers/net/wireless/microchip/wilc1000/Kconfig
+++ b/drivers/net/wireless/microchip/wilc1000/Kconfig
@@ -27,6 +27,7 @@ config WILC1000_SPI
depends on CFG80211 && INET && SPI
select WILC1000
select CRC7
+ select CRC_ITU_T
help
This module adds support for the SPI interface of adapters using
WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index b0e096a03a28..c745a440d273 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -7,10 +7,23 @@
#include <linux/clk.h>
#include <linux/spi/spi.h>
#include <linux/crc7.h>
+#include <linux/crc-itu-t.h>
#include "netdev.h"
#include "cfg80211.h"
+/**
+ * Establish the driver's desired CRC configuration. CRC7 is used for
+ * command transfers which have no other protection against corruption
+ * during the SPI transfer. Commands are short so CRC7 is relatively
+ * cheap. CRC16 is used for data transfers, including network packet
+ * transfers. Since those transfers can be large, CRC16 is relatively
+ * expensive. CRC16 is also often redundant as network packets
+ * typically are protected by their own, higher-level checksum.
+ */
+#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands with CRC7 */
+#define ENABLE_CRC16 0 /* set to 1 to protect SPI data with CRC16 */
+
/*
* For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or
* more zero bytes between the command response and the DATA Start tag
@@ -22,7 +35,8 @@
#define WILC_SPI_RSP_HDR_EXTRA_DATA 8
struct wilc_spi {
- int crc_off;
+ bool crc7_enabled;
+ bool crc16_enabled;
};
static const struct wilc_hif_func wilc_hif_spi;
@@ -123,6 +137,10 @@ static int wilc_bus_probe(struct spi_device *spi)
if (!spi_priv)
return -ENOMEM;
+ /* WILC chip resets to both CRCs enabled: */
+ spi_priv->crc7_enabled = true;
+ spi_priv->crc16_enabled = true;
+
ret = wilc_cfg80211_init(&wilc, &spi->dev, WILC_HIF_SPI, &wilc_hif_spi);
if (ret) {
kfree(spi_priv);
@@ -303,7 +321,8 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
struct wilc_spi *spi_priv = wilc->bus_data;
int ix, nbytes;
int result = 0;
- u8 cmd, order, crc[2] = {0};
+ u8 cmd, order, crc[2];
+ u16 crc_calc;
/*
* Data
@@ -345,9 +364,12 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
}
/*
- * Write Crc
+ * Write CRC
*/
- if (!spi_priv->crc_off) {
+ if (spi_priv->crc16_enabled) {
+ crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
+ crc[0] = crc_calc >> 8;
+ crc[1] = crc_calc;
if (wilc_spi_tx(wilc, crc, 2)) {
dev_err(&spi->dev, "Failed data block crc write, bus error...\n");
result = -EINVAL;
@@ -381,11 +403,11 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
struct spi_device *spi = to_spi_device(wilc->dev);
struct wilc_spi *spi_priv = wilc->bus_data;
u8 wb[32], rb[32];
- u8 crc[2];
int cmd_len, resp_len, i;
+ u16 crc_calc, crc_recv;
struct wilc_spi_cmd *c;
- struct wilc_spi_read_rsp_data *r_data;
struct wilc_spi_rsp_data *r;
+ struct wilc_spi_read_rsp_data *r_data;
memset(wb, 0x0, sizeof(wb));
memset(rb, 0x0, sizeof(rb));
@@ -409,7 +431,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
cmd_len = offsetof(struct wilc_spi_cmd, u.simple_cmd.crc);
resp_len = sizeof(*r) + sizeof(*r_data) + WILC_SPI_RSP_HDR_EXTRA_DATA;
- if (!spi_priv->crc_off) {
+ if (spi_priv->crc7_enabled) {
c->u.simple_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
cmd_len += 1;
resp_len += 2;
@@ -455,8 +477,16 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
if (b)
memcpy(b, r_data->data, 4);
- if (!spi_priv->crc_off)
- memcpy(crc, r_data->crc, 2);
+ if (!clockless && spi_priv->crc16_enabled) {
+ crc_recv = (r_data->crc[0] << 8) | r_data->crc[1];
+ crc_calc = crc_itu_t(0xffff, r_data->data, 4);
+ if (crc_recv != crc_calc) {
+ dev_err(&spi->dev, "%s: bad CRC 0x%04x "
+ "(calculated 0x%04x)\n", __func__,
+ crc_recv, crc_calc);
+ return -EINVAL;
+ }
+ }
return 0;
}
@@ -483,7 +513,7 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
c->u.internal_w_cmd.addr[1] = adr;
c->u.internal_w_cmd.data = cpu_to_be32(data);
cmd_len = offsetof(struct wilc_spi_cmd, u.internal_w_cmd.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.internal_w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
} else if (cmd == CMD_SINGLE_WRITE) {
c->u.w_cmd.addr[0] = adr >> 16;
@@ -491,14 +521,14 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
c->u.w_cmd.addr[2] = adr;
c->u.w_cmd.data = cpu_to_be32(data);
cmd_len = offsetof(struct wilc_spi_cmd, u.w_cmd.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
} else {
dev_err(&spi->dev, "write cmd [%x] not supported\n", cmd);
return -EINVAL;
}
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
cmd_len += 1;
resp_len = sizeof(*r);
@@ -536,6 +566,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
{
struct spi_device *spi = to_spi_device(wilc->dev);
struct wilc_spi *spi_priv = wilc->bus_data;
+ u16 crc_recv, crc_calc;
u8 wb[32], rb[32];
int cmd_len, resp_len;
int retry, ix = 0;
@@ -554,7 +585,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
c->u.dma_cmd.size[0] = sz >> 8;
c->u.dma_cmd.size[1] = sz;
cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.dma_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
} else if (cmd == CMD_DMA_EXT_WRITE || cmd == CMD_DMA_EXT_READ) {
c->u.dma_cmd_ext.addr[0] = adr >> 16;
@@ -564,14 +595,14 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
c->u.dma_cmd_ext.size[1] = sz >> 8;
c->u.dma_cmd_ext.size[2] = sz;
cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd_ext.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.dma_cmd_ext.crc[0] = wilc_get_crc7(wb, cmd_len);
} else {
dev_err(&spi->dev, "dma read write cmd [%x] not supported\n",
cmd);
return -EINVAL;
}
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
cmd_len += 1;
resp_len = sizeof(*r);
@@ -637,12 +668,35 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
}
/*
- * Read Crc
+ * Read CRC
*/
- if (!spi_priv->crc_off && wilc_spi_rx(wilc, crc, 2)) {
- dev_err(&spi->dev,
- "Failed block crc read, bus err\n");
- return -EINVAL;
+ if (spi_priv->crc16_enabled) {
+ if (wilc_spi_rx(wilc, crc, 2)) {
+ dev_err(&spi->dev,
+ "Failed block crc read, bus err\n");
+ return -EINVAL;
+ }
+ crc_recv = (crc[0] << 8) | crc[1];
+ crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
+ if (crc_recv != crc_calc) {
+ dev_err(&spi->dev, "%s: bad CRC 0x%04x "
+ "(calculated 0x%04x)\n", __func__,
+ crc_recv, crc_calc);
+
+ {
+ u8 byte;
+ int i;
+
+ for (i = 0; i < 16384; ++i) {
+ byte = 0;
+ wilc_spi_rx(wilc, &byte, 1);
+ if (!byte)
+ break;
+ }
+ }
+
+ return -EINVAL;
+ }
}
ix += nbytes;
@@ -871,43 +925,52 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
if (ret) {
/*
- * Read failed. Try with CRC off. This might happen when module
- * is removed but chip isn't reset
+ * Read failed. Try with CRC7 off. This might happen
+ * when module is removed but chip isn't reset.
*/
- spi_priv->crc_off = 1;
+ spi_priv->crc7_enabled = false;
dev_err(&spi->dev,
- "Failed read with CRC on, retrying with CRC off\n");
+ "Failed read with CRC7 on, retrying with CRC7 off\n");
ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
if (ret) {
/*
- * Read failed with both CRC on and off,
+ * Read failed with both CRC7 on and off,
* something went bad
*/
dev_err(&spi->dev, "Failed internal read protocol\n");
return ret;
}
}
- if (spi_priv->crc_off == 0) {
- /* disable crc checking: */
- reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
-
- /* set the data packet size: */
- BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
- || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
- reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
- reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
- DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
-
- ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
- if (ret) {
- dev_err(&spi->dev,
- "[wilc spi %d]: Failed internal write reg\n",
- __LINE__);
- return ret;
- }
- spi_priv->crc_off = 1;
+
+ /* set up the desired CRC configuration: */
+ reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
+#if ENABLE_CRC7
+ reg |= PROTOCOL_REG_CRC7_MASK;
+#endif
+#if ENABLE_CRC16
+ reg |= PROTOCOL_REG_CRC16_MASK;
+#endif
+
+ /* set up the data packet size: */
+ BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
+ || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
+ reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
+ reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
+ DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
+
+ /* establish the new setup: */
+ ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
+ if (ret) {
+ dev_err(&spi->dev,
+ "[wilc spi %d]: Failed internal write reg\n",
+ __LINE__);
+ return ret;
}
+ /* now that new set up is established, update our state to match: */
+ spi_priv->crc7_enabled = ENABLE_CRC7;
+ spi_priv->crc16_enabled = ENABLE_CRC16;
+
/*
* make sure can read back chip id correctly
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 5:52 ` [PATCH 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
@ 2021-02-24 10:01 ` Ajay.Kathat
2021-02-24 16:25 ` David Mosberger-Tang
2021-02-24 13:35 ` Ajay.Kathat
2021-02-24 21:19 ` Julian Calaby
2 siblings, 1 reply; 24+ messages in thread
From: Ajay.Kathat @ 2021-02-24 10:01 UTC (permalink / raw)
To: davidm, linux-wireless; +Cc: Claudiu.Beznea
On 24/02/21 11:22 am, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The driver so far has always disabled CRC protection. This means any
> data corruption that occurred during the SPI transfers could
> potentially go unnoticed. This patch adds the macros ENABLE_CRC7 and
> ENABLE_CRC16 to allow compile-time selection of whether or not CRC7
> and CRC16, respectively, should be enabled.
>
> The default configuration remains unchanged, with both CRC7 and CRC16
> off.
>
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
> .../net/wireless/microchip/wilc1000/Kconfig | 1 +
> drivers/net/wireless/microchip/wilc1000/spi.c | 151 +++++++++++++-----
> 2 files changed, 108 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig b/drivers/net/wireless/microchip/wilc1000/Kconfig
> index 7f15e42602dd..62cfcdc9aacc 100644
> --- a/drivers/net/wireless/microchip/wilc1000/Kconfig
> +++ b/drivers/net/wireless/microchip/wilc1000/Kconfig
> @@ -27,6 +27,7 @@ config WILC1000_SPI
> depends on CFG80211 && INET && SPI
> select WILC1000
> select CRC7
> + select CRC_ITU_T
> help
> This module adds support for the SPI interface of adapters using
> WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index b0e096a03a28..c745a440d273 100644
> --- a/drivers/net/wireless/microchip/wilc1000/spi.c
> +++ b/drivers/net/wireless/microchip/wilc1000/spi.c
> @@ -7,10 +7,23 @@
> #include <linux/clk.h>
> #include <linux/spi/spi.h>
> #include <linux/crc7.h>
> +#include <linux/crc-itu-t.h>
>
> #include "netdev.h"
> #include "cfg80211.h"
>
> +/**
> + * Establish the driver's desired CRC configuration. CRC7 is used for
> + * command transfers which have no other protection against corruption
> + * during the SPI transfer. Commands are short so CRC7 is relatively
> + * cheap. CRC16 is used for data transfers, including network packet
> + * transfers. Since those transfers can be large, CRC16 is relatively
> + * expensive. CRC16 is also often redundant as network packets
> + * typically are protected by their own, higher-level checksum.
> + */
> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands with CRC7 */
> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data with CRC16 */
> +
> /*
> * For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or
> * more zero bytes between the command response and the DATA Start tag
> @@ -22,7 +35,8 @@
> #define WILC_SPI_RSP_HDR_EXTRA_DATA 8
>
> struct wilc_spi {
> - int crc_off;
> + bool crc7_enabled;
> + bool crc16_enabled;
> };
>
> static const struct wilc_hif_func wilc_hif_spi;
> @@ -123,6 +137,10 @@ static int wilc_bus_probe(struct spi_device *spi)
> if (!spi_priv)
> return -ENOMEM;
>
> + /* WILC chip resets to both CRCs enabled: */
> + spi_priv->crc7_enabled = true;
> + spi_priv->crc16_enabled = true;
> +
> ret = wilc_cfg80211_init(&wilc, &spi->dev, WILC_HIF_SPI, &wilc_hif_spi);
> if (ret) {
> kfree(spi_priv);
> @@ -303,7 +321,8 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
> struct wilc_spi *spi_priv = wilc->bus_data;
> int ix, nbytes;
> int result = 0;
> - u8 cmd, order, crc[2] = {0};
> + u8 cmd, order, crc[2];
> + u16 crc_calc;
>
> /*
> * Data
> @@ -345,9 +364,12 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
> }
>
> /*
> - * Write Crc
> + * Write CRC
> */
> - if (!spi_priv->crc_off) {
> + if (spi_priv->crc16_enabled) {
> + crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
> + crc[0] = crc_calc >> 8;
> + crc[1] = crc_calc;
> if (wilc_spi_tx(wilc, crc, 2)) {
> dev_err(&spi->dev, "Failed data block crc write, bus error...\n");
> result = -EINVAL;
> @@ -381,11 +403,11 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
> struct spi_device *spi = to_spi_device(wilc->dev);
> struct wilc_spi *spi_priv = wilc->bus_data;
> u8 wb[32], rb[32];
> - u8 crc[2];
> int cmd_len, resp_len, i;
> + u16 crc_calc, crc_recv;
> struct wilc_spi_cmd *c;
> - struct wilc_spi_read_rsp_data *r_data;
> struct wilc_spi_rsp_data *r;
> + struct wilc_spi_read_rsp_data *r_data;
>
> memset(wb, 0x0, sizeof(wb));
> memset(rb, 0x0, sizeof(rb));
> @@ -409,7 +431,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
> cmd_len = offsetof(struct wilc_spi_cmd, u.simple_cmd.crc);
> resp_len = sizeof(*r) + sizeof(*r_data) + WILC_SPI_RSP_HDR_EXTRA_DATA;
>
> - if (!spi_priv->crc_off) {
> + if (spi_priv->crc7_enabled) {
> c->u.simple_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> cmd_len += 1;
> resp_len += 2;
> @@ -455,8 +477,16 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
> if (b)
> memcpy(b, r_data->data, 4);
>
> - if (!spi_priv->crc_off)
> - memcpy(crc, r_data->crc, 2);
> + if (!clockless && spi_priv->crc16_enabled) {
> + crc_recv = (r_data->crc[0] << 8) | r_data->crc[1];
> + crc_calc = crc_itu_t(0xffff, r_data->data, 4);
> + if (crc_recv != crc_calc) {
> + dev_err(&spi->dev, "%s: bad CRC 0x%04x "
> + "(calculated 0x%04x)\n", __func__,
> + crc_recv, crc_calc);
> + return -EINVAL;
> + }
> + }
>
> return 0;
> }
> @@ -483,7 +513,7 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
> c->u.internal_w_cmd.addr[1] = adr;
> c->u.internal_w_cmd.data = cpu_to_be32(data);
> cmd_len = offsetof(struct wilc_spi_cmd, u.internal_w_cmd.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.internal_w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else if (cmd == CMD_SINGLE_WRITE) {
> c->u.w_cmd.addr[0] = adr >> 16;
> @@ -491,14 +521,14 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
> c->u.w_cmd.addr[2] = adr;
> c->u.w_cmd.data = cpu_to_be32(data);
> cmd_len = offsetof(struct wilc_spi_cmd, u.w_cmd.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else {
> dev_err(&spi->dev, "write cmd [%x] not supported\n", cmd);
> return -EINVAL;
> }
>
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> cmd_len += 1;
>
> resp_len = sizeof(*r);
> @@ -536,6 +566,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> {
> struct spi_device *spi = to_spi_device(wilc->dev);
> struct wilc_spi *spi_priv = wilc->bus_data;
> + u16 crc_recv, crc_calc;
> u8 wb[32], rb[32];
> int cmd_len, resp_len;
> int retry, ix = 0;
> @@ -554,7 +585,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> c->u.dma_cmd.size[0] = sz >> 8;
> c->u.dma_cmd.size[1] = sz;
> cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.dma_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else if (cmd == CMD_DMA_EXT_WRITE || cmd == CMD_DMA_EXT_READ) {
> c->u.dma_cmd_ext.addr[0] = adr >> 16;
> @@ -564,14 +595,14 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> c->u.dma_cmd_ext.size[1] = sz >> 8;
> c->u.dma_cmd_ext.size[2] = sz;
> cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd_ext.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.dma_cmd_ext.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else {
> dev_err(&spi->dev, "dma read write cmd [%x] not supported\n",
> cmd);
> return -EINVAL;
> }
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> cmd_len += 1;
>
> resp_len = sizeof(*r);
> @@ -637,12 +668,35 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> }
>
> /*
> - * Read Crc
> + * Read CRC
> */
> - if (!spi_priv->crc_off && wilc_spi_rx(wilc, crc, 2)) {
> - dev_err(&spi->dev,
> - "Failed block crc read, bus err\n");
> - return -EINVAL;
> + if (spi_priv->crc16_enabled) {
> + if (wilc_spi_rx(wilc, crc, 2)) {
> + dev_err(&spi->dev,
> + "Failed block crc read, bus err\n");
> + return -EINVAL;
> + }
> + crc_recv = (crc[0] << 8) | crc[1];
> + crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
> + if (crc_recv != crc_calc) {
> + dev_err(&spi->dev, "%s: bad CRC 0x%04x "
> + "(calculated 0x%04x)\n", __func__,
> + crc_recv, crc_calc);
> +
> + {
> + u8 byte;
> + int i;
> +
> + for (i = 0; i < 16384; ++i) {
> + byte = 0;
> + wilc_spi_rx(wilc, &byte, 1);
> + if (!byte)
> + break;
> + }
> + }
> +
> + return -EINVAL;
> + }
> }
>
> ix += nbytes;
> @@ -871,43 +925,52 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
> ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
> if (ret) {
> /*
> - * Read failed. Try with CRC off. This might happen when module
> - * is removed but chip isn't reset
> + * Read failed. Try with CRC7 off. This might happen
> + * when module is removed but chip isn't reset.
> */
> - spi_priv->crc_off = 1;
> + spi_priv->crc7_enabled = false;
This patch series also looks okay to me. I just have one input which is
captured below.
We need to disable both crc7 and crc16 while retrying on failure attempt
by adding below line
spi_priv->crc16_enabled = false;
By default the CRC checks are disabled, so if the kernel module is
reloaded it should reattempt with both disabled.
Regards
Ajay
> dev_err(&spi->dev,
> - "Failed read with CRC on, retrying with CRC off\n");
> + "Failed read with CRC7 on, retrying with CRC7 off\n");
> ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
> if (ret) {
> /*
> - * Read failed with both CRC on and off,
> + * Read failed with both CRC7 on and off,
> * something went bad
> */
> dev_err(&spi->dev, "Failed internal read protocol\n");
> return ret;
> }
> }
> - if (spi_priv->crc_off == 0) {
> - /* disable crc checking: */
> - reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
> -
> - /* set the data packet size: */
> - BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
> - || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
> - reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
> - reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
> - DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
> -
> - ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
> - if (ret) {
> - dev_err(&spi->dev,
> - "[wilc spi %d]: Failed internal write reg\n",
> - __LINE__);
> - return ret;
> - }
> - spi_priv->crc_off = 1;
> +
> + /* set up the desired CRC configuration: */
> + reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
> +#if ENABLE_CRC7
> + reg |= PROTOCOL_REG_CRC7_MASK;
> +#endif
> +#if ENABLE_CRC16
> + reg |= PROTOCOL_REG_CRC16_MASK;
> +#endif
> +
> + /* set up the data packet size: */
> + BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
> + || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
> + reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
> + reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
> + DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
> +
> + /* establish the new setup: */
> + ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
> + if (ret) {
> + dev_err(&spi->dev,
> + "[wilc spi %d]: Failed internal write reg\n",
> + __LINE__);
> + return ret;
> }
>
> + /* now that new set up is established, update our state to match: */
> + spi_priv->crc7_enabled = ENABLE_CRC7;
> + spi_priv->crc16_enabled = ENABLE_CRC16;
> +
> /*
> * make sure can read back chip id correctly
> */
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 10:01 ` Ajay.Kathat
@ 2021-02-24 16:25 ` David Mosberger-Tang
2021-02-25 4:50 ` Ajay.Kathat
0 siblings, 1 reply; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-24 16:25 UTC (permalink / raw)
To: Ajay.Kathat, linux-wireless; +Cc: Claudiu.Beznea
On Wed, 2021-02-24 at 10:01 +0000, Ajay.Kathat@microchip.com wrote:
> This patch series also looks okay to me. I just have one input which is
> captured below.
>
> We need to disable both crc7 and crc16 while retrying on failure attempt
> by adding below line
>
> spi_priv->crc16_enabled = false;
Ah, you're right. We can always probe with CRC16 off since the chip
returns valid register data regardless of whether crc16_enabled is
true or false. I'm thinking something like this:
spi_priv->crc16_enabled = false; // ignore CRC16 during CRC-
detection
for (crc7 = 1; crc7 >= 0; --crc7) {
spi_priv->crc7_enabled = crc7;
ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET,
®);
if (ret == 0)
break;
}
if (ret) {
dev_err(&spi->dev, "Failed with all possible CRC
settings.\n");
return ret;
}
> By default the CRC checks are disabled, so if the kernel module is
> reloaded it should reattempt with both disabled.
Are you sure about that? My test devices resets into:
PROTOCOL_REG = 0x2e
which should be CRC7 and CRC16 on, right?
--david
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 16:25 ` David Mosberger-Tang
@ 2021-02-25 4:50 ` Ajay.Kathat
0 siblings, 0 replies; 24+ messages in thread
From: Ajay.Kathat @ 2021-02-25 4:50 UTC (permalink / raw)
To: davidm, linux-wireless; +Cc: Claudiu.Beznea
On 24/02/21 9:55 pm, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, 2021-02-24 at 10:01 +0000, Ajay.Kathat@microchip.com wrote:
>
>> This patch series also looks okay to me. I just have one input which is
>> captured below.
>>
>> We need to disable both crc7 and crc16 while retrying on failure attempt
>> by adding below line
>>
>> spi_priv->crc16_enabled = false;
>
> Ah, you're right. We can always probe with CRC16 off since the chip
> returns valid register data regardless of whether crc16_enabled is
> true or false. I'm thinking something like this:
>
> spi_priv->crc16_enabled = false; // ignore CRC16 during CRC-
> detection
> for (crc7 = 1; crc7 >= 0; --crc7) {
Its better to first try with crc7 disable and then with enable because
our default configuration has both disabled so it will return success
immediatly on reattempt for default configuration.
for (crc7 = 0; crc7 <= 1; ++crc7) {
> spi_priv->crc7_enabled = crc7;
> ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET,
> ®);
> if (ret == 0)
> break;
> }
> if (ret) {
> dev_err(&spi->dev, "Failed with all possible CRC
> settings.\n");
> return ret;
> }
>
>> By default the CRC checks are disabled, so if the kernel module is
>> reloaded it should reattempt with both disabled.
>
> Are you sure about that? My test devices resets into:
>
> PROTOCOL_REG = 0x2e
> > which should be CRC7 and CRC16 on, right?
The workaround was added for scenario when kernel module is loaded
without CRC enabled in WILC1000 chip. The scenario was observed during
the kernel module unload(rmmod) then reload(insmod/modprobe). Mostly the
driver will read with CRC ON but if it fails then read attempt should be
tried with CRC OFF.
Regards,
Ajay
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 5:52 ` [PATCH 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
2021-02-24 10:01 ` Ajay.Kathat
@ 2021-02-24 13:35 ` Ajay.Kathat
2021-02-24 15:47 ` David Mosberger-Tang
2021-02-24 21:19 ` Julian Calaby
2 siblings, 1 reply; 24+ messages in thread
From: Ajay.Kathat @ 2021-02-24 13:35 UTC (permalink / raw)
To: davidm, linux-wireless; +Cc: Claudiu.Beznea
On 24/02/21 11:22 am, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> The driver so far has always disabled CRC protection. This means any
> data corruption that occurred during the SPI transfers could
> potentially go unnoticed. This patch adds the macros ENABLE_CRC7 and
> ENABLE_CRC16 to allow compile-time selection of whether or not CRC7
> and CRC16, respectively, should be enabled.
>
> The default configuration remains unchanged, with both CRC7 and CRC16
> off.
>
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
> .../net/wireless/microchip/wilc1000/Kconfig | 1 +
> drivers/net/wireless/microchip/wilc1000/spi.c | 151 +++++++++++++-----
> 2 files changed, 108 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig b/drivers/net/wireless/microchip/wilc1000/Kconfig
> index 7f15e42602dd..62cfcdc9aacc 100644
> --- a/drivers/net/wireless/microchip/wilc1000/Kconfig
> +++ b/drivers/net/wireless/microchip/wilc1000/Kconfig
> @@ -27,6 +27,7 @@ config WILC1000_SPI
> depends on CFG80211 && INET && SPI
> select WILC1000
> select CRC7
> + select CRC_ITU_T
> help
> This module adds support for the SPI interface of adapters using
> WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index b0e096a03a28..c745a440d273 100644
> --- a/drivers/net/wireless/microchip/wilc1000/spi.c
> +++ b/drivers/net/wireless/microchip/wilc1000/spi.c
> @@ -7,10 +7,23 @@
> #include <linux/clk.h>
> #include <linux/spi/spi.h>
> #include <linux/crc7.h>
> +#include <linux/crc-itu-t.h>
>
> #include "netdev.h"
> #include "cfg80211.h"
>
> +/**
> + * Establish the driver's desired CRC configuration. CRC7 is used for
> + * command transfers which have no other protection against corruption
> + * during the SPI transfer. Commands are short so CRC7 is relatively
> + * cheap. CRC16 is used for data transfers, including network packet
> + * transfers. Since those transfers can be large, CRC16 is relatively
> + * expensive. CRC16 is also often redundant as network packets
> + * typically are protected by their own, higher-level checksum.
> + */
> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands with CRC7 */
> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data with CRC16 */
> +
> /*
> * For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or
> * more zero bytes between the command response and the DATA Start tag
> @@ -22,7 +35,8 @@
> #define WILC_SPI_RSP_HDR_EXTRA_DATA 8
>
> struct wilc_spi {
> - int crc_off;
> + bool crc7_enabled;
> + bool crc16_enabled;
> };
>
> static const struct wilc_hif_func wilc_hif_spi;
> @@ -123,6 +137,10 @@ static int wilc_bus_probe(struct spi_device *spi)
> if (!spi_priv)
> return -ENOMEM;
>
> + /* WILC chip resets to both CRCs enabled: */
> + spi_priv->crc7_enabled = true;
> + spi_priv->crc16_enabled = true;
> +
> ret = wilc_cfg80211_init(&wilc, &spi->dev, WILC_HIF_SPI, &wilc_hif_spi);
> if (ret) {
> kfree(spi_priv);
> @@ -303,7 +321,8 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
> struct wilc_spi *spi_priv = wilc->bus_data;
> int ix, nbytes;
> int result = 0;
> - u8 cmd, order, crc[2] = {0};
> + u8 cmd, order, crc[2];
> + u16 crc_calc;
>
> /*
> * Data
> @@ -345,9 +364,12 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
> }
>
> /*
> - * Write Crc
> + * Write CRC
> */
> - if (!spi_priv->crc_off) {
> + if (spi_priv->crc16_enabled) {
> + crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
> + crc[0] = crc_calc >> 8;
> + crc[1] = crc_calc;
> if (wilc_spi_tx(wilc, crc, 2)) {
> dev_err(&spi->dev, "Failed data block crc write, bus error...\n");
> result = -EINVAL;
> @@ -381,11 +403,11 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
> struct spi_device *spi = to_spi_device(wilc->dev);
> struct wilc_spi *spi_priv = wilc->bus_data;
> u8 wb[32], rb[32];
> - u8 crc[2];
> int cmd_len, resp_len, i;
> + u16 crc_calc, crc_recv;
> struct wilc_spi_cmd *c;
> - struct wilc_spi_read_rsp_data *r_data;
> struct wilc_spi_rsp_data *r;
> + struct wilc_spi_read_rsp_data *r_data;
>
> memset(wb, 0x0, sizeof(wb));
> memset(rb, 0x0, sizeof(rb));
> @@ -409,7 +431,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
> cmd_len = offsetof(struct wilc_spi_cmd, u.simple_cmd.crc);
> resp_len = sizeof(*r) + sizeof(*r_data) + WILC_SPI_RSP_HDR_EXTRA_DATA;
>
> - if (!spi_priv->crc_off) {
> + if (spi_priv->crc7_enabled) {
> c->u.simple_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> cmd_len += 1;
> resp_len += 2;
> @@ -455,8 +477,16 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
> if (b)
> memcpy(b, r_data->data, 4);
>
> - if (!spi_priv->crc_off)
> - memcpy(crc, r_data->crc, 2);
> + if (!clockless && spi_priv->crc16_enabled) {
> + crc_recv = (r_data->crc[0] << 8) | r_data->crc[1];
> + crc_calc = crc_itu_t(0xffff, r_data->data, 4);
> + if (crc_recv != crc_calc) {
> + dev_err(&spi->dev, "%s: bad CRC 0x%04x "
> + "(calculated 0x%04x)\n", __func__,
> + crc_recv, crc_calc);
> + return -EINVAL;
> + }
> + }
>
> return 0;
> }
> @@ -483,7 +513,7 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
> c->u.internal_w_cmd.addr[1] = adr;
> c->u.internal_w_cmd.data = cpu_to_be32(data);
> cmd_len = offsetof(struct wilc_spi_cmd, u.internal_w_cmd.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.internal_w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else if (cmd == CMD_SINGLE_WRITE) {
> c->u.w_cmd.addr[0] = adr >> 16;
> @@ -491,14 +521,14 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
> c->u.w_cmd.addr[2] = adr;
> c->u.w_cmd.data = cpu_to_be32(data);
> cmd_len = offsetof(struct wilc_spi_cmd, u.w_cmd.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else {
> dev_err(&spi->dev, "write cmd [%x] not supported\n", cmd);
> return -EINVAL;
> }
>
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> cmd_len += 1;
>
> resp_len = sizeof(*r);
> @@ -536,6 +566,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> {
> struct spi_device *spi = to_spi_device(wilc->dev);
> struct wilc_spi *spi_priv = wilc->bus_data;
> + u16 crc_recv, crc_calc;
> u8 wb[32], rb[32];
> int cmd_len, resp_len;
> int retry, ix = 0;
> @@ -554,7 +585,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> c->u.dma_cmd.size[0] = sz >> 8;
> c->u.dma_cmd.size[1] = sz;
> cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.dma_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else if (cmd == CMD_DMA_EXT_WRITE || cmd == CMD_DMA_EXT_READ) {
> c->u.dma_cmd_ext.addr[0] = adr >> 16;
> @@ -564,14 +595,14 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> c->u.dma_cmd_ext.size[1] = sz >> 8;
> c->u.dma_cmd_ext.size[2] = sz;
> cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd_ext.crc);
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> c->u.dma_cmd_ext.crc[0] = wilc_get_crc7(wb, cmd_len);
> } else {
> dev_err(&spi->dev, "dma read write cmd [%x] not supported\n",
> cmd);
> return -EINVAL;
> }
> - if (!spi_priv->crc_off)
> + if (spi_priv->crc7_enabled)
> cmd_len += 1;
>
> resp_len = sizeof(*r);
> @@ -637,12 +668,35 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
> }
>
> /*
> - * Read Crc
> + * Read CRC
> */
> - if (!spi_priv->crc_off && wilc_spi_rx(wilc, crc, 2)) {
> - dev_err(&spi->dev,
> - "Failed block crc read, bus err\n");
> - return -EINVAL;
> + if (spi_priv->crc16_enabled) {
> + if (wilc_spi_rx(wilc, crc, 2)) {
> + dev_err(&spi->dev,
> + "Failed block crc read, bus err\n");
> + return -EINVAL;
> + }
> + crc_recv = (crc[0] << 8) | crc[1];
> + crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
> + if (crc_recv != crc_calc) {
> + dev_err(&spi->dev, "%s: bad CRC 0x%04x "
> + "(calculated 0x%04x)\n", __func__,
> + crc_recv, crc_calc);
One more observation.
I am not clear if the below block is really needed. Have you faced any
issue here and did the below logic of skipping data helped to come out
of it. Also checking the limit of 16384(2*8KB) byte looks odd when the
max limit for data packet is around 8KB. Am I missing something here.
> +
> + {
> + u8 byte;
> + int i;
> +
> + for (i = 0; i < 16384; ++i) {
> + byte = 0;
> + wilc_spi_rx(wilc, &byte, 1);
> + if (!byte)
> + break;
> + }
> + }
> +
> + return -EINVAL;
> + }
> }
>
> ix += nbytes;
> @@ -871,43 +925,52 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
> ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
> if (ret) {
> /*
> - * Read failed. Try with CRC off. This might happen when module
> - * is removed but chip isn't reset
> + * Read failed. Try with CRC7 off. This might happen
> + * when module is removed but chip isn't reset.
> */
> - spi_priv->crc_off = 1;
> + spi_priv->crc7_enabled = false;
> dev_err(&spi->dev,
> - "Failed read with CRC on, retrying with CRC off\n");
> + "Failed read with CRC7 on, retrying with CRC7 off\n");
> ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
> if (ret) {
> /*
> - * Read failed with both CRC on and off,
> + * Read failed with both CRC7 on and off,
> * something went bad
> */
> dev_err(&spi->dev, "Failed internal read protocol\n");
> return ret;
> }
> }
> - if (spi_priv->crc_off == 0) {
> - /* disable crc checking: */
> - reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
> -
> - /* set the data packet size: */
> - BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
> - || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
> - reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
> - reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
> - DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
> -
> - ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
> - if (ret) {
> - dev_err(&spi->dev,
> - "[wilc spi %d]: Failed internal write reg\n",
> - __LINE__);
> - return ret;
> - }
> - spi_priv->crc_off = 1;
> +
> + /* set up the desired CRC configuration: */
> + reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
> +#if ENABLE_CRC7
> + reg |= PROTOCOL_REG_CRC7_MASK;
> +#endif
> +#if ENABLE_CRC16
> + reg |= PROTOCOL_REG_CRC16_MASK;
> +#endif
> +
> + /* set up the data packet size: */
> + BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
> + || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
> + reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
> + reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
> + DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
> +
> + /* establish the new setup: */
> + ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
> + if (ret) {
> + dev_err(&spi->dev,
> + "[wilc spi %d]: Failed internal write reg\n",
> + __LINE__);
> + return ret;
> }
>
> + /* now that new set up is established, update our state to match: */
> + spi_priv->crc7_enabled = ENABLE_CRC7;
> + spi_priv->crc16_enabled = ENABLE_CRC16;
> +
> /*
> * make sure can read back chip id correctly
> */
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 13:35 ` Ajay.Kathat
@ 2021-02-24 15:47 ` David Mosberger-Tang
2021-02-25 4:58 ` Ajay.Kathat
0 siblings, 1 reply; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-24 15:47 UTC (permalink / raw)
To: Ajay.Kathat, linux-wireless; +Cc: Claudiu.Beznea
Ajay,
On Wed, 2021-02-24 at 13:35 +0000, Ajay.Kathat@microchip.com wrote:
>
> One more observation.
> I am not clear if the below block is really needed. Have you faced any
> issue here and did the below logic of skipping data helped to come out
> of it. Also checking the limit of 16384(2*8KB) byte looks odd when the
> max limit for data packet is around 8KB. Am I missing something here.
>
> > +
> > + {
> > + u8 byte;
> > + int i;
> > +
> > + for (i = 0; i < 16384; ++i) {
> > + byte = 0;
> > + wilc_spi_rx(wilc, &byte, 1);
> > + if (!byte)
> > + break;
> > + }
> > + }
Ouch, that's definitely not supposed to be there! It's left-over debug
code from when I was tracking down the power-save issue. Not sure how
I missed that. Thanks for catching it and being so gentle about it!
How do I fix this best? Do I resend the entire patch series or is it
OK to create a V2 of just this last patch?
--david
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 15:47 ` David Mosberger-Tang
@ 2021-02-25 4:58 ` Ajay.Kathat
2021-02-25 8:25 ` Kalle Valo
0 siblings, 1 reply; 24+ messages in thread
From: Ajay.Kathat @ 2021-02-25 4:58 UTC (permalink / raw)
To: davidm, linux-wireless; +Cc: Claudiu.Beznea
Hi David,
On 24/02/21 9:17 pm, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Ajay,
>
> On Wed, 2021-02-24 at 13:35 +0000, Ajay.Kathat@microchip.com wrote:
>>
>> One more observation.
>> I am not clear if the below block is really needed. Have you faced any
>> issue here and did the below logic of skipping data helped to come out
>> of it. Also checking the limit of 16384(2*8KB) byte looks odd when the
>> max limit for data packet is around 8KB. Am I missing something here.
>>
>>> +
>>> + {
>>> + u8 byte;
>>> + int i;
>>> +
>>> + for (i = 0; i < 16384; ++i) {
>>> + byte = 0;
>>> + wilc_spi_rx(wilc, &byte, 1);
>>> + if (!byte)
>>> + break;
>>> + }
>>> + }
>
> Ouch, that's definitely not supposed to be there! It's left-over debug
> code from when I was tracking down the power-save issue. Not sure how
> I missed that. Thanks for catching it and being so gentle about it!
>
> How do I fix this best? Do I resend the entire patch series or is it
> OK to create a V2 of just this last patch?
>
It's better to resubmit the complete patch series v2, as it would be
convenient for Kalle to apply the patches in order.
Regards,
Ajay
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-25 4:58 ` Ajay.Kathat
@ 2021-02-25 8:25 ` Kalle Valo
0 siblings, 0 replies; 24+ messages in thread
From: Kalle Valo @ 2021-02-25 8:25 UTC (permalink / raw)
To: Ajay.Kathat; +Cc: davidm, linux-wireless, Claudiu.Beznea
<Ajay.Kathat@microchip.com> writes:
> Hi David,
>
> On 24/02/21 9:17 pm, David Mosberger-Tang wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Ajay,
>>
>> On Wed, 2021-02-24 at 13:35 +0000, Ajay.Kathat@microchip.com wrote:
>>>
>>> One more observation.
>>> I am not clear if the below block is really needed. Have you faced any
>>> issue here and did the below logic of skipping data helped to come out
>>> of it. Also checking the limit of 16384(2*8KB) byte looks odd when the
>>> max limit for data packet is around 8KB. Am I missing something here.
>>>
>>>> +
>>>> + {
>>>> + u8 byte;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < 16384; ++i) {
>>>> + byte = 0;
>>>> + wilc_spi_rx(wilc, &byte, 1);
>>>> + if (!byte)
>>>> + break;
>>>> + }
>>>> + }
>>
>> Ouch, that's definitely not supposed to be there! It's left-over debug
>> code from when I was tracking down the power-save issue. Not sure how
>> I missed that. Thanks for catching it and being so gentle about it!
>>
>> How do I fix this best? Do I resend the entire patch series or is it
>> OK to create a V2 of just this last patch?
>>
>
> It's better to resubmit the complete patch series v2, as it would be
> convenient for Kalle to apply the patches in order.
Correct. This is also documented in the wiki, see the link below.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 5:52 ` [PATCH 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
2021-02-24 10:01 ` Ajay.Kathat
2021-02-24 13:35 ` Ajay.Kathat
@ 2021-02-24 21:19 ` Julian Calaby
2021-02-24 23:36 ` David Mosberger-Tang
2 siblings, 1 reply; 24+ messages in thread
From: Julian Calaby @ 2021-02-24 21:19 UTC (permalink / raw)
To: David Mosberger-Tang; +Cc: linux-wireless, Ajay Singh, Claudiu Beznea
Hi David,
On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <davidm@egauge.net> wrote:
>
> The driver so far has always disabled CRC protection. This means any
> data corruption that occurred during the SPI transfers could
> potentially go unnoticed. This patch adds the macros ENABLE_CRC7 and
> ENABLE_CRC16 to allow compile-time selection of whether or not CRC7
> and CRC16, respectively, should be enabled.
>
> The default configuration remains unchanged, with both CRC7 and CRC16
> off.
>
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
> .../net/wireless/microchip/wilc1000/Kconfig | 1 +
> drivers/net/wireless/microchip/wilc1000/spi.c | 151 +++++++++++++-----
> 2 files changed, 108 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig b/drivers/net/wireless/microchip/wilc1000/Kconfig
> index 7f15e42602dd..62cfcdc9aacc 100644
> --- a/drivers/net/wireless/microchip/wilc1000/Kconfig
> +++ b/drivers/net/wireless/microchip/wilc1000/Kconfig
> @@ -27,6 +27,7 @@ config WILC1000_SPI
> depends on CFG80211 && INET && SPI
> select WILC1000
> select CRC7
> + select CRC_ITU_T
> help
> This module adds support for the SPI interface of adapters using
> WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index b0e096a03a28..c745a440d273 100644
> --- a/drivers/net/wireless/microchip/wilc1000/spi.c
> +++ b/drivers/net/wireless/microchip/wilc1000/spi.c
> @@ -7,10 +7,23 @@
> #include <linux/clk.h>
> #include <linux/spi/spi.h>
> #include <linux/crc7.h>
> +#include <linux/crc-itu-t.h>
>
> #include "netdev.h"
> #include "cfg80211.h"
>
> +/**
> + * Establish the driver's desired CRC configuration. CRC7 is used for
> + * command transfers which have no other protection against corruption
> + * during the SPI transfer. Commands are short so CRC7 is relatively
> + * cheap. CRC16 is used for data transfers, including network packet
> + * transfers. Since those transfers can be large, CRC16 is relatively
> + * expensive. CRC16 is also often redundant as network packets
> + * typically are protected by their own, higher-level checksum.
> + */
> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands with CRC7 */
> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data with CRC16 */
Should these be Kconfig variables instead?
Thanks,
--
Julian Calaby
Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 21:19 ` Julian Calaby
@ 2021-02-24 23:36 ` David Mosberger-Tang
2021-02-25 5:56 ` Ajay.Kathat
0 siblings, 1 reply; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-24 23:36 UTC (permalink / raw)
To: Julian Calaby; +Cc: linux-wireless, Ajay Singh, Claudiu Beznea
Julian,
On Thu, 2021-02-25 at 08:19 +1100, Julian Calaby wrote:
> Hi David,
>
> On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <
> davidm@egauge.net> wrote:
> >
> > +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands
> > with CRC7 */
> > +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data
> > with CRC16 */
>
> Should these be Kconfig variables instead?
I'd certainly like that. Ajay, would you be OK with that?
--david
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-24 23:36 ` David Mosberger-Tang
@ 2021-02-25 5:56 ` Ajay.Kathat
2021-02-25 8:22 ` Kalle Valo
0 siblings, 1 reply; 24+ messages in thread
From: Ajay.Kathat @ 2021-02-25 5:56 UTC (permalink / raw)
To: davidm, julian.calaby; +Cc: linux-wireless, Claudiu.Beznea
On 25/02/21 5:06 am, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Julian,
>
> On Thu, 2021-02-25 at 08:19 +1100, Julian Calaby wrote:
>> Hi David,
>>
>> On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <
>> davidm@egauge.net> wrote:
>>>
>>> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands
>>> with CRC7 */
>>> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data
>>> with CRC16 */
>>
>> Should these be Kconfig variables instead?
>
> I'd certainly like that. Ajay, would you be OK with that?
>
Yes, I am fine with the changes to move as Kconfig variable.
Regards
Ajay
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-25 5:56 ` Ajay.Kathat
@ 2021-02-25 8:22 ` Kalle Valo
2021-02-25 11:03 ` Ajay.Kathat
0 siblings, 1 reply; 24+ messages in thread
From: Kalle Valo @ 2021-02-25 8:22 UTC (permalink / raw)
To: Ajay.Kathat; +Cc: davidm, julian.calaby, linux-wireless, Claudiu.Beznea
<Ajay.Kathat@microchip.com> writes:
> On 25/02/21 5:06 am, David Mosberger-Tang wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Julian,
>>
>> On Thu, 2021-02-25 at 08:19 +1100, Julian Calaby wrote:
>>> Hi David,
>>>
>>> On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <
>>> davidm@egauge.net> wrote:
>>>>
>>>> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands
>>>> with CRC7 */
>>>> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data
>>>> with CRC16 */
>>>
>>> Should these be Kconfig variables instead?
>>
>> I'd certainly like that. Ajay, would you be OK with that?
>>
>
> Yes, I am fine with the changes to move as Kconfig variable.
Kconfig is not ideal for configuring functionality, something like a
module parameter is usually better. But why not just enable CRC always?
Why would the user need to disable this?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-25 8:22 ` Kalle Valo
@ 2021-02-25 11:03 ` Ajay.Kathat
2021-02-26 7:09 ` Kalle Valo
0 siblings, 1 reply; 24+ messages in thread
From: Ajay.Kathat @ 2021-02-25 11:03 UTC (permalink / raw)
To: kvalo; +Cc: davidm, julian.calaby, linux-wireless, Claudiu.Beznea
On 25/02/21 1:52 pm, Kalle Valo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> <Ajay.Kathat@microchip.com> writes:
>
>> On 25/02/21 5:06 am, David Mosberger-Tang wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Julian,
>>>
>>> On Thu, 2021-02-25 at 08:19 +1100, Julian Calaby wrote:
>>>> Hi David,
>>>>
>>>> On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <
>>>> davidm@egauge.net> wrote:
>>>>>
>>>>> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands
>>>>> with CRC7 */
>>>>> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data
>>>>> with CRC16 */
>>>>
>>>> Should these be Kconfig variables instead?
>>>
>>> I'd certainly like that. Ajay, would you be OK with that?
>>>
>>
>> Yes, I am fine with the changes to move as Kconfig variable.
>
> Kconfig is not ideal for configuring functionality, something like a
> module parameter is usually better. But why not just enable CRC always?
> Why would the user need to disable this?
As I know, the CRC check can be an time taking operation for each data
packets and in turn, can have impact on throughput performance.
Generally, it is recommended to keep this CRC configuration disabled.
But someone is cautious can enable it on a need basis by knowing the
possible reduction on throughput number.
Regards,
Ajay
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
2021-02-25 11:03 ` Ajay.Kathat
@ 2021-02-26 7:09 ` Kalle Valo
0 siblings, 0 replies; 24+ messages in thread
From: Kalle Valo @ 2021-02-26 7:09 UTC (permalink / raw)
To: Ajay.Kathat; +Cc: davidm, julian.calaby, linux-wireless, Claudiu.Beznea
<Ajay.Kathat@microchip.com> writes:
> On 25/02/21 1:52 pm, Kalle Valo wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> <Ajay.Kathat@microchip.com> writes:
>>
>>> On 25/02/21 5:06 am, David Mosberger-Tang wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Julian,
>>>>
>>>> On Thu, 2021-02-25 at 08:19 +1100, Julian Calaby wrote:
>>>>> Hi David,
>>>>>
>>>>> On Wed, Feb 24, 2021 at 6:56 PM David Mosberger-Tang <
>>>>> davidm@egauge.net> wrote:
>>>>>>
>>>>>> +#define ENABLE_CRC7 0 /* set to 1 to protect SPI commands
>>>>>> with CRC7 */
>>>>>> +#define ENABLE_CRC16 0 /* set to 1 to protect SPI data
>>>>>> with CRC16 */
>>>>>
>>>>> Should these be Kconfig variables instead?
>>>>
>>>> I'd certainly like that. Ajay, would you be OK with that?
>>>>
>>>
>>> Yes, I am fine with the changes to move as Kconfig variable.
>>
>> Kconfig is not ideal for configuring functionality, something like a
>> module parameter is usually better. But why not just enable CRC always?
>> Why would the user need to disable this?
>
> As I know, the CRC check can be an time taking operation for each data
> packets and in turn, can have impact on throughput performance.
> Generally, it is recommended to keep this CRC configuration disabled.
> But someone is cautious can enable it on a need basis by knowing the
> possible reduction on throughput number.
Then I think a module parameter would be the best approach, I don't see
the need to recompile anything for this. And remember to document the
impact for the performance in the commit log and/or in the module
parameter documentation.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/4] wilc1000: Make SPI transfers work at 48MHz
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
` (2 preceding siblings ...)
2021-02-24 5:52 ` [PATCH 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
@ 2021-02-27 17:29 ` David Mosberger-Tang
2021-04-17 17:48 ` Kalle Valo
2021-02-27 17:29 ` [PATCH v2 2/4] wilc1000: Introduce symbolic names for SPI protocol register David Mosberger-Tang
` (2 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-27 17:29 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or more
zero bytes between the command response and the DATA Start tag (0xf3).
This behavior appears to be undocumented in "ATWILC1000 USER GUIDE"
(https://tinyurl.com/4hhshdts) but we have observed 1-4 zero bytes
when the SPI bus operates at 48MHz and none when it operates at 1MHz.
This code is derived from the equivalent code of the wilc driver in
the linux-at91 repository.
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
drivers/net/wireless/microchip/wilc1000/spi.c | 42 +++++++++++++------
1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index be732929322c..d11e365eeee2 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -11,6 +11,16 @@
#include "netdev.h"
#include "cfg80211.h"
+/*
+ * For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or
+ * more zero bytes between the command response and the DATA Start tag
+ * (0xf3). This behavior appears to be undocumented in "ATWILC1000
+ * USER GUIDE" (https://tinyurl.com/4hhshdts) but we have observed 1-4
+ * zero bytes when the SPI bus operates at 48MHz and none when it
+ * operates at 1MHz.
+ */
+#define WILC_SPI_RSP_HDR_EXTRA_DATA 8
+
struct wilc_spi {
int crc_off;
};
@@ -79,16 +89,15 @@ struct wilc_spi_cmd {
} __packed;
struct wilc_spi_read_rsp_data {
- u8 rsp_cmd_type;
- u8 status;
- u8 resp_header;
- u8 resp_data[4];
+ u8 header;
+ u8 data[4];
u8 crc[];
} __packed;
struct wilc_spi_rsp_data {
u8 rsp_cmd_type;
u8 status;
+ u8 data[];
} __packed;
static int wilc_bus_probe(struct spi_device *spi)
@@ -359,10 +368,11 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
struct spi_device *spi = to_spi_device(wilc->dev);
struct wilc_spi *spi_priv = wilc->bus_data;
u8 wb[32], rb[32];
- int cmd_len, resp_len;
u8 crc[2];
+ int cmd_len, resp_len, i;
struct wilc_spi_cmd *c;
- struct wilc_spi_read_rsp_data *r;
+ struct wilc_spi_read_rsp_data *r_data;
+ struct wilc_spi_rsp_data *r;
memset(wb, 0x0, sizeof(wb));
memset(rb, 0x0, sizeof(rb));
@@ -384,7 +394,8 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
}
cmd_len = offsetof(struct wilc_spi_cmd, u.simple_cmd.crc);
- resp_len = sizeof(*r);
+ resp_len = sizeof(*r) + sizeof(*r_data) + WILC_SPI_RSP_HDR_EXTRA_DATA;
+
if (!spi_priv->crc_off) {
c->u.simple_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
cmd_len += 1;
@@ -403,7 +414,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
return -EINVAL;
}
- r = (struct wilc_spi_read_rsp_data *)&rb[cmd_len];
+ r = (struct wilc_spi_rsp_data *)&rb[cmd_len];
if (r->rsp_cmd_type != cmd) {
dev_err(&spi->dev,
"Failed cmd response, cmd (%02x), resp (%02x)\n",
@@ -417,17 +428,22 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
return -EINVAL;
}
- if (WILC_GET_RESP_HDR_START(r->resp_header) != 0xf) {
- dev_err(&spi->dev, "Error, data read response (%02x)\n",
- r->resp_header);
+ for (i = 0; i < WILC_SPI_RSP_HDR_EXTRA_DATA; ++i)
+ if (WILC_GET_RESP_HDR_START(r->data[i]) == 0xf)
+ break;
+
+ if (i >= WILC_SPI_RSP_HDR_EXTRA_DATA) {
+ dev_err(&spi->dev, "Error, data start missing\n");
return -EINVAL;
}
+ r_data = (struct wilc_spi_read_rsp_data *)&r->data[i];
+
if (b)
- memcpy(b, r->resp_data, 4);
+ memcpy(b, r_data->data, 4);
if (!spi_priv->crc_off)
- memcpy(crc, r->crc, 2);
+ memcpy(crc, r_data->crc, 2);
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 2/4] wilc1000: Introduce symbolic names for SPI protocol register
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
` (3 preceding siblings ...)
2021-02-27 17:29 ` [PATCH v2 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
@ 2021-02-27 17:29 ` David Mosberger-Tang
2021-02-27 17:29 ` [PATCH v2 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
2021-02-27 17:31 ` [PATCH v2 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
6 siblings, 0 replies; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-27 17:29 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
The WILC1000 protocol control register has bits for enabling the CRCs
(CRC7 for commands and CRC16 for data) and to set the data packet
size. Define symbolic names for those so the code is more easily
understood.
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
drivers/net/wireless/microchip/wilc1000/spi.c | 38 ++++++++++++++-----
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index d11e365eeee2..fca34d1999ec 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -46,12 +46,25 @@ static const struct wilc_hif_func wilc_hif_spi;
#define CMD_RESET 0xcf
#define SPI_ENABLE_VMM_RETRY_LIMIT 2
-#define DATA_PKT_SZ_256 256
-#define DATA_PKT_SZ_512 512
-#define DATA_PKT_SZ_1K 1024
-#define DATA_PKT_SZ_4K (4 * 1024)
-#define DATA_PKT_SZ_8K (8 * 1024)
-#define DATA_PKT_SZ DATA_PKT_SZ_8K
+
+#define PROTOCOL_REG_PKT_SZ_MASK GENMASK(6, 4)
+#define PROTOCOL_REG_CRC16_MASK GENMASK(3, 3)
+#define PROTOCOL_REG_CRC7_MASK GENMASK(2, 2)
+
+/*
+ * The SPI data packet size may be any integer power of two in the
+ * range from 256 to 8192 bytes.
+ */
+#define DATA_PKT_LOG_SZ_MIN 8 /* 256 B */
+#define DATA_PKT_LOG_SZ_MAX 13 /* 8 KiB */
+
+/*
+ * Select the data packet size (log2 of number of bytes): Use the
+ * maximum data packet size. We only retransmit complete packets, so
+ * there is no benefit from using smaller data packets.
+ */
+#define DATA_PKT_LOG_SZ DATA_PKT_LOG_SZ_MAX
+#define DATA_PKT_SZ (1 << DATA_PKT_LOG_SZ)
#define USE_SPI_DMA 0
@@ -827,9 +840,16 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
}
}
if (spi_priv->crc_off == 0) {
- reg &= ~0xc; /* disable crc checking */
- reg &= ~0x70;
- reg |= (0x5 << 4);
+ /* disable crc checking: */
+ reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
+
+ /* set the data packet size: */
+ BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
+ || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
+ reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
+ reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
+ DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
+
ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
if (ret) {
dev_err(&spi->dev,
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 3/4] wilc1000: Check for errors at end of DMA write
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
` (4 preceding siblings ...)
2021-02-27 17:29 ` [PATCH v2 2/4] wilc1000: Introduce symbolic names for SPI protocol register David Mosberger-Tang
@ 2021-02-27 17:29 ` David Mosberger-Tang
2021-02-27 17:31 ` [PATCH v2 4/4] wilc1000: Add support for enabling CRC David Mosberger-Tang
6 siblings, 0 replies; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-27 17:29 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
After a DMA write to the WILC chip, check for and report any errors.
This is based on code from the wilc driver in the linux-at91
repository.
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
drivers/net/wireless/microchip/wilc1000/spi.c | 62 ++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index fca34d1999ec..0be93eabad69 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -47,6 +47,17 @@ static const struct wilc_hif_func wilc_hif_spi;
#define SPI_ENABLE_VMM_RETRY_LIMIT 2
+/* SPI response fields (section 11.1.2 in ATWILC1000 User Guide): */
+#define RSP_START_FIELD GENMASK(7, 4)
+#define RSP_TYPE_FIELD GENMASK(3, 0)
+
+/* SPI response values for the response fields: */
+#define RSP_START_TAG 0xc
+#define RSP_TYPE_FIRST_PACKET 0x1
+#define RSP_TYPE_INNER_PACKET 0x2
+#define RSP_TYPE_LAST_PACKET 0x3
+#define RSP_STATE_NO_ERROR 0x00
+
#define PROTOCOL_REG_PKT_SZ_MASK GENMASK(6, 4)
#define PROTOCOL_REG_CRC16_MASK GENMASK(3, 3)
#define PROTOCOL_REG_CRC7_MASK GENMASK(2, 2)
@@ -750,6 +761,52 @@ static int wilc_spi_write_reg(struct wilc *wilc, u32 addr, u32 data)
return 0;
}
+static int spi_data_rsp(struct wilc *wilc, u8 cmd)
+{
+ struct spi_device *spi = to_spi_device(wilc->dev);
+ int result, i;
+ u8 rsp[4];
+
+ /*
+ * The response to data packets is two bytes long. For
+ * efficiency's sake, wilc_spi_write() wisely ignores the
+ * responses for all packets but the final one. The downside
+ * of that optimization is that when the final data packet is
+ * short, we may receive (part of) the response to the
+ * second-to-last packet before the one for the final packet.
+ * To handle this, we always read 4 bytes and then search for
+ * the last byte that contains the "Response Start" code (0xc
+ * in the top 4 bits). We then know that this byte is the
+ * first response byte of the final data packet.
+ */
+ result = wilc_spi_rx(wilc, rsp, sizeof(rsp));
+ if (result) {
+ dev_err(&spi->dev, "Failed bus error...\n");
+ return result;
+ }
+
+ for (i = sizeof(rsp) - 2; i >= 0; --i)
+ if (FIELD_GET(RSP_START_FIELD, rsp[i]) == RSP_START_TAG)
+ break;
+
+ if (i < 0) {
+ dev_err(&spi->dev,
+ "Data packet response missing (%02x %02x %02x %02x)\n",
+ rsp[0], rsp[1], rsp[2], rsp[3]);
+ return -1;
+ }
+
+ /* rsp[i] is the last response start byte */
+
+ if (FIELD_GET(RSP_TYPE_FIELD, rsp[i]) != RSP_TYPE_LAST_PACKET
+ || rsp[i + 1] != RSP_STATE_NO_ERROR) {
+ dev_err(&spi->dev, "Data response error (%02x %02x)\n",
+ rsp[i], rsp[i + 1]);
+ return -1;
+ }
+ return 0;
+}
+
static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
{
struct spi_device *spi = to_spi_device(wilc->dev);
@@ -777,7 +834,10 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
return result;
}
- return 0;
+ /*
+ * Data response
+ */
+ return spi_data_rsp(wilc, CMD_DMA_EXT_WRITE);
}
/********************************************
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 4/4] wilc1000: Add support for enabling CRC
2021-02-24 5:51 [PATCH 1/4] wilc1000: Make SPI transfers work at 48MHz David Mosberger-Tang
` (5 preceding siblings ...)
2021-02-27 17:29 ` [PATCH v2 3/4] wilc1000: Check for errors at end of DMA write David Mosberger-Tang
@ 2021-02-27 17:31 ` David Mosberger-Tang
6 siblings, 0 replies; 24+ messages in thread
From: David Mosberger-Tang @ 2021-02-27 17:31 UTC (permalink / raw)
To: linux-wireless; +Cc: Ajay Singh, Claudiu Beznea, davidm
The driver so far has always disabled CRC protection. This means any
data corruption that occurrs during the SPI transfers could go
undetected. This patch adds module parameters enable_crc7 and
enable_crc16 to selectively turn on CRC7 (for command transfers) and
CRC16 (for data transfers), respectively.
The default configuration remains unchanged, with both CRC7 and CRC16
off.
The performance impact of CRC was measured by running ttcp -t four
times in a row on a SAMA5 device:
CRC7 CRC16 Throughput: Standard deviation:
---- ----- ----------- -------------------
off off 1720 +/- 48 KB/s
on off 1658 +/- 58 KB/s
on on 1579 +/- 84 KB/s
Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
.../net/wireless/microchip/wilc1000/Kconfig | 1 +
drivers/net/wireless/microchip/wilc1000/spi.c | 178 +++++++++++-------
2 files changed, 115 insertions(+), 64 deletions(-)
diff --git a/drivers/net/wireless/microchip/wilc1000/Kconfig b/drivers/net/wireless/microchip/wilc1000/Kconfig
index 7f15e42602dd..62cfcdc9aacc 100644
--- a/drivers/net/wireless/microchip/wilc1000/Kconfig
+++ b/drivers/net/wireless/microchip/wilc1000/Kconfig
@@ -27,6 +27,7 @@ config WILC1000_SPI
depends on CFG80211 && INET && SPI
select WILC1000
select CRC7
+ select CRC_ITU_T
help
This module adds support for the SPI interface of adapters using
WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 0be93eabad69..1472e9843896 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -7,10 +7,27 @@
#include <linux/clk.h>
#include <linux/spi/spi.h>
#include <linux/crc7.h>
+#include <linux/crc-itu-t.h>
#include "netdev.h"
#include "cfg80211.h"
+static bool enable_crc7; /* protect SPI commands with CRC7 */
+module_param(enable_crc7, bool, 0644);
+MODULE_PARM_DESC(enable_crc7,
+ "Enable CRC7 checksum to protect command transfers\n"
+ "\t\t\tagainst corruption during the SPI transfer.\n"
+ "\t\t\tCommand transfers are short and the CPU-cycle cost\n"
+ "\t\t\tof enabling this is small.");
+
+static bool enable_crc16; /* protect SPI data with CRC16 */
+module_param(enable_crc16, bool, 0644);
+MODULE_PARM_DESC(enable_crc16,
+ "Enable CRC16 checksum to protect data transfers\n"
+ "\t\t\tagainst corruption during the SPI transfer.\n"
+ "\t\t\tData transfers can be large and the CPU-cycle cost\n"
+ "\t\t\tof enabling this may be substantial.");
+
/*
* For CMD_SINGLE_READ and CMD_INTERNAL_READ, WILC may insert one or
* more zero bytes between the command response and the DATA Start tag
@@ -22,7 +39,9 @@
#define WILC_SPI_RSP_HDR_EXTRA_DATA 8
struct wilc_spi {
- int crc_off;
+ bool probing_crc; /* true if we're probing chip's CRC config */
+ bool crc7_enabled; /* true if crc7 is currently enabled */
+ bool crc16_enabled; /* true if crc16 is currently enabled */
};
static const struct wilc_hif_func wilc_hif_spi;
@@ -314,7 +333,8 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
struct wilc_spi *spi_priv = wilc->bus_data;
int ix, nbytes;
int result = 0;
- u8 cmd, order, crc[2] = {0};
+ u8 cmd, order, crc[2];
+ u16 crc_calc;
/*
* Data
@@ -356,9 +376,12 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz)
}
/*
- * Write Crc
+ * Write CRC
*/
- if (!spi_priv->crc_off) {
+ if (spi_priv->crc16_enabled) {
+ crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
+ crc[0] = crc_calc >> 8;
+ crc[1] = crc_calc;
if (wilc_spi_tx(wilc, crc, 2)) {
dev_err(&spi->dev, "Failed data block crc write, bus error...\n");
result = -EINVAL;
@@ -392,11 +415,11 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
struct spi_device *spi = to_spi_device(wilc->dev);
struct wilc_spi *spi_priv = wilc->bus_data;
u8 wb[32], rb[32];
- u8 crc[2];
int cmd_len, resp_len, i;
+ u16 crc_calc, crc_recv;
struct wilc_spi_cmd *c;
- struct wilc_spi_read_rsp_data *r_data;
struct wilc_spi_rsp_data *r;
+ struct wilc_spi_read_rsp_data *r_data;
memset(wb, 0x0, sizeof(wb));
memset(rb, 0x0, sizeof(rb));
@@ -420,7 +443,7 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
cmd_len = offsetof(struct wilc_spi_cmd, u.simple_cmd.crc);
resp_len = sizeof(*r) + sizeof(*r_data) + WILC_SPI_RSP_HDR_EXTRA_DATA;
- if (!spi_priv->crc_off) {
+ if (spi_priv->crc7_enabled) {
c->u.simple_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
cmd_len += 1;
resp_len += 2;
@@ -440,9 +463,10 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
r = (struct wilc_spi_rsp_data *)&rb[cmd_len];
if (r->rsp_cmd_type != cmd) {
- dev_err(&spi->dev,
- "Failed cmd response, cmd (%02x), resp (%02x)\n",
- cmd, r->rsp_cmd_type);
+ if (!spi_priv->probing_crc)
+ dev_err(&spi->dev,
+ "Failed cmd, cmd (%02x), resp (%02x)\n",
+ cmd, r->rsp_cmd_type);
return -EINVAL;
}
@@ -466,8 +490,16 @@ static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b,
if (b)
memcpy(b, r_data->data, 4);
- if (!spi_priv->crc_off)
- memcpy(crc, r_data->crc, 2);
+ if (!clockless && spi_priv->crc16_enabled) {
+ crc_recv = (r_data->crc[0] << 8) | r_data->crc[1];
+ crc_calc = crc_itu_t(0xffff, r_data->data, 4);
+ if (crc_recv != crc_calc) {
+ dev_err(&spi->dev, "%s: bad CRC 0x%04x "
+ "(calculated 0x%04x)\n", __func__,
+ crc_recv, crc_calc);
+ return -EINVAL;
+ }
+ }
return 0;
}
@@ -494,7 +526,7 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
c->u.internal_w_cmd.addr[1] = adr;
c->u.internal_w_cmd.data = cpu_to_be32(data);
cmd_len = offsetof(struct wilc_spi_cmd, u.internal_w_cmd.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.internal_w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
} else if (cmd == CMD_SINGLE_WRITE) {
c->u.w_cmd.addr[0] = adr >> 16;
@@ -502,14 +534,14 @@ static int wilc_spi_write_cmd(struct wilc *wilc, u8 cmd, u32 adr, u32 data,
c->u.w_cmd.addr[2] = adr;
c->u.w_cmd.data = cpu_to_be32(data);
cmd_len = offsetof(struct wilc_spi_cmd, u.w_cmd.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.w_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
} else {
dev_err(&spi->dev, "write cmd [%x] not supported\n", cmd);
return -EINVAL;
}
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
cmd_len += 1;
resp_len = sizeof(*r);
@@ -547,6 +579,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
{
struct spi_device *spi = to_spi_device(wilc->dev);
struct wilc_spi *spi_priv = wilc->bus_data;
+ u16 crc_recv, crc_calc;
u8 wb[32], rb[32];
int cmd_len, resp_len;
int retry, ix = 0;
@@ -565,7 +598,7 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
c->u.dma_cmd.size[0] = sz >> 8;
c->u.dma_cmd.size[1] = sz;
cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.dma_cmd.crc[0] = wilc_get_crc7(wb, cmd_len);
} else if (cmd == CMD_DMA_EXT_WRITE || cmd == CMD_DMA_EXT_READ) {
c->u.dma_cmd_ext.addr[0] = adr >> 16;
@@ -575,14 +608,14 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
c->u.dma_cmd_ext.size[1] = sz >> 8;
c->u.dma_cmd_ext.size[2] = sz;
cmd_len = offsetof(struct wilc_spi_cmd, u.dma_cmd_ext.crc);
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
c->u.dma_cmd_ext.crc[0] = wilc_get_crc7(wb, cmd_len);
} else {
dev_err(&spi->dev, "dma read write cmd [%x] not supported\n",
cmd);
return -EINVAL;
}
- if (!spi_priv->crc_off)
+ if (spi_priv->crc7_enabled)
cmd_len += 1;
resp_len = sizeof(*r);
@@ -648,12 +681,22 @@ static int wilc_spi_dma_rw(struct wilc *wilc, u8 cmd, u32 adr, u8 *b, u32 sz)
}
/*
- * Read Crc
+ * Read CRC
*/
- if (!spi_priv->crc_off && wilc_spi_rx(wilc, crc, 2)) {
- dev_err(&spi->dev,
- "Failed block crc read, bus err\n");
- return -EINVAL;
+ if (spi_priv->crc16_enabled) {
+ if (wilc_spi_rx(wilc, crc, 2)) {
+ dev_err(&spi->dev,
+ "Failed block CRC read, bus err\n");
+ return -EINVAL;
+ }
+ crc_recv = (crc[0] << 8) | crc[1];
+ crc_calc = crc_itu_t(0xffff, &b[ix], nbytes);
+ if (crc_recv != crc_calc) {
+ dev_err(&spi->dev, "%s: bad CRC 0x%04x "
+ "(calculated 0x%04x)\n", __func__,
+ crc_recv, crc_calc);
+ return -EINVAL;
+ }
}
ix += nbytes;
@@ -720,11 +763,13 @@ static int spi_internal_write(struct wilc *wilc, u32 adr, u32 dat)
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;
result = wilc_spi_single_read(wilc, CMD_INTERNAL_READ, adr, data, 0);
if (result) {
- dev_err(&spi->dev, "Failed internal read cmd...\n");
+ if (!spi_priv->probing_crc)
+ dev_err(&spi->dev, "Failed internal read cmd...\n");
return result;
}
@@ -861,7 +906,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
u32 reg;
u32 chipid;
static int isinit;
- int ret;
+ int ret, i;
if (isinit) {
ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid);
@@ -876,49 +921,54 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
*/
/*
- * TODO: We can remove the CRC trials if there is a definite
- * way to reset
+ * Infer the CRC settings that are currently in effect. This
+ * is necessary because we can't be sure that the chip has
+ * been RESET (e.g, after module unload and reload).
*/
- /* the SPI to it's initial value. */
- ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
- if (ret) {
- /*
- * Read failed. Try with CRC off. This might happen when module
- * is removed but chip isn't reset
- */
- spi_priv->crc_off = 1;
- dev_err(&spi->dev,
- "Failed read with CRC on, retrying with CRC off\n");
+ spi_priv->probing_crc = true;
+ spi_priv->crc7_enabled = enable_crc7;
+ spi_priv->crc16_enabled = false; /* don't check CRC16 during probing */
+ for (i = 0; i < 2; ++i) {
ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
- if (ret) {
- /*
- * Read failed with both CRC on and off,
- * something went bad
- */
- dev_err(&spi->dev, "Failed internal read protocol\n");
- return ret;
- }
+ if (ret == 0)
+ break;
+ spi_priv->crc7_enabled = !enable_crc7;
}
- if (spi_priv->crc_off == 0) {
- /* disable crc checking: */
- reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
-
- /* set the data packet size: */
- BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
- || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
- reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
- reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
- DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
-
- ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
- if (ret) {
- dev_err(&spi->dev,
- "[wilc spi %d]: Failed internal write reg\n",
- __LINE__);
- return ret;
- }
- spi_priv->crc_off = 1;
+ if (ret) {
+ dev_err(&spi->dev, "Failed with CRC7 on and off.\n");
+ return ret;
+ }
+
+ /* set up the desired CRC configuration: */
+ reg &= ~(PROTOCOL_REG_CRC7_MASK | PROTOCOL_REG_CRC16_MASK);
+ if (enable_crc7)
+ reg |= PROTOCOL_REG_CRC7_MASK;
+ if (enable_crc16)
+ reg |= PROTOCOL_REG_CRC16_MASK;
+
+ /* set up the data packet size: */
+ BUILD_BUG_ON(DATA_PKT_LOG_SZ < DATA_PKT_LOG_SZ_MIN
+ || DATA_PKT_LOG_SZ > DATA_PKT_LOG_SZ_MAX);
+ reg &= ~PROTOCOL_REG_PKT_SZ_MASK;
+ reg |= FIELD_PREP(PROTOCOL_REG_PKT_SZ_MASK,
+ DATA_PKT_LOG_SZ - DATA_PKT_LOG_SZ_MIN);
+
+ /* establish the new setup: */
+ ret = spi_internal_write(wilc, WILC_SPI_PROTOCOL_OFFSET, reg);
+ if (ret) {
+ dev_err(&spi->dev,
+ "[wilc spi %d]: Failed internal write reg\n",
+ __LINE__);
+ return ret;
}
+ /* update our state to match new protocol settings: */
+ spi_priv->crc7_enabled = enable_crc7;
+ spi_priv->crc16_enabled = enable_crc16;
+
+ /* re-read to make sure new settings are in effect: */
+ spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET, ®);
+
+ spi_priv->probing_crc = false;
/*
* make sure can read back chip id correctly
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread