Skip to contentSkip to conversation

i3c: remove device if failed on pre_assign_dyn_addr()

PATCH In review Vitor Soares 4 participants 14 messages Sep 3, 2019 – Sep 4, 2019

Subsystems (4)
Stable branch Device tree bindings I3C I3C driver for synopsys DesignWare
lore
Share & tools
lore

Vitor Soares

author
#0 lore raw

v2 cover i3c: remove device if failed on pre_assign_dyn_addr()

This patch series remove the devices that fail during pre_assign_dyn_addr() and were being sent on DEFSVLS command. With the change above, during the i3c_master_add_i3c_dev_locked() is necessary to check if the device has i3c_boardinfo and try to assign the i3c_dev_boardinfo->init_dyn_addr if there no oldev. This change will allow to describe in DT device with preferable dynamic address but without static address.

Change in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Change i3c_master_search_i3c_boardinfo(newdev) to i3c_master_init_i3c_dev_boardinfo(newdev)
- Add fixes, stable tags on patch 2
- Add a note for no guarantee of 'assigned-address' use

Vitor Soares (5):
i3c: master: detach and free device if pre_assign_dyn_addr() fails
i3c: master: make sure ->boardinfo is initialized in

    add_i3c_dev_locked()

dt-bindings: i3c: make 'assigned-address' valid if static address == 0
dt-bindings: i3c: add a note for no guarantee of 'assigned-address' use
i3c: master: dw: reattach device on first available location of
address table

 Documentation/devicetree/bindings/i3c/i3c.txt | 15 ++++++--
 drivers/i3c/master.c                          | 49 ++++++++++++++++++++++-----
 drivers/i3c/master/dw-i3c-master.c            | 16 +++++++++
 3 files changed, 68 insertions(+), 12 deletions(-)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH v2 1/5 i3c: master: detach and free device if pre_assign_dyn_addr() fails

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Convert i3c_master_pre_assign_dyn_addr() to return an int

 drivers/i3c/master.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/i3c/master.c +15 -7
index 5f4bd52..586e34f 100644
@@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
 	int ret;
 
 	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
 	    !dev->boardinfo->static_addr)
-		return;
+		return 0;
 
 	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
 					dev->boardinfo->init_dyn_addr);
 	if (ret)
-		return;
+		return ret;
 
 	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
 	ret = i3c_master_reattach_i3c_dev(dev, 0);

@@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	return 0;
 
 err_rstdaa:
 	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+	return ret;
 }
 
 static void

@@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
+	struct i3c_dev_desc *i3cdev, *i3ctmp;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 

@@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+				 common.node) {
+		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
+		if (ret) {
+			i3c_master_detach_i3c_dev(i3cdev);
+			i3c_master_free_i3c_dev(i3cdev);
+		}
+	}
 
 	ret = i3c_master_do_daa(master);
 	if (ret)
Signature · 1 line
2.7.4
#2 lore raw

