Fix for imx-spi CS GPIOs

The driver will fail to load if no gpio chip selects are specified, this patch changes this so that it no longer fails.

It's possible to use all native chip selects, in which case there is no reason to have a gpio chip select array. This is what happens if the *optional* device tree property "cs-gpios" is omitted.

Quoted 3 lines · Trent Piepho

The driver will fail to load if no gpio chip selects are specified, this patch changes this so that it no longer fails.

Quoted 3 lines

It's possible to use all native chip selects, in which case there is no reason to have a gpio chip select array. This is what happens if the *optional* device tree property "cs-gpios" is omitted.

Do the native chip selects actually work usefully on this hardware? There used to be problems with it wanting to do things like bounce the chip select on every word which made it extremely difficult to use with Linux.

Quoted 4 lines · Mark Brown

Do the native chip selects actually work usefully on this hardware? There used to be problems with it wanting to do things like bounce the chip select on every word which made it extremely difficult to use with Linux.

Still are annoying, but on the device we have connected to it, it ends up working as desired.

I've not thoroughly investigated this hardware to find the details. IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it was a flaw in the driver and I was able to fix it. I've come to expect it, as every new SPI master I use doesn't work properly in some different way.

Quoted 5 lines · Trent Piepho

Quoted 4 lines · Mark Brown

Do the native chip selects actually work usefully on this hardware? There used to be problems with it wanting to do things like bounce the chip select on every word which made it extremely difficult to use with Linux.

Quoted 7 lines

Still are annoying, but on the device we have connected to it, it ends up working as desired.
I've not thoroughly investigated this hardware to find the details. IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it was a flaw in the driver and I was able to fix it. I've come to expect it, as every new SPI master I use doesn't work properly in some different way.

It's one of the reasons why I'm suspicous of making the GPIO optional, lots of chips use GPIO chipselects because a lot of the breakage is confined to chip select handling and I remember the i.MX as being especially far from useful.

Just to be clear, one doesn't need to use GPIOs with the driver as is. But the bindings to do that are non-standard and these patches make the driver follow the standard. (and don't break anyone).

It's unfortunate that this, and in my experience every, SPI master doesn't always (or ever) generate the waveforms it should according to the Linux API. But I don't think not following the specification for the device tree bindings is a mitigation of this. If anything, it just creates more problems.

If the goal is to document the hardware quirks, then shouldn't this be done through documentation? A note or pointer in the kconfig, something in the source, a better description in Documention/ somewhere, etc. That would have a better chance of being seen before hardware is designed, and would explain the issue too, instead of just appearing as another quirk in device tree bindings.

Quoted 3 lines · Trent Piepho

Just to be clear, one doesn't need to use GPIOs with the driver as is. But the bindings to do that are non-standard and these patches make the driver follow the standard. (and don't break anyone).

If there are non-standard bindings then mark them as deprecated. I can't immediately find *any* binding documentation for this controller. The last commit looks like it was more attempting to work round broken board bindings and do something sensible than add a new binding, at least that's what I remember my sense of it being.

