Re: [PATCH v6 04/12] i3c: master: Add support for devices using SETAASA

[Akhil R <akhilrajeev@nvidia.com>]

On Mon, 27 Jul 2026 12:12:50 -0400 Frank Li wrote:
> On Mon, Jul 27, 2026 at 12:57:45PM +0000, Akhil R wrote:
>> On Tue, 21 Jul 2026 04:30:58 +0000, Sashiko AI review <sashiko-bot@kernel.org> wrote:
>> > commit e158b576a9f733695d868ee16f34bacdb43285b3
>> > Author: Akhil R <akhilrajeev@nvidia.com>
>> >
>> > i3c: master: Add support for devices using SETAASA
>> >
>> > This commit adds support for discovering I3C devices using the SETAASA
>> > procedure, such as SPD5118 and SPD5108 attached to DDR5 memory modules. It
>> > adds the SETAASA and SETHID CCC commands and updates the discovery logic.
>> >
>> > Link: https://www.mipi.org/mipi-disco-for-i3c-download
>> >
>> >> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> >> index 623c6b2247d9f..b18dda89c4737 100644
>> >> --- a/drivers/i3c/master.c
>> >> +++ b/drivers/i3c/master.c
>> >
>> > [ ... ]
>> >
>> >> @@ -1102,6 +1103,51 @@ static int i3c_master_rstdaa_locked(struct i3c_master_controller *master,
>> >>  	return ret;
>> >>  }
>> >>
>> >> +static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
>> >> +{
>> >> +	struct i3c_ccc_cmd_dest dest;
>> >> +	struct i3c_ccc_cmd cmd;
>> >> +	int ret;
>> >> +
>> >> +	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
>> >> +	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_VENDOR(0, true), &dest, 1);
>> >> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
>> >
>> > [Severity: High]
>> > Will this fail on most existing I3C master controllers?
>> >
>> > Looking at i3c_master_send_ccc_cmd_locked(), it calls
>> > master->ops->supports_ccc_cmd() before sending. Many core controller drivers
>> > (like dw-i3c-master.c) have strict switch-statement whitelists for supported
>> > CCC IDs that do not include these new SETAASA (0x29) and SETHID (0x61)
>> > commands.
>> >
>> > Would this result in returning -EOPNOTSUPP and aborting the entire bus
>> > initialization on systems with these controllers?
>>
>> SETAASA/SETHID are only issued when a device opts in to the SETAASA method
>> (i3c_master_setaasa_locked() is called solely when addr_method has
>> I3C_ADDR_METHOD_SETAASA, and SETHID is sent from within it). Existing
>> drivers are unaffected because SETAASA is a new capability they do not yet
>> advertise; they can whitelist these CCCs when they add SETAASA support,
>> which will have to be updated in the individual drivers. This patchset
>> does that for the DesignWare contoller driver.
>>
>> > [ ... ]
>> >
>> >> @@ -1878,6 +1924,22 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
>> >>  	if (ret)
>> >>  		goto err_free_dev;
>> >>
>> >> +	if (i3cdev->boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
>> >> +		i3cdev->info.dyn_addr = i3cdev->boardinfo->static_addr;
>> >> +		ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
>> >> +		if (ret)
>> >> +			goto err_rstdaa;
>> >
>> > [Severity: High]
>> > If i3c_master_reattach_i3c_dev_locked() fails here, we jump to err_rstdaa
>> > and eventually free the device structure. However, i3c_master_bus_init()
>> > later unconditionally broadcasts SETAASA to the bus.
>> >
>> > Won't the physical device claim this static address during the broadcast,
>> > even though the controller software no longer tracks it? This could lead to
>> > a bus collision when the software later tries to dynamically assign this
>> > "free" address slot to a different device.
>>
>> Address conflicts between statically-addressed devices are rejected up
>> front in i3c_master_get_i3c_addrs(), so it should not cause any address
>> collision with other devices on the bus unless the same address is being
>> assigned through ENTDAA. If this is a concern, we can avoid freeing the
>> static address for SETAASA devices even if software attachment fails.
>>
>> Let me know if this needs to be done.
> 
> Try it.
> 
>>
>> > [Severity: High]
>> > This is a pre-existing issue, but does jumping to err_rstdaa correctly reset
>> > the hardware if reattachment fails during SETDASA or SETAASA?
>> >
>> > If i3c_master_reattach_i3c_dev_locked() fails, its internal error path calls
>> > i3c_master_put_i3c_addrs(), which marks the slot as I3C_ADDR_SLOT_FREE.
>> >
>> > When we reach the err_rstdaa path:
>> >
>> > err_rstdaa:
>> >     i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
>> >
>> > i3c_master_rstdaa_locked() checks i3c_bus_get_addr_slot_status(). Because the
>> > slot is already marked FREE, it returns -EINVAL and silently aborts without
>> > sending the RSTDAA command to the hardware.
>> >
>> > Does this leave the physical device holding the address while the software
>> > considers it free?
>>
>> Pre-existing; not introduced by this patch. Better addressed separately.
>>
>> >> +
>> >> +		return 0;
>> >> +	}
>> >> +
>> >>  	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
>> >>  					i3cdev->boardinfo->init_dyn_addr);
>> >
>> > [ ... ]
>> >
>> >> @@ -2741,7 +2816,19 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>> >> +	if (boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
>> >> +		/* For SETAASA, static address is taken as the dynamic address. */
>> >> +		init_dyn_addr = boardinfo->static_addr;
>> >> +	}
>> >> +
>> >> +	/* Update the address methods required for device discovery */
>> >> +	master->addr_method |= boardinfo->static_addr_method;
>> >> +
>> >>  	boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>> >>
>> >>  	if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||
>> >
>> > [Severity: High]
>> > By relaxing the PID validation for SETAASA devices in this function, pid can
>> > default to 0. If a system configures multiple SETAASA devices (like DDR5
>> > SPDs) without PIDs, will this cause a kobject name collision?
>> >
>> > During registration, i3c_master_register_new_i3c_devs() names the device
>> > using its bus ID and PID:
>> >
>> > dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id, desc->info.pid);
>> >
>> > If multiple devices have pid=0, they would all be named "0-0", causing
>> > device_register() to fail with -EEXIST for subsequent devices and preventing
>> > them from functioning.
>>
>> Static addressing requires each device to have a distinct static address on
>> the bus,
> 
> Any check and report error if two devices have the same address?
> 

Yes. For SETAASA devices the static address is also the dynamic address, so a
duplicate is caught in i3c_master_bus_init() while reserving the address slots
before pre-assignment. It fails with -EBUSY and the bus initialization is
aborted. For SETDASA devices, a duplicate static address is rejected in
get_i3c_addrs() when the device is attached.

The kobject conflict is resolved in the subsequent patch, where SETAASA or
PID-less devices are named with the static address instead of the PID.

> 
>> and PID-less devices are named from that unique static address
>> ("%d-%02x"). The bus address-slot management also prevents two live devices
>> from occupying the same address. So a collision would require a malformed
>> description with duplicate static addresses, which is not a valid bus.
>>

Best Regards,
Akhil

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c