Re: [PATCH v2 1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails

Quoted 5 lines · On Tue, 3 Sep 2019 12:35:50 +0200 Vitor Soares <[email protected]>…

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

Quoted 8 lines

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()

So, you decided to ignore my comment about leaving the i3c_dev_desc allocated and skipping entries that don't have a dynamic address when forging the DEFSLVS frame instead of doing this allocate/free/re-allocate dance, and more importantly, you didn't even bother explaining why.

I'm not willing to accept this patch unless you come up with solid reasons.

author
#3 lore raw

RE: [PATCH v2 1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails

Hi Boris,

From: Boris Brezillon <[email protected]>
Date: Tue, Sep 03, 2019 at 11:52:37

Quoted 17 lines
Quoted 5 lines · On Tue, 3 Sep 2019 12:35:50 +0200 Vitor Soares <[email protected]>…

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

Quoted 8 lines

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()

Quoted 8 lines

So, you decided to ignore my comment about leaving the i3c_dev_desc allocated and skipping entries that don't have a dynamic address when forging the DEFSLVS frame instead of doing this allocate/free/re-allocate dance, and more importantly, you didn't even bother explaining why.

I'm not willing to accept this patch unless you come up with solid reasons.

I think I already give a strong reason for my decision. Let say that my controller only has space for 4 devices and one of them is offline during pre_assign_dyn_addr() and hence it fails. When device tries to do the HJ I won't be able to do the enumeration because there is no space left. Anyway, you are right and I need to add this to commit message.

BTW, It is not clear to me why should we keep non addressed devices allocated when we have i3c_dev_boardinfo list with ->of_node information and what is the problem with allocate/free/re-allocate dance?

Best regards,
Vitor Soares

#4 lore raw

Re: [PATCH v2 1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails

In reply to Vitor Soares

Hi Vitor,

I'm sorry for the delay.

Quoted 11 lines · Vitor Soares

EXTERNAL MAIL

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

What is the problem with sending devices without dynamic address in DEFSLVS? Shouldn't we still inform rest of the devices about that? About fact that device with SA without DA exists on the bus?

I think that this is limitation for us. Espacially we have SA and DA fields in DEFSLVS frame so we are able to distinguish devices with and without Dynamic Address.

Quoted 80 lines
Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Convert i3c_master_pre_assign_dyn_addr() to return an int

 drivers/i3c/master.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/i3c/master.c +15 -7
index 5f4bd52..586e34f 100644
@@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
 	int ret;
 
 	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
 	    !dev->boardinfo->static_addr)
-		return;
+		return 0;
 
 	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
 					dev->boardinfo->init_dyn_addr);
 	if (ret)
-		return;
+		return ret;
 
 	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
 	ret = i3c_master_reattach_i3c_dev(dev, 0);

@@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	return 0;
 
 err_rstdaa:
 	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+	return ret;
 }
 
 static void

@@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
+	struct i3c_dev_desc *i3cdev, *i3ctmp;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 

@@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+				 common.node) {
+		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
+		if (ret) {
+			i3c_master_detach_i3c_dev(i3cdev);
+			i3c_master_free_i3c_dev(i3cdev);
+		}
+	}
 
 	ret = i3c_master_do_daa(master);
 	if (ret)
Signature · 1 line
2.7.4
Signature · 2 lines
-- 
Przemyslaw Gaj
author
#5 lore raw

RE: [PATCH v2 1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails

From: Przemyslaw Gaj <[email protected]>
Date: Tue, Sep 03, 2019 at 12:13:57

Quoted 13 lines

Hi Vitor,

I'm sorry for the delay.

Quoted 8 lines · Vitor Soares

EXTERNAL MAIL

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

Quoted 5 lines
Quoted 3 lines

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

What is the problem with sending devices without dynamic address in DEFSLVS?

How do you address them?
For the DA field in DEFSLVS frame, the spec says: "shall contain the current value of the Slave's assigned 7-bit Dynamic address". If the device doesn't have the dynamic address assigned yet why send it?

Quoted 2 lines

Shouldn't we still inform rest of the devices about that? About fact that device with SA without DA exists on the bus?

I would like to understand what is the use case for this?

On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary Master is not responsible to do Dynamic Address Assignment but it is optional to do Hot-Join Dynamic Address Assignment.

Quoted 4 lines

I think that this is limitation for us. Espacially we have SA and DA fields in DEFSLVS frame so we are able to distinguish devices with and without Dynamic Address.

I would say the reason behind is regarding how to distinguish i2c from i3c devices.

Quoted 76 lines
Quoted 76 lines
Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Convert i3c_master_pre_assign_dyn_addr() to return an int

 drivers/i3c/master.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/i3c/master.c +15 -7
index 5f4bd52..586e34f 100644
@@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
 	int ret;
 
 	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
 	    !dev->boardinfo->static_addr)
-		return;
+		return 0;
 
 	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
 					dev->boardinfo->init_dyn_addr);
 	if (ret)
-		return;
+		return ret;
 
 	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
 	ret = i3c_master_reattach_i3c_dev(dev, 0);

@@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	return 0;
 
 err_rstdaa:
 	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+	return ret;
 }
 
 static void

@@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
+	struct i3c_dev_desc *i3cdev, *i3ctmp;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 

