[U-Boot] [PATCH v2] dm: spi: prevent setting a speed of 0 Hz
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Signed-off-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com ---
Changes in v2: - create a define for the 100 kHz constant (used 3 times) - assign default value when binding new device and speed is 0 - assign default value when spi_slave_ofdata_to_platdata finds that spi-max-frequency property is missing
drivers/spi/spi-uclass.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index b84255bd27..2bc289a74c 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -15,6 +15,8 @@
DECLARE_GLOBAL_DATA_PTR;
+#define SPI_DEFAULT_SPEED_HZ 100000 + static int spi_set_speed_mode(struct udevice *bus, int speed, int mode) { struct dm_spi_ops *ops; @@ -58,7 +60,7 @@ int dm_spi_claim_bus(struct udevice *dev) speed = spi->max_hz; } if (!speed) - speed = 100000; + speed = SPI_DEFAULT_SPEED_HZ; if (speed != slave->speed) { int ret = spi_set_speed_mode(bus, speed, slave->mode);
@@ -300,7 +302,13 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, } plat = dev_get_parent_platdata(dev); plat->cs = cs; - plat->max_hz = speed; + if (speed) { + plat->max_hz = speed; + } else { + printf("Warning: SPI speed fallback to %u kHz\n", + SPI_DEFAULT_SPEED_HZ / 1000); + plat->max_hz = SPI_DEFAULT_SPEED_HZ; + } plat->mode = mode; created = true; } else if (ret) { @@ -374,7 +382,8 @@ int spi_slave_ofdata_to_platdata(struct udevice *dev, int value;
plat->cs = dev_read_u32_default(dev, "reg", -1); - plat->max_hz = dev_read_u32_default(dev, "spi-max-frequency", 0); + plat->max_hz = dev_read_u32_default(dev, "spi-max-frequency", + SPI_DEFAULT_SPEED_HZ); if (dev_read_bool(dev, "spi-cpol")) mode |= SPI_CPOL; if (dev_read_bool(dev, "spi-cpha"))
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Simon
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Simon
On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/master, thanks!
On Fri, Nov 23, 2018 at 1:51 AM sjg@google.com wrote:
On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/master, thanks!
I hold this because, there is a discussion going on with me and Simon.k about Linux compatible "jedec,spi-nor". adding support for this binding would eventually not reproduce this issue.
[Responding from my work address due to problems with gmail]
On 23.11.2018 13:27, Jagan Teki wrote:
On Fri, Nov 23, 2018 at 1:51 AM sjg@google.com wrote:
On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
When the device tree is missing a correct spi slave description below the bus (compatible "spi-flash" or spi-max-frequency are missing), the 'set_speed' callback can be called with 'speed' == 0 Hz. At least with cadence qspi, this leads to a division by zero.
Prevent this by initializing speed to 100 kHz in this case (same fallback value as is done in 'dm_spi_claim_bus') and issue a warning to console.
Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/master, thanks!
I hold this because, there is a discussion going on with me and Simon.k about Linux compatible "jedec,spi-nor". adding support for this binding would eventually not reproduce this issue.
That's not completely true. Adding "jedec,spi-nor" to U-Boot would definitively make it less likely to run into this issue.
However, Linux works correctly without "jedec,spi-nor", so people can still run into problems when porting a dts from Linux. This is why I still would like to get this in.
Simon
On Fri, Nov 23, 2018 at 6:24 PM Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com wrote:
[Responding from my work address due to problems with gmail]
On 23.11.2018 13:27, Jagan Teki wrote:
On Fri, Nov 23, 2018 at 1:51 AM sjg@google.com wrote:
On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote:
On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote: > When the device tree is missing a correct spi slave description below > the bus (compatible "spi-flash" or spi-max-frequency are missing), > the 'set_speed' callback can be called with 'speed' == 0 Hz. > At least with cadence qspi, this leads to a division by zero. > > Prevent this by initializing speed to 100 kHz in this case (same > fallback value as is done in 'dm_spi_claim_bus') and issue a warning > to console. Why can't driver plat->frequency in cadence driver initialize 100KH? plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000)
I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/master, thanks!
I hold this because, there is a discussion going on with me and Simon.k about Linux compatible "jedec,spi-nor". adding support for this binding would eventually not reproduce this issue.
That's not completely true. Adding "jedec,spi-nor" to U-Boot would definitively make it less likely to run into this issue.
I don't this is what we discussed above, adding the Linux compatible with relevant spi-frequnecy make the issue non-reproducible this is what exactly we ended-up.
However, Linux works correctly without "jedec,spi-nor", so people can still run into problems when porting a dts from Linux. This is why I still would like to get this in.
Why? do they specify property?
On 23.11.2018 14:12, Jagan Teki wrote:
On Fri, Nov 23, 2018 at 6:24 PM Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com wrote:
[Responding from my work address due to problems with gmail]
On 23.11.2018 13:27, Jagan Teki wrote:
On Fri, Nov 23, 2018 at 1:51 AM sjg@google.com wrote:
On Fri, 16 Nov 2018 at 12:40, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 31.10.2018 07:42, Simon Goldschmidt wrote:
On Wed, Oct 31, 2018 at 7:22 AM Jagan Teki jagan@amarulasolutions.com wrote: > On Wed, Oct 31, 2018 at 1:40 AM Simon Goldschmidt > simon.k.r.goldschmidt@gmail.com wrote: >> When the device tree is missing a correct spi slave description below >> the bus (compatible "spi-flash" or spi-max-frequency are missing), >> the 'set_speed' callback can be called with 'speed' == 0 Hz. >> At least with cadence qspi, this leads to a division by zero. >> >> Prevent this by initializing speed to 100 kHz in this case (same >> fallback value as is done in 'dm_spi_claim_bus') and issue a warning >> to console. > Why can't driver plat->frequency in cadence driver initialize 100KH? > plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", 100000) I'm not sure I understand. The cadence driver initializes its 'plat->max_hz' from "spi-max-frequency" property and defaults to 500 kHz. No problem there.
However, the problem I want to solve is if someone puts a flash chip below there which has compatible != "spi-flash", they will get a hard fault which is hard to debug. This is because the node is not parsed because of the wrong compatible string (even if there is an "spi-max-frequency" property) and thus, "sf probe" tries to continue with 0 Hz.
And this can happen easily when porting device trees from Linux as there, the boards have compatible "n25q00" etc. instead of "spi-flash", which is U-Boot specific (sadly).
This patch is not required to make valid U-Boot devicetrees work, it is meant to get better error handling for devicetrees ported from Linux.
An even better fix would be for U-Boot not to require the compatible = "spi-flash" string but just work correctly with Linux device trees, but that's not within my possibilities right now :-(
Ping? This is a bug that should hit nearly everyone porting a board dts working in Linux to U-Boot, can we please have some kind of fix for this? Currently, using a dts with an spi flash chip that works in Linux may abort in U-Boot...
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm/master, thanks!
I hold this because, there is a discussion going on with me and Simon.k about Linux compatible "jedec,spi-nor". adding support for this binding would eventually not reproduce this issue.
That's not completely true. Adding "jedec,spi-nor" to U-Boot would definitively make it less likely to run into this issue.
I don't this is what we discussed above, adding the Linux compatible with relevant spi-frequnecy make the issue non-reproducible this is what exactly we ended-up.
However, Linux works correctly without "jedec,spi-nor", so people can still run into problems when porting a dts from Linux. This is why I still would like to get this in.
Why? do they specify property?
There are dts files with compatible = "n25q256a" and others. I don't find the thread right now, but I said I would try to upstream changes to Linux so that the socfgpa_cyclone5 dts files would have an additional "jedec,spi-nor" compatible strings.
However, I cannot change all Linux devicetress and having "jedec-spi-nor" does *not* seem mandatory for Linux devicetrees.
This patch ensures a devicetree that works with Linux does not lead to a "divide-by-zero" abort with U-Boot. I do think that's an improvement for U-Boot!
Regards, Simon
participants (5)
-
Jagan Teki -
Simon Glass -
Simon Goldschmidt -
Simon Goldschmidt -
sjg＠google.com