[resend v4 09/12] cmd: Add i3c command support.

[Heinrich Schuchardt]

On 4/17/25 04:18, dinesh.maniyam at altera.com wrote:
> From: Dinesh Maniyam <dinesh.maniyam at altera.com>
> 
> Add i3c command file to support select, get i3c device
> target list, read and write operation.
> 
> Signed-off-by: Dinesh Maniyam <dinesh.maniyam at altera.com>
> ---
>   cmd/Kconfig                        |   6 +
>   cmd/Makefile                       |   1 +
>   cmd/i3c.c                          | 193 +++++++++++++++++++++++++++++
>   doc/usage/cmd/i3c.rst              |  98 +++++++++++++++
>   doc/usage/index.rst                |   1 +
>   drivers/i3c/master/dw-i3c-master.c |  35 +++++-
>   include/dw-i3c.h                   |   2 +
>   include/i3c.h                      |   4 +
>   8 files changed, 339 insertions(+), 1 deletion(-)
>   create mode 100644 cmd/i3c.c
>   create mode 100644 doc/usage/cmd/i3c.rst
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 642cc1116e8..551959731f0 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1327,6 +1327,12 @@ config CMD_I2C
>   	help
>   	  I2C support.
>   
> +config CMD_I3C
> +	bool "i3c"
> +	help
> +	  Enable command to list i3c devices connected to the i3c controller
> +	  and perform read and write on the connected i3c devices.
> +
>   config CMD_W1
>   	depends on W1
>   	default y if W1
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 8410be576bb..082470fa104 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -95,6 +95,7 @@ obj-$(CONFIG_CMD_GPIO) += gpio.o
>   obj-$(CONFIG_CMD_HISTORY) += history.o
>   obj-$(CONFIG_CMD_HVC) += smccc.o
>   obj-$(CONFIG_CMD_I2C) += i2c.o
> +obj-$(CONFIG_CMD_I3C) += i3c.o
>   obj-$(CONFIG_CMD_IOTRACE) += iotrace.o
>   obj-$(CONFIG_CMD_HASH) += hash.o
>   obj-$(CONFIG_CMD_IDE) += ide.o disk.o
> diff --git a/cmd/i3c.c b/cmd/i3c.c
> new file mode 100644
> index 00000000000..5631f487cc2
> --- /dev/null
> +++ b/cmd/i3c.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2023 Intel Coporation.
> + */
> +
> +#include <bootretry.h>
> +#include <cli.h>
> +#include <command.h>
> +#include <console.h>
> +#include <dm.h>
> +#include <dw-i3c.h>
> +#include <edid.h>
> +#include <errno.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <asm/byteorder.h>
> +#include <linux/compiler.h>
> +#include <linux/delay.h>
> +#include <u-boot/crc.h>
> +#include <linux/i3c/master.h>
> +
> +static struct udevice *currdev;
> +static struct udevice *prevdev;
> +static struct dw_i3c_master *master;
> +
> +void low_to_high_bytes(void *data, size_t size)
> +{
> +	unsigned char *byte_data = (unsigned char *)data;
> +	size_t start = 0;
> +	size_t end = size - 1;
> +
> +	while (start < end) {
> +		unsigned char temp = byte_data[start];
> +
> +		byte_data[start] = byte_data[end];
> +		byte_data[end] = temp;
> +		start++;
> +		end--;
> +	}
> +}
> +
> +/**
> + * do_i3c() - Handle the "i3c" command-line command
> + * @cmdtp:    Command data struct pointer
> + * @flag:    Command flag
> + * @argc:    Command-line argument count
> + * @argv:    Array of command-line arguments
> + *
> + * Returns zero on success, CMD_RET_USAGE in case of misuse and negative
> + * on error.
> + */
> +static int do_i3c(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	struct uclass *uc;
> +	int ret;
> +	struct udevice *dev_list;
> +	u32 length, bytes;
> +	u8 *data;
> +	u8 *rdata;
> +	u8 device_num;
> +
> +	if (argc > 1) {
> +		ret = uclass_get_device_by_name(UCLASS_I3C, argv[1], &currdev);
> +		if (ret) {
> +			if (!strcmp(argv[1], "help"))
> +				return CMD_RET_USAGE;

You don't need to check for "help". Just return CMD_RET_USAGE if the 
first argument does not match any of the subcommands.

> +
> +			currdev = prevdev;
> +			if (!currdev) {
> +				ret = uclass_get(UCLASS_I3C, &uc);
> +				if (ret)
> +					return CMD_RET_FAILURE;
> +
> +				uclass_foreach_dev(dev_list, uc)
> +					printf("%s (%s)\n", dev_list->name, dev_list->driver->name);
> +
> +				printf("i3c master controller is not initialized: %s\n", argv[1]);
> +				return CMD_RET_FAILURE;
> +			}
> +		} else {
> +			master = dev_get_priv(currdev);
> +			printf("current dev: %s\n", currdev->name);
> +			prevdev = currdev;
> +		}
> +	} else {
> +		if (!currdev) {
> +			printf("No i3c device set!\n");
> +			return CMD_RET_FAILURE;
> +		}
> +		printf("dev: %s\n", currdev->name);
> +	}
> +
> +	if (!strcmp(argv[1], "list")) {

Why would you want to reach this if statment for argc == 1?

> +		ret = uclass_get(UCLASS_I3C, &uc);
> +		if (ret)
> +			return CMD_RET_FAILURE;
> +
> +		uclass_foreach_dev(dev_list, uc)
> +		printf("%s (%s)\n", dev_list->name, dev_list->driver->name);
> +	}
> +
> +	if (!strcmp(argv[1], "current")) {
> +		if (!currdev)
> +			printf("no current dev!\n");
> +		else
> +			printf("current dev: %s\n", currdev->name);
> +	}
> +
> +	if ((!strcmp(argv[1], "write")) && !(argv[2] == NULL) && !(argv[3] == NULL) &&
> +	    !(argv[4] == NULL)) {
> +		length = hextoul(argv[3], NULL);
> +
> +		data = (u8 *)malloc(sizeof(u8) * length);
> +		if (!data) {
> +			debug("Memory allocation for data failed\n");
> +			return -ENOMEM;
> +		}
> +
> +		device_num = hextoul(argv[4], NULL);

device_num is u8. You should check that the value returned by hextoul 
does not exceed 0xff.

> +		bytes = hextoul(argv[2], NULL);

Bytes is only 32bit wide. But the number returned by hextoul can be 
larger than UINT_MAX leading to truncation.

Please, check the value range.

> +
> +		for (int i = 0; i < length; i++)
> +			data[i] = (u8)((bytes >> (8 * i)) & 0xFF);
> +
> +		low_to_high_bytes(data, length);
> +		ret = dm_i3c_write(currdev, device_num, data, length);

For unknown reasons you have defined the num_bytes parameter as signed 
int. So length > INT_MAX will be transferred as a negative number.

Please, be consistent.

> +		if (ret) {
> +			ret = CMD_RET_FAILURE;
> +			goto write_out;
> +		}
> +	}
> +
> +	if (!strcmp(argv[1], "read") && !(argv[2] == NULL) && !(argv[3] == NULL)) {
> +		length = hextoul(argv[2], NULL);

Each subcommand should be handled in a separate static function to avoid 
sphaghetti code.

> +
> +		rdata = (u8 *) malloc(sizeof(u8) * length);

This is not C++ code. Please, remove the (u8 *) conversion.

I would not expect sizeof(u8) to differ from 1. So remove it.

> +		if (!rdata) {
> +			debug("Memory allocation for rdata failed\n");
> +			return -ENOMEM;
> +		}
> +
> +		device_num = hextoul(argv[3], NULL);

Check that hextoul() returns a number in the 0 - 255 range.

> +
> +		ret = dm_i3c_read(currdev, device_num, rdata, length);
> +		if (ret) {
> +			ret =  CMD_RET_FAILURE;

Please, avoid duplicate space after '='.

> +		} else {
> +			printf("Read buff: ");
> +			for (size_t i = 0; i < length; ++i)

length is u32. Why use a larger type for the index?

> +				printf("%02x", rdata[i]);

Please, use print_hex_dump() here. This is especially helpful when 
reading hundres of bytes.

To be honest, it would make more sense to provide an address in memory 
to receive the data than printing up to INT_MAX bytes.

> +
> +			printf("\n");
> +		}
> +
> +		goto read_out;
> +	}
> +
> +	if (!strcmp(argv[1], "device_list")) {
> +		if (master) {
> +			for (int i = 0; i < master->num_i3cdevs; i++) {
> +				printf("device:%d\n", i);
> +				printf("dynamic address:0x%X\n", master->i3cdev[i]->info.dyn_addr);
> +				printf("PID:%llx\n", master->i3cdev[i]->info.pid);
> +				printf("BCR:%X\n", master->i3cdev[i]->info.bcr);
> +				printf("DCR:%X\n", master->i3cdev[i]->info.dcr);
> +				printf("Max Read DS:%X\n", master->i3cdev[i]->info.max_read_ds);
> +				printf("Max Write DS:%X\n", master->i3cdev[i]->info.max_write_ds);
> +				printf("\n");
> +			}
> +		}
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +
> +read_out:
> +	free(rdata);
> +	return ret;
> +
> +write_out:
> +	free(data);
> +	return ret;
> +}
> +
> +U_BOOT_CMD(
> +	i3c,    5,    1,     do_i3c,
> +	"access the system i3c\n",
> +	"i3c write<data><length><device_number> - write to device.\n"
> +	"i3c read<length><device_number> - read from device.\n"
> +	"i3c device_list   - List valid target devices\n"
> +	"i3c <i3c name>   - Select i3c controller.\n"

%s/i3c name/host controller/

> +	"i3c list   - List all the available i3c controller.\n"
> +	"i3c current   - print current i3c controller.\n"
> +);
> diff --git a/doc/usage/cmd/i3c.rst b/doc/usage/cmd/i3c.rst
> new file mode 100644
> index 00000000000..ade7b5c7fc4
> --- /dev/null
> +++ b/doc/usage/cmd/i3c.rst
> @@ -0,0 +1,98 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +.. index::
> +   single: i3c (command)
> +
> +i3c command
> +===========
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    i3c <i3c name>

%s/i3c name/host_controller/

> +    i3c current
> +    i3c list
> +    i3c device_list
> +    i3c write <data> <length> <device_number>

For sending large amounts of data it would be preferable to replace 
direct data by a memory start address.

I would not want to type all bytes needed for displaying an image on an 
LCD screen.


> +    i3c read <length> <device_number>

Same for reading. When reading a camera image you don't want megabytes 
of data flooding the screen.

> +
> +
> +Description
> +-----------
> +
> +The ``i3c`` command is used to probe i3c master controller and perform read

%s/i3c master/an I3C host/

> +and write on the i3c devices.

%s/write on the i3c/write operations on the connected I3C/

i3c
---

Probe an I3C host controller and select it for the other sub-commands.

host_controller
     name of the host controller to be probed and selected

> +
> +i3c current
> +------------
> +
> +* Display the current probed i3c controller.

%s/current probed i3c/currently selected I3C host/

> +
> +i3c list
> +----------
> +
> +List all the i3c controllers defined in the DT.

%s/i3c/I3C/

%s/DT/device-tree/

> +
> +i3c device_list
> +----------------
> +
> +List all the i3c devices' device number, dynamic address, PID, BCR, DCR,
 > +Max Write Speed and Max Read Speed of the probed i3c controller.

List all the I3C devices connected to the current host controller, 
displaying

* device number
* dynamic address
* provisional ID (PID)
* value of the Bus Characteristics Register (BCR)
* value of the Device Characteristics Register (DCR)
* maximum write speed in MHz
* maximum read speed in MHz

Why wouldn't you print the static address, too?

> +
> +i3c write
> +-----------
> +
> +Perform write to the i3c device.
> +
> +i3c read
> +-----------
> +
> +Perform read from the i3c device.


Parameters
----------

> +
> +i3c name

host_controller

> +    i3c name defined in DT

Name of the host controller as defined in the device-tree

> +
> +length
> +    Size of the data.
> +
> +device_number
> +    device number that connected to i3c controller.

It is unclear what is meant by device number. Is it the static address 
or a device number in the driver model?

> +
> +data
> +    Bytes to be written to device.
> +
> +Examples
> +--------
> +
> +Probe i3c0::
> +
> +    => i3c i3c0
> +
> +Check cuurent i3c controller::

Display current I3C host controller::

> +
> +    => i3c current
> +
> +Check the device number and PID of the connected devices::
> +
> +    => i3c device_list
> +
> +Perform write AA byte to a device 0::

Do you mean:

Write a single byte 0xaa to the device with static address 0 on the 
selected host controller.

> +
> +    => i3c write AA 1 0

It is unclear how to format multiple bytes to be written. Please, 
provide an example with multiple bytes.

Does the data have to be capitalized or is lower case working, too?

> +
> +Perform read from a device 0::
> +
> +    => i3c read 1 0

Where is the example output?

Best regards

Heinrich


> +
> +Configuration
> +-------------
> +
> +The ``i3c`` command is only available if CONFIG_CMD_I3C=y.
> +
> +Return value
> +------------
> +
> +If the command succeeds, the return value ``$?`` is set to 0. If an error occurs, the
> +return value ``$?`` is set to 1.
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index bf2335dc8f0..718b606b162 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -80,6 +80,7 @@ Shell commands
>      cmd/if
>      cmd/itest
>      cmd/imxtract
> +   cmd/i3c
>      cmd/load
>      cmd/loadb
>      cmd/loadm
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index bf5d57937c7..623f5b01dbb 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -674,8 +674,11 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
>   	newdevs &= ~olddevs;
>   
>   	for (pos = 0; pos < master->maxdevs; pos++) {
> -		if (newdevs & BIT(pos))
> +		if (newdevs & BIT(pos)) {
>   			i3c_master_add_i3c_dev_locked(m, master->addrs[pos]);
> +			master->i3cdev[pos] = m->this;
> +			master->num_i3cdevs++;
> +		}
>   	}
>   
>   	dw_i3c_master_free_xfer(xfer);
> @@ -1010,8 +1013,38 @@ err_assert_rst:
>   	return ret;
>   }
>   
> +static int dw_i3c_master_priv_read(struct udevice *dev, u8 dev_number,
> +				   u8 *buf, int buf_size)
> +{
> +	struct dw_i3c_master *master = dev_get_priv(dev);
> +	struct i3c_dev_desc *i3cdev = master->i3cdev[dev_number];
> +	struct i3c_priv_xfer i3c_xfers;
> +
> +	i3c_xfers.data.in = buf;
> +	i3c_xfers.len = buf_size;
> +	i3c_xfers.rnw = I3C_MSG_READ;
> +
> +	return dw_i3c_master_priv_xfers(i3cdev, &i3c_xfers, 1);
> +}
> +
> +static int dw_i3c_master_priv_write(struct udevice *dev, u8 dev_number,
> +				    u8 *buf, int buf_size)
> +{
> +	struct dw_i3c_master *master = dev_get_priv(dev);
> +	struct i3c_dev_desc *i3cdev = master->i3cdev[dev_number];
> +	struct i3c_priv_xfer i3c_xfers;
> +
> +	i3c_xfers.data.out = buf;
> +	i3c_xfers.len = buf_size;
> +	i3c_xfers.rnw = I3C_MSG_WRITE;
> +
> +	return dw_i3c_master_priv_xfers(i3cdev, &i3c_xfers, 1);
> +}
> +
>   static const struct dm_i3c_ops dw_i3c_ops = {
>   	.i3c_xfers = dw_i3c_master_priv_xfers,
> +	.read = dw_i3c_master_priv_read,
> +	.write = dw_i3c_master_priv_write,
>   };
>   
>   static const struct udevice_id dw_i3c_ids[] = {
> diff --git a/include/dw-i3c.h b/include/dw-i3c.h
> index 42c37d6dfa2..c652de77404 100644
> --- a/include/dw-i3c.h
> +++ b/include/dw-i3c.h
> @@ -241,6 +241,8 @@ struct dw_i3c_master {
>   	char type[5];
>   	u8 addrs[MAX_DEVS];
>   	bool first_broadcast;
> +	struct i3c_dev_desc *i3cdev[I3C_BUS_MAX_DEVS];
> +	u16 num_i3cdevs;
>   };
>   
>   struct dw_i3c_i2c_dev_data {
> diff --git a/include/i3c.h b/include/i3c.h
> index 6ef4982dcf6..969753ae49d 100644
> --- a/include/i3c.h
> +++ b/include/i3c.h
> @@ -28,6 +28,10 @@ struct dm_i3c_ops {
>   	int (*i3c_xfers)(struct i3c_dev_desc *dev,
>   			 struct i3c_priv_xfer *i3c_xfers,
>   			 int i3c_nxfers);
> +	int (*read)(struct udevice *dev, u8 dev_number,
> +		    u8 *buf, int num_bytes);
> +	int (*write)(struct udevice *dev, u8 dev_number,
> +		     u8 *buf, int num_bytes);
>   };
>   
>   #define i3c_get_ops(dev)	((struct dm_i3c_ops *)(dev)->driver->ops)