Discussion:
[PATCH v2] staging: mt7621-spi: drop the broken full-duplex mode
Chuanhong Guo
2018-12-06 11:18:47 UTC
Permalink
According to John Crispin (aka blogic) on IRC on Nov 26 2018:
so basically i made cs1 work for MTK/labs when i built
the linkit smart for them. the req-sheet said that cs1 should be proper
duplex spi. however ....
1) the core will always send 1 byte before any transfer, this is the
m25p80 command.
2) mode 3 is broken and bit reversed (?)
3) some bit are incorrectly wired in hw for mode2/3
we wrote a test script and test for [0-0xffff] on all modes and certain
bits are swizzled under certain conditions and it was not possible to
fix this even using a hack.
we then decided to use spi-gpio and i never removed the errornous code
basically the spi is fecked for anything but half duplex spi mode0
running a sflash on it

The controller will always send some data from OPCODE register under half
duplex mode before starting a full-duplex transfer, so the full-duplex
mode is broken.
This piece of code also make CS1 unavailable since it forces the
broken full-duplex mode to be used on CS1.

Signed-off-by: Chuanhong Guo <***@gmail.com>
---
Changes since v1:
Quoted John's reply in commit message
slightly modified code comment

drivers/staging/mt7621-spi/spi-mt7621.c | 120 +++---------------------
1 file changed, 15 insertions(+), 105 deletions(-)

diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
index d045b5568e0f..f115a8bf652d 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -86,16 +86,13 @@ static inline void mt7621_spi_write(struct mt7621_spi *rs, u32 reg, u32 val)
iowrite32(val, rs->base + reg);
}

