RE: [PATCH] i3c: master: fix for SETDASA and DAA process
>> This patch fix following issues. >> 1. 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. Remaining devices are handled in DAA. >> 2. Since we alreay handled devices with init_dyn_addr, removed >>it's handling from i3c_master_add_i3c_dev_locked(). >>Now only case handled is devices already with dyn_addr >>participated in DAA, and again got new dyn_addr with an >>extra slot in the master controller. > >I don't get that one. I mean retry to assign requested init_dyn_addr in i3c_master_add_i3c_dev_locked(). Since we handle devices with init_dyn_addr in i3c_master_pre_assign_dyn_addr, we should have assigned dynamic address to all devices with both static_addr and init_dyn_addr. Unless SETDASA failed or device only have init_dyn_addr but no static_addr, in those cases dyn_addr is allocated in DAA. >I think we should fix re-attach instead, which is what we discussed >with Przemek if I remember correctly. Sorry, I was not aware of that. But, yes I agree to fix driver re-attach instead of removing re attach here. But we should keep in mind about potential failure here. Currently reattach only update dyn_addr, but IMO it should update other fields as well. Also I see re attach have a unused argument, which I suppose we can get rid of. For now I will keep reattach here and post reattach fix in separate patch. >Can you please split the patch accordingly (one fix per commit) I think only extra changes are releasing the old address in reattach API and removal of reattach call in i3c_master_add_i3c_dev_locked() after a scan for duplicate I3C device. I will remove those changes and repost rest. Regards, Parshuram Thombare
Re: [PATCH] i3c: master: fix for SETDASA and DAA process
On Thu, 14 May 2020 18:30:09 +0200 Parshuram Thombare wrote: > This patch fix following issues. > 1. 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. Remaining devices are handled in DAA. > 2. Since we alreay handled devices with init_dyn_addr, removed >it's handling from i3c_master_add_i3c_dev_locked(). >Now only case handled is devices already with dyn_addr >participated in DAA, and again got new dyn_addr with an >extra slot in the master controller. I don't get that one. > 3. Removed unnecessary i3c_master_reattach_i3c_dev() from >i3c_master_add_i3c_dev_locked(), right away after finding >if duplicate device exists in the I3C list. >In case of different new and old dyn_addr >i3c_master_reattach_i3c_dev() will fail which is wrong, >and in case of same dyn_addr it doesn't add anything new. I think we should fix re-attach instead, which is what we discussed with Przemek if I remember correctly. > Can you please split the patch accordingly (one fix per commit)? > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > Signed-off-by: Parshuram Thombare > --- > drivers/i3c/master.c | 111 ++- > 1 file changed, 46 insertions(+), 65 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 5f4bd52121fe..f1d929b58549 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1375,6 +1375,11 @@ static int i3c_master_reattach_i3c_dev(struct > i3c_dev_desc *dev, > i3c_bus_set_addr_slot_status(>bus, >dev->info.dyn_addr, >I3C_ADDR_SLOT_I3C_DEV); > + > + if (old_dyn_addr) > + i3c_bus_set_addr_slot_status(>bus, > + old_dyn_addr, > + I3C_ADDR_SLOT_FREE); > } > > if (master->ops->reattach_i3c_dev) { > @@ -1426,33 +1431,52 @@ 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 void 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) > + /* > + * 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; > + > + i3cdev = i3c_master_alloc_i3c_dev(master, ); > + if (IS_ERR(i3cdev)) > return; > > - ret = i3c_master_setdasa_locked(master, dev->info.static_addr, > - dev->boardinfo->init_dyn_addr); > + i3cdev->boardinfo = boardinfo; > + > + ret = i3c_master_attach_i3c_dev(master, i3cdev); > if (ret) > return; > > - 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_rstdaa; > + goto err_setdasa; > > - ret = i3c_master_retrieve_dev_info(dev); > + i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr; > + ret = i3c_master_reattach_i3c_dev(i3cdev, 0); > if (ret) > goto err_rstdaa; > > - return; > + ret = i3c_master_retrieve_dev_info(i3cdev); > + if (ret) > + goto err_rstdaa; > > 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); > + i3c_master_free_i3c_dev(i3cdev); > } > > 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
[PATCH] i3c: master: fix for SETDASA and DAA process
This patch fix following issues. 1. 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. Remaining devices are handled in DAA. 2. Since we alreay handled devices with init_dyn_addr, removed it's handling from i3c_master_add_i3c_dev_locked(). Now only case handled is devices already with dyn_addr participated in DAA, and again got new dyn_addr with an extra slot in the master controller. 3. Removed unnecessary i3c_master_reattach_i3c_dev() from i3c_master_add_i3c_dev_locked(), right away after finding if duplicate device exists in the I3C list. In case of different new and old dyn_addr i3c_master_reattach_i3c_dev() will fail which is wrong, and in case of same dyn_addr it doesn't add anything new. Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") Signed-off-by: Parshuram Thombare --- drivers/i3c/master.c | 111 ++- 1 file changed, 46 insertions(+), 65 deletions(-) diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 5f4bd52121fe..f1d929b58549 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1375,6 +1375,11 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, i3c_bus_set_addr_slot_status(>bus, dev->info.dyn_addr, I3C_ADDR_SLOT_I3C_DEV); + + if (old_dyn_addr) + i3c_bus_set_addr_slot_status(>bus, +old_dyn_addr, +I3C_ADDR_SLOT_FREE); } if (master->ops->reattach_i3c_dev) { @@ -1426,33 +1431,52 @@ 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 void 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) + /* +* 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; + + i3cdev = i3c_master_alloc_i3c_dev(master, ); + if (IS_ERR(i3cdev)) return; - ret = i3c_master_setdasa_locked(master, dev->info.static_addr, - dev->boardinfo->init_dyn_addr); + i3cdev->boardinfo = boardinfo; + + ret = i3c_master_attach_i3c_dev(master, i3cdev); if (ret) return; - 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_rstdaa; + goto err_setdasa; - ret = i3c_master_retrieve_dev_info(dev); + i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr; + ret = i3c_master_reattach_i3c_dev(i3cdev, 0); if (ret) goto err_rstdaa; - return; + ret = i3c_master_retrieve_dev_info(i3cdev); + if (ret) + goto err_rstdaa; 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); + i3c_master_free_i3c_dev(i3cdev); } 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 _master_controller_ops->bus_init() method to initialize *the master controller. That's usually where the bus mode is selected @@ -1647,7 +1671,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)