Re: [PATCH v4] i3c: master: fix for SETDASA and DAA process
On Sun, 23 Aug 2020 11:59:18 +0200 Boris Brezillon wrote: > > -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > +static int i3c_master_early_i3c_dev_add(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; > > + i3cdev = i3c_master_alloc_i3c_dev(master, ); > > + if (IS_ERR(i3cdev)) > > + return -ENOMEM; > > > > - 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; > > + goto err_attach; > > + > > + ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr, > > + i3cdev->boardinfo->init_dyn_addr); > > + if (ret) > > + goto err_setdasa; > > > > - dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > > - ret = i3c_master_reattach_i3c_dev(dev, 0); > > + 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: Let's be consistent with the rest of the framework. Labels don't encode what the error is but what's expected to be undone. IOW, this one should be err_detach_dev > > + i3c_master_detach_i3c_dev(i3cdev); > > +err_attach: and this one err_free_dev. > > + i3c_master_free_i3c_dev(i3cdev); > > + > > + return ret; > > } > > > > static void > > @@ -1619,8 +1637,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 > > @@ -1633,10 +1651,14 @@ 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. If only init_dyn_addr is available > > + *or if SETDASA fails, reserve those init_dyn_addr to be used later to > > set > > + *address using SETNEWDA after DAA. I'd re-order this to match what really happens: first reserve slots for devices that have an init_dyn_addr defined, then try to create+attach devices that also have a static address defined. > > * > > * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to > > all > > - *remaining I3C devices > > + *remaining I3C devices and attach them to the master if the dynamic > > address > > + *assignment succeeds Can we move that change to a separate patch? > > * > > * Once this is done, all I3C and I2C devices should be usable. > > * > > @@ -1647,7 +1669,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 +1700,6 @@ static int i3c_master_bus_init(struct > > i3c_master_controller *master) > > goto err_detach_devs; > > } > > } > > - list_for_each_entry(i3cboardinfo, >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(>bus, >
Re: [PATCH v4] i3c: master: fix for SETDASA and DAA process
On Fri, 21 Aug 2020 11:13:15 +0200 Parshuram Thombare 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. > > Signed-off-by: Parshuram Thombare > --- > Changes between v3 and v4 are: > 1. Code rectoring and removed Fixes tag > > Changes between v2 and v3 are: > 1. Keeping init_dyn_addr reserved. > 2. Code refactoring and changes in comments. > > 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. > --- > drivers/i3c/master.c | 116 - > 1 files changed, 66 insertions(+), 50 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 3d995f2..3ff95e4 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1367,7 +1367,9 @@ static int i3c_master_reattach_i3c_dev(struct > i3c_dev_desc *dev, > enum i3c_addr_slot_status status; > int ret; > > - if (dev->info.dyn_addr != old_dyn_addr) { > + if (dev->info.dyn_addr != old_dyn_addr && > + (!dev->boardinfo || > + dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) { > status = i3c_bus_get_addr_slot_status(>bus, > dev->info.dyn_addr); > if (status != I3C_ADDR_SLOT_FREE) > @@ -1426,33 +1428,49 @@ 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_early_i3c_dev_add(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; > + i3cdev = i3c_master_alloc_i3c_dev(master, ); > + if (IS_ERR(i3cdev)) > + return -ENOMEM; > > - 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; > + goto err_attach; > + > + ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr, > + i3cdev->boardinfo->init_dyn_addr); > + if (ret) > + goto err_setdasa; > > - dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > - ret = i3c_master_reattach_i3c_dev(dev, 0); > + 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 +1637,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 > @@ -1633,10 +1651,14 @@ 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
[PATCH v4] i3c: master: fix for SETDASA and DAA process
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. Signed-off-by: Parshuram Thombare --- Changes between v3 and v4 are: 1. Code rectoring and removed Fixes tag Changes between v2 and v3 are: 1. Keeping init_dyn_addr reserved. 2. Code refactoring and changes in comments. 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. --- drivers/i3c/master.c | 116 - 1 files changed, 66 insertions(+), 50 deletions(-) diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 3d995f2..3ff95e4 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1367,7 +1367,9 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, enum i3c_addr_slot_status status; int ret; - if (dev->info.dyn_addr != old_dyn_addr) { + if (dev->info.dyn_addr != old_dyn_addr && + (!dev->boardinfo || +dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) { status = i3c_bus_get_addr_slot_status(>bus, dev->info.dyn_addr); if (status != I3C_ADDR_SLOT_FREE) @@ -1426,33 +1428,49 @@ 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_early_i3c_dev_add(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; + i3cdev = i3c_master_alloc_i3c_dev(master, ); + if (IS_ERR(i3cdev)) + return -ENOMEM; - 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; + goto err_attach; + + ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr, + i3cdev->boardinfo->init_dyn_addr); + if (ret) + goto err_setdasa; - dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; - ret = i3c_master_reattach_i3c_dev(dev, 0); + 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 +1637,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 @@ -1633,10 +1651,14 @@ 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. If only init_dyn_addr is available + *or if SETDASA fails, reserve those init_dyn_addr to be used later to set + *