Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] i3c: remove device if failed on pre_assign_dyn_addr()
@ 2019-08-29 10:19 Vitor Soares
  2019-08-29 10:19 ` [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails Vitor Soares
                   ` (3 more replies)
  0 siblings, 4 replies; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 10:19 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-i3c
  Cc: mark.rutland, Joao.Pinto, robh+dt, Vitor Soares, bbrezillon

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.

Vitor Soares (4):
  "i3c: detach and free device if pre_assign_dyn_addr fails "
  i3c: check i3c_boardinfo during i3c_master_add_i3c_dev_locked
  update i3c bingins
  i3c: master: dw: Reattach device on first empty location of DAT

 Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++---
 drivers/i3c/master.c                          | 33 ++++++++++++++++++++++++---
 drivers/i3c/master/dw-i3c-master.c            | 16 +++++++++++++
 3 files changed, 56 insertions(+), 6 deletions(-)

-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 10:19 [PATCH 0/4] i3c: remove device if failed on pre_assign_dyn_addr() Vitor Soares
@ 2019-08-29 10:19 ` Vitor Soares
  2019-08-29 10:41   ` Boris Brezillon
  2019-08-29 10:19 ` [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked() Vitor Soares
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 10:19 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-i3c
  Cc: mark.rutland, Joao.Pinto, robh+dt, Vitor Soares, bbrezillon

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 <vitor.soares@synopsys.com>
---
 drivers/i3c/master.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52..4d29e1f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1438,7 +1438,7 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
 					dev->boardinfo->init_dyn_addr);
 	if (ret)
-		return;
+		goto err_detach_dev;
 
 	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
 	ret = i3c_master_reattach_i3c_dev(dev, 0);
@@ -1453,6 +1453,10 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 
 err_rstdaa:
 	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+err_detach_dev:
+	i3c_master_detach_i3c_dev(dev);
+	i3c_master_free_i3c_dev(dev);
 }
 
 static void
@@ -1647,7 +1651,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,7 +1750,8 @@ 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)
+	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+				 common.node)
 		i3c_master_pre_assign_dyn_addr(i3cdev);
 
 	ret = i3c_master_do_daa(master);
-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 10:19 [PATCH 0/4] i3c: remove device if failed on pre_assign_dyn_addr() Vitor Soares
  2019-08-29 10:19 ` [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails Vitor Soares
@ 2019-08-29 10:19 ` Vitor Soares
  2019-08-29 10:44   ` Boris Brezillon
  2019-08-29 16:27   ` Boris Brezillon
  2019-08-29 10:19 ` [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0 Vitor Soares
  2019-08-29 10:19 ` [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table Vitor Soares
  3 siblings, 2 replies; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 10:19 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-i3c
  Cc: mark.rutland, Joao.Pinto, robh+dt, Vitor Soares, bbrezillon

The I3C devices described in DT might not be attached to the master which
doesn't allow to assign a specific dynamic address.

This patch check if a device has i3c_dev_boardinfo and add it to
i3c_dev_desc structure. In this conditions, the framework will try to
assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/master.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 4d29e1f..85fbda6 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
 	return NULL;
 }
 
+static struct i3c_dev_boardinfo *
+i3c_master_search_i3c_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 NULL;
+
+	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
+		if (dev->info.pid == boardinfo->pid)
+			return boardinfo;
+	}
+
+	return NULL;
+}
+
 /**
  * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
  * @master: master used to send frames on the bus
@@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 {
 	struct i3c_device_info info = { .dyn_addr = addr };
 	struct i3c_dev_desc *newdev, *olddev;
+	struct i3c_dev_boardinfo *boardinfo;
 	u8 old_dyn_addr = addr, expected_dyn_addr;
 	struct i3c_ibi_setup ibireq = { };
 	bool enable_ibi = false;
@@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto err_detach_dev;
 
+	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
+	if (boardinfo)
+		newdev->boardinfo = boardinfo;
+
 	/*
 	 * Depending on our previous state, the expected dynamic address might
 	 * differ:
-- 
2.7.4


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0
  2019-08-29 10:19 [PATCH 0/4] i3c: remove device if failed on pre_assign_dyn_addr() Vitor Soares
  2019-08-29 10:19 ` [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails Vitor Soares
  2019-08-29 10:19 ` [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked() Vitor Soares
@ 2019-08-29 10:19 ` Vitor Soares
  2019-08-29 10:51   ` Boris Brezillon
  2019-08-29 10:19 ` [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table Vitor Soares
  3 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 10:19 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-i3c
  Cc: mark.rutland, Joao.Pinto, robh+dt, Vitor Soares, bbrezillon

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table
  2019-08-29 10:19 [PATCH 0/4] i3c: remove device if failed on pre_assign_dyn_addr() Vitor Soares
                   ` (2 preceding siblings ...)
  2019-08-29 10:19 ` [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0 Vitor Soares
@ 2019-08-29 10:19 ` Vitor Soares
  2019-08-29 11:15   ` Boris Brezillon
  3 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 10:19 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-i3c
  Cc: mark.rutland, Joao.Pinto, robh+dt, Vitor Soares, bbrezillon

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 10:19 ` [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails Vitor Soares
@ 2019-08-29 10:41   ` Boris Brezillon
  2019-08-29 13:53     ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 10:41 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	Przemyslaw Gaj, robh+dt, linux-i3c

+Przemek

Please try to Cc active I3C contributors so they get a chance to
comment on your patches.

On Thu, 29 Aug 2019 12:19:32 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

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

I don't think removing those entries is a good strategy, as one might
want to try to use a different dynamic address if the requested one
is not available. Why not simply skipping entries that have ->dyn_addr
set to 0 when preparing a DEFSLVS frame

> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/master.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52..4d29e1f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1438,7 +1438,7 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
>  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
>  					dev->boardinfo->init_dyn_addr);
>  	if (ret)
> -		return;
> +		goto err_detach_dev;
>  
>  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
>  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> @@ -1453,6 +1453,10 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
>  
>  err_rstdaa:
>  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> +
> +err_detach_dev:
> +	i3c_master_detach_i3c_dev(dev);
> +	i3c_master_free_i3c_dev(dev);

We certainly shouldn't detach/free the i3c_dev_desc from here. If it
has to be done somewhere (which I'd like to avoid), it should be done
in i3c_master_bus_init() (i3c_master_pre_assign_dyn_addr() should be
converted to return an int in that case).

>  }
>  
>  static void
> @@ -1647,7 +1651,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,7 +1750,8 @@ 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)
> +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> +				 common.node)
>  		i3c_master_pre_assign_dyn_addr(i3cdev);
>  
>  	ret = i3c_master_do_daa(master);


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 10:19 ` [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked() Vitor Soares
@ 2019-08-29 10:44   ` Boris Brezillon
  2019-08-29 14:00     ` Vitor Soares
  2019-08-29 16:27   ` Boris Brezillon
  1 sibling, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 10:44 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 12:19:33 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> The I3C devices described in DT might not be attached to the master which
> doesn't allow to assign a specific dynamic address.

I remember testing this when developing the framework, so, unless
another patch regressed it, it should already work. I suspect patch 1
is actually the regressing this use case.

> 
> This patch check if a device has i3c_dev_boardinfo and add it to
> i3c_dev_desc structure. In this conditions, the framework will try to
> assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/master.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4d29e1f..85fbda6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +static struct i3c_dev_boardinfo *
> +i3c_master_search_i3c_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 NULL;
> +
> +	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> +		if (dev->info.pid == boardinfo->pid)
> +			return boardinfo;
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  {
>  	struct i3c_device_info info = { .dyn_addr = addr };
>  	struct i3c_dev_desc *newdev, *olddev;
> +	struct i3c_dev_boardinfo *boardinfo;
>  	u8 old_dyn_addr = addr, expected_dyn_addr;
>  	struct i3c_ibi_setup ibireq = { };
>  	bool enable_ibi = false;
> @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_detach_dev;
>  
> +	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> +	if (boardinfo)
> +		newdev->boardinfo = boardinfo;
> +
>  	/*
>  	 * Depending on our previous state, the expected dynamic address might
>  	 * differ:


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0
  2019-08-29 10:19 ` [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0 Vitor Soares
@ 2019-08-29 10:51   ` Boris Brezillon
  2019-08-29 14:06     ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 10:51 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 12:19:34 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

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

           ^ you mean static address == 0, right?

Yes, it makes sense to support that case and do our best to assign the
requested address after DAA has taken place by explicitly executing
SETDA.

> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  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.

We should probably mention that we don't provide strong guarantees
here. We will try to assign this dynamic address to the device, but if
something fails (like another device owning the address and refusing to
give it up), the actual dynamic address will be different.
This clarification can be done in a separate patch.

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


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table
  2019-08-29 10:19 ` [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table Vitor Soares
@ 2019-08-29 11:15   ` Boris Brezillon
  2019-08-29 14:09     ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 11:15 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 12:19:35 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

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

Given the number of available slots I honestly don't think it makes a
difference, but I also don't mind this change, so

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  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 +


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 10:41   ` Boris Brezillon
@ 2019-08-29 13:53     ` Vitor Soares
  2019-08-29 14:35       ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 13:53 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	Przemyslaw Gaj, robh+dt, linux-i3c

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 11:41:15

> +Przemek
> 
> Please try to Cc active I3C contributors so they get a chance to
> comment on your patches.

I can do that next time.

> 
> On Thu, 29 Aug 2019 12:19:32 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > 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().
> 
> I don't think removing those entries is a good strategy, as one might
> want to try to use a different dynamic address if the requested one
> is not available.

Do you mean same 'assigned-address' attribute in DT?

If so, it is checked here:

static int i3c_master_bus_init(struct i3c_master_controller *master)
...
	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
		struct i3c_device_info info = {
			.static_addr = i3cboardinfo->static_addr,
		};

		if (i3cboardinfo->init_dyn_addr) {
			status = i3c_bus_get_addr_slot_status(&master->bus,
			^
						i3cboardinfo->init_dyn_addr);
			if (status != I3C_ADDR_SLOT_FREE) {
				ret = -EBUSY;
				goto err_detach_devs;
			}
		}

		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
		if (IS_ERR(i3cdev)) {
			ret = PTR_ERR(i3cdev);
			goto err_detach_devs;
		}

		i3cdev->boardinfo = i3cboardinfo;

		ret = i3c_master_attach_i3c_dev(master, i3cdev);
		if (ret) {
			i3c_master_free_i3c_dev(i3cdev);
			goto err_detach_devs;
		}
	}
...

and later if it fails i3c_master_pre_assign_dyn_addr(), the device can 
participate in Enter Dynamic Address Assignment process.
I may need to check the boardinfo->init_dyn_addr status on 
i3c_master_add_i3c_dev_locked before i3c_master_setnewda_locked().

> Why not simply skipping entries that have ->dyn_addr
> set to 0 when preparing a DEFSLVS frame

I considered that solution too but if the device isn't enumerated why 
should it be attached and kept in memory? Anyway we have i3c_boardinfo 
with DT information.

> 
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  drivers/i3c/master.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 5f4bd52..4d29e1f 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1438,7 +1438,7 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> >  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> >  					dev->boardinfo->init_dyn_addr);
> >  	if (ret)
> > -		return;
> > +		goto err_detach_dev;
> >  
> >  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> >  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> > @@ -1453,6 +1453,10 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> >  
> >  err_rstdaa:
> >  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> > +
> > +err_detach_dev:
> > +	i3c_master_detach_i3c_dev(dev);
> > +	i3c_master_free_i3c_dev(dev);
> 
> We certainly shouldn't detach/free the i3c_dev_desc from here. If it
> has to be done somewhere (which I'd like to avoid), it should be done
> in i3c_master_bus_init() (i3c_master_pre_assign_dyn_addr() should be
> converted to return an int in that case).

I can change it to return an error. 

> 
> >  }
> >  
> >  static void
> > @@ -1647,7 +1651,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,7 +1750,8 @@ 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)
> > +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> > +				 common.node)
> >  		i3c_master_pre_assign_dyn_addr(i3cdev);
> >  
> >  	ret = i3c_master_do_daa(master);

Thank for your feedback.

Best regards,
Vitor Soares

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 10:44   ` Boris Brezillon
@ 2019-08-29 14:00     ` Vitor Soares
  2019-08-29 14:39       ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 14:00 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 11:44:57

> On Thu, 29 Aug 2019 12:19:33 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > The I3C devices described in DT might not be attached to the master which
> > doesn't allow to assign a specific dynamic address.
> 
> I remember testing this when developing the framework, so, unless
> another patch regressed it, it should already work. I suspect patch 1
> is actually the regressing this use case.

For today it doesn't address the case where the device is described with 
static address = 0, which isn't attached to the controller.
With this change both cases are covered.

> 
> > 
> > This patch check if a device has i3c_dev_boardinfo and add it to
> > i3c_dev_desc structure. In this conditions, the framework will try to
> > assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  drivers/i3c/master.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 4d29e1f..85fbda6 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> >  	return NULL;
> >  }
> >  
> > +static struct i3c_dev_boardinfo *
> > +i3c_master_search_i3c_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 NULL;
> > +
> > +	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> > +		if (dev->info.pid == boardinfo->pid)
> > +			return boardinfo;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  /**
> >   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> >   * @master: master used to send frames on the bus
> > @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> >  {
> >  	struct i3c_device_info info = { .dyn_addr = addr };
> >  	struct i3c_dev_desc *newdev, *olddev;
> > +	struct i3c_dev_boardinfo *boardinfo;
> >  	u8 old_dyn_addr = addr, expected_dyn_addr;
> >  	struct i3c_ibi_setup ibireq = { };
> >  	bool enable_ibi = false;
> > @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> >  	if (ret)
> >  		goto err_detach_dev;
> >  
> > +	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> > +	if (boardinfo)
> > +		newdev->boardinfo = boardinfo;
> > +
> >  	/*
> >  	 * Depending on our previous state, the expected dynamic address might
> >  	 * differ:

Best regards,
Vitor Soares


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0
  2019-08-29 10:51   ` Boris Brezillon
@ 2019-08-29 14:06     ` Vitor Soares
  0 siblings, 0 replies; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 14:06 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 11:51:38

> On Thu, 29 Aug 2019 12:19:34 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > 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.
> 
>            ^ you mean static address == 0, right?

Indeed.

> 
> Yes, it makes sense to support that case and do our best to assign the
> requested address after DAA has taken place by explicitly executing
> SETDA.
> 
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  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.
> 
> We should probably mention that we don't provide strong guarantees
> here. We will try to assign this dynamic address to the device, but if
> something fails (like another device owning the address and refusing to
> give it up), the actual dynamic address will be different.
> This clarification can be done in a separate patch.

So, another patch on top of this one explaining that, right?

I would suggest to use a dynamic address, like 0x40 (mid priority), on 
ENTDAA so lowers addresses (High Priority) can be used in DT or another 
method.

What do you think about this? 

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

Best regards,
Vitor Soares

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table
  2019-08-29 11:15   ` Boris Brezillon
@ 2019-08-29 14:09     ` Vitor Soares
  0 siblings, 0 replies; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 14:09 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 12:15:19

> On Thu, 29 Aug 2019 12:19:35 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > 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.
> 
> Given the number of available slots I honestly don't think it makes a
> difference, but I also don't mind this change, so

The slots are HW dependent. The point is, I need to guarantee the 
available slot are consecutives.
If you have any suggestion I appreciate.

> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  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 +

Best regards,
Vitor Soares


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 13:53     ` Vitor Soares
@ 2019-08-29 14:35       ` Boris Brezillon
  2019-08-29 15:23         ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 14:35 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	Przemyslaw Gaj, robh+dt, linux-i3c

On Thu, 29 Aug 2019 13:53:24 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Aug 29, 2019 at 11:41:15
> 
> > +Przemek
> > 
> > Please try to Cc active I3C contributors so they get a chance to
> > comment on your patches.  
> 
> I can do that next time.
> 
> > 
> > On Thu, 29 Aug 2019 12:19:32 +0200
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > 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().  
> > 
> > I don't think removing those entries is a good strategy, as one might
> > want to try to use a different dynamic address if the requested one
> > is not available.  
> 
> Do you mean same 'assigned-address' attribute in DT?

Yes, or say it's another device that got the address we want and this
device doesn't want to release the address (I'm assuming the !SA case).

> 
> If so, it is checked here:
> 
> static int i3c_master_bus_init(struct i3c_master_controller *master)
> ...
> 	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> 		struct i3c_device_info info = {
> 			.static_addr = i3cboardinfo->static_addr,
> 		};
> 
> 		if (i3cboardinfo->init_dyn_addr) {
> 			status = i3c_bus_get_addr_slot_status(&master->bus,
> 			^
> 						i3cboardinfo->init_dyn_addr);
> 			if (status != I3C_ADDR_SLOT_FREE) {
> 				ret = -EBUSY;
> 				goto err_detach_devs;
> 			}
> 		}
> 
> 		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> 		if (IS_ERR(i3cdev)) {
> 			ret = PTR_ERR(i3cdev);
> 			goto err_detach_devs;
> 		}
> 
> 		i3cdev->boardinfo = i3cboardinfo;
> 
> 		ret = i3c_master_attach_i3c_dev(master, i3cdev);
> 		if (ret) {
> 			i3c_master_free_i3c_dev(i3cdev);
> 			goto err_detach_devs;
> 		}
> 	}
> ...
> 
> and later if it fails i3c_master_pre_assign_dyn_addr(), the device can 
> participate in Enter Dynamic Address Assignment process.
> I may need to check the boardinfo->init_dyn_addr status on 
> i3c_master_add_i3c_dev_locked before i3c_master_setnewda_locked().

I need to double check but I thought we were already handling that case
properly.

> 
> > Why not simply skipping entries that have ->dyn_addr
> > set to 0 when preparing a DEFSLVS frame  
> 
> I considered that solution too but if the device isn't enumerated why 
> should it be attached and kept in memory?

Might be a device that supports HJ, and in that case we might want the
controller to reserve a slot in its device table for that device.
Anyway, it doesn't hurt to have it around as long as we don't pass the
device through DEFSLVS if it doesn't have a dynamic address. I really
prefer to keep the logic unchanged and fix it if it needs to be fixed.

> Anyway we have i3c_boardinfo 
> with DT information.
> 
> >   
> > > 
> > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > ---
> > >  drivers/i3c/master.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index 5f4bd52..4d29e1f 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -1438,7 +1438,7 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > >  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> > >  					dev->boardinfo->init_dyn_addr);
> > >  	if (ret)
> > > -		return;
> > > +		goto err_detach_dev;
> > >  
> > >  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> > >  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> > > @@ -1453,6 +1453,10 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > >  
> > >  err_rstdaa:
> > >  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> > > +
> > > +err_detach_dev:
> > > +	i3c_master_detach_i3c_dev(dev);
> > > +	i3c_master_free_i3c_dev(dev);  
> > 
> > We certainly shouldn't detach/free the i3c_dev_desc from here. If it
> > has to be done somewhere (which I'd like to avoid), it should be done
> > in i3c_master_bus_init() (i3c_master_pre_assign_dyn_addr() should be
> > converted to return an int in that case).  
> 
> I can change it to return an error. 
> 
> >   
> > >  }
> > >  
> > >  static void
> > > @@ -1647,7 +1651,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,7 +1750,8 @@ 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)
> > > +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> > > +				 common.node)
> > >  		i3c_master_pre_assign_dyn_addr(i3cdev);
> > >  
> > >  	ret = i3c_master_do_daa(master);  
> 
> Thank for your feedback.
> 
> Best regards,
> Vitor Soares


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 14:00     ` Vitor Soares
@ 2019-08-29 14:39       ` Boris Brezillon
  2019-08-29 14:39         ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 14:39 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 14:00:44 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Aug 29, 2019 at 11:44:57
> 
> > On Thu, 29 Aug 2019 12:19:33 +0200
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > The I3C devices described in DT might not be attached to the master which
> > > doesn't allow to assign a specific dynamic address.  
> > 
> > I remember testing this when developing the framework, so, unless
> > another patch regressed it, it should already work. I suspect patch 1
> > is actually the regressing this use case.  
> 
> For today it doesn't address the case where the device is described with 
> static address = 0, which isn't attached to the controller.

Hm, I'm pretty sure I had designed the code to support that case (see
[1]). It might be buggy, but nothing we can't fix I guess.


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 14:39       ` Boris Brezillon
@ 2019-08-29 14:39         ` Boris Brezillon
  2019-08-29 15:07           ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 14:39 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 16:39:18 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 29 Aug 2019 14:00:44 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > Hi Boris,
> > 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Thu, Aug 29, 2019 at 11:44:57
> >   
> > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >     
> > > > The I3C devices described in DT might not be attached to the master which
> > > > doesn't allow to assign a specific dynamic address.    
> > > 
> > > I remember testing this when developing the framework, so, unless
> > > another patch regressed it, it should already work. I suspect patch 1
> > > is actually the regressing this use case.    
> > 
> > For today it doesn't address the case where the device is described with 
> > static address = 0, which isn't attached to the controller.  
> 
> Hm, I'm pretty sure I had designed the code to support that case (see
> [1]). It might be buggy, but nothing we can't fix I guess.
> 

[1]https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/i3c/master.c#L1898

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 14:39         ` Boris Brezillon
@ 2019-08-29 15:07           ` Vitor Soares
  2019-08-29 15:24             ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 15:07 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 15:39:41

> On Thu, 29 Aug 2019 16:39:18 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Thu, 29 Aug 2019 14:00:44 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > 
> > > Hi Boris,
> > > 
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Thu, Aug 29, 2019 at 11:44:57
> > >   
> > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >     
> > > > > The I3C devices described in DT might not be attached to the master which
> > > > > doesn't allow to assign a specific dynamic address.    
> > > > 
> > > > I remember testing this when developing the framework, so, unless
> > > > another patch regressed it, it should already work. I suspect patch 1
> > > > is actually the regressing this use case.    
> > > 
> > > For today it doesn't address the case where the device is described with 
> > > static address = 0, which isn't attached to the controller.  
> > 
> > Hm, I'm pretty sure I had designed the code to support that case (see
> > [1]). It might be buggy, but nothing we can't fix I guess.
> > 
> 
> [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e= 

That is only valid if you have olddev which will only exist if static 
address != 0.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 14:35       ` Boris Brezillon
@ 2019-08-29 15:23         ` Vitor Soares
  2019-08-29 15:37           ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 15:23 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	Przemyslaw Gaj, robh+dt, linux-i3c

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 15:35:20

> On Thu, 29 Aug 2019 13:53:24 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > Hi Boris,
> > 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Thu, Aug 29, 2019 at 11:41:15
> > 
> > > +Przemek
> > > 
> > > Please try to Cc active I3C contributors so they get a chance to
> > > comment on your patches.  
> > 
> > I can do that next time.
> > 
> > > 
> > > On Thu, 29 Aug 2019 12:19:32 +0200
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > 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().  
> > > 
> > > I don't think removing those entries is a good strategy, as one might
> > > want to try to use a different dynamic address if the requested one
> > > is not available.  
> > 
> > Do you mean same 'assigned-address' attribute in DT?
> 
> Yes, or say it's another device that got the address we want and this
> device doesn't want to release the address (I'm assuming the !SA case).
> 
> > 
> > If so, it is checked here:
> > 
> > static int i3c_master_bus_init(struct i3c_master_controller *master)
> > ...
> > 	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> > 		struct i3c_device_info info = {
> > 			.static_addr = i3cboardinfo->static_addr,
> > 		};
> > 
> > 		if (i3cboardinfo->init_dyn_addr) {
> > 			status = i3c_bus_get_addr_slot_status(&master->bus,
> > 			^
> > 						i3cboardinfo->init_dyn_addr);
> > 			if (status != I3C_ADDR_SLOT_FREE) {
> > 				ret = -EBUSY;
> > 				goto err_detach_devs;
> > 			}
> > 		}
> > 
> > 		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> > 		if (IS_ERR(i3cdev)) {
> > 			ret = PTR_ERR(i3cdev);
> > 			goto err_detach_devs;
> > 		}
> > 
> > 		i3cdev->boardinfo = i3cboardinfo;
> > 
> > 		ret = i3c_master_attach_i3c_dev(master, i3cdev);
> > 		if (ret) {
> > 			i3c_master_free_i3c_dev(i3cdev);
> > 			goto err_detach_devs;
> > 		}
> > 	}
> > ...
> > 
> > and later if it fails i3c_master_pre_assign_dyn_addr(), the device can 
> > participate in Enter Dynamic Address Assignment process.
> > I may need to check the boardinfo->init_dyn_addr status on 
> > i3c_master_add_i3c_dev_locked before i3c_master_setnewda_locked().
> 
> I need to double check but I thought we were already handling that case
> properly.

Yes, it is handled in the code above.

> 
> > 
> > > Why not simply skipping entries that have ->dyn_addr
> > > set to 0 when preparing a DEFSLVS frame  
> > 
> > I considered that solution too but if the device isn't enumerated why 
> > should it be attached and kept in memory?
> 
> Might be a device that supports HJ, and in that case we might want the
> controller to reserve a slot in its device table for that device.
> Anyway, it doesn't hurt to have it around as long as we don't pass the
> device through DEFSLVS if it doesn't have a dynamic address. I really
> prefer to keep the logic unchanged and fix it if it needs to be fixed.

Well, we aren't reserving a slot because we need another one to attach 
the device when it is enumerated and hence a device may be using 2 slots 
in the controller.
It may cause problems in HC with reduced slots and it is another reason 
why I think we should detach device without dynamic address after the 
enumeration phase.

> 
> > Anyway we have i3c_boardinfo 
> > with DT information.
> > 
> > >   
> > > > 
> > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > > ---
> > > >  drivers/i3c/master.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > index 5f4bd52..4d29e1f 100644
> > > > --- a/drivers/i3c/master.c
> > > > +++ b/drivers/i3c/master.c
> > > > @@ -1438,7 +1438,7 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > > >  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> > > >  					dev->boardinfo->init_dyn_addr);
> > > >  	if (ret)
> > > > -		return;
> > > > +		goto err_detach_dev;
> > > >  
> > > >  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> > > >  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> > > > @@ -1453,6 +1453,10 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > > >  
> > > >  err_rstdaa:
> > > >  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> > > > +
> > > > +err_detach_dev:
> > > > +	i3c_master_detach_i3c_dev(dev);
> > > > +	i3c_master_free_i3c_dev(dev);  
> > > 
> > > We certainly shouldn't detach/free the i3c_dev_desc from here. If it
> > > has to be done somewhere (which I'd like to avoid), it should be done
> > > in i3c_master_bus_init() (i3c_master_pre_assign_dyn_addr() should be
> > > converted to return an int in that case).  
> > 
> > I can change it to return an error. 
> > 
> > >   
> > > >  }
> > > >  
> > > >  static void
> > > > @@ -1647,7 +1651,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,7 +1750,8 @@ 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)
> > > > +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> > > > +				 common.node)
> > > >  		i3c_master_pre_assign_dyn_addr(i3cdev);
> > > >  
> > > >  	ret = i3c_master_do_daa(master);  
> > 
> > Thank for your feedback.
> > 
> > Best regards,
> > Vitor Soares



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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 15:07           ` Vitor Soares
@ 2019-08-29 15:24             ` Boris Brezillon
  2019-08-29 15:57               ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 15:24 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 15:07:08 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Aug 29, 2019 at 15:39:41
> 
> > On Thu, 29 Aug 2019 16:39:18 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > >     
> > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > >       
> > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > doesn't allow to assign a specific dynamic address.      
> > > > > 
> > > > > I remember testing this when developing the framework, so, unless
> > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > is actually the regressing this use case.      
> > > > 
> > > > For today it doesn't address the case where the device is described with 
> > > > static address = 0, which isn't attached to the controller.    
> > > 
> > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > [1]). It might be buggy, but nothing we can't fix I guess.
> > >   
> > 
> > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=   
> 
> That is only valid if you have olddev which will only exist if static 
> address != 0.

Hm, if you revert patch 1 (and assuming the device is properly defined
in the DT), you should have olddev != NULL when reaching that point. If
that's not the case there's a bug somewhere that should be fixed.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 15:23         ` Vitor Soares
@ 2019-08-29 15:37           ` Boris Brezillon
  2019-08-29 16:23             ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 15:37 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	Przemyslaw Gaj, robh+dt, linux-i3c

On Thu, 29 Aug 2019 15:23:30 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Aug 29, 2019 at 15:35:20
> 
> > On Thu, 29 Aug 2019 13:53:24 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > Hi Boris,
> > > 
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Thu, Aug 29, 2019 at 11:41:15
> > >   
> > > > +Przemek
> > > > 
> > > > Please try to Cc active I3C contributors so they get a chance to
> > > > comment on your patches.    
> > > 
> > > I can do that next time.
> > >   
> > > > 
> > > > On Thu, 29 Aug 2019 12:19:32 +0200
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >     
> > > > > 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().    
> > > > 
> > > > I don't think removing those entries is a good strategy, as one might
> > > > want to try to use a different dynamic address if the requested one
> > > > is not available.    
> > > 
> > > Do you mean same 'assigned-address' attribute in DT?  
> > 
> > Yes, or say it's another device that got the address we want and this
> > device doesn't want to release the address (I'm assuming the !SA case).
> >   
> > > 
> > > If so, it is checked here:
> > > 
> > > static int i3c_master_bus_init(struct i3c_master_controller *master)
> > > ...
> > > 	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> > > 		struct i3c_device_info info = {
> > > 			.static_addr = i3cboardinfo->static_addr,
> > > 		};
> > > 
> > > 		if (i3cboardinfo->init_dyn_addr) {
> > > 			status = i3c_bus_get_addr_slot_status(&master->bus,
> > > 			^
> > > 						i3cboardinfo->init_dyn_addr);
> > > 			if (status != I3C_ADDR_SLOT_FREE) {
> > > 				ret = -EBUSY;
> > > 				goto err_detach_devs;
> > > 			}
> > > 		}
> > > 
> > > 		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> > > 		if (IS_ERR(i3cdev)) {
> > > 			ret = PTR_ERR(i3cdev);
> > > 			goto err_detach_devs;
> > > 		}
> > > 
> > > 		i3cdev->boardinfo = i3cboardinfo;
> > > 
> > > 		ret = i3c_master_attach_i3c_dev(master, i3cdev);
> > > 		if (ret) {
> > > 			i3c_master_free_i3c_dev(i3cdev);
> > > 			goto err_detach_devs;
> > > 		}
> > > 	}
> > > ...
> > > 
> > > and later if it fails i3c_master_pre_assign_dyn_addr(), the device can 
> > > participate in Enter Dynamic Address Assignment process.
> > > I may need to check the boardinfo->init_dyn_addr status on 
> > > i3c_master_add_i3c_dev_locked before i3c_master_setnewda_locked().  
> > 
> > I need to double check but I thought we were already handling that case
> > properly.  
> 
> Yes, it is handled in the code above.

No, I meant the 'assign init_dyn_addr even if !SA', and the code I
pointed in my other reply tends to confirm that this is something we
already take into account (maybe not correctly, but the code is here).

> 
> >   
> > >   
> > > > Why not simply skipping entries that have ->dyn_addr
> > > > set to 0 when preparing a DEFSLVS frame    
> > > 
> > > I considered that solution too but if the device isn't enumerated why 
> > > should it be attached and kept in memory?  
> > 
> > Might be a device that supports HJ, and in that case we might want the
> > controller to reserve a slot in its device table for that device.
> > Anyway, it doesn't hurt to have it around as long as we don't pass the
> > device through DEFSLVS if it doesn't have a dynamic address. I really
> > prefer to keep the logic unchanged and fix it if it needs to be fixed.  
> 
> Well, we aren't reserving a slot because we need another one to attach 
> the device when it is enumerated and hence a device may be using 2 slots 
> in the controller.

Right, you shouldn't reserve a slot when ->static_address == 0 &&
->dynamic_address == 0, but I still don't see where the problem is with
the solution we have right now, sorry. Note that even if you reserve a
slot in that case, the device only occupies 2 slots for a short amount
of time, because the add_i3c_dev() logic will detect that the descriptor
already exists and reattach the device with its new address.

> It may cause problems in HC with reduced slots and it is another reason 
> why I think we should detach device without dynamic address after the 
> enumeration phase.

Can you please try the approach I suggest? => fix the existing logic to
make it work without this "free undiscovered dev desc, reallocate later"
dance.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 15:24             ` Boris Brezillon
@ 2019-08-29 15:57               ` Vitor Soares
  2019-08-29 16:15                 ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 15:57 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c



-----Original Message-----
From: Boris Brezillon 
<boris.brezillon@collabora.com> 
Sent: Thursday, August 29, 2019 4:25 
PM
To: Vitor Soares <Vitor.Soares@synopsys.com>
Cc: 
linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; 
linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; 
mark.rutland@arm.com; Joao.Pinto@synopsys.com
Subject: Re: [PATCH 2/4] 
i3c: master: Check if devices have i3c_dev_boardinfo on 
i3c_master_add_i3c_dev_locked()

On Thu, 29 Aug 2019 15:07:08 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon 
<boris.brezillon@collabora.com>
> Date: Thu, Aug 29, 2019 at 15:39:41
> 

> > On Thu, 29 Aug 2019 16:39:18 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > >     
> > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > >       
> > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > doesn't allow to assign a specific dynamic address.      
> > > > > 
> > > > > I remember testing this when developing the framework, so, unless
> > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > is actually the regressing this use case.      
> > > > 
> > > > For today it doesn't address the case where the device is described with 
> > > > static address = 0, which isn't attached to the controller.    
> > > 
> > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > [1]). It might be buggy, but nothing we can't fix I guess.
> > >   
> > 
> > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=   
> 
> That is only valid if you have olddev which will only exist if static 
> address != 0.

Hm, if you revert patch 1 (and assuming the device is properly defined
in the DT), you should have olddev != NULL when reaching that point. If
that's not the case there's a bug somewhere that should be fixed.

No, because the device is not attached.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 15:57               ` Vitor Soares
@ 2019-08-29 16:15                 ` Boris Brezillon
  2019-08-29 16:33                   ` Vitor Soares
  0 siblings, 1 reply; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 16:15 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 15:57:32 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> -----Original Message-----
> From: Boris Brezillon 
> <boris.brezillon@collabora.com> 
> Sent: Thursday, August 29, 2019 4:25 
> PM
> To: Vitor Soares <Vitor.Soares@synopsys.com>
> Cc: 
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; 
> linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; 
> mark.rutland@arm.com; Joao.Pinto@synopsys.com
> Subject: Re: [PATCH 2/4] 
> i3c: master: Check if devices have i3c_dev_boardinfo on 
> i3c_master_add_i3c_dev_locked()
> 
> On Thu, 29 Aug 2019 15:07:08 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > From: Boris Brezillon   
> <boris.brezillon@collabora.com>
> > Date: Thu, Aug 29, 2019 at 15:39:41
> >   
> 
> > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > >     
> > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > >     
> > > > > Hi Boris,
> > > > > 
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > >       
> > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > > >         
> > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > doesn't allow to assign a specific dynamic address.        
> > > > > > 
> > > > > > I remember testing this when developing the framework, so, unless
> > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > is actually the regressing this use case.        
> > > > > 
> > > > > For today it doesn't address the case where the device is described with 
> > > > > static address = 0, which isn't attached to the controller.      
> > > > 
> > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > >     
> > > 
> > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=     
> > 
> > That is only valid if you have olddev which will only exist if static 
> > address != 0.  
> 
> Hm, if you revert patch 1 (and assuming the device is properly defined
> in the DT), you should have olddev != NULL when reaching that point. If
> that's not the case there's a bug somewhere that should be fixed.
> 
> No, because the device is not attached.

Oh, my bad, I see what you mean now. This is definitely a bug and should
have the Fixes tags. I mean, even if we don't care about dynamic
address assignment, I3C drivers might care about the ->of_node that's
attached to the device.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
  2019-08-29 15:37           ` Boris Brezillon
@ 2019-08-29 16:23             ` Vitor Soares
  0 siblings, 0 replies; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 16:23 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	Przemyslaw Gaj, robh+dt, linux-i3c

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 16:37:09

> On Thu, 29 Aug 2019 15:23:30 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Thu, Aug 29, 2019 at 15:35:20
> > 
> > > On Thu, 29 Aug 2019 13:53:24 +0000
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Date: Thu, Aug 29, 2019 at 11:41:15
> > > >   
> > > > > +Przemek
> > > > > 
> > > > > Please try to Cc active I3C contributors so they get a chance to
> > > > > comment on your patches.    
> > > > 
> > > > I can do that next time.
> > > >   
> > > > > 
> > > > > On Thu, 29 Aug 2019 12:19:32 +0200
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > >     
> > > > > > 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().    
> > > > > 
> > > > > I don't think removing those entries is a good strategy, as one might
> > > > > want to try to use a different dynamic address if the requested one
> > > > > is not available.    
> > > > 
> > > > Do you mean same 'assigned-address' attribute in DT?  
> > > 
> > > Yes, or say it's another device that got the address we want and this
> > > device doesn't want to release the address (I'm assuming the !SA case).
> > >   
> > > > 
> > > > If so, it is checked here:
> > > > 
> > > > static int i3c_master_bus_init(struct i3c_master_controller *master)
> > > > ...
> > > > 	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> > > > 		struct i3c_device_info info = {
> > > > 			.static_addr = i3cboardinfo->static_addr,
> > > > 		};
> > > > 
> > > > 		if (i3cboardinfo->init_dyn_addr) {
> > > > 			status = i3c_bus_get_addr_slot_status(&master->bus,
> > > > 			^
> > > > 						i3cboardinfo->init_dyn_addr);
> > > > 			if (status != I3C_ADDR_SLOT_FREE) {
> > > > 				ret = -EBUSY;
> > > > 				goto err_detach_devs;
> > > > 			}
> > > > 		}
> > > > 
> > > > 		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> > > > 		if (IS_ERR(i3cdev)) {
> > > > 			ret = PTR_ERR(i3cdev);
> > > > 			goto err_detach_devs;
> > > > 		}
> > > > 
> > > > 		i3cdev->boardinfo = i3cboardinfo;
> > > > 
> > > > 		ret = i3c_master_attach_i3c_dev(master, i3cdev);
> > > > 		if (ret) {
> > > > 			i3c_master_free_i3c_dev(i3cdev);
> > > > 			goto err_detach_devs;
> > > > 		}
> > > > 	}
> > > > ...
> > > > 
> > > > and later if it fails i3c_master_pre_assign_dyn_addr(), the device can 
> > > > participate in Enter Dynamic Address Assignment process.
> > > > I may need to check the boardinfo->init_dyn_addr status on 
> > > > i3c_master_add_i3c_dev_locked before i3c_master_setnewda_locked().  
> > > 
> > > I need to double check but I thought we were already handling that case
> > > properly.  
> > 
> > Yes, it is handled in the code above.
> 
> No, I meant the 'assign init_dyn_addr even if !SA', and the code I
> pointed in my other reply tends to confirm that this is something we
> already take into account (maybe not correctly, but the code is here).

Please check my last comment in patch 2/4.

> 
> > 
> > >   
> > > >   
> > > > > Why not simply skipping entries that have ->dyn_addr
> > > > > set to 0 when preparing a DEFSLVS frame    
> > > > 
> > > > I considered that solution too but if the device isn't enumerated why 
> > > > should it be attached and kept in memory?  
> > > 
> > > Might be a device that supports HJ, and in that case we might want the
> > > controller to reserve a slot in its device table for that device.
> > > Anyway, it doesn't hurt to have it around as long as we don't pass the
> > > device through DEFSLVS if it doesn't have a dynamic address. I really
> > > prefer to keep the logic unchanged and fix it if it needs to be fixed.  
> > 
> > Well, we aren't reserving a slot because we need another one to attach 
> > the device when it is enumerated and hence a device may be using 2 slots 
> > in the controller.
> 
> Right, you shouldn't reserve a slot when ->static_address == 0 &&
> ->dynamic_address == 0, but I still don't see where the problem is with
> the solution we have right now, sorry. Note that even if you reserve a
> slot in that case, the device only occupies 2 slots for a short amount
> of time, because the add_i3c_dev() logic will detect that the descriptor
> already exists and reattach the device with its new address.

I understand that but if we have limited resources it is a problem.

> 
> > It may cause problems in HC with reduced slots and it is another reason 
> > why I think we should detach device without dynamic address after the 
> > enumeration phase.
> 
> Can you please try the approach I suggest? => fix the existing logic to
> make it work without this "free undiscovered dev desc, reallocate later"
> dance.

I can do that but we still have the issue above to solve and figure how 
to set a dynamic address for devices without static address.







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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 10:19 ` [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked() Vitor Soares
  2019-08-29 10:44   ` Boris Brezillon
@ 2019-08-29 16:27   ` Boris Brezillon
  1 sibling, 0 replies; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 16:27 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 12:19:33 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> The I3C devices described in DT might not be attached to the master which
> doesn't allow to assign a specific dynamic address.

Dynamic address assignment is not the only problem here (see my
comment about missing ->of_node info). I would simply say that
newdev->boardinfo assignment was missing in
i3c_master_add_i3c_dev_locked() which is already a bug by itself.

I would also change the subject to

"i3c: master: Make sure ->boardinfo is initialized in add_i3c_dev_locked()"

> 
> This patch check if a device has i3c_dev_boardinfo and add it to
> i3c_dev_desc structure. In this conditions, the framework will try to
> assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.

I would get rid of this explanation and add Fixes/Cc-stable tags.

> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/master.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4d29e1f..85fbda6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +static struct i3c_dev_boardinfo *
> +i3c_master_search_i3c_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 NULL;
> +
> +	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> +		if (dev->info.pid == boardinfo->pid)
> +			return boardinfo;
> +	}
> +
> +	return NULL;
> +}

Can we instead have:

static void i3c_master_init_i3c_dev_boardinfo(struct i3c_dev_desc *dev)
{
	struct i3c_master_controller *master = i3c_dev_get_master(dev);

	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
		if (dev->info.pid == boardinfo->pid)
			dev->boardinfo = boardinfo;
	}
}

> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  {
>  	struct i3c_device_info info = { .dyn_addr = addr };
>  	struct i3c_dev_desc *newdev, *olddev;
> +	struct i3c_dev_boardinfo *boardinfo;
>  	u8 old_dyn_addr = addr, expected_dyn_addr;
>  	struct i3c_ibi_setup ibireq = { };
>  	bool enable_ibi = false;
> @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_detach_dev;
>  
> +	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> +	if (boardinfo)
> +		newdev->boardinfo = boardinfo;
> +

And here:

	i3c_master_init_i3c_dev_boardinfo(newdev);

>  	/*
>  	 * Depending on our previous state, the expected dynamic address might
>  	 * differ:


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 16:15                 ` Boris Brezillon
@ 2019-08-29 16:33                   ` Vitor Soares
  2019-08-29 16:37                     ` Boris Brezillon
  0 siblings, 1 reply; 26+ messages in thread
From: Vitor Soares @ 2019-08-29 16:33 UTC (permalink / raw)
  To: Boris Brezillon, Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Thu, Aug 29, 2019 at 17:15:20

> On Thu, 29 Aug 2019 15:57:32 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > -----Original Message-----
> > From: Boris Brezillon 
> > <boris.brezillon@collabora.com> 
> > Sent: Thursday, August 29, 2019 4:25 
> > PM
> > To: Vitor Soares <Vitor.Soares@synopsys.com>
> > Cc: 
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; 
> > linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; 
> > mark.rutland@arm.com; Joao.Pinto@synopsys.com
> > Subject: Re: [PATCH 2/4] 
> > i3c: master: Check if devices have i3c_dev_boardinfo on 
> > i3c_master_add_i3c_dev_locked()
> > 
> > On Thu, 29 Aug 2019 15:07:08 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > 
> > > From: Boris Brezillon   
> > <boris.brezillon@collabora.com>
> > > Date: Thu, Aug 29, 2019 at 15:39:41
> > >   
> > 
> > > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > > >     
> > > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > >     
> > > > > > Hi Boris,
> > > > > > 
> > > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > > >       
> > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > > > >         
> > > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > > doesn't allow to assign a specific dynamic address.        
> > > > > > > 
> > > > > > > I remember testing this when developing the framework, so, unless
> > > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > > is actually the regressing this use case.        
> > > > > > 
> > > > > > For today it doesn't address the case where the device is described with 
> > > > > > static address = 0, which isn't attached to the controller.      
> > > > > 
> > > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > > >     
> > > > 
> > > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=     
> > > 
> > > That is only valid if you have olddev which will only exist if static 
> > > address != 0.  
> > 
> > Hm, if you revert patch 1 (and assuming the device is properly defined
> > in the DT), you should have olddev != NULL when reaching that point. If
> > that's not the case there's a bug somewhere that should be fixed.
> > 
> > No, because the device is not attached.
> 
> Oh, my bad, I see what you mean now. This is definitely a bug and should
> have the Fixes tags. I mean, even if we don't care about dynamic
> address assignment, I3C drivers might care about the ->of_node that's
> attached to the device.

I didn't consider a bug because in dt-bindings says to not use 'assigned 
address' if SA = 0.
Do you think there is a better way to solve it?

Best regards,
Vitor Soares

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
  2019-08-29 16:33                   ` Vitor Soares
@ 2019-08-29 16:37                     ` Boris Brezillon
  0 siblings, 0 replies; 26+ messages in thread
From: Boris Brezillon @ 2019-08-29 16:37 UTC (permalink / raw)
  To: Vitor Soares
  Cc: mark.rutland, devicetree, Joao.Pinto, bbrezillon, linux-kernel,
	robh+dt, linux-i3c

On Thu, 29 Aug 2019 16:33:43 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Thu, Aug 29, 2019 at 17:15:20
> 
> > On Thu, 29 Aug 2019 15:57:32 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > -----Original Message-----
> > > From: Boris Brezillon 
> > > <boris.brezillon@collabora.com> 
> > > Sent: Thursday, August 29, 2019 4:25 
> > > PM
> > > To: Vitor Soares <Vitor.Soares@synopsys.com>
> > > Cc: 
> > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; 
> > > linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; 
> > > mark.rutland@arm.com; Joao.Pinto@synopsys.com
> > > Subject: Re: [PATCH 2/4] 
> > > i3c: master: Check if devices have i3c_dev_boardinfo on 
> > > i3c_master_add_i3c_dev_locked()
> > > 
> > > On Thu, 29 Aug 2019 15:07:08 +0000
> > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > >   
> > > > From: Boris Brezillon     
> > > <boris.brezillon@collabora.com>  
> > > > Date: Thu, Aug 29, 2019 at 15:39:41
> > > >     
> > >   
> > > > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > > > >       
> > > > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > > >       
> > > > > > > Hi Boris,
> > > > > > > 
> > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > > > >         
> > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > > > > >           
> > > > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > > > doesn't allow to assign a specific dynamic address.          
> > > > > > > > 
> > > > > > > > I remember testing this when developing the framework, so, unless
> > > > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > > > is actually the regressing this use case.          
> > > > > > > 
> > > > > > > For today it doesn't address the case where the device is described with 
> > > > > > > static address = 0, which isn't attached to the controller.        
> > > > > > 
> > > > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > > > >       
> > > > > 
> > > > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=       
> > > > 
> > > > That is only valid if you have olddev which will only exist if static 
> > > > address != 0.    
> > > 
> > > Hm, if you revert patch 1 (and assuming the device is properly defined
> > > in the DT), you should have olddev != NULL when reaching that point. If
> > > that's not the case there's a bug somewhere that should be fixed.
> > > 
> > > No, because the device is not attached.  
> > 
> > Oh, my bad, I see what you mean now. This is definitely a bug and should
> > have the Fixes tags. I mean, even if we don't care about dynamic
> > address assignment, I3C drivers might care about the ->of_node that's
> > attached to the device.  
> 
> I didn't consider a bug because in dt-bindings says to not use 'assigned 
> address' if SA = 0.

The fact that we don't try to assign a predefined dynamic address
when !SA is indeed not a bug, but failing to propagate the ->of_node
info to the i3c_device is one.

> Do you think there is a better way to solve it?

Your patch is correct, I just proposed a few adjustments.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-29 10:19 [PATCH 0/4] i3c: remove device if failed on pre_assign_dyn_addr() Vitor Soares
2019-08-29 10:19 ` [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails Vitor Soares
2019-08-29 10:41   ` Boris Brezillon
2019-08-29 13:53     ` Vitor Soares
2019-08-29 14:35       ` Boris Brezillon
2019-08-29 15:23         ` Vitor Soares
2019-08-29 15:37           ` Boris Brezillon
2019-08-29 16:23             ` Vitor Soares
2019-08-29 10:19 ` [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked() Vitor Soares
2019-08-29 10:44   ` Boris Brezillon
2019-08-29 14:00     ` Vitor Soares
2019-08-29 14:39       ` Boris Brezillon
2019-08-29 14:39         ` Boris Brezillon
2019-08-29 15:07           ` Vitor Soares
2019-08-29 15:24             ` Boris Brezillon
2019-08-29 15:57               ` Vitor Soares
2019-08-29 16:15                 ` Boris Brezillon
2019-08-29 16:33                   ` Vitor Soares
2019-08-29 16:37                     ` Boris Brezillon
2019-08-29 16:27   ` Boris Brezillon
2019-08-29 10:19 ` [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0 Vitor Soares
2019-08-29 10:51   ` Boris Brezillon
2019-08-29 14:06     ` Vitor Soares
2019-08-29 10:19 ` [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table Vitor Soares
2019-08-29 11:15   ` Boris Brezillon
2019-08-29 14:09     ` Vitor Soares

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org linux-i3c@archiver.kernel.org
	public-inbox-index linux-i3c


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/ public-inbox