@@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+				 common.node) {
+		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
+		if (ret) {
+			i3c_master_detach_i3c_dev(i3cdev);
+			i3c_master_free_i3c_dev(i3cdev);
+		}
+	}
Quoted 9 lines
Quoted 5 lines
drivers/i3c/master.c +0 -0
 	ret = i3c_master_do_daa(master);
 	if (ret)
Signature · 1 line
2.7.4
Signature · 2 lines
-- 
Przemyslaw Gaj

Best regards,
Vitor Soares

#6 lore raw

Re: [PATCH v2 1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails

Quoted 23 lines · Vitor Soares

EXTERNAL MAIL

From: Przemyslaw Gaj <[email protected]>
Date: Tue, Sep 03, 2019 at 12:13:57

Quoted 17 lines

Hi Vitor,

I'm sorry for the delay.

Quoted 11 lines · Vitor Soares

EXTERNAL MAIL

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

Quoted 7 lines
Quoted 1 line

What is the problem with sending devices without dynamic address in DEFSLVS?

How do you address them?
For the DA field in DEFSLVS frame, the spec says: "shall contain the current value of the Slave's assigned 7-bit Dynamic address". If the device doesn't have the dynamic address assigned yet why send it?

What stops us from operating with this device in I2C mode using his SA?

Quoted 4 lines
Quoted 2 lines

Shouldn't we still inform rest of the devices about that? About fact that device with SA without DA exists on the bus?

I would like to understand what is the use case for this?

Look above. I2C mode still should be possible IMO. Just consider the case when you really need some information from that device, main master can assign dynamic address later but you can make some transfers. I know our framework does not support that case but secondary master can be different system which should know that this device exists and don't have DA yet, so I2C mode is available.

Quoted 5 lines

On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary Master is not responsible to do Dynamic Address Assignment but it is optional to do Hot-Join Dynamic Address Assignment.

Yes, we discussed that few times during the review of Mastership patch series.

Quoted 96 lines
Quoted 4 lines

I think that this is limitation for us. Espacially we have SA and DA fields in DEFSLVS frame so we are able to distinguish devices with and without Dynamic Address.

I would say the reason behind is regarding how to distinguish i2c from i3c devices.

Quoted 76 lines
Quoted 76 lines
Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Convert i3c_master_pre_assign_dyn_addr() to return an int

 drivers/i3c/master.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/i3c/master.c +15 -7
index 5f4bd52..586e34f 100644
@@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
 	int ret;
 
 	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
 	    !dev->boardinfo->static_addr)
-		return;
+		return 0;
 
 	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
 					dev->boardinfo->init_dyn_addr);
 	if (ret)
-		return;
+		return ret;
 
 	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
 	ret = i3c_master_reattach_i3c_dev(dev, 0);

@@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	return 0;
 
 err_rstdaa:
 	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+	return ret;
 }
 
 static void

@@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
+	struct i3c_dev_desc *i3cdev, *i3ctmp;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 

@@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+				 common.node) {
+		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
+		if (ret) {
+			i3c_master_detach_i3c_dev(i3cdev);
+			i3c_master_free_i3c_dev(i3cdev);
+		}
+	}
Quoted 9 lines
Quoted 5 lines
drivers/i3c/master.c +0 -0
 	ret = i3c_master_do_daa(master);
 	if (ret)
Signature · 1 line
2.7.4
Signature · 2 lines
-- 
Przemyslaw Gaj

Best regards,
Vitor Soares

Signature · 2 lines
-- 
Przemyslaw Gaj
author
#7 lore raw

