> -----Original Message-----
> From: Heinrich Schuchardt <xypron.g...@gmx.de>
> Sent: Thursday, 17 April 2025 3:25 pm
> To: Maniyam, Dinesh <dinesh.mani...@altera.com>
> Cc: Marek <ma...@denx.de>; Simon <simon.k.r.goldschm...@gmail.com>;
> Simon Glass <s...@chromium.org>; Tom Rini <tr...@konsulko.com>; Dario
> Binacchi <dario.binac...@amarulasolutions.com>; Ilias Apalodimas
> <ilias.apalodi...@linaro.org>; Jerome Forissier <jerome.foriss...@linaro.org>;
> Mattijs Korpershoek <mkorpersh...@baylibre.com>; Ibai Erkiaga <ibai.erkiaga-
> elo...@amd.com>; Michal Simek <michal.si...@amd.com>; Dmitry Rokosov
> <ddroko...@salutedevices.com>; Jonas Karlman <jo...@kwiboo.se>; Sebastian
> Reichel <sebastian.reic...@collabora.com>; Meng, Tingting
> <tingting.m...@altera.com>; Chee, Tien Fong <tien.fong.c...@altera.com>;
> Hea, Kok Kiang <kok.kiang....@altera.com>; Ng, Boon Khai
> <boon.khai...@altera.com>; Yuslaimi, Alif Zakuan
> <alif.zakuan.yusla...@altera.com>; Zamri, Muhammad Hazim Izzat
> <muhammad.hazim.izzat.za...@altera.com>; Lim, Jit Loon
> <jit.loon....@altera.com>; Tang, Sieu Mun <sieu.mun.t...@altera.com>; u-
> b...@lists.denx.de
> Subject: Re: [resend v4 09/12] cmd: Add i3c command support.
>
> [CAUTION: This email is from outside your organization. Unless you trust the
> sender, do not click on links or open attachments as it may be a fraudulent
> email
> attempting to steal your information and/or compromise your computer.]
>
> On 4/17/25 04:18, dinesh.mani...@altera.com wrote:
> > From: Dinesh Maniyam <dinesh.mani...@altera.com>
> >
> > Add i3c command file to support select, get i3c device target list,
> > read and write operation.
> >
> > Signed-off-by: Dinesh Maniyam <dinesh.mani...@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.
Will remove this and return CMD_RET_USAGE.
>
> > +
> > + 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?
To list how many instances bind to the drivers.
>
> > + 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.
>
Sure, I will check the value range and update it.
> > +
> > + 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.
>
Noted, I will take note on this.
> > + 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.
>
Sure, will create a separate static functions for each subcommand.
> > +
> > + 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.
>
Will 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 '='.
Will correct it.
>
> > + } else {
> > + printf("Read buff: ");
> > + for (size_t i = 0; i < length; ++i)
>
> length is u32. Why use a larger type for the index?
Not supposed to use larger type of index, will take note on this and be more
consistent.
>
> > + printf("%02x", rdata[i]);
>
> Please, use print_hex_dump() here. This is especially helpful when reading
> hundres of bytes.
>
Noted. Will replace with print_hex_dump()
> 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.
>
You are right. To handle larger data byte, better using address in memory to
send
and receive the data. Will rework.
> > +
> > + 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/
>
Will be replaced.
> > + "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/
>
Will be replaced.
> > + 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.
>
You are right. To handle larger data byte, better using address in memory to
send
and receive the data. Will rework.
>
> > + 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/
>
Will be replaced.
> > +and write on the i3c devices.
>
> %s/write on the i3c/write operations on the connected I3C/
>
Will be replaced.
> 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/
>
Will be replaced.
> > +
> > +i3c list
> > +----------
> > +
> > +List all the i3c controllers defined in the DT.
>
> %s/i3c/I3C/
>
Will be replaced.
> %s/DT/device-tree/
Will be replaced.
>
> > +
> > +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?
>
Will include the static address.
> > +
> > +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
>
This will be replaced.
> > +
> > +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?
>
It should be a device number.
> > +
> > +data
> > + Bytes to be written to device.
> > +
> > +Examples
> > +--------
> > +
> > +Probe i3c0::
> > +
> > + => i3c i3c0
> > +
> > +Check cuurent i3c controller::
>
> Display current I3C host controller::
>
Will be replaced.
> > +
> > + => 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.
>
No. device 0 is the device number in the driver model.
> > +
> > + => 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?
>
Will provide the example for multiple bytes on next version.
Lower case is already working and taken care of.
> > +
> > +Perform read from a device 0::
> > +
> > + => i3c read 1 0
>
> Where is the example output?
>
Missing output. Will add it.
> Best regards
>
> Heinrich
>
Please expect v5, which I will rework based on the comments.
Regards
Dinesh
>
> > +
> > +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)
