| Message ID | 20171027010841.28624-2-tpiepho@impinj.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote: > 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. 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.
On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote: > On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote: > > 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. > > 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.
On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote: > On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote: > > 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. 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.
On Thu, 2017-11-02 at 15:14 +0000, Mark Brown wrote: > On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote: > > On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote: > > > 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. > > 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. As someone who has done bringup a more than a few devices, I know what I want the hardware to do, and I know the proper bindings to do that. But someone comes back and says they don't work and now I need to dig into the driver source and figure out what its particular flavor of non-standard behavior is. I don't think that helped me any. It didn't tell what the hardware/driver's quirks are w.r.t. ability to follow the Linux SPI API either. This also happens after the hardware is designed and built, so it's a little late to choose another pin. 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. Also consider there is a lot of re-implemented code in each driver relating to device tree parsing and also GPIO CS. Non-standard behavior in a driver doesn't help any refactoring effort.
On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote: > 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. > 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 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. > 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.
On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote: > On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote: > > > 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. > 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. > 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. > > 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. 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? The goal here is that everyone doesn't have to stick a scope on the board and re-discover that the native CS doesn't do what they want, right? Been there, done that, and can appreciate the sentiment. I don't think not following the standard is an effective way to do that. From a sample of three developers, two came up with dt bindings to use native cs and decided they apparently misunderstood the spi dt spec to explain why what should have worked did not. The third (me) looked into the driver source to find the why, and even then it wasn't clear the behavior was intentional or an oversight. I think documenting the known flaws would be a better and more effective way to get that information across.
On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote: > > 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. Is there any documentation of the bindings for this driver at all? I wasn't able to find it. > > 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. > > 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. > > 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.
On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote: > On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote: > > > 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. > > 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 Doesn't note anything special about the native chip selects. > > > 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. And they are already supported too. The issue I want to fix is the need for non-standard bindings to use them. > > > 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. > > > > 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. Ok, I'll adjust the series to document the real issue.
On Fri, Nov 03, 2017 at 08:09:19PM +0000, Trent Piepho wrote: > On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote: > > 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 Ah, good - for some reason I'd thought that was a different IP block (SPI controllers are like serial ports, endless room for innovation!).
On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote: > > On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote: > > > > > 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. > > > 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. 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. Sascha
On Mon, 2017-11-06 at 08:32 +0100, Sascha Hauer wrote: > On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > > > > 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. It's in arch/arm/plat-mxc/include/mach/spi.h. > 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. Indeed I'm not forced to, but native is more efficient and faster, and since I'm lucky enough that they work for me, I'd like to be able to use them. And I can already, but the DT bindings are non-standard and I don't see any value in that. DT bindings are complex enough without drivers adding quirks. And my experience was no one connected the non- standard bindings with native CS not working well. I think explicit documentation will be more effective at that.
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index babb15f07995..07e6250f2dad 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -1457,22 +1457,19 @@ static int spi_imx_probe(struct platform_device *pdev) goto out_clk_put; } - if (!master->cs_gpios) { - dev_err(&pdev->dev, "No CS GPIOs available\n"); - ret = -EINVAL; - goto out_clk_put; - } - - for (i = 0; i < master->num_chipselect; i++) { - if (!gpio_is_valid(master->cs_gpios[i])) - continue; - - ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i], - DRIVER_NAME); - if (ret) { - dev_err(&pdev->dev, "Can't get CS GPIO %i\n", - master->cs_gpios[i]); - goto out_clk_put; + /* Request GPIO CS lines, if any */ + if (master->cs_gpios) { + for (i = 0; i < master->num_chipselect; i++) { + if (!gpio_is_valid(master->cs_gpios[i])) + continue; + + ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i], + DRIVER_NAME); + if (ret) { + dev_err(&pdev->dev, "Can't get CS GPIO %i\n", + master->cs_gpios[i]); + goto out_clk_put; + } } }