[PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[MD Danish Anwar]

The fs-loader driver reads env storage_interface and uses it to load
firmware file into memory using the medium set by env. Update the driver
to use env fw_storage_interface as this variable is only used to load
firmwares. This is to keep all variables used by fs-loader driver with
'fw_' prefix. All other variables have 'fw_' prefix except for
storage_interface.

The env storage_interface will act as fallback so that the
existing implementations do not break.

Also update the FS Loader documentation accordingly.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
Cc: Sean Anderson <seanga2@gmail.com>
Changes from v4 to v5:
*) Modified commit message to make the motive cleared for this commit.
*) Added RB tag of Ravi.
*) Rebased on the latest u-boot/master [commit
   a4650bf65e4b7d3ef04c90ba8031374428e4a682]

v4: https://lore.kernel.org/all/20240130062627.2344282-1-danishanwar@ti.com/

 doc/develop/driver-model/fs_firmware_loader.rst | 5 ++++-
 drivers/misc/fs_loader.c                        | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/doc/develop/driver-model/fs_firmware_loader.rst b/doc/develop/driver-model/fs_firmware_loader.rst
index 149b8b436e..410cc1442d 100644
--- a/doc/develop/driver-model/fs_firmware_loader.rst
+++ b/doc/develop/driver-model/fs_firmware_loader.rst
@@ -98,8 +98,11 @@ through the U-Boot environment variable during run time.
 
 For examples:
 
+fw_storage_interface:
+  Firmware storage interface, it can be "mmc", "usb", "sata" or "ubi".
 storage_interface:
-  Storage interface, it can be "mmc", "usb", "sata" or "ubi".
+  Storage interface, it can be "mmc", "usb", "sata" or "ubi". This acts
+  as a fallback if fw_storage_interface is not set.
 fw_dev_part:
   Block device number and its partition, it can be "0:1".
 fw_ubi_mtdpart:
diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
index 1ffc199ba1..3798dab5b6 100644
--- a/drivers/misc/fs_loader.c
+++ b/drivers/misc/fs_loader.c
@@ -153,7 +153,10 @@ static int fw_get_filesystem_firmware(struct udevice *dev)
 	char *storage_interface, *dev_part, *ubi_mtdpart, *ubi_volume;
 	int ret;
 
-	storage_interface = env_get("storage_interface");
+	storage_interface = env_get("fw_storage_interface");
+	if (!storage_interface)
+		storage_interface = env_get("storage_interface");
+
 	dev_part = env_get("fw_dev_part");
 	ubi_mtdpart = env_get("fw_ubi_mtdpart");
 	ubi_volume = env_get("fw_ubi_volume");
-- 
2.34.1

Re: [PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[MD Danish Anwar]

On 09/02/24 3:38 pm, MD Danish Anwar wrote:
> The fs-loader driver reads env storage_interface and uses it to load
> firmware file into memory using the medium set by env. Update the driver
> to use env fw_storage_interface as this variable is only used to load
> firmwares. This is to keep all variables used by fs-loader driver with
> 'fw_' prefix. All other variables have 'fw_' prefix except for
> storage_interface.
> 
> The env storage_interface will act as fallback so that the
> existing implementations do not break.
> 
> Also update the FS Loader documentation accordingly.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---

Hi Tom / Sean, can you please pick this patch if there is no pending
comments to address.

> Cc: Sean Anderson <seanga2@gmail.com>
> Changes from v4 to v5:
> *) Modified commit message to make the motive cleared for this commit.
> *) Added RB tag of Ravi.
> *) Rebased on the latest u-boot/master [commit
>    a4650bf65e4b7d3ef04c90ba8031374428e4a682]
> 
> v4: https://lore.kernel.org/all/20240130062627.2344282-1-danishanwar@ti.com/
> 
>  doc/develop/driver-model/fs_firmware_loader.rst | 5 ++++-
>  drivers/misc/fs_loader.c                        | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/develop/driver-model/fs_firmware_loader.rst b/doc/develop/driver-model/fs_firmware_loader.rst
> index 149b8b436e..410cc1442d 100644
> --- a/doc/develop/driver-model/fs_firmware_loader.rst
> +++ b/doc/develop/driver-model/fs_firmware_loader.rst
> @@ -98,8 +98,11 @@ through the U-Boot environment variable during run time.
>  
>  For examples:
>  
> +fw_storage_interface:
> +  Firmware storage interface, it can be "mmc", "usb", "sata" or "ubi".
>  storage_interface:
> -  Storage interface, it can be "mmc", "usb", "sata" or "ubi".
> +  Storage interface, it can be "mmc", "usb", "sata" or "ubi". This acts
> +  as a fallback if fw_storage_interface is not set.
>  fw_dev_part:
>    Block device number and its partition, it can be "0:1".
>  fw_ubi_mtdpart:
> diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
> index 1ffc199ba1..3798dab5b6 100644
> --- a/drivers/misc/fs_loader.c
> +++ b/drivers/misc/fs_loader.c
> @@ -153,7 +153,10 @@ static int fw_get_filesystem_firmware(struct udevice *dev)
>  	char *storage_interface, *dev_part, *ubi_mtdpart, *ubi_volume;
>  	int ret;
>  
> -	storage_interface = env_get("storage_interface");
> +	storage_interface = env_get("fw_storage_interface");
> +	if (!storage_interface)
> +		storage_interface = env_get("storage_interface");
> +
>  	dev_part = env_get("fw_dev_part");
>  	ubi_mtdpart = env_get("fw_ubi_mtdpart");
>  	ubi_volume = env_get("fw_ubi_volume");

