Re: [PATCH 2/3] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Wed, Oct 02, 2024 at 09:49:44AM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.Li@xxxxxxx wrote on Tue, 01 Oct 2024 13:08:21 -0400:
>
> > Extend the address status bit to 4 and introduce the I3C_ADDR_SLOT_EXT_INIT
> > macro to indicate that a device prefers a specific address. This is
> > generally set by the 'assigned-address' in the device tree source (dts)
> > file.
> >
> >  ┌────┬─────────────┬───┬─────────┬───┐
> >  │S/Sr│ 7'h7E RnW=0 │ACK│ ENTDAA  │ T ├────┐
> >  └────┴─────────────┴───┴─────────┴───┘    │
> >  ┌─────────────────────────────────────────┘
> >  │  ┌──┬─────────────┬───┬─────────────────┬────────────────┬───┬─────────┐
> >  └─►│Sr│7'h7E RnW=1  │ACK│48bit UID BCR DCR│Assign 7bit Addr│PAR│ ACK/NACK│
> >     └──┴─────────────┴───┴─────────────────┴────────────────┴───┴─────────┘
> >
> > Some master controllers (such as HCI) need to prepare the entire above
> > transaction before sending it out to the I3C bus. This means that a 7-bit
> > dynamic address needs to be allocated before knowing the target device's
> > UID information.
> >
> > However, some I3C targets want a specific address (called as
> > "init_dyn_addr"), which is typically specified by the DT's assigned-address
> > property. (Lower addresses have higher IBI priority, and the target can
> > adjust this by using the assigned-address property if using DT). The
> > function i3c_master_add_i3c_dev_locked() will switch to this
> > "init_dyn_addr" if it is not in use.
> >
> > Therefore, i3c_bus_get_free_addr() should return a free address that has
> > not been claimed by any target devices as "init_dyn_addr" (indicated by
> > I3C_ADDR_SLOT_EXT_INIT). This allows the device with the "init_dyn_addr"
> > to switch to its "init_dyn_addr" when it hot-joins the I3C bus. Otherwise,
> > if the "init_dyn_addr" is already in use by another I3C device, the target
> > device will not be able to switch to its desired address.
> >
> > If all of above address are already used, i3c_bus_get_free_addr() return
> > one from the claimed as init_dyn_addr and free address slot. This ensures
> > support devices as much as possible.
> >
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > change from v3 to v4
> > - rewrite commit message and comment for i3c_bus_get_free_addr()
> > ---
> >  drivers/i3c/master.c       | 68 ++++++++++++++++++++++++++++++++++++++++------
> >  include/linux/i3c/master.h |  7 +++--
> >  2 files changed, 64 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index dcf8d23c5941a..a56cb281e6b6d 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -345,7 +345,7 @@ const struct bus_type i3c_bus_type = {
> >  EXPORT_SYMBOL_GPL(i3c_bus_type);
> >
> >  static enum i3c_addr_slot_status
> > -i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
> > +i3c_bus_get_addr_slot_status_ext(struct i3c_bus *bus, u16 addr)
> >  {
> >  	unsigned long status;
> >  	int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
> > @@ -356,11 +356,17 @@ i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
> >  	status = bus->addrslots[bitpos / BITS_PER_LONG];
> >  	status >>= bitpos % BITS_PER_LONG;
> >
> > -	return status & I3C_ADDR_SLOT_STATUS_MASK;
> > +	return status & I3C_ADDR_SLOT_EXT_STATUS_MASK;
> >  }
> >
> > -static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> > -					 enum i3c_addr_slot_status status)
> > +static enum i3c_addr_slot_status
> > +i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
> > +{
> > +	return i3c_bus_get_addr_slot_status_ext(bus, addr) & I3C_ADDR_SLOT_STATUS_MASK;
> > +}
> > +
> > +static void i3c_bus_set_addr_slot_status_mask(struct i3c_bus *bus, u16 addr,
> > +					      enum i3c_addr_slot_status status, int mask)
> >  {
> >  	int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
> >  	unsigned long *ptr;
> > @@ -369,11 +375,22 @@ static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> >  		return;
> >
> >  	ptr = bus->addrslots + (bitpos / BITS_PER_LONG);
> > -	*ptr &= ~((unsigned long)I3C_ADDR_SLOT_STATUS_MASK <<
> > -						(bitpos % BITS_PER_LONG));
> > +	*ptr &= ~((unsigned long)mask << (bitpos % BITS_PER_LONG));
> >  	*ptr |= (unsigned long)status << (bitpos % BITS_PER_LONG);
> >  }
> >
> > +static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> > +					 enum i3c_addr_slot_status status)
> > +{
> > +	i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_STATUS_MASK);
> > +}
> > +
> > +static void i3c_bus_set_addr_slot_status_ext(struct i3c_bus *bus, u16 addr,
> > +					     enum i3c_addr_slot_status status)
> > +{
> > +	i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_EXT_STATUS_MASK);
> > +}
>
> Can we drop this helper and instead modify the
> i3c_bus_set_addr_slot_status() prototype to get the mask from its
> parameters?

git grep "i3c_bus_set_addr_slot_status(" drivers/i3c/ | wc

There are 18 places need be modified without this helper function.
are you sue what you want?

Maybe drop i3c_bus_set_addr_slot_status_ext() is good idea by direct use
i3c_bus_set_addr_slot_status_mask().

