* [PATCH v2 0/2] I3C SETDASA and DAA process fix @ 2020-05-21 9:31 Parshuram Thombare 2020-05-21 9:32 ` [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo Parshuram Thombare 2020-05-21 9:33 ` [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process Parshuram Thombare 0 siblings, 2 replies; 7+ messages in thread From: Parshuram Thombare @ 2020-05-21 9:31 UTC (permalink / raw) To: bbrezillon, vitor.soares Cc: mparab, Parshuram Thombare, praneeth, linux-kernel, pgaj, linux-i3c Changes between v1 and v2 are: 1. Added boardinfo attach fix. 2. Removed reattach issue related fix. 3. Reserve init_dyn_addr initially, so that it will not be used in DAA and attempt can be made to set those firmware requested dynamic address after DAA. Regards, Parshuram Thombare Parshuram Thombare (2): i3c: master add i3c_master_attach_boardinfo to preserve boardinfo i3c: master: fix for SETDASA and DAA process drivers/i3c/master.c | 138 +++++++++++++++++++++++++++---------------- 1 file changed, 88 insertions(+), 50 deletions(-) -- 2.17.1 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo 2020-05-21 9:31 [PATCH v2 0/2] I3C SETDASA and DAA process fix Parshuram Thombare @ 2020-05-21 9:32 ` Parshuram Thombare 2020-05-29 14:42 ` Boris Brezillon 2020-05-21 9:33 ` [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process Parshuram Thombare 1 sibling, 1 reply; 7+ messages in thread From: Parshuram Thombare @ 2020-05-21 9:32 UTC (permalink / raw) To: bbrezillon, vitor.soares Cc: mparab, Parshuram Thombare, praneeth, linux-kernel, pgaj, linux-i3c Boardinfo was lost if I3C object for devices with boardinfo available are not created or not added to the I3C device list because of some failure e.g. SETDASA failed, retrieve info failed etc This patch adds i3c_master_attach_boardinfo which scan boardinfo list in the master object and 'attach' it to the I3C device object. Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/i3c/master.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 5f4bd52121fe..3d995f247cb7 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1776,6 +1776,21 @@ static void i3c_master_bus_cleanup(struct i3c_master_controller *master) i3c_master_detach_free_devs(master); } +static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev) +{ + struct i3c_master_controller *master = i3cdev->common.master; + struct i3c_dev_boardinfo *i3cboardinfo; + + list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { + if (i3cdev->info.pid != i3cboardinfo->pid) + continue; + + i3cdev->boardinfo = i3cboardinfo; + i3cdev->info.static_addr = i3cboardinfo->static_addr; + return; + } +} + static struct i3c_dev_desc * i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) { @@ -1831,10 +1846,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, if (ret) goto err_detach_dev; + i3c_master_attach_boardinfo(newdev); + olddev = i3c_master_search_i3c_dev_duplicate(newdev); if (olddev) { - newdev->boardinfo = olddev->boardinfo; - newdev->info.static_addr = olddev->info.static_addr; newdev->dev = olddev->dev; if (newdev->dev) newdev->dev->desc = newdev; -- 2.17.1 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo 2020-05-21 9:32 ` [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo Parshuram Thombare @ 2020-05-29 14:42 ` Boris Brezillon 0 siblings, 0 replies; 7+ messages in thread From: Boris Brezillon @ 2020-05-29 14:42 UTC (permalink / raw) Cc: mparab, bbrezillon, praneeth, Parshuram Thombare, linux-kernel, vitor.soares, pgaj, linux-i3c On Thu, 21 May 2020 11:32:22 +0200 Parshuram Thombare <pthombar@cadence.com> wrote: > Boardinfo was lost if I3C object for devices with boardinfo > available are not created or not added to the I3C device list > because of some failure e.g. SETDASA failed, retrieve info failed etc > This patch adds i3c_master_attach_boardinfo which scan boardinfo list > in the master object and 'attach' it to the I3C device object. > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> This patch looks good to me. I'll apply it just after the merge window. > --- > drivers/i3c/master.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 5f4bd52121fe..3d995f247cb7 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1776,6 +1776,21 @@ static void i3c_master_bus_cleanup(struct i3c_master_controller *master) > i3c_master_detach_free_devs(master); > } > > +static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev) > +{ > + struct i3c_master_controller *master = i3cdev->common.master; > + struct i3c_dev_boardinfo *i3cboardinfo; > + > + list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { > + if (i3cdev->info.pid != i3cboardinfo->pid) > + continue; > + > + i3cdev->boardinfo = i3cboardinfo; > + i3cdev->info.static_addr = i3cboardinfo->static_addr; > + return; > + } > +} > + > static struct i3c_dev_desc * > i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) > { > @@ -1831,10 +1846,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > if (ret) > goto err_detach_dev; > > + i3c_master_attach_boardinfo(newdev); > + > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > if (olddev) { > - newdev->boardinfo = olddev->boardinfo; > - newdev->info.static_addr = olddev->info.static_addr; > newdev->dev = olddev->dev; > if (newdev->dev) > newdev->dev->desc = newdev; _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process 2020-05-21 9:31 [PATCH v2 0/2] I3C SETDASA and DAA process fix Parshuram Thombare 2020-05-21 9:32 ` [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo Parshuram Thombare @ 2020-05-21 9:33 ` Parshuram Thombare 2020-08-19 8:12 ` Boris Brezillon 1 sibling, 1 reply; 7+ messages in thread From: Parshuram Thombare @ 2020-05-21 9:33 UTC (permalink / raw) To: bbrezillon, vitor.soares Cc: mparab, Parshuram Thombare, praneeth, linux-kernel, pgaj, linux-i3c This patch fix following issue. Controller slots blocked for devices with static_addr but no init_dyn_addr may limit the number of I3C devices on the bus which gets dynamic address in DAA. So instead of attaching all the devices with static_addr, now we only attach the devices which successfully complete SETDASA. For remaining devices with init_dyn_addr, i3c_master_add_i3c_dev_locked() will try to set requested dynamic address after DAA. Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") Signed-off-by: Parshuram Thombare <pthombar@cadence.com> --- drivers/i3c/master.c | 119 ++++++++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 48 deletions(-) diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 3d995f247cb7..5e0438ecf95c 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1426,33 +1426,57 @@ 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_master_controller *master, + struct i3c_dev_boardinfo *boardinfo) { - struct i3c_master_controller *master = i3c_dev_get_master(dev); + struct i3c_device_info info = { + .static_addr = boardinfo->static_addr, + }; + struct i3c_dev_desc *i3cdev; int ret; - if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || - !dev->boardinfo->static_addr) - return; + /* + * We anyway don't attach devices which are not addressable + * (no static_addr and dyn_addr) and devices with static_addr + * but no init_dyn_addr will participate in DAA. + */ + if (!boardinfo->static_addr || !boardinfo->init_dyn_addr) + return -EINVAL; - ret = i3c_master_setdasa_locked(master, dev->info.static_addr, - dev->boardinfo->init_dyn_addr); + i3cdev = i3c_master_alloc_i3c_dev(master, &info); + if (IS_ERR(i3cdev)) + return -ENOMEM; + + i3cdev->boardinfo = boardinfo; + + ret = i3c_master_attach_i3c_dev(master, i3cdev); if (ret) - return; + goto err_attach; - dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; - ret = i3c_master_reattach_i3c_dev(dev, 0); + ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr, + i3cdev->boardinfo->init_dyn_addr); + if (ret) + goto err_setdasa; + + i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr; + ret = i3c_master_reattach_i3c_dev(i3cdev, 0); if (ret) goto err_rstdaa; - ret = i3c_master_retrieve_dev_info(dev); + ret = i3c_master_retrieve_dev_info(i3cdev); if (ret) goto err_rstdaa; - return; + return 0; err_rstdaa: - i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); + i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr); +err_setdasa: + i3c_master_detach_i3c_dev(i3cdev); +err_attach: + i3c_master_free_i3c_dev(i3cdev); + + return ret; } static void @@ -1619,8 +1643,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) * This function is following all initialisation steps described in the I3C * specification: * - * 1. Attach I2C and statically defined I3C devs to the master so that the - * master can fill its internal device table appropriately + * 1. Attach I2C devs to the master so that the master can fill its internal + * device table appropriately * * 2. Call &i3c_master_controller_ops->bus_init() method to initialize * the master controller. That's usually where the bus mode is selected @@ -1633,7 +1657,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) * 4. Disable all slave events. * * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C - * devices that have a static address + * devices that have a static address and attach corresponding statically + * defined I3C devices to the master. * * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all * remaining I3C devices @@ -1647,7 +1672,6 @@ 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 i2c_dev_desc *i2cdev; int ret; @@ -1679,34 +1703,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) goto err_detach_devs; } } - 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; - } - } /* * Now execute the controller specific ->bus_init() routine, which @@ -1744,10 +1740,26 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) /* * Pre-assign dynamic address and retrieve device information if - * needed. + * needed. And reserve the init_dyn_addr in case of failure, to retry + * setting the requested address after DAA is done in + * i3c_master_add_i3c_dev_locked(). */ - i3c_bus_for_each_i3cdev(&master->bus, i3cdev) - i3c_master_pre_assign_dyn_addr(i3cdev); + list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { + ret = i3c_master_pre_assign_dyn_addr(master, i3cboardinfo); + if (!ret || !i3cboardinfo->init_dyn_addr) + continue; + + ret = i3c_bus_get_addr_slot_status(&master->bus, + i3cboardinfo->init_dyn_addr); + if (ret != I3C_ADDR_SLOT_FREE) { + ret = -EBUSY; + goto err_rstdaa; + } + + i3c_bus_set_addr_slot_status(&master->bus, + i3cboardinfo->init_dyn_addr, + I3C_ADDR_SLOT_I3C_DEV); + } ret = i3c_master_do_daa(master); if (ret) @@ -1780,6 +1792,7 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev) { struct i3c_master_controller *master = i3cdev->common.master; struct i3c_dev_boardinfo *i3cboardinfo; + u8 init_dyn_addr; list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { if (i3cdev->info.pid != i3cboardinfo->pid) @@ -1787,6 +1800,16 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev) i3cdev->boardinfo = i3cboardinfo; i3cdev->info.static_addr = i3cboardinfo->static_addr; + init_dyn_addr = i3cboardinfo->init_dyn_addr; + /* + * Free reserved init_dyn_addr so that attach can + * get it before trying setnewda. + */ + if (i3cboardinfo->init_dyn_addr) + i3c_bus_set_addr_slot_status(&master->bus, + init_dyn_addr, + I3C_ADDR_SLOT_FREE); + return; } } -- 2.17.1 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process 2020-05-21 9:33 ` [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process Parshuram Thombare @ 2020-08-19 8:12 ` Boris Brezillon 2020-08-20 9:23 ` Parshuram Raju Thombare 0 siblings, 1 reply; 7+ messages in thread From: Boris Brezillon @ 2020-08-19 8:12 UTC (permalink / raw) To: Parshuram Thombare Cc: mparab, bbrezillon, praneeth, linux-kernel, vitor.soares, pgaj, linux-i3c Hello Parshuram, Sorry for the late reply :-/. On Thu, 21 May 2020 11:33:01 +0200 Parshuram Thombare <pthombar@cadence.com> wrote: > This patch fix following issue. > Controller slots blocked for devices with static_addr > but no init_dyn_addr may limit the number of I3C devices > on the bus which gets dynamic address in DAA. So > instead of attaching all the devices with static_addr, > now we only attach the devices which successfully > complete SETDASA. For remaining devices with init_dyn_addr, > i3c_master_add_i3c_dev_locked() will try to set requested > dynamic address after DAA. > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > Signed-off-by: Parshuram Thombare <pthombar@cadence.com> > --- > drivers/i3c/master.c | 119 ++++++++++++++++++++++++++----------------- > 1 file changed, 71 insertions(+), 48 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 3d995f247cb7..5e0438ecf95c 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1426,33 +1426,57 @@ 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_master_controller *master, > + struct i3c_dev_boardinfo *boardinfo) > { > - struct i3c_master_controller *master = i3c_dev_get_master(dev); > + struct i3c_device_info info = { > + .static_addr = boardinfo->static_addr, > + }; > + struct i3c_dev_desc *i3cdev; > int ret; > > - if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || > - !dev->boardinfo->static_addr) > - return; > + /* > + * We anyway don't attach devices which are not addressable You can drop the anyway. > + * (no static_addr and dyn_addr) and devices with static_addr > + * but no init_dyn_addr will participate in DAA. > + */ > + if (!boardinfo->static_addr || !boardinfo->init_dyn_addr) > + return -EINVAL; If we consider this as an error, we should probably check that before calling i3c_master_pre_assign_dyn_addr() in i3c_master_bus_init(). > > - ret = i3c_master_setdasa_locked(master, dev->info.static_addr, > - dev->boardinfo->init_dyn_addr); > + i3cdev = i3c_master_alloc_i3c_dev(master, &info); > + if (IS_ERR(i3cdev)) > + return -ENOMEM; > + > + i3cdev->boardinfo = boardinfo; > + > + ret = i3c_master_attach_i3c_dev(master, i3cdev); > if (ret) > - return; > + goto err_attach; > > - dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > - ret = i3c_master_reattach_i3c_dev(dev, 0); > + ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr, > + i3cdev->boardinfo->init_dyn_addr); > + if (ret) > + goto err_setdasa; > + > + i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr; > + ret = i3c_master_reattach_i3c_dev(i3cdev, 0); > if (ret) > goto err_rstdaa; > > - ret = i3c_master_retrieve_dev_info(dev); > + ret = i3c_master_retrieve_dev_info(i3cdev); > if (ret) > goto err_rstdaa; > > - return; > + return 0; > > err_rstdaa: > - i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); > + i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr); > +err_setdasa: > + i3c_master_detach_i3c_dev(i3cdev); > +err_attach: > + i3c_master_free_i3c_dev(i3cdev); > + > + return ret; > } > > static void > @@ -1619,8 +1643,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) > * This function is following all initialisation steps described in the I3C > * specification: > * > - * 1. Attach I2C and statically defined I3C devs to the master so that the > - * master can fill its internal device table appropriately > + * 1. Attach I2C devs to the master so that the master can fill its internal > + * device table appropriately > * > * 2. Call &i3c_master_controller_ops->bus_init() method to initialize > * the master controller. That's usually where the bus mode is selected > @@ -1633,7 +1657,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) > * 4. Disable all slave events. > * > * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C > - * devices that have a static address > + * devices that have a static address and attach corresponding statically > + * defined I3C devices to the master. and attach them to the master if the dynamic address assignment succeeds > * > * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all > * remaining I3C devices > @@ -1647,7 +1672,6 @@ 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 i2c_dev_desc *i2cdev; > int ret; > > @@ -1679,34 +1703,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > goto err_detach_devs; > } > } > - 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; > - } > - } > > /* > * Now execute the controller specific ->bus_init() routine, which > @@ -1744,10 +1740,26 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > /* > * Pre-assign dynamic address and retrieve device information if > - * needed. > + * needed. And reserve the init_dyn_addr in case of failure, to retry > + * setting the requested address after DAA is done in > + * i3c_master_add_i3c_dev_locked(). > */ > - i3c_bus_for_each_i3cdev(&master->bus, i3cdev) > - i3c_master_pre_assign_dyn_addr(i3cdev); > + list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { > + ret = i3c_master_pre_assign_dyn_addr(master, i3cboardinfo); > + if (!ret || !i3cboardinfo->init_dyn_addr) > + continue; > + > + ret = i3c_bus_get_addr_slot_status(&master->bus, > + i3cboardinfo->init_dyn_addr); > + if (ret != I3C_ADDR_SLOT_FREE) { > + ret = -EBUSY; > + goto err_rstdaa; > + } > + > + i3c_bus_set_addr_slot_status(&master->bus, > + i3cboardinfo->init_dyn_addr, > + I3C_ADDR_SLOT_I3C_DEV); > + } > > ret = i3c_master_do_daa(master); > if (ret) > @@ -1780,6 +1792,7 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev) > { > struct i3c_master_controller *master = i3cdev->common.master; > struct i3c_dev_boardinfo *i3cboardinfo; > + u8 init_dyn_addr; > > list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { > if (i3cdev->info.pid != i3cboardinfo->pid) > @@ -1787,6 +1800,16 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev) > > i3cdev->boardinfo = i3cboardinfo; > i3cdev->info.static_addr = i3cboardinfo->static_addr; > + init_dyn_addr = i3cboardinfo->init_dyn_addr; > + /* > + * Free reserved init_dyn_addr so that attach can > + * get it before trying setnewda. > + */ > + if (i3cboardinfo->init_dyn_addr) > + i3c_bus_set_addr_slot_status(&master->bus, > + init_dyn_addr, > + I3C_ADDR_SLOT_FREE); Hm, it's a bit more complicated. I don't think we can unconditionally release the init_dyn_addr here. Say you have a device that's been assigned its init_dyn_addr, and userspace decided to re-assign a new one (the feature is not available yet, but I thought about letting userspace write to the dyn_addr sysfs entry and wire that to a SETDA). The init_dyn_addr can now be re-assigned to a different device. After some time the device ends up resetting and thus lose its DA. A new DAA is issued to re-discover it, but you want this device to be assigned its last known address not the init address. And when i3c_master_attach_boardinfo() is called on this new device, you release a slot that's no longer yours. That was the rational behind the "address slots are attached to i3cdevs not boardinfo". Maybe we should have another list where we keep i3c devs that have not been discovered yet but have boardinfo attached to them. This way we can reserve dynamic addresses without blocking a slot in the master device table. > + > return; > } > } -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process 2020-08-19 8:12 ` Boris Brezillon @ 2020-08-20 9:23 ` Parshuram Raju Thombare 2020-08-20 9:47 ` Boris Brezillon 0 siblings, 1 reply; 7+ messages in thread From: Parshuram Raju Thombare @ 2020-08-20 9:23 UTC (permalink / raw) To: Boris Brezillon Cc: Milind Parab, bbrezillon, praneeth, linux-kernel, vitor.soares, Przemyslaw Gaj, linux-i3c Hi Boris, Thanks for your comments. >> + * We anyway don't attach devices which are not addressable > >You can drop the anyway. Sure, I will make above mentioned change in the comment. >> + * (no static_addr and dyn_addr) and devices with static_addr >> + * but no init_dyn_addr will participate in DAA. >> + */ >> + if (!boardinfo->static_addr || !boardinfo->init_dyn_addr) >> + return -EINVAL; > >If we consider this as an error, we should probably check that before >calling i3c_master_pre_assign_dyn_addr() in i3c_master_bus_init(). Ok, I will move this check to i3c_master_bus_init(), before calling i3c_master_pre_assign_dyn_addr. It will probably add extra if condition, but will save one function call. >> * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C >> - * devices that have a static address >> + * devices that have a static address and attach corresponding statically >> + * defined I3C devices to the master. > > and attach them to the > master if > the dynamic address assignment succeeds Sure, I will append above mentioned change to the comment. >> + /* >> + * Free reserved init_dyn_addr so that attach can >> + * get it before trying setnewda. >> + */ >> + if (i3cboardinfo->init_dyn_addr) >> + i3c_bus_set_addr_slot_status(&master->bus, >> + init_dyn_addr, >> + I3C_ADDR_SLOT_FREE); > >Hm, it's a bit more complicated. I don't think we can unconditionally >release the init_dyn_addr here. Say you have a device that's been >assigned its init_dyn_addr, and userspace decided to re-assign a new >one (the feature is not available yet, but I thought about letting >userspace write to the dyn_addr sysfs entry and wire that to a SETDA). >The init_dyn_addr can now be re-assigned to a different device. After >some time the device ends up resetting and thus lose its DA. A new DAA >is issued to re-discover it, but you want this device to be assigned its >last known address not the init address. And when >i3c_master_attach_boardinfo() is called on this new device, you release >a slot that's no longer yours. > >That was the rational behind the "address slots are attached to i3cdevs >not boardinfo". Maybe we should have another list where we keep i3c >devs that have not been discovered yet but have boardinfo attached to >them. This way we can reserve dynamic addresses without blocking a >slot in the master device table. I think the sequence of events you are discussing here is 1. User assign address to device A with init_dyn_addr in boardinfo. 2. That particular init_dyn_addr is assigned to device B, which may be hotplugged ? and don't have boardinfo or init_dyn_addr in boardinfo ? 3. Device A resets and trigger DAA due to hot plug ? A. Here now init_dyn_addr is already assigned to device B so device A shouldn't be freeing it. B. Now preferable dyn_addr is the one received from user in step 1. If we are to prefer init_dyn_addr always, that will rule out possibility of making init_dyn_addr available to any other device when original device is assigned with user or master provided address owing to SETDATA or SETNEWDA failures. And we can be sure of not freeing init_dyn_addr inadvertently while it is being used by any other device. Else if we want to prefer user provided address even across resets, since we don't need init_dyn_addr anymore, it can be used to store user provided address. This will serve both the purposes A and B stated above. And in my opinion this can be handled when we add code to allow user to change the device address. Regards, Parshuram Thombare -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process 2020-08-20 9:23 ` Parshuram Raju Thombare @ 2020-08-20 9:47 ` Boris Brezillon 0 siblings, 0 replies; 7+ messages in thread From: Boris Brezillon @ 2020-08-20 9:47 UTC (permalink / raw) To: Parshuram Raju Thombare Cc: Milind Parab, bbrezillon, praneeth, linux-kernel, vitor.soares, Przemyslaw Gaj, linux-i3c On Thu, 20 Aug 2020 09:23:25 +0000 Parshuram Raju Thombare <pthombar@cadence.com> wrote: > Hi Boris, > > Thanks for your comments. > > >> + * We anyway don't attach devices which are not addressable > > > >You can drop the anyway. > Sure, I will make above mentioned change in the comment. > > >> + * (no static_addr and dyn_addr) and devices with static_addr > >> + * but no init_dyn_addr will participate in DAA. > >> + */ > >> + if (!boardinfo->static_addr || !boardinfo->init_dyn_addr) > >> + return -EINVAL; > > > >If we consider this as an error, we should probably check that before > >calling i3c_master_pre_assign_dyn_addr() in i3c_master_bus_init(). > Ok, I will move this check to i3c_master_bus_init(), before calling > i3c_master_pre_assign_dyn_addr. It will probably add extra if condition, > but will save one function call. > > >> * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C > >> - * devices that have a static address > >> + * devices that have a static address and attach corresponding statically > >> + * defined I3C devices to the master. > > > > and attach them to the > > master if > > the dynamic address assignment succeeds > Sure, I will append above mentioned change to the comment. > > >> + /* > >> + * Free reserved init_dyn_addr so that attach can > >> + * get it before trying setnewda. > >> + */ > >> + if (i3cboardinfo->init_dyn_addr) > >> + i3c_bus_set_addr_slot_status(&master->bus, > >> + init_dyn_addr, > >> + I3C_ADDR_SLOT_FREE); > > > >Hm, it's a bit more complicated. I don't think we can unconditionally > >release the init_dyn_addr here. Say you have a device that's been > >assigned its init_dyn_addr, and userspace decided to re-assign a new > >one (the feature is not available yet, but I thought about letting > >userspace write to the dyn_addr sysfs entry and wire that to a SETDA). > >The init_dyn_addr can now be re-assigned to a different device. After > >some time the device ends up resetting and thus lose its DA. A new DAA > >is issued to re-discover it, but you want this device to be assigned its > >last known address not the init address. And when > >i3c_master_attach_boardinfo() is called on this new device, you release > >a slot that's no longer yours. > > > >That was the rational behind the "address slots are attached to i3cdevs > >not boardinfo". Maybe we should have another list where we keep i3c > >devs that have not been discovered yet but have boardinfo attached to > >them. This way we can reserve dynamic addresses without blocking a > >slot in the master device table. > > I think the sequence of events you are discussing here is > 1. User assign address to device A with init_dyn_addr in boardinfo. > 2. That particular init_dyn_addr is assigned to device B, which may be hotplugged ? > and don't have boardinfo or init_dyn_addr in boardinfo ? > 3. Device A resets and trigger DAA due to hot plug ? > A. Here now init_dyn_addr is already assigned to device B so device A shouldn't be freeing it. > B. Now preferable dyn_addr is the one received from user in step 1. No, that's not what I'm talking about. I meant: 1. Device A is assigned a default init address X in the DT. 2. Device B has no init address 3. The framework reserves address X for and assigns it to device A when it appears on the bus (DAA, SETDASA or HJ+DDA) 4. Device B is assigned address Y 5. User decides to explicitly assign a different address to device A by issuing "echo Z > /sys/bus/i3c/..../<i3c-dev>/dyn_addr" (not yet supported, but I think we should allow that at some point), such that device A gets a lower/higher priority 6. User manually assigns address X to device B (that should be allowed since device A no longer uses X) 7. Device A is reset for some reason and loses its dynamic address, thus requiring a new DAA (or HJ+DAA). During this new discovery, device A is re-assigned its last known address (Z), but in the meantime you've marked address X as free (when attaching boardinfo to the newdev object). > > If we are to prefer init_dyn_addr always, that will rule out possibility of making init_dyn_addr > available to any other device when original device is assigned with user or master > provided address owing to SETDATA or SETNEWDA failures. And we can be sure of not freeing > init_dyn_addr inadvertently while it is being used by any other device. > > Else if we want to prefer user provided address even across resets, since we don't need init_dyn_addr > anymore, it can be used to store user provided address. This will serve both the purposes A and B stated above. > > And in my opinion this can be handled when we add code to allow user to change the device address. If we go for a temporary solution, I'd opt for relaxing the test done in i3c_master_get_i3c_addrs() to not reserve the init address (since it should have been reserved at probe time) and keep those init addressed reserved. -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-08-23 9:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-21 9:31 [PATCH v2 0/2] I3C SETDASA and DAA process fix Parshuram Thombare 2020-05-21 9:32 ` [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo Parshuram Thombare 2020-05-29 14:42 ` Boris Brezillon 2020-05-21 9:33 ` [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process Parshuram Thombare 2020-08-19 8:12 ` Boris Brezillon 2020-08-20 9:23 ` Parshuram Raju Thombare 2020-08-20 9:47 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).