| Message ID | 20191024110757.25820-4-alvaro.gamez@hazent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: set bits_per_word based on controller's bits_per_word_mask | expand |
On Thu, Oct 24, 2019 at 01:07:57PM +0200, Alvaro Gamez Machado wrote: > By leaving this value unset, a default value of 8 was being set later on. > > If it happens that the SPI master driver doesn't support this value of 8, > there will be an initial inconsistency between the SPI master and the device > itself. This isn't a problem for most devices because kernel drivers This will break things, client devices are working on the basis that the default transfer width is 8 bits. As I've repeatedly said if we have different parts of the system with different ideas about the word size we're going to end up with data corruption. Please take this feedback on board.
On Thu, Oct 24, 2019 at 12:13:00PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 01:07:57PM +0200, Alvaro Gamez Machado wrote: > > By leaving this value unset, a default value of 8 was being set later on. > > > > If it happens that the SPI master driver doesn't support this value of 8, > > there will be an initial inconsistency between the SPI master and the device > > itself. This isn't a problem for most devices because kernel drivers > > This will break things, client devices are working on the basis that the > default transfer width is 8 bits. As I've repeatedly said if we have > different parts of the system with different ideas about the word size > we're going to end up with data corruption. Please take this feedback > on board. Oh, ok. I didn't understand this cleary from previous mails, now I see what you mean. I think then the only way this would be feasible is to check if 8 bits is an acceptable number for the master and, if it isn't, apply the lowest available data width. I believe this cannot break anything, as it leaves 8 as the default unless the master can't work with that number, in which case it really doesn't matter what client device wants because the hardware can't provide it. Thanks! diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 794e20e54237..4e26ac79e133 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -3079,8 +3079,12 @@ int spi_setup(struct spi_device *spi) return -EINVAL; } - if (!spi->bits_per_word) - spi->bits_per_word = 8; + if (!spi->bits_per_word) { + if (spi->controller->bits_per_word_mask & SPI_BPW_MASK(8)) + spi->bits_per_word = 8; + else + spi->bits_per_word = ffs(spi->controller->bits_per_word_mask); + } status = __spi_validate_bits_per_word(spi->controller, spi->bits_per_word);
On Thu, Oct 24, 2019 at 02:54:37PM +0200, Alvaro Gamez Machado wrote: > I think then the only way this would be feasible is to check if 8 bits is an > acceptable number for the master and, if it isn't, apply the lowest > available data width. I believe this cannot break anything, as it leaves 8 > as the default unless the master can't work with that number, in which case > it really doesn't matter what client device wants because the hardware can't > provide it. No, that still leaves the slave driver thinking it's sending 8 bits when really it's sending something else - the default is just 8 bits, if the controller can't do it then the transfer can't happen and there's an error. It's not a good idea to carry on if we're likely to introduce data corruption.
On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 02:54:37PM +0200, Alvaro Gamez Machado wrote: > > > I think then the only way this would be feasible is to check if 8 bits is an > > acceptable number for the master and, if it isn't, apply the lowest > > available data width. I believe this cannot break anything, as it leaves 8 > > as the default unless the master can't work with that number, in which case > > it really doesn't matter what client device wants because the hardware can't > > provide it. > > No, that still leaves the slave driver thinking it's sending 8 bits when > really it's sending something else - the default is just 8 bits, if the > controller can't do it then the transfer can't happen and there's an > error. It's not a good idea to carry on if we're likely to introduce > data corruption. Well, yes. But I don't think that's a software issue but a hardware one. If you have a board that has a SPI master that cannot talk to an 8 bits device and you expect to communicate with anything that accepts 8 bits you're not going to be able to. Either the kernel raises an error or it shuts up and tries its best. I understand the first option is better, but I also think that's not a software issue, that hardware simply cannot work as is regardless of what we do in software. The hardware devices simply can't talk to each other.
On Thu, Oct 24, 2019 at 03:18:57PM +0200, Alvaro Gamez Machado wrote: > On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote: > > No, that still leaves the slave driver thinking it's sending 8 bits when > > really it's sending something else - the default is just 8 bits, if the > > controller can't do it then the transfer can't happen and there's an > > error. It's not a good idea to carry on if we're likely to introduce > > data corruption. > Well, yes. But I don't think that's a software issue but a hardware one. > If you have a board that has a SPI master that cannot talk to an 8 bits > device and you expect to communicate with anything that accepts 8 bits > you're not going to be able to. Either the kernel raises an error or it > shuts up and tries its best. I understand the first option is better, but I > also think that's not a software issue, that hardware simply cannot work as > is regardless of what we do in software. The hardware devices simply can't > talk to each other. Sure, but then it's going to be easier to diagnose problems if the software says that it's identified a data format problem than it is to try to figure out the results of data corruption. There is also the possibility that if the formats the hardware needs to use can be made to align through rewriting software can persuade things to interoperate (eg, if all the transfers are multiples of 32 bits then a device can probably work with a controller that only supports 32 bit words even if the device expects a byte stream) but that requires explicit code rather than just silently accepting the data and hoping for the best.
On Thu, Oct 24, 2019 at 02:41:16PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 03:18:57PM +0200, Alvaro Gamez Machado wrote: > > On Thu, Oct 24, 2019 at 02:11:29PM +0100, Mark Brown wrote: > > > > No, that still leaves the slave driver thinking it's sending 8 bits when > > > really it's sending something else - the default is just 8 bits, if the > > > controller can't do it then the transfer can't happen and there's an > > > error. It's not a good idea to carry on if we're likely to introduce > > > data corruption. > > > Well, yes. But I don't think that's a software issue but a hardware one. > > > If you have a board that has a SPI master that cannot talk to an 8 bits > > device and you expect to communicate with anything that accepts 8 bits > > you're not going to be able to. Either the kernel raises an error or it > > shuts up and tries its best. I understand the first option is better, but I > > also think that's not a software issue, that hardware simply cannot work as > > is regardless of what we do in software. The hardware devices simply can't > > talk to each other. > > Sure, but then it's going to be easier to diagnose problems if the > software says that it's identified a data format problem than it is to > try to figure out the results of data corruption. There is also the > possibility that if the formats the hardware needs to use can be made to > align through rewriting software can persuade things to interoperate > (eg, if all the transfers are multiples of 32 bits then a device can > probably work with a controller that only supports 32 bit words even if > the device expects a byte stream) but that requires explicit code rather > than just silently accepting the data and hoping for the best. I guess there could be some workarounds to help in that situation. But I see that as an hypothetical occurrence whereas I have with me a physical board that needs 32 bits in both master and slave that I want to access using spidev and cannot. Lots of I's in that sentence, I know :) Anyhow, if this is not acceptable, the only other alternative I see right now is adding a new DT property, as in my emails yesterday. diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index f9502dbbb5c1..06424b7b0d73 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1792,6 +1792,10 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, } spi->max_speed_hz = value; + /* Bits per word */ + if (!of_property_read_u32(nc, "spi-bits-per-word", &value)) + spi->bits_per_word = value; + return 0; } What do you think about this? This requires the user to explicitly select a different value rather than the default, so it can't break anything and would allow with the diagnostic of such broken hardware. I still think I like more changing the default to something the master is able to do. Otherwise we're going to keep trying to send 8 bits using a master that simply cannot do that, but this solution would work fine as well. Regards,
On Thu, Oct 24, 2019 at 04:07:32PM +0200, Alvaro Gamez Machado wrote: > I guess there could be some workarounds to help in that situation. But I see > that as an hypothetical occurrence whereas I have with me a physical board > that needs 32 bits in both master and slave that I want to access using > spidev and cannot. Lots of I's in that sentence, I know :) If you want to access this using spidev then add support for changing the word size to spidev and use that as I think Geert already suggested.
On Thu, Oct 24, 2019 at 06:40:33PM +0100, Mark Brown wrote: > On Thu, Oct 24, 2019 at 04:07:32PM +0200, Alvaro Gamez Machado wrote: > > > I guess there could be some workarounds to help in that situation. But I see > > that as an hypothetical occurrence whereas I have with me a physical board > > that needs 32 bits in both master and slave that I want to access using > > spidev and cannot. Lots of I's in that sentence, I know :) > > If you want to access this using spidev then add support for changing > the word size to spidev and use that as I think Geert already suggested. I've been trying to do so for a couple hours and I've reached a conclusion. I've been too focused on my personal use case (too many I's indeed) and thought that the problem was in fact in spidev as Geert indicated, but now I think it isn't, so I must present my excuses for mistakenly drive the conversation in that direction. Geert also thought this could be an SPI core bug, and he was right on that, I think. In fact, not a single line of spidev is being executed when the error message is printed: xilinx_spi 44a00000.spi: at 0x44A00000 mapped to 0x(ptrval), irq=3 xilinx_spi 44a10000.spi: can't setup spi1.0, status -22 spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0 spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0 This does not come from spidev but directly from spi. What is happening is that when SPI slaves are defined via DT, their bits_per_word value is always unset (as 0), which turns later on in a default value of 8. Inside spi_setup function immediately after setting this default value to 8, __spi_validate_bits_per_word is called, that checks whether bits_per_word for the slave matches the controller available bitwidths: if (!spi->bits_per_word) spi->bits_per_word = 8; status = __spi_validate_bits_per_word(spi->controller, spi->bits_per_word); if (status) return status; This means that it doesn't really matter which is the driver that is going to claim the specific SPI slave. It may be spidev as in my use case, or it may really be any other driver. But its probe() function is never going to be called because the error is not raised inside the driver, but immediately after forcibly setting the default value to 8 in spi.c I can't modify spidev because spidev doesn't even know this is happening. I was completely wrong in my blaming of spidev, but now I'm reassured that my previous patches were going to core of the issue, regardless of my mistaken initial diagnostic. Thanks,
On Fri, Oct 25, 2019 at 08:39:48AM +0200, Alvaro Gamez Machado wrote: > to claim the specific SPI slave. It may be spidev as in my use case, or it > may really be any other driver. But its probe() function is never going to > be called because the error is not raised inside the driver, but immediately > after forcibly setting the default value to 8 in spi.c Then you need to extend the validation the core is doing here to skip this parameter when registering the device and only enforce it after a driver is bound, we don't have a driver at the time we initially register the device so we can't enforce this. > I can't modify spidev because spidev doesn't even know this is happening. You are, at some point, going to need to set your spidev to 32 bits per word (spidev does already support this).
On Fri, Oct 25, 2019 at 12:56:55PM +0100, Mark Brown wrote: > On Fri, Oct 25, 2019 at 08:39:48AM +0200, Alvaro Gamez Machado wrote: > > > to claim the specific SPI slave. It may be spidev as in my use case, or it > > may really be any other driver. But its probe() function is never going to > > be called because the error is not raised inside the driver, but immediately > > after forcibly setting the default value to 8 in spi.c > > Then you need to extend the validation the core is doing here to > skip this parameter when registering the device and only enforce > it after a driver is bound, we don't have a driver at the time we > initially register the device so we can't enforce this. Ok, so I can remove the call to __spi_validate_bits_per_word() from spi_setup(), and instead place it on spi_drv_probe after calling spi_drv_probe() and detach the device if it returns with an error. The behaviour would be basically the same as it is now, but the slave driver will have an opportunity to choose a different transfer width if it wants to and can match the capabilities of the master. > > > I can't modify spidev because spidev doesn't even know this is happening. > > You are, at some point, going to need to set your spidev to 32 > bits per word (spidev does already support this). If I do the former, spidev will then need to load the initial bits per word value from DT before returning from probe(), otherwise it will end up detached when the check above is performed. Should I go for this? Thanks
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index f9502dbbb5c1..794e20e54237 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1792,6 +1792,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, } spi->max_speed_hz = value; + spi->bits_per_word = ffs(ctlr->bits_per_word_mask); + return 0; }
By leaving this value unset, a default value of 8 was being set later on. If it happens that the SPI master driver doesn't support this value of 8, there will be an initial inconsistency between the SPI master and the device itself. This isn't a problem for most devices because kernel drivers associated to any device set the correct value themselves, but for device drivers that do not change this value (such as spidev that provides a userspace accesible device, which doesn't and can't know the value it has to use), an error is raised: xilinx_spi 44a10000.spi: can't setup spi1.0, status -22 spi_master spi1: spi_device register error /amba_pl/spi@44a10000/spidev@0 spi_master spi1: Failed to create SPI device for /amba_pl/spi@44a10000/spidev@0 When this happens, the expected /dev/spidevX.Y device isn't created, and thus the user can't use the SPI_IOC_WR_BITS_PER_WORD ioctl to set the desired value. This change sets the initial value of bits_per to the smallest word that the controller allows, so of_spi_parse_dt sets a value that is usable by the controller. Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com> --- drivers/spi/spi.c | 2 ++ 1 file changed, 2 insertions(+)