i3c: remove device if failed on pre_assign_dyn_addr()
Subsystems (3)
cover i3c: remove device if failed on pre_assign_dyn_addr()
This patch series remove the devices that fail during pre_assign_dyn_addr() and were being sent on DEFSVLS command. With the change above, during the i3c_master_add_i3c_dev_locked() is necessary to check if the device has i3c_boardinfo and try to assign the i3c_dev_boardinfo->init_dyn_addr if there no oldev. This change will allow to describe in DT device with preferable dynamic address but without static address.
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(-)
Signature · 1 line
2.7.4
PATCH 1/4 i3c: master: detach and free device if pre_assign_dyn_addr() fails
On pre_assing_dyn_addr() the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()
are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr().
--- drivers/i3c/master.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/i3c/master.c +8 -3
@@ -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);
Signature · 1 line
2.7.4
Re: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
+Przemek
Please try to Cc active I3C contributors so they get a chance to comment on your patches.
Quoted 8 lines · On Thu, 29 Aug 2019 12:19:32 +0200 Vitor Soares <[email protected]>…
On
pre_assing_dyn_addr()the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()are kept in memory and
master->bus.devslist. 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 onpre_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
Quoted 20 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/i3c/master.c +1
-1@@ -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 );
drivers/i3c/master.c static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) · line 1459
Quoted 7 lines
drivers/i3c/master.c +4
-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).
Quoted 22 lines
drivers/i3c/master.c +3
-2} 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);
RE: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:41:15
Quoted 4 lines
+Przemek
Please try to Cc active I3C contributors so they get a chance to comment on your patches.
I can do that next time.
Quoted 9 lines
Quoted 5 lines · On Thu, 29 Aug 2019 12:19:32 +0200 Vitor Soares <[email protected]>…
On
pre_assing_dyn_addr()the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()
Quoted 7 lines
Quoted 3 lines
are kept in memory and
master->bus.devslist. 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 onpre_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().
Quoted 2 lines
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.
Quoted 22 lines
Quoted 22 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/i3c/master.c +1
-1@@ -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)
Quoted 11 lines
drivers/i3c/master.c static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) · line 1459
Quoted 6 lines
drivers/i3c/master.c +4
-0err_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_descfrom here. If it has to be done somewhere (which I'd like to avoid), it should be done ini3c_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.
Quoted 22 lines
Quoted 22 lines
drivers/i3c/master.c +3
-2} 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);
Quoted 1 line
drivers/i3c/master.c static int i3c_master_bus_init(struct i3c_master_controller *master) · line 1757
Quoted 1 line
drivers/i3c/master.c +0
-0ret =i3c_master_do_daa (master);
Thank for your feedback.
Best regards,
Vitor Soares
Re: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
Quoted 25 lines · On Thu, 29 Aug 2019 13:53:24 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:41:15Quoted 4 lines
+Przemek
Please try to Cc active I3C contributors so they get a chance to comment on your patches.
I can do that next time.
Quoted 13 lines
Quoted 8 lines · On Thu, 29 Aug 2019 12:19:32 +0200 Vitor Soares <[email protected]>…
On
pre_assing_dyn_addr()the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()are kept in memory and
master->bus.devslist. 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 onpre_assign_dyn_addr().
Quoted 5 lines
Quoted 3 lines
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).
Quoted 34 lines
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; } }
Quoted 6 lines
...
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 theboardinfo->init_dyn_addrstatus oni3c_master_add_i3c_dev_lockedbeforei3c_master_setnewda_locked().
I need to double check but I thought we were already handling that case properly.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 67 lines
Anyway we have
i3c_boardinfo
with DT information.Quoted 22 lines
Quoted 22 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/i3c/master.c +1
-1@@ -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)Quoted 11 lines
drivers/i3c/master.c static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) · line 1459
Quoted 6 lines
drivers/i3c/master.c +4
-0err_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_descfrom here. If it has to be done somewhere (which I'd like to avoid), it should be done ini3c_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.
Quoted 22 lines
Quoted 22 lines
drivers/i3c/master.c +3
-2} 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);Quoted 1 line
drivers/i3c/master.c static int i3c_master_bus_init(struct i3c_master_controller *master) · line 1757
Quoted 1 line
drivers/i3c/master.c +0
-0ret =i3c_master_do_daa (master);Thank for your feedback.
Best regards,
Vitor Soares
RE: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 15:35:20
Quoted 73 lines
Quoted 25 lines · On Thu, 29 Aug 2019 13:53:24 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:41:15Quoted 4 lines
+Przemek
Please try to Cc active I3C contributors so they get a chance to comment on your patches.
I can do that next time.
Quoted 13 lines
Quoted 8 lines · On Thu, 29 Aug 2019 12:19:32 +0200 Vitor Soares <[email protected]>…
On
pre_assing_dyn_addr()the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()are kept in memory and
master->bus.devslist. 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 onpre_assign_dyn_addr().Quoted 5 lines
Quoted 3 lines
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).
Quoted 36 lines
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; } }...
Quoted 7 lines
Quoted 4 lines
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 theboardinfo->init_dyn_addrstatus oni3c_master_add_i3c_dev_lockedbeforei3c_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.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Why not simply skipping entries that have ->dyn_addr set to 0 when preparing a DEFSLVS frame
Quoted 8 lines
Quoted 2 lines
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.
Quoted 68 lines
Quoted 68 lines
Anyway we have
i3c_boardinfo
with DT information.Quoted 22 lines
Quoted 22 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/i3c/master.c +1
-1@@ -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)Quoted 11 lines
drivers/i3c/master.c static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) · line 1459
Quoted 6 lines
drivers/i3c/master.c +4
-0err_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_descfrom here. If it has to be done somewhere (which I'd like to avoid), it should be done ini3c_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.
Quoted 22 lines
Quoted 22 lines
drivers/i3c/master.c +3
-2} 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);Quoted 1 line
drivers/i3c/master.c static int i3c_master_bus_init(struct i3c_master_controller *master) · line 1757
Quoted 1 line
drivers/i3c/master.c +0
-0ret =i3c_master_do_daa (master);Thank for your feedback.
Best regards,
Vitor Soares
Re: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
Quoted 81 lines · On Thu, 29 Aug 2019 15:23:30 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 15:35:20Quoted 78 lines
Quoted 25 lines · On Thu, 29 Aug 2019 13:53:24 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:41:15Quoted 4 lines
+Przemek
Please try to Cc active I3C contributors so they get a chance to comment on your patches.
I can do that next time.
Quoted 13 lines
Quoted 8 lines · On Thu, 29 Aug 2019 12:19:32 +0200 Vitor Soares <[email protected]>…
On
pre_assing_dyn_addr()the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()are kept in memory and
master->bus.devslist. 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 onpre_assign_dyn_addr().Quoted 5 lines
Quoted 3 lines
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).
Quoted 40 lines
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 theboardinfo->init_dyn_addrstatus oni3c_master_add_i3c_dev_lockedbeforei3c_master_setnewda_locked().
Quoted 4 lines
Quoted 2 lines
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).
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Why not simply skipping entries that have ->dyn_addr set to 0 when preparing a DEFSLVS frame
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
RE: [PATCH 1/4] i3c: master: detach and free device if pre_assign_dyn_addr() fails
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 16:37:09
Quoted 84 lines
Quoted 81 lines · On Thu, 29 Aug 2019 15:23:30 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 15:35:20Quoted 78 lines
Quoted 25 lines · On Thu, 29 Aug 2019 13:53:24 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:41:15Quoted 4 lines
+Przemek
Please try to Cc active I3C contributors so they get a chance to comment on your patches.
I can do that next time.
Quoted 13 lines
Quoted 8 lines · On Thu, 29 Aug 2019 12:19:32 +0200 Vitor Soares <[email protected]>…
On
pre_assing_dyn_addr()the devices that fail:
i3c_master_setdasa_locked()
i3c_master_reattach_i3c_dev()
i3c_master_retrieve_dev_info()are kept in memory and
master->bus.devslist. 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 onpre_assign_dyn_addr().Quoted 5 lines
Quoted 3 lines
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).
Quoted 40 lines
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 theboardinfo->init_dyn_addrstatus oni3c_master_add_i3c_dev_lockedbeforei3c_master_setnewda_locked().
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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_addreven 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.
Quoted 20 lines
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Why not simply skipping entries that have ->dyn_addr set to 0 when preparing a DEFSLVS frame
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
PATCH 2/4 i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
In reply to Vitor SoaresThe 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.
--- drivers/i3c/master.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
drivers/i3c/master.c +22 -0
@@ -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:
Signature · 1 line
2.7.4
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 57 lines
This patch check if a device has
i3c_dev_boardinfoand add it toi3c_dev_descstructure. In this conditions, the framework will try to assign thei3c_dev_boardinfo->init_dyn_addreven if stactic address = 0.Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/i3c/master.c +22
-0@@ -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:
RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57
Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 51 lines
Quoted 51 lines
This patch check if a device has
i3c_dev_boardinfoand add it toi3c_dev_descstructure. In this conditions, the framework will try to assign thei3c_dev_boardinfo->init_dyn_addreven if stactic address = 0.Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/i3c/master.c +18
-0@@ -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;
Quoted 7 lines
drivers/i3c/master.c int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, · line 1902
Quoted 7 lines
drivers/i3c/master.c +4
-0+ 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
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 11 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 6 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
The I3C devices described in DT might not be attached to the master which doesn't allow to assign a specific dynamic address.
Quoted 6 lines
Quoted 3 lines
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.
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 18 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 15 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 6 lines
Quoted 2 lines
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
RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 15:39:41
Quoted 21 lines
Quoted 18 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 15 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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.
That is only valid if you have olddev which will only exist if static address != 0.
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 27 lines · On Thu, 29 Aug 2019 15:07:08 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 15:39:41Quoted 24 lines
Quoted 21 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 17 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
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.
RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
-----Original Message-----
From: Boris Brezillon
<[email protected]>
Sent: Thursday, August 29, 2019 4:25
PM
To: Vitor Soares <[email protected]>
Cc:
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/4]
i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 1 line · On Thu, 29 Aug 2019 15:07:08 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon
Quoted 27 lines
Date: Thu, Aug 29, 2019 at 15:39:41
Quoted 24 lines
Quoted 21 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 17 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
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.
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 52 lines · On Thu, 29 Aug 2019 15:57:32 +0000 Vitor Soares <[email protected]>…
-----Original Message-----
From: Boris Brezillon
<[email protected]>
Sent: Thursday, August 29, 2019 4:25
PM
To: Vitor Soares <[email protected]>
Cc:
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/4]
i3c: master: Check if devices havei3c_dev_boardinfooni3c_master_add_i3c_dev_locked()Quoted 1 line · On Thu, 29 Aug 2019 15:07:08 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon
Quoted 33 lines
Date: Thu, Aug 29, 2019 at 15:39:41
Quoted 29 lines
Quoted 18 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 15 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
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.
RE: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 17:15:20
Quoted 62 lines
Quoted 59 lines · On Thu, 29 Aug 2019 15:57:32 +0000 Vitor Soares <[email protected]>…
-----Original Message-----
From: Boris Brezillon
<[email protected]>
Sent: Thursday, August 29, 2019 4:25
PM
To: Vitor Soares <[email protected]>
Cc:
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/4]
i3c: master: Check if devices havei3c_dev_boardinfooni3c_master_add_i3c_dev_locked()Quoted 1 line · On Thu, 29 Aug 2019 15:07:08 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon
Quoted 27 lines
Date: Thu, Aug 29, 2019 at 15:39:41
Quoted 24 lines
Quoted 21 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 17 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
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.
Quoted 6 lines
Quoted 1 line
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
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
Quoted 67 lines · On Thu, 29 Aug 2019 16:33:43 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 17:15:20Quoted 64 lines
Quoted 60 lines · On Thu, 29 Aug 2019 15:57:32 +0000 Vitor Soares <[email protected]>…
-----Original Message-----
From: Boris Brezillon
<[email protected]>
Sent: Thursday, August 29, 2019 4:25
PM
To: Vitor Soares <[email protected]>
Cc:
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 2/4]
i3c: master: Check if devices havei3c_dev_boardinfooni3c_master_add_i3c_dev_locked()Quoted 1 line · On Thu, 29 Aug 2019 15:07:08 +0000 Vitor Soares <[email protected]>…
From: Boris Brezillon
Quoted 27 lines
Date: Thu, Aug 29, 2019 at 15:39:41
Quoted 24 lines
Quoted 21 lines · On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon…
Quoted 17 lines · On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <[email protected]>…
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:44:57Quoted 9 lines
Quoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 1 line
Do you think there is a better way to solve it?
Your patch is correct, I just proposed a few adjustments.
Re: [PATCH 2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()
In reply to Vitor SoaresQuoted 2 lines · On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <[email protected]>…
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()"
Quoted 4 lines
This patch check if a device has
i3c_dev_boardinfoand add it toi3c_dev_descstructure. In this conditions, the framework will try to assign thei3c_dev_boardinfo->init_dyn_addreven if stactic address = 0.
I would get rid of this explanation and add Fixes/Cc-stable tags.
Quoted 20 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/i3c/master.c +6
-0@@ -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; +
drivers/i3c/master.c i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) · line 1813
Quoted 10 lines
drivers/i3c/master.c +10
-0+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;
}
}Quoted 13 lines
drivers/i3c/master.c +2
-0+/** * 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;
drivers/i3c/master.c int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, · line 1899
Quoted 7 lines
drivers/i3c/master.c +4
-0@@ -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);
Quoted 3 lines
drivers/i3c/master.c +0
-0/* * Depending on our previous state, the expected dynamic address might * differ:
PATCH 3/4 dt-bindings: i3c: Make 'assigned-address' valid if static address != 0
In reply to Vitor SoaresThe 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.
--- Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/i3c/i3c.txt +10 -3
@@ -98,9 +98,7 @@ Required properties Optional properties ------------------- -- assigned-address: dynamic address to be assigned to this device. This - property is only valid if the I3C device has a static - address (first cell of the reg property != 0). +- assigned-address: dynamic address to be assigned to this device. Example:
@@ -129,6 +127,15 @@ Example: /* * I3C device without a static I2C address but requiring + * specific dynamic address. + */ + sensor@0,39200154004 { + reg = <0x0 0x6072 0x303904d2>; + assigned-address = <0xb>; + }; + + /* + * I3C device without a static I2C address but requiring * resources described in the DT. */ sensor@0,39200154004 {
Signature · 1 line
2.7.4
Re: [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0
Quoted 5 lines · On Thu, 29 Aug 2019 12:19:34 +0200 Vitor Soares <[email protected]>…
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.
Quoted 11 lines
Signed-off-by Vitor Soares <[email protected]>--- Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)Documentation/devicetree/bindings/i3c/i3c.txt +0
-0
Documentation/devicetree/bindings/i3c/i3c.txt Required properties · line 101
Quoted 7 lines
Documentation/devicetree/bindings/i3c/i3c.txt +1
-3@@ -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.
Quoted 19 lines
Documentation/devicetree/bindings/i3c/i3c.txt +9
-0Example:@@ -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 {
RE: [PATCH 3/4] dt-bindings: i3c: Make 'assigned-address' valid if static address != 0
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 11:51:38
Quoted 10 lines
Quoted 5 lines · On Thu, 29 Aug 2019 12:19:34 +0200 Vitor Soares <[email protected]>…
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.
Quoted 17 lines
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.
Quoted 12 lines
Signed-off-by Vitor Soares <[email protected]>--- Documentation/devicetree/bindings/i3c/i3c.txt | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)Documentation/devicetree/bindings/i3c/i3c.txt +0
-0@@ -98,9 +98,7 @@ Required properties
Quoted 12 lines
Documentation/devicetree/bindings/i3c/i3c.txt Required properties · line 101
Quoted 6 lines
Documentation/devicetree/bindings/i3c/i3c.txt +1
-3Optional 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?
Quoted 6 lines
Quoted 6 lines
Documentation/devicetree/bindings/i3c/i3c.txt +0
-0Example:@@ -129,6 +127,15 @@ Example:
Quoted 14 lines
Quoted 4 lines
Documentation/devicetree/bindings/i3c/i3c.txt +2
-0/* * I3C device without a static I2C address but requiring + * specific dynamic address. + */Documentation/devicetree/bindings/i3c/i3c.txt Example: · line 141
Quoted 10 lines
Documentation/devicetree/bindings/i3c/i3c.txt +7
-0+ 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
PATCH 4/4 i3c: master: dw: reattach device on first available location of address table
In reply to Vitor SoaresFor 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.
--- drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/i3c/master/dw-i3c-master.c +16 -0
@@ -898,6 +898,22 @@ static int dw_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,struct dw_i3c_i2c_dev_data *data =i3c_dev_get_master_data (dev);struct i3c_master_controller *m =i3c_dev_get_master (dev);struct dw_i3c_master *master =to_dw_i3c_master (m); +int pos; + + pos =dw_i3c_master_get_free_pos (master); + +if (data->index > pos && pos >0 ) { +writel (0 , + master->regs + +DEV_ADDR_TABLE_LOC (master->datstartaddr, data->index)); + + master->addrs[data->index] =0 ; + master->free_pos |=BIT (data->index); + + data->index = pos; + master->addrs[pos] = dev->info.dyn_addr; + master->free_pos &= ~BIT (pos); + } writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(dev->info.dyn_addr), master->regs +
Signature · 1 line
2.7.4
Re: [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table
Quoted 6 lines · On Thu, 29 Aug 2019 12:19:35 +0200 Vitor Soares <[email protected]>…
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
Quoted 33 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/i3c/master/dw-i3c-master.c +16
-0@@ -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 +
RE: [PATCH 4/4] i3c: master: dw: reattach device on first available location of address table
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Thu, Aug 29, 2019 at 12:15:19
Quoted 6 lines
Quoted 3 lines · On Thu, 29 Aug 2019 12:19:35 +0200 Vitor Soares <[email protected]>…
For today the reattach function only update the device address on the controller.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 34 lines
Reviewed-by Boris Brezillon <[email protected]>Quoted 31 lines
Signed-off-by Vitor Soares <[email protected]>--- drivers/i3c/master/dw-i3c-master.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)drivers/i3c/master/dw-i3c-master.c +16
-0@@ -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); + }
Quoted 2 lines
drivers/i3c/master/dw-i3c-master.c static int dw_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, · line 919
Quoted 2 lines
drivers/i3c/master/dw-i3c-master.c +0
-0writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(dev->info.dyn_addr), master->regs +
Best regards,
Vitor Soares