[U-Boot] [PATCH] dm: spi: Read default speed, mode values from DT

[U-Boot] [PATCH] dm: spi: Read default speed, mode values from DT

[Vignesh R]

In case of DT boot, don't read default speed and mode for SPI from
CONFIG_*, instead read from DT node. This will make sure that boards
with multiple SPI/QSPI controllers can be probed at different
bus frequencies and modes.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 cmd/sf.c                 | 2 ++
 drivers/spi/spi-uclass.c | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index 42862d9d921a..4c40d5104422 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 #ifdef CONFIG_DM_SPI_FLASH
 	struct udevice *new, *bus_dev;
 	int ret;
+	/* In DM mode defaults wiil be taken from DT */
+	speed = -1, mode = -1;
 #else
 	struct spi_flash *new;
 #endif
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 5561f36762f9..55ebb5ef19b7 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 		       struct udevice **busp, struct spi_slave **devp)
 {
 	struct udevice *bus, *dev;
+	struct dm_spi_slave_platdata *plat;
 	bool created = false;
 	int ret;
 
@@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 	 * SPI flash chip - we will bind to the correct driver.
 	 */
 	if (ret == -ENODEV && drv_name) {
-		struct dm_spi_slave_platdata *plat;
-
 		debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
 		      __func__, dev_name, busnum, cs, drv_name);
 		ret = device_bind_driver(bus, drv_name, dev_name, &dev);
@@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 		slave->dev = dev;
 	}
 
+	plat = dev_get_parent_platdata(dev);
+	if (speed == -1)
+		speed = plat->max_hz;
+	if (mode == -1)
+		mode = plat->mode;
 	ret = spi_set_speed_mode(bus, speed, mode);
 	if (ret)
 		goto err;
-- 
2.8.1

[U-Boot] [PATCH] dm: spi: Read default speed, mode values from DT

[Tom Rini]

On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote:

> In case of DT boot, don't read default speed and mode for SPI from
> CONFIG_*, instead read from DT node. This will make sure that boards
> with multiple SPI/QSPI controllers can be probed at different
> bus frequencies and modes.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  cmd/sf.c                 | 2 ++
>  drivers/spi/spi-uclass.c | 8 ++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/sf.c b/cmd/sf.c
> index 42862d9d921a..4c40d5104422 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>  #ifdef CONFIG_DM_SPI_FLASH
>  	struct udevice *new, *bus_dev;
>  	int ret;
> +	/* In DM mode defaults wiil be taken from DT */
> +	speed = -1, mode = -1;
>  #else
>  	struct spi_flash *new;
>  #endif
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index 5561f36762f9..55ebb5ef19b7 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>  		       struct udevice **busp, struct spi_slave **devp)
>  {
>  	struct udevice *bus, *dev;
> +	struct dm_spi_slave_platdata *plat;
>  	bool created = false;
>  	int ret;
>  
> @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>  	 * SPI flash chip - we will bind to the correct driver.
>  	 */
>  	if (ret == -ENODEV && drv_name) {
> -		struct dm_spi_slave_platdata *plat;
> -
>  		debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
>  		      __func__, dev_name, busnum, cs, drv_name);
>  		ret = device_bind_driver(bus, drv_name, dev_name, &dev);
> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>  		slave->dev = dev;
>  	}
>  
> +	plat = dev_get_parent_platdata(dev);
> +	if (speed == -1)
> +		speed = plat->max_hz;
> +	if (mode == -1)
> +		mode = plat->mode;
>  	ret = spi_set_speed_mode(bus, speed, mode);
>  	if (ret)
>  		goto err;

I like the concept but if it can fail, passing -1 for speed/mode seems
like a bad idea and we should just fail.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160408/c0d38710/attachment.sig>

[U-Boot] [PATCH] dm: spi: Read default speed, mode values from DT

[Vignesh R]

On 04/09/2016 01:15 AM, Tom Rini wrote:
> On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote:
> 
>> In case of DT boot, don't read default speed and mode for SPI from
>> CONFIG_*, instead read from DT node. This will make sure that boards
>> with multiple SPI/QSPI controllers can be probed at different
>> bus frequencies and modes.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  cmd/sf.c                 | 2 ++
>>  drivers/spi/spi-uclass.c | 8 ++++++--
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/cmd/sf.c b/cmd/sf.c
>> index 42862d9d921a..4c40d5104422 100644
>> --- a/cmd/sf.c
>> +++ b/cmd/sf.c
>> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>>  #ifdef CONFIG_DM_SPI_FLASH
>>  	struct udevice *new, *bus_dev;
>>  	int ret;
>> +	/* In DM mode defaults wiil be taken from DT */
>> +	speed = -1, mode = -1;
>>  #else
>>  	struct spi_flash *new;
>>  #endif
>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
>> index 5561f36762f9..55ebb5ef19b7 100644
>> --- a/drivers/spi/spi-uclass.c
>> +++ b/drivers/spi/spi-uclass.c
>> @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>  		       struct udevice **busp, struct spi_slave **devp)
>>  {
>>  	struct udevice *bus, *dev;
>> +	struct dm_spi_slave_platdata *plat;
>>  	bool created = false;
>>  	int ret;
>>  
>> @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>  	 * SPI flash chip - we will bind to the correct driver.
>>  	 */
>>  	if (ret == -ENODEV && drv_name) {
>> -		struct dm_spi_slave_platdata *plat;
>> -
>>  		debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
>>  		      __func__, dev_name, busnum, cs, drv_name);
>>  		ret = device_bind_driver(bus, drv_name, dev_name, &dev);
>> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>  		slave->dev = dev;
>>  	}
>>  
>> +	plat = dev_get_parent_platdata(dev);
>> +	if (speed == -1)
>> +		speed = plat->max_hz;
>> +	if (mode == -1)
>> +		mode = plat->mode;
>>  	ret = spi_set_speed_mode(bus, speed, mode);
>>  	if (ret)
>>  		goto err;
> 
> I like the concept but if it can fail, passing -1 for speed/mode seems
> like a bad idea and we should just fail.
> 
I didn't quite understand your comment. Could you explain what can fail?
I don't think mode/speed will be -1 when calling spi_set_speed_mode()
after above change. speed/mode will either be command line arg passed to
sf probe cmd or CONFIG_SF_DEFAULT_* in case of non DM driver or
mode/speed specified in DT node in case of DM.



