| Message ID | 1407051288-17324-2-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Sunday, August 03, 2014 at 09:34:31 AM, Nikita Kiryanov wrote: > Currently we can define CONFIG_SPL_SPI_<any parameter except SPI MODE>. > Define CONFIG_SPL_SPI_MODE option, and provide a default value for > backwards compatibility. > Default values are also provided for the rest of the spi_flash_probe > parameters (like we do in cmd_sf), to help with config file brevity. > > Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> > Cc: Tom Rini <trini@ti.com> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> You might actually be even more bold and check if you cannot fall back to the CONFIG_DEFAULT_SPI_MODE etc. What do you think ? Best regards, Marek Vasut
On 03/08/14 16:44, Marek Vasut wrote: > On Sunday, August 03, 2014 at 09:34:31 AM, Nikita Kiryanov wrote: >> Currently we can define CONFIG_SPL_SPI_<any parameter except SPI MODE>. >> Define CONFIG_SPL_SPI_MODE option, and provide a default value for >> backwards compatibility. >> Default values are also provided for the rest of the spi_flash_probe >> parameters (like we do in cmd_sf), to help with config file brevity. >> >> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> >> Cc: Tom Rini <trini@ti.com> >> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > You might actually be even more bold and check if you cannot fall back to the > CONFIG_DEFAULT_SPI_MODE etc. What do you think ? Not a fan of the idea. It will: - Complicate the #ifdefs - Complicate the relationship between CONFIG_DEFAULT_SPI_* and CONFIG_SPL_SPI_* #defines - Not get much use: most boards do not #define CONFIG_DEFAULT_SPI_* values in the config files, and of the ones that do, only two (dra7xx_evm and cm_fx6) use SPI in SPL. > > Best regards, > Marek Vasut >
On Tuesday, August 05, 2014 at 03:28:04 PM, Nikita Kiryanov wrote: > On 03/08/14 16:44, Marek Vasut wrote: > > On Sunday, August 03, 2014 at 09:34:31 AM, Nikita Kiryanov wrote: > >> Currently we can define CONFIG_SPL_SPI_<any parameter except SPI MODE>. > >> Define CONFIG_SPL_SPI_MODE option, and provide a default value for > >> backwards compatibility. > >> Default values are also provided for the rest of the spi_flash_probe > >> parameters (like we do in cmd_sf), to help with config file brevity. > >> > >> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> > >> Cc: Tom Rini <trini@ti.com> > >> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > > > You might actually be even more bold and check if you cannot fall back to > > the CONFIG_DEFAULT_SPI_MODE etc. What do you think ? > > Not a fan of the idea. It will: > - Complicate the #ifdefs > - Complicate the relationship between CONFIG_DEFAULT_SPI_* and > CONFIG_SPL_SPI_* #defines > - Not get much use: most boards do not #define CONFIG_DEFAULT_SPI_* > values in the config files, and of the ones that do, only two > (dra7xx_evm and cm_fx6) use SPI in SPL. On the other hand, it's now only a matter of time until we get CONFIG_TPL_SPI_* m which gives us _another_ set of defines. So the question is -- what is your proposition to keep the amount of new ad-hoc defines low and cater for this case? Best regards, Marek Vasut
On 05/08/14 17:11, Marek Vasut wrote: > On Tuesday, August 05, 2014 at 03:28:04 PM, Nikita Kiryanov wrote: >> On 03/08/14 16:44, Marek Vasut wrote: >>> On Sunday, August 03, 2014 at 09:34:31 AM, Nikita Kiryanov wrote: >>>> Currently we can define CONFIG_SPL_SPI_<any parameter except SPI MODE>. >>>> Define CONFIG_SPL_SPI_MODE option, and provide a default value for >>>> backwards compatibility. >>>> Default values are also provided for the rest of the spi_flash_probe >>>> parameters (like we do in cmd_sf), to help with config file brevity. >>>> >>>> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> >>>> Cc: Tom Rini <trini@ti.com> >>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >>> >>> You might actually be even more bold and check if you cannot fall back to >>> the CONFIG_DEFAULT_SPI_MODE etc. What do you think ? >> >> Not a fan of the idea. It will: >> - Complicate the #ifdefs >> - Complicate the relationship between CONFIG_DEFAULT_SPI_* and >> CONFIG_SPL_SPI_* #defines >> - Not get much use: most boards do not #define CONFIG_DEFAULT_SPI_* >> values in the config files, and of the ones that do, only two >> (dra7xx_evm and cm_fx6) use SPI in SPL. > > On the other hand, it's now only a matter of time until we get CONFIG_TPL_SPI_* > m which gives us _another_ set of defines. So the question is -- what is your > proposition to keep the amount of new ad-hoc defines low and cater for this > case? OK I think I may have misunderstood your suggestion. You wanted to replace CONFIG_SPL_SPI_* #defines with CONFIG_DEFAULT_SPI_* #defines, not use both, right? Based on cursory grepping, this seems possible, though I think CONFIG_SF_DEFAULT_* is a better candidate. I'll prepare a patch.. > > Best regards, > Marek Vasut >
On Wednesday, August 06, 2014 at 12:53:19 PM, Nikita Kiryanov wrote: > On 05/08/14 17:11, Marek Vasut wrote: > > On Tuesday, August 05, 2014 at 03:28:04 PM, Nikita Kiryanov wrote: > >> On 03/08/14 16:44, Marek Vasut wrote: > >>> On Sunday, August 03, 2014 at 09:34:31 AM, Nikita Kiryanov wrote: > >>>> Currently we can define CONFIG_SPL_SPI_<any parameter except SPI > >>>> MODE>. Define CONFIG_SPL_SPI_MODE option, and provide a default value > >>>> for backwards compatibility. > >>>> Default values are also provided for the rest of the spi_flash_probe > >>>> parameters (like we do in cmd_sf), to help with config file brevity. > >>>> > >>>> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> > >>>> Cc: Tom Rini <trini@ti.com> > >>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > >>> > >>> You might actually be even more bold and check if you cannot fall back > >>> to the CONFIG_DEFAULT_SPI_MODE etc. What do you think ? > >> > >> Not a fan of the idea. It will: > >> - Complicate the #ifdefs > >> - Complicate the relationship between CONFIG_DEFAULT_SPI_* and > >> > >> CONFIG_SPL_SPI_* #defines > >> > >> - Not get much use: most boards do not #define CONFIG_DEFAULT_SPI_* > >> > >> values in the config files, and of the ones that do, only two > >> (dra7xx_evm and cm_fx6) use SPI in SPL. > > > > On the other hand, it's now only a matter of time until we get > > CONFIG_TPL_SPI_* m which gives us _another_ set of defines. So the > > question is -- what is your proposition to keep the amount of new ad-hoc > > defines low and cater for this case? > > OK I think I may have misunderstood your suggestion. You wanted to > replace CONFIG_SPL_SPI_* #defines with CONFIG_DEFAULT_SPI_* #defines, > not use both, right? > Based on cursory grepping, this seems possible, though I think > CONFIG_SF_DEFAULT_* is a better candidate. > > I'll prepare a patch.. Yep, thank you! Best regards, Marek Vasut
diff --git a/README b/README index f704eb3..d7c55f8 100644 --- a/README +++ b/README @@ -2910,6 +2910,13 @@ CBFS (Coreboot Filesystem) support CONFIG_SF_DEFAULT_MODE (see include/spi.h) CONFIG_SF_DEFAULT_SPEED in Hz + The following defaults may be provided by the platform to + override SPL defaults for SPI. + + CONFIG_SPL_SPI_MODE SPI mode Default SPI_MODE3 + CONFIG_SPL_SPI_CS Chip-select Default 0 + CONFIG_SPL_SPI_BUS Bus identifier Default 0 + CONFIG_CMD_SF_TEST Define this option to include a destructive SPI flash diff --git a/drivers/mtd/spi/spi_spl_load.c b/drivers/mtd/spi/spi_spl_load.c index 1954b7e..b270b82 100644 --- a/drivers/mtd/spi/spi_spl_load.c +++ b/drivers/mtd/spi/spi_spl_load.c @@ -13,6 +13,19 @@ #include <spi_flash.h> #include <spl.h> +#ifndef CONFIG_SF_DEFAULT_SPEED +# define CONFIG_SF_DEFAULT_SPEED 1000000 +#endif +#ifndef CONFIG_SPL_SPI_MODE +# define CONFIG_SPL_SPI_MODE SPI_MODE_3 +#endif +#ifndef CONFIG_SPL_SPI_CS +# define CONFIG_SPL_SPI_CS 0 +#endif +#ifndef CONFIG_SPL_SPI_BUS +# define CONFIG_SPL_SPI_BUS 0 +#endif + #ifdef CONFIG_SPL_OS_BOOT /* * Load the kernel, check for a valid header we can parse, and if found load @@ -57,7 +70,7 @@ void spl_spi_load_image(void) */ flash = spi_flash_probe(CONFIG_SPL_SPI_BUS, CONFIG_SPL_SPI_CS, - CONFIG_SF_DEFAULT_SPEED, SPI_MODE_3); + CONFIG_SF_DEFAULT_SPEED, CONFIG_SPL_SPI_MODE); if (!flash) { puts("SPI probe failed.\n"); hang();
Currently we can define CONFIG_SPL_SPI_<any parameter except SPI MODE>. Define CONFIG_SPL_SPI_MODE option, and provide a default value for backwards compatibility. Default values are also provided for the rest of the spi_flash_probe parameters (like we do in cmd_sf), to help with config file brevity. Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com> Cc: Tom Rini <trini@ti.com> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> --- README | 7 +++++++ drivers/mtd/spi/spi_spl_load.c | 15 ++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)