Re: [PATCH 15/17] i3c: mipi-i3c-hci: Support configurable device NACK retries

[Adrian Hunter <adrian.hunter@intel.com>]

On 14/09/2026 21:21, Frank Li wrote:
> On Mon, Sep 14, 2026 at 02:30:01PM +0300, Adrian Hunter wrote:
>> Implement the .set_dev_nack_retry() master operation using the
>> DAT_0_DEV_NACK_RETRY_CNT field in Device Address Table entries.
>>
>> Since the retry count is programmed per DAT entry, update all allocated
>> entries when the setting changes and initialize newly allocated entries
>> with the current value so that a consistent retry policy is applied
>> across all devices.
>>
>> Add DAT helper operations to update individual entries and all allocated
>> entries.  Return -ERANGE if the requested retry count exceeds the
>> hardware field width and -EOPNOTSUPP when the active command descriptor
>> model does not use DAT entries.
>>
>> In addition, default the retry count to 1.  The I3C specification
>> mandates a retry when a Target NACKs its Dynamic Address.  HCI v1.1
>> explicitly preserves that behaviour for Direct CCCs even when
>> DEV_NACK_RETRY_CNT is programmed to 0, but HCI v1.0 defines the field
>> only as a device-specific retry count and does not provide the same
>> exception.  As a result, a v1.0 controller left at the reset value of 0
>> may perform no retry whereas a v1.1 controller will retry once.
>> Promoting 0 to 1 at probe time makes the behaviour consistent across
>> controller versions.  Userspace can still select 0 retries explicitly
>> via dev_nack_retry_count.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/i3c/master/mipi-i3c-hci/core.c   | 25 ++++++++++++++++++++++++
>>  drivers/i3c/master/mipi-i3c-hci/dat.h    |  2 ++
>>  drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 25 ++++++++++++++++++++++++
>>  3 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
>> index 7a39be64c4e1..57d84d7682d7 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
>> @@ -555,6 +555,11 @@ static int i3c_hci_i2c_xfers(struct i2c_dev_desc *dev,
>>  	return ret;
>>  }
>>
>> +static void i3c_hci_dat_v1_set_curr_nack_retry(struct i3c_hci *hci, unsigned int dat_idx)
>> +{
>> +	mipi_i3c_hci_dat_v1.set_nack_retry(hci, dat_idx, hci->master.dev_nack_retry_count);
>> +}
>> +
>>  static int i3c_hci_attach_i3c_dev(struct i3c_dev_desc *dev)
>>  {
>>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>> @@ -573,6 +578,7 @@ static int i3c_hci_attach_i3c_dev(struct i3c_dev_desc *dev)
>>  		}
>>  		mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, ret,
>>  						     dev->info.dyn_addr ?: dev->info.static_addr);
>> +		i3c_hci_dat_v1_set_curr_nack_retry(hci, ret);
>>  		dev_data->dat_idx = ret;
>>  	}
>>  	i3c_dev_set_master_data(dev, dev_data);
>> @@ -622,6 +628,7 @@ static int i3c_hci_attach_i2c_dev(struct i2c_dev_desc *dev)
>>  	}
>>  	mipi_i3c_hci_dat_v1.set_static_addr(hci, ret, dev->addr);
>>  	mipi_i3c_hci_dat_v1.set_flags(hci, ret, DAT_0_I2C_DEVICE, 0);
>> +	i3c_hci_dat_v1_set_curr_nack_retry(hci, ret);
>>  	dev_data->dat_idx = ret;
>>  	i2c_dev_set_master_data(dev, dev_data);
>>  	return 0;
>> @@ -733,6 +740,16 @@ static void i3c_hci_recycle_ibi_slot(struct i3c_dev_desc *dev,
>>  	hci->io->recycle_ibi_slot(hci, dev, slot);
>>  }
>>
>> +static int i3c_hci_set_dev_nack_retry(struct i3c_master_controller *m, unsigned int cnt)
>> +{
>> +	struct i3c_hci *hci = to_i3c_hci(m);
>> +
>> +	if (hci->cmd != &mipi_i3c_hci_cmd_v1)
>> +		return -EOPNOTSUPP;
>> +
>> +	return mipi_i3c_hci_dat_v1.set_all_nack_retry(hci, cnt);
>> +}
>> +
>>  static const struct i3c_master_controller_ops i3c_hci_ops = {
>>  	.bus_init		= i3c_hci_bus_init,
>>  	.bus_cleanup		= i3c_hci_bus_cleanup,
>> @@ -752,6 +769,7 @@ static const struct i3c_master_controller_ops i3c_hci_ops = {
>>  	.recycle_ibi_slot	= i3c_hci_recycle_ibi_slot,
>>  	.enable_hotjoin		= i3c_hci_enable_hotjoin,
>>  	.disable_hotjoin	= i3c_hci_disable_hotjoin,
>> +	.set_dev_nack_retry	= i3c_hci_set_dev_nack_retry,
>>  };
>>
>>  static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>> @@ -1186,6 +1204,13 @@ static int i3c_hci_probe(struct platform_device *pdev)
>>  	if (device_can_wakeup(i3c_hci_sysdev(&pdev->dev)))
>>  		hci->master.ibi_wakeup = true;
>>
>> +	/*
>> +	 * HCI v1.1 onward does 1 retry for Direct CCCs anyway, so for v1.0 to
>> +	 * be consistent, promote 0 to 1.
>> +	 */
>> +	if (!hci->master.dev_nack_retry_count)
>> +		hci->master.dev_nack_retry_count = 1;
>> +
>>  	return i3c_master_register(&hci->master, &pdev->dev, &i3c_hci_ops, false);
>>  }
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat.h b/drivers/i3c/master/mipi-i3c-hci/dat.h
>> index 6881f19da77f..a07086bf717a 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dat.h
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dat.h
>> @@ -25,6 +25,8 @@ struct hci_dat_ops {
>>  	void (*clear_flags)(struct i3c_hci *hci, unsigned int dat_idx, u32 w0, u32 w1);
>>  	int (*get_index)(struct i3c_hci *hci, u8 address);
>>  	void (*restore)(struct i3c_hci *hci);
>> +	void (*set_nack_retry)(struct i3c_hci *hci, unsigned int dat_idx, unsigned int cnt);
>> +	int (*set_all_nack_retry)(struct i3c_hci *hci, unsigned int cnt);
>>  };
>>
>>  extern const struct hci_dat_ops mipi_i3c_hci_dat_v1;
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
>> index 852966aa20d9..cb9aa43e69fb 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
>> @@ -189,6 +189,29 @@ static void hci_dat_v1_restore(struct i3c_hci *hci)
>>  	}
>>  }
>>
>> +static void hci_dat_v1_set_nack_retry(struct i3c_hci *hci, unsigned int dat_idx, unsigned int cnt)
>> +{
>> +	u32 dat_w0;
>> +
>> +	dat_w0 = dat_w0_read(dat_idx);
>> +	dat_w0 &= ~DAT_0_DEV_NACK_RETRY_CNT;
>> +	dat_w0 |= FIELD_PREP(DAT_0_DEV_NACK_RETRY_CNT, cnt);
>> +	dat_w0_write(dat_idx, dat_w0);
>> +}
>> +
>> +static int hci_dat_v1_set_all_nack_retry(struct i3c_hci *hci, unsigned int cnt)
>> +{
> 
> do you have other implement for set_all_nack_retry()?

If you mean the naming "dat_v1", that is just a common name for the
DAT implementation, presumably assuming there might be a dat_v2 one
day.

If you mean adding the function to struct hci_dat_ops, that is just
for consistency with how all other DAT functions are accessed.

> 
> Frank
>> +	unsigned int dat_idx;
>> +
>> +	if (cnt > FIELD_MAX(DAT_0_DEV_NACK_RETRY_CNT))
>> +		return -ERANGE;
>> +
>> +	for_each_set_bit(dat_idx, hci->DAT_data, hci->DAT_entries)
>> +		hci_dat_v1_set_nack_retry(hci, dat_idx, cnt);
>> +
>> +	return 0;
>> +}
>> +
>>  const struct hci_dat_ops mipi_i3c_hci_dat_v1 = {
>>  	.init			= hci_dat_v1_init,
>>  	.alloc_entry		= hci_dat_v1_alloc_entry,
>> @@ -199,4 +222,6 @@ const struct hci_dat_ops mipi_i3c_hci_dat_v1 = {
>>  	.clear_flags		= hci_dat_v1_clear_flags,
>>  	.get_index		= hci_dat_v1_get_index,
>>  	.restore		= hci_dat_v1_restore,
>> +	.set_nack_retry		= hci_dat_v1_set_nack_retry,
>> +	.set_all_nack_retry	= hci_dat_v1_set_all_nack_retry,
>>  };
>> --
>> 2.53.0
>>