-- 
Regards
Vignesh

[U-Boot] [PATCH] dm: spi: Read default speed, mode values from DT

[Mugunthan V N]

On Monday 11 April 2016 10:12 AM, Vignesh R wrote:
> 
> 
> On 04/09/2016 01:15 AM, Tom Rini wrote:
>> On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote:
>>
>>> In case of DT boot, don't read default speed and mode for SPI from
>>> CONFIG_*, instead read from DT node. This will make sure that boards
>>> with multiple SPI/QSPI controllers can be probed at different
>>> bus frequencies and modes.
>>>
>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>> ---
>>>  cmd/sf.c                 | 2 ++
>>>  drivers/spi/spi-uclass.c | 8 ++++++--
>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/sf.c b/cmd/sf.c
>>> index 42862d9d921a..4c40d5104422 100644
>>> --- a/cmd/sf.c
>>> +++ b/cmd/sf.c
>>> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>>>  #ifdef CONFIG_DM_SPI_FLASH
>>>  	struct udevice *new, *bus_dev;
>>>  	int ret;
>>> +	/* In DM mode defaults wiil be taken from DT */
>>> +	speed = -1, mode = -1;

speed and mode are uint and it is assigned with -1 which is not making
sense. Assigning 0 will be a better option.

Regards
Mugunthan V N

>>>  #else
>>>  	struct spi_flash *new;
>>>  #endif
>>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
>>> index 5561f36762f9..55ebb5ef19b7 100644
>>> --- a/drivers/spi/spi-uclass.c
>>> +++ b/drivers/spi/spi-uclass.c
>>> @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>>  		       struct udevice **busp, struct spi_slave **devp)
>>>  {
>>>  	struct udevice *bus, *dev;
>>> +	struct dm_spi_slave_platdata *plat;
>>>  	bool created = false;
>>>  	int ret;
>>>  
>>> @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>>  	 * SPI flash chip - we will bind to the correct driver.
>>>  	 */
>>>  	if (ret == -ENODEV && drv_name) {
>>> -		struct dm_spi_slave_platdata *plat;
>>> -
>>>  		debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
>>>  		      __func__, dev_name, busnum, cs, drv_name);
>>>  		ret = device_bind_driver(bus, drv_name, dev_name, &dev);
>>> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
>>>  		slave->dev = dev;
>>>  	}
>>>  
>>> +	plat = dev_get_parent_platdata(dev);
>>> +	if (speed == -1)
>>> +		speed = plat->max_hz;
>>> +	if (mode == -1)
>>> +		mode = plat->mode;
>>>  	ret = spi_set_speed_mode(bus, speed, mode);
>>>  	if (ret)
>>>  		goto err;
>>
>> I like the concept but if it can fail, passing -1 for speed/mode seems
>> like a bad idea and we should just fail.
>>
> I didn't quite understand your comment. Could you explain what can fail?
> I don't think mode/speed will be -1 when calling spi_set_speed_mode()
> after above change. speed/mode will either be command line arg passed to
> sf probe cmd or CONFIG_SF_DEFAULT_* in case of non DM driver or
> mode/speed specified in DT node in case of DM.
> 
> 
> 

[U-Boot] [PATCH] dm: spi: Read default speed, mode values from DT

[Vignesh R]

On 04/11/2016 11:18 AM, Mugunthan V N wrote:
> On Monday 11 April 2016 10:12 AM, Vignesh R wrote:
>>
>>
>> On 04/09/2016 01:15 AM, Tom Rini wrote:
>>> On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote:
>>>
>>>> In case of DT boot, don't read default speed and mode for SPI from
>>>> CONFIG_*, instead read from DT node. This will make sure that boards
>>>> with multiple SPI/QSPI controllers can be probed at different
>>>> bus frequencies and modes.
>>>>
>>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>>> ---
>>>>  cmd/sf.c                 | 2 ++
>>>>  drivers/spi/spi-uclass.c | 8 ++++++--
>>>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/cmd/sf.c b/cmd/sf.c
>>>> index 42862d9d921a..4c40d5104422 100644
>>>> --- a/cmd/sf.c
>>>> +++ b/cmd/sf.c
>>>> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
>>>>  #ifdef CONFIG_DM_SPI_FLASH
>>>>  	struct udevice *new, *bus_dev;
>>>>  	int ret;
>>>> +	/* In DM mode defaults wiil be taken from DT */
>>>> +	speed = -1, mode = -1;
> 
> speed and mode are uint and it is assigned with -1 which is not making
> sense. Assigning 0 will be a better option.

Ah, thanks! I will fix this up in the next version.

-- 
Regards
Vignesh