-static void mt7621_spi_reset(struct mt7621_spi *rs, int duplex)
+static void mt7621_spi_reset(struct mt7621_spi *rs)
{
u32 master = mt7621_spi_read(rs, MT7621_SPI_MASTER);

master |= 7 << 29;
master |= 1 << 2;
- if (duplex)
- master |= 1 << 10;
- else
- master &= ~(1 << 10);
+ master &= ~(1 << 10);

mt7621_spi_write(rs, MT7621_SPI_MASTER, master);
rs->pending_write = 0;
@@ -107,7 +104,7 @@ static void mt7621_spi_set_cs(struct spi_device *spi, int enable)
int cs = spi->chip_select;
u32 polar = 0;

- mt7621_spi_reset(rs, cs);
+ mt7621_spi_reset(rs);
if (enable)
polar = BIT(cs);
mt7621_spi_write(rs, MT7621_SPI_POLAR, polar);
@@ -261,7 +258,7 @@ static void mt7621_spi_write_half_duplex(struct mt7621_spi *rs,
rs->pending_write = len;
}

-static int mt7621_spi_transfer_half_duplex(struct spi_master *master,
+static int mt7621_spi_transfer_one_message(struct spi_master *master,
struct spi_message *m)
{
struct mt7621_spi *rs = spi_master_get_devdata(master);
@@ -284,7 +281,16 @@ static int mt7621_spi_transfer_half_duplex(struct spi_master *master,
mt7621_spi_set_cs(spi, 1);
m->actual_length = 0;
list_for_each_entry(t, &m->transfers, transfer_list) {
- if (t->rx_buf)
+ if((t->rx_buf) && (t->tx_buf)) {
+ /* This controller will shift some extra data out
+ * of spi_opcode if (mosi_bit_cnt > 0) &&
+ * (cmd_bit_cnt == 0). So the claimed full-duplex
+ * support is broken since we have no way to read
+ * the MISO value during that bit.
+ */
+ status = -EIO;
+ goto msg_done;
+ } else if (t->rx_buf)
mt7621_spi_read_half_duplex(rs, t->len, t->rx_buf);
else if (t->tx_buf)
mt7621_spi_write_half_duplex(rs, t->len, t->tx_buf);
@@ -300,102 +306,6 @@ static int mt7621_spi_transfer_half_duplex(struct spi_master *master,
return 0;
}

-static int mt7621_spi_transfer_full_duplex(struct spi_master *master,
- struct spi_message *m)
-{
- struct mt7621_spi *rs = spi_master_get_devdata(master);
- struct spi_device *spi = m->spi;
- unsigned int speed = spi->max_speed_hz;
- struct spi_transfer *t = NULL;
- int status = 0;
- int i, len = 0;
- int rx_len = 0;
- u32 data[9] = { 0 };
- u32 val = 0;
-
- mt7621_spi_wait_till_ready(rs);
-
- list_for_each_entry(t, &m->transfers, transfer_list) {
- const u8 *buf = t->tx_buf;
-
- if (t->rx_buf)
- rx_len += t->len;
-
- if (!buf)
- continue;
-
- if (WARN_ON(len + t->len > 16)) {
- status = -EIO;
- goto msg_done;
- }
-
- for (i = 0; i < t->len; i++, len++)
- data[len / 4] |= buf[i] << (8 * (len & 3));
- if (speed > t->speed_hz)
- speed = t->speed_hz;
- }
-
- if (WARN_ON(rx_len > 16)) {
- status = -EIO;
- goto msg_done;
- }
-
- if (mt7621_spi_prepare(spi, speed)) {
- status = -EIO;
- goto msg_done;
- }
-
- for (i = 0; i < len; i += 4)
- mt7621_spi_write(rs, MT7621_SPI_DATA0 + i, data[i / 4]);
-
- val |= len * 8;
- val |= (rx_len * 8) << 12;
- mt7621_spi_write(rs, MT7621_SPI_MOREBUF, val);
-
- mt7621_spi_set_cs(spi, 1);
-
- val = mt7621_spi_read(rs, MT7621_SPI_TRANS);
- val |= SPI_CTL_START;
- mt7621_spi_write(rs, MT7621_SPI_TRANS, val);
-
- mt7621_spi_wait_till_ready(rs);
-
- mt7621_spi_set_cs(spi, 0);
-
- for (i = 0; i < rx_len; i += 4)
- data[i / 4] = mt7621_spi_read(rs, MT7621_SPI_DATA4 + i);
-
- m->actual_length = rx_len;
-
- len = 0;
- list_for_each_entry(t, &m->transfers, transfer_list) {
- u8 *buf = t->rx_buf;
-
- if (!buf)
- continue;
-
- for (i = 0; i < t->len; i++, len++)
- buf[i] = data[len / 4] >> (8 * (len & 3));
- }
-
-msg_done:
- m->status = status;
- spi_finalize_current_message(master);
-
- return 0;
-}
-
-static int mt7621_spi_transfer_one_message(struct spi_master *master,
- struct spi_message *m)
-{
- struct spi_device *spi = m->spi;
- int cs = spi->chip_select;
-
- if (cs)
- return mt7621_spi_transfer_full_duplex(master, m);
- return mt7621_spi_transfer_half_duplex(master, m);
-}
-
static int mt7621_spi_setup(struct spi_device *spi)
{
struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
@@ -478,7 +388,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)

device_reset(&pdev->dev);

- mt7621_spi_reset(rs, 0);
+ mt7621_spi_reset(rs);

return spi_register_master(master);
}
--
2.19.1
Chuanhong Guo
2018-12-06 11:18:48 UTC
Permalink
As explained in previous patch, this SPI controller seems to be
tested on SPI flash only before mass production and some bits are
swizzled under other SPI modes probably due to incorrect wiring
inside the silicon. Drop implementation of SPI mode 1/2/3 since
they are broken.

Also drop RT2880_SPI_MODE_BITS macro because we now have only
SPI_LSB_FIRST implemented and the mode_bits is so short that we
don't need a macro there.

Signed-off-by: Chuanhong Guo <***@gmail.com>
---
Changes since v1:
drop unspoorted modes from mode_bits instead of reject them
in mt7621_spi_prepare.
dropped RT2880_SPI_MODES_BITS macro
modified code comment

drivers/staging/mt7621-spi/spi-mt7621.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
index f115a8bf652d..ba2d24ce0eb1 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -55,9 +55,6 @@
#define MT7621_CPOL BIT(4)
#define MT7621_LSB_FIRST BIT(3)

-#define RT2880_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | \
- SPI_LSB_FIRST | SPI_CS_HIGH)
-
struct mt7621_spi;

struct mt7621_spi {
@@ -136,20 +133,14 @@ static int mt7621_spi_prepare(struct spi_device *spi, unsigned int speed)
if (spi->mode & SPI_LSB_FIRST)
reg |= MT7621_LSB_FIRST;

+
+ /* This SPI controller seems to be tested on SPI flash only
+ * and some bits are swizzled under other SPI modes probably
+ * due to incorrect wiring inside the silicon. Only mode 0
+ * works correctly.
+ */
reg &= ~(MT7621_CPHA | MT7621_CPOL);
- switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
- case SPI_MODE_0:
- break;
- case SPI_MODE_1:
- reg |= MT7621_CPHA;
- break;
- case SPI_MODE_2:
- reg |= MT7621_CPOL;
- break;
- case SPI_MODE_3:
- reg |= MT7621_CPOL | MT7621_CPHA;
- break;
- }
+
mt7621_spi_write(rs, MT7621_SPI_MASTER, reg);

return 0;
@@ -367,7 +358,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)
return -ENOMEM;
}

- master->mode_bits = RT2880_SPI_MODE_BITS;
+ master->mode_bits = SPI_LSB_FIRST;

master->setup = mt7621_spi_setup;
master->transfer_one_message = mt7621_spi_transfer_one_message;
--
2.19.1
John Crispin
2018-12-06 11:24:18 UTC
Permalink
Post by Chuanhong Guo
As explained in previous patch, this SPI controller seems to be
tested on SPI flash only before mass production and some bits are
swizzled under other SPI modes probably due to incorrect wiring
inside the silicon. Drop implementation of SPI mode 1/2/3 since
they are broken.
Also drop RT2880_SPI_MODE_BITS macro because we now have only
SPI_LSB_FIRST implemented and the mode_bits is so short that we
don't need a macro there.
---
drop unspoorted modes from mode_bits instead of reject them
in mt7621_spi_prepare.
dropped RT2880_SPI_MODES_BITS macro
modified code comment
drivers/staging/mt7621-spi/spi-mt7621.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
index f115a8bf652d..ba2d24ce0eb1 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -55,9 +55,6 @@
#define MT7621_CPOL BIT(4)
#define MT7621_LSB_FIRST BIT(3)
-#define RT2880_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | \
- SPI_LSB_FIRST | SPI_CS_HIGH)
-
struct mt7621_spi;
struct mt7621_spi {
@@ -136,20 +133,14 @@ static int mt7621_spi_prepare(struct spi_device *spi, unsigned int speed)
if (spi->mode & SPI_LSB_FIRST)
reg |= MT7621_LSB_FIRST;
+
+ /* This SPI controller seems to be tested on SPI flash only
+ * and some bits are swizzled under other SPI modes probably
+ * due to incorrect wiring inside the silicon. Only mode 0
+ * works correctly.
+ */
reg &= ~(MT7621_CPHA | MT7621_CPOL);
- switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
- break;
- reg |= MT7621_CPHA;
- break;
- reg |= MT7621_CPOL;
- break;
- reg |= MT7621_CPOL | MT7621_CPHA;
- break;
- }
+
mt7621_spi_write(rs, MT7621_SPI_MASTER, reg);
return 0;
@@ -367,7 +358,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)
return -ENOMEM;
}
- master->mode_bits = RT2880_SPI_MODE_BITS;
+ master->mode_bits = SPI_LSB_FIRST;
master->setup = mt7621_spi_setup;
master->transfer_one_message = mt7621_spi_transfer_one_message;
Chuanhong Guo
2018-12-06 12:18:55 UTC
Permalink
Post by Chuanhong Guo
As explained in previous patch, this SPI controller seems to be
tested on SPI flash only before mass production and some bits are
swizzled under other SPI modes probably due to incorrect wiring
inside the silicon. Drop implementation of SPI mode 1/2/3 since
they are broken.
Also drop RT2880_SPI_MODE_BITS macro because we now have only
SPI_LSB_FIRST implemented and the mode_bits is so short that we
don't need a macro there.
---
drop unspoorted modes from mode_bits instead of reject them
in mt7621_spi_prepare.
dropped RT2880_SPI_MODES_BITS macro
modified code comment
drivers/staging/mt7621-spi/spi-mt7621.c | 25 ++++++++-----------------
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
index f115a8bf652d..ba2d24ce0eb1 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -55,9 +55,6 @@
#define MT7621_CPOL BIT(4)
#define MT7621_LSB_FIRST BIT(3)
-#define RT2880_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | \
- SPI_LSB_FIRST | SPI_CS_HIGH)
-
struct mt7621_spi;
struct mt7621_spi {
@@ -136,20 +133,14 @@ static int mt7621_spi_prepare(struct spi_device *spi, unsigned int speed)
if (spi->mode & SPI_LSB_FIRST)
reg |= MT7621_LSB_FIRST;
+
Sorry. I accidentally added a line here and it's complained by
checkpatch.pl. I'll send a v3 later.
Post by Chuanhong Guo
+ /* This SPI controller seems to be tested on SPI flash only
+ * and some bits are swizzled under other SPI modes probably
+ * due to incorrect wiring inside the silicon. Only mode 0
+ * works correctly.
+ */
reg &= ~(MT7621_CPHA | MT7621_CPOL);
- switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
- break;
- reg |= MT7621_CPHA;
- break;
- reg |= MT7621_CPOL;
- break;
- reg |= MT7621_CPOL | MT7621_CPHA;
- break;
- }
+
mt7621_spi_write(rs, MT7621_SPI_MASTER, reg);
return 0;
@@ -367,7 +358,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)
return -ENOMEM;
}
- master->mode_bits = RT2880_SPI_MODE_BITS;
+ master->mode_bits = SPI_LSB_FIRST;
master->setup = mt7621_spi_setup;
master->transfer_one_message = mt7621_spi_transfer_one_message;
--
2.19.1
Chuanhong Guo
2018-12-06 11:37:46 UTC
Permalink
Post by Chuanhong Guo
so basically i made cs1 work for MTK/labs when i built
the linkit smart for them. the req-sheet said that cs1 should be proper
duplex spi. however ....
1) the core will always send 1 byte before any transfer, this is the
m25p80 command.
2) mode 3 is broken and bit reversed (?)
3) some bit are incorrectly wired in hw for mode2/3
we wrote a test script and test for [0-0xffff] on all modes and certain
bits are swizzled under certain conditions and it was not possible to
fix this even using a hack.
we then decided to use spi-gpio and i never removed the errornous code
basically the spi is fecked for anything but half duplex spi mode0
running a sflash on it
The controller will always send some data from OPCODE register under half
duplex mode before starting a full-duplex transfer, so the full-duplex
mode is broken.
This piece of code also make CS1 unavailable since it forces the
broken full-duplex mode to be used on CS1.
---
Quoted John's reply in commit message
slightly modified code comment
drivers/staging/mt7621-spi/spi-mt7621.c | 120 +++---------------------
1 file changed, 15 insertions(+), 105 deletions(-)
diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
index d045b5568e0f..f115a8bf652d 100644
--- a/drivers/staging/mt7621-spi/spi-mt7621.c
+++ b/drivers/staging/mt7621-spi/spi-mt7621.c
@@ -86,16 +86,13 @@ static inline void mt7621_spi_write(struct mt7621_spi *rs, u32 reg, u32 val)
iowrite32(val, rs->base + reg);
}
-static void mt7621_spi_reset(struct mt7621_spi *rs, int duplex)
+static void mt7621_spi_reset(struct mt7621_spi *rs)
{
u32 master = mt7621_spi_read(rs, MT7621_SPI_MASTER);
master |= 7 << 29;
master |= 1 << 2;
- if (duplex)
- master |= 1 << 10;
- else
- master &= ~(1 << 10);
+ master &= ~(1 << 10);
mt7621_spi_write(rs, MT7621_SPI_MASTER, master);
rs->pending_write = 0;
@@ -107,7 +104,7 @@ static void mt7621_spi_set_cs(struct spi_device *spi, int enable)
int cs = spi->chip_select;
u32 polar = 0;
- mt7621_spi_reset(rs, cs);
+ mt7621_spi_reset(rs);
if (enable)
polar = BIT(cs);
mt7621_spi_write(rs, MT7621_SPI_POLAR, polar);
@@ -261,7 +258,7 @@ static void mt7621_spi_write_half_duplex(struct mt7621_spi *rs,
rs->pending_write = len;
}
-static int mt7621_spi_transfer_half_duplex(struct spi_master *master,
+static int mt7621_spi_transfer_one_message(struct spi_master *master,
struct spi_message *m)
{
struct mt7621_spi *rs = spi_master_get_devdata(master);
@@ -284,7 +281,16 @@ static int mt7621_spi_transfer_half_duplex(struct spi_master *master,
mt7621_spi_set_cs(spi, 1);
m->actual_length = 0;
list_for_each_entry(t, &m->transfers, transfer_list) {
- if (t->rx_buf)
+ if((t->rx_buf) && (t->tx_buf)) {
Sorry, checkpatch.pl complained about this. I'll send v3 later.
Post by Chuanhong Guo
+ /* This controller will shift some extra data out
+ * of spi_opcode if (mosi_bit_cnt > 0) &&
+ * (cmd_bit_cnt == 0). So the claimed full-duplex
+ * support is broken since we have no way to read
+ * the MISO value during that bit.
+ */
+ status = -EIO;
+ goto msg_done;
+ } else if (t->rx_buf)
mt7621_spi_read_half_duplex(rs, t->len, t->rx_buf);
else if (t->tx_buf)
mt7621_spi_write_half_duplex(rs, t->len, t->tx_buf);
@@ -300,102 +306,6 @@ static int mt7621_spi_transfer_half_duplex(struct spi_master *master,
return 0;
}
-static int mt7621_spi_transfer_full_duplex(struct spi_master *master,
- struct spi_message *m)
-{
- struct mt7621_spi *rs = spi_master_get_devdata(master);
- struct spi_device *spi = m->spi;
- unsigned int speed = spi->max_speed_hz;
- struct spi_transfer *t = NULL;
- int status = 0;
- int i, len = 0;
- int rx_len = 0;
- u32 data[9] = { 0 };
- u32 val = 0;
-
- mt7621_spi_wait_till_ready(rs);
-
- list_for_each_entry(t, &m->transfers, transfer_list) {
- const u8 *buf = t->tx_buf;
-
- if (t->rx_buf)
- rx_len += t->len;
-
- if (!buf)
- continue;
-
- if (WARN_ON(len + t->len > 16)) {
- status = -EIO;
- goto msg_done;
- }
-
- for (i = 0; i < t->len; i++, len++)
- data[len / 4] |= buf[i] << (8 * (len & 3));
- if (speed > t->speed_hz)
- speed = t->speed_hz;
- }
-
- if (WARN_ON(rx_len > 16)) {
- status = -EIO;
- goto msg_done;
- }
-
- if (mt7621_spi_prepare(spi, speed)) {
- status = -EIO;
- goto msg_done;
- }
-
- for (i = 0; i < len; i += 4)
- mt7621_spi_write(rs, MT7621_SPI_DATA0 + i, data[i / 4]);
-
- val |= len * 8;
- val |= (rx_len * 8) << 12;
- mt7621_spi_write(rs, MT7621_SPI_MOREBUF, val);
-
- mt7621_spi_set_cs(spi, 1);
-
- val = mt7621_spi_read(rs, MT7621_SPI_TRANS);
- val |= SPI_CTL_START;
- mt7621_spi_write(rs, MT7621_SPI_TRANS, val);
-
- mt7621_spi_wait_till_ready(rs);
-
- mt7621_spi_set_cs(spi, 0);
-
- for (i = 0; i < rx_len; i += 4)
- data[i / 4] = mt7621_spi_read(rs, MT7621_SPI_DATA4 + i);
-
- m->actual_length = rx_len;
-
- len = 0;
- list_for_each_entry(t, &m->transfers, transfer_list) {
- u8 *buf = t->rx_buf;
-
- if (!buf)
- continue;
-
- for (i = 0; i < t->len; i++, len++)
- buf[i] = data[len / 4] >> (8 * (len & 3));
- }
-
- m->status = status;
- spi_finalize_current_message(master);
-
- return 0;
-}
-
-static int mt7621_spi_transfer_one_message(struct spi_master *master,
- struct spi_message *m)
-{
- struct spi_device *spi = m->spi;
- int cs = spi->chip_select;
-
- if (cs)
- return mt7621_spi_transfer_full_duplex(master, m);
- return mt7621_spi_transfer_half_duplex(master, m);
-}
-
static int mt7621_spi_setup(struct spi_device *spi)
{
struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
@@ -478,7 +388,7 @@ static int mt7621_spi_probe(struct platform_device *pdev)
device_reset(&pdev->dev);
- mt7621_spi_reset(rs, 0);
+ mt7621_spi_reset(rs);
return spi_register_master(master);
}
--
2.19.1
Loading...