[v6,2/3] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_DESIRED

Commit Message

Frank Li Oct. 3, 2024, 8:14 p.m. UTC

Extend the address status bit to 4 and introduce the
I3C_ADDR_SLOT_EXT_DESIRED 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 may request specific addresses (called as
"init_dyn_addr"), which is typically specified by the DT-'s
assigned-address property. Lower addresses having higher IBI priority. If
it is available, i3c_bus_get_free_addr() preferably return a free address
that is not in the list of desired addresses (called 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 the previous step fails, fallback returning one of the remaining
unassigned address, regardless of its state in the desired list.

Signed-off-by: Frank Li <[e-mail]>
---
Change from v5 to v6
- fix version number, should start v5
- change to I3C_ADDR_SLOT_EXT_DESIRED
- remove _ext function and direct use _mask function
- rework commit message and comments according to Miquèl's feedback.
- change mask type to u32
change from v3 to v4
- rewrite commit message and comment for i3c_bus_get_free_addr()
---
 drivers/i3c/master.c       | 60 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/i3c/master.h |  7 ++++--
 2 files changed, 56 insertions(+), 11 deletions(-)

Comments

Miquel Raynal Oct. 4, 2024, 7:29 a.m. UTC | #1

Hi Frank,

> + * However, some I3C targets may request specific addresses (called as "init_dyn_addr"), which is
> + * typically specified by the DT-'s assigned-address property. Lower addresses having higher IBI
> + * priority. If it is available, i3c_bus_get_free_addr() preferably return a free address that is
> + * not in the list of desired addresses (called 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 the previous step fails, fallback returning one of the remaining unassigned address,
> + * regardless of its state in the desired list.
> + */
>  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_mask(bus, addr,
> +							   I3C_ADDR_SLOT_EXT_STATUS_MASK);
> +		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);

Maybe here we could also use the same status_mask() with the other mask,
just to clarify the difference.

>  		if (status == I3C_ADDR_SLOT_FREE)
> @@ -1918,9 +1959,10 @@ 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_mask(&master->bus,
> +						 i3cboardinfo->init_dyn_addr,
> +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
> +						 I3C_ADDR_SLOT_EXT_STATUS_MASK);

However I'm not sure I understand the use of the
set_addr_slot_status_mask() function. Can't we just use the normal
function and just extend the mask in the fist place?

>  		/*
>  		 * 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..6e5328c6c6afd 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_DESIRED: the bitmask represents addresses that are preferred by some devices,
> + *			       such as the "assigned-address" property in a device tree source.
>   * 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_DESIRED = BIT(2),
>  };
>  
> -#define I3C_ADDR_SLOT_STATUS_BITS 2
> +#define I3C_ADDR_SLOT_STATUS_BITS 4
>  
>  /**
>   * struct i3c_bus - I3C bus object
> 

Otherwise lgtm.

Thanks,
Miquèl

Frank Li Oct. 4, 2024, 3:20 p.m. UTC | #2

On Fri, Oct 04, 2024 at 09:29:14AM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> > + * However, some I3C targets may request specific addresses (called as "init_dyn_addr"), which is
> > + * typically specified by the DT-'s assigned-address property. Lower addresses having higher IBI
> > + * priority. If it is available, i3c_bus_get_free_addr() preferably return a free address that is
> > + * not in the list of desired addresses (called 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 the previous step fails, fallback returning one of the remaining unassigned address,
> > + * regardless of its state in the desired list.
> > + */
> >  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_mask(bus, addr,
> > +							   I3C_ADDR_SLOT_EXT_STATUS_MASK);
> > +		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);
>
> Maybe here we could also use the same status_mask() with the other mask,
> just to clarify the difference.

Make sense.

>
> >  		if (status == I3C_ADDR_SLOT_FREE)
> > @@ -1918,9 +1959,10 @@ 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_mask(&master->bus,
> > +						 i3cboardinfo->init_dyn_addr,
> > +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
> > +						 I3C_ADDR_SLOT_EXT_STATUS_MASK);
>
> However I'm not sure I understand the use of the
> set_addr_slot_status_mask() function. Can't we just use the normal
> function and just extend the mask in the fist place?

The major purpose of set_addr_slot_status_mask() is that reduce code
change. There are already address alloc/free by using I3C_ADDR_SLOT_FREE.

we don't want i3c_bus_set_addr_slot_status() touch bit
I3C_ADDR_SLOT_EXT_DESIRED since it was init at scan dts.

There are 18 place, using i3c_bus_set_addr_slot_status(), but we only need
touch I3C_ADDR_SLOT_EXT_DESIRED bit when scan dts.

Frank