If the hardware is as broken as these controllers always were in the past and there are workarounds which work in all practical situations (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

documenting something as supported is just going to make people miserable. The reason I know about this breakage is that I had to go through the process of working out that the native chip select support didn't work on a system.

Quoted 6 lines

If the goal is to document the hardware quirks, then shouldn't this be done through documentation? A note or pointer in the kconfig, something in the source, a better description in Documention/ somewhere, etc. That would have a better chance of being seen before hardware is designed, and would explain the issue too, instead of just appearing as another quirk in device tree bindings.

Yes, better documentation would be great.

Quoted 5 lines · Mark Brown

If there are non-standard bindings then mark them as deprecated. I can't immediately find *any* binding documentation for this controller. The last commit looks like it was more attempting to work round broken board bindings and do something sensible than add a new binding, at least that's what I remember my sense of it being.

The non-standard part is needing to add cs-gpios = <0> to get a native chip select when that is documented as being optional. It doesn't follow the spec. It doesn't match other drivers (and dw-spi is equally as broken in the same manner, probably others too) that do follow the spec.

Quoted 3 lines

If the hardware is as broken as these controllers always were in the past and there are workarounds which work in all practical situations (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

Comments in the driver indicate that some SoCs do not allow GPIO usage on all chip select pins.

Quoted 4 lines

documenting something as supported is just going to make people miserable. The reason I know about this breakage is that I had to go through the process of working out that the native chip select support didn't work on a system.

I just don't see how not following the device tree binding specification documents the hardware flaw, or how following the spec documents that the flaw does not exist.

Quoted 1 line

Yes, better documentation would be great.

How about I add something to Documentation/spi and add a note in Kconfig, but make the driver standard compliant in its device tree bindings?

Quoted 6 lines · Trent Piepho

Quoted 5 lines · Mark Brown

If there are non-standard bindings then mark them as deprecated. I can't immediately find *any* binding documentation for this controller. The last commit looks like it was more attempting to work round broken board bindings and do something sensible than add a new binding, at least that's what I remember my sense of it being.

Quoted 5 lines

The non-standard part is needing to add cs-gpios = <0> to get a native chip select when that is documented as being optional. It doesn't follow the spec. It doesn't match other drivers (and dw-spi is equally as broken in the same manner, probably others too) that do follow the spec.

Is there any documentation of the bindings for this driver at all? I wasn't able to find it.

Quoted 5 lines

Quoted 3 lines

If the hardware is as broken as these controllers always were in the past and there are workarounds which work in all practical situations (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

Comments in the driver indicate that some SoCs do not allow GPIO usage on all chip select pins.

Ah, that's an issue. We will need support for the hardware chip selects then.

Quoted 7 lines

Quoted 4 lines

documenting something as supported is just going to make people miserable. The reason I know about this breakage is that I had to go through the process of working out that the native chip select support didn't work on a system.

I just don't see how not following the device tree binding specification documents the hardware flaw, or how following the spec documents that the flaw does not exist.

Hardware chip selects aren't present in all controllers and at times have entertaining enumerations in the hardware, the details on them need to be covered in the device specific binding (which like I say seems to be missing for this controller). If they just don't work well the kindest thing may be to not support them and document the binding that way.

Quoted 4 lines

Quoted 1 line

Yes, better documentation would be great.

How about I add something to Documentation/spi and add a note in Kconfig, but make the driver standard compliant in its device tree bindings?

Writing a binding document for the controller would probably cover it.

Quoted 2 lines · Mark Brown

Is there any documentation of the bindings for this driver at all? I wasn't able to find it.

Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

Quoted 11 lines · Mark Brown

Quoted 3 lines · Trent Piepho

Just to be clear, one doesn't need to use GPIOs with the driver as is. But the bindings to do that are non-standard and these patches make the driver follow the standard. (and don't break anyone).

If there are non-standard bindings then mark them as deprecated. I can't immediately find *any* binding documentation for this controller. The last commit looks like it was more attempting to work round broken board bindings and do something sensible than add a new binding, at least that's what I remember my sense of it being.

The non-standard part is needing to add cs-gpios = <0> to get a native chip select when that is documented as being optional. It doesn't follow the spec. It doesn't match other drivers (and dw-spi is equally as broken in the same manner, probably others too) that do follow the spec.

Quoted 3 lines

If the hardware is as broken as these controllers always were in the past and there are workarounds which work in all practical situations (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

Comments in the driver indicate that some SoCs do not allow GPIO usage on all chip select pins.

Quoted 3 lines · Trent Piepho

Comments in the driver indicate that some SoCs do not allow GPIO usage on all chip select pins.

Which comments do you mean? I remember there were comments, but I can't find any in the driver.

The only SoC which has dedicated SPI CS pins which can't be configured as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be forced to use native chip selects.

If the array is not present, assume all chip selects are native. This is the standard behavior for SPI masters configured via the device tree and the behavior of this driver as well when it is configured via device tree.

This reduces platform data vs DT differences and allows most of the platform data based boards to remove their chip select arrays.

CC Shawn Guo <[email protected]>

CC Sascha Hauer <[email protected]>

CC Fabio Estevam <[email protected]>

CC Mark Brown <[email protected]>

Signed-off-by Trent Piepho <[email protected]>

---
 drivers/spi/spi-imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/spi/spi-imx.c +0 -0

index fea46cbf458a..535378ebab18 100644

drivers/spi/spi-imx.c +1 -1

 		master->cs_gpios = devm_kzalloc(&master->dev,
 			sizeof(int) * master->num_chipselect, GFP_KERNEL);
 		if (!master->cs_gpios)

@@ -1371,7 +1370,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 
 		for (i = 0; i < master->num_chipselect; i++)
 			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
- 	}
+	}
 
 	spi_imx->bitbang.chipselect = spi_imx_chipselect;
 	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;

Signature · 1 line

2.14.3