| Message ID | 20180118081543.26412-1-sgoldschmidt@de.pepperl-fuchs.com |
|---|---|
| State | Rejected |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot] dm: spi: prevent setting a speed of 0 Hz | expand |
Hi On Thu, Jan 18, 2018 at 9:15 AM, Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> wrote: > When the device tree is missing a correct spi slave description below > the bus, 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, as is > done in 'dm_spi_claim_bus'. > > Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> > --- > > drivers/spi/spi-uclass.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c > index e06a603ab1..41ecef77db 100644 > --- a/drivers/spi/spi-uclass.c > +++ b/drivers/spi/spi-uclass.c > @@ -325,6 +325,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > if (!speed) { > speed = plat->max_hz; > mode = plat->mode; > + if (!speed) > + speed = 100000; You should add a warming message Michael > } > ret = spi_set_speed_mode(bus, speed, mode); > if (ret) > -- > 2.11.0 > > > Pepperl+Fuchs GmbH, Mannheim > Geschaeftsfuehrer/Managing Directors: Dr.-Ing. Gunther Kegel (Vors./CEO), Werner Guthier, Mehmet Hatiboglu > Vorsitzender des Aufsichtsrats/Chairman of the supervisory board: Claus Michael > Registergericht/Register Court: AG Mannheim HRB 4713 > > Wichtiger Hinweis: > Diese E-Mail einschliesslich ihrer Anhaenge enthaelt vertrauliche und rechtlich geschuetzte Informationen, die nur fuer den Adressaten bestimmt sind. > Sollten Sie nicht der bezeichnete Adressat sein, so teilen Sie dies bitte dem Absender umgehend mit und loeschen Sie diese Nachricht und ihre Anhaenge. Die unbefugte Weitergabe, das Anfertigen von Kopien und jede Veraenderung der E-Mail ist untersagt. Der Absender haftet nicht fuer Inhalte von veraenderten E-Mails. > > > Important Information: > This e-mail message including its attachments contains confidential and legally protected information solely intended for the addressee. If you are not the intended addressee of this message, please contact the addresser immediately and delete this message including its attachments. The unauthorized dissemination, copying and change of this e-mail are strictly forbidden. The addresser shall not be liable for the content of such changed e-mails. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On 18.01.2018 09:23, Michael Nazzareno Trimarchi wrote: > Hi > > On Thu, Jan 18, 2018 at 9:15 AM, Simon Goldschmidt > <sgoldschmidt@de.pepperl-fuchs.com> wrote: >> When the device tree is missing a correct spi slave description below >> the bus, 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, as is >> done in 'dm_spi_claim_bus'. >> >> Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> >> --- >> >> drivers/spi/spi-uclass.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >> index e06a603ab1..41ecef77db 100644 >> --- a/drivers/spi/spi-uclass.c >> +++ b/drivers/spi/spi-uclass.c >> @@ -325,6 +325,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> if (!speed) { >> speed = plat->max_hz; >> mode = plat->mode; >> + if (!speed) >> + speed = 100000; > You should add a warming message Hmm, I just copied what 'dm_spi_claim_bus' does some hundred lines above in the same file. Why should one code path warn when the other doesn't? Simon > > Michael > >> } >> ret = spi_set_speed_mode(bus, speed, mode); >> if (ret) >> -- >> 2.11.0 <snip>
Hi On Thu, Jan 18, 2018 at 9:27 AM, Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> wrote: > On 18.01.2018 09:23, Michael Nazzareno Trimarchi wrote: >> >> Hi >> >> On Thu, Jan 18, 2018 at 9:15 AM, Simon Goldschmidt >> <sgoldschmidt@de.pepperl-fuchs.com> wrote: >>> >>> When the device tree is missing a correct spi slave description below >>> the bus, 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, as is >>> done in 'dm_spi_claim_bus'. >>> >>> Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> >>> --- >>> >>> drivers/spi/spi-uclass.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >>> index e06a603ab1..41ecef77db 100644 >>> --- a/drivers/spi/spi-uclass.c >>> +++ b/drivers/spi/spi-uclass.c >>> @@ -325,6 +325,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, >>> int mode, >>> if (!speed) { >>> speed = plat->max_hz; >>> mode = plat->mode; >>> + if (!speed) >>> + speed = 100000; >> >> You should add a warming message > > > Hmm, I just copied what 'dm_spi_claim_bus' does some hundred lines above in > the same file. Why should one code path warn when the other doesn't? I just check this page but if you have some missing definition in device tree maybe make sense to make it visible Michael > > Simon > >> >> Michael >> >>> } >>> ret = spi_set_speed_mode(bus, speed, mode); >>> if (ret) >>> -- >>> 2.11.0 > > <snip> >
Hi Simon, On 18 January 2018 at 01:15, Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> wrote: > When the device tree is missing a correct spi slave description below > the bus, 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, as is > done in 'dm_spi_claim_bus'. > > Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> > --- > > drivers/spi/spi-uclass.c | 2 ++ > 1 file changed, 2 insertions(+) > Another option is to have a sensible default when reading from the DT fails. See spi_slave_ofdata_to_platdata() - you can add the default there. Would that work? Regards, Simon
On Thursday 18 January 2018 01:45 PM, Simon Goldschmidt wrote: > When the device tree is missing a correct spi slave description below > the bus, 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, as is > done in 'dm_spi_claim_bus'. > As per doc/device-tree-bindings/spi/spi-bus.txt, spi-max-frequency - (required) Maximum SPI clocking speed of device in Hz IMO, the correct fix would be to error out with proper warning, if spi-max-frequency property is absent in DT. > Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> > --- > > drivers/spi/spi-uclass.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c > index e06a603ab1..41ecef77db 100644 > --- a/drivers/spi/spi-uclass.c > +++ b/drivers/spi/spi-uclass.c > @@ -325,6 +325,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > if (!speed) { > speed = plat->max_hz; > mode = plat->mode; > + if (!speed) > + speed = 100000; > } > ret = spi_set_speed_mode(bus, speed, mode); > if (ret) >
On Thu, Jan 18, 2018 at 1:45 PM, Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> wrote: > When the device tree is missing a correct spi slave description below > the bus, 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, as is > done in 'dm_spi_claim_bus'. Better go-with default baudrate if speed == 0 this what usually does in remaining drivers.
On 22.01.2018 01:29, Simon Glass wrote: > Hi Simon, > > On 18 January 2018 at 01:15, Simon Goldschmidt > <sgoldschmidt@de.pepperl-fuchs.com> wrote: >> When the device tree is missing a correct spi slave description below >> the bus, 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, as is >> done in 'dm_spi_claim_bus'. >> >> Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> >> --- >> >> drivers/spi/spi-uclass.c | 2 ++ >> 1 file changed, 2 insertions(+) >> > Another option is to have a sensible default when reading from the DT > fails. See spi_slave_ofdata_to_platdata() - you can add the default > there. > > Would that work? This seems like a good idea, but I'm not sure it fixes my 'divide-by-zero' bug because that bug also triggered if the subnode of my spi controller was missing the compatible field for 'spi-flash'. I'd have to check that. Regards, Simon
On 22.01.2018 06:04, Vignesh R wrote: > On Thursday 18 January 2018 01:45 PM, Simon Goldschmidt wrote: >> When the device tree is missing a correct spi slave description below >> the bus, 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, as is >> done in 'dm_spi_claim_bus'. >> > As per doc/device-tree-bindings/spi/spi-bus.txt, > spi-max-frequency - (required) Maximum SPI clocking speed of device in Hz > > IMO, the correct fix would be to error out with proper warning, if > spi-max-frequency property is absent in DT. By now, I think so, too. Fallback to 100.000 Hz really hurts when loading a 16 MByte fit image from qspi. A warning is required at least, but completely failing might be better indeed... Regards, Simon > >> Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> >> --- >> >> drivers/spi/spi-uclass.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >> index e06a603ab1..41ecef77db 100644 >> --- a/drivers/spi/spi-uclass.c >> +++ b/drivers/spi/spi-uclass.c >> @@ -325,6 +325,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> if (!speed) { >> speed = plat->max_hz; >> mode = plat->mode; >> + if (!speed) >> + speed = 100000; >> } >> ret = spi_set_speed_mode(bus, speed, mode); >> if (ret) >>
On 22.01.2018 07:01, Jagan Teki wrote: > On Thu, Jan 18, 2018 at 1:45 PM, Simon Goldschmidt > <sgoldschmidt@de.pepperl-fuchs.com> wrote: >> When the device tree is missing a correct spi slave description below >> the bus, 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, as is >> done in 'dm_spi_claim_bus'. > Better go-with default baudrate if speed == 0 this what usually does > in remaining drivers. That's what I did. I set a default value of 100 kHz. The difference seems to me that the remaining drivers are host drivers whereas I was trying to fix an invalid declaration of a df chip. Regards, Simon
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index e06a603ab1..41ecef77db 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -325,6 +325,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, if (!speed) { speed = plat->max_hz; mode = plat->mode; + if (!speed) + speed = 100000; } ret = spi_set_speed_mode(bus, speed, mode); if (ret)
When the device tree is missing a correct spi slave description below the bus, 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, as is done in 'dm_spi_claim_bus'. Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> --- drivers/spi/spi-uclass.c | 2 ++ 1 file changed, 2 insertions(+)