As for today, the I3C framework is keeping in memory and master->bus.devs
list the devices that fail during pre_assign_dyn_addr() and send them on
DEFSLVS command.
According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
Secondary Master about the Dynamic Addresses that were assigned to I3C
devices and the I2C devices present on the bus.
This issue could be fixed by changing i3c_master_defslvs_locked() to
ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
attached to HC unnecessarily. This can cause that some HC aren't able to
do DAA for HJ capable devices due of lack of space.
This patch-series propose to detach/free devices that are failing during
pre_assign_dyn_addr() and to propagate i3c_boardinfo, if available, to
i3c_dev_desc during i3c_master_add_i3c_dev_locked(). Besides the fix for
the problem mention above, this change will permit to describe devices
with a preferable dynamic address (important due to priority reason) but
without a static address in DT.
In addition, I'm improving the management of the Data Address Table in
DW I3C Master by keeping the free slots consecutive.
Change in v3:
- Change cover letter
- Change commit message for patch 1
- Add Rob rb-tags
Change in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Change i3c_master_search_i3c_boardinfo(newdev) to
i3c_master_init_i3c_dev_boardinfo(newdev)
- Add fixes, stable tags on patch 2
- Add a note for no guarantee of 'assigned-address' use
Vitor Soares (5):
i3c: master: detach and free device if pre_assign_dyn_addr() fails
i3c: master: make sure ->boardinfo is initialized in
add_i3c_dev_locked()
dt-bindings: i3c: Make 'assigned-address' valid if static address == 0
dt-bindings: i3c: add a note for no guarantee of 'assigned-address'
use
i3c: master: dw: reattach device on first available location of
address table
Documentation/devicetree/bindings/i3c/i3c.txt | 15 ++++++--
drivers/i3c/master.c | 49 ++++++++++++++++++++++-----
drivers/i3c/master/dw-i3c-master.c | 16 +++++++++
3 files changed, 68 insertions(+), 12 deletions(-)
--
2.7.4
As for today, the I3C framework is keeping in memory and master->bus.devs
list the devices that fail during pre_assign_dyn_addr() and send them on
DEFSLVS command.
According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
Secondary Master about the Dynamic Addresses that were assigned to I3C
devices and the I2C devices present on the bus.
This issue could be fixed by changing i3c_master_defslvs_locked() to
ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
attached to HC unnecessarily. This can cause that some HC aren't able to
do DAA for HJ capable devices due to lack of space in controller.
Signed-off-by: Vitor Soares <[email protected]>
---
Changes in v3:
- Change commit message
Changes in v2:
- Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
- Convert i3c_master_pre_assign_dyn_addr() to return an int
drivers/i3c/master.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52..586e34f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
master->ops->detach_i2c_dev(dev);
}
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
{
struct i3c_master_controller *master = i3c_dev_get_master(dev);
int ret;
if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
!dev->boardinfo->static_addr)
- return;
+ return 0;
ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
dev->boardinfo->init_dyn_addr);
if (ret)
- return;
+ return ret;
dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
ret = i3c_master_reattach_i3c_dev(dev, 0);
@@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
if (ret)
goto err_rstdaa;
- return;
+ return 0;
err_rstdaa:
i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+ return ret;
}
static void
@@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
enum i3c_addr_slot_status status;
struct i2c_dev_boardinfo *i2cboardinfo;
struct i3c_dev_boardinfo *i3cboardinfo;
- struct i3c_dev_desc *i3cdev;
+ struct i3c_dev_desc *i3cdev, *i3ctmp;
struct i2c_dev_desc *i2cdev;
int ret;
@@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
* Pre-assign dynamic address and retrieve device information if
* needed.
*/
- i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
- i3c_master_pre_assign_dyn_addr(i3cdev);
+ list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+ common.node) {
+ ret = i3c_master_pre_assign_dyn_addr(i3cdev);
+ if (ret) {
+ i3c_master_detach_i3c_dev(i3cdev);
+ i3c_master_free_i3c_dev(i3cdev);
+ }
+ }
ret = i3c_master_do_daa(master);
if (ret)
--
2.7.4
For today the reattach function only update the device address on the
controller.
Update the location to the first available too, will optimize the
enumeration process avoiding additional checks to keep the available
positions on address table consecutive.
Signed-off-by: Vitor Soares <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
---
Change in v3:
- None
Change in v2:
- Add Boris rb-tag
drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 1d83c97..62261ac 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -898,6 +898,22 @@ static int dw_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
struct i3c_master_controller *m = i3c_dev_get_master(dev);
struct dw_i3c_master *master = to_dw_i3c_master(m);
+ int pos;
+
+ pos = dw_i3c_master_get_free_pos(master);
+
+ if (data->index > pos && pos > 0) {
+ writel(0,
+ master->regs +
+ DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+
+ master->addrs[data->index] = 0;
+ master->free_pos |= BIT(data->index);
+
+ data->index = pos;
+ master->addrs[pos] = dev->info.dyn_addr;
+ master->free_pos &= ~BIT(pos);
+ }
writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(dev->info.dyn_addr),
master->regs +
--
2.7.4
The I3C devices without a static address can require a specific dynamic
address for priority reasons.
Let's update the binding document to make the 'assigned-address' property
valid if static address == 0 and add an example with this use case.
Signed-off-by: Vitor Soares <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Change in v3:
- Add Rob rb-tag
Change in v2:
- Fix typo in commit message
Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/i3c/i3c.txt b/Documentation/devicetree/bindings/i3c/i3c.txt
index ab729a0..c851e75 100644
--- a/Documentation/devicetree/bindings/i3c/i3c.txt
+++ b/Documentation/devicetree/bindings/i3c/i3c.txt
@@ -98,9 +98,7 @@ Required properties
Optional properties
-------------------
-- assigned-address: dynamic address to be assigned to this device. This
- property is only valid if the I3C device has a static
- address (first cell of the reg property != 0).
+- assigned-address: dynamic address to be assigned to this device.
Example:
@@ -129,6 +127,15 @@ Example:
/*
* I3C device without a static I2C address but requiring
+ * specific dynamic address.
+ */
+ sensor@0,39200154004 {
+ reg = <0x0 0x6072 0x303904d2>;
+ assigned-address = <0xb>;
+ };
+
+ /*
+ * I3C device without a static I2C address but requiring
* resources described in the DT.
*/
sensor@0,39200154004 {
--
2.7.4
Hi Boris,
From: Vitor Soares <[email protected]>
Date: Thu, Sep 05, 2019 at 11:00:34
> As for today, the I3C framework is keeping in memory and master->bus.devs
> list the devices that fail during pre_assign_dyn_addr() and send them on
> DEFSLVS command.
>
> According to MIPI I3C Bus spec the DEFSLVS command is used to inform any
> Secondary Master about the Dynamic Addresses that were assigned to I3C
> devices and the I2C devices present on the bus.
>
> This issue could be fixed by changing i3c_master_defslvs_locked() to
> ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and
> attached to HC unnecessarily. This can cause that some HC aren't able to
> do DAA for HJ capable devices due to lack of space in controller.
>
> Signed-off-by: Vitor Soares <[email protected]>
> ---
> Changes in v3:
> - Change commit message
>
> Changes in v2:
> - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
> - Convert i3c_master_pre_assign_dyn_addr() to return an int
>
> drivers/i3c/master.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52..586e34f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
> master->ops->detach_i2c_dev(dev);
> }
>
> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> +static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> {
> struct i3c_master_controller *master = i3c_dev_get_master(dev);
> int ret;
>
> if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
> !dev->boardinfo->static_addr)
> - return;
> + return 0;
>
> ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> dev->boardinfo->init_dyn_addr);
> if (ret)
> - return;
> + return ret;
>
> dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> ret = i3c_master_reattach_i3c_dev(dev, 0);
> @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> if (ret)
> goto err_rstdaa;
>
> - return;
> + return 0;
>
> err_rstdaa:
> i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> +
> + return ret;
> }
>
> static void
> @@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> enum i3c_addr_slot_status status;
> struct i2c_dev_boardinfo *i2cboardinfo;
> struct i3c_dev_boardinfo *i3cboardinfo;
> - struct i3c_dev_desc *i3cdev;
> + struct i3c_dev_desc *i3cdev, *i3ctmp;
> struct i2c_dev_desc *i2cdev;
> int ret;
>
> @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> * Pre-assign dynamic address and retrieve device information if
> * needed.
> */
> - i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
> - i3c_master_pre_assign_dyn_addr(i3cdev);
> + list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> + common.node) {
> + ret = i3c_master_pre_assign_dyn_addr(i3cdev);
> + if (ret) {
> + i3c_master_detach_i3c_dev(i3cdev);
> + i3c_master_free_i3c_dev(i3cdev);
> + }
> + }
>
> ret = i3c_master_do_daa(master);
> if (ret)
> --
> 2.7.4
I think I clearly explain why the current solution is problematic and my
choices for the proposal solution.
Please let me know if there is anything else that I can improve in this
patch.
Best regards,
Vitor Soares