RE: [PATCH v2 1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails

From: Przemyslaw Gaj <[email protected]>
Date: Wed, Sep 04, 2019 at 05:47:18

Quoted 26 lines
Quoted 25 lines · Vitor Soares

EXTERNAL MAIL

From: Przemyslaw Gaj <[email protected]>
Date: Tue, Sep 03, 2019 at 12:13:57

Quoted 18 lines

Hi Vitor,

I'm sorry for the delay.

Quoted 11 lines · Vitor Soares

EXTERNAL MAIL

On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()

are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().

What is the problem with sending devices without dynamic address in DEFSLVS?

Quoted 7 lines
Quoted 5 lines

How do you address them?
For the DA field in DEFSLVS frame, the spec says: "shall contain the current value of the Slave's assigned 7-bit Dynamic address". If the device doesn't have the dynamic address assigned yet why send it?

What stops us from operating with this device in I2C mode using his SA?

So, send it as an I2C device as spec says.

Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

Shouldn't we still inform rest of the devices about that? About fact that device with SA without DA exists on the bus?

I would like to understand what is the use case for this?

Look above. I2C mode still should be possible IMO. Just consider the case when you really need some information from that device, main master can assign dynamic address later but you can make some transfers.

It is true, but again it is a I2C device and not I3C device. It won't reply to I3C commands until has a DA.
How will you know that the DA sent in DEFSLVS is not assigned yet?

Quoted 4 lines

I know our framework
does not support that case but secondary master can be different system which should know that this device exists and don't have DA yet, so I2C mode is available.

If the HJ happen in secondary master side, there is a change to describe in DT what should be DA. Please check 2nd patch.

Quoted 8 lines
Quoted 6 lines

On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary Master is not responsible to do Dynamic Address Assignment but it is optional to do Hot-Join Dynamic Address Assignment.

Yes, we discussed that few times during the review of Mastership patch series.

Hence, based on that table, secondary master won't do DAA just because he want, It needs a HJ to trigger DAA.
Sorry, but for me based on what spec says this use case is not feasible.

Best regards,
Vitor Soares

author
#8 lore raw

PATCH v2 2/5 i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

In reply to Vitor Soares

The newdev->boardinfo assignment was missing in i3c_master_add_i3c_dev_locked() and hence the ->of_node info isn't propagated to i3c_dev_desc.

Fix this by trying to initialize device i3c_dev_boardinfo if available.

Cc <[email protected]>
Fixes 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by Vitor Soares <[email protected]>

---
Changes in v2:
- Change commit message
- Change i3c_master_search_i3c_boardinfo(newdev) to i3c_master_init_i3c_dev_boardinfo(newdev)
- Add fixes, stable tags

 drivers/i3c/master.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)
drivers/i3c/master.c +25 -2
index 586e34f..9fb99bc 100644
@@ -1798,6 +1798,22 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
 	return NULL;
 }
 
