[PATCH 0/3] I3C: master: fix the address assign issue if assign-address is exist in dts

[PATCH 0/3] I3C: master: fix the address assign issue if assign-address is exist in dts

[Frank Li]

These patches are split from
https://lore.kernel.org/linux-i3c/ZvrAuOBLgi+HtrPD@lizhi-Precision-Tower-5810/#R

There are discussion on
https://lore.kernel.org/linux-i3c/20240819-i3c_fix-v3-0-7d69f7b0a05e@nxp.com/T/#m16fa9bb875b0ae9d37c5f6e91f90e375551c6366

Basic back ground is
The current framework is
1. get free i3c dynamic address
2. if found dt have assign-address for such device (identify by PID),
change to such address.

There are problem in current implement.
If device A have assign-address 0xa, device B have assign-address 0xB,
which described at dts file.

If device A is not ready during i3c probe, and device B hotjoin happen,
0xA will assign to device B, so if device A hotjoin later, address 0xA
Can't assign to A because B already use it.

Mirquel's opinion is return address B when B hotjoin by scan dts by PID.

The issue is the controller HCI (i3C standard),

I3C HCI Spec 1.2, sec 6.4.1, when do DAA,  "DAA CMD and dynmatic address"
queue to cmd together.  We don't know PID before DAA CMD. So dynamic
address can NOT get based on PID.

When do DAA in HCI, it needs a dynamtic address firstly before get PID
information.

Consider this need more time to discuss, so split from previous big serial
to avoid prevent other fix patches can't be merged into i3c tree.

This patches's overall design:

1. keep current frame's work flow
2. reserver all address, which assigned in dts.
3. the device with assigned address have high priorioty to get such
address.
4. if all address without assigned by dt are used, use offline devices's
assigned address.

To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: linux-i3c@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: arnd@arndb.de
Cc: bbrezillon@kernel.org
Cc: boris.brezillon@collabora.com
Cc: conor.culhane@silvaco.com
Cc: gregkh@linuxfoundation.org
Cc: imx@lists.linux.dev
Cc: linux-i3c@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: miquel.raynal@bootlin.com
Cc: pthombar@cadence.com
Cc: ravindra.yashvant.shinde@nxp.com

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (3):
      i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_STATUS_BITS
      i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT
      i3c: master: Fix dynamic address leak when 'assigned-address' is present

 drivers/i3c/master.c       | 85 +++++++++++++++++++++++++++++++++++-----------
 include/linux/i3c/master.h |  9 +++--
 2 files changed, 72 insertions(+), 22 deletions(-)
---
base-commit: 77df9e4bb2224d8ffbddec04c333a9d7965dad6c
change-id: 20241001-i3c_dts_assign-d615fc33cc1d

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH 1/3] i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_STATUS_BITS

[Frank Li]

Replace the hardcoded value 2, which indicates 2 bits for I3C address
status, with the predefined macro I3C_ADDR_SLOT_STATUS_BITS.

Improve maintainability and extensibility of the code.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v3 to v4
- rename to I3C_ADDR_SLOT_STATUS_BITS
---
 drivers/i3c/master.c       | 4 ++--
 include/linux/i3c/master.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 6f3eb710a75d6..dcf8d23c5941a 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -348,7 +348,7 @@ static enum i3c_addr_slot_status
 i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
 {
 	unsigned long status;
-	int bitpos = addr * 2;
+	int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
 
 	if (addr > I2C_MAX_ADDR)
 		return I3C_ADDR_SLOT_RSVD;
@@ -362,7 +362,7 @@ i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
 static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
 					 enum i3c_addr_slot_status status)
 {
-	int bitpos = addr * 2;
+	int bitpos = addr * I3C_ADDR_SLOT_STATUS_BITS;
 	unsigned long *ptr;
 
 	if (addr > I2C_MAX_ADDR)
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 2a1ed05d5782a..2100547b2d8d2 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -313,6 +313,8 @@ enum i3c_addr_slot_status {
 	I3C_ADDR_SLOT_STATUS_MASK = 3,
 };
 
+#define I3C_ADDR_SLOT_STATUS_BITS 2
+
 /**
  * struct i3c_bus - I3C bus object
  * @cur_master: I3C master currently driving the bus. Since I3C is multi-master
@@ -354,7 +356,7 @@ enum i3c_addr_slot_status {
 struct i3c_bus {
 	struct i3c_dev_desc *cur_master;
 	int id;
-	unsigned long addrslots[((I2C_MAX_ADDR + 1) * 2) / BITS_PER_LONG];
+	unsigned long addrslots[((I2C_MAX_ADDR + 1) * I3C_ADDR_SLOT_STATUS_BITS) / BITS_PER_LONG];
 	enum i3c_bus_mode mode;
 	struct {
 		unsigned long i3c;

-- 
2.34.1

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

[Frank Li]

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@nxp.com>
---
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);
+}
+
 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
+ * 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". 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.
+ */
 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);