>
> >  		/*
> >  		 * 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..6e5328c6c6afd 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_DESIRED: the bitmask represents addresses that are preferred by some devices,
> > + *			       such as the "assigned-address" property in a device tree source.
> >   * 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_DESIRED = BIT(2),
> >  };
> >
> > -#define I3C_ADDR_SLOT_STATUS_BITS 2
> > +#define I3C_ADDR_SLOT_STATUS_BITS 4
> >
> >  /**
> >   * struct i3c_bus - I3C bus object
> >
>
> Otherwise lgtm.
>
> Thanks,
> Miquèl
>
> --
> linux-i3c mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-i3c

Miquel Raynal Oct. 4, 2024, 4:22 p.m. UTC | #3

Hi Frank,

> > > @@ -1918,9 +1959,10 @@ 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_mask(&master->bus,
> > > +						 i3cboardinfo->init_dyn_addr,
> > > +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
> > > +						 I3C_ADDR_SLOT_EXT_STATUS_MASK);  
> >
> > However I'm not sure I understand the use of the
> > set_addr_slot_status_mask() function. Can't we just use the normal
> > function and just extend the mask in the fist place?  
> 
> The major purpose of set_addr_slot_status_mask() is that reduce code
> change. There are already address alloc/free by using I3C_ADDR_SLOT_FREE.
> 
> we don't want i3c_bus_set_addr_slot_status() touch bit
> I3C_ADDR_SLOT_EXT_DESIRED since it was init at scan dts.

I agree, but in general you will never remove any "desired" slot, so
the "set status", besides at init time, should never touch these extra
bits?

> There are 18 place, using i3c_bus_set_addr_slot_status(), but we only need
> touch I3C_ADDR_SLOT_EXT_DESIRED bit when scan dts.
> 

Thanks,
Miquèl

Frank Li Oct. 4, 2024, 6:58 p.m. UTC | #4

On Fri, Oct 04, 2024 at 06:22:29PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> > > > @@ -1918,9 +1959,10 @@ 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_mask(&master->bus,
> > > > +						 i3cboardinfo->init_dyn_addr,
> > > > +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
> > > > +						 I3C_ADDR_SLOT_EXT_STATUS_MASK);
> > >
> > > However I'm not sure I understand the use of the
> > > set_addr_slot_status_mask() function. Can't we just use the normal
> > > function and just extend the mask in the fist place?
> >
> > The major purpose of set_addr_slot_status_mask() is that reduce code
> > change. There are already address alloc/free by using I3C_ADDR_SLOT_FREE.
> >
> > we don't want i3c_bus_set_addr_slot_status() touch bit
> > I3C_ADDR_SLOT_EXT_DESIRED since it was init at scan dts.
>
> I agree, but in general you will never remove any "desired" slot, so
> the "set status", besides at init time, should never touch these extra
> bits?

I am not sure I understand what your means exactly.

Yes, it is first and only place to set I3C_ADDR_SLOT_EXT_DESIRED.

Other place's i3c_bus_set_addr_slot_status() will not touch these bits.

Frank

>
> > There are 18 place, using i3c_bus_set_addr_slot_status(), but we only need
> > touch I3C_ADDR_SLOT_EXT_DESIRED bit when scan dts.
> >
>
> Thanks,
> Miquèl

Patch

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index dcf8d23c5941a..68411f1cf80d6 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_mask(struct i3c_bus *bus, u16 addr, u32 mask)
 {
 	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 & 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_mask(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, u32 mask)
 {
 	int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
 	unsigned long *ptr;
@@ -369,11 +375,16 @@  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 bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
 {
 	enum i3c_addr_slot_status status;
@@ -383,11 +394,41 @@  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 may request specific addresses (called as "init_dyn_addr"), which is
+ * typically specified by the DT-'s assigned-address property. Lower addresses having higher IBI
+ * priority. If it is available, i3c_bus_get_free_addr() preferably return a free address that is
+ * not in the list of desired addresses (called 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 the previous step fails, fallback returning one of the remaining unassigned address,
+ * regardless of its state in the desired list.
+ */
 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_mask(bus, addr,
+							   I3C_ADDR_SLOT_EXT_STATUS_MASK);
+		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 +1959,10 @@  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_mask(&master->bus,
+						 i3cboardinfo->init_dyn_addr,
+						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_DESIRED,
+						 I3C_ADDR_SLOT_EXT_STATUS_MASK);
 
 		/*
 		 * 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..6e5328c6c6afd 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_DESIRED: the bitmask represents addresses that are preferred by some devices,
+ *			       such as the "assigned-address" property in a device tree source.
  * 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_DESIRED = BIT(2),
 };
 
-#define I3C_ADDR_SLOT_STATUS_BITS 2
+#define I3C_ADDR_SLOT_STATUS_BITS 4
 
 /**
  * struct i3c_bus - I3C bus object