| Message ID | 20190310135843.21154-4-pgaj@cadence.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add the I3C mastership request | expand |
Hi Przemek, Sorry for the late review. On Sun, 10 Mar 2019 13:58:40 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > This patch adds support for mastership request to I3C subsystem. > > Mastership event is enabled globally. > > Mastership is requested automatically when device driver > tries to transfer data using master controller in slave mode. > > There is still some limitation: > - I2C devices are registered on secondary master side if boardinfo > entry matching the info transmitted through the DEFSLVS frame. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > --- > > Main changes between v3 and v4: > - Add i3c_master_acquire_bus_ownership to acquire the bus > - Refactored the code > > Main changes between v2 and v3: > - Add i3c_bus_downgrade_maintenance_lock() for downgrading the bus > lock from maintenance to normal use > - Add additional fields to i2c_dev_desc for DEFSLVS purpose (addr, lvr) > - Add i3c_master_register_new_i2c_devs() function to register I2C devices > - Reworked I2C devices registration on secondary master side > > Changes in v2: > - Add mastership disable event hook > - Changed name of mastership enable event hook > - Add function to test if master owns the bus > - Add function to test if master owns the bus > - Removed op_mode > - Changed parameter of i3c_master_get_accmst_locked, no need to > pass full i3c_device_info > - Changed name of mastership enable event hook > - Add function to test if master owns the bus > - Removed op_mode > - Changed parameter of i3c_master_get_accmst_locked, no need to > pass full i3c_device_info > - Removed redundant DEFSLVS command before GETACCMST > - Add i3c_master_bus_takeover function. There is a need to lock > the bus before adding devices and no matter of the controller > devices have to be added after mastership takeover. > - Add device registration during initialization on secondary master > side. Devices received by DEFSLVS (if occured). If not, device > initialization is deffered untill next mastership request. > --- > drivers/i3c/device.c | 26 +++ > drivers/i3c/internals.h | 4 + > drivers/i3c/master.c | 417 +++++++++++++++++++++++++++++++++++++-------- > include/linux/i3c/master.h | 17 ++ > 4 files changed, 391 insertions(+), 73 deletions(-) > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > index 69cc040..b60f637 100644 > --- a/drivers/i3c/device.c > +++ b/drivers/i3c/device.c > @@ -43,7 +43,13 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > } > > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers); > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > > return ret; > @@ -114,11 +120,17 @@ int i3c_device_enable_ibi(struct i3c_device *dev) > int ret = -ENOENT; > > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > if (dev->desc) { > mutex_lock(&dev->desc->ibi_lock); > ret = i3c_dev_enable_ibi_locked(dev->desc); > mutex_unlock(&dev->desc->ibi_lock); > } > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > > return ret; > @@ -145,11 +157,17 @@ int i3c_device_request_ibi(struct i3c_device *dev, > return -EINVAL; > > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > if (dev->desc) { > mutex_lock(&dev->desc->ibi_lock); > ret = i3c_dev_request_ibi_locked(dev->desc, req); > mutex_unlock(&dev->desc->ibi_lock); > } > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > > return ret; > @@ -166,12 +184,20 @@ EXPORT_SYMBOL_GPL(i3c_device_request_ibi); > */ > void i3c_device_free_ibi(struct i3c_device *dev) > { > + int ret; > + > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > if (dev->desc) { > mutex_lock(&dev->desc->ibi_lock); > i3c_dev_free_ibi_locked(dev->desc); > mutex_unlock(&dev->desc->ibi_lock); > } > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > } > EXPORT_SYMBOL_GPL(i3c_device_free_ibi); > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > index 86b7b44..929ca6b 100644 > --- a/drivers/i3c/internals.h > +++ b/drivers/i3c/internals.h > @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type; > > void i3c_bus_normaluse_lock(struct i3c_bus *bus); > void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus); > +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master); > +int i3c_master_request_mastership_locked(struct i3c_master_controller *master); > + > > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > struct i3c_priv_xfer *xfers, > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index aea4309..7a84158 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > up_read(&bus->lock); > } > > +/** > + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal > + * operation > + * @bus: I3C bus to downgrade the lock on > + * > + * Should be called when a maintenance operation is done and normal > + * operation is planned. See i3c_bus_maintenance_lock() and > + * i3c_bus_normaluse_lock() for more details. > + */ > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus) This function is only used in this file, please make it static and don't expose it in internals.h. Also, the comment above the function should not be part of the doc, as its supposed to be a private function. Just make it a normal comment (/* instead of /**) and don't document the input param. > +{ > + downgrade_write(&bus->lock); > +} > + > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > { > return container_of(dev, struct i3c_master_controller, dev); > @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev) > return driver->probe(i3cdev); > } > > +static void i3c_master_enable_mr_events(struct i3c_master_controller *master) > +{ > + if (!master->ops->enable_mr_events) > + return; > + > + master->ops->enable_mr_events(master); > +} > + > +static void i3c_master_disable_mr_events(struct i3c_master_controller *master) > +{ > + if (!master->ops->disable_mr_events) > + return; > + > + master->ops->disable_mr_events(master); > +} > + > static int i3c_device_remove(struct device *dev) > { > struct i3c_device *i3cdev = dev_to_i3cdev(dev); > @@ -462,6 +492,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus) > return 0; > } > > +int i3c_master_request_mastership_locked(struct i3c_master_controller *master) > +{ > + if (WARN_ON(master->init_done && > + !rwsem_is_locked(&master->bus.lock))) > + return -EINVAL; > + > + if (!master->ops->request_mastership) > + return -ENOTSUPP; > + > + return master->ops->request_mastership(master); > +} Same goes for this function. Please make it private. > + > +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master) > +{ > + int ret; > + > + if (master->bus.cur_master != master->this) { > + i3c_bus_normaluse_unlock(&master->bus); > + i3c_bus_maintenance_lock(&master->bus); > + > + ret = i3c_master_request_mastership_locked(master); > + if (ret) { > + i3c_bus_maintenance_unlock(&master->bus); > + return ret; > + } > + i3c_bus_downgrade_maintenance_lock(&master->bus); > + } > + return 0; > +} > + > static const char * const i3c_bus_mode_strings[] = { > [I3C_BUS_MODE_PURE] = "pure", > [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", > @@ -620,6 +680,25 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master, > > dev->common.master = master; > dev->boardinfo = boardinfo; > + dev->addr = boardinfo->base.addr; > + dev->lvr = boardinfo->lvr; > + > + return dev; > +} This code should be part of patch 1. > + > +static struct i2c_dev_desc * > +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master, > + u16 addr, u8 lvr) > +{ > + struct i2c_dev_desc *dev; > + > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) > + return ERR_PTR(-ENOMEM); > + > + dev->common.master = master; > + dev->addr = addr; > + dev->lvr = lvr; > > return dev; > } > @@ -693,6 +772,9 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master, > struct i2c_dev_desc *dev; > > i3c_bus_for_each_i2cdev(&master->bus, dev) { > + if (!dev->boardinfo) > + continue; > + > if (dev->boardinfo->base.addr == addr) > return dev; > } > @@ -939,8 +1021,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master) > > desc = defslvs->slaves; > i3c_bus_for_each_i2cdev(bus, i2cdev) { > - desc->lvr = i2cdev->boardinfo->lvr; > - desc->static_addr = i2cdev->boardinfo->base.addr << 1; > + desc->lvr = i2cdev->lvr; > + desc->static_addr = i2cdev->addr << 1; > desc++; > } > > @@ -1492,6 +1574,83 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > } > } > > +static struct i2c_dev_boardinfo * > +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master, > + u16 addr, u8 lvr) > +{ > + struct i2c_dev_boardinfo *i2cboardinfo; > + > + list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { > + if (i2cboardinfo->base.addr == addr && > + i2cboardinfo->lvr == lvr) > + return i2cboardinfo; > + } > + > + return NULL; > +} > + > +static void > +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master) > +{ > + struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master); > + struct i2c_dev_desc *i2cdev; > + > + if (!master->init_done) > + return; > + > + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) { > + if (i2cdev->dev) > + continue; > + > + if (!i2cdev->boardinfo) > + continue; > + > + i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base); > + } > +} > + > +/** > + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command > + * @master: master used to send frames on the bus > + * @info: I3C device information > + * > + * Send a GETACCMST CCC command. > + * > + * This should be called if the curent master acknowledges bus takeover. > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: 0 in case of success, a positive I3C error code if the error is > + * one of the official Mx error codes, and a negative error code otherwise. > + */ > +int i3c_master_get_accmst_locked(struct i3c_master_controller *master, > + u8 addr) Looks like you call the i3c_master_mastership_ack() wrapper in the cadence driver, so please keep this function private. > +{ > + struct i3c_ccc_getaccmst *accmst; > + struct i3c_ccc_cmd_dest dest; > + struct i3c_ccc_cmd cmd; > + int ret; > + > + accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst)); > + if (!accmst) > + return -ENOMEM; > + > + i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1); > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + goto out; > + > + if (dest.payload.len != sizeof(*accmst)) > + ret = -EIO; > + > +out: > + i3c_ccc_cmd_dest_cleanup(&dest); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked); > + > /** > * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) > * @master: master doing the DAA > @@ -1559,10 +1718,6 @@ int i3c_master_set_info(struct i3c_master_controller *master, > if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr)) > return -EINVAL; > > - if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER && > - master->secondary) > - return -EINVAL; > - > if (master->this) > return -EINVAL; > > @@ -1607,43 +1762,13 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) > common.node) { > i3c_master_detach_i2c_dev(i2cdev); > i3c_bus_set_addr_slot_status(&master->bus, > - i2cdev->boardinfo->base.addr, > - I3C_ADDR_SLOT_FREE); > + i2cdev->addr, > + I3C_ADDR_SLOT_FREE); Should be part of patch 1. > i3c_master_free_i2c_dev(i2cdev); > } > } > > -/** > - * i3c_master_bus_init() - initialize an I3C bus > - * @master: main master initializing the bus > - * > - * 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 > - * > - * 2. Call &i3c_master_controller_ops->bus_init() method to initialize > - * the master controller. That's usually where the bus mode is selected > - * (pure bus or mixed fast/slow bus) > - * > - * 3. Instruct all devices on the bus to drop their dynamic address. This is > - * particularly important when the bus was previously configured by someone > - * else (for example the bootloader) > - * > - * 4. Disable all slave events. > - * > - * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C > - * devices that have a static address > - * > - * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all > - * remaining I3C devices > - * > - * Once this is done, all I3C and I2C devices should be usable. > - * > - * Return: a 0 in case of success, an negative error code otherwise. > - */ > -static int i3c_master_bus_init(struct i3c_master_controller *master) > +static int i3c_master_attach_static_devs(struct i3c_master_controller *master) > { > enum i3c_addr_slot_status status; > struct i2c_dev_boardinfo *i2cboardinfo; > @@ -1652,32 +1777,24 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > struct i2c_dev_desc *i2cdev; > int ret; > > - /* > - * First attach all devices with static definitions provided by the > - * FW. > - */ > list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { > status = i3c_bus_get_addr_slot_status(&master->bus, > i2cboardinfo->base.addr); > - if (status != I3C_ADDR_SLOT_FREE) { > - ret = -EBUSY; > - goto err_detach_devs; > - } > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > > i3c_bus_set_addr_slot_status(&master->bus, > i2cboardinfo->base.addr, > I3C_ADDR_SLOT_I2C_DEV); > > i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo); > - if (IS_ERR(i2cdev)) { > - ret = PTR_ERR(i2cdev); > - goto err_detach_devs; > - } > + if (IS_ERR(i2cdev)) > + return PTR_ERR(i2cdev); > > ret = i3c_master_attach_i2c_dev(master, i2cdev); > if (ret) { > i3c_master_free_i2c_dev(i2cdev); > - goto err_detach_devs; > + return ret; > } > } > list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { > @@ -1687,28 +1804,71 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > 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; > - } > + i3cboardinfo->init_dyn_addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > } > > i3cdev = i3c_master_alloc_i3c_dev(master, &info); > - if (IS_ERR(i3cdev)) { > - ret = PTR_ERR(i3cdev); > - goto err_detach_devs; > - } > + if (IS_ERR(i3cdev)) > + return PTR_ERR(i3cdev); > > i3cdev->boardinfo = i3cboardinfo; > > ret = i3c_master_attach_i3c_dev(master, i3cdev); > if (ret) { > i3c_master_free_i3c_dev(i3cdev); > - goto err_detach_devs; > + return ret; > } > } > > + return 0; > +} > + > +/** > + * i3c_master_bus_init() - initialize an I3C bus > + * @master: main master initializing the bus > + * > + * 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 > + * > + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize > + * the master controller. That's usually where the bus mode is selected > + * (pure bus or mixed fast/slow bus) > + * > + * 3. Instruct all devices on the bus to drop their dynamic address. This is > + * particularly important when the bus was previously configured by someone > + * else (for example the bootloader) > + * > + * 4. Disable all slave events. > + * > + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C > + * devices that have a static address > + * > + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all > + * remaining I3C devices > + * > + * Once this is done, all I3C and I2C devices should be usable. > + * > + * Return: a 0 in case of success, an negative error code otherwise. > + */ > +static int i3c_master_bus_init(struct i3c_master_controller *master) > +{ > + struct i3c_dev_desc *i3cdev; > + int ret; > + > + /* > + * First attach all devices with static definitions provided by the > + * FW. > + */ > + if (!master->secondary) { > + ret = i3c_master_attach_static_devs(master); > + if (ret) > + goto err_detach_devs; > + } > /* > * Now execute the controller specific ->bus_init() routine, which > * might configure its internal logic to match the bus limitations. > @@ -1729,6 +1889,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > } > > /* > + * Don't reset addresses if this is secondary master. > + * Secondary masters can't do DAA. > + */ > + if (master->secondary) > + return 0; > + > + /* > * Reset all dynamic address that may have been assigned before > * (assigned by the bootloader for example). > */ > @@ -1791,6 +1958,49 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) > return NULL; > } > > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, > + u16 addr, u8 lvr) Documentation is missing for this function. And I guess the lock must be held, so please suffix it with _locked. > +{ > + enum i3c_addr_slot_status status; > + struct i2c_dev_desc *i2cdev; > + int ret; > + > + if (!master) > + return -EINVAL; > + > + status = i3c_bus_get_addr_slot_status(&master->bus, > + addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > + > + i3c_bus_set_addr_slot_status(&master->bus, addr, > + I3C_ADDR_SLOT_I2C_DEV); > + > + i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr); > + if (IS_ERR(i2cdev)) { > + ret = PTR_ERR(i2cdev); > + goto err_free_dev; > + } > + > + i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr); > + > + ret = i3c_master_attach_i2c_dev(master, i2cdev); > + if (ret) { > + ret = PTR_ERR(i2cdev); > + goto err_free_dev; > + } > + > + return 0; > + > +err_free_dev: > + i3c_bus_set_addr_slot_status(&master->bus, addr, > + I3C_ADDR_SLOT_FREE); > + i3c_master_free_i2c_dev(i2cdev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev); > + > /** > * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus > * @master: master used to send frames on the bus > @@ -2113,11 +2323,17 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, > } > > i3c_bus_normaluse_lock(&master->bus); > + ret = i3c_master_acquire_bus_ownership(master); > + if (ret) > + goto err_unlock_bus; > + > dev = i3c_master_find_i2c_dev_by_addr(master, addr); > if (!dev) > ret = -ENOENT; > else > ret = master->ops->i2c_xfers(dev, xfers, nxfers); > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(&master->bus); > > return ret ? ret : nxfers; > @@ -2151,8 +2367,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master) > * We silently ignore failures here. The bus should keep working > * correctly even if one or more i2c devices are not registered. > */ > - i3c_bus_for_each_i2cdev(&master->bus, i2cdev) > + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) { > + if (!i2cdev->boardinfo) > + continue; > + > i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base); > + } > > return 0; > } > @@ -2393,18 +2613,43 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) > !ops->recycle_ibi_slot)) > return -EINVAL; > > + if (ops->request_mastership && > + (!ops->enable_mr_events || !ops->disable_mr_events)) > + return -EINVAL; Can you maybe add a comment explaining why ->enable/disable_mr_events() are required? > + > return 0; > } > > /** > + * i3c_master_register_new_devs() - register new devices > + * @master: master used to send frames on the bus > + * > + * This function is useful when devices were not added > + * during initialization or when new device joined the bus > + * which was under control of different master. > + */ > +void i3c_master_register_new_devs(struct i3c_master_controller *master) > +{ > + /* > + * We can register I3C devices received from master by DEFSLVS. > + */ > + i3c_bus_normaluse_lock(&master->bus); > + i3c_master_register_new_i3c_devs(master); > + i3c_bus_normaluse_unlock(&master->bus); > + > + i3c_bus_normaluse_lock(&master->bus); Why do you release+take the lock here? Can't you keep it? > + i3c_master_register_new_i2c_devs(master); > + i3c_bus_normaluse_unlock(&master->bus); > +} > +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs); > + > +/** > * i3c_master_register() - register an I3C master > * @master: master used to send frames on the bus > * @parent: the parent device (the one that provides this I3C master > * controller) > * @ops: the master controller operations > - * @secondary: true if you are registering a secondary master. Will return > - * -ENOTSUPP if set to true since secondary masters are not yet > - * supported > + * @secondary: true if you are registering a secondary master. > * > * This function takes care of everything for you: > * > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller *master, > struct i2c_dev_boardinfo *i2cbi; > int ret; > > - /* We do not support secondary masters yet. */ > - if (secondary) > - return -ENOTSUPP; > - > ret = i3c_master_check_ops(ops); > if (ret) > return ret; > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller *master, > * register I3C devices dicovered during the initial DAA. > */ > master->init_done = true; > - i3c_bus_normaluse_lock(&master->bus); > - i3c_master_register_new_i3c_devs(master); > - i3c_bus_normaluse_unlock(&master->bus); > + i3c_master_register_new_devs(master); > + > + i3c_bus_maintenance_lock(&master->bus); > + i3c_master_enable_mr_events(master); i3c_master_enable_mr_events() should be suffixed with _locked. And please add a comment explaining why you enable MR events here. > + i3c_bus_maintenance_unlock(&master->bus); > > return 0; > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller *master, > EXPORT_SYMBOL_GPL(i3c_master_register); > > /** > + * i3c_master_mastership_ack() - performs operations before bus handover. > + * @master: master used to send frames on the bus > + * @addr: I3C device address > + * > + * Basically, it sends DEFSLVS command to ensure new master is taking Are you sure of that? It seems to only send GETACCMST, and DEFSLVS has probably been sent early, when the secondary master was discovered (during DAA). > + * the bus with complete list of devices and then acknowledges bus takeover. > + * > + * Return: 0 in case of success, a negative error code otherwise. > + */ > +int i3c_master_mastership_ack(struct i3c_master_controller *master, > + u8 addr) > +{ > + int ret; > + > + i3c_bus_maintenance_lock(&master->bus); > + ret = i3c_master_get_accmst_locked(master, addr); > + i3c_bus_maintenance_unlock(&master->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); > + > + > +/** > * i3c_master_unregister() - unregister an I3C master > * @master: master used to send frames on the bus > * > @@ -2533,6 +2800,10 @@ EXPORT_SYMBOL_GPL(i3c_master_register); > */ > int i3c_master_unregister(struct i3c_master_controller *master) > { > + i3c_bus_maintenance_lock(&master->bus); > + i3c_master_disable_mr_events(master); > + i3c_bus_maintenance_unlock(&master->bus); > + > i3c_master_i2c_adapter_cleanup(master); > i3c_master_unregister_i3c_devs(master); > i3c_master_bus_cleanup(master); > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index 42bb215..f32afd3 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -421,6 +421,12 @@ struct i3c_bus { > * for a future IBI > * This method is mandatory only if ->request_ibi is not > * NULL. > + * @request_mastership: requests bus mastership. Mastership is requested > + * automatically when device driver wants to transfer > + * data using master in slave mode. ^ a master that does not currently owns the bus > + * @enable_mr_events: enable the Mastership event. > + * Mastership does not require handler. I don't understand this comment. Also, I think it deserves some extra details, like what drivers are supposed to do here (send ENEC(MR) + possibly enable auto-ack?) > + * @disable_mr_events: disable the Mastership event. Same here, we need extra details. > */ > struct i3c_master_controller_ops { > int (*bus_init)(struct i3c_master_controller *master); > @@ -447,6 +453,10 @@ struct i3c_master_controller_ops { > int (*disable_ibi)(struct i3c_dev_desc *dev); > void (*recycle_ibi_slot)(struct i3c_dev_desc *dev, > struct i3c_ibi_slot *slot); > + void (*update_devs)(struct i3c_master_controller *master); This one is no longer used AFAICS. > + int (*request_mastership)(struct i3c_master_controller *master); > + int (*enable_mr_events)(struct i3c_master_controller *master); > + int (*disable_mr_events)(struct i3c_master_controller *master); > }; > > /** > @@ -526,6 +536,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master); > int i3c_master_get_free_addr(struct i3c_master_controller *master, > u8 start_addr); > > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, > + u16 addr, u8 lvr); > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > u8 addr); > int i3c_master_do_daa(struct i3c_master_controller *master); > @@ -538,6 +550,11 @@ int i3c_master_register(struct i3c_master_controller *master, > const struct i3c_master_controller_ops *ops, > bool secondary); > int i3c_master_unregister(struct i3c_master_controller *master); > +int i3c_master_get_accmst_locked(struct i3c_master_controller *master, > + u8 addr); > +int i3c_master_mastership_ack(struct i3c_master_controller *master, > + u8 addr); > +void i3c_master_register_new_devs(struct i3c_master_controller *master); > > /** > * i3c_dev_get_master_data() - get master private data attached to an I3C
Hi Przemek, On 10/03/19 13:58, Przemyslaw Gaj wrote: > This patch adds support for mastership request to I3C subsystem. > > Mastership event is enabled globally. > > Mastership is requested automatically when device driver > tries to transfer data using master controller in slave mode. > > There is still some limitation: > - I2C devices are registered on secondary master side if boardinfo > entry matching the info transmitted through the DEFSLVS frame. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > --- > > Main changes between v3 and v4: > - Add i3c_master_acquire_bus_ownership to acquire the bus > - Refactored the code > > Main changes between v2 and v3: > - Add i3c_bus_downgrade_maintenance_lock() for downgrading the bus > lock from maintenance to normal use > - Add additional fields to i2c_dev_desc for DEFSLVS purpose (addr, lvr) > - Add i3c_master_register_new_i2c_devs() function to register I2C devices > - Reworked I2C devices registration on secondary master side > > Changes in v2: > - Add mastership disable event hook > - Changed name of mastership enable event hook > - Add function to test if master owns the bus > - Add function to test if master owns the bus > - Removed op_mode > - Changed parameter of i3c_master_get_accmst_locked, no need to > pass full i3c_device_info > - Changed name of mastership enable event hook > - Add function to test if master owns the bus > - Removed op_mode > - Changed parameter of i3c_master_get_accmst_locked, no need to > pass full i3c_device_info > - Removed redundant DEFSLVS command before GETACCMST > - Add i3c_master_bus_takeover function. There is a need to lock > the bus before adding devices and no matter of the controller > devices have to be added after mastership takeover. > - Add device registration during initialization on secondary master > side. Devices received by DEFSLVS (if occured). If not, device > initialization is deffered untill next mastership request. > --- > drivers/i3c/device.c | 26 +++ > drivers/i3c/internals.h | 4 + > drivers/i3c/master.c | 417 +++++++++++++++++++++++++++++++++++++-------- > include/linux/i3c/master.h | 17 ++ > 4 files changed, 391 insertions(+), 73 deletions(-) > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > index 69cc040..b60f637 100644 > --- a/drivers/i3c/device.c > +++ b/drivers/i3c/device.c > @@ -43,7 +43,13 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > } > > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers); > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > > return ret; > @@ -114,11 +120,17 @@ int i3c_device_enable_ibi(struct i3c_device *dev) > int ret = -ENOENT; > > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > if (dev->desc) { > mutex_lock(&dev->desc->ibi_lock); > ret = i3c_dev_enable_ibi_locked(dev->desc); > mutex_unlock(&dev->desc->ibi_lock); > } > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > > return ret; > @@ -145,11 +157,17 @@ int i3c_device_request_ibi(struct i3c_device *dev, > return -EINVAL; > > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > if (dev->desc) { > mutex_lock(&dev->desc->ibi_lock); > ret = i3c_dev_request_ibi_locked(dev->desc, req); > mutex_unlock(&dev->desc->ibi_lock); > } > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > > return ret; > @@ -166,12 +184,20 @@ EXPORT_SYMBOL_GPL(i3c_device_request_ibi); > */ > void i3c_device_free_ibi(struct i3c_device *dev) > { > + int ret; > + > i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > + if (ret) > + goto err_unlock_bus; > + > if (dev->desc) { > mutex_lock(&dev->desc->ibi_lock); > i3c_dev_free_ibi_locked(dev->desc); > mutex_unlock(&dev->desc->ibi_lock); > } > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(dev->bus); > } > EXPORT_SYMBOL_GPL(i3c_device_free_ibi); Can't we verify if it is the current master no master.c side? > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > index 86b7b44..929ca6b 100644 > --- a/drivers/i3c/internals.h > +++ b/drivers/i3c/internals.h > @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type; > > void i3c_bus_normaluse_lock(struct i3c_bus *bus); > void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus); > +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master); > +int i3c_master_request_mastership_locked(struct i3c_master_controller *master); > + > > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > struct i3c_priv_xfer *xfers, > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index aea4309..7a84158 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > up_read(&bus->lock); > } > > +/** > + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal > + * operation > + * @bus: I3C bus to downgrade the lock on > + * > + * Should be called when a maintenance operation is done and normal > + * operation is planned. See i3c_bus_maintenance_lock() and > + * i3c_bus_normaluse_lock() for more details. > + */ > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus) > +{ > + downgrade_write(&bus->lock); > +} > + > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > { > return container_of(dev, struct i3c_master_controller, dev); > @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev) > return driver->probe(i3cdev); > } > > +static void i3c_master_enable_mr_events(struct i3c_master_controller *master) > +{ > + if (!master->ops->enable_mr_events) > + return; > + > + master->ops->enable_mr_events(master); > +} > + > +static void i3c_master_disable_mr_events(struct i3c_master_controller *master) > +{ > + if (!master->ops->disable_mr_events) > + return; > + > + master->ops->disable_mr_events(master); > +} > + do we want to do this in broadcast? Is it save to give mastership capabilities to all devices? > static int i3c_device_remove(struct device *dev) > { > struct i3c_device *i3cdev = dev_to_i3cdev(dev); > @@ -462,6 +492,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus) > return 0; > } > > +int i3c_master_request_mastership_locked(struct i3c_master_controller *master) > +{ > + if (WARN_ON(master->init_done && > + !rwsem_is_locked(&master->bus.lock))) > + return -EINVAL; > + > + if (!master->ops->request_mastership) > + return -ENOTSUPP; > + > + return master->ops->request_mastership(master); > +} > + > +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master) > +{ > + int ret; > + > + if (master->bus.cur_master != master->this) { > + i3c_bus_normaluse_unlock(&master->bus); > + i3c_bus_maintenance_lock(&master->bus); > + > + ret = i3c_master_request_mastership_locked(master); > + if (ret) { > + i3c_bus_maintenance_unlock(&master->bus); > + return ret; > + } > + i3c_bus_downgrade_maintenance_lock(&master->bus); > + } > + return 0; > +} > + > static const char * const i3c_bus_mode_strings[] = { > [I3C_BUS_MODE_PURE] = "pure", > [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", > @@ -620,6 +680,25 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master, > > dev->common.master = master; > dev->boardinfo = boardinfo; > + dev->addr = boardinfo->base.addr; > + dev->lvr = boardinfo->lvr; > + > + return dev; > +} > + > +static struct i2c_dev_desc * > +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master, > + u16 addr, u8 lvr) > +{ > + struct i2c_dev_desc *dev; > + > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) > + return ERR_PTR(-ENOMEM); > + > + dev->common.master = master; > + dev->addr = addr; > + dev->lvr = lvr; > > return dev; > } > @@ -693,6 +772,9 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master, > struct i2c_dev_desc *dev; > > i3c_bus_for_each_i2cdev(&master->bus, dev) { > + if (!dev->boardinfo) > + continue; > + > if (dev->boardinfo->base.addr == addr) > return dev; > } > @@ -939,8 +1021,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master) > > desc = defslvs->slaves; > i3c_bus_for_each_i2cdev(bus, i2cdev) { > - desc->lvr = i2cdev->boardinfo->lvr; > - desc->static_addr = i2cdev->boardinfo->base.addr << 1; > + desc->lvr = i2cdev->lvr; > + desc->static_addr = i2cdev->addr << 1; > desc++; > } > > @@ -1492,6 +1574,83 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > } > } > > +static struct i2c_dev_boardinfo * > +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master, > + u16 addr, u8 lvr) > +{ > + struct i2c_dev_boardinfo *i2cboardinfo; > + > + list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { > + if (i2cboardinfo->base.addr == addr && > + i2cboardinfo->lvr == lvr) > + return i2cboardinfo; > + } > + > + return NULL; > +} > + > +static void > +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master) > +{ > + struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master); > + struct i2c_dev_desc *i2cdev; > + > + if (!master->init_done) > + return; > + > + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) { > + if (i2cdev->dev) > + continue; > + > + if (!i2cdev->boardinfo) > + continue; > + > + i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base); > + } > +} > + > +/** > + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command > + * @master: master used to send frames on the bus > + * @info: I3C device information > + * > + * Send a GETACCMST CCC command. > + * > + * This should be called if the curent master acknowledges bus takeover. > + * > + * This function must be called with the bus lock held in write mode. > + * > + * Return: 0 in case of success, a positive I3C error code if the error is > + * one of the official Mx error codes, and a negative error code otherwise. > + */ > +int i3c_master_get_accmst_locked(struct i3c_master_controller *master, > + u8 addr) > +{ > + struct i3c_ccc_getaccmst *accmst; > + struct i3c_ccc_cmd_dest dest; > + struct i3c_ccc_cmd cmd; > + int ret; > + > + accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst)); > + if (!accmst) > + return -ENOMEM; > + > + i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1); > + > + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > + if (ret) > + goto out; > + > + if (dest.payload.len != sizeof(*accmst)) > + ret = -EIO; > + > +out: > + i3c_ccc_cmd_dest_cleanup(&dest); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked); > + > /** > * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) > * @master: master doing the DAA > @@ -1559,10 +1718,6 @@ int i3c_master_set_info(struct i3c_master_controller *master, > if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr)) > return -EINVAL; > > - if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER && > - master->secondary) > - return -EINVAL; > - > if (master->this) > return -EINVAL; > > @@ -1607,43 +1762,13 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) > common.node) { > i3c_master_detach_i2c_dev(i2cdev); > i3c_bus_set_addr_slot_status(&master->bus, > - i2cdev->boardinfo->base.addr, > - I3C_ADDR_SLOT_FREE); > + i2cdev->addr, > + I3C_ADDR_SLOT_FREE); > i3c_master_free_i2c_dev(i2cdev); > } > } > > -/** > - * i3c_master_bus_init() - initialize an I3C bus > - * @master: main master initializing the bus > - * > - * 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 > - * > - * 2. Call &i3c_master_controller_ops->bus_init() method to initialize > - * the master controller. That's usually where the bus mode is selected > - * (pure bus or mixed fast/slow bus) > - * > - * 3. Instruct all devices on the bus to drop their dynamic address. This is > - * particularly important when the bus was previously configured by someone > - * else (for example the bootloader) > - * > - * 4. Disable all slave events. > - * > - * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C > - * devices that have a static address > - * > - * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all > - * remaining I3C devices > - * > - * Once this is done, all I3C and I2C devices should be usable. > - * > - * Return: a 0 in case of success, an negative error code otherwise. > - */ > -static int i3c_master_bus_init(struct i3c_master_controller *master) > +static int i3c_master_attach_static_devs(struct i3c_master_controller *master) > { > enum i3c_addr_slot_status status; > struct i2c_dev_boardinfo *i2cboardinfo; > @@ -1652,32 +1777,24 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > struct i2c_dev_desc *i2cdev; > int ret; > > - /* > - * First attach all devices with static definitions provided by the > - * FW. > - */ > list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { > status = i3c_bus_get_addr_slot_status(&master->bus, > i2cboardinfo->base.addr); > - if (status != I3C_ADDR_SLOT_FREE) { > - ret = -EBUSY; > - goto err_detach_devs; > - } > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > > i3c_bus_set_addr_slot_status(&master->bus, > i2cboardinfo->base.addr, > I3C_ADDR_SLOT_I2C_DEV); > > i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo); > - if (IS_ERR(i2cdev)) { > - ret = PTR_ERR(i2cdev); > - goto err_detach_devs; > - } > + if (IS_ERR(i2cdev)) > + return PTR_ERR(i2cdev); > > ret = i3c_master_attach_i2c_dev(master, i2cdev); > if (ret) { > i3c_master_free_i2c_dev(i2cdev); > - goto err_detach_devs; > + return ret; > } > } > list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { > @@ -1687,28 +1804,71 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > 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; > - } > + i3cboardinfo->init_dyn_addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > } > > i3cdev = i3c_master_alloc_i3c_dev(master, &info); > - if (IS_ERR(i3cdev)) { > - ret = PTR_ERR(i3cdev); > - goto err_detach_devs; > - } > + if (IS_ERR(i3cdev)) > + return PTR_ERR(i3cdev); > > i3cdev->boardinfo = i3cboardinfo; > > ret = i3c_master_attach_i3c_dev(master, i3cdev); > if (ret) { > i3c_master_free_i3c_dev(i3cdev); > - goto err_detach_devs; > + return ret; > } > } > > + return 0; > +} > + > +/** > + * i3c_master_bus_init() - initialize an I3C bus > + * @master: main master initializing the bus > + * > + * 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 > + * > + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize > + * the master controller. That's usually where the bus mode is selected > + * (pure bus or mixed fast/slow bus) > + * > + * 3. Instruct all devices on the bus to drop their dynamic address. This is > + * particularly important when the bus was previously configured by someone > + * else (for example the bootloader) > + * > + * 4. Disable all slave events. > + * > + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C > + * devices that have a static address > + * > + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all > + * remaining I3C devices > + * > + * Once this is done, all I3C and I2C devices should be usable. > + * > + * Return: a 0 in case of success, an negative error code otherwise. > + */ > +static int i3c_master_bus_init(struct i3c_master_controller *master) > +{ > + struct i3c_dev_desc *i3cdev; > + int ret; > + > + /* > + * First attach all devices with static definitions provided by the > + * FW. > + */ > + if (!master->secondary) { > + ret = i3c_master_attach_static_devs(master); > + if (ret) > + goto err_detach_devs; > + } When do we get the i3c_dev_info()? > /* > * Now execute the controller specific ->bus_init() routine, which > * might configure its internal logic to match the bus limitations. > @@ -1729,6 +1889,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > } > > /* > + * Don't reset addresses if this is secondary master. > + * Secondary masters can't do DAA. > + */ > + if (master->secondary) > + return 0; > + > + /* > * Reset all dynamic address that may have been assigned before > * (assigned by the bootloader for example). > */ > @@ -1791,6 +1958,49 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) > return NULL; > } > > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, > + u16 addr, u8 lvr) > +{ > + enum i3c_addr_slot_status status; > + struct i2c_dev_desc *i2cdev; > + int ret; > + > + if (!master) > + return -EINVAL; > + > + status = i3c_bus_get_addr_slot_status(&master->bus, > + addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > + > + i3c_bus_set_addr_slot_status(&master->bus, addr, > + I3C_ADDR_SLOT_I2C_DEV); > + > + i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr); > + if (IS_ERR(i2cdev)) { > + ret = PTR_ERR(i2cdev); > + goto err_free_dev; > + } > + > + i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr); > + > + ret = i3c_master_attach_i2c_dev(master, i2cdev); > + if (ret) { > + ret = PTR_ERR(i2cdev); > + goto err_free_dev; > + } > + > + return 0; > + > +err_free_dev: > + i3c_bus_set_addr_slot_status(&master->bus, addr, > + I3C_ADDR_SLOT_FREE); > + i3c_master_free_i2c_dev(i2cdev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev); > + > /** > * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus > * @master: master used to send frames on the bus > @@ -2113,11 +2323,17 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, > } > > i3c_bus_normaluse_lock(&master->bus); > + ret = i3c_master_acquire_bus_ownership(master); > + if (ret) > + goto err_unlock_bus; > + > dev = i3c_master_find_i2c_dev_by_addr(master, addr); > if (!dev) > ret = -ENOENT; > else > ret = master->ops->i2c_xfers(dev, xfers, nxfers); > + > +err_unlock_bus: > i3c_bus_normaluse_unlock(&master->bus); > > return ret ? ret : nxfers; > @@ -2151,8 +2367,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master) > * We silently ignore failures here. The bus should keep working > * correctly even if one or more i2c devices are not registered. > */ > - i3c_bus_for_each_i2cdev(&master->bus, i2cdev) > + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) { > + if (!i2cdev->boardinfo) > + continue; > + > i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base); > + } > > return 0; > } > @@ -2393,18 +2613,43 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) > !ops->recycle_ibi_slot)) > return -EINVAL; > > + if (ops->request_mastership && > + (!ops->enable_mr_events || !ops->disable_mr_events)) > + return -EINVAL; > + > return 0; > } > > /** > + * i3c_master_register_new_devs() - register new devices > + * @master: master used to send frames on the bus > + * > + * This function is useful when devices were not added > + * during initialization or when new device joined the bus > + * which was under control of different master. > + */ > +void i3c_master_register_new_devs(struct i3c_master_controller *master) > +{ > + /* > + * We can register I3C devices received from master by DEFSLVS. > + */ > + i3c_bus_normaluse_lock(&master->bus); > + i3c_master_register_new_i3c_devs(master); > + i3c_bus_normaluse_unlock(&master->bus); > + > + i3c_bus_normaluse_lock(&master->bus); > + i3c_master_register_new_i2c_devs(master); > + i3c_bus_normaluse_unlock(&master->bus); > +} > +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs); > + I would say a function to add DEVSLVS. From the master driver you can pack all received slaves into i3c_ccc_defslvs struct and rely the task of add those devices to the subsystem. > +/** > * i3c_master_register() - register an I3C master > * @master: master used to send frames on the bus > * @parent: the parent device (the one that provides this I3C master > * controller) > * @ops: the master controller operations > - * @secondary: true if you are registering a secondary master. Will return > - * -ENOTSUPP if set to true since secondary masters are not yet > - * supported > + * @secondary: true if you are registering a secondary master. > * > * This function takes care of everything for you: > * > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller *master, > struct i2c_dev_boardinfo *i2cbi; > int ret; > > - /* We do not support secondary masters yet. */ > - if (secondary) > - return -ENOTSUPP; > - > ret = i3c_master_check_ops(ops); > if (ret) > return ret; > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller *master, > * register I3C devices dicovered during the initial DAA. > */ > master->init_done = true; > - i3c_bus_normaluse_lock(&master->bus); > - i3c_master_register_new_i3c_devs(master); > - i3c_bus_normaluse_unlock(&master->bus); > + i3c_master_register_new_devs(master); > + > + i3c_bus_maintenance_lock(&master->bus); > + i3c_master_enable_mr_events(master); > + i3c_bus_maintenance_unlock(&master->bus); > > return 0; > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller *master, > EXPORT_SYMBOL_GPL(i3c_master_register); > > /** > + * i3c_master_mastership_ack() - performs operations before bus handover. > + * @master: master used to send frames on the bus > + * @addr: I3C device address > + * > + * Basically, it sends DEFSLVS command to ensure new master is taking > + * the bus with complete list of devices and then acknowledges bus takeover. > + * > + * Return: 0 in case of success, a negative error code otherwise. > + */ > +int i3c_master_mastership_ack(struct i3c_master_controller *master, > + u8 addr) > +{ > + int ret; > + > + i3c_bus_maintenance_lock(&master->bus); > + ret = i3c_master_get_accmst_locked(master, addr); > + i3c_bus_maintenance_unlock(&master->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); > + > + > +/** > * i3c_master_unregister() - unregister an I3C master > * @master: master used to send frames on the bus > * > @@ -2533,6 +2800,10 @@ EXPORT_SYMBOL_GPL(i3c_master_register); > */ > int i3c_master_unregister(struct i3c_master_controller *master) > { > + i3c_bus_maintenance_lock(&master->bus); > + i3c_master_disable_mr_events(master); > + i3c_bus_maintenance_unlock(&master->bus); > + > i3c_master_i2c_adapter_cleanup(master); > i3c_master_unregister_i3c_devs(master); > i3c_master_bus_cleanup(master); > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index 42bb215..f32afd3 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -421,6 +421,12 @@ struct i3c_bus { > * for a future IBI > * This method is mandatory only if ->request_ibi is not > * NULL. > + * @request_mastership: requests bus mastership. Mastership is requested > + * automatically when device driver wants to transfer > + * data using master in slave mode. > + * @enable_mr_events: enable the Mastership event. > + * Mastership does not require handler. > + * @disable_mr_events: disable the Mastership event. > */ > struct i3c_master_controller_ops { > int (*bus_init)(struct i3c_master_controller *master); > @@ -447,6 +453,10 @@ struct i3c_master_controller_ops { > int (*disable_ibi)(struct i3c_dev_desc *dev); > void (*recycle_ibi_slot)(struct i3c_dev_desc *dev, > struct i3c_ibi_slot *slot); > + void (*update_devs)(struct i3c_master_controller *master); > + int (*request_mastership)(struct i3c_master_controller *master); > + int (*enable_mr_events)(struct i3c_master_controller *master); > + int (*disable_mr_events)(struct i3c_master_controller *master); > }; > > /** > @@ -526,6 +536,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master); > int i3c_master_get_free_addr(struct i3c_master_controller *master, > u8 start_addr); > > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, > + u16 addr, u8 lvr); > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > u8 addr); > int i3c_master_do_daa(struct i3c_master_controller *master); > @@ -538,6 +550,11 @@ int i3c_master_register(struct i3c_master_controller *master, > const struct i3c_master_controller_ops *ops, > bool secondary); > int i3c_master_unregister(struct i3c_master_controller *master); > +int i3c_master_get_accmst_locked(struct i3c_master_controller *master, > + u8 addr); > +int i3c_master_mastership_ack(struct i3c_master_controller *master, > + u8 addr); > +void i3c_master_register_new_devs(struct i3c_master_controller *master); > > /** > * i3c_dev_get_master_data() - get master private data attached to an I3C Best regards, Vitor Soares
On Mon, 1 Apr 2019 19:41:39 +0100 vitor <vitor.soares@synopsys.com> wrote: > > void i3c_device_free_ibi(struct i3c_device *dev) > > { > > + int ret; > > + > > i3c_bus_normaluse_lock(dev->bus); > > + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); > > + if (ret) > > + goto err_unlock_bus; > > + > > if (dev->desc) { > > mutex_lock(&dev->desc->ibi_lock); > > i3c_dev_free_ibi_locked(dev->desc); > > mutex_unlock(&dev->desc->ibi_lock); > > } > > + > > +err_unlock_bus: > > i3c_bus_normaluse_unlock(dev->bus); > > } > > EXPORT_SYMBOL_GPL(i3c_device_free_ibi); > > Can't we verify if it is the current master no master.c side? You mean acquiring the bus in i3c_dev_free_ibi_locked()? It doesn't work if we want things to play well with lockdep (which tracks locking order to make sure deadlocks can't happen). To acquire the bus you need to take the bus lock in write/maintenance mode and downgrade it to a read/normaluse lock once you're done. If you release/acquire the lock inside i3c_dev_free_ibi_locked() that means some path will have LOCK(bus); LOCK(ibi); UNLOCK(bus); LOCK(bus); UNLOCK(ibi); UNLOCK(bus); and others LOCK(bus); LOCK(ibi); UNLOCK(ibi); UNLOCK(bus); BTW, it won't make a difference in term of LoC to move that to logic master.c, and the i3c_master_acquire_bus_ownership() helper already hides most of the complexity. > > > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > > index 86b7b44..929ca6b 100644 > > --- a/drivers/i3c/internals.h > > +++ b/drivers/i3c/internals.h > > @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type; > > > > void i3c_bus_normaluse_lock(struct i3c_bus *bus); > > void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus); > > +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master); > > +int i3c_master_request_mastership_locked(struct i3c_master_controller *master); > > + > > > > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > > struct i3c_priv_xfer *xfers, > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > index aea4309..7a84158 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > > up_read(&bus->lock); > > } > > > > +/** > > + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal > > + * operation > > + * @bus: I3C bus to downgrade the lock on > > + * > > + * Should be called when a maintenance operation is done and normal > > + * operation is planned. See i3c_bus_maintenance_lock() and > > + * i3c_bus_normaluse_lock() for more details. > > + */ > > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus) > > +{ > > + downgrade_write(&bus->lock); > > +} > > + > > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > > { > > return container_of(dev, struct i3c_master_controller, dev); > > @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev) > > return driver->probe(i3cdev); > > } > > > > +static void i3c_master_enable_mr_events(struct i3c_master_controller *master) > > +{ > > + if (!master->ops->enable_mr_events) > > + return; > > + > > + master->ops->enable_mr_events(master); > > +} > > + > > +static void i3c_master_disable_mr_events(struct i3c_master_controller *master) > > +{ > > + if (!master->ops->disable_mr_events) > > + return; > > + > > + master->ops->disable_mr_events(master); > > +} > > + > > do we want to do this in broadcast? Is it save to give mastership capabilities to all devices? Good question. Sounds like a policy decision that we should leave to the user (kernel parameter, sysfs iface?). Regarding the i3c_master_controller_ops iface, if we want to allow MR on a per-master basis, that's something we could store in the i3c_dev_desc struct and let masters iterate over this list to decide who they should send ENEC() events to. We probably also want a default policy like reject or accept all. Anyway, those are things we can work on once this patch series has landed. > > /** > > + * i3c_master_register_new_devs() - register new devices > > + * @master: master used to send frames on the bus > > + * > > + * This function is useful when devices were not added > > + * during initialization or when new device joined the bus > > + * which was under control of different master. > > + */ > > +void i3c_master_register_new_devs(struct i3c_master_controller *master) > > +{ > > + /* > > + * We can register I3C devices received from master by DEFSLVS. > > + */ > > + i3c_bus_normaluse_lock(&master->bus); > > + i3c_master_register_new_i3c_devs(master); > > + i3c_bus_normaluse_unlock(&master->bus); > > + > > + i3c_bus_normaluse_lock(&master->bus); > > + i3c_master_register_new_i2c_devs(master); > > + i3c_bus_normaluse_unlock(&master->bus); > > +} > > +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs); > > + > > I would say a function to add DEVSLVS. From the master driver you can pack all > received slaves into i3c_ccc_defslvs struct and rely the task of add those > devices to the subsystem. Unfortunately, controllers might have dedicated logic parsing the DEFSLV frame and populating the device table (AFAIR Cadence controller is working this way). Your solution would imply reconstructing a DEFSLVS frame which I don't think is worth it. If we have several controllers exposing directly the DEFSLVS frames, then we can add an helper to do what you suggest though.
Hi Przemek, From my analyses you do i3c_master_register for secondary master in follow cases: - HC has already an dynamic address and it is the owner of the bus. Or - when it receive MR event enable. Is this correct? From: vitor <soares@synopsys.com> Date: Mon, Apr 01, 2019 at 19:41:39 > Hi Przemek, > > > +/** > > * i3c_master_register() - register an I3C master > > * @master: master used to send frames on the bus > > * @parent: the parent device (the one that provides this I3C master > > * controller) > > * @ops: the master controller operations > > - * @secondary: true if you are registering a secondary master. Will return > > - * -ENOTSUPP if set to true since secondary masters are not yet > > - * supported > > + * @secondary: true if you are registering a secondary master. > > * > > * This function takes care of everything for you: > > * > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller > *master, > > struct i2c_dev_boardinfo *i2cbi; > > int ret; > > > > - /* We do not support secondary masters yet. */ > > - if (secondary) > > - return -ENOTSUPP; > > - > > ret = i3c_master_check_ops(ops); > > if (ret) > > return ret; The bus mode is set before the sec master does i3c_master_add_i3c_dev_locked() and i3c_master_add_i2c_dev() and after that it isn't updated. This can cause troubles for HDR-TS modes and when you have i2c devices INDEX(2) on the bus. > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller > *master, > > * register I3C devices dicovered during the initial DAA. > > */ > > master->init_done = true; > > - i3c_bus_normaluse_lock(&master->bus); > > - i3c_master_register_new_i3c_devs(master); > > - i3c_bus_normaluse_unlock(&master->bus); > > + i3c_master_register_new_devs(master); > > + > > + i3c_bus_maintenance_lock(&master->bus); > > + i3c_master_enable_mr_events(master); Why are you enabling MR events here? As a standard function this might case issues because the master can support ENEC/DISEC commands but not multi-master typologies. > > + i3c_bus_maintenance_unlock(&master->bus); > > > > return 0; > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller > *master, > > EXPORT_SYMBOL_GPL(i3c_master_register); > > I'm thinking if isn't better to instantiate the bus apart and them register the secondary master. In this way you are able to add DEFSLVS even before the HC has enable MR events like it is done with dt devices. Best regards, Vitor Soares
Hi Vitor, The 04/09/2019 14:31, Vitor Soares wrote: > > Hi Przemek, > > From my analyses you do i3c_master_register for secondary master in follow cases: > - HC has already an dynamic address and it is the owner of the bus. Correct. It requests bus ownership before registration, if DA is assigned. > Or > - when it receive MR event enabled Actually, when MR event is enabled and secondary master is not initialized yet. > > Is this correct? > > From: vitor <soares@synopsys.com> > Date: Mon, Apr 01, 2019 at 19:41:39 > > > Hi Przemek, > > > > > +/** > > > * i3c_master_register() - register an I3C master > > > * @master: master used to send frames on the bus > > > * @parent: the parent device (the one that provides this I3C master > > > * controller) > > > * @ops: the master controller operations > > > - * @secondary: true if you are registering a secondary master. Will return > > > - * -ENOTSUPP if set to true since secondary masters are not yet > > > - * supported > > > + * @secondary: true if you are registering a secondary master. > > > * > > > * This function takes care of everything for you: > > > * > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller > > *master, > > > struct i2c_dev_boardinfo *i2cbi; > > > int ret; > > > > > > - /* We do not support secondary masters yet. */ > > > - if (secondary) > > > - return -ENOTSUPP; > > > - > > > ret = i3c_master_check_ops(ops); > > > if (ret) > > > return ret; > > The bus mode is set before the sec master does i3c_master_add_i3c_dev_locked() and > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause troubles for > HDR-TS modes and when you have i2c devices INDEX(2) on the bus. Yes, I see this now. It may happen before HC driver updates device list. I should update device list right before i3c_master_register(). > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller > > *master, > > > * register I3C devices dicovered during the initial DAA. > > > */ > > > master->init_done = true; > > > - i3c_bus_normaluse_lock(&master->bus); > > > - i3c_master_register_new_i3c_devs(master); > > > - i3c_bus_normaluse_unlock(&master->bus); > > > + i3c_master_register_new_devs(master); > > > + > > > + i3c_bus_maintenance_lock(&master->bus); > > > + i3c_master_enable_mr_events(master); > > Why are you enabling MR events here? As a standard function this might case issues > because the master can support ENEC/DISEC commands but not multi-master typologies. I enable it at the end of master registration to be sure that current master is ready for bus mastership relinquish. If controller does not support secondary master role it can just do nothing with it. > > > > + i3c_bus_maintenance_unlock(&master->bus); > > > > > > return 0; > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller > > *master, > > > EXPORT_SYMBOL_GPL(i3c_master_register); > > > > > I'm thinking if isn't better to instantiate the bus apart and them register the secondary master. It looked like that before but we decided to register everything or nothing. > In this way you are able to add DEFSLVS even before the HC has enable MR events like it is done > with dt devices. I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received from current master right after ENTDAA. Could you please explain? > > Best regards, > Vitor Soares > > > > > > > >
Hi Przemek, From: Przemyslaw Gaj <pgaj@cadence.com> Date: Tue, Apr 09, 2019 at 16:20:30 > Hi Vitor, > > The 04/09/2019 14:31, Vitor Soares wrote: > > > > Hi Przemek, > > > > From my analyses you do i3c_master_register for secondary master in follow > cases: > > - HC has already an dynamic address and it is the owner of the bus. > > Correct. It requests bus ownership before registration, if DA is assigned. > > > Or > > - when it receive MR event enabled > > Actually, when MR event is enabled and secondary master is not initialized > yet. I didn't find where you do this verification, can you point me? > > > > > Is this correct? > > > > From: vitor <soares@synopsys.com> > > Date: Mon, Apr 01, 2019 at 19:41:39 > > > > > Hi Przemek, > > > > > > > +/** > > > > * i3c_master_register() - register an I3C master > > > > * @master: master used to send frames on the bus > > > > * @parent: the parent device (the one that provides this I3C master > > > > * controller) > > > > * @ops: the master controller operations > > > > - * @secondary: true if you are registering a secondary master. Will > return > > > > - * -ENOTSUPP if set to true since secondary masters are not yet > > > > - * supported > > > > + * @secondary: true if you are registering a secondary master. > > > > * > > > > * This function takes care of everything for you: > > > > * > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct > i3c_master_controller > > > *master, > > > > struct i2c_dev_boardinfo *i2cbi; > > > > int ret; > > > > > > > > - /* We do not support secondary masters yet. */ > > > > - if (secondary) > > > > - return -ENOTSUPP; > > > > - > > > > ret = i3c_master_check_ops(ops); > > > > if (ret) > > > > return ret; > > > > The bus mode is set before the sec master does > i3c_master_add_i3c_dev_locked() and > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause > troubles for > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus. > > Yes, I see this now. It may happen before HC driver updates device list. I > should update device list right before i3c_master_register(). > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct > i3c_master_controller > > > *master, > > > > * register I3C devices dicovered during the initial DAA. > > > > */ > > > > master->init_done = true; > > > > - i3c_bus_normaluse_lock(&master->bus); > > > > - i3c_master_register_new_i3c_devs(master); > > > > - i3c_bus_normaluse_unlock(&master->bus); > > > > + i3c_master_register_new_devs(master); > > > > + > > > > + i3c_bus_maintenance_lock(&master->bus); > > > > + i3c_master_enable_mr_events(master); > > > > Why are you enabling MR events here? As a standard function this might case > issues > > because the master can support ENEC/DISEC commands but not multi-master > typologies. > > I enable it at the end of master registration to be sure that current master > is > ready for bus mastership relinquish. If controller does not support secondary > master role it can just do nothing with it. I think it isn't good idea to have this here because you are forcing HC driver to verify it. If it is to be done in subsystem side probably it is better to have a master/slave functionalities structure. > > > > > > > + i3c_bus_maintenance_unlock(&master->bus); > > > > > > > > return 0; > > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct > i3c_master_controller > > > *master, > > > > EXPORT_SYMBOL_GPL(i3c_master_register); > > > > > > > > I'm thinking if isn't better to instantiate the bus apart and them register > the secondary master. > > It looked like that before but we decided to register everything or nothing. I see your point, but for the future, slave implementation, we can have a function to instantiate just the bus and another for master or slave. > > > In this way you are able to add DEFSLVS even before the HC has enable MR > events like it is done > > with dt devices. > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > from > current master right after ENTDAA. Could you please explain? So the current master receive an hot join and send the DEFSLVS, when do you add them to the bus? Will you go for the all process to get all dev info and so on? > > > > > Best regards, > > Vitor Soares > > -- > -- > Przemyslaw Gaj > Best regards, Vitor Soares
Hi Vitor, The 04/09/2019 15:46, Vitor Soares wrote: > > Hi Przemek, > > From: Przemyslaw Gaj <pgaj@cadence.com> > Date: Tue, Apr 09, 2019 at 16:20:30 > > > Hi Vitor, > > > > The 04/09/2019 14:31, Vitor Soares wrote: > > > > > > Hi Przemek, > > > > > > From my analyses you do i3c_master_register for secondary master in follow > > cases: > > > - HC has already an dynamic address and it is the owner of the bus. > > > > Correct. It requests bus ownership before registration, if DA is assigned. > > > > > Or > > > - when it receive MR event enabled > > > > Actually, when MR event is enabled and secondary master is not initialized > > yet. > > I didn't find where you do this verification, can you point me? In the driver, cdns_i3c_sec_master_event_up(). I'm requesting mastership and registering the master when event is enabled and master is not registered yet. > > > > > > > > > Is this correct? > > > > > > From: vitor <soares@synopsys.com> > > > Date: Mon, Apr 01, 2019 at 19:41:39 > > > > > > > Hi Przemek, > > > > > > > > > +/** > > > > > * i3c_master_register() - register an I3C master > > > > > * @master: master used to send frames on the bus > > > > > * @parent: the parent device (the one that provides this I3C master > > > > > * controller) > > > > > * @ops: the master controller operations > > > > > - * @secondary: true if you are registering a secondary master. Will > > return > > > > > - * -ENOTSUPP if set to true since secondary masters are not yet > > > > > - * supported > > > > > + * @secondary: true if you are registering a secondary master. > > > > > * > > > > > * This function takes care of everything for you: > > > > > * > > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct > > i3c_master_controller > > > > *master, > > > > > struct i2c_dev_boardinfo *i2cbi; > > > > > int ret; > > > > > > > > > > - /* We do not support secondary masters yet. */ > > > > > - if (secondary) > > > > > - return -ENOTSUPP; > > > > > - > > > > > ret = i3c_master_check_ops(ops); > > > > > if (ret) > > > > > return ret; > > > > > > The bus mode is set before the sec master does > > i3c_master_add_i3c_dev_locked() and > > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause > > troubles for > > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus. > > > > Yes, I see this now. It may happen before HC driver updates device list. I > > should update device list right before i3c_master_register(). > > > > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct > > i3c_master_controller > > > > *master, > > > > > * register I3C devices dicovered during the initial DAA. > > > > > */ > > > > > master->init_done = true; > > > > > - i3c_bus_normaluse_lock(&master->bus); > > > > > - i3c_master_register_new_i3c_devs(master); > > > > > - i3c_bus_normaluse_unlock(&master->bus); > > > > > + i3c_master_register_new_devs(master); > > > > > + > > > > > + i3c_bus_maintenance_lock(&master->bus); > > > > > + i3c_master_enable_mr_events(master); > > > > > > Why are you enabling MR events here? As a standard function this might case > > issues > > > because the master can support ENEC/DISEC commands but not multi-master > > typologies. > > > > I enable it at the end of master registration to be sure that current master > > is > > ready for bus mastership relinquish. If controller does not support secondary > > master role it can just do nothing with it. > > I think it isn't good idea to have this here because you are forcing HC driver to > verify it. If it is to be done in subsystem side probably it is better to have a > master/slave functionalities structure. If multi-master topology is not supported, request_mastership() hook won't be implemented and also you will not implement enable/disable_mr_events() hooks. You don't have to verify it in the driver. > > > > > > > > > > > + i3c_bus_maintenance_unlock(&master->bus); > > > > > > > > > > return 0; > > > > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct > > i3c_master_controller > > > > *master, > > > > > EXPORT_SYMBOL_GPL(i3c_master_register); > > > > > > > > > > > I'm thinking if isn't better to instantiate the bus apart and them register > > the secondary master. > > > > It looked like that before but we decided to register everything or nothing. > > I see your point, but for the future, slave implementation, we can have a > function to instantiate just the bus and another for master or slave. > We can go the same way with slave, and register bus and slave at once. If this is the case. > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > events like it is done > > > with dt devices. > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > from > > current master right after ENTDAA. Could you please explain? > > So the current master receive an hot join and send the DEFSLVS, when do you > add them to the bus? Will you go for the all process to get all dev info and so on? > When current master receives an hot-join, do ENTDAA to enumerate device which joined the bus and then sends DEFSLVS. This data is stored in secondary master controller and if secondary master can request mastership, collects all required informations and adds the devices. It has to collect PID, DEFSLVS does not provide this information. > > > > > > > > Best regards, > > > Vitor Soares > > > > -- > > -- > > Przemyslaw Gaj > > > > Best regards, > Vitor Soares >
Hi Przemek, From: Przemyslaw Gaj <pgaj@cadence.com> Date: Wed, Apr 10, 2019 at 07:53:40 > Hi Vitor, > > The 04/09/2019 15:46, Vitor Soares wrote: > > > > Hi Przemek, > > > > From: Przemyslaw Gaj <pgaj@cadence.com> > > Date: Tue, Apr 09, 2019 at 16:20:30 > > > > > Hi Vitor, > > > > > > The 04/09/2019 14:31, Vitor Soares wrote: > > > > > > > > Hi Przemek, > > > > > > > > From my analyses you do i3c_master_register for secondary master in follow > > > cases: > > > > - HC has already an dynamic address and it is the owner of the bus. > > > > > > Correct. It requests bus ownership before registration, if DA is assigned. > > > > > > > Or > > > > - when it receive MR event enabled > > > > > > Actually, when MR event is enabled and secondary master is not initialized > > > yet. > > > > I didn't find where you do this verification, can you point me? > > In the driver, cdns_i3c_sec_master_event_up(). I'm requesting mastership and > registering the master when event is enabled and master is not registered yet. > Sorry, I really don't see that. I assume it is some logic in the controller. > > > > > > > > > > > > > Is this correct? > > > > > > > > From: vitor <soares@synopsys.com> > > > > Date: Mon, Apr 01, 2019 at 19:41:39 > > > > > > > > > Hi Przemek, > > > > > > > > > > > +/** > > > > > > * i3c_master_register() - register an I3C master > > > > > > * @master: master used to send frames on the bus > > > > > > * @parent: the parent device (the one that provides this I3C master > > > > > > * controller) > > > > > > * @ops: the master controller operations > > > > > > - * @secondary: true if you are registering a secondary master. Will > > > return > > > > > > - * -ENOTSUPP if set to true since secondary masters are not yet > > > > > > - * supported > > > > > > + * @secondary: true if you are registering a secondary master. > > > > > > * > > > > > > * This function takes care of everything for you: > > > > > > * > > > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct > > > i3c_master_controller > > > > > *master, > > > > > > struct i2c_dev_boardinfo *i2cbi; > > > > > > int ret; > > > > > > > > > > > > - /* We do not support secondary masters yet. */ > > > > > > - if (secondary) > > > > > > - return -ENOTSUPP; > > > > > > - > > > > > > ret = i3c_master_check_ops(ops); > > > > > > if (ret) > > > > > > return ret; > > > > > > > > The bus mode is set before the sec master does > > > i3c_master_add_i3c_dev_locked() and > > > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause > > > troubles for > > > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus. > > > > > > Yes, I see this now. It may happen before HC driver updates device list. I > > > should update device list right before i3c_master_register(). > > > > > > > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct > > > i3c_master_controller > > > > > *master, > > > > > > * register I3C devices dicovered during the initial DAA. > > > > > > */ > > > > > > master->init_done = true; > > > > > > - i3c_bus_normaluse_lock(&master->bus); > > > > > > - i3c_master_register_new_i3c_devs(master); > > > > > > - i3c_bus_normaluse_unlock(&master->bus); > > > > > > + i3c_master_register_new_devs(master); > > > > > > + > > > > > > + i3c_bus_maintenance_lock(&master->bus); > > > > > > + i3c_master_enable_mr_events(master); > > > > > > > > Why are you enabling MR events here? As a standard function this might case > > > issues > > > > because the master can support ENEC/DISEC commands but not multi-master > > > typologies. > > > > > > I enable it at the end of master registration to be sure that current master > > > is > > > ready for bus mastership relinquish. If controller does not support secondary > > > master role it can just do nothing with it. > > > > I think it isn't good idea to have this here because you are forcing HC driver to > > verify it. If it is to be done in subsystem side probably it is better to have a > > master/slave functionalities structure. > > If multi-master topology is not supported, request_mastership() hook won't be > implemented and also you will not implement enable/disable_mr_events() hooks. > You don't have to verify it in the driver. In that case we will need a driver for each role/set of available features and duplicate the code otherwise this need to be checked in HC side. > > > > > > > > > > > > > > > > + i3c_bus_maintenance_unlock(&master->bus); > > > > > > > > > > > > return 0; > > > > > > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct > > > i3c_master_controller > > > > > *master, > > > > > > EXPORT_SYMBOL_GPL(i3c_master_register); > > > > > > > > > > > > > > I'm thinking if isn't better to instantiate the bus apart and them register > > > the secondary master. > > > > > > It looked like that before but we decided to register everything or nothing. > > > > I see your point, but for the future, slave implementation, we can have a > > function to instantiate just the bus and another for master or slave. > > > > We can go the same way with slave, and register bus and slave at once. If this > is the case. What if the slave is a secondary master? In your opinion what should be the flow? > > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > > events like it is done > > > > with dt devices. > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > > from > > > current master right after ENTDAA. Could you please explain? > > > > So the current master receive an hot join and send the DEFSLVS, when do you > > add them to the bus? Will you go for the all process to get all dev info and so on? > > > > When current master receives an hot-join, do ENTDAA to enumerate device which > joined the bus and then sends DEFSLVS. This data is stored in secondary master > controller and if secondary master can request mastership, collects all > required informations and adds the devices. It has to collect PID, DEFSLVS does > not provide this information. I see now. You need to take care here because when the secondary master add the i3c_dev it might change the address. This is one of the reasons I would prefer a dedicated function to add the DEFSLVS. Another point is what happen if the current master received MR request during this registration? Best regards, Vitor Soares
On Wed, 10 Apr 2019 10:05:33 +0000 Vitor Soares <vitor.soares@synopsys.com> wrote: > > > > > > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct > > > > i3c_master_controller > > > > > > *master, > > > > > > > * register I3C devices dicovered during the initial DAA. > > > > > > > */ > > > > > > > master->init_done = true; > > > > > > > - i3c_bus_normaluse_lock(&master->bus); > > > > > > > - i3c_master_register_new_i3c_devs(master); > > > > > > > - i3c_bus_normaluse_unlock(&master->bus); > > > > > > > + i3c_master_register_new_devs(master); > > > > > > > + > > > > > > > + i3c_bus_maintenance_lock(&master->bus); > > > > > > > + i3c_master_enable_mr_events(master); > > > > > > > > > > Why are you enabling MR events here? As a standard function this might case > > > > issues > > > > > because the master can support ENEC/DISEC commands but not multi-master > > > > typologies. > > > > > > > > I enable it at the end of master registration to be sure that current master > > > > is > > > > ready for bus mastership relinquish. If controller does not support secondary > > > > master role it can just do nothing with it. > > > > > > I think it isn't good idea to have this here because you are forcing HC driver to > > > verify it. If it is to be done in subsystem side probably it is better to have a > > > master/slave functionalities structure. > > > > If multi-master topology is not supported, request_mastership() hook won't be > > implemented and also you will not implement enable/disable_mr_events() hooks. > > You don't have to verify it in the driver. > > In that case we will need a driver for each role/set of available > features and duplicate the code otherwise this need to be checked in HC > side. Not really, just 2 set of ops, one with the hooks set and another one with the hooks left unassigned (NULL). But I guess we can also add a caps field where we'd list i3c master caps (secondary master, supports MR, ...). > > > > > > > > > > > > > > > > > > > > > > + i3c_bus_maintenance_unlock(&master->bus); > > > > > > > > > > > > > > return 0; > > > > > > > > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct > > > > i3c_master_controller > > > > > > *master, > > > > > > > EXPORT_SYMBOL_GPL(i3c_master_register); > > > > > > > > > > > > > > > > > I'm thinking if isn't better to instantiate the bus apart and them register > > > > the secondary master. > > > > > > > > It looked like that before but we decided to register everything or nothing. > > > > > > I see your point, but for the future, slave implementation, we can have a > > > function to instantiate just the bus and another for master or slave. > > > > > > > We can go the same way with slave, and register bus and slave at once. If this > > is the case. The slave framework isn't there yet, and I don't think we should expose the bus concept to slave drivers anyway. If a master can act both as a slave (I mean a slave that can do more than just send MR requests) and a master (secondary), the driver should register to both framework. > > What if the slave is a secondary master? In your opinion what should be > the flow? i3c_slave_regiter(&ctrl->slave); i3c_master_regiter(&ctrl->master); The order is arbitrary, and those might actually be called from different path if it makes more sense. I'm just pointing out that registering a slave and registering a master are 2 different things, and the master side of the framework should probably not automate slave registration, at least not until we have a better idea of what the slave API will look like. > > > > > > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > > > events like it is done > > > > > with dt devices. > > > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > > > from > > > > current master right after ENTDAA. Could you please explain? > > > > > > So the current master receive an hot join and send the DEFSLVS, when do you > > > add them to the bus? Will you go for the all process to get all dev info and so on? > > > > > > > When current master receives an hot-join, do ENTDAA to enumerate device which > > joined the bus and then sends DEFSLVS. This data is stored in secondary master > > controller and if secondary master can request mastership, collects all > > required informations and adds the devices. It has to collect PID, DEFSLVS does > > not provide this information. > > I see now. You need to take care here because when the secondary master > add the i3c_dev it might change the address. > This is one of the reasons I would prefer a dedicated function to add the > DEFSLVS. IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is automatically filled based on that. We should only add the DEFSLVS parsing helper once we start seeing a need for it (probably when adding secondary slave support since you seem to insist on this aspect :-)). > > Another point is what happen if the current master received MR request > during this registration? You mean when registering the primary I3C master? The driver should take care of disabling MR interrupts and if possible reject all incoming MR. MR should only be re-enabled when ->enable_mr_events() is called by the core.
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Wed, Apr 10, 2019 at 11:36:03 > On Wed, 10 Apr 2019 10:05:33 +0000 > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > > > > > > > > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct > > > > > i3c_master_controller > > > > > > > *master, > > > > > > > > * register I3C devices dicovered during the initial DAA. > > > > > > > > */ > > > > > > > > master->init_done = true; > > > > > > > > - i3c_bus_normaluse_lock(&master->bus); > > > > > > > > - i3c_master_register_new_i3c_devs(master); > > > > > > > > - i3c_bus_normaluse_unlock(&master->bus); > > > > > > > > + i3c_master_register_new_devs(master); > > > > > > > > + > > > > > > > > + i3c_bus_maintenance_lock(&master->bus); > > > > > > > > + i3c_master_enable_mr_events(master); > > > > > > > > > > > > Why are you enabling MR events here? As a standard function this might case > > > > > issues > > > > > > because the master can support ENEC/DISEC commands but not multi-master > > > > > typologies. > > > > > > > > > > I enable it at the end of master registration to be sure that current master > > > > > is > > > > > ready for bus mastership relinquish. If controller does not support secondary > > > > > master role it can just do nothing with it. > > > > > > > > I think it isn't good idea to have this here because you are forcing HC driver to > > > > verify it. If it is to be done in subsystem side probably it is better to have a > > > > master/slave functionalities structure. > > > > > > If multi-master topology is not supported, request_mastership() hook won't be > > > implemented and also you will not implement enable/disable_mr_events() hooks. > > > You don't have to verify it in the driver. > > > > In that case we will need a driver for each role/set of available > > features and duplicate the code otherwise this need to be checked in HC > > side. > > Not really, just 2 set of ops, one with the hooks set and another one > with the hooks left unassigned (NULL). But I guess we can also add > a caps field where we'd list i3c master caps (secondary master, > supports MR, ...). Yes we can do this, but what I see is to have several ops structures based on HC capabilities. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + i3c_bus_maintenance_unlock(&master->bus); > > > > > > > > > > > > > > > > return 0; > > > > > > > > > > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct > > > > > i3c_master_controller > > > > > > > *master, > > > > > > > > EXPORT_SYMBOL_GPL(i3c_master_register); > > > > > > > > > > > > > > > > > > > > I'm thinking if isn't better to instantiate the bus apart and them register > > > > > the secondary master. > > > > > > > > > > It looked like that before but we decided to register everything or nothing. > > > > > > > > I see your point, but for the future, slave implementation, we can have a > > > > function to instantiate just the bus and another for master or slave. > > > > > > > > > > We can go the same way with slave, and register bus and slave at once. If this > > > is the case. > > The slave framework isn't there yet, and I don't think we should expose > the bus concept to slave drivers anyway. If a master can act both as a > slave (I mean a slave that can do more than just send MR requests) and > a master (secondary), the driver should register to both framework. > > > > > What if the slave is a secondary master? In your opinion what should be > > the flow? > > i3c_slave_regiter(&ctrl->slave); > i3c_master_regiter(&ctrl->master); > > The order is arbitrary, and those might actually be called from > different path if it makes more sense. I'm just pointing out that > registering a slave and registering a master are 2 different things, > and the master side of the framework should probably not automate slave > registration, at least not until we have a better idea of what the > slave API will look like. We need to understand better your point. The secondary master can have both roles and even not implementing all slave function it is a slave when is not a master. For me still keep the idea: bus / \ / \ slave master and just one role is active at time. I didn't get why the bus shouldn't be instantiated for slave. Can you explain? In any case we will need a common point to switch the roles. > > > > > > > > > > > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > > > > events like it is done > > > > > > with dt devices. > > > > > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > > > > from > > > > > current master right after ENTDAA. Could you please explain? > > > > > > > > So the current master receive an hot join and send the DEFSLVS, when do you > > > > add them to the bus? Will you go for the all process to get all dev info and so on? > > > > > > > > > > When current master receives an hot-join, do ENTDAA to enumerate device which > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master > > > controller and if secondary master can request mastership, collects all > > > required informations and adds the devices. It has to collect PID, DEFSLVS does > > > not provide this information. > > > > I see now. You need to take care here because when the secondary master > > add the i3c_dev it might change the address. > > This is one of the reasons I would prefer a dedicated function to add the > > DEFSLVS. > > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is > automatically filled based on that. We should only add the DEFSLVS > parsing helper once we start seeing a need for it (probably when adding > secondary slave support since you seem to insist on this aspect :-)). For me the subsystem should hold and handle all this information. Anyway, this information is received and needed before the mastership takeover. Basically I want to avoid to put this type of logistics in HC driver layer and call the maintenance_lock. I'm insisting because it seems that I have a different use case to address and I don't see how this fit on it. > > > > > Another point is what happen if the current master received MR request > > during this registration? > > You mean when registering the primary I3C master? The driver should > take care of disabling MR interrupts and if possible reject all > incoming MR. MR should only be re-enabled when ->enable_mr_events() is > called by the core. I'm aware of that I just didn't see any disable events. Best regards, Vitor Soares
On Fri, 12 Apr 2019 14:28:26 +0000 Vitor Soares <vitor.soares@synopsys.com> wrote: > > > What if the slave is a secondary master? In your opinion what should be > > > the flow? > > > > i3c_slave_regiter(&ctrl->slave); > > i3c_master_regiter(&ctrl->master); > > > > The order is arbitrary, and those might actually be called from > > different path if it makes more sense. I'm just pointing out that > > registering a slave and registering a master are 2 different things, > > and the master side of the framework should probably not automate slave > > registration, at least not until we have a better idea of what the > > slave API will look like. > > We need to understand better your point. > > The secondary master can have both roles and even not implementing all > slave function it is a slave when is not a master. > For me still keep the idea: > > bus > / \ > / \ > slave master > > and just one role is active at time. > > I didn't get why the bus shouldn't be instantiated for slave. Can you > explain? Because, from a SW PoV, pure slave devices don't care about the bus concept. Do you have use cases where you'd need to know what bus the slave is connected to? AFAICT, all you can do is reply to master requests (probably with some predefined messages, like values stored in a regmap or data queued in a FIFO). > > In any case we will need a common point to switch the roles. We'd need a way to relinquish bus ownership, that's all. When the master is not the current master, it automatically becomes a slave, and if it has any "I3C slave" profile registered, it can reply to requests coming from the current master. > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > > > > > events like it is done > > > > > > > with dt devices. > > > > > > > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > > > > > from > > > > > > current master right after ENTDAA. Could you please explain? > > > > > > > > > > So the current master receive an hot join and send the DEFSLVS, when do you > > > > > add them to the bus? Will you go for the all process to get all dev info and so on? > > > > > > > > > > > > > When current master receives an hot-join, do ENTDAA to enumerate device which > > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master > > > > controller and if secondary master can request mastership, collects all > > > > required informations and adds the devices. It has to collect PID, DEFSLVS does > > > > not provide this information. > > > > > > I see now. You need to take care here because when the secondary master > > > add the i3c_dev it might change the address. > > > This is one of the reasons I would prefer a dedicated function to add the > > > DEFSLVS. > > > > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is > > automatically filled based on that. We should only add the DEFSLVS > > parsing helper once we start seeing a need for it (probably when adding > > secondary slave support since you seem to insist on this aspect :-)). > > For me the subsystem should hold and handle all this information. Anyway, > this information is received and needed before the mastership takeover. It is, and we already have a bunch of helpers to add new devices. Maybe we need a few more, I'm just saying that forging a DEFSLVS frame to then pass it to the core is not the right solution IMO. If you need an helper that automatically parses a DEFSLVS frame and add the new devices, then fine, add this helper to the framework and use it in your driver, but don't force other drivers to use this method. > Basically I want to avoid to put this type of logistics in HC driver > layer and call the maintenance_lock. And yet, I don't think forging a DEFSLVS frame is the right way to provide the kind of abstraction you're talking about. Note that I said a few things should be provided as helpers in my review. > > I'm insisting because it seems that I have a different use case to > address and I don't see how this fit on it. Can you be more specific, because we don't know about your use cases.
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Fri, Apr 12, 2019 at 15:57:19 > On Fri, 12 Apr 2019 14:28:26 +0000 > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > > > What if the slave is a secondary master? In your opinion what should be > > > > the flow? > > > > > > i3c_slave_regiter(&ctrl->slave); > > > i3c_master_regiter(&ctrl->master); > > > > > > The order is arbitrary, and those might actually be called from > > > different path if it makes more sense. I'm just pointing out that > > > registering a slave and registering a master are 2 different things, > > > and the master side of the framework should probably not automate slave > > > registration, at least not until we have a better idea of what the > > > slave API will look like. > > > > We need to understand better your point. > > > > The secondary master can have both roles and even not implementing all > > slave function it is a slave when is not a master. > > For me still keep the idea: > > > > bus > > / \ > > / \ > > slave master > > > > and just one role is active at time. > > > > I didn't get why the bus shouldn't be instantiated for slave. Can you > > explain? > > Because, from a SW PoV, pure slave devices don't care about the bus > concept. Do you have use cases where you'd need to know what bus the > slave is connected to? > In device model I expect that the slave is under a bus. You may not need to know the bus topologies and the devices connect but at least we might need to know the bus is alive. The I2C subsystem implement that and it is working. I still don't understand your position, is there any other reasons? > AFAICT, all you can do is reply to master requests (probably with some > predefined messages, like values stored in a regmap or data queued in > a FIFO). We can list it: - RX/TX SDR data - RX/TX HDR data - SIR - HJ - MR (case of sec master) - CCC - and perhaps timing control. > > > > > In any case we will need a common point to switch the roles. > > We'd need a way to relinquish bus ownership, that's all. When the > master is not the current master, it automatically becomes a slave, and > if it has any "I3C slave" profile registered, it can reply to requests > coming from the current master. This is too abstract to me. Can you point a current example? Because for me we need a common layer for both master slave structures. > > > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > > > > > > events like it is done > > > > > > > > with dt devices. > > > > > > > > > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > > > > > > from > > > > > > > current master right after ENTDAA. Could you please explain? > > > > > > > > > > > > So the current master receive an hot join and send the DEFSLVS, when do you > > > > > > add them to the bus? Will you go for the all process to get all dev info and so on? > > > > > > > > > > > > > > > > When current master receives an hot-join, do ENTDAA to enumerate device which > > > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master > > > > > controller and if secondary master can request mastership, collects all > > > > > required informations and adds the devices. It has to collect PID, DEFSLVS does > > > > > not provide this information. > > > > > > > > I see now. You need to take care here because when the secondary master > > > > add the i3c_dev it might change the address. > > > > This is one of the reasons I would prefer a dedicated function to add the > > > > DEFSLVS. > > > > > > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is > > > automatically filled based on that. We should only add the DEFSLVS > > > parsing helper once we start seeing a need for it (probably when adding > > > secondary slave support since you seem to insist on this aspect :-)). > > > > For me the subsystem should hold and handle all this information. Anyway, > > this information is received and needed before the mastership takeover. > > It is, and we already have a bunch of helpers to add new devices. Maybe > we need a few more, I'm just saying that forging a DEFSLVS frame to then > pass it to the core is not the right solution IMO. > If you need an helper > that automatically parses a DEFSLVS frame and add the new devices, then > fine, add this helper to the framework and use it in your driver, but > don't force other drivers to use this method. This is not based in a need. I can also add the devices one by one. If I have everything available why not send it straight away to the subsystem and let it do the management? But ok, I understand your point and I can do the helper to parse the DEFSLVS. Another question: How do we know what is the main master in case we want to send back the bus mastership? > > > Basically I want to avoid to put this type of logistics in HC driver > > layer and call the maintenance_lock. > > And yet, I don't think forging a DEFSLVS frame is the right way to > provide the kind of abstraction you're talking about. Can you explain? > Note that I said > a few things should be provided as helpers in my review. > > > > > I'm insisting because it seems that I have a different use case to > > address and I don't see how this fit on it. > > Can you be more specific, because we don't know about your use cases. According to the spec the sec master may have all function of a Main master and may have all function of a slave. The implementation should made with this in mind and I'm asking flexibility for that. Best regards, Vitor Soares
Hi Vitor, On Mon, 15 Apr 2019 12:02:02 +0000 Vitor Soares <vitor.soares@synopsys.com> wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Fri, Apr 12, > 2019 at 15:57:19 > > > On Fri, 12 Apr 2019 14:28:26 +0000 > > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > > > > > What if the slave is a secondary master? In your opinion what should be > > > > > the flow? > > > > > > > > i3c_slave_regiter(&ctrl->slave); > > > > i3c_master_regiter(&ctrl->master); > > > > > > > > The order is arbitrary, and those might actually be called from > > > > different path if it makes more sense. I'm just pointing out that > > > > registering a slave and registering a master are 2 different things, > > > > and the master side of the framework should probably not automate slave > > > > registration, at least not until we have a better idea of what the > > > > slave API will look like. > > > > > > We need to understand better your point. > > > > > > The secondary master can have both roles and even not implementing all > > > slave function it is a slave when is not a master. > > > For me still keep the idea: > > > > > > bus > > > / \ > > > / \ > > > slave master > > > > > > and just one role is active at time. > > > > > > I didn't get why the bus shouldn't be instantiated for slave. Can you > > > explain? > > > > Because, from a SW PoV, pure slave devices don't care about the bus > > concept. Do you have use cases where you'd need to know what bus the > > slave is connected to? > > > > In device model I expect that the slave is under a bus. Attached to a bus_type, maybe, under an I3C bus object, I'm not convinced. > You may not need > to know the bus topologies and the devices connect but at least we might > need to know the bus is alive. Yes, AFAICT, the slave status (and potentially the I3C dynamic address) are the only thing an I3C slave controller driver might need. > > The I2C subsystem implement that and it is working. But is it needed? That's what I'm questioning here. What would you put in the bus object if the only thing you know about it is that the slave controller is present on the bus. Does it make sense to re-use the same logic/struct for the slave case? Maybe my feeling about this problem is wrong, but as usual, I don't see a real use case expressed or a PoC that shows me why you'd need to have an i3c_bus attached to the slave. So it's hard to say who's right/wrong. If you come up with an RFC for the slave API we'll have something concrete to discuss about. Until this happens, I'd like to close this topic. And of course, I'd appreciate that we keep focusing on the mastership handover/secondary master case without thinking too much about how things will be impacted by the slave API. > > I still don't understand your position, is there any other reasons? My position is: I don't see the point of adding complex concepts to something that's otherwise way simpler than managing an I3C bus. You can prove me wrong with a detailed example, or even better, a patch series adding the slave API plus a reference driver to show how it's supposed to work :P. > > > AFAICT, all you can do is reply to master requests (probably with some > > predefined messages, like values stored in a regmap or data queued in > > a FIFO). > > We can list it: > - RX/TX SDR data > - RX/TX HDR data > - SIR > - HJ > - MR (case of sec master) Yes, but that case falls in the I3C master API. > - CCC > - and perhaps timing control. > > > > > > > > > In any case we will need a common point to switch the roles. > > > > We'd need a way to relinquish bus ownership, that's all. When the > > master is not the current master, it automatically becomes a slave, and > > if it has any "I3C slave" profile registered, it can reply to requests > > coming from the current master. > > This is too abstract to me. Can you point a current example? Because for > me we need a common layer for both master slave structures. We need to share a few things (CCC defs and probably other things, but I don't know which ones yet), yes, but I don't think the i3c_bus struct is one of them. I also don't know what the slave API will look like, and that's actually something someone (you??) should work on before having advanced discussions about what has to be shared between the master/slave layers. > > > > > > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR > > > > > > > > events like it is done > > > > > > > > > with dt devices. > > > > > > > > > > > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received > > > > > > > > from > > > > > > > > current master right after ENTDAA. Could you please explain? > > > > > > > > > > > > > > So the current master receive an hot join and send the DEFSLVS, when do you > > > > > > > add them to the bus? Will you go for the all process to get all dev info and so on? > > > > > > > > > > > > > > > > > > > When current master receives an hot-join, do ENTDAA to enumerate device which > > > > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master > > > > > > controller and if secondary master can request mastership, collects all > > > > > > required informations and adds the devices. It has to collect PID, DEFSLVS does > > > > > > not provide this information. > > > > > > > > > > I see now. You need to take care here because when the secondary master > > > > > add the i3c_dev it might change the address. > > > > > This is one of the reasons I would prefer a dedicated function to add the > > > > > DEFSLVS. > > > > > > > > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is > > > > automatically filled based on that. We should only add the DEFSLVS > > > > parsing helper once we start seeing a need for it (probably when adding > > > > secondary slave support since you seem to insist on this aspect :-)). > > > > > > For me the subsystem should hold and handle all this information. Anyway, > > > this information is received and needed before the mastership takeover. > > > > It is, and we already have a bunch of helpers to add new devices. Maybe > > we need a few more, I'm just saying that forging a DEFSLVS frame to then > > pass it to the core is not the right solution IMO. > > If you need an helper > > that automatically parses a DEFSLVS frame and add the new devices, then > > fine, add this helper to the framework and use it in your driver, but > > don't force other drivers to use this method. > > This is not based in a need. I can also add the devices one by one. > If I have everything available why not send it straight away to the > subsystem and let it do the management? This is exactly what I recommend you to do: provide an helper that will automate that, but don't force other drivers to use this helper. Since the Cadence driver will not use this helper, it should not be introduced in this patch series, that's all I said. > But ok, I understand your point and I can do the helper to parse the > DEFSLVS. > > Another question: > How do we know what is the main master in case we want to send back the > bus mastership? There's a ->cur_master field in struct i3c_bus. It should be updated when the bus owner changes. Not sure exactly how other masters can be informed about that, but those doing the handshake know about the change, that's for sure. > > > > > > Basically I want to avoid to put this type of logistics in HC driver > > > layer and call the maintenance_lock. > > > > And yet, I don't think forging a DEFSLVS frame is the right way to > > provide the kind of abstraction you're talking about. > > Can you explain? I keep explaining you that reconstructing a fake DEFSLVS frame just for the sake of having all drivers use the helper you want to add is not a good idea. To sum-up one last time: adding the i3c_master_parse_defslvs_and_add_devs() helper is fine, forcing all drivers to use it is not. Is that clear enough? > > > Note that I said > > a few things should be provided as helpers in my review. > > > > > > > > I'm insisting because it seems that I have a different use case to > > > address and I don't see how this fit on it. > > > > Can you be more specific, because we don't know about your use cases. > > According to the spec the sec master may have all function of a Main > master and may have all function of a slave. So you're back to the master/slave dual role thing? > > The implementation should made with this in mind and I'm asking > flexibility for that. What you're asking for is the opposite of flexibility, and most importantly it's based on assumptions about something that has not been fully designed yet: the I3C slave API. So let's have a plan for this discussion to actually lead to something useful: 1/ propose an RFC for the slave API + a reference driver 2/ think about how dual role devs can implement both interfaces and the things they need to share See, we keep discussing #2 while #1 is not even ready yet, and it's definitely a pre-requisite to #2. So please work #1 first. Regards, Boris
diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c index 69cc040..b60f637 100644 --- a/drivers/i3c/device.c +++ b/drivers/i3c/device.c @@ -43,7 +43,13 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, } i3c_bus_normaluse_lock(dev->bus); + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); + if (ret) + goto err_unlock_bus; + ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers); + +err_unlock_bus: i3c_bus_normaluse_unlock(dev->bus); return ret; @@ -114,11 +120,17 @@ int i3c_device_enable_ibi(struct i3c_device *dev) int ret = -ENOENT; i3c_bus_normaluse_lock(dev->bus); + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); + if (ret) + goto err_unlock_bus; + if (dev->desc) { mutex_lock(&dev->desc->ibi_lock); ret = i3c_dev_enable_ibi_locked(dev->desc); mutex_unlock(&dev->desc->ibi_lock); } + +err_unlock_bus: i3c_bus_normaluse_unlock(dev->bus); return ret; @@ -145,11 +157,17 @@ int i3c_device_request_ibi(struct i3c_device *dev, return -EINVAL; i3c_bus_normaluse_lock(dev->bus); + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); + if (ret) + goto err_unlock_bus; + if (dev->desc) { mutex_lock(&dev->desc->ibi_lock); ret = i3c_dev_request_ibi_locked(dev->desc, req); mutex_unlock(&dev->desc->ibi_lock); } + +err_unlock_bus: i3c_bus_normaluse_unlock(dev->bus); return ret; @@ -166,12 +184,20 @@ EXPORT_SYMBOL_GPL(i3c_device_request_ibi); */ void i3c_device_free_ibi(struct i3c_device *dev) { + int ret; + i3c_bus_normaluse_lock(dev->bus); + ret = i3c_master_acquire_bus_ownership(dev->desc->common.master); + if (ret) + goto err_unlock_bus; + if (dev->desc) { mutex_lock(&dev->desc->ibi_lock); i3c_dev_free_ibi_locked(dev->desc); mutex_unlock(&dev->desc->ibi_lock); } + +err_unlock_bus: i3c_bus_normaluse_unlock(dev->bus); } EXPORT_SYMBOL_GPL(i3c_device_free_ibi); diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h index 86b7b44..929ca6b 100644 --- a/drivers/i3c/internals.h +++ b/drivers/i3c/internals.h @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type; void i3c_bus_normaluse_lock(struct i3c_bus *bus); void i3c_bus_normaluse_unlock(struct i3c_bus *bus); +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus); +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master); +int i3c_master_request_mastership_locked(struct i3c_master_controller *master); + int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index aea4309..7a84158 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) up_read(&bus->lock); } +/** + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal + * operation + * @bus: I3C bus to downgrade the lock on + * + * Should be called when a maintenance operation is done and normal + * operation is planned. See i3c_bus_maintenance_lock() and + * i3c_bus_normaluse_lock() for more details. + */ +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus) +{ + downgrade_write(&bus->lock); +} + static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) { return container_of(dev, struct i3c_master_controller, dev); @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev) return driver->probe(i3cdev); } +static void i3c_master_enable_mr_events(struct i3c_master_controller *master) +{ + if (!master->ops->enable_mr_events) + return; + + master->ops->enable_mr_events(master); +} + +static void i3c_master_disable_mr_events(struct i3c_master_controller *master) +{ + if (!master->ops->disable_mr_events) + return; + + master->ops->disable_mr_events(master); +} + static int i3c_device_remove(struct device *dev) { struct i3c_device *i3cdev = dev_to_i3cdev(dev); @@ -462,6 +492,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus) return 0; } +int i3c_master_request_mastership_locked(struct i3c_master_controller *master) +{ + if (WARN_ON(master->init_done && + !rwsem_is_locked(&master->bus.lock))) + return -EINVAL; + + if (!master->ops->request_mastership) + return -ENOTSUPP; + + return master->ops->request_mastership(master); +} + +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master) +{ + int ret; + + if (master->bus.cur_master != master->this) { + i3c_bus_normaluse_unlock(&master->bus); + i3c_bus_maintenance_lock(&master->bus); + + ret = i3c_master_request_mastership_locked(master); + if (ret) { + i3c_bus_maintenance_unlock(&master->bus); + return ret; + } + i3c_bus_downgrade_maintenance_lock(&master->bus); + } + return 0; +} + static const char * const i3c_bus_mode_strings[] = { [I3C_BUS_MODE_PURE] = "pure", [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", @@ -620,6 +680,25 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master, dev->common.master = master; dev->boardinfo = boardinfo; + dev->addr = boardinfo->base.addr; + dev->lvr = boardinfo->lvr; + + return dev; +} + +static struct i2c_dev_desc * +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master, + u16 addr, u8 lvr) +{ + struct i2c_dev_desc *dev; + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) + return ERR_PTR(-ENOMEM); + + dev->common.master = master; + dev->addr = addr; + dev->lvr = lvr; return dev; } @@ -693,6 +772,9 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master, struct i2c_dev_desc *dev; i3c_bus_for_each_i2cdev(&master->bus, dev) { + if (!dev->boardinfo) + continue; + if (dev->boardinfo->base.addr == addr) return dev; } @@ -939,8 +1021,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master) desc = defslvs->slaves; i3c_bus_for_each_i2cdev(bus, i2cdev) { - desc->lvr = i2cdev->boardinfo->lvr; - desc->static_addr = i2cdev->boardinfo->base.addr << 1; + desc->lvr = i2cdev->lvr; + desc->static_addr = i2cdev->addr << 1; desc++; } @@ -1492,6 +1574,83 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) } } +static struct i2c_dev_boardinfo * +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master, + u16 addr, u8 lvr) +{ + struct i2c_dev_boardinfo *i2cboardinfo; + + list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { + if (i2cboardinfo->base.addr == addr && + i2cboardinfo->lvr == lvr) + return i2cboardinfo; + } + + return NULL; +} + +static void +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master) +{ + struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master); + struct i2c_dev_desc *i2cdev; + + if (!master->init_done) + return; + + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) { + if (i2cdev->dev) + continue; + + if (!i2cdev->boardinfo) + continue; + + i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base); + } +} + +/** + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command + * @master: master used to send frames on the bus + * @info: I3C device information + * + * Send a GETACCMST CCC command. + * + * This should be called if the curent master acknowledges bus takeover. + * + * This function must be called with the bus lock held in write mode. + * + * Return: 0 in case of success, a positive I3C error code if the error is + * one of the official Mx error codes, and a negative error code otherwise. + */ +int i3c_master_get_accmst_locked(struct i3c_master_controller *master, + u8 addr) +{ + struct i3c_ccc_getaccmst *accmst; + struct i3c_ccc_cmd_dest dest; + struct i3c_ccc_cmd cmd; + int ret; + + accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst)); + if (!accmst) + return -ENOMEM; + + i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1); + + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); + if (ret) + goto out; + + if (dest.payload.len != sizeof(*accmst)) + ret = -EIO; + +out: + i3c_ccc_cmd_dest_cleanup(&dest); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked); + /** * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) * @master: master doing the DAA @@ -1559,10 +1718,6 @@ int i3c_master_set_info(struct i3c_master_controller *master, if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr)) return -EINVAL; - if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER && - master->secondary) - return -EINVAL; - if (master->this) return -EINVAL; @@ -1607,43 +1762,13 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master) common.node) { i3c_master_detach_i2c_dev(i2cdev); i3c_bus_set_addr_slot_status(&master->bus, - i2cdev->boardinfo->base.addr, - I3C_ADDR_SLOT_FREE); + i2cdev->addr, + I3C_ADDR_SLOT_FREE); i3c_master_free_i2c_dev(i2cdev); } } -/** - * i3c_master_bus_init() - initialize an I3C bus - * @master: main master initializing the bus - * - * 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 - * - * 2. Call &i3c_master_controller_ops->bus_init() method to initialize - * the master controller. That's usually where the bus mode is selected - * (pure bus or mixed fast/slow bus) - * - * 3. Instruct all devices on the bus to drop their dynamic address. This is - * particularly important when the bus was previously configured by someone - * else (for example the bootloader) - * - * 4. Disable all slave events. - * - * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C - * devices that have a static address - * - * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all - * remaining I3C devices - * - * Once this is done, all I3C and I2C devices should be usable. - * - * Return: a 0 in case of success, an negative error code otherwise. - */ -static int i3c_master_bus_init(struct i3c_master_controller *master) +static int i3c_master_attach_static_devs(struct i3c_master_controller *master) { enum i3c_addr_slot_status status; struct i2c_dev_boardinfo *i2cboardinfo; @@ -1652,32 +1777,24 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) struct i2c_dev_desc *i2cdev; int ret; - /* - * First attach all devices with static definitions provided by the - * FW. - */ list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) { status = i3c_bus_get_addr_slot_status(&master->bus, i2cboardinfo->base.addr); - if (status != I3C_ADDR_SLOT_FREE) { - ret = -EBUSY; - goto err_detach_devs; - } + if (status != I3C_ADDR_SLOT_FREE) + return -EBUSY; i3c_bus_set_addr_slot_status(&master->bus, i2cboardinfo->base.addr, I3C_ADDR_SLOT_I2C_DEV); i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo); - if (IS_ERR(i2cdev)) { - ret = PTR_ERR(i2cdev); - goto err_detach_devs; - } + if (IS_ERR(i2cdev)) + return PTR_ERR(i2cdev); ret = i3c_master_attach_i2c_dev(master, i2cdev); if (ret) { i3c_master_free_i2c_dev(i2cdev); - goto err_detach_devs; + return ret; } } list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) { @@ -1687,28 +1804,71 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) 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; - } + i3cboardinfo->init_dyn_addr); + if (status != I3C_ADDR_SLOT_FREE) + return -EBUSY; } i3cdev = i3c_master_alloc_i3c_dev(master, &info); - if (IS_ERR(i3cdev)) { - ret = PTR_ERR(i3cdev); - goto err_detach_devs; - } + if (IS_ERR(i3cdev)) + return PTR_ERR(i3cdev); i3cdev->boardinfo = i3cboardinfo; ret = i3c_master_attach_i3c_dev(master, i3cdev); if (ret) { i3c_master_free_i3c_dev(i3cdev); - goto err_detach_devs; + return ret; } } + return 0; +} + +/** + * i3c_master_bus_init() - initialize an I3C bus + * @master: main master initializing the bus + * + * 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 + * + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize + * the master controller. That's usually where the bus mode is selected + * (pure bus or mixed fast/slow bus) + * + * 3. Instruct all devices on the bus to drop their dynamic address. This is + * particularly important when the bus was previously configured by someone + * else (for example the bootloader) + * + * 4. Disable all slave events. + * + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C + * devices that have a static address + * + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all + * remaining I3C devices + * + * Once this is done, all I3C and I2C devices should be usable. + * + * Return: a 0 in case of success, an negative error code otherwise. + */ +static int i3c_master_bus_init(struct i3c_master_controller *master) +{ + struct i3c_dev_desc *i3cdev; + int ret; + + /* + * First attach all devices with static definitions provided by the + * FW. + */ + if (!master->secondary) { + ret = i3c_master_attach_static_devs(master); + if (ret) + goto err_detach_devs; + } /* * Now execute the controller specific ->bus_init() routine, which * might configure its internal logic to match the bus limitations. @@ -1729,6 +1889,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) } /* + * Don't reset addresses if this is secondary master. + * Secondary masters can't do DAA. + */ + if (master->secondary) + return 0; + + /* * Reset all dynamic address that may have been assigned before * (assigned by the bootloader for example). */ @@ -1791,6 +1958,49 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) return NULL; } +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, + u16 addr, u8 lvr) +{ + enum i3c_addr_slot_status status; + struct i2c_dev_desc *i2cdev; + int ret; + + if (!master) + return -EINVAL; + + status = i3c_bus_get_addr_slot_status(&master->bus, + addr); + if (status != I3C_ADDR_SLOT_FREE) + return -EBUSY; + + i3c_bus_set_addr_slot_status(&master->bus, addr, + I3C_ADDR_SLOT_I2C_DEV); + + i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr); + if (IS_ERR(i2cdev)) { + ret = PTR_ERR(i2cdev); + goto err_free_dev; + } + + i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr); + + ret = i3c_master_attach_i2c_dev(master, i2cdev); + if (ret) { + ret = PTR_ERR(i2cdev); + goto err_free_dev; + } + + return 0; + +err_free_dev: + i3c_bus_set_addr_slot_status(&master->bus, addr, + I3C_ADDR_SLOT_FREE); + i3c_master_free_i2c_dev(i2cdev); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev); + /** * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus * @master: master used to send frames on the bus @@ -2113,11 +2323,17 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, } i3c_bus_normaluse_lock(&master->bus); + ret = i3c_master_acquire_bus_ownership(master); + if (ret) + goto err_unlock_bus; + dev = i3c_master_find_i2c_dev_by_addr(master, addr); if (!dev) ret = -ENOENT; else ret = master->ops->i2c_xfers(dev, xfers, nxfers); + +err_unlock_bus: i3c_bus_normaluse_unlock(&master->bus); return ret ? ret : nxfers; @@ -2151,8 +2367,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master) * We silently ignore failures here. The bus should keep working * correctly even if one or more i2c devices are not registered. */ - i3c_bus_for_each_i2cdev(&master->bus, i2cdev) + i3c_bus_for_each_i2cdev(&master->bus, i2cdev) { + if (!i2cdev->boardinfo) + continue; + i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base); + } return 0; } @@ -2393,18 +2613,43 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) !ops->recycle_ibi_slot)) return -EINVAL; + if (ops->request_mastership && + (!ops->enable_mr_events || !ops->disable_mr_events)) + return -EINVAL; + return 0; } /** + * i3c_master_register_new_devs() - register new devices + * @master: master used to send frames on the bus + * + * This function is useful when devices were not added + * during initialization or when new device joined the bus + * which was under control of different master. + */ +void i3c_master_register_new_devs(struct i3c_master_controller *master) +{ + /* + * We can register I3C devices received from master by DEFSLVS. + */ + i3c_bus_normaluse_lock(&master->bus); + i3c_master_register_new_i3c_devs(master); + i3c_bus_normaluse_unlock(&master->bus); + + i3c_bus_normaluse_lock(&master->bus); + i3c_master_register_new_i2c_devs(master); + i3c_bus_normaluse_unlock(&master->bus); +} +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs); + +/** * i3c_master_register() - register an I3C master * @master: master used to send frames on the bus * @parent: the parent device (the one that provides this I3C master * controller) * @ops: the master controller operations - * @secondary: true if you are registering a secondary master. Will return - * -ENOTSUPP if set to true since secondary masters are not yet - * supported + * @secondary: true if you are registering a secondary master. * * This function takes care of everything for you: * @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller *master, struct i2c_dev_boardinfo *i2cbi; int ret; - /* We do not support secondary masters yet. */ - if (secondary) - return -ENOTSUPP; - ret = i3c_master_check_ops(ops); if (ret) return ret; @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller *master, * register I3C devices dicovered during the initial DAA. */ master->init_done = true; - i3c_bus_normaluse_lock(&master->bus); - i3c_master_register_new_i3c_devs(master); - i3c_bus_normaluse_unlock(&master->bus); + i3c_master_register_new_devs(master); + + i3c_bus_maintenance_lock(&master->bus); + i3c_master_enable_mr_events(master); + i3c_bus_maintenance_unlock(&master->bus); return 0; @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller *master, EXPORT_SYMBOL_GPL(i3c_master_register); /** + * i3c_master_mastership_ack() - performs operations before bus handover. + * @master: master used to send frames on the bus + * @addr: I3C device address + * + * Basically, it sends DEFSLVS command to ensure new master is taking + * the bus with complete list of devices and then acknowledges bus takeover. + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_mastership_ack(struct i3c_master_controller *master, + u8 addr) +{ + int ret; + + i3c_bus_maintenance_lock(&master->bus); + ret = i3c_master_get_accmst_locked(master, addr); + i3c_bus_maintenance_unlock(&master->bus); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); + + +/** * i3c_master_unregister() - unregister an I3C master * @master: master used to send frames on the bus * @@ -2533,6 +2800,10 @@ EXPORT_SYMBOL_GPL(i3c_master_register); */ int i3c_master_unregister(struct i3c_master_controller *master) { + i3c_bus_maintenance_lock(&master->bus); + i3c_master_disable_mr_events(master); + i3c_bus_maintenance_unlock(&master->bus); + i3c_master_i2c_adapter_cleanup(master); i3c_master_unregister_i3c_devs(master); i3c_master_bus_cleanup(master); diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h index 42bb215..f32afd3 100644 --- a/include/linux/i3c/master.h +++ b/include/linux/i3c/master.h @@ -421,6 +421,12 @@ struct i3c_bus { * for a future IBI * This method is mandatory only if ->request_ibi is not * NULL. + * @request_mastership: requests bus mastership. Mastership is requested + * automatically when device driver wants to transfer + * data using master in slave mode. + * @enable_mr_events: enable the Mastership event. + * Mastership does not require handler. + * @disable_mr_events: disable the Mastership event. */ struct i3c_master_controller_ops { int (*bus_init)(struct i3c_master_controller *master); @@ -447,6 +453,10 @@ struct i3c_master_controller_ops { int (*disable_ibi)(struct i3c_dev_desc *dev); void (*recycle_ibi_slot)(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot); + void (*update_devs)(struct i3c_master_controller *master); + int (*request_mastership)(struct i3c_master_controller *master); + int (*enable_mr_events)(struct i3c_master_controller *master); + int (*disable_mr_events)(struct i3c_master_controller *master); }; /** @@ -526,6 +536,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master); int i3c_master_get_free_addr(struct i3c_master_controller *master, u8 start_addr); +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, + u16 addr, u8 lvr); int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, u8 addr); int i3c_master_do_daa(struct i3c_master_controller *master); @@ -538,6 +550,11 @@ int i3c_master_register(struct i3c_master_controller *master, const struct i3c_master_controller_ops *ops, bool secondary); int i3c_master_unregister(struct i3c_master_controller *master); +int i3c_master_get_accmst_locked(struct i3c_master_controller *master, + u8 addr); +int i3c_master_mastership_ack(struct i3c_master_controller *master, + u8 addr); +void i3c_master_register_new_devs(struct i3c_master_controller *master); /** * i3c_dev_get_master_data() - get master private data attached to an I3C
This patch adds support for mastership request to I3C subsystem. Mastership event is enabled globally. Mastership is requested automatically when device driver tries to transfer data using master controller in slave mode. There is still some limitation: - I2C devices are registered on secondary master side if boardinfo entry matching the info transmitted through the DEFSLVS frame. Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> --- Main changes between v3 and v4: - Add i3c_master_acquire_bus_ownership to acquire the bus - Refactored the code Main changes between v2 and v3: - Add i3c_bus_downgrade_maintenance_lock() for downgrading the bus lock from maintenance to normal use - Add additional fields to i2c_dev_desc for DEFSLVS purpose (addr, lvr) - Add i3c_master_register_new_i2c_devs() function to register I2C devices - Reworked I2C devices registration on secondary master side Changes in v2: - Add mastership disable event hook - Changed name of mastership enable event hook - Add function to test if master owns the bus - Add function to test if master owns the bus - Removed op_mode - Changed parameter of i3c_master_get_accmst_locked, no need to pass full i3c_device_info - Changed name of mastership enable event hook - Add function to test if master owns the bus - Removed op_mode - Changed parameter of i3c_master_get_accmst_locked, no need to pass full i3c_device_info - Removed redundant DEFSLVS command before GETACCMST - Add i3c_master_bus_takeover function. There is a need to lock the bus before adding devices and no matter of the controller devices have to be added after mastership takeover. - Add device registration during initialization on secondary master side. Devices received by DEFSLVS (if occured). If not, device initialization is deffered untill next mastership request. --- drivers/i3c/device.c | 26 +++ drivers/i3c/internals.h | 4 + drivers/i3c/master.c | 417 +++++++++++++++++++++++++++++++++++++-------- include/linux/i3c/master.h | 17 ++ 4 files changed, 391 insertions(+), 73 deletions(-)