[PATCH v3 3/5] ufs: extending ufs cmd line

[Tom Rini]

On Mon, Jun 08, 2026 at 01:33:27PM +0300, Raz Ben Yehuda wrote:

> The UFS tool provides utilities for querying, displaying, and modifying
> UFS device configuration and descriptor information.

This still reads like you're adding a new command, which you aren't
anymore. Please reword all of this to make sense again given that it's
now part of the existing ufs command.

[snip]
> diff --git a/cmd/ufs.c b/cmd/ufs.c
> index 790dab50f18..203a58d6026 100644
> --- a/cmd/ufs.c
> +++ b/cmd/ufs.c
> @@ -3,12 +3,1191 @@
>   * ufs.c - UFS specific U-Boot commands
>   *
>   * Copyright (C) 2019 Texas Instruments Incorporated - https://www.ti.com
> - *
> + * Copyright (C) 2026 Mobileye Incorporated - https://www.mobileye.com
>   */
> +#include <asm/io.h>
> +#include <dm/devres.h>
> +#include <errno.h>
> +#include <limits.h>
> +#include <linux/bitops.h>
> +#include <linux/byteorder/little_endian.h>
> +#include <linux/compiler.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <malloc.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <time.h>
> +#include <dm/uclass.h>
> +#include <dm/device.h>
>  #include <command.h>
> +
>  #include <ufs.h>
> -#include <vsprintf.h>
> -#include <linux/string.h>

Please audit this new big list of includes to make sure you're only
adding what's required.

[snip]
> +int ufshcd_query_desc_idn_power(struct ufs_hba *hba, int lun,
> +				struct ufs_power_parameters_descriptor
> +				*out_desc)
> +{
> +	unsigned char Space[QUERY_DESC_POWER_DEF_SIZE];

That's not a good variable name, and do we need to allocate this on the
stack like that?

> +#define CMD_HELP \
> +		"query:\n" \
> +		"\tDevice desc (0h)\n" \
> +		"\tGeometry desc(7h)\n" \
> +		"\tPower desc (8h)\n" \
> +		"\tUnit desc(2h)\n" \
> +		"\tConfig desc(1h)\n" \
> +		"****************************\n" \
> +		"\n\tset_lun [LUN] (parm)\n" \
> +		"\t\tlu_enable\n" \
> +		"\t\tboot_lun_id\n" \
> +		"\t\tlu_write_protect\n" \
> +		"\t\tmemory_type\n" \
> +		"\t\tdata_reliability\n" \
> +		"\t\tnum_allocunits\n" \
> +		"\t\tlogical_block_size\n" \
> +		"\t\tprovisioning_type\n" \
> +		"\t\tcontext_capabilities\n" \
> +		"\t\tlu_num_write_booster_buffer_allocunits\n\n" \
> +		"****************************\n" \
> +		"\n\tset_cfg_desc [parm] [value]\n" \
> +		"\t\tlength\n" \
> +		"\t\tdescriptor_idn\n" \
> +		"\t\tconf_desc_continue\n" \
> +		"\t\tboot_enable\n" \
> +		"\t\tdescr_access_en\n" \
> +		"\t\tinitpower_mode\n" \
> +		"\t\thigh_priority_lun\n" \
> +		"\t\tsecure_removal_type\n" \
> +		"\t\tinit_active_icc_level\n" \
> +		"\t\tperiodic_rtc_update\n" \
> +		"\t\treserved_HPB\n" \
> +		"\t\trpmb_region_enable\n" \
> +		"\t\trpmb_region1_size\n" \
> +		"\t\trpmb_region2_size\n" \
> +		"\t\trpmb_region3_size\n" \
> +		"\t\twrite_booster_buffer_preserve_user_space_en\n" \
> +		"\t\twrite_booster_buffer_type\n" \
> +		"\t\tnum_shared_write_booster_buffer_allocunits\n" \
> +		"****************************\n" \
> +		"\n\tgetflag [flag name]\n" \
> +		"\t\tdeviceinit\n" \
> +		"\t\tpermanent_wpe\n" \
> +		"\t\tpwr_on_wpe\n" \
> +		"\t\tbkops_en\n" \
> +		"\t\tlife_span_mode\n" \
> +		"\t\tpurge_enable\n" \
> +		"\t\tfphy_resource_removal\n" \
> +		"\t\tbusy_rtc\n" \
> +		"\t\tpermanently_disable_fw_update\n" \
> +		"\t\twrite_booster_en\n" \
> +		"\t\twb_buf_flush_en\n" \
> +		"\t\twb_buf_flush_h8\n" \
> +		"****************************\n" \
> +		"\texamples :\n" \
> +		"\n\nto write lun please\n" \
> +		"\tufs query 0 1\n" \
> +		"\tfix the luns, disable lu_enable\n" \
> +		"\tuse ufs set_lun [LUN] num_allocunits=xxx to set size\n" \
> +		"\tufs set_cfg_desc num_shared_write_booster_buffer_allocunits 100000\n" \
> +		"\tufs set_cfg_desc write_booster_buffer_type 1\n" \
> +		"\tufs commit\n"
> +
> +void ufs_tool_help(void)
> +{
> +	puts(CMD_HELP);
> +}

Please integrate this, properly, to the existing command. It seems like
you just copy/pasted the previous ufs_tool.c file in to cmd/ufs.c
instead.

[snip]
> diff --git a/include/ufs.h b/include/ufs.h
> index f6e27d90e43..89fde263fcf 100644
> --- a/include/ufs.h
> +++ b/include/ufs.h
[snip]
> +int ufshcd_query_desc_idn_geometry(struct ufs_hba *hba,
> +				   struct ufs_geometry_descriptor *decs);
> +int ufshcd_query_desc_idn_config(struct ufs_hba *hba, struct ufs_config_descriptor *cfg);
> +int ufshcd_query_desc_idn_device(struct ufs_hba *hba, struct ufs_device_descriptor *desc);
> +int ufshcd_query_desc_idn_unit(struct ufs_hba *hba, int lun,
> +			       struct unit_descriptor *desc);
> +void ufshcd_print_idn_geometry_desc(struct ufs_geometry_descriptor *desc);
> +void ufshcd_print_device_descriptor(struct ufs_device_descriptor *desc);
> +void ufshcd_print_unit_descriptor(struct unit_descriptor *desc);
> +int ufshcd_query_desc_idn_power(struct ufs_hba *hba, int lun,
> +				struct ufs_power_parameters_descriptor *desc);
> +void print_ufs_power_parameters(struct ufs_power_parameters_descriptor *desc);
> +int ufshcd_query_user_flag(struct ufs_hba *hba, const char *flag_name);

These all need appropriate kernel-doc comments.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20260608/0bb58ac4/attachment.sig>