mxc_spi refactoring (for mx6q and mx6qsabrelite)

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 <[email protected]>

Acked-by Dirk Behme <[email protected]>

Acked-by Stefano Babic <[email protected]>

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

Quoted 4 lines · 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.

Quoted 5 lines

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

common/cmd_sf.c static int do_spi_flash_probe(int argc, char * const argv[]) · line 76

Quoted 7 lines

common/cmd_sf.c +3 -0

 
+#ifndef CONFIG_SPI_FLASH_CS
+	if (argc<  2) {
+		printf("%s: missing arguments\n", __func__);
  		return -1;

	return cmd_usage(cmdtp);

common/cmd_sf.c static int do_spi_flash_probe(int argc, char * const argv[]) · line 80

Quoted 7 lines

common/cmd_sf.c +4 -3

-	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

Quoted 6 lines

U_BOOT_CMD(
sf, 5, 1, do_spi_flash,
"SPI flash sub-system",

common/cmd_sf.c +2 -0

+#ifndef CONFIG_SPI_FLASH_CS
  	"probe [bus:]cs [hz] [mode]	- init flash device on given SPI bus\n"
+#else

common/cmd_sf.c usage: · line 317

Quoted 5 lines

common/cmd_sf.c +2 -0

+	"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.

Quoted 12 lines · Mike Frysinger

Quoted 11 lines · 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

Quoted 4 lines

Quoted 1 line

	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.

Quoted 17 lines

common/cmd_sf.c static int do_spi_flash_probe(int argc, char * const argv[]) · line 80

Quoted 10 lines

common/cmd_sf.c +4 -3

 
-	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

Quoted 3 lines

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

Quoted 18 lines · On Thursday 26 January 2012 20:22:22 Eric Nelson wrote:

Quoted 16 lines · Mike Frysinger

Quoted 4 lines · 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.

Quoted 8 lines

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

Quoted 2 lines

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

Quoted 23 lines

Quoted 14 lines

common/cmd_sf.c static int do_spi_flash_probe(int argc, char * const argv[]) · line 80

Quoted 10 lines

common/cmd_sf.c +4 -3

 
-	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

Quoted 6 lines

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,

Quoted 2 lines

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

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 <[email protected]>

Acked-by Dirk Behme <[email protected]>

Acked-by Stefano Babic <[email protected]>

---
 common/cmd_sf.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

common/cmd_sf.c +0 -0

index 7225656..4b32171 100644
@@ -70,20 +70,28 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 	char *endp;

common/cmd_sf.c +13 -3

+#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(

Hi Eric,

please see my comments below.

Quoted 25 lines · Eric Nelson

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<[email protected]>

Acked-by Dirk Behme<[email protected]>

Acked-by Stefano Babic<[email protected]>

---
  common/cmd_sf.c |   34 +++++++++++++++++++++++-----------
  1 files changed, 23 insertions(+), 11 deletions(-)

common/cmd_sf.c +0 -0

index 7225656..4b32171 100644
@@ -70,20 +70,28 @@ static int do_spi_flash_probe(int argc, char * const argv[])
  	char *endp;
  	struct spi_flash *new;
 

common/cmd_sf.c static int do_spi_flash_probe(int argc, char * const argv[]) · line 75

Quoted 8 lines

common/cmd_sf.c +3 -5

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

I think this format for the error message is a little bit untypical for u-boot. We do not show up the internal C function name. Better would be to show the command usage, right?

common/cmd_sf.c static int do_spi_flash_probe(int argc, char * const argv[]) · line 79

Quoted 7 lines

common/cmd_sf.c +3 -3

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

Other options for the spi flash subsystem are called CONFIG_SF_DEFAULT_MODE|SPEED. It think it make sense to call this CONFIG_SF_DEFAULT_CS and CONFIG_SF_DEFAULT_BUS (see below).

Quoted 23 lines

common/cmd_sf.c +13 -3

+#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(

common/cmd_sf.c usage: · line 317

Quoted 10 lines

common/cmd_sf.c +4 -0

  	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"

Can you also add a config option for the SPI bus number? I think these two need to handled in the same patch.

So you could add this stuff:

common/cmd_sf.c +0 -0

index 9e97c8e..fa4312a 100644
@@ -63,7 +63,11 @@ static int sf_parse_len_arg(char *arg, ulong *len)
 

Quoted 7 lines · Matthias Fuchs

Quoted 6 lines · Eric Nelson

common/cmd_sf.c +0 -0

 
  	char *endp;
  	struct spi_flash *new;

Quoted 12 lines

common/cmd_sf.c

Quoted 8 lines

common/cmd_sf.c +3 -5

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

I think this format for the error message is a little bit untypical for u-boot. We do not show up the internal C function name. Better would be to show the command usage, right?

Looking at this area of the code in more detail, there are quite a few cases where improper usage silently return -1.

I'm inclined to either follow that lead or toss them together with a "goto usage" as done in do_spi_flash().

Any preferences?