| Message ID | 20210914032849.273625-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: sf: Set SF parameters as env variables | expand |
On 14/09/21 05:28AM, Marek Vasut wrote: > Set the SF page size, erase block size and total size as an environment > variable after "sf probe". This lets us discern boards with multiple > distinct SPI flash options and also e.g. set mtdparts accordingly. I don't quite follow the rationale for making these environment variables. Wouldn't you be better off finding this info out via mtd or sf command? > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Vignesh R <vigneshr@ti.com> > --- > drivers/mtd/spi/spi-nor-core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > index d5d905fa5a1..448653b9931 100644 > --- a/drivers/mtd/spi/spi-nor-core.c > +++ b/drivers/mtd/spi/spi-nor-core.c > @@ -15,6 +15,7 @@ > #include <dm.h> > #include <dm/device_compat.h> > #include <dm/devres.h> > +#include <env.h> > #include <linux/bitops.h> > #include <linux/err.h> > #include <linux/errno.h> > @@ -3829,8 +3830,11 @@ int spi_nor_scan(struct spi_nor *nor) > #ifndef CONFIG_SPL_BUILD > printf("SF: Detected %s with page size ", nor->name); > print_size(nor->page_size, ", erase size "); > + env_set_hex("sf_pagesize", nor->page_size); > print_size(nor->erase_size, ", total "); > + env_set_hex("sf_erasesize", nor->erase_size); > print_size(nor->size, ""); > + env_set_hex("sf_size", nor->size); > puts("\n"); > #endif > > -- > 2.33.0 >
On 9/23/21 8:53 PM, Pratyush Yadav wrote: > On 14/09/21 05:28AM, Marek Vasut wrote: >> Set the SF page size, erase block size and total size as an environment >> variable after "sf probe". This lets us discern boards with multiple >> distinct SPI flash options and also e.g. set mtdparts accordingly. > > I don't quite follow the rationale for making these environment > variables. Wouldn't you be better off finding this info out via mtd or > sf command? I need to use that info in a script, that's why these env vars.
On 23/09/21 10:00PM, Marek Vasut wrote: > On 9/23/21 8:53 PM, Pratyush Yadav wrote: > > On 14/09/21 05:28AM, Marek Vasut wrote: > > > Set the SF page size, erase block size and total size as an environment > > > variable after "sf probe". This lets us discern boards with multiple > > > distinct SPI flash options and also e.g. set mtdparts accordingly. > > > > I don't quite follow the rationale for making these environment > > variables. Wouldn't you be better off finding this info out via mtd or > > sf command? > > I need to use that info in a script, that's why these env vars. Ok. Honestly, it doesn't feel quite right to me. I haven't played with U-Boot's shell too much but is there no way to assign variables from command outputs? For example, can we do something like `foo=$(cat a.txt)`? If that is possible, maybe add some new subcommands to "sf" that return this information?
On 10/7/21 2:40 PM, Pratyush Yadav wrote: > On 23/09/21 10:00PM, Marek Vasut wrote: >> On 9/23/21 8:53 PM, Pratyush Yadav wrote: >>> On 14/09/21 05:28AM, Marek Vasut wrote: >>>> Set the SF page size, erase block size and total size as an environment >>>> variable after "sf probe". This lets us discern boards with multiple >>>> distinct SPI flash options and also e.g. set mtdparts accordingly. >>> >>> I don't quite follow the rationale for making these environment >>> variables. Wouldn't you be better off finding this info out via mtd or >>> sf command? >> >> I need to use that info in a script, that's why these env vars. > > Ok. Honestly, it doesn't feel quite right to me. > > I haven't played with U-Boot's shell too much but is there no way to > assign variables from command outputs? Not that I know of. > For example, can we do something > like `foo=$(cat a.txt)`? If that is possible, maybe add some new > subcommands to "sf" that return this information? What you would want to have is per-device runtime properties, kind-of like reduced sysfs, which would need VFS in U-Boot, and we do not have that yet.
On 10/7/21 2:46 PM, Marek Vasut wrote: > On 10/7/21 2:40 PM, Pratyush Yadav wrote: >> On 23/09/21 10:00PM, Marek Vasut wrote: >>> On 9/23/21 8:53 PM, Pratyush Yadav wrote: >>>> On 14/09/21 05:28AM, Marek Vasut wrote: >>>>> Set the SF page size, erase block size and total size as an >>>>> environment >>>>> variable after "sf probe". This lets us discern boards with multiple >>>>> distinct SPI flash options and also e.g. set mtdparts accordingly. >>>> >>>> I don't quite follow the rationale for making these environment >>>> variables. Wouldn't you be better off finding this info out via mtd or >>>> sf command? >>> >>> I need to use that info in a script, that's why these env vars. >> >> Ok. Honestly, it doesn't feel quite right to me. >> >> I haven't played with U-Boot's shell too much but is there no way to >> assign variables from command outputs? > > Not that I know of. > >> For example, can we do something >> like `foo=$(cat a.txt)`? If that is possible, maybe add some new >> subcommands to "sf" that return this information? > > What you would want to have is per-device runtime properties, kind-of > like reduced sysfs, which would need VFS in U-Boot, and we do not have > that yet. Are there any news on this patch ?
On Tue, Sep 14, 2021 at 8:59 AM Marek Vasut <marex@denx.de> wrote: > > Set the SF page size, erase block size and total size as an environment > variable after "sf probe". This lets us discern boards with multiple > distinct SPI flash options and also e.g. set mtdparts accordingly. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Vignesh R <vigneshr@ti.com> > --- > drivers/mtd/spi/spi-nor-core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > index d5d905fa5a1..448653b9931 100644 > --- a/drivers/mtd/spi/spi-nor-core.c > +++ b/drivers/mtd/spi/spi-nor-core.c > @@ -15,6 +15,7 @@ > #include <dm.h> > #include <dm/device_compat.h> > #include <dm/devres.h> > +#include <env.h> > #include <linux/bitops.h> > #include <linux/err.h> > #include <linux/errno.h> > @@ -3829,8 +3830,11 @@ int spi_nor_scan(struct spi_nor *nor) > #ifndef CONFIG_SPL_BUILD > printf("SF: Detected %s with page size ", nor->name); > print_size(nor->page_size, ", erase size "); > + env_set_hex("sf_pagesize", nor->page_size); > print_size(nor->erase_size, ", total "); > + env_set_hex("sf_erasesize", nor->erase_size); > print_size(nor->size, ""); > + env_set_hex("sf_size", nor->size); > puts("\n"); I feel this unnecessarily add env variables which are not needed for those SF enabled boards, isn't it? Jagan.
On 12/2/21 06:47, Jagan Teki wrote: > On Tue, Sep 14, 2021 at 8:59 AM Marek Vasut <marex@denx.de> wrote: >> >> Set the SF page size, erase block size and total size as an environment >> variable after "sf probe". This lets us discern boards with multiple >> distinct SPI flash options and also e.g. set mtdparts accordingly. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Vignesh R <vigneshr@ti.com> >> --- >> drivers/mtd/spi/spi-nor-core.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c >> index d5d905fa5a1..448653b9931 100644 >> --- a/drivers/mtd/spi/spi-nor-core.c >> +++ b/drivers/mtd/spi/spi-nor-core.c >> @@ -15,6 +15,7 @@ >> #include <dm.h> >> #include <dm/device_compat.h> >> #include <dm/devres.h> >> +#include <env.h> >> #include <linux/bitops.h> >> #include <linux/err.h> >> #include <linux/errno.h> >> @@ -3829,8 +3830,11 @@ int spi_nor_scan(struct spi_nor *nor) >> #ifndef CONFIG_SPL_BUILD >> printf("SF: Detected %s with page size ", nor->name); >> print_size(nor->page_size, ", erase size "); >> + env_set_hex("sf_pagesize", nor->page_size); >> print_size(nor->erase_size, ", total "); >> + env_set_hex("sf_erasesize", nor->erase_size); >> print_size(nor->size, ""); >> + env_set_hex("sf_size", nor->size); >> puts("\n"); > > I feel this unnecessarily add env variables which are not needed for > those SF enabled boards, isn't it? So, how do you determine current SPI NOR size from U-Boot shell ?
On Thu, Dec 2, 2021 at 11:19 AM Marek Vasut <marex@denx.de> wrote: > > On 12/2/21 06:47, Jagan Teki wrote: > > On Tue, Sep 14, 2021 at 8:59 AM Marek Vasut <marex@denx.de> wrote: > >> > >> Set the SF page size, erase block size and total size as an environment > >> variable after "sf probe". This lets us discern boards with multiple > >> distinct SPI flash options and also e.g. set mtdparts accordingly. > >> > >> Signed-off-by: Marek Vasut <marex@denx.de> > >> Cc: Jagan Teki <jagan@amarulasolutions.com> > >> Cc: Vignesh R <vigneshr@ti.com> > >> --- > >> drivers/mtd/spi/spi-nor-core.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > >> index d5d905fa5a1..448653b9931 100644 > >> --- a/drivers/mtd/spi/spi-nor-core.c > >> +++ b/drivers/mtd/spi/spi-nor-core.c > >> @@ -15,6 +15,7 @@ > >> #include <dm.h> > >> #include <dm/device_compat.h> > >> #include <dm/devres.h> > >> +#include <env.h> > >> #include <linux/bitops.h> > >> #include <linux/err.h> > >> #include <linux/errno.h> > >> @@ -3829,8 +3830,11 @@ int spi_nor_scan(struct spi_nor *nor) > >> #ifndef CONFIG_SPL_BUILD > >> printf("SF: Detected %s with page size ", nor->name); > >> print_size(nor->page_size, ", erase size "); > >> + env_set_hex("sf_pagesize", nor->page_size); > >> print_size(nor->erase_size, ", total "); > >> + env_set_hex("sf_erasesize", nor->erase_size); > >> print_size(nor->size, ""); > >> + env_set_hex("sf_size", nor->size); > >> puts("\n"); > > > > I feel this unnecessarily add env variables which are not needed for > > those SF enabled boards, isn't it? > > So, how do you determine current SPI NOR size from U-Boot shell ? I think we need to know the size for those who enabled mtd command (or mtdparts), any way to preserve it via mtd command or set the env via mtd command side. Jagan.
On 12/2/21 06:57, Jagan Teki wrote: > On Thu, Dec 2, 2021 at 11:19 AM Marek Vasut <marex@denx.de> wrote: >> >> On 12/2/21 06:47, Jagan Teki wrote: >>> On Tue, Sep 14, 2021 at 8:59 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> Set the SF page size, erase block size and total size as an environment >>>> variable after "sf probe". This lets us discern boards with multiple >>>> distinct SPI flash options and also e.g. set mtdparts accordingly. >>>> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>> Cc: Vignesh R <vigneshr@ti.com> >>>> --- >>>> drivers/mtd/spi/spi-nor-core.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c >>>> index d5d905fa5a1..448653b9931 100644 >>>> --- a/drivers/mtd/spi/spi-nor-core.c >>>> +++ b/drivers/mtd/spi/spi-nor-core.c >>>> @@ -15,6 +15,7 @@ >>>> #include <dm.h> >>>> #include <dm/device_compat.h> >>>> #include <dm/devres.h> >>>> +#include <env.h> >>>> #include <linux/bitops.h> >>>> #include <linux/err.h> >>>> #include <linux/errno.h> >>>> @@ -3829,8 +3830,11 @@ int spi_nor_scan(struct spi_nor *nor) >>>> #ifndef CONFIG_SPL_BUILD >>>> printf("SF: Detected %s with page size ", nor->name); >>>> print_size(nor->page_size, ", erase size "); >>>> + env_set_hex("sf_pagesize", nor->page_size); >>>> print_size(nor->erase_size, ", total "); >>>> + env_set_hex("sf_erasesize", nor->erase_size); >>>> print_size(nor->size, ""); >>>> + env_set_hex("sf_size", nor->size); >>>> puts("\n"); >>> >>> I feel this unnecessarily add env variables which are not needed for >>> those SF enabled boards, isn't it? >> >> So, how do you determine current SPI NOR size from U-Boot shell ? > > I think we need to know the size for those who enabled mtd command (or > mtdparts), any way to preserve it via mtd command or set the env via > mtd command side. I need to know the size of SPI NOR flash to use it in an U-Boot script which handle different board population options. This has nothing to do with mtd command.
Hi Marek, On 28/11/21 10:56PM, Marek Vasut wrote: > On 10/7/21 2:46 PM, Marek Vasut wrote: > > On 10/7/21 2:40 PM, Pratyush Yadav wrote: > > > On 23/09/21 10:00PM, Marek Vasut wrote: > > > > On 9/23/21 8:53 PM, Pratyush Yadav wrote: > > > > > On 14/09/21 05:28AM, Marek Vasut wrote: > > > > > > Set the SF page size, erase block size and total size as > > > > > > an environment > > > > > > variable after "sf probe". This lets us discern boards with multiple > > > > > > distinct SPI flash options and also e.g. set mtdparts accordingly. > > > > > > > > > > I don't quite follow the rationale for making these environment > > > > > variables. Wouldn't you be better off finding this info out via mtd or > > > > > sf command? > > > > > > > > I need to use that info in a script, that's why these env vars. > > > > > > Ok. Honestly, it doesn't feel quite right to me. > > > > > > I haven't played with U-Boot's shell too much but is there no way to > > > assign variables from command outputs? > > > > Not that I know of. > > > > > For example, can we do something > > > like `foo=$(cat a.txt)`? If that is possible, maybe add some new > > > subcommands to "sf" that return this information? > > > > What you would want to have is per-device runtime properties, kind-of > > like reduced sysfs, which would need VFS in U-Boot, and we do not have > > that yet. > > Are there any news on this patch ? All I would say is that I do not particularly like what this patch is doing, but I also can't give you an alternative solution to the problem unfortunately. So I drop my $0.02 here and leave it up to you and Jagan to figure it out.
On 10/7/21 8:40 AM, Pratyush Yadav wrote: > On 23/09/21 10:00PM, Marek Vasut wrote: >> On 9/23/21 8:53 PM, Pratyush Yadav wrote: >> > On 14/09/21 05:28AM, Marek Vasut wrote: >> > > Set the SF page size, erase block size and total size as an environment >> > > variable after "sf probe". This lets us discern boards with multiple >> > > distinct SPI flash options and also e.g. set mtdparts accordingly. >> > >> > I don't quite follow the rationale for making these environment >> > variables. Wouldn't you be better off finding this info out via mtd or >> > sf command? >> >> I need to use that info in a script, that's why these env vars. > > Ok. Honestly, it doesn't feel quite right to me. > > I haven't played with U-Boot's shell too much but is there no way to > assign variables from command outputs? For example, can we do something > like `foo=$(cat a.txt)`? If that is possible, maybe add some new > subcommands to "sf" that return this information? > There is not. IMO the current solution (exporting to environment variables in an ad-hoc manner) is rather ugly, but fixing it is difficult :) --Sean
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index d5d905fa5a1..448653b9931 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -15,6 +15,7 @@ #include <dm.h> #include <dm/device_compat.h> #include <dm/devres.h> +#include <env.h> #include <linux/bitops.h> #include <linux/err.h> #include <linux/errno.h> @@ -3829,8 +3830,11 @@ int spi_nor_scan(struct spi_nor *nor) #ifndef CONFIG_SPL_BUILD printf("SF: Detected %s with page size ", nor->name); print_size(nor->page_size, ", erase size "); + env_set_hex("sf_pagesize", nor->page_size); print_size(nor->erase_size, ", total "); + env_set_hex("sf_erasesize", nor->erase_size); print_size(nor->size, ""); + env_set_hex("sf_size", nor->size); puts("\n"); #endif
Set the SF page size, erase block size and total size as an environment variable after "sf probe". This lets us discern boards with multiple distinct SPI flash options and also e.g. set mtdparts accordingly. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Vignesh R <vigneshr@ti.com> --- drivers/mtd/spi/spi-nor-core.c | 4 ++++ 1 file changed, 4 insertions(+)