Hi Gerhard,
On Thu, Dec 26, 2013 at 04:33:46PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote:
> > On Thu, Dec 26, 2013 at 12:42:58PM +0100, Gerhard Sittig wrote:
> > > On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> > > > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> > > > index c3354e8..4e01019 100644
> > > > --- a/drivers/spi/spi-dw.c
> > > > +++ b/drivers/spi/spi-dw.c
> > > > @@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
> > > > dws->tx_end = dws->tx + transfer->len;
> > > > dws->rx = transfer->rx_buf;
> > > > dws->rx_end = dws->rx + transfer->len;
> > > > - dws->cs_change = transfer->cs_change;
> > > > dws->len = dws->cur_transfer->len;
> > > > if (chip != dws->prev_chip)
> > > > cs_change = 1;
> > > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > > > index 1ccfc18..cb52cfe 100644
> > > > --- a/drivers/spi/spi-dw.h
> > > > +++ b/drivers/spi/spi-dw.h
> > > > @@ -135,7 +135,6 @@ struct dw_spi {
> > > > u8 n_bytes; /* current is a 1/2
> > > > bytes op */
> > > > u8 max_bits_per_word; /* maxim is 16b
> > > > */
> > > > u32 dma_width;
> > > > - int cs_change;
> > > > irqreturn_t (*transfer_handler)(struct dw_spi *dws);
> > > > void (*cs_control)(u32 command);
> > > >
> > > > --
> > >
> > > This looks suspicious. Are you (the driver) ignoring the
> > > cs_change information which the caller (the code which emits the
> > > SPI message transfer call) provides? This appears to be a
> > > deficiency in the SPI master's code then. Callers should be able
> > > to control CS behaviour between the partial transfers of an SPI
> > > message. It's part of the API.
> >
> > The pump_transfers() local cs_change variable tracks chip select status
> > between transfers. In practice this field is meaningless for this hardware
> > without external chip select control, like GPIO.
>
> Please re-check, or tell me if your code base is different from
> mainline and I'm missing something.
>
> Inspection of local code (v3.13-rc5-4-gf399199c01c7) suggests
> that the local cs_change gets preset to 0 and enforced to 1 if
> the "chip" (the SPI slave?) changes. I can't see any other
> condition that gets evaluated. So the current driver does ignore
> the caller's request.
Right. I missed that.
> Your change removes the dws->cs_change member and its assignment
> from transfer->cs_change. A ':grep cs_change drivers/spi/*dw*'
> suggests that the CS change condition as specified by callers
> completely gets ignored. :-O This is a bug in my book.
Yes. But this patch doesn't change the situation.
> Regardless of whether the internal hardware implementation of the
> SPI controller's dedicated CS lines don't support software
> control of the line (which may be the cause of the bug, and
> causes trouble even for SPI messages which consist of a single
> transfer), and can get fixed or not, is just an implementation
> detail that's specific to this setup in combination with the
> internal CS line. But please don't ignore the caller's spec and
> thus break the SPI master's use for all other setups as well.
> Especially when you already have GPIO controlled CS lines on your
> radar. I still agree with others that the hardware CS line
> behaviour should be considered broken (dramatically reduces the
> usefulness of the controller), and only GPIO backed CS lines
> actually unbreak this master since they do introduce software
> control for the signal which previously was completely absent.
It seems that the cs_contorl callback is a driver specific hack that was meant
to allow software control of the actual chip-select signal. However, there is
no implementation of cs_control in current mainline code.
> > > While you are checking whether the SPI master obeys the caller's
> > > CS change spec, can you check the optional delay between partial
> > > transfers as well (I assume that both get handled in the same
> > > spots of the code path)? ISTR that these two aspects (and the
> > > lack of GPIO backed chip selects given how the hardware CS line
> > > acts) were real obstacles when trying to use this DW SPI master.
> >
> > The code in pump_transfers() seems to honor delay_usecs. What was the
> > problem
> > you encountered?
>
> Since you made me look at the code: The delay_usecs spec appears
> to only get obeyed _between_ transfers. It gets ignored for the
> last or only transfer of a message. Which may be unexpected, and
> differs from what other SPI controllers implement. Can't tell
> whether this breaks existing SPI slaves, but the delay probably
> was introduced for some reason. I guess deasserting CS while the
> slave still wants it does have implications...
Right. This is a bug.
> And I remember that the DW-SPI master code I've seen in the past
> may not have been outright wrong, but somehow was organized in
> unexpected ways. IIRC it handled the end of a transfer and the
> end of a message in separate code paths, duplicating quite some
> code yet handling the last transfer in different ways. I was
> irritated. But that must have been a different implementation, I
> know for sure it wasn't mainline, or else I had done something
> about it.
Well, I once wrote a driver for this SPI master. I didn't get much response on
the mailing list at the time, so I moved on, and this driver went in instead.
I hope it's not my code that irritated you.
> Please also note that switching to common queue support for the
> SPI master (which you have in your queue as well) may in
> bypassing fix the last transfer's being special for the DW-SPI
> driver. Then you should not care, it's all just transfers, no
> matter how many of them form a message.
Great. So this problem would solve itself if/when my common queue patch goes
in.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
