[PATCH v1 0/3] Enable env in UFS

[PATCH v1 0/3] Enable env in UFS

[Varadarajan Narayanan]

The qcs9100 based Ride platforms have UFS as their primary storage.
Hence add support to U-Boot env framework to be able to save and
retrieve the environment from UFS. The environment will be
saved/retrieved from the partition specified in the config option
CONFIG_SYS_UFS_ENV_PART.

Also add an API to convert partition name string to block device
descriptor for UFS. This API will be used to get the block device
descriptor for the partition specified in CONFIG_SYS_UFS_ENV_PART.

Varadarajan Narayanan (3):
  scsi: Implement get_blk() function
  env: Add support for storing env variables in UFS
  configs: qcs9100: Enable env in UFS

 configs/qcs9100_defconfig  |   4 ++
 drivers/scsi/scsi-uclass.c |  30 ++++++++++
 env/Kconfig                |  15 ++++-
 env/Makefile               |   1 +
 env/env.c                  |   3 +
 env/ufs.c                  | 115 +++++++++++++++++++++++++++++++++++++
 include/env_internal.h     |   1 +
 include/scsi.h             |  12 ++++
 8 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 env/ufs.c


base-commit: 244e61fbb7f5045e4e187024f7ae80434c952145
-- 
2.34.1

[PATCH v1 1/3] scsi: Implement get_blk() function

[Varadarajan Narayanan]

Add a function to obtain the block device for SCSI.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 drivers/scsi/scsi-uclass.c | 30 ++++++++++++++++++++++++++++++
 include/scsi.h             | 12 ++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
index 1ee8236c05c..c2e34082bce 100644
--- a/drivers/scsi/scsi-uclass.c
+++ b/drivers/scsi/scsi-uclass.c
@@ -10,7 +10,9 @@
 
 #define LOG_CATEGORY UCLASS_SCSI
 
+#include <blk.h>
 #include <dm.h>
+#include <part.h>
 #include <scsi.h>
 
 int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
@@ -23,6 +25,34 @@ int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
 	return ops->exec(dev, pccb);
 }
 
