spi: set bits_per_word based on controller's bits_per_word_mask
diff mbox series

Message ID 20191024110757.25820-4-alvaro.gamez@hazent.com
State New
Headers show
Series
  • spi: set bits_per_word based on controller's bits_per_word_mask
Related show

Commit Message

Alvaro Gamez Machado Oct. 24, 2019, 11:07 a.m. UTC
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(+)

Comments

Mark Brown Oct. 24, 2019, 11:13 a.m. UTC | #1
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.
Alvaro Gamez Machado Oct. 24, 2019, 12:54 p.m. UTC | #2
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);
Mark Brown Oct. 24, 2019, 1:11 p.m. UTC | #3
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.
Alvaro Gamez Machado Oct. 24, 2019, 1:18 p.m. UTC | #4
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.
Mark Brown Oct. 24, 2019, 1:41 p.m. UTC | #5
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.
Alvaro Gamez Machado Oct. 24, 2019, 2:07 p.m. UTC | #6
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,
Mark Brown Oct. 24, 2019, 5:40 p.m. UTC | #7
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.
Alvaro Gamez Machado Oct. 25, 2019, 6:39 a.m. UTC | #8
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,
Mark Brown Oct. 25, 2019, 11:56 a.m. UTC | #9
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).
Alvaro Gamez Machado Oct. 28, 2019, 9:43 a.m. UTC | #10
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

Patch
diff mbox series

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;
 }