[PATCH v3 0/5] i3c: detach/free device fail pre_assign_dyn_addr()

[PATCH v3 0/5] i3c: detach/free device fail pre_assign_dyn_addr()

[Vitor Soares]

As for today, the I3C framework is keeping in memory and master->bus.devs
list the devices that fail during pre_assign_dyn_addr() and send them on
DEFSLVS command.

According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
Secondary Master about the Dynamic Addresses that were assigned to I3C
devices and the I2C devices present on the bus.

This issue could be fixed by changing i3c_master_defslvs_locked() to
ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
attached to HC unnecessarily. This can cause that some HC aren't able to
do DAA for HJ capable devices due of lack of space.

This patch-series propose to detach/free devices that are failing during
pre_assign_dyn_addr() and to propagate i3c_boardinfo, if available, to
i3c_dev_desc during i3c_master_add_i3c_dev_locked(). Besides the fix for
the problem mention above, this change will permit to describe devices
with a preferable dynamic address (important due to priority reason) but
without a static address in DT.

In addition, I'm improving the management of the Data Address Table in
DW I3C Master by keeping the free slots consecutive.

Change in v3:
  - Change cover letter
  - Change commit message for patch 1
  - Add Rob rb-tags

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

-- 
2.7.4

[PATCH v3 0/5] i3c: detach/free device fail pre_assign_dyn_addr()

[Vitor Soares]

As for today, the I3C framework is keeping in memory and master->bus.devs
list the devices that fail during pre_assign_dyn_addr() and send them on
DEFSLVS command.

According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
Secondary Master about the Dynamic Addresses that were assigned to I3C
devices and the I2C devices present on the bus.

This issue could be fixed by changing i3c_master_defslvs_locked() to
ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
attached to HC unnecessarily. This can cause that some HC aren't able to
do DAA for HJ capable devices due of lack of space.

This patch-series propose to detach/free devices that are failing during
pre_assign_dyn_addr() and to propagate i3c_boardinfo, if available, to
i3c_dev_desc during i3c_master_add_i3c_dev_locked(). Besides the fix for
the problem mention above, this change will permit to describe devices
with a preferable dynamic address (important due to priority reason) but
without a static address in DT.

In addition, I'm improving the management of the Data Address Table in
DW I3C Master by keeping the free slots consecutive.

Change in v3:
  - Change cover letter
  - Change commit message for patch 1
  - Add Rob rb-tags

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

-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 1/5] i3c: master: detach/free devices fail on pre_assign_dyn_addr()

[Vitor Soares]

As for today, the I3C framework is keeping in memory and master->bus.devs
list the devices that fail during pre_assign_dyn_addr() and send them on
DEFSLVS command.

According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
Secondary Master about the Dynamic Addresses that were assigned to I3C
devices and the I2C devices present on the bus.

This issue could be fixed by changing i3c_master_defslvs_locked() to
ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
attached to HC unnecessarily. This can cause that some HC aren't able to
do DAA for HJ capable devices due to lack of space in controller.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
Changes in v3:
  - Change commit message

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

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52..586e34f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -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)
-- 
2.7.4

[PATCH v3 1/5] i3c: master: detach/free devices fail on pre_assign_dyn_addr()

[Vitor Soares]

As for today, the I3C framework is keeping in memory and master->bus.devs
list the devices that fail during pre_assign_dyn_addr() and send them on
DEFSLVS command.

According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
Secondary Master about the Dynamic Addresses that were assigned to I3C
devices and the I2C devices present on the bus.

