| Message ID | 1327421904-21487-5-git-send-email-eric.nelson@boundarydevices.com |
|---|---|
| State | Superseded |
| Headers | show |
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>
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
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.
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
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.
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"