| Message ID | 134cfa84519ca84268fc0ef502fba12773224133.1388040447.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0a47d3c40428fb8174ea36ede35267ddc7042f34 |
| Headers | show |
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. 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. Thank you for looking at it (and for having GPIO CS lines in your queue, which would unbreak the worst blocker)! virtually yours Gerhard Sittig
Hi Gerhard, 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. > 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? > Thank you for looking at it (and for having GPIO CS lines in your > queue, which would unbreak the worst blocker)! I really hope I would be able to test these patches. There are some organizational changes taking place at the moment that might prevent this. If you are interested I'll send you the current queue. baruch
On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote: > > Hi Gerhard, > > 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. 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. 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. > > 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... 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. 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. > > Thank you for looking at it (and for having GPIO CS lines in your > > queue, which would unbreak the worst blocker)! > > I really hope I would be able to test these patches. There are some > organizational changes taking place at the moment that might prevent this. If > you are interested I'll send you the current queue. Don't worry. No need to rush anything. virtually yours Gerhard Sittig
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
On Thu, Dec 26, 2013 at 09:00:15AM +0200, Baruch Siach wrote: > Cc: Feng Tang <feng.tang@intel.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> Applied, thanks.
Hi Gerhard, On Thu, 26 Dec 2013 16:33:46 +0100 Gerhard Sittig <gsi@denx.de> wrote: > On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote: > > 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. For gpio-CS spi slave devices, I don't have much experience. This dw-spi driver has worked with both low-speed uart and high-speed modem device with the internal CS register setting (no extern cs line) on our platforms (Medfield/Cloverview platforms) IIRC (I haven't touch this code for long time), some other developer contributed the "cs_control" callback to dw spi driver, and I guess it was for the external CS line. As myself have no HW to debug the external CS-line, any improvement in this area for the driver will be highly appreciated. > > > > 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... Actually quiet several spi controller drivers handle the delay_usecs this way, like the Blackfin, pl022, you can simply grep delay_usecs in drivers/spi/ I wrote this driver several years ago, and IIRC, the intention of skipping the udelay for last xfer is trying to save some udelay time, as switching to another spi_msg will surely take a while. I didn't see problem with it used by low-speed uart spi device or high speed modem on our platforms. But if you do see problem with this behavior, we can change it and also notify the other spi controllers who use the same method. Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 02, 2014 at 12:21:33AM +0800, Feng Tang wrote: > But if you do see problem with this behavior, we can change it and > also notify the other spi controllers who use the same method. All the drivers should be being converted to use transfer_one() (and the forthcoming stuff extending that) which will handle delay_usecs for them anyway.
Hi Mark, Feng, On Wed, Jan 01, 2014 at 05:32:49PM +0000, Mark Brown wrote: > On Thu, Jan 02, 2014 at 12:21:33AM +0800, Feng Tang wrote: > > But if you do see problem with this behavior, we can change it and > > also notify the other spi controllers who use the same method. > > All the drivers should be being converted to use transfer_one() (and the > forthcoming stuff extending that) which will handle delay_usecs for them > anyway. As I mentioned in the cover letter of this series, I have patches to convert this driver to the generic queue. If someone with access to hardware is willing to test, I'll post it. It is not clear yet whether I'll have access to hardware in the near future. baruch
On Wed, 1 Jan 2014 17:32:49 +0000 Mark Brown <broonie@kernel.org> wrote: > On Thu, Jan 02, 2014 at 12:21:33AM +0800, Feng Tang wrote: > > > But if you do see problem with this behavior, we can change it and > > also notify the other spi controllers who use the same method. > > All the drivers should be being converted to use transfer_one() (and the > forthcoming stuff extending that) which will handle delay_usecs for them > anyway. That's nice! These general thing should be taken care of by the spi core. Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
Cc: Feng Tang <feng.tang@intel.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/spi/spi-dw.c | 1 - drivers/spi/spi-dw.h | 1 - 2 files changed, 2 deletions(-)