Patchwork [U-Boot,V3,4/6] sf command: allow default chip select through CONFIG_SPI_FLASH_CS

login
register
mail settings
Submitter Eric Nelson
Date Jan. 24, 2012, 4:18 p.m.
Message ID <1327421904-21487-5-git-send-email-eric.nelson@boundarydevices.com>
Download mbox | patch
Permalink /patch/137588/
State Superseded
Headers show

Comments

Eric Nelson - Jan. 24, 2012, 4:18 p.m.
This patch allows a board configuration file to provide a default 
chip-select for serial flash so that first argument to the 'sf' command
is optional.

On boards that use the mxc_spi driver and a GPIO for chip select, this allows 
a much simpler command line:
	U-Boot> sf probe
instead of
	U-Boot> sf probe 0x5300

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
Acked-by: Dirk Behme <dirk.behme@de.bosch.com> 
Acked-by: Stefano Babic <sbabic@denx.de>
---
 common/cmd_sf.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)
Fabio Estevam - Jan. 24, 2012, 4:27 p.m.
On 1/24/12, Eric Nelson <eric.nelson@boundarydevices.com> wrote:
> This patch allows a board configuration file to provide a default
> chip-select for serial flash so that first argument to the 'sf' command
> is optional.
>
> On boards that use the mxc_spi driver and a GPIO for chip select, this
> allows
> a much simpler command line:
> 	U-Boot> sf probe
> instead of
> 	U-Boot> sf probe 0x5300
>
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> Acked-by: Dirk Behme <dirk.behme@de.bosch.com>
> Acked-by: Stefano Babic <sbabic@denx.de>

Acked-by: Fabio Estevam <fabio.estevam@freescale.com>
Mike Frysinger - Jan. 24, 2012, 6:08 p.m.
On Tuesday 24 January 2012 11:18:22 Eric Nelson wrote:
> This patch allows a board configuration file to provide a default
> chip-select for serial flash so that first argument to the 'sf' command
> is optional.
> 
> On boards that use the mxc_spi driver and a GPIO for chip select, this
> allows a much simpler command line:
> 	U-Boot> sf probe
> instead of
> 	U-Boot> sf probe 0x5300

NAK (to this version of the patch): missing README update, and other issues 
below

> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
>
> +#ifndef CONFIG_SPI_FLASH_CS
> +	if (argc < 2) {
> +		printf("%s: missing arguments\n", __func__);
>  		return -1;

	return cmd_usage(cmdtp);

> -	if (*endp == ':') {
> -		if (endp[1] == 0)
> -			return -1;
> +	}
> +#else
> +	cs = CONFIG_SPI_FLASH_CS ;
> +#endif

you're setting the default CS, not locking it in.  so a better config knob name 
would be something like:
	CONFIG_SF_DEFAULT_CS
this matches the existing CONFIG_SF_XXX defines

also, you have a spurious space before the semicolon there

>  U_BOOT_CMD(
>  	sf,	5,	1,	do_spi_flash,
>  	"SPI flash sub-system",
> +#ifndef CONFIG_SPI_FLASH_CS
>  	"probe [bus:]cs [hz] [mode]	- init flash device on given SPI bus\n"
> +#else
> +	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
> +#endif
>  	"				  and chip select\n"
>  	"sf read addr offset len 	- read `len' bytes starting at\n"
>  	"				  `offset' to memory at `addr'\n"

this is ugly.  i'd rather just omit it and not worry about the syntax being 
perfect.
-mike
Eric Nelson - Jan. 27, 2012, 1:22 a.m.
On 01/24/2012 11:08 AM, Mike Frysinger wrote:
> On Tuesday 24 January 2012 11:18:22 Eric Nelson wrote:
>> This patch allows a board configuration file to provide a default
>> chip-select for serial flash so that first argument to the 'sf' command
>> is optional.
>>
>> On boards that use the mxc_spi driver and a GPIO for chip select, this
>> allows a much simpler command line:
>> 	U-Boot>  sf probe
>> instead of
>> 	U-Boot>  sf probe 0x5300
>
> NAK (to this version of the patch): missing README update, and other issues
> below
>

Which README? The only references I find to serial flash support
are in board-specific README files.

~/u-boot$ find . -iname \*readme\* | xargs grep -w sf
./doc/README.p2041rdb:	=> sf erase 0 100000
./doc/README.p2041rdb:	=> sf write 1000000 0 $filesize
./doc/README.p2041rdb:	=> sf erase 110000 10000
./doc/README.p2041rdb:	=> sf write 1000000 110000 $filesize
./doc/README.sh7757lcr:   => sf probe 0
./doc/README.sh7757lcr:   => sf erase 0 80000
./doc/README.sh7757lcr:   => sf write 0x89000000 0 80000

I can start one of those for the SabreLite board, but that's un-related
to this patch.

>> --- a/common/cmd_sf.c
>> +++ b/common/cmd_sf.c
>>
>> +#ifndef CONFIG_SPI_FLASH_CS
>> +	if (argc<  2) {
>> +		printf("%s: missing arguments\n", __func__);
>>   		return -1;
>
> 	return cmd_usage(cmdtp);
>
>> -	if (*endp == ':') {
>> -		if (endp[1] == 0)
>> -			return -1;
>> +	}
>> +#else
>> +	cs = CONFIG_SPI_FLASH_CS ;
>> +#endif
>
> you're setting the default CS, not locking it in.  so a better config knob name
> would be something like:
> 	CONFIG_SF_DEFAULT_CS
> this matches the existing CONFIG_SF_XXX defines
>
> also, you have a spurious space before the semicolon there
>
Thanks Mike,

FWIW, I chose this name on purpose to make life easier on a couple of
other boards immediately (efika and vision2):

~/u-boot$ grep CONFIG_SPI_FLASH_CS include/configs/*
include/configs/efikamx.h:#define CONFIG_SPI_FLASH_CS		(1 | 121 << 8)
include/configs/m28evk.h:#define	CONFIG_SPI_FLASH_CS		2
include/configs/mx6qsabrelite.h.rej: 	#define CONFIG_SPI_FLASH_CS	1
include/configs/vision2.h:#define CONFIG_SPI_FLASH_CS	(1 | (121 << 8))

>>   U_BOOT_CMD(
>>   	sf,	5,	1,	do_spi_flash,
>>   	"SPI flash sub-system",
>> +#ifndef CONFIG_SPI_FLASH_CS
>>   	"probe [bus:]cs [hz] [mode]	- init flash device on given SPI bus\n"
>> +#else
>> +	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
>> +#endif
>>   	"				  and chip select\n"
>>   	"sf read addr offset len 	- read `len' bytes starting at\n"
>>   	"				  `offset' to memory at `addr'\n"
>
> this is ugly.  i'd rather just omit it and not worry about the syntax being
> perfect.

Works for me.
Mike Frysinger - Jan. 27, 2012, 2:50 a.m.
On Thursday 26 January 2012 20:22:22 Eric Nelson wrote:
> On 01/24/2012 11:08 AM, Mike Frysinger wrote:
> > On Tuesday 24 January 2012 11:18:22 Eric Nelson wrote:
> >> This patch allows a board configuration file to provide a default
> >> chip-select for serial flash so that first argument to the 'sf' command
> >> is optional.
> >> 
> >> On boards that use the mxc_spi driver and a GPIO for chip select, this
> >> 
> >> allows a much simpler command line:
> >> 	U-Boot>  sf probe
> >> 
> >> instead of
> >> 
> >> 	U-Boot>  sf probe 0x5300
> > 
> > NAK (to this version of the patch): missing README update, and other
> > issues below
> 
> Which README? The only references I find to serial flash support
> are in board-specific README files.

all new CONFIG_xxx defines should be documented in the top level README.  
granted, the existin SF ones have slipped in without being documented, but 
that's bad for them ;).

> >> --- a/common/cmd_sf.c
> >> +++ b/common/cmd_sf.c
> >> 
> >> -	if (*endp == ':') {
> >> -		if (endp[1] == 0)
> >> -			return -1;
> >> +	}
> >> +#else
> >> +	cs = CONFIG_SPI_FLASH_CS ;
> >> +#endif
> > 
> > you're setting the default CS, not locking it in.  so a better config
> > knob name
> > 
> > would be something like:
> > 	CONFIG_SF_DEFAULT_CS
> > 
> > this matches the existing CONFIG_SF_XXX defines
> > 
> > also, you have a spurious space before the semicolon there
> 
> Thanks Mike,
> 
> FWIW, I chose this name on purpose to make life easier on a couple of
> other boards immediately (efika and vision2):

those boards are dumb -- that define isn't used anywhere, so that's just dead 
code.  cmd_sf has a standard already, so new defines specific to cmd_sf should 
follow that.
-mike
Eric Nelson - Jan. 27, 2012, 1:39 p.m.
On 01/26/2012 07:50 PM, Mike Frysinger wrote:
> On Thursday 26 January 2012 20:22:22 Eric Nelson wrote:
>> On 01/24/2012 11:08 AM, Mike Frysinger wrote:
>>> On Tuesday 24 January 2012 11:18:22 Eric Nelson wrote:
>>>> This patch allows a board configuration file to provide a default
>>>> chip-select for serial flash so that first argument to the 'sf' command
>>>> is optional.
>>>>
>>>> On boards that use the mxc_spi driver and a GPIO for chip select, this
>>>>
>>>> allows a much simpler command line:
>>>> 	U-Boot>   sf probe
>>>>
>>>> instead of
>>>>
>>>> 	U-Boot>   sf probe 0x5300
>>>
>>> NAK (to this version of the patch): missing README update, and other
>>> issues below
>>
>> Which README? The only references I find to serial flash support
>> are in board-specific README files.
>
> all new CONFIG_xxx defines should be documented in the top level README.
> granted, the existin SF ones have slipped in without being documented, but
> that's bad for them ;).
>

Ok. I'll re-send with README updates.

>>>> --- a/common/cmd_sf.c
>>>> +++ b/common/cmd_sf.c
>>>>
>>>> -	if (*endp == ':') {
>>>> -		if (endp[1] == 0)
>>>> -			return -1;
>>>> +	}
>>>> +#else
>>>> +	cs = CONFIG_SPI_FLASH_CS ;
>>>> +#endif
>>>
>>> you're setting the default CS, not locking it in.  so a better config
>>> knob name
>>>
>>> would be something like:
>>> 	CONFIG_SF_DEFAULT_CS
>>>
>>> this matches the existing CONFIG_SF_XXX defines
>>>
>>> also, you have a spurious space before the semicolon there
>>
>> Thanks Mike,
>>
>> FWIW, I chose this name on purpose to make life easier on a couple of
>> other boards immediately (efika and vision2):
>
> those boards are dumb -- that define isn't used anywhere, so that's just dead
> code.  cmd_sf has a standard already, so new defines specific to cmd_sf should
> follow that.
> -mike

Ok. I'll let their maintainers update appropriately.

Patch

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 7225656..4b32171 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -70,20 +70,28 @@  static int do_spi_flash_probe(int argc, char * const argv[])
 	char *endp;
 	struct spi_flash *new;
 
-	if (argc < 2)
-		return -1;
-
-	cs = simple_strtoul(argv[1], &endp, 0);
-	if (*argv[1] == 0 || (*endp != 0 && *endp != ':'))
+#ifndef CONFIG_SPI_FLASH_CS
+	if (argc < 2) {
+		printf("%s: missing arguments\n", __func__);
 		return -1;
-	if (*endp == ':') {
-		if (endp[1] == 0)
-			return -1;
+	}
+#else
+	cs = CONFIG_SPI_FLASH_CS ;
+#endif
 
-		bus = cs;
-		cs = simple_strtoul(endp + 1, &endp, 0);
-		if (*endp != 0)
+	if (argc >= 2) {
+		cs = simple_strtoul(argv[1], &endp, 0);
+		if (*argv[1] == 0 || (*endp != 0 && *endp != ':'))
 			return -1;
+		if (*endp == ':') {
+			if (endp[1] == 0)
+				return -1;
+
+			bus = cs;
+			cs = simple_strtoul(endp + 1, &endp, 0);
+			if (*endp != 0)
+				return -1;
+		}
 	}
 
 	if (argc >= 3) {
@@ -299,7 +307,11 @@  usage:
 U_BOOT_CMD(
 	sf,	5,	1,	do_spi_flash,
 	"SPI flash sub-system",
+#ifndef CONFIG_SPI_FLASH_CS
 	"probe [bus:]cs [hz] [mode]	- init flash device on given SPI bus\n"
+#else
+	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
+#endif
 	"				  and chip select\n"
 	"sf read addr offset len 	- read `len' bytes starting at\n"
 	"				  `offset' to memory at `addr'\n"