-- 
Thanks and Regards,
Danish

Re: [PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[Sean Anderson]

Hi Danish,

On 2/27/24 05:26, MD Danish Anwar wrote:
> On 09/02/24 3:38 pm, MD Danish Anwar wrote:
>> The fs-loader driver reads env storage_interface and uses it to load
>> firmware file into memory using the medium set by env. Update the driver
>> to use env fw_storage_interface as this variable is only used to load
>> firmwares. This is to keep all variables used by fs-loader driver with
>> 'fw_' prefix. All other variables have 'fw_' prefix except for
>> storage_interface.
>>
>> The env storage_interface will act as fallback so that the
>> existing implementations do not break.
>>
>> Also update the FS Loader documentation accordingly.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
> 
> Hi Tom / Sean, can you please pick this patch if there is no pending
> comments to address.
> 

Sorry, I forgot to respond to this earlier.

To be honest, I'm not really convinced. We have plenty of environmental
variables which are inconsistent (e.g. ethaddr, eth2addr, eth3addr) and it
doesn't cause any issues. While fixing code has no cost, the environment
is an ABI which we can't break. So we'd have to support both of these
variables forever. I'm not really a fan of doing that without good reason,
and I think aesthetics of the variable name isn't really compelling.

--Sean

Re: [PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[MD Danish Anwar]

On 27/02/24 7:33 pm, Sean Anderson wrote:
> Hi Danish,
> 
> On 2/27/24 05:26, MD Danish Anwar wrote:
>> On 09/02/24 3:38 pm, MD Danish Anwar wrote:
>>> The fs-loader driver reads env storage_interface and uses it to load
>>> firmware file into memory using the medium set by env. Update the driver
>>> to use env fw_storage_interface as this variable is only used to load
>>> firmwares. This is to keep all variables used by fs-loader driver with
>>> 'fw_' prefix. All other variables have 'fw_' prefix except for
>>> storage_interface.
>>>
>>> The env storage_interface will act as fallback so that the
>>> existing implementations do not break.
>>>
>>> Also update the FS Loader documentation accordingly.
>>>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---
>>
>> Hi Tom / Sean, can you please pick this patch if there is no pending
>> comments to address.
>>
> 
> Sorry, I forgot to respond to this earlier.
> 
> To be honest, I'm not really convinced. We have plenty of environmental
> variables which are inconsistent (e.g. ethaddr, eth2addr, eth3addr) and it
> doesn't cause any issues. While fixing code has no cost, the environment
> is an ABI which we can't break. So we'd have to support both of these
> variables forever. I'm not really a fan of doing that without good reason,
> and I think aesthetics of the variable name isn't really compelling.
> 

Roger, should I keep the env variable name as it is and don't rename it?
Sean's concern seems valid, can you please comment here.

> --Sean

-- 
Thanks and Regards,
Danish

Re: [PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[Roger Quadros]

On 28/02/2024 11:02, MD Danish Anwar wrote:
> 
> On 27/02/24 7:33 pm, Sean Anderson wrote:
>> Hi Danish,
>>
>> On 2/27/24 05:26, MD Danish Anwar wrote:
>>> On 09/02/24 3:38 pm, MD Danish Anwar wrote:
>>>> The fs-loader driver reads env storage_interface and uses it to load
>>>> firmware file into memory using the medium set by env. Update the driver
>>>> to use env fw_storage_interface as this variable is only used to load
>>>> firmwares. This is to keep all variables used by fs-loader driver with
>>>> 'fw_' prefix. All other variables have 'fw_' prefix except for
>>>> storage_interface.
>>>>
>>>> The env storage_interface will act as fallback so that the
>>>> existing implementations do not break.
>>>>
>>>> Also update the FS Loader documentation accordingly.
>>>>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> ---
>>>
>>> Hi Tom / Sean, can you please pick this patch if there is no pending
>>> comments to address.
>>>
>>
>> Sorry, I forgot to respond to this earlier.
>>
>> To be honest, I'm not really convinced. We have plenty of environmental
>> variables which are inconsistent (e.g. ethaddr, eth2addr, eth3addr) and it
>> doesn't cause any issues. While fixing code has no cost, the environment
>> is an ABI which we can't break. So we'd have to support both of these
>> variables forever. I'm not really a fan of doing that without good reason,
>> and I think aesthetics of the variable name isn't really compelling.
>>
> 
> Roger, should I keep the env variable name as it is and don't rename it?
> Sean's concern seems valid, can you please comment here.

Sure.

-- 
cheers,
-roger