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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 04 2018, Chuanhong Guo wrote:

> Hi!
> NeilBrown <neil@xxxxxxxxxx> 于2018年12月4日周二 上午5:55写道:
>>
>> On Mon, Dec 03 2018, Chuanhong Guo wrote:
>>
>> > 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.
> 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

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


> ---
> 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?
>>
>> Thanks,
>> NeilBrown
>>
>>
>> >
>> > Signed-off-by: Chuanhong Guo <gch981213@xxxxxxxxx>
>> > ---
>> >  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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux