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

[PATCH v4] 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. 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>
---
Changes from v3 to v4:
*) No functional change. Splitted the patch out of the series as suggested
   by Nishant.
*) Droppped the RFC tag.

v3: https://lore.kernel.org/all/20240124064930.1787929-3-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 v4] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[Ravi Gunasekaran]

On 1/30/2024 11:56 AM, 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. 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>
> ---
> Changes from v3 to v4:
> *) No functional change. Splitted the patch out of the series as suggested
>    by Nishant.
> *) Droppped the RFC tag.
>
> v3: https://lore.kernel.org/all/20240124064930.1787929-3-danishanwar@ti.com/

Acked-by: Ravi Gunasekaran <r-gunasekaran@ti.com>

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

[Sean Anderson]

On 1/30/24 01:26, 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. The env storage_interface will act as fallback so that the
> existing implementations do not break.
> 
> Also update the FS Loader documentation accordingly.

So why do you want to do this? I don't see what the point of renaming the
variable is, since you are not e.g. adding any new functionality, and we
have to pay for the rename in code size.

--Sean

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

[MD Danish Anwar]

Hi Sean,

On 07/02/24 11:14 pm, Sean Anderson wrote:
> On 1/30/24 01:26, 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. The env storage_interface will act as fallback so that the
>> existing implementations do not break.
>>
>> Also update the FS Loader documentation accordingly.
> 
> So why do you want to do this? I don't see what the point of renaming the
> variable is, since you are not e.g. adding any new functionality, and we
> have to pay for the rename in code size.
> 

I am upstreaming TI's ICSSG driver for u-boot and during code review
Roger Quadros <rogerq@kernel.org> commented asking to rename this
variable [1]. I think the motive here was to keep all variables used by
fs-loader driver with 'fw_' prefix. All other variables had 'fw_' prefix
except for storage_interface.

[1]
https://lore.kernel.org/all/4721f3b9-f823-47f3-b4f3-ed40002af31b@kernel.org/

> --Sean

-- 
Thanks and Regards,
Danish

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

[Roger Quadros]

On 08/02/2024 07:19, MD Danish Anwar wrote:
> Hi Sean,
> 
> On 07/02/24 11:14 pm, Sean Anderson wrote:
>> On 1/30/24 01:26, 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. The env storage_interface will act as fallback so that the
>>> existing implementations do not break.
>>>
>>> Also update the FS Loader documentation accordingly.
>>
>> So why do you want to do this? I don't see what the point of renaming the
>> variable is, since you are not e.g. adding any new functionality, and we
>> have to pay for the rename in code size.
>>
> 
> I am upstreaming TI's ICSSG driver for u-boot and during code review
> Roger Quadros <rogerq@kernel.org> commented asking to rename this
> variable [1]. I think the motive here was to keep all variables used by
> fs-loader driver with 'fw_' prefix. All other variables had 'fw_' prefix
> except for storage_interface.

Can you please mention this motive in the Commit message?

> 
> [1]
> https://lore.kernel.org/all/4721f3b9-f823-47f3-b4f3-ed40002af31b@kernel.org/
> 
>> --Sean
> 

-- 
cheers,
-roger

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

[MD Danish Anwar]

On 08/02/24 5:28 pm, Roger Quadros wrote:
> 
> 
> On 08/02/2024 07:19, MD Danish Anwar wrote:
>> Hi Sean,
>>
>> On 07/02/24 11:14 pm, Sean Anderson wrote:
>>> On 1/30/24 01:26, 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. The env storage_interface will act as fallback so that the
>>>> existing implementations do not break.
>>>>
>>>> Also update the FS Loader documentation accordingly.
>>>
>>> So why do you want to do this? I don't see what the point of renaming the
>>> variable is, since you are not e.g. adding any new functionality, and we
>>> have to pay for the rename in code size.
>>>
>>
>> I am upstreaming TI's ICSSG driver for u-boot and during code review
>> Roger Quadros <rogerq@kernel.org> commented asking to rename this
>> variable [1]. I think the motive here was to keep all variables used by
>> fs-loader driver with 'fw_' prefix. All other variables had 'fw_' prefix
>> except for storage_interface.
> 
> Can you please mention this motive in the Commit message?
> 

Sure Roger, I will resend the patch after updating commit message.

>>
>> [1]
>> https://lore.kernel.org/all/4721f3b9-f823-47f3-b4f3-ed40002af31b@kernel.org/
>>
>>> --Sean
>>
> 

-- 
Thanks and Regards,
Danish