[PATCH] mtd: sf: Set SF parameters as env variables

[PATCH] mtd: sf: Set SF parameters as env variables

[Marek Vasut]

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");
 #endif
 
-- 
2.33.0

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Pratyush Yadav]

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
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Marek Vasut]

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.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Pratyush Yadav]

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?

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Marek Vasut]

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.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Marek Vasut]

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 ?

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Jagan Teki]

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.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Marek Vasut]

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 ?

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Jagan Teki]

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.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Marek Vasut]

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.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH] mtd: sf: Set SF parameters as env variables

[Sean Anderson]

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