+		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).
  * 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

-- 
2.34.1

[PATCH 3/3] i3c: master: Fix dynamic address leak when 'assigned-address' is present

[Frank Li]

If the DTS contains 'assigned-address', a dynamic address leak occurs
during hotjoin events.

Assume a device have assigned-address 0xb.
  - Device issue Hotjoin
  - Call i3c_master_do_daa()
  - Call driver xxx_do_daa()
  - Call i3c_master_get_free_addr() to get dynamic address 0x9
  - i3c_master_add_i3c_dev_locked(0x9)
  -     expected_dyn_addr  = newdev->boardinfo->init_dyn_addr (0xb);
  -     i3c_master_reattach_i3c_dev(newdev(0xb), old_dyn_addr(0x9));
  -         if (dev->info.dyn_addr != old_dyn_addr &&
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 0xb != 0x9 -> TRUE
                (!dev->boardinfo ||
                 ^^^^^^^^^^^^^^^ ->  FALSE
                 dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) {
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 0xb != 0xb      ->  FALSE
                 ...
                 i3c_bus_set_addr_slot_status(&master->bus, old_dyn_addr,
                                                     I3C_ADDR_SLOT_FREE);
		 ^^^
                 This will be skipped. So old_dyn_addr never free
            }

  - i3c_master_get_free_addr() will return increased sequence number.

Remove dev->info.dyn_addr != dev->boardinfo->init_dyn_addr condition check.
dev->info.dyn_addr should be checked before calling this function because
i3c_master_setnewda_locked() has already been called and the target device
has already accepted dyn_addr. It is too late to check if dyn_addr is free
in i3c_master_reattach_i3c_dev().

Add check to ensure expected_dyn_addr is free before
i3c_master_setnewda_locked().

Fixes: cc3a392d69b6 ("i3c: master: fix for SETDASA and DAA process")
Cc: stable@kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change v3 to v4
- none
---
 drivers/i3c/master.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index a56cb281e6b6d..eae7449f0dacb 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1556,16 +1556,9 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 				       u8 old_dyn_addr)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
-	enum i3c_addr_slot_status status;
 	int ret;
 
-	if (dev->info.dyn_addr != old_dyn_addr &&
-	    (!dev->boardinfo ||
-	     dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) {
-		status = i3c_bus_get_addr_slot_status(&master->bus,
-						      dev->info.dyn_addr);
-		if (status != I3C_ADDR_SLOT_FREE)
-			return -EBUSY;
+	if (dev->info.dyn_addr != old_dyn_addr) {
 		i3c_bus_set_addr_slot_status(&master->bus,
 					     dev->info.dyn_addr,
 					     I3C_ADDR_SLOT_I3C_DEV);
@@ -1968,9 +1961,10 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 			goto err_rstdaa;
 		}
 
+		/* Not mark as occupied until real device exist in bus */
 		i3c_bus_set_addr_slot_status_ext(&master->bus,
 						 i3cboardinfo->init_dyn_addr,
-						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
+						 I3C_ADDR_SLOT_EXT_INIT);
 
 		/*
 		 * Only try to create/attach devices that have a static
@@ -2133,7 +2127,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	else
 		expected_dyn_addr = newdev->info.dyn_addr;
 
-	if (newdev->info.dyn_addr != expected_dyn_addr) {
+	if (newdev->info.dyn_addr != expected_dyn_addr &&
+	    i3c_bus_get_addr_slot_status(&master->bus, expected_dyn_addr) == I3C_ADDR_SLOT_FREE) {
 		/*
 		 * Try to apply the expected dynamic address. If it fails, keep
 		 * the address assigned by the master.

-- 
2.34.1

Re: [PATCH 1/3] i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_STATUS_BITS

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 13:08:20 -0400:

> Replace the hardcoded value 2, which indicates 2 bits for I3C address
> status, with the predefined macro I3C_ADDR_SLOT_STATUS_BITS.
> 
> Improve maintainability and extensibility of the code.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

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

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com 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@nxp.com>
> ---
> 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? 

> +
>  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

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@nxp.com 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@nxp.com>
> > ---
> > 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

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

[Miquel Raynal]

Hi Frank,

> > >
> > > -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().

Ok fine for _mask.

Thanks,
Miquèl