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
signature.asc
Description: PGP signature