+int scsi_get_blk(const char *partition_name,
+		 struct blk_desc **blk_desc_ptr,
+		 struct disk_partition *part_info_ptr)
+{
+	static int is_scsi_scanned;
+	struct blk_desc *blk;
+	int i, ret;
+
+	if (!is_scsi_scanned) {
+		scsi_scan(false /* no verbose */);
+		is_scsi_scanned = 1;
+	}
+
+	for (i = 0; i < blk_find_max_devnum(UCLASS_SCSI) + 1; i++) {
+		ret = blk_get_desc(UCLASS_SCSI, i, &blk);
+		if (ret)
+			continue;
+
+		ret = part_get_info_by_name(blk, partition_name, part_info_ptr);
+		if (ret > 0) {
+			*blk_desc_ptr = blk;
+			return 0;
+		}
+	}
+
+	return -1;
+}
+
 int scsi_bus_reset(struct udevice *dev)
 {
 	struct scsi_ops *ops = scsi_get_ops(dev);
diff --git a/include/scsi.h b/include/scsi.h
index b18ae37b861..1ba55516588 100644
--- a/include/scsi.h
+++ b/include/scsi.h
@@ -9,6 +9,7 @@
 #include <asm/cache.h>
 #include <bouncebuf.h>
 #include <linux/dma-direction.h>
+#include <part.h>
 
 struct udevice;
 
@@ -349,6 +350,17 @@ int scsi_scan(bool verbose);
  */
 int scsi_scan_dev(struct udevice *dev, bool verbose);
 
+/**
+ * scsi_get_blk() - Provides SCSI partition information.
+ *
+ * @partition_name: Partition name for fetching its info
+ * @blk_desc_ptr:   Provides the blk descriptor
+ * @part_info_ptr:  Provides partition info
+ */
+int scsi_get_blk(const char *partition_name,
+		 struct blk_desc **blk_desc_ptr,
+		 struct disk_partition *part_info_ptr);
+
 #define SCSI_IDENTIFY					0xC0  /* not used */
 
 /* Hardware errors  */
-- 
2.34.1

Re: [PATCH v1 1/3] scsi: Implement get_blk() function

[Heinrich Schuchardt]

On 01.04.25 10:00, Varadarajan Narayanan wrote:
> Add a function to obtain the block device for SCSI.
>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>   drivers/scsi/scsi-uclass.c | 30 ++++++++++++++++++++++++++++++
>   include/scsi.h             | 12 ++++++++++++
>   2 files changed, 42 insertions(+)
>
> diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
> index 1ee8236c05c..c2e34082bce 100644
> --- a/drivers/scsi/scsi-uclass.c
> +++ b/drivers/scsi/scsi-uclass.c
> @@ -10,7 +10,9 @@
>
>   #define LOG_CATEGORY UCLASS_SCSI
>
> +#include <blk.h>
>   #include <dm.h>
> +#include <part.h>
>   #include <scsi.h>
>
>   int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
> @@ -23,6 +25,34 @@ int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
>   	return ops->exec(dev, pccb);
>   }
>
> +int scsi_get_blk(const char *partition_name,
> +		 struct blk_desc **blk_desc_ptr,
> +		 struct disk_partition *part_info_ptr)
> +{
> +	static int is_scsi_scanned;
> +	struct blk_desc *blk;
> +	int i, ret;
> +
> +	if (!is_scsi_scanned) {
> +		scsi_scan(false /* no verbose */);
> +		is_scsi_scanned = 1;
> +	}
> +
> +	for (i = 0; i < blk_find_max_devnum(UCLASS_SCSI) + 1; i++) {
> +		ret = blk_get_desc(UCLASS_SCSI, i, &blk);
> +		if (ret)
> +			continue;
> +
> +		ret = part_get_info_by_name(blk, partition_name, part_info_ptr);
> +		if (ret > 0) {
> +			*blk_desc_ptr = blk;
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
>   int scsi_bus_reset(struct udevice *dev)
>   {
>   	struct scsi_ops *ops = scsi_get_ops(dev);
> diff --git a/include/scsi.h b/include/scsi.h
> index b18ae37b861..1ba55516588 100644
> --- a/include/scsi.h
> +++ b/include/scsi.h
> @@ -9,6 +9,7 @@
>   #include <asm/cache.h>
>   #include <bouncebuf.h>
>   #include <linux/dma-direction.h>
> +#include <part.h>
>
>   struct udevice;
>
> @@ -349,6 +350,17 @@ int scsi_scan(bool verbose);
>    */
>   int scsi_scan_dev(struct udevice *dev, bool verbose);
>
> +/**
> + * scsi_get_blk() - Provides SCSI partition information.
> + *
> + * @partition_name: Partition name for fetching its info

Partition names cannot be expected to be unique.
UUIDs should be.

Best regards

Heinrich

> + * @blk_desc_ptr:   Provides the blk descriptor
> + * @part_info_ptr:  Provides partition info
> + */
> +int scsi_get_blk(const char *partition_name,
> +		 struct blk_desc **blk_desc_ptr,
> +		 struct disk_partition *part_info_ptr);
> +
>   #define SCSI_IDENTIFY					0xC0  /* not used */
>
>   /* Hardware errors  */

Re: [PATCH v1 1/3] scsi: Implement get_blk() function

[Varadarajan Narayanan]

On Tue, Apr 01, 2025 at 06:29:31PM +0200, Heinrich Schuchardt wrote:
> On 01.04.25 10:00, Varadarajan Narayanan wrote:
> > Add a function to obtain the block device for SCSI.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >   drivers/scsi/scsi-uclass.c | 30 ++++++++++++++++++++++++++++++
> >   include/scsi.h             | 12 ++++++++++++
> >   2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
> > index 1ee8236c05c..c2e34082bce 100644
> > --- a/drivers/scsi/scsi-uclass.c
> > +++ b/drivers/scsi/scsi-uclass.c
> > @@ -10,7 +10,9 @@
> >
> >   #define LOG_CATEGORY UCLASS_SCSI
> >
> > +#include <blk.h>
> >   #include <dm.h>
> > +#include <part.h>
> >   #include <scsi.h>
> >
> >   int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
> > @@ -23,6 +25,34 @@ int scsi_exec(struct udevice *dev, struct scsi_cmd *pccb)
> >   	return ops->exec(dev, pccb);
> >   }
> >
> > +int scsi_get_blk(const char *partition_name,
> > +		 struct blk_desc **blk_desc_ptr,
> > +		 struct disk_partition *part_info_ptr)
> > +{
> > +	static int is_scsi_scanned;
> > +	struct blk_desc *blk;
> > +	int i, ret;
> > +
> > +	if (!is_scsi_scanned) {
> > +		scsi_scan(false /* no verbose */);
> > +		is_scsi_scanned = 1;
> > +	}
> > +
> > +	for (i = 0; i < blk_find_max_devnum(UCLASS_SCSI) + 1; i++) {
> > +		ret = blk_get_desc(UCLASS_SCSI, i, &blk);
> > +		if (ret)
> > +			continue;
> > +
> > +		ret = part_get_info_by_name(blk, partition_name, part_info_ptr);
> > +		if (ret > 0) {
> > +			*blk_desc_ptr = blk;
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -1;
> > +}
> > +
> >   int scsi_bus_reset(struct udevice *dev)
> >   {
> >   	struct scsi_ops *ops = scsi_get_ops(dev);
> > diff --git a/include/scsi.h b/include/scsi.h
> > index b18ae37b861..1ba55516588 100644
> > --- a/include/scsi.h
> > +++ b/include/scsi.h
> > @@ -9,6 +9,7 @@
> >   #include <asm/cache.h>
> >   #include <bouncebuf.h>
> >   #include <linux/dma-direction.h>
> > +#include <part.h>
> >
> >   struct udevice;
> >
> > @@ -349,6 +350,17 @@ int scsi_scan(bool verbose);
> >    */
> >   int scsi_scan_dev(struct udevice *dev, bool verbose);
> >
> > +/**
> > + * scsi_get_blk() - Provides SCSI partition information.
> > + *
> > + * @partition_name: Partition name for fetching its info
>
> Partition names cannot be expected to be unique.
> UUIDs should be.

Will change to UUID and post a new version.

Thanks
Varada

[PATCH v1 2/3] env: Add support for storing env variables in UFS

[Varadarajan Narayanan]

Allow UFS to be able to store environment variables.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 env/Kconfig            |  15 +++++-
 env/Makefile           |   1 +
 env/env.c              |   3 ++
 env/ufs.c              | 115 +++++++++++++++++++++++++++++++++++++++++
 include/env_internal.h |   1 +
 5 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 env/ufs.c

diff --git a/env/Kconfig b/env/Kconfig
index 4438f0b392c..8ea1f87cb18 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -74,7 +74,7 @@ config ENV_IS_DEFAULT
 		     !ENV_IS_IN_MMC && !ENV_IS_IN_NAND && \
 		     !ENV_IS_IN_NVRAM && !ENV_IS_IN_ONENAND && \
 		     !ENV_IS_IN_REMOTE && !ENV_IS_IN_SPI_FLASH && \
-		     !ENV_IS_IN_UBI
+		     !ENV_IS_IN_UBI && !ENV_IS_IN_UFS
 	select ENV_IS_NOWHERE
 
 config ENV_IS_NOWHERE
@@ -297,6 +297,13 @@ config ENV_IS_IN_NAND
 	  Currently, CONFIG_ENV_OFFSET_REDUND is not supported when
 	  using CONFIG_ENV_OFFSET_OOB.
 
+config ENV_IS_IN_UFS
+	bool "Environment in an UFS device"
+	depends on UFS
+	help
+	  Define this if you have an UFS device which you want to use for the
+	  environment.
+
 config ENV_RANGE
 	hex "Length of the region in which the environment can be written"
 	depends on ENV_IS_IN_NAND
@@ -706,6 +713,12 @@ config ENV_MMC_USE_DT
 	  The 2 defines CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND
 	  are not used as fallback.
 
+config SYS_UFS_ENV_PART
+	string "UFS partition name for saving environment"
+	depends on ENV_IS_IN_UFS
+	help
+	  Name of the UFS partition that you want to store the environment in.
+
 config USE_DEFAULT_ENV_FILE
 	bool "Create default environment from file"
 	help
diff --git a/env/Makefile b/env/Makefile
index a54e924d419..4a5de0dea1d 100644
--- a/env/Makefile
+++ b/env/Makefile
@@ -27,5 +27,6 @@ obj-$(CONFIG_$(PHASE_)ENV_IS_IN_EXT4) += ext4.o
 obj-$(CONFIG_$(PHASE_)ENV_IS_IN_NAND) += nand.o
 obj-$(CONFIG_$(PHASE_)ENV_IS_IN_SPI_FLASH) += sf.o
 obj-$(CONFIG_$(PHASE_)ENV_IS_IN_FLASH) += flash.o
+obj-$(CONFIG_$(PHASE_)ENV_IS_IN_UFS) += ufs.o
 
 CFLAGS_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)
diff --git a/env/env.c b/env/env.c
index bcc189e14db..4535af92ba9 100644
--- a/env/env.c
+++ b/env/env.c
@@ -46,6 +46,9 @@ static enum env_location env_locations[] = {
 #ifdef CONFIG_ENV_IS_IN_MMC
 	ENVL_MMC,
 #endif
+#ifdef CONFIG_ENV_IS_IN_UFS
+	ENVL_UFS,
+#endif
 #ifdef CONFIG_ENV_IS_IN_NAND
 	ENVL_NAND,
 #endif
diff --git a/env/ufs.c b/env/ufs.c
new file mode 100644
index 00000000000..d44053d3551
--- /dev/null
+++ b/env/ufs.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2008-2011 Freescale Semiconductor, Inc.
+ */
+
+/* #define DEBUG */
+
+#include <asm/global_data.h>
+
+#include <command.h>
+#include <env.h>
+#include <env_internal.h>
+#include <fdtdec.h>
+#include <linux/stddef.h>
+#include <malloc.h>
+#include <memalign.h>
+#include <part.h>
+#include <search.h>
+#include <scsi.h>
+#include <errno.h>
+#include <dm/ofnode.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+static env_t envbuf;
+
+struct env_ufs_info {
+	struct blk_desc *blk;
+	struct disk_partition part;
+	int count;
+};
+
+static struct env_ufs_info env_part;
+
+static inline struct env_ufs_info *env_ufs_get_part(void)
+{
+	struct env_ufs_info *ep = &env_part;
+
+	if (scsi_get_blk(CONFIG_SYS_UFS_ENV_PART, &ep->blk, &ep->part))
+		return NULL;
+
+	ep->count = CONFIG_ENV_SIZE / ep->part.blksz;
+
+	return ep;
+}
+
+static int env_ufs_save(void)
+{
+	struct env_ufs_info *ep = env_ufs_get_part();
+	int ret;
+
+	if (!ep)
+		return -ENOENT;
+
+	ret = env_export(&envbuf);
+	if (ret)
+		return ret;
+
+	if (blk_dwrite(ep->blk, ep->part.start, ep->count, &envbuf) != ep->count)
+		return -EIO;
+
+	return 0;
+}
+
+static int env_ufs_erase(void)
+{
+	struct env_ufs_info *ep = env_ufs_get_part();
+
+	if (!ep)
+		return -ENOENT;
+
+	return (int)blk_derase(ep->blk, ep->part.start, ep->count);
+}
+
+#if defined(ENV_IS_EMBEDDED)
+static int env_ufs_load(void)
+{
+	return 0;
+}
+#else
+static int env_ufs_load(void)
+{
+	struct env_ufs_info *ep = env_ufs_get_part();
+	int ret;
+
+	if (!ep) {
+		env_set_default(CONFIG_SYS_UFS_ENV_PART " partition not found", 0);
+		return -ENOENT;
+	}
+
+	if (blk_dread(ep->blk, ep->part.start, ep->count, &envbuf) != ep->count) {
+		env_set_default(CONFIG_SYS_UFS_ENV_PART " partition read failed", 0);
+		return -EIO;
+	}
+
+	ret = env_import((char *)&envbuf, 1, H_EXTERNAL);
+	if (ret) {
+		debug("ENV import failed\n");
+		env_set_default("Cannot load environment", 0);
+	} else {
+		gd->env_addr = (ulong)envbuf.data;
+	}
+
+	return ret;
+}
+#endif
+
+U_BOOT_ENV_LOCATION(ufs) = {
+	.location	= ENVL_UFS,
+	ENV_NAME("UFS")
+	.load		= env_ufs_load,
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_XPL_BUILD)
+	.save		= env_save_ptr(env_ufs_save),
+	.erase		= ENV_ERASE_PTR(env_ufs_erase),
+#endif
+};
diff --git a/include/env_internal.h b/include/env_internal.h
index c1c0727e4d0..acc2ba19e86 100644
--- a/include/env_internal.h
+++ b/include/env_internal.h
@@ -114,6 +114,7 @@ enum env_location {
 	ENVL_REMOTE,
 	ENVL_SPI_FLASH,
 	ENVL_UBI,
+	ENVL_UFS,
 	ENVL_NOWHERE,
 
 	ENVL_COUNT,
-- 
2.34.1

[PATCH v1 3/3] configs: qcs9100: Enable env in UFS

[Varadarajan Narayanan]

Enable CONFIG_ENV_IS_IN_UFS to store environment variables in UFS.
Set env variables partition name as 'ubootenv'.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 configs/qcs9100_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configs/qcs9100_defconfig b/configs/qcs9100_defconfig
index 10ff4d25398..d56f08f0ad0 100644
--- a/configs/qcs9100_defconfig
+++ b/configs/qcs9100_defconfig
@@ -14,5 +14,9 @@ CONFIG_DEBUG_UART_CLOCK=14745600
 # Address where U-Boot will be loaded
 CONFIG_TEXT_BASE=0xaf000000
 CONFIG_REMAKE_ELF=y
+CONFIG_ENV_IS_IN_UFS=y
+CONFIG_SYS_UFS_ENV_PART="ubootenv"
 
 CONFIG_DEFAULT_DEVICE_TREE="qcom/qcs9100-ride-r3"
+# CONFIG_ENV_IS_DEFAULT is not set
+# CONFIG_ENV_IS_NOWHERE is not set
-- 
2.34.1

Re: [PATCH v1 3/3] configs: qcs9100: Enable env in UFS

[Michal Simek]

On 4/1/25 10:00, Varadarajan Narayanan wrote:
> Enable CONFIG_ENV_IS_IN_UFS to store environment variables in UFS.
> Set env variables partition name as 'ubootenv'.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>   configs/qcs9100_defconfig | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/configs/qcs9100_defconfig b/configs/qcs9100_defconfig
> index 10ff4d25398..d56f08f0ad0 100644
> --- a/configs/qcs9100_defconfig
> +++ b/configs/qcs9100_defconfig
> @@ -14,5 +14,9 @@ CONFIG_DEBUG_UART_CLOCK=14745600
>   # Address where U-Boot will be loaded
>   CONFIG_TEXT_BASE=0xaf000000
>   CONFIG_REMAKE_ELF=y
> +CONFIG_ENV_IS_IN_UFS=y
> +CONFIG_SYS_UFS_ENV_PART="ubootenv"
>   
>   CONFIG_DEFAULT_DEVICE_TREE="qcom/qcs9100-ride-r3"
> +# CONFIG_ENV_IS_DEFAULT is not set
> +# CONFIG_ENV_IS_NOWHERE is not set


That newline in the middle is weird.
You should start to use
make savedefconfig
cp defconfig configs/qcs9100_defconfig

to have it aligned with Kconfig layout.

M

Re: [PATCH v1 0/3] Enable env in UFS

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

On Tue, Apr 01, 2025 at 01:30:12PM +0530, Varadarajan Narayanan wrote:

> The qcs9100 based Ride platforms have UFS as their primary storage.
> Hence add support to U-Boot env framework to be able to save and
> retrieve the environment from UFS. The environment will be
> saved/retrieved from the partition specified in the config option
> CONFIG_SYS_UFS_ENV_PART.
> 
> Also add an API to convert partition name string to block device
> descriptor for UFS. This API will be used to get the block device
> descriptor for the partition specified in CONFIG_SYS_UFS_ENV_PART.

In general, I'm glad to see this, thanks! In specifics, Marek is trying
to bring more consistency to some of the env symbol names and so I know
CONFIG_SYS_UFS_ENV_PART is patterned on CONFIG_SYS_MMC_ENV_PART but lets
use CONFIG_ENV_UFS_PART instead which I think follows where Marek is
going.

Also, this seems to be a generic ENV_IS_IN_SCSI implementation and it's
just that UFS is accessed via "SCSI"? Perhaps we should name things a
bit more generically, and it should already support various AHCI SATA
devices out of the box?

However all of that said, do we want to be encouraging environment to be
stored directly in blocks like this rather than a filesystem on UFS?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 0/3] Enable env in UFS

[neil.armstrong]

On 01/04/2025 16:10, Tom Rini wrote:
> On Tue, Apr 01, 2025 at 01:30:12PM +0530, Varadarajan Narayanan wrote:
> 
>> The qcs9100 based Ride platforms have UFS as their primary storage.
>> Hence add support to U-Boot env framework to be able to save and
>> retrieve the environment from UFS. The environment will be
>> saved/retrieved from the partition specified in the config option
>> CONFIG_SYS_UFS_ENV_PART.
>>
>> Also add an API to convert partition name string to block device
>> descriptor for UFS. This API will be used to get the block device
>> descriptor for the partition specified in CONFIG_SYS_UFS_ENV_PART.
> 
> In general, I'm glad to see this, thanks! In specifics, Marek is trying
> to bring more consistency to some of the env symbol names and so I know
> CONFIG_SYS_UFS_ENV_PART is patterned on CONFIG_SYS_MMC_ENV_PART but lets
> use CONFIG_ENV_UFS_PART instead which I think follows where Marek is
> going.
> 
> Also, this seems to be a generic ENV_IS_IN_SCSI implementation and it's
> just that UFS is accessed via "SCSI"? Perhaps we should name things a
> bit more generically, and it should already support various AHCI SATA
> devices out of the box?

I agree we should use scsi to access ufs, we do not need a specific
ufs backend anywhere.

Neil

> 
> However all of that said, do we want to be encouraging environment to be
> stored directly in blocks like this rather than a filesystem on UFS?
> 

Re: [PATCH v1 0/3] Enable env in UFS

[Varadarajan Narayanan]

On Tue, Apr 01, 2025 at 04:37:40PM +0200, neil.armstrong@linaro.org wrote:
> On 01/04/2025 16:10, Tom Rini wrote:
> > On Tue, Apr 01, 2025 at 01:30:12PM +0530, Varadarajan Narayanan wrote:
> >
> > > The qcs9100 based Ride platforms have UFS as their primary storage.
> > > Hence add support to U-Boot env framework to be able to save and
> > > retrieve the environment from UFS. The environment will be
> > > saved/retrieved from the partition specified in the config option
> > > CONFIG_SYS_UFS_ENV_PART.
> > >
> > > Also add an API to convert partition name string to block device
> > > descriptor for UFS. This API will be used to get the block device
> > > descriptor for the partition specified in CONFIG_SYS_UFS_ENV_PART.
> >
> > In general, I'm glad to see this, thanks! In specifics, Marek is trying
> > to bring more consistency to some of the env symbol names and so I know
> > CONFIG_SYS_UFS_ENV_PART is patterned on CONFIG_SYS_MMC_ENV_PART but lets
> > use CONFIG_ENV_UFS_PART instead which I think follows where Marek is
> > going.
> >
> > Also, this seems to be a generic ENV_IS_IN_SCSI implementation and it's
> > just that UFS is accessed via "SCSI"? Perhaps we should name things a
> > bit more generically, and it should already support various AHCI SATA
> > devices out of the box?
>
> I agree we should use scsi to access ufs, we do not need a specific
> ufs backend anywhere.

Reviewers,

Thanks for the feedback. Have posted v2 addressing the concerns.
Please take a look.

> > However all of that said, do we want to be encouraging environment to be
> > stored directly in blocks like this rather than a filesystem on UFS?

Enabling CONFIG_ENV_IS_IN_FAT and configuring CONFIG_ENV_FAT_xxx options
appropriately works for these platforms. However, the current build
system doesn't generate a FS image for default env settings. Hence,
going with direct block storage instead of FS storage. Hope that is ok.

Thanks again.

-Varada

Re: [PATCH v1 0/3] Enable env in UFS

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On Tue, May 06, 2025 at 12:59:56PM +0530, Varadarajan Narayanan wrote:
> On Tue, Apr 01, 2025 at 04:37:40PM +0200, neil.armstrong@linaro.org wrote:
> > On 01/04/2025 16:10, Tom Rini wrote:
> > > On Tue, Apr 01, 2025 at 01:30:12PM +0530, Varadarajan Narayanan wrote:
> > >
> > > > The qcs9100 based Ride platforms have UFS as their primary storage.
> > > > Hence add support to U-Boot env framework to be able to save and
> > > > retrieve the environment from UFS. The environment will be
> > > > saved/retrieved from the partition specified in the config option
> > > > CONFIG_SYS_UFS_ENV_PART.
> > > >
> > > > Also add an API to convert partition name string to block device
> > > > descriptor for UFS. This API will be used to get the block device
> > > > descriptor for the partition specified in CONFIG_SYS_UFS_ENV_PART.
> > >
> > > In general, I'm glad to see this, thanks! In specifics, Marek is trying
> > > to bring more consistency to some of the env symbol names and so I know
> > > CONFIG_SYS_UFS_ENV_PART is patterned on CONFIG_SYS_MMC_ENV_PART but lets
> > > use CONFIG_ENV_UFS_PART instead which I think follows where Marek is
> > > going.
> > >
> > > Also, this seems to be a generic ENV_IS_IN_SCSI implementation and it's
> > > just that UFS is accessed via "SCSI"? Perhaps we should name things a
> > > bit more generically, and it should already support various AHCI SATA
> > > devices out of the box?
> >
> > I agree we should use scsi to access ufs, we do not need a specific
> > ufs backend anywhere.
> 
> Reviewers,
> 
> Thanks for the feedback. Have posted v2 addressing the concerns.
> Please take a look.
> 
> > > However all of that said, do we want to be encouraging environment to be
> > > stored directly in blocks like this rather than a filesystem on UFS?
> 
> Enabling CONFIG_ENV_IS_IN_FAT and configuring CONFIG_ENV_FAT_xxx options
> appropriately works for these platforms. However, the current build
> system doesn't generate a FS image for default env settings. Hence,
> going with direct block storage instead of FS storage. Hope that is ok.

In the case of first boot where the env isn't found, it will use the
default built-in and create it upon "saveenv". This is the normal flow,
so I don't follow you here, sorry.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]