This issue could be fixed by changing i3c_master_defslvs_locked() to
ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
attached to HC unnecessarily. This can cause that some HC aren't able to
do DAA for HJ capable devices due to lack of space in controller.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
Changes in v3:
  - Change commit message

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

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52..586e34f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -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)
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[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: <stable@vger.kernel.org>
Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
Change in v3:
  - None

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

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 586e34f..9fb99bc 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -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.
-- 
2.7.4

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

[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: <stable@vger.kernel.org>
Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
Change in v3:
  - None

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

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 586e34f..9fb99bc 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -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.
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[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 <vitor.soares@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change in v3:
  - Add Rob rb-tag

Change in v2:
  - Fix typo in commit message

 Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i3c/i3c.txt b/Documentation/devicetree/bindings/i3c/i3c.txt
index ab729a0..c851e75 100644
--- a/Documentation/devicetree/bindings/i3c/i3c.txt
+++ b/Documentation/devicetree/bindings/i3c/i3c.txt
@@ -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 {
-- 
2.7.4

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

[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 <vitor.soares@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change in v3:
  - Add Rob rb-tag

Change in v2:
  - Fix typo in commit message

 Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i3c/i3c.txt b/Documentation/devicetree/bindings/i3c/i3c.txt
index ab729a0..c851e75 100644
--- a/Documentation/devicetree/bindings/i3c/i3c.txt
+++ b/Documentation/devicetree/bindings/i3c/i3c.txt
@@ -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 {
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[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 <vitor.soares@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change in v3:
  - Add Rob rb-tag

 Documentation/devicetree/bindings/i3c/i3c.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i3c/i3c.txt b/Documentation/devicetree/bindings/i3c/i3c.txt
index c851e75..e777f09 100644
--- a/Documentation/devicetree/bindings/i3c/i3c.txt
+++ b/Documentation/devicetree/bindings/i3c/i3c.txt
@@ -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:
-- 
2.7.4

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

[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 <vitor.soares@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change in v3:
  - Add Rob rb-tag

 Documentation/devicetree/bindings/i3c/i3c.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i3c/i3c.txt b/Documentation/devicetree/bindings/i3c/i3c.txt
index c851e75..e777f09 100644
--- a/Documentation/devicetree/bindings/i3c/i3c.txt
+++ b/Documentation/devicetree/bindings/i3c/i3c.txt
@@ -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:
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[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 <vitor.soares@synopsys.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Change in v3:
  - None

Change in v2:
  - Add Boris rb-tag

 drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 1d83c97..62261ac 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -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 +
-- 
2.7.4

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

[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 <vitor.soares@synopsys.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Change in v3:
  - None

Change in v2:
  - Add Boris rb-tag

 drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 1d83c97..62261ac 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -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 +
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 1/5] i3c: master: detach/free devices fail on pre_assign_dyn_addr()

[Vitor Soares]

Hi Boris,

From: Vitor Soares <vitor.soares@synopsys.com>
Date: Thu, Sep 05, 2019 at 11:00:34

> As for today, the I3C framework is keeping in memory and master->bus.devs
> list the devices that fail during pre_assign_dyn_addr() and send them on
> DEFSLVS command.
> 
> According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
> Secondary Master about the Dynamic Addresses that were assigned to I3C
> devices and the I2C devices present on the bus.
> 
> This issue could be fixed by changing i3c_master_defslvs_locked() to
> ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
> attached to HC unnecessarily. This can cause that some HC aren't able to
> do DAA for HJ capable devices due to lack of space in controller.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
> Changes in v3:
>   - Change commit message
> 
> 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(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52..586e34f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -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)
> -- 
> 2.7.4

I think I clearly explain why the current solution is problematic and my 
choices for the proposal solution.
Please let me know if there is anything else that I can improve in this 
patch.

Best regards,
Vitor Soares

RE: [PATCH v3 1/5] i3c: master: detach/free devices fail on pre_assign_dyn_addr()

[Vitor Soares]

Hi Boris,

From: Vitor Soares <vitor.soares@synopsys.com>
Date: Thu, Sep 05, 2019 at 11:00:34

> As for today, the I3C framework is keeping in memory and master->bus.devs
> list the devices that fail during pre_assign_dyn_addr() and send them on
> DEFSLVS command.
> 
> According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
> Secondary Master about the Dynamic Addresses that were assigned to I3C
> devices and the I2C devices present on the bus.
> 
> This issue could be fixed by changing i3c_master_defslvs_locked() to
> ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
> attached to HC unnecessarily. This can cause that some HC aren't able to
> do DAA for HJ capable devices due to lack of space in controller.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
> Changes in v3:
>   - Change commit message
> 
> 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(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52..586e34f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -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)
> -- 
> 2.7.4

I think I clearly explain why the current solution is problematic and my 
choices for the proposal solution.
Please let me know if there is anything else that I can improve in this 
patch.

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

[Boris Brezillon]

On Thu,  5 Sep 2019 12:00:35 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> 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: <stable@vger.kernel.org>
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
> Change in v3:
>   - None
> 
> 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(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 586e34f..9fb99bc 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -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) {

First, this change shouldn't be part of this patch, since the commit
message only mentions the boardinfo init stuff, not the extra 'is slot
free check'. Plus, I want the fix to be backported so we should avoid
any unneeded deps.

But even with those 2 things addressed, I'm still convinced the
'free desc when device is not reachable' change you do in patch 1 is
not that great, and the fact that you can't pre-reserve the address to
make sure no one uses it until the device had a chance to show up tends
to prove me right.

Can we please do what I suggest and solve the "not enough dev slots"
problem later on (if we really have to).

>  		/*
>  		 * Try to apply the expected dynamic address. If it fails, keep
>  		 * the address assigned by the master.

Re: [PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

[Boris Brezillon]

On Thu,  5 Sep 2019 12:00:35 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> 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: <stable@vger.kernel.org>
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
> Change in v3:
>   - None
> 
> 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(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 586e34f..9fb99bc 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -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) {

First, this change shouldn't be part of this patch, since the commit
message only mentions the boardinfo init stuff, not the extra 'is slot
free check'. Plus, I want the fix to be backported so we should avoid
any unneeded deps.

But even with those 2 things addressed, I'm still convinced the
'free desc when device is not reachable' change you do in patch 1 is
not that great, and the fact that you can't pre-reserve the address to
make sure no one uses it until the device had a chance to show up tends
to prove me right.

Can we please do what I suggest and solve the "not enough dev slots"
problem later on (if we really have to).

>  		/*
>  		 * Try to apply the expected dynamic address. If it fails, keep
>  		 * the address assigned by the master.


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Oct 03, 2019 at 15:29:43

> On Thu,  5 Sep 2019 12:00:35 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > 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: <stable@vger.kernel.org>
> > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> > Change in v3:
> >   - None
> > 
> > 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
> > 
> >  /**
> >   * 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) {
> 
> First, this change shouldn't be part of this patch, since the commit
> message only mentions the boardinfo init stuff,

This is not an issue, I can create a patch just for boardinfo init fix.

> not the extra 'is slot
> free check'.

Even ignoring patch 1, it is necessary to check if the slot is free 
because if SETDASA fails the boardinfo->init_dyn_addr can be assigned to 
another device. That's why we need to check if expected_dyn_addr is free.

> Plus, I want the fix to be backported so we should avoid
> any unneeded deps.
> 
> But even with those 2 things addressed, I'm still convinced the
> 'free desc when device is not reachable' change you do in patch 1 is
> not that great, 

If I'm doing wrong I really appreciate you tell me the reason.

> and the fact that you can't pre-reserve the address to
> make sure no one uses it until the device had a chance to show up tends
> to prove me right.

This is a different corner case and I though we agreed that the framework 
doesn't provide guarantees to assign boardinfo->init_dyn_addr [1].

Yet, I don't disagree with the idea of pre-reserve the 
boardinfo->init_dyn_addr.
I can do this but we need to align how it should be done.

> 
> Can we please do what I suggest and solve the "not enough dev slots"
> problem later on (if we really have to).

I have this use case where the HC has only 4 slot for 4 devices. 
Sometimes the one or more devices can be sleeping and when they trigger 
HJ there is no space in HC.

Best regards,
Vitor Soares

[1] https://patchwork.kernel.org/patch/11120841/

RE: [PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Oct 03, 2019 at 15:29:43

> On Thu,  5 Sep 2019 12:00:35 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > 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: <stable@vger.kernel.org>
> > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> > Change in v3:
> >   - None
> > 
> > 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
> > 
> >  /**
> >   * 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) {
> 
> First, this change shouldn't be part of this patch, since the commit
> message only mentions the boardinfo init stuff,

This is not an issue, I can create a patch just for boardinfo init fix.

> not the extra 'is slot
> free check'.

Even ignoring patch 1, it is necessary to check if the slot is free 
because if SETDASA fails the boardinfo->init_dyn_addr can be assigned to 
another device. That's why we need to check if expected_dyn_addr is free.

> Plus, I want the fix to be backported so we should avoid
> any unneeded deps.
> 
> But even with those 2 things addressed, I'm still convinced the
> 'free desc when device is not reachable' change you do in patch 1 is
> not that great, 

If I'm doing wrong I really appreciate you tell me the reason.

> and the fact that you can't pre-reserve the address to
> make sure no one uses it until the device had a chance to show up tends
> to prove me right.

This is a different corner case and I though we agreed that the framework 
doesn't provide guarantees to assign boardinfo->init_dyn_addr [1].

Yet, I don't disagree with the idea of pre-reserve the 
boardinfo->init_dyn_addr.
I can do this but we need to align how it should be done.

> 
> Can we please do what I suggest and solve the "not enough dev slots"
> problem later on (if we really have to).

I have this use case where the HC has only 4 slot for 4 devices. 
Sometimes the one or more devices can be sleeping and when they trigger 
HJ there is no space in HC.

Best regards,
Vitor Soares

[1] https://patchwork.kernel.org/patch/11120841/

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

[Boris Brezillon]

On Thu, 3 Oct 2019 17:37:40 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Oct 03, 2019 at 15:29:43
> 
> > On Thu,  5 Sep 2019 12:00:35 +0200
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > 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: <stable@vger.kernel.org>
> > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > ---
> > > Change in v3:
> > >   - None
> > > 
> > > 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
> > > 
> > >  /**
> > >   * 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) {  
> > 
> > First, this change shouldn't be part of this patch, since the commit
> > message only mentions the boardinfo init stuff,  
> 
> This is not an issue, I can create a patch just for boardinfo init fix.
> 
> > not the extra 'is slot
> > free check'.  
> 
> Even ignoring patch 1, it is necessary to check if the slot is free 
> because if SETDASA fails the boardinfo->init_dyn_addr can be assigned to 
> another device. That's why we need to check if expected_dyn_addr is free.

Correct. I thought we were already pre-reserving the init_addr (as
described here [1]), but it looks like the code is buggy. That's
probably something we should fix  (we should reserve ->init_i3c_addr
here [2], not ->dyn_addr).

> 
> > Plus, I want the fix to be backported so we should avoid
> > any unneeded deps.
> > 
> > But even with those 2 things addressed, I'm still convinced the
> > 'free desc when device is not reachable' change you do in patch 1 is
> > not that great,   
> 
> If I'm doing wrong I really appreciate you tell me the reason.

I just think it's easier to keep track of things (like reserved
addresses) if the descriptor stays around even if the device is not yet
accessible.

> 
> > and the fact that you can't pre-reserve the address to
> > make sure no one uses it until the device had a chance to show up tends
> > to prove me right.  
> 
> This is a different corner case and I though we agreed that the framework 
> doesn't provide guarantees to assign boardinfo->init_dyn_addr [1].

Well, it doesn't, but we should try hard to not use addresses that
have been requested by a device.

> 
> Yet, I don't disagree with the idea of pre-reserve the 
> boardinfo->init_dyn_addr.
> I can do this but we need to align how it should be done.

Keep the device around even if SETDASA fails and make sure the
->init_dyn_addr is reserved. It's how it was supposed to work, there's
just a bug in the logic.

> 
> > 
> > Can we please do what I suggest and solve the "not enough dev slots"
> > problem later on (if we really have to).  
> 
> I have this use case where the HC has only 4 slot for 4 devices. 
> Sometimes the one or more devices can be sleeping and when they trigger 
> HJ there is no space in HC.

Let's address that separately please. I want to solve one problem at a
time.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L1330
[2]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L1307

Re: [PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()

[Boris Brezillon]

On Thu, 3 Oct 2019 17:37:40 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Oct 03, 2019 at 15:29:43
> 
> > On Thu,  5 Sep 2019 12:00:35 +0200
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > 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: <stable@vger.kernel.org>
> > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > ---
> > > Change in v3:
> > >   - None
> > > 
> > > 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
> > > 
> > >  /**
> > >   * 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) {  
> > 
> > First, this change shouldn't be part of this patch, since the commit
> > message only mentions the boardinfo init stuff,  
> 
> This is not an issue, I can create a patch just for boardinfo init fix.
> 
> > not the extra 'is slot
> > free check'.  
> 
> Even ignoring patch 1, it is necessary to check if the slot is free 
> because if SETDASA fails the boardinfo->init_dyn_addr can be assigned to 
> another device. That's why we need to check if expected_dyn_addr is free.

Correct. I thought we were already pre-reserving the init_addr (as
described here [1]), but it looks like the code is buggy. That's
probably something we should fix  (we should reserve ->init_i3c_addr
here [2], not ->dyn_addr).

> 
> > Plus, I want the fix to be backported so we should avoid
> > any unneeded deps.
> > 
> > But even with those 2 things addressed, I'm still convinced the
> > 'free desc when device is not reachable' change you do in patch 1 is
> > not that great,   
> 
> If I'm doing wrong I really appreciate you tell me the reason.

I just think it's easier to keep track of things (like reserved
addresses) if the descriptor stays around even if the device is not yet
accessible.

> 
> > and the fact that you can't pre-reserve the address to
> > make sure no one uses it until the device had a chance to show up tends
> > to prove me right.  
> 
> This is a different corner case and I though we agreed that the framework 
> doesn't provide guarantees to assign boardinfo->init_dyn_addr [1].

Well, it doesn't, but we should try hard to not use addresses that
have been requested by a device.

> 
> Yet, I don't disagree with the idea of pre-reserve the 
> boardinfo->init_dyn_addr.
> I can do this but we need to align how it should be done.

Keep the device around even if SETDASA fails and make sure the
->init_dyn_addr is reserved. It's how it was supposed to work, there's
just a bug in the logic.

> 
> > 
> > Can we please do what I suggest and solve the "not enough dev slots"
> > problem later on (if we really have to).  
> 
> I have this use case where the HC has only 4 slot for 4 devices. 
> Sometimes the one or more devices can be sleeping and when they trigger 
> HJ there is no space in HC.

Let's address that separately please. I want to solve one problem at a
time.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L1330
[2]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L1307

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c