Messages in this thread | | | Date | Wed, 20 May 2020 10:58:55 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH] i3c: master: fix for SETDASA and DAA process |
| |
On Thu, 14 May 2020 18:30:09 +0200 Parshuram Thombare <pthombar@cadence.com> 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 <pthombar@cadence.com> > --- > 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(&master->bus, > dev->info.dyn_addr, > I3C_ADDR_SLOT_I3C_DEV); > + > + if (old_dyn_addr) > + i3c_bus_set_addr_slot_status(&master->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, &info); > + 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 &i3c_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) > 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 +1702,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 > @@ -1746,8 +1741,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) > - i3c_master_pre_assign_dyn_addr(i3cdev); > + list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) > + i3c_master_pre_assign_dyn_addr(master, i3cboardinfo); > > ret = i3c_master_do_daa(master); > if (ret) > @@ -1811,7 +1806,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; > - u8 old_dyn_addr = addr, expected_dyn_addr; > + u8 old_dyn_addr = addr; > struct i3c_ibi_setup ibireq = { }; > bool enable_ibi = false; > int ret; > @@ -1866,39 +1861,25 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > i3c_master_free_i3c_dev(olddev); > } > > - ret = i3c_master_reattach_i3c_dev(newdev, old_dyn_addr); > - if (ret) > - goto err_detach_dev; > - > /* > * Depending on our previous state, the expected dynamic address might > * differ: > * - if the device already had a dynamic address assigned, let's try to > - * re-apply this one > - * - if the device did not have a dynamic address and the firmware > - * requested a specific address, pick this one > + * re-apply this one. Device with dyn_addr participated in DAA ? > * - in any other case, keep the address automatically assigned by the > * master > */ > - if (old_dyn_addr && old_dyn_addr != newdev->info.dyn_addr) > - expected_dyn_addr = old_dyn_addr; > - else if (newdev->boardinfo && newdev->boardinfo->init_dyn_addr) > - expected_dyn_addr = newdev->boardinfo->init_dyn_addr; > - else > - expected_dyn_addr = newdev->info.dyn_addr; > - > - if (newdev->info.dyn_addr != expected_dyn_addr) { > + if (old_dyn_addr && old_dyn_addr != newdev->info.dyn_addr) { > /* > * Try to apply the expected dynamic address. If it fails, keep > * the address assigned by the master. > */ > ret = i3c_master_setnewda_locked(master, > newdev->info.dyn_addr, > - expected_dyn_addr); > + old_dyn_addr); > if (!ret) { > - old_dyn_addr = newdev->info.dyn_addr; > - newdev->info.dyn_addr = expected_dyn_addr; > - i3c_master_reattach_i3c_dev(newdev, old_dyn_addr); > + newdev->info.dyn_addr = old_dyn_addr; > + i3c_master_reattach_i3c_dev(newdev, addr); > } else { > dev_err(&master->dev, > "Failed to assign reserved/old address to device %d%llx",
| |