[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Simon Goldschmidt]

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;
 	}
 	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] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Michael Nazzareno Trimarchi]

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Simon Goldschmidt]

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>

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Michael Nazzareno Trimarchi]

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



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Simon Glass]

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

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Vignesh R]

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)
> 

-- 
Regards
Vignesh

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Jagan Teki]

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.

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Simon Goldschmidt]

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

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Simon Goldschmidt]

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)
>>

[U-Boot] [PATCH] dm: spi: prevent setting a speed of 0 Hz

[Simon Goldschmidt]

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