[PATCH] staging: mt7621-spi: drop the broken full-duplex mode

Discussion:

Chuanhong Guo

2018-12-03 10:34:53 UTC

Under MORE_BUF_MODE the controller will always shift one bit out of
spi_opcode if (mosi_bit_cnt > 0) && (cmd_bit_cnt == 0) so the full-
duplex mode is broken since we can't read anything from MISO during
writing spi_opcode.
This piece of code also make CS1 unavailable since it forces the
broken full-duplex mode to be enabled on CS1.

Signed-off-by: Chuanhong Guo <***@gmail.com>
---
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..8af6f307e176 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 one extra bit 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

NeilBrown

2018-12-03 21:55:39 UTC

Permalink

Post by Chuanhong Guo
Under MORE_BUF_MODE the controller will always shift one bit out of
spi_opcode if (mosi_bit_cnt > 0) && (cmd_bit_cnt == 0) so the full-
duplex mode is broken since we can't read anything from MISO during
writing spi_opcode.
This piece of code also make CS1 unavailable since it forces the
broken full-duplex mode to be enabled on CS1.

Hi,
How do you know these details of the hardware? Do you have detailed
specs for the hardware or have you experimented with it or are you one
of the designers or ....?
It would be nice to have the source of this information documented.

Thanks,
NeilBrown

Post by Chuanhong Guo
---
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..8af6f307e176 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 one extra bit 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

Chuanhong Guo

2018-12-04 04:43:32 UTC

Permalink

Hi!

Post by NeilBrown

Those info are provided by John Crispin on IRC #openwrt-devel. Here is
the quoted reply:
---
Nov 26 17:00:15 <blogic> gch981213[w]: 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 ....
Nov 26 17:00:34 <blogic> 1) the core will always send 1 byte before
any transfer, this is the m25p80 command.
Nov 26 17:00:49 <blogic> 2) mode 3 is broken and bit reversed (?)
Nov 26 17:01:01 <blogic> 3) some bit are incorrectly wired in hw for mode2/3
Nov 26 17:01:50 <blogic> 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.
Nov 26 17:02:06 <blogic> we then decided to use spi-gpio and i never
removed the errornous code
Nov 26 17:02:38 <blogic> basically the spi is fecked for anything but
half duplex spi mode0 running a sflash on it
---
And another guy told me that his experiment shows there will be one
extra bit if we set cmd_bit_cnt to 0 and mosi_bit_cnt > 0.
I guess it doesn't matter whether it's an extra "byte" or "bit" since
there are always some data that have to be sent under half-duplex.

Do I need to add "some experiment shows that" at the beginning of my
commit message?

Post by NeilBrown
Thanks,
NeilBrown

NeilBrown

2018-12-05 00:06:14 UTC

Permalink

Post by Chuanhong Guo
Hi!

Post by NeilBrown

Those info are provided by John Crispin on IRC #openwrt-devel. Here is
---
Nov 26 17:00:15 <blogic> gch981213[w]: 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 ....
Nov 26 17:00:34 <blogic> 1) the core will always send 1 byte before
any transfer, this is the m25p80 command.
Nov 26 17:00:49 <blogic> 2) mode 3 is broken and bit reversed (?)
Nov 26 17:01:01 <blogic> 3) some bit are incorrectly wired in hw for mode2/3
Nov 26 17:01:50 <blogic> 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.
Nov 26 17:02:06 <blogic> we then decided to use spi-gpio and i never
removed the errornous code
Nov 26 17:02:38 <blogic> basically the spi is fecked for anything but
half duplex spi mode0 running a sflash on it

I think this is useful information that is worth preserving in the
commit message. I would strip the time stamps, and make it something
like this:

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

If you add that to the commit message, I would be quite happy to Ack it.

Thanks,
NeilBrown

Post by Chuanhong Guo
---
And another guy told me that his experiment shows there will be one
extra bit if we set cmd_bit_cnt to 0 and mosi_bit_cnt > 0.
I guess it doesn't matter whether it's an extra "byte" or "bit" since
there are always some data that have to be sent under half-duplex.
Do I need to add "some experiment shows that" at the beginning of my
commit message?

Post by NeilBrown
Thanks,
NeilBrown

Chuanhong Guo

2018-12-05 15:23:40 UTC

Permalink

forgot to reply to mailing list...

Post by NeilBrown

Post by Chuanhong Guo
Hi!

Post by NeilBrown

I think this is useful information that is worth preserving in the
commit message. I would strip the time stamps, and make it something
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
If you add that to the commit message, I would be quite happy to Ack it.

I'll do that tomorrow.
Should I duplicate the above message in the other patch (the one
dropping SPI_MODE1/2/3) or squash the two patches together or just
keep that one untouched?

Post by NeilBrown
Thanks,
NeilBrown