Frank

>
> > +
> >  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> >  {
> >  	enum i3c_addr_slot_status status;
> > @@ -383,11 +400,44 @@ static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> >  	return status == I3C_ADDR_SLOT_FREE;
> >  }
> >
> > +/*
> > + * ┌────┬─────────────┬───┬─────────┬───┐
> > + * │S/Sr│ 7'h7E RnW=0 │ACK│ ENTDAA  │ T ├────┐
> > + * └────┴─────────────┴───┴─────────┴───┘    │
> > + * ┌─────────────────────────────────────────┘
> > + * │  ┌──┬─────────────┬───┬─────────────────┬────────────────┬───┬─────────┐
> > + * └─►│Sr│7'h7E RnW=1  │ACK│48bit UID BCR DCR│Assign 7bit Addr│PAR│ ACK/NACK│
> > + *    └──┴─────────────┴───┴─────────────────┴────────────────┴───┴─────────┘
> > + * Some master controllers (such as HCI) need to prepare the entire above transaction before
> > + * sending it out to the I3C bus. This means that a 7-bit dynamic address needs to be allocated
> > + * before knowing the target device's UID information.
> > + *
> > + * However, some I3C targets want a specific address (called as "init_dyn_addr"), which is
>
> 				may request specific addresses (called "init...
>
> > + * typically specified by the DT's assigned-address property. (Lower addresses have higher IBI
>
> 				   -'s
>
> > + * priority, and the target can adjust this by using the assigned-address property if using DT).
>
> Can we remove the whole "( ... )" sentence, and replace it with:
>
> 	"... property, lower addresses having higher IBI priority."
>
> > + * The function i3c_master_add_i3c_dev_locked() will switch to this "init_dyn_addr" if it is not
> > + * in use.
>
> 	if it is available.
>
> > + *
> > + * Therefore, i3c_bus_get_free_addr() should return a free address that has not been claimed by any
>
> 					preferably return
>
> 	that is not in the list of desired addresses.
>
> > + * target devices as "init_dyn_addr". This allows the device with the "init_dyn_addr" to switch to
> > + * its "init_dyn_addr" when it hot-joins the I3C bus. Otherwise, if the "init_dyn_addr" is already
> > + * in use by another I3C device, the target device will not be able to switch to its desired
> > + * address.
> > + *
> > + * If all of above address are already used, i3c_bus_get_free_addr() return one from the claimed as
> > + * init_dyn_addr and free address slot. This ensures support devices as much as possible.
>
> If the previous step fails, fallback returning one of the remaining
> unassigned address, regardless of its state in the desired list.
>
> > + */
>
> Please update your commit message as well with these changes.
>
> >  static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> >  {
> >  	enum i3c_addr_slot_status status;
> >  	u8 addr;
> >
> > +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> > +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
>
> So here it could look like:
>
> 		status = ...get_addr_slot_status(bus, addr, <extended>)
>
> > +		if (status == I3C_ADDR_SLOT_FREE)
> > +			return addr;
> > +	}
> > +
> >  	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> >  		status = i3c_bus_get_addr_slot_status(bus, addr);
> >  		if (status == I3C_ADDR_SLOT_FREE)
> > @@ -1918,9 +1968,9 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> >  			goto err_rstdaa;
> >  		}
> >
> > -		i3c_bus_set_addr_slot_status(&master->bus,
> > -					     i3cboardinfo->init_dyn_addr,
> > -					     I3C_ADDR_SLOT_I3C_DEV);
> > +		i3c_bus_set_addr_slot_status_ext(&master->bus,
> > +						 i3cboardinfo->init_dyn_addr,
> > +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
> >
> >  		/*
> >  		 * Only try to create/attach devices that have a static
> > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > index 2100547b2d8d2..57ad6044ac856 100644
> > --- a/include/linux/i3c/master.h
> > +++ b/include/linux/i3c/master.h
> > @@ -298,7 +298,8 @@ enum i3c_open_drain_speed {
> >   * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device
> >   * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device
> >   * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask
> > - *
> > + * @I3C_ADDR_SLOT_EXT_INIT: the bitmask represents addresses that are preferred by some devices,
> > + *			    such as the "assigned-address" property in a device tree source (DTS).
>
> The naming could be improved, because "extended" does not mean much. I
> believe we should express the fact that this is a desired addressed, so
> what about:
>
> 	I3C_ADDR_SLOT_I3C_ASSIGNED/DESIRED
>
> >   * On an I3C bus, addresses are assigned dynamically, and we need to know which
> >   * addresses are free to use and which ones are already assigned.
> >   *
> > @@ -311,9 +312,11 @@ enum i3c_addr_slot_status {
> >  	I3C_ADDR_SLOT_I2C_DEV,
> >  	I3C_ADDR_SLOT_I3C_DEV,
> >  	I3C_ADDR_SLOT_STATUS_MASK = 3,
> > +	I3C_ADDR_SLOT_EXT_STATUS_MASK = 7,
> > +	I3C_ADDR_SLOT_EXT_INIT = BIT(2),
> >  };
> >
> > -#define I3C_ADDR_SLOT_STATUS_BITS 2
> > +#define I3C_ADDR_SLOT_STATUS_BITS 4
> >
> >  /**
> >   * struct i3c_bus - I3C bus object
> >
>
>
> Thanks,
> Miquèl