+static void i3c_master_init_i3c_dev_boardinfo(struct i3c_dev_desc *dev)
+{
+	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_dev_boardinfo *boardinfo;
+
+	if (dev->boardinfo)
+		return;
+
+	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
+		if (dev->info.pid == boardinfo->pid) {
+			dev->boardinfo = boardinfo;
+			return;
+		}
+	}
+}
+
 /**
  * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
  * @master: master used to send frames on the bus

@@ -1818,8 +1834,9 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 				  u8 addr)
 {
 	struct i3c_device_info info = { .dyn_addr = addr };
-	struct i3c_dev_desc *newdev, *olddev;
 	u8 old_dyn_addr = addr, expected_dyn_addr;
+	enum i3c_addr_slot_status addrstatus;
+	struct i3c_dev_desc *newdev, *olddev;
 	struct i3c_ibi_setup ibireq = { };
 	bool enable_ibi = false;
 	int ret;

@@ -1878,6 +1895,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto err_detach_dev;
 
+	i3c_master_init_i3c_dev_boardinfo(newdev);
+
 	/*
 	 * Depending on our previous state, the expected dynamic address might
 	 * differ:

@@ -1895,7 +1914,11 @@ 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) {
+	addrstatus = i3c_bus_get_addr_slot_status(&master->bus,
+						  expected_dyn_addr);
+
+	if (newdev->info.dyn_addr != expected_dyn_addr &&
+	    addrstatus == I3C_ADDR_SLOT_FREE) {
 		/*
 		 * Try to apply the expected dynamic address. If it fails, keep
 		 * the address assigned by the master.
Signature · 1 line
2.7.4
author
#9 lore raw

PATCH v2 3/5 dt-bindings: i3c: Make 'assigned-address' valid if static address == 0

In reply to Vitor Soares

The I3C devices without a static address can require a specific dynamic address for priority reasons.

Let's update the binding document to make the 'assigned-address' property valid if static address == 0 and add an example with this use case.

Signed-off-by Vitor Soares <[email protected]>
---
 Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/i3c/i3c.txt +10 -3
index ab729a0..c851e75 100644
@@ -98,9 +98,7 @@ Required properties
 
 Optional properties
 -------------------
-- assigned-address: dynamic address to be assigned to this device. This
-		    property is only valid if the I3C device has a static
-		    address (first cell of the reg property != 0).
+- assigned-address: dynamic address to be assigned to this device.
 
 
 Example:

@@ -129,6 +127,15 @@ Example:
 
 		/*
 		 * I3C device without a static I2C address but requiring
+		 * specific dynamic address.
+		 */
+		sensor@0,39200154004 {
+			reg = <0x0 0x6072 0x303904d2>;
+			assigned-address = <0xb>;
+		};
+
+		/*
+		 * I3C device without a static I2C address but requiring
 		 * resources described in the DT.
 		 */
 		sensor@0,39200154004 {
Signature · 1 line
2.7.4
maintainer Reviewed-by on v2 3/5
#10 lore raw

Re: [PATCH v2 3/5] dt-bindings: i3c: Make 'assigned-address' valid if static address == 0

Quoted 11 lines · Vitor Soares

The I3C devices without a static address can require a specific dynamic address for priority reasons.

Let's update the binding document to make the 'assigned-address' property valid if static address == 0 and add an example with this use case.

Signed-off-by Vitor Soares <[email protected]>
---
 Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
Reviewed-by Rob Herring <[email protected]>
author
#11 lore raw

PATCH v2 4/5 dt-bindings: i3c: add a note for no guarantee of 'assigned-address' use

In reply to Vitor Soares

By default, the framework will try to assign the 'assigned-address' to the device but if the address is already in use the device dynamic address will be different.

Signed-off-by Vitor Soares <[email protected]>
---
 Documentation/devicetree/bindings/i3c/i3c.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/i3c/i3c.txt +3 -1
index c851e75..e777f09 100644
@@ -98,7 +98,9 @@ Required properties
 
 Optional properties
 -------------------
-- assigned-address: dynamic address to be assigned to this device.
+- assigned-address: dynamic address to be assigned to this device. The framework
+		    will try to assign this dynamic address but if something
+		    fails the device dynamic address will be different.
 
 
 Example:
Signature · 1 line
2.7.4
maintainer Reviewed-by on v2 4/5
#12 lore raw

Re: [PATCH v2 4/5] dt-bindings: i3c: add a note for no guarantee of 'assigned-address' use

Quoted 9 lines · Vitor Soares

By default, the framework will try to assign the 'assigned-address' to the device but if the address is already in use the device dynamic address will be different.

Signed-off-by Vitor Soares <[email protected]>
---
 Documentation/devicetree/bindings/i3c/i3c.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by Rob Herring <[email protected]>
author
#13 lore raw

PATCH v2 5/5 i3c: master: dw: reattach device on first available location of address table

In reply to Vitor Soares

For today the reattach function only update the device address on the controller.

Update the location to the first available too, will optimize the enumeration process avoiding additional checks to keep the available positions on address table consecutive.

Signed-off-by Vitor Soares <[email protected]>
Reviewed-by Boris Brezillon <[email protected]>

---
Change in v2:
- Add Boris rb-tag

 drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/i3c/master/dw-i3c-master.c +16 -0
index 1d83c97..62261ac 100644
@@ -898,6 +898,22 @@ static int dw_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	int pos;
+
+	pos = dw_i3c_master_get_free_pos(master);
+
+	if (data->index > pos && pos > 0) {
+		writel(0,
+		       master->regs +
+		       DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+
+		master->addrs[data->index] = 0;
+		master->free_pos |= BIT(data->index);
+
+		data->index = pos;
+		master->addrs[pos] = dev->info.dyn_addr;
+		master->free_pos &= ~BIT(pos);
+	}
 
 	writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(dev->info.dyn_addr),
 	       master->regs +
Signature · 1 line
2.7.4