| Message ID | 1415140617-2028-1-git-send-email-tthayer@opensource.altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2014-11-04 at 16:36 -0600, tthayer@opensource.altera.com wrote: > From: Thor Thayer <tthayer@opensource.altera.com> > > Changing SPI transfer speed using a utility such as spi_config with > spidev updates chip->speed_hz which is compared to the next transfer > speed. > > When speed_hz is not declared in a SPI transfer, the transfer speed is > not updated for the next read/write on /dev/spidevX.Y. The element > spi_transfer->speed_hz is filled with spi->max_speed_hz. The test of > if (transfer->speed_hz != speed) doesn't work because the chip->speed_hz > matches transfer->speed_hz and the clock divider is not updated. > > This fix: On each transfer update the clock divider, compare to the > previous clock divider and update if necessary. This fixes another > bug where the clock divider calculation at the top of the > pump_transfers() function could be an odd-number. > My intention is to use SPI core API as much as possible. Thus, pump_transfers() I think should be gone in future. Instead of doing an additional work can you provide a helper function to set speed_hz and call it from pump_transfers()? > Reported-by: Vlastimil Setka <setka@vsis.cz> > Signed-off-by: Vlastimil Setka <setka@vsis.cz> > Signed-off-by: Thor Thayer <tthayer@opensource.altera.com> > --- > drivers/spi/spi-dw.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c > index 72e12ba..3456b34 100644 > --- a/drivers/spi/spi-dw.c > +++ b/drivers/spi/spi-dw.c > @@ -376,9 +376,6 @@ static void pump_transfers(unsigned long data) > chip = dws->cur_chip; > spi = message->spi; > > - if (unlikely(!chip->clk_div)) > - chip->clk_div = dws->max_freq / chip->speed_hz; > - > if (message->state == ERROR_STATE) { > message->status = -EIO; > goto early_exit; > @@ -415,21 +412,27 @@ static void pump_transfers(unsigned long data) > > cr0 = chip->cr0; > > - /* Handle per transfer options for bpw and speed */ > - if (transfer->speed_hz) { > - speed = chip->speed_hz; > + /* Always calculate the desired clock divider */ > + speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz; > + > + if (speed > dws->max_freq) { > + dev_err(&spi->dev, "Unsupported SPI freq: %d Hz\n", speed); > + message->status = -EIO; > + goto early_exit; > + } > + > + /* clk_div doesn't support odd number */ > + clk_div = dws->max_freq / speed; > + clk_div = (clk_div + 1) & 0xfffe; > > - if (transfer->speed_hz != speed) { > - speed = transfer->speed_hz; > + /* Determine if the clock divider changed, if so update chip struct */ Maybe "…update speed_hz" ? > + if (clk_div != chip->clk_div) > + chip->clk_div = clk_div; > + else > + clk_div = 0; /* Prevent register programming below */ > > - /* clk_div doesn't support odd number */ > - clk_div = dws->max_freq / speed; > - clk_div = (clk_div + 1) & 0xfffe; > + chip->speed_hz = speed; > > - chip->speed_hz = speed; > - chip->clk_div = clk_div; > - } > - } > if (transfer->bits_per_word) { > bits = transfer->bits_per_word; > dws->n_bytes = dws->dma_width = bits >> 3;
On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer@opensource.altera.com wrote: > When speed_hz is not declared in a SPI transfer, the transfer speed is > not updated for the next read/write on /dev/spidevX.Y. The element Why is the behaviour of spidev relevant here, if there is a problem with spidev why is it being addressed in a specific driver? > spi_transfer->speed_hz is filled with spi->max_speed_hz. The test of > if (transfer->speed_hz != speed) doesn't work because the chip->speed_hz > matches transfer->speed_hz and the clock divider is not updated. In what way does the test "not work"? What should the test be doing and what is it actually doing? > + /* Always calculate the desired clock divider */ > + speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz; Please avoid using the ternery operator standalone, write a normal if to help people read the code. Though in this case the core should always ensure that there is a speed set on each transfer so I'm not clear when this test would ever use anything other than transfer->speed_hz anyway.
5.11.2014 11:54, Mark Brown: > On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer@opensource.altera.com wrote: > >> When speed_hz is not declared in a SPI transfer, the transfer speed is >> not updated for the next read/write on /dev/spidevX.Y. The element > Why is the behaviour of spidev relevant here, if there is a problem with > spidev why is it being addressed in a specific driver? You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case you can declare speed_hz per transfer, and everything is OK in spi-dw. If you do not declare speed_hz (default is 0) when using SPI_IOC_MESSAGE, or when you do just read/write on /dev/spidevX.Y, the spi_transfer->speed_hz is filled by spi->max_speed_hz (which is the value previously set by SPI_IOC_WR_MAX_SPEED_HZ). This triggers a problem in spi-dw. >> spi_transfer->speed_hz is filled with spi->max_speed_hz. The test of >> if (transfer->speed_hz != speed) doesn't work because the chip->speed_hz >> matches transfer->speed_hz and the clock divider is not updated. > In what way does the test "not work"? What should the test be doing and > what is it actually doing? Test: - set SPI clock to 100 000 Hz by SPI_IOC_WR_MAX_SPEED_HZ ioctl - write something to /dev/spidevX.Y (or use SPI_IOC_MESSAGE with zero speed_hz) - set SPI clock to 200 000 Hz by SPI_IOC_WR_MAX_SPEED_HZ ioctl - write something to /dev/spidevX.Y (or use SPI_IOC_MESSAGE with zero speed_hz) In last step, the clock should be 200 000 Hz, but actually it's 100 000 Hz. The reason is buggy condition which encloses chip->clk_div recalculation: if (transfer->speed_hz) { speed = chip->speed_hz; if (transfer->speed_hz != speed) { because transfer->speed_hz is filled by spi->max_speed_hz, which is equal to chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in dw_spi_setup): chip->speed_hz = spi->max_speed_hz = new_spi_speed > >> + /* Always calculate the desired clock divider */ >> + speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz; > Please avoid using the ternery operator standalone, write a normal if to > help people read the code. Though in this case the core should always > ensure that there is a speed set on each transfer so I'm not clear when > this test would ever use anything other than transfer->speed_hz anyway. Yes, as I can catch from code, the transfer->speed_hz should never be 0. But in current spi-dw.c version there is a `if (transfer->speed_hz)` condition, so I kept it in the patch. -- 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 Wed, Nov 05, 2014 at 01:07:36PM +0100, Vlastimil Šetka wrote: > 5.11.2014 11:54, Mark Brown: > >On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer@opensource.altera.com wrote: > >>When speed_hz is not declared in a SPI transfer, the transfer speed is > >>not updated for the next read/write on /dev/spidevX.Y. The element > >Why is the behaviour of spidev relevant here, if there is a problem with > >spidev why is it being addressed in a specific driver? > You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case you > can declare speed_hz per transfer, and everything is OK in spi-dw. You can declare a speed per transfer for *any* client, this is totally generic behaviour. What is the specific relevance of spidev here? > If you do not declare speed_hz (default is 0) when using SPI_IOC_MESSAGE, or > when you do just read/write on /dev/spidevX.Y, the spi_transfer->speed_hz is > filled by spi->max_speed_hz (which is the value previously set by > SPI_IOC_WR_MAX_SPEED_HZ). This triggers a problem in spi-dw. Again, this is something that any client could do... > The reason is buggy condition which encloses chip->clk_div recalculation: > if (transfer->speed_hz) { > speed = chip->speed_hz; > if (transfer->speed_hz != speed) { > because transfer->speed_hz is filled by spi->max_speed_hz, which is equal to > chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in dw_spi_setup): To repeat again: please talk about driver problems in terms of the driver not in terms of a particular driver. Glancing at the code here it looks like spidev is buggy here, it's just blindly allowing userspace to overwrite the maximum speed configured for the device which seems like a bad idea, drivers should have no reason to expect that something called max_speed is actually variable. It looks like spidev is abusing this as a default speed. > chip->speed_hz = spi->max_speed_hz = new_spi_speed So the driver is overriding its idea of the current speed without actually updatng the hardware. Why is the fix here not to just delete the assignment here, it seems fairly obviously buggy?
On 11/05/2014 03:57 AM, Andy Shevchenko wrote: > On Tue, 2014-11-04 at 16:36 -0600, tthayer@opensource.altera.com wrote: >> From: Thor Thayer <tthayer@opensource.altera.com> >> >> Changing SPI transfer speed using a utility such as spi_config with >> spidev updates chip->speed_hz which is compared to the next transfer >> speed. >> >> When speed_hz is not declared in a SPI transfer, the transfer speed is >> not updated for the next read/write on /dev/spidevX.Y. The element >> spi_transfer->speed_hz is filled with spi->max_speed_hz. The test of >> if (transfer->speed_hz != speed) doesn't work because the chip->speed_hz >> matches transfer->speed_hz and the clock divider is not updated. >> >> This fix: On each transfer update the clock divider, compare to the >> previous clock divider and update if necessary. This fixes another >> bug where the clock divider calculation at the top of the >> pump_transfers() function could be an odd-number. >> > My intention is to use SPI core API as much as possible. Thus, > pump_transfers() I think should be gone in future. Instead of doing an > additional work can you provide a helper function to set speed_hz and > call it from pump_transfers()? > Hi. I see your point but this patch may change significantly as a result of discussion. If some form of clock divider changes are implemented, I'm happy to make a helper function. Thanks for reviewing! > >> Reported-by: Vlastimil Setka <setka@vsis.cz> >> Signed-off-by: Vlastimil Setka <setka@vsis.cz> >> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com> >> --- >> drivers/spi/spi-dw.c | 33 ++++++++++++++++++--------------- >> 1 file changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c >> index 72e12ba..3456b34 100644 >> --- a/drivers/spi/spi-dw.c >> +++ b/drivers/spi/spi-dw.c >> @@ -376,9 +376,6 @@ static void pump_transfers(unsigned long data) >> chip = dws->cur_chip; >> spi = message->spi; >> >> - if (unlikely(!chip->clk_div)) >> - chip->clk_div = dws->max_freq / chip->speed_hz; >> - >> if (message->state == ERROR_STATE) { >> message->status = -EIO; >> goto early_exit; >> @@ -415,21 +412,27 @@ static void pump_transfers(unsigned long data) >> >> cr0 = chip->cr0; >> >> - /* Handle per transfer options for bpw and speed */ >> - if (transfer->speed_hz) { >> - speed = chip->speed_hz; >> + /* Always calculate the desired clock divider */ >> + speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz; >> + >> + if (speed > dws->max_freq) { >> + dev_err(&spi->dev, "Unsupported SPI freq: %d Hz\n", speed); >> + message->status = -EIO; >> + goto early_exit; >> + } >> + >> + /* clk_div doesn't support odd number */ >> + clk_div = dws->max_freq / speed; >> + clk_div = (clk_div + 1) & 0xfffe; >> >> - if (transfer->speed_hz != speed) { >> - speed = transfer->speed_hz; >> + /* Determine if the clock divider changed, if so update chip struct */ > Maybe "…update speed_hz" ? > >> + if (clk_div != chip->clk_div) >> + chip->clk_div = clk_div; >> + else >> + clk_div = 0; /* Prevent register programming below */ >> >> - /* clk_div doesn't support odd number */ >> - clk_div = dws->max_freq / speed; >> - clk_div = (clk_div + 1) & 0xfffe; >> + chip->speed_hz = speed; >> >> - chip->speed_hz = speed; >> - chip->clk_div = clk_div; >> - } >> - } >> if (transfer->bits_per_word) { >> bits = transfer->bits_per_word; >> dws->n_bytes = dws->dma_width = bits >> 3; > -- 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 11/05/2014 07:24 AM, Mark Brown wrote: > On Wed, Nov 05, 2014 at 01:07:36PM +0100, Vlastimil Šetka wrote: >> 5.11.2014 11:54, Mark Brown: >>> On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer@opensource.altera.com wrote: >>>> When speed_hz is not declared in a SPI transfer, the transfer speed is >>>> not updated for the next read/write on /dev/spidevX.Y. The element >>> Why is the behaviour of spidev relevant here, if there is a problem with >>> spidev why is it being addressed in a specific driver? >> You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case you >> can declare speed_hz per transfer, and everything is OK in spi-dw. > You can declare a speed per transfer for *any* client, this is totally > generic behaviour. What is the specific relevance of spidev here? spidev was just the vehicle that the issue was observed on and is useful for debugging. >> If you do not declare speed_hz (default is 0) when using SPI_IOC_MESSAGE, or >> when you do just read/write on /dev/spidevX.Y, the spi_transfer->speed_hz is >> filled by spi->max_speed_hz (which is the value previously set by >> SPI_IOC_WR_MAX_SPEED_HZ). This triggers a problem in spi-dw. > Again, this is something that any client could do... > >> The reason is buggy condition which encloses chip->clk_div recalculation: >> if (transfer->speed_hz) { >> speed = chip->speed_hz; >> if (transfer->speed_hz != speed) { >> because transfer->speed_hz is filled by spi->max_speed_hz, which is equal to >> chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in dw_spi_setup): > To repeat again: please talk about driver problems in terms of the > driver not in terms of a particular driver. Glancing at the code here > it looks like spidev is buggy here, it's just blindly allowing userspace > to overwrite the maximum speed configured for the device which seems > like a bad idea, drivers should have no reason to expect that something > called max_speed is actually variable. It looks like spidev is abusing > this as a default speed. spidev is calling the case condition SPI_IOC_WR_MAX_SPEED_HZ in spidev_ioctl() to overwrite the maximum speed. From the code and the name, it seems like overwriting the maximum speed was the intended use and not a side effect. >> chip->speed_hz = spi->max_speed_hz = new_spi_speed > So the driver is overriding its idea of the current speed without > actually updatng the hardware. Why is the fix here not to just delete > the assignment here, it seems fairly obviously buggy? Yes, removing line 591 of the spi-dw.c (chip->speed_hz = spi->max_speed_hz;) will solve the problem and seems like a clean fix. In this case the chip->speed_hz will persist the last transfer speed setting. The chip->speed_hz parameter won't be updated as part of the spi-dw driver initialization. This may not matter since it will get updated on the first transfer in the (transfer->speed != speed) test shown above. -- 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 Wed, Nov 05, 2014 at 02:25:59PM -0600, Thor Thayer wrote: Please leave blank lines between paragraphs and quoted bits, it makes things much easier to read than an unbroken wall of text covering many screenfuls. > On 11/05/2014 07:24 AM, Mark Brown wrote: > >To repeat again: please talk about driver problems in terms of the > >driver not in terms of a particular driver. Glancing at the code here > >it looks like spidev is buggy here, it's just blindly allowing userspace > >to overwrite the maximum speed configured for the device which seems > >like a bad idea, drivers should have no reason to expect that something > >called max_speed is actually variable. It looks like spidev is abusing > >this as a default speed. > spidev is calling the case condition SPI_IOC_WR_MAX_SPEED_HZ in > spidev_ioctl() to overwrite the maximum speed. From the code and the name, > it seems like overwriting the maximum speed was the intended use and not a > side effect. This means it was an API that was implemented thoughtlessly rather than anything else; it should be fixed to cache and use the current speed after one has been set. > >> chip->speed_hz = spi->max_speed_hz = new_spi_speed > >So the driver is overriding its idea of the current speed without > >actually updatng the hardware. Why is the fix here not to just delete > >the assignment here, it seems fairly obviously buggy? > Yes, removing line 591 of the spi-dw.c (chip->speed_hz = spi->max_speed_hz;) > will solve the problem and seems like a clean fix. In this case the > chip->speed_hz will persist the last transfer speed setting. > The chip->speed_hz parameter won't be updated as part of the spi-dw driver > initialization. This may not matter since it will get updated on the first > transfer in the (transfer->speed != speed) test shown above. OK, great - can you submit that version please?
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index 72e12ba..3456b34 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -376,9 +376,6 @@ static void pump_transfers(unsigned long data) chip = dws->cur_chip; spi = message->spi; - if (unlikely(!chip->clk_div)) - chip->clk_div = dws->max_freq / chip->speed_hz; - if (message->state == ERROR_STATE) { message->status = -EIO; goto early_exit; @@ -415,21 +412,27 @@ static void pump_transfers(unsigned long data) cr0 = chip->cr0; - /* Handle per transfer options for bpw and speed */ - if (transfer->speed_hz) { - speed = chip->speed_hz; + /* Always calculate the desired clock divider */ + speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz; + + if (speed > dws->max_freq) { + dev_err(&spi->dev, "Unsupported SPI freq: %d Hz\n", speed); + message->status = -EIO; + goto early_exit; + } + + /* clk_div doesn't support odd number */ + clk_div = dws->max_freq / speed; + clk_div = (clk_div + 1) & 0xfffe; - if (transfer->speed_hz != speed) { - speed = transfer->speed_hz; + /* Determine if the clock divider changed, if so update chip struct */ + if (clk_div != chip->clk_div) + chip->clk_div = clk_div; + else + clk_div = 0; /* Prevent register programming below */ - /* clk_div doesn't support odd number */ - clk_div = dws->max_freq / speed; - clk_div = (clk_div + 1) & 0xfffe; + chip->speed_hz = speed; - chip->speed_hz = speed; - chip->clk_div = clk_div; - } - } if (transfer->bits_per_word) { bits = transfer->bits_per_word; dws->n_bytes = dws->dma_width = bits >> 3;