* [PATCH 0/2] Add the I3C mastership request
@ 2018-12-13 12:27 Przemyslaw Gaj
2018-12-13 12:28 ` [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Przemyslaw Gaj @ 2018-12-13 12:27 UTC (permalink / raw)
To: bbrezillon, linux-i3c; +Cc: Przemyslaw Gaj, psroka, rafalc, vitor.soares
This patch series adds support for mastership request to I3C
subsystem and Cadence I3C master driver. Mastership request
allows slave to become the master of the I3C bus.
Przemyslaw Gaj (2):
i3c: Add support for mastership request to I3C subsystem
i3c: master: cdns: add support for mastership request to Cadence I3C
master driver.
drivers/i3c/master.c | 260 ++++++++++++++++++++++++++--
drivers/i3c/master/i3c-master-cdns.c | 316 ++++++++++++++++++++++++++++++-----
include/linux/i3c/master.h | 43 ++++-
3 files changed, 560 insertions(+), 59 deletions(-)
--
2.4.5
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem 2018-12-13 12:27 [PATCH 0/2] Add the I3C mastership request Przemyslaw Gaj @ 2018-12-13 12:28 ` Przemyslaw Gaj 2018-12-13 13:50 ` Boris Brezillon 2018-12-13 12:28 ` [PATCH 2/2] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj 2018-12-13 12:47 ` [PATCH 0/2] Add the I3C mastership request Boris Brezillon 2 siblings, 1 reply; 9+ messages in thread From: Przemyslaw Gaj @ 2018-12-13 12:28 UTC (permalink / raw) To: bbrezillon, linux-i3c; +Cc: Przemyslaw Gaj, psroka, rafalc, vitor.soares This patch adds support for mastership request to I3C subsystem. Mastership event is enabled for all the masters on the I3C bus by default. Mastership request occurs in the following cases: - Mastership is requested automatically after secondary master receives mastership ENEC event. This allows secondary masters to initialize their bus - If above procedure fails for some reason, user can force mastership request through sysfs. Of course, mastership event still has to be enabled on current master side - Mastership is also requested automatically when device driver tries to transfer data using master controller in slave mode. There is still some limitation: - I2C devices are not registered on secondary master side. I2C devices received in DEFSLVS CCC command are added to device list just to send back this information to the other master controllers in case of bus handoff. We can add support for this in the future if there is such use case. Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> --- drivers/i3c/master.c | 260 ++++++++++++++++++++++++++++++++++++++++++--- include/linux/i3c/master.h | 43 +++++++- 2 files changed, 289 insertions(+), 14 deletions(-) diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 68d6553..e98b600 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -460,6 +460,15 @@ static int i3c_bus_init(struct i3c_bus *i3cbus) return 0; } +static int i3c_master_request_mastership(struct i3c_master_controller *master) +{ + if (!master->ops->request_mastership) + return -ENOTSUPP; + if (master->ops->request_mastership(master)) + return -EIO; + return 0; +} + static const char * const i3c_bus_mode_strings[] = { [I3C_BUS_MODE_PURE] = "pure", [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", @@ -491,8 +500,15 @@ static ssize_t current_master_show(struct device *dev, char *buf) { struct i3c_bus *i3cbus = dev_to_i3cbus(dev); + struct i3c_master_controller *master; ssize_t ret; + if (!i3cbus->cur_master) { + master = container_of(i3cbus, struct i3c_master_controller, bus); + if (i3c_master_request_mastership(master)) + return sprintf(buf, "unknown\n"); + } + i3c_bus_normaluse_lock(i3cbus); ret = sprintf(buf, "%d-%llx\n", i3cbus->id, i3cbus->cur_master->info.pid); @@ -659,6 +675,11 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master, if (!cmd || !master) return -EINVAL; + if (master->op_mode == I3C_SLAVE_MODE) { + if (i3c_master_request_mastership(master)) + return -EIO; + } + if (WARN_ON(master->init_done && !rwsem_is_locked(&master->bus.lock))) return -EINVAL; @@ -1371,6 +1392,7 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, dev->info.dyn_addr); if (status != I3C_ADDR_SLOT_FREE) return -EBUSY; + i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr, I3C_ADDR_SLOT_I3C_DEV); @@ -1491,6 +1513,46 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) } /** + * 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, + const struct i3c_device_info *info) +{ + 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, info->dyn_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) + return ret; + + if (dest.payload.len != sizeof(*accmst)) + return -EIO; + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked); + +/** * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) * @master: master doing the DAA * @@ -1554,11 +1616,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, struct i3c_dev_desc *i3cdev; int ret; - 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) + if (master->op_mode == I3C_MASTER_MODE && + !i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr)) return -EINVAL; if (master->this) @@ -1569,7 +1628,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, return PTR_ERR(i3cdev); master->this = i3cdev; - master->bus.cur_master = master->this; + if (master->op_mode == I3C_MASTER_MODE) + master->bus.cur_master = master->this; ret = i3c_master_attach_i3c_dev(master, i3cdev); if (ret) @@ -1727,6 +1787,12 @@ 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). */ @@ -1738,6 +1804,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR, I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ); + if (ret && ret != I3C_ERROR_M2) goto err_bus_cleanup; @@ -1789,6 +1856,44 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) return NULL; } +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, + struct i2c_dev_boardinfo *info) +{ + enum i3c_addr_slot_status status; + struct device *dev = &master->dev; + struct i2c_dev_boardinfo *boardinfo; + struct i2c_dev_desc *i2cdev; + int ret; + + status = i3c_bus_get_addr_slot_status(&master->bus, + info->base.addr); + if (status != I3C_ADDR_SLOT_FREE) + return -EBUSY; + + boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL); + if (!boardinfo) + return -ENOMEM; + + i3c_bus_set_addr_slot_status(&master->bus, + info->base.addr, + I3C_ADDR_SLOT_I2C_DEV); + + boardinfo->base.addr = info->base.addr; + boardinfo->lvr = info->lvr; + + i2cdev = i3c_master_alloc_i2c_dev(master, boardinfo); + if (IS_ERR(i2cdev)) + return PTR_ERR(i2cdev); + + ret = i3c_master_attach_i2c_dev(master, i2cdev); + if (ret) { + i3c_master_free_i2c_dev(i2cdev); + return ret; + } + return 0; +} +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 @@ -1832,6 +1937,15 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, if (ret) goto err_free_dev; + if (I3C_BCR_DEVICE_ROLE(newdev->info.bcr) == I3C_BCR_I3C_MASTER && + master->ops->enable_mastership) { + if (master->ops->enable_mastership(newdev)) { + dev_warn(&master->dev, + "Failed to enable mastership for device: %d%llx", + master->bus.id, newdev->info.pid); + } + } + olddev = i3c_master_search_i3c_dev_duplicate(newdev); if (olddev) { newdev->boardinfo = olddev->boardinfo; @@ -2102,6 +2216,11 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, return -ENOTSUPP; } + if (master->op_mode == I3C_SLAVE_MODE) { + if (i3c_master_request_mastership(master)) + return -EIO; + } + i3c_bus_normaluse_lock(&master->bus); dev = i3c_master_find_i2c_dev_by_addr(master, addr); if (!dev) @@ -2393,15 +2512,31 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) return 0; } +static void i3c_master_bus_takeover(struct work_struct *work) +{ + struct i3c_master_controller *master; + + master = container_of(work, struct i3c_master_controller, mastership); + + i3c_bus_maintenance_lock(&master->bus); + master->ops->update_devs(master); + i3c_bus_maintenance_unlock(&master->bus); + /* + * We can register I3C devices received from master by DEFSLVS. + */ + 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() - 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: * @@ -2424,10 +2559,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; @@ -2439,6 +2570,7 @@ int i3c_master_register(struct i3c_master_controller *master, master->dev.release = i3c_masterdev_release; master->ops = ops; master->secondary = secondary; + master->op_mode = secondary ? I3C_SLAVE_MODE : I3C_MASTER_MODE; INIT_LIST_HEAD(&master->boardinfo.i2c); INIT_LIST_HEAD(&master->boardinfo.i3c); @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master, goto err_del_dev; /* + * We can stop here. Devices will be attached after bus takeover. + * Secondary masters can't do DAA. + */ + if (master->secondary) + return 0; + + /* * We're done initializing the bus and the controller, we can now * register I3C devices dicovered during the initial DAA. */ @@ -2521,6 +2660,95 @@ int i3c_master_register(struct i3c_master_controller *master, EXPORT_SYMBOL_GPL(i3c_master_register); /** + * i3c_master_switch_operation_mode() - changes operation mode of the controller + * @master: master used to send frames on the bus + * @new_master: the device that takes control of the bus + * @op_mode: the master new operation mode + * + * Return: 0 in case of success, a negative error code otherwise. + */ +int i3c_master_switch_operation_mode(struct i3c_master_controller *master, + struct i3c_dev_desc *new_master, + enum i3c_op_mode new_op_mode) +{ + if (master->op_mode == new_op_mode) + return -EEXIST; + + master->op_mode = new_op_mode; + + if (new_op_mode == I3C_SLAVE_MODE) { + master->bus.cur_master = new_master; + } else { + master->bus.cur_master = master->this; + INIT_WORK(&master->mastership, i3c_master_bus_takeover); + queue_work(master->wq, &master->mastership); + } + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode); + +/** + * i3c_master_mastership_ack() - performs operations before bus handover. + * @master: master used to send frames on the bus + * @info: I3C device information + * + * 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, + const struct i3c_device_info *info) +{ + int ret; + + i3c_bus_maintenance_lock(&master->bus); + ret = i3c_master_defslvs_locked(master); + i3c_bus_maintenance_unlock(&master->bus); + if (ret) + return ret; + + i3c_bus_maintenance_lock(&master->bus); + ret = i3c_master_get_accmst_locked(master, info); + i3c_bus_maintenance_unlock(&master->bus); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); + +static void i3c_secondary_master_bus_init(struct work_struct *work) +{ + struct i3c_master_controller *master; + + master = container_of(work, struct i3c_master_controller, mastership); + + if (i3c_master_request_mastership(master)) + dev_err(&master->dev, "Mastership failed."); +} + +/** + * i3c_secondary_master_events_enabled() - event from current master + * @master: master used to send frames on the bus + * @evts: enabled events + * + * This function allows to perform required operations after current + * master enables particular events on the bus. + */ +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, + u8 evts) +{ + if ((evts & I3C_CCC_EVENT_MR) && + !master->init_done) { + INIT_WORK(&master->mastership, i3c_secondary_master_bus_init); + queue_work(master->wq, &master->mastership); + } +} +EXPORT_SYMBOL_GPL(i3c_secondary_master_events_enabled); + +/** * i3c_master_unregister() - unregister an I3C master * @master: master used to send frames on the bus * @@ -2552,6 +2780,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, if (!master || !xfers) return -EINVAL; + if (master->op_mode == I3C_SLAVE_MODE) { + if (i3c_master_request_mastership(master)) + return -EIO; + } + if (!master->ops->priv_xfers) return -ENOTSUPP; @@ -2657,5 +2890,6 @@ static void __exit i3c_exit(void) module_exit(i3c_exit); MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); +MODULE_AUTHOR("Przemyslaw Gaj <pgaj@cadence.com>"); MODULE_DESCRIPTION("I3C core"); MODULE_LICENSE("GPL v2"); diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h index f13fd8b..ada956a 100644 --- a/include/linux/i3c/master.h +++ b/include/linux/i3c/master.h @@ -282,6 +282,16 @@ enum i3c_addr_slot_status { }; /** + * enum i3c_op_mode - I3C controller operation mode + * @I3C_SLAVE_MODE: I3C controller operates in slave mode + * @I3C_MASTER_MODE: I3C controller operates in master mode + */ +enum i3c_op_mode { + I3C_SLAVE_MODE, + I3C_MASTER_MODE +}; + +/** * struct i3c_bus - I3C bus object * @cur_master: I3C master currently driving the bus. Since I3C is multi-master * this can change over the time. Will be used to let a master @@ -418,6 +428,20 @@ struct i3c_bus { * for a future IBI * This method is mandatory only if ->request_ibi is not * NULL. + * @update_devs: updates device list. Called after bus takeover. Secondary + * master can't perform DAA procedure. This function allows to + * update devices received from previous bus owner in DEFSLVS + * command. Useful also when new device joins the bus controlled + * by secondary master, main master will be able to add + * this device after mastership takeover. + * @request_mastership: requests bus mastership. By default called by secondary + * master after ENEC CCC is received and when devices were + * not fully initialized yet. Mastership is also requested + * when device driver wants to transfer data using master + * in slave mode. + * @enable_mastership: enable the Mastership for specified device. Mastership + * does not require handler. Mastership is enabled for all + * masters by default. */ struct i3c_master_controller_ops { int (*bus_init)(struct i3c_master_controller *master); @@ -445,6 +469,9 @@ 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_mastership)(struct i3c_dev_desc *dev); }; /** @@ -458,6 +485,7 @@ struct i3c_master_controller_ops { * @ops: master operations. See &struct i3c_master_controller_ops * @secondary: true if the master is a secondary master * @init_done: true when the bus initialization is done + * @op_mode: controller operation mode * @boardinfo.i3c: list of I3C boardinfo objects * @boardinfo.i2c: list of I2C boardinfo objects * @boardinfo: board-level information attached to devices connected on the bus @@ -467,6 +495,7 @@ struct i3c_master_controller_ops { * in a thread context. Typical examples are Hot Join processing which * requires taking the bus lock in maintenance, which in turn, can only * be done from a sleep-able context + * @mastership: work for switching operation mode after bus takeover * * A &struct i3c_master_controller has to be registered to the I3C subsystem * through i3c_master_register(). None of &struct i3c_master_controller fields @@ -480,12 +509,14 @@ struct i3c_master_controller { const struct i3c_master_controller_ops *ops; unsigned int secondary : 1; unsigned int init_done : 1; + enum i3c_op_mode op_mode; struct { struct list_head i3c; struct list_head i2c; } boardinfo; struct i3c_bus bus; struct workqueue_struct *wq; + struct work_struct mastership; }; /** @@ -523,7 +554,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, + struct i2c_dev_boardinfo *info); int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, u8 addr); int i3c_master_do_daa(struct i3c_master_controller *master); @@ -536,6 +568,15 @@ 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, + const struct i3c_device_info *info); +int i3c_master_switch_operation_mode(struct i3c_master_controller *master, + struct i3c_dev_desc *new_master, + enum i3c_op_mode new_op_mode); +int i3c_master_mastership_ack(struct i3c_master_controller *master, + const struct i3c_device_info *info); +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, + u8 evts); /** * i3c_dev_get_master_data() - get master private data attached to an I3C -- 2.4.5 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem 2018-12-13 12:28 ` [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj @ 2018-12-13 13:50 ` Boris Brezillon 2018-12-14 7:14 ` Przemyslaw Gaj 0 siblings, 1 reply; 9+ messages in thread From: Boris Brezillon @ 2018-12-13 13:50 UTC (permalink / raw) To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares On Thu, 13 Dec 2018 12:28:00 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > This patch adds support for mastership request to I3C subsystem. > > Mastership event is enabled for all the masters on the I3C bus > by default. > > Mastership request occurs in the following cases: > - Mastership is requested automatically after secondary master > receives mastership ENEC event. This allows secondary masters to > initialize their bus I guess this only happens when the secondary master received a DEFSLVS packet, right? > - If above procedure fails for some reason, user can force > mastership request through sysfs. Of course, mastership event > still has to be enabled on current master side Sounds like a good idea. > - Mastership is also requested automatically when device driver > tries to transfer data using master controller in slave mode. And that too. > > There is still some limitation: > - I2C devices are not registered on secondary master side. I2C > devices received in DEFSLVS CCC command are added to device list > just to send back this information to the other master controllers > in case of bus handoff. We can add support for this in the future > if there is such use case. I2C devices must be described under the bus represented by the secondary master for that to work properly (we need a way to bind devices to drivers). So, it's probably a good thing to discard (or leave them inactive) any devices that do not have a i2c_board_info entry on the secondary master side. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > --- > drivers/i3c/master.c | 260 ++++++++++++++++++++++++++++++++++++++++++--- > include/linux/i3c/master.h | 43 +++++++- > 2 files changed, 289 insertions(+), 14 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 68d6553..e98b600 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -460,6 +460,15 @@ static int i3c_bus_init(struct i3c_bus *i3cbus) > return 0; > } > > +static int i3c_master_request_mastership(struct i3c_master_controller *master) > +{ > + if (!master->ops->request_mastership) > + return -ENOTSUPP; Please add a blank line here > + if (master->ops->request_mastership(master)) > + return -EIO; and here. > + return 0; > +} > + > static const char * const i3c_bus_mode_strings[] = { > [I3C_BUS_MODE_PURE] = "pure", > [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", > @@ -491,8 +500,15 @@ static ssize_t current_master_show(struct device *dev, > char *buf) > { > struct i3c_bus *i3cbus = dev_to_i3cbus(dev); > + struct i3c_master_controller *master; > ssize_t ret; > > + if (!i3cbus->cur_master) { > + master = container_of(i3cbus, struct i3c_master_controller, bus); > + if (i3c_master_request_mastership(master)) > + return sprintf(buf, "unknown\n"); > + } Hm, why is that needed? When you are a secondary master ->cur_master should be set to the i3c_dev_desc representing the main master at init time. > + > i3c_bus_normaluse_lock(i3cbus); > ret = sprintf(buf, "%d-%llx\n", i3cbus->id, > i3cbus->cur_master->info.pid); > @@ -659,6 +675,11 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master, > if (!cmd || !master) > return -EINVAL; > > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) > + return -EIO; > + } > + This shouldn't be needed either. if i3cbus->cur_master != master->this, that means the master is operating in slave mode. > if (WARN_ON(master->init_done && > !rwsem_is_locked(&master->bus.lock))) > return -EINVAL; > @@ -1371,6 +1392,7 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, > dev->info.dyn_addr); > if (status != I3C_ADDR_SLOT_FREE) > return -EBUSY; > + Drop this change (or make it part of a separate patch fixing coding style issues). > i3c_bus_set_addr_slot_status(&master->bus, > dev->info.dyn_addr, > I3C_ADDR_SLOT_I3C_DEV); > @@ -1491,6 +1513,46 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > } > > /** > + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command > + * @master: master used to send frames on the bus > + * @info: I3C device information Maybe you can simply pass the dynamic address instead of the full device_info struct. > + * > + * Send a GETACCMST CCC command. > + * > + * This should be called if the curent master acknowledges bus takeover. I really thought this would be all automated by the master IP, but apparently it's not :-). > + * > + * 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, > + const struct i3c_device_info *info) > +{ > + 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, info->dyn_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) > + return ret; > + > + if (dest.payload.len != sizeof(*accmst)) > + return -EIO; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked); > + > +/** > * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) > * @master: master doing the DAA > * > @@ -1554,11 +1616,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, > struct i3c_dev_desc *i3cdev; > int ret; > > - 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) > + if (master->op_mode == I3C_MASTER_MODE && > + !i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr)) Please place this test in a separate if branch, as they are not really related. > return -EINVAL; > > if (master->this) > @@ -1569,7 +1628,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, > return PTR_ERR(i3cdev); > > master->this = i3cdev; > - master->bus.cur_master = master->this; > + if (master->op_mode == I3C_MASTER_MODE) > + master->bus.cur_master = master->this; When you hare a secondary master, you should make ->cur_master point to an i3c_dev_desc describing the current/main master. Should be possible thanks to the DEFSLVS info. > > ret = i3c_master_attach_i3c_dev(master, i3cdev); > if (ret) > @@ -1727,6 +1787,12 @@ 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. > + */ Hm, I'm not sure how that's supposed to work. What if the secondary master was initialized by the bootloader. Don't you need to reset something and maybe trigger a MSTREQ+DAA or a HotJoin? > + if (master->secondary) > + return 0; Add a blank like here. > + /* > * Reset all dynamic address that may have been assigned before > * (assigned by the bootloader for example). > */ > @@ -1738,6 +1804,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR, > I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR | > I3C_CCC_EVENT_HJ); > + Drop this blank line. > if (ret && ret != I3C_ERROR_M2) > goto err_bus_cleanup; > > @@ -1789,6 +1856,44 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) > return NULL; > } > > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, > + struct i2c_dev_boardinfo *info) > +{ > + enum i3c_addr_slot_status status; > + struct device *dev = &master->dev; > + struct i2c_dev_boardinfo *boardinfo; > + struct i2c_dev_desc *i2cdev; > + int ret; > + > + status = i3c_bus_get_addr_slot_status(&master->bus, > + info->base.addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > + > + boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL); > + if (!boardinfo) > + return -ENOMEM; > + > + i3c_bus_set_addr_slot_status(&master->bus, > + info->base.addr, > + I3C_ADDR_SLOT_I2C_DEV); > + > + boardinfo->base.addr = info->base.addr; > + boardinfo->lvr = info->lvr; > + > + i2cdev = i3c_master_alloc_i2c_dev(master, boardinfo); > + if (IS_ERR(i2cdev)) > + return PTR_ERR(i2cdev); > + > + ret = i3c_master_attach_i2c_dev(master, i2cdev); > + if (ret) { > + i3c_master_free_i2c_dev(i2cdev); > + return ret; > + } > + return 0; > +} > +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 > @@ -1832,6 +1937,15 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > if (ret) > goto err_free_dev; > > + if (I3C_BCR_DEVICE_ROLE(newdev->info.bcr) == I3C_BCR_I3C_MASTER && > + master->ops->enable_mastership) { > + if (master->ops->enable_mastership(newdev)) { > + dev_warn(&master->dev, > + "Failed to enable mastership for device: %d%llx", > + master->bus.id, newdev->info.pid); > + } > + } > + > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > if (olddev) { > newdev->boardinfo = olddev->boardinfo; > @@ -2102,6 +2216,11 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, > return -ENOTSUPP; > } > > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) Shouldn't this be called with the lock held? Oh, and we might have a race here if the master gains bus ownership and looses it just after that and before it was able to the transfer. I think we should: 1/ send a GETACCMST 2/ disable MR EVENTS 3/ do the I3C/I2C transfers 4/ enable MR EVENTS 1 and 2 should be sent in the same transaction to avoid cases where another master tries to acquire the bus, or if that's not possible, the master that just gained ownership of the bus should reject any incoming MR until DISEC(MR) has been sent. > + return -EIO; > + } > + > i3c_bus_normaluse_lock(&master->bus); > dev = i3c_master_find_i2c_dev_by_addr(master, addr); > if (!dev) > @@ -2393,15 +2512,31 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) > return 0; > } > > +static void i3c_master_bus_takeover(struct work_struct *work) > +{ > + struct i3c_master_controller *master; > + > + master = container_of(work, struct i3c_master_controller, mastership); > + > + i3c_bus_maintenance_lock(&master->bus); > + master->ops->update_devs(master); > + i3c_bus_maintenance_unlock(&master->bus); Blank like here please. > + /* > + * We can register I3C devices received from master by DEFSLVS. > + */ > + master->init_done = true; I think this should be set before that, in the i3c_master_register() function. When a secondary master is initialized, it should populate the dev list based on a previous DEFSLVS frame it has received before the ->probe() call, or just start with an empty list. > + i3c_bus_normaluse_lock(&master->bus); > + i3c_master_register_new_i3c_devs(master); > + i3c_bus_normaluse_unlock(&master->bus); > +} > + > /** > * 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: > * > @@ -2424,10 +2559,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; > @@ -2439,6 +2570,7 @@ int i3c_master_register(struct i3c_master_controller *master, > master->dev.release = i3c_masterdev_release; > master->ops = ops; > master->secondary = secondary; > + master->op_mode = secondary ? I3C_SLAVE_MODE : I3C_MASTER_MODE; > INIT_LIST_HEAD(&master->boardinfo.i2c); > INIT_LIST_HEAD(&master->boardinfo.i3c); > > @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master, > goto err_del_dev; > > /* > + * We can stop here. Devices will be attached after bus takeover. > + * Secondary masters can't do DAA. > + */ > + if (master->secondary) > + return 0; See, I don't think we should stop here. We should provide a hook to let secondary slaves populate the dev list based on data they might have received before that. > + > + /* > * We're done initializing the bus and the controller, we can now > * register I3C devices dicovered during the initial DAA. > */ > @@ -2521,6 +2660,95 @@ int i3c_master_register(struct i3c_master_controller *master, > EXPORT_SYMBOL_GPL(i3c_master_register); > > /** > + * i3c_master_switch_operation_mode() - changes operation mode of the controller > + * @master: master used to send frames on the bus > + * @new_master: the device that takes control of the bus > + * @op_mode: the master new operation mode > + * > + * Return: 0 in case of success, a negative error code otherwise. > + */ > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master, > + struct i3c_dev_desc *new_master, > + enum i3c_op_mode new_op_mode) > +{ > + if (master->op_mode == new_op_mode) > + return -EEXIST; > + > + master->op_mode = new_op_mode; > + > + if (new_op_mode == I3C_SLAVE_MODE) { > + master->bus.cur_master = new_master; > + } else { > + master->bus.cur_master = master->this; This is done too early. You should update cur->master only after the GETACCMST operation succeeds. > + INIT_WORK(&master->mastership, i3c_master_bus_takeover); INIT_WORK() should be called once at init time, not everytime you switch from one mode to another. > + queue_work(master->wq, &master->mastership); > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode); > + > +/** > + * i3c_master_mastership_ack() - performs operations before bus handover. > + * @master: master used to send frames on the bus > + * @info: I3C device information > + * > + * 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, > + const struct i3c_device_info *info) > +{ > + int ret; > + > + i3c_bus_maintenance_lock(&master->bus); > + ret = i3c_master_defslvs_locked(master); > + i3c_bus_maintenance_unlock(&master->bus); > + if (ret) > + return ret; > + Hm, this should have been sent during DAA, no need to do it again here, right? > + i3c_bus_maintenance_lock(&master->bus); > + ret = i3c_master_get_accmst_locked(master, info); > + i3c_bus_maintenance_unlock(&master->bus); > + if (ret) > + return ret; Hm, will this really work? I thought the mastership handover procedure had to be done atomically (using Sr between each transfer to avoid external interruptions). I might be wrong though (need to read the spec to refresh my memory). > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); > + > +static void i3c_secondary_master_bus_init(struct work_struct *work) > +{ > + struct i3c_master_controller *master; > + > + master = container_of(work, struct i3c_master_controller, mastership); > + > + if (i3c_master_request_mastership(master)) > + dev_err(&master->dev, "Mastership failed."); > +} > + > +/** > + * i3c_secondary_master_events_enabled() - event from current master > + * @master: master used to send frames on the bus > + * @evts: enabled events > + * > + * This function allows to perform required operations after current > + * master enables particular events on the bus. > + */ > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, > + u8 evts) > +{ > + if ((evts & I3C_CCC_EVENT_MR) && > + !master->init_done) { > + INIT_WORK(&master->mastership, i3c_secondary_master_bus_init); > + queue_work(master->wq, &master->mastership); > + } > +} > +EXPORT_SYMBOL_GPL(i3c_secondary_master_events_enabled); Hm, so you're trying to standardize events handling. I had initially left that to the driver as it's likely to be controller specific. > + > +/** > * i3c_master_unregister() - unregister an I3C master > * @master: master used to send frames on the bus > * > @@ -2552,6 +2780,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > if (!master || !xfers) > return -EINVAL; > > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) > + return -EIO; > + } > + > if (!master->ops->priv_xfers) > return -ENOTSUPP; > > @@ -2657,5 +2890,6 @@ static void __exit i3c_exit(void) > module_exit(i3c_exit); > > MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); > +MODULE_AUTHOR("Przemyslaw Gaj <pgaj@cadence.com>"); Please do that in a separate patch. > MODULE_DESCRIPTION("I3C core"); > MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index f13fd8b..ada956a 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -282,6 +282,16 @@ enum i3c_addr_slot_status { > }; > > /** > + * enum i3c_op_mode - I3C controller operation mode > + * @I3C_SLAVE_MODE: I3C controller operates in slave mode > + * @I3C_MASTER_MODE: I3C controller operates in master mode > + */ > +enum i3c_op_mode { > + I3C_SLAVE_MODE, > + I3C_MASTER_MODE > +}; > + > +/** > * struct i3c_bus - I3C bus object > * @cur_master: I3C master currently driving the bus. Since I3C is multi-master > * this can change over the time. Will be used to let a master > @@ -418,6 +428,20 @@ struct i3c_bus { > * for a future IBI > * This method is mandatory only if ->request_ibi is not > * NULL. > + * @update_devs: updates device list. Called after bus takeover. Secondary > + * master can't perform DAA procedure. Is that really true? Can you point me to the relevant section in the spec describing this constraint? > This function allows to > + * update devices received from previous bus owner in DEFSLVS > + * command. Useful also when new device joins the bus controlled > + * by secondary master, main master will be able to add > + * this device after mastership takeover. > + * @request_mastership: requests bus mastership. By default called by secondary > + * master after ENEC CCC is received and when devices were > + * not fully initialized yet. Mastership is also requested > + * when device driver wants to transfer data using master > + * in slave mode. > + * @enable_mastership: enable the Mastership for specified device. Mastership > + * does not require handler. Mastership is enabled for all > + * masters by default. Maybe ->enable_mr_events() (->enable_mastership() is just unclear to me). Oh, and if you have an enable function you should have a disable one too. > */ > struct i3c_master_controller_ops { > int (*bus_init)(struct i3c_master_controller *master); > @@ -445,6 +469,9 @@ 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_mastership)(struct i3c_dev_desc *dev); > }; > > /** > @@ -458,6 +485,7 @@ struct i3c_master_controller_ops { > * @ops: master operations. See &struct i3c_master_controller_ops > * @secondary: true if the master is a secondary master > * @init_done: true when the bus initialization is done > + * @op_mode: controller operation mode As said earlier, I don't think this is needed. Just check ->cur_master to know whether the master owns the bus or not. > * @boardinfo.i3c: list of I3C boardinfo objects > * @boardinfo.i2c: list of I2C boardinfo objects > * @boardinfo: board-level information attached to devices connected on the bus > @@ -467,6 +495,7 @@ struct i3c_master_controller_ops { > * in a thread context. Typical examples are Hot Join processing which > * requires taking the bus lock in maintenance, which in turn, can only > * be done from a sleep-able context > + * @mastership: work for switching operation mode after bus takeover How about mr_work or something like that. mastership is a bit ambiguous, and it's not really clear that it's representing a work object. > * > * A &struct i3c_master_controller has to be registered to the I3C subsystem > * through i3c_master_register(). None of &struct i3c_master_controller fields > @@ -480,12 +509,14 @@ struct i3c_master_controller { > const struct i3c_master_controller_ops *ops; > unsigned int secondary : 1; > unsigned int init_done : 1; > + enum i3c_op_mode op_mode; > struct { > struct list_head i3c; > struct list_head i2c; > } boardinfo; > struct i3c_bus bus; > struct workqueue_struct *wq; > + struct work_struct mastership; > }; > > /** > @@ -523,7 +554,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, > + struct i2c_dev_boardinfo *info); > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > u8 addr); > int i3c_master_do_daa(struct i3c_master_controller *master); > @@ -536,6 +568,15 @@ 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, > + const struct i3c_device_info *info); > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master, > + struct i3c_dev_desc *new_master, > + enum i3c_op_mode new_op_mode); > +int i3c_master_mastership_ack(struct i3c_master_controller *master, > + const struct i3c_device_info *info); > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, > + u8 evts); > > /** > * i3c_dev_get_master_data() - get master private data attached to an I3C _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem 2018-12-13 13:50 ` Boris Brezillon @ 2018-12-14 7:14 ` Przemyslaw Gaj 2018-12-14 8:24 ` Boris Brezillon 0 siblings, 1 reply; 9+ messages in thread From: Przemyslaw Gaj @ 2018-12-14 7:14 UTC (permalink / raw) To: Boris Brezillon; +Cc: linux-i3c, Przemyslaw Sroka, Rafal Ciepiela, vitor.soares Hi Boris, Thank you for reviewing this so quickly. On 12/13/18, 2:50 PM, "Boris Brezillon" <bbrezillon@kernel.org> wrote: EXTERNAL MAIL On Thu, 13 Dec 2018 12:28:00 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > This patch adds support for mastership request to I3C subsystem. > > Mastership event is enabled for all the masters on the I3C bus > by default. > > Mastership request occurs in the following cases: > - Mastership is requested automatically after secondary master > receives mastership ENEC event. This allows secondary masters to > initialize their bus I guess this only happens when the secondary master received a DEFSLVS packet, right? Actually, this happens when current master enables MR event. He should take care of providing devices list. Of course, we can consider adding a flag which indicates if slaves are already defined or not. > - If above procedure fails for some reason, user can force > mastership request through sysfs. Of course, mastership event > still has to be enabled on current master side Sounds like a good idea. > - Mastership is also requested automatically when device driver > tries to transfer data using master controller in slave mode. And that too. > > There is still some limitation: > - I2C devices are not registered on secondary master side. I2C > devices received in DEFSLVS CCC command are added to device list > just to send back this information to the other master controllers > in case of bus handoff. We can add support for this in the future > if there is such use case. I2C devices must be described under the bus represented by the secondary master for that to work properly (we need a way to bind devices to drivers). So, it's probably a good thing to discard (or leave them inactive) any devices that do not have a i2c_board_info entry on the secondary master side. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > --- > drivers/i3c/master.c | 260 ++++++++++++++++++++++++++++++++++++++++++--- > include/linux/i3c/master.h | 43 +++++++- > 2 files changed, 289 insertions(+), 14 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 68d6553..e98b600 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -460,6 +460,15 @@ static int i3c_bus_init(struct i3c_bus *i3cbus) > return 0; > } > > +static int i3c_master_request_mastership(struct i3c_master_controller *master) > +{ > + if (!master->ops->request_mastership) > + return -ENOTSUPP; Please add a blank line here > + if (master->ops->request_mastership(master)) > + return -EIO; and here. Ok for both. > + return 0; > +} > + > static const char * const i3c_bus_mode_strings[] = { > [I3C_BUS_MODE_PURE] = "pure", > [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", > @@ -491,8 +500,15 @@ static ssize_t current_master_show(struct device *dev, > char *buf) > { > struct i3c_bus *i3cbus = dev_to_i3cbus(dev); > + struct i3c_master_controller *master; > ssize_t ret; > > + if (!i3cbus->cur_master) { > + master = container_of(i3cbus, struct i3c_master_controller, bus); > + if (i3c_master_request_mastership(master)) > + return sprintf(buf, "unknown\n"); > + } Hm, why is that needed? When you are a secondary master ->cur_master should be set to the i3c_dev_desc representing the main master at init time. I do not add devices at initialization time, so secondary master does not have i3c_dev_desc object describing main master. DEFSLVS is providing us incomplete information, we don’t have PID yet. PID is retrieved from device when secondary master adds device. This is possible only if current master is ready to relinquish bus control. I decided to set cur_master after secondary master completely initialized his bus. What do you think? > + > i3c_bus_normaluse_lock(i3cbus); > ret = sprintf(buf, "%d-%llx\n", i3cbus->id, > i3cbus->cur_master->info.pid); > @@ -659,6 +675,11 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master, > if (!cmd || !master) > return -EINVAL; > > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) > + return -EIO; > + } > + This shouldn't be needed either. if i3cbus->cur_master != master->this, that means the master is operating in slave mode. I'll try that. > if (WARN_ON(master->init_done && > !rwsem_is_locked(&master->bus.lock))) > return -EINVAL; > @@ -1371,6 +1392,7 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, > dev->info.dyn_addr); > if (status != I3C_ADDR_SLOT_FREE) > return -EBUSY; > + Drop this change (or make it part of a separate patch fixing coding style issues). Ok. > i3c_bus_set_addr_slot_status(&master->bus, > dev->info.dyn_addr, > I3C_ADDR_SLOT_I3C_DEV); > @@ -1491,6 +1513,46 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master) > } > > /** > + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command > + * @master: master used to send frames on the bus > + * @info: I3C device information Maybe you can simply pass the dynamic address instead of the full device_info struct. No problem. Now I see it's not needed. > + * > + * Send a GETACCMST CCC command. > + * > + * This should be called if the curent master acknowledges bus takeover. I really thought this would be all automated by the master IP, but apparently it's not :-). > + * > + * 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, > + const struct i3c_device_info *info) > +{ > + 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, info->dyn_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) > + return ret; > + > + if (dest.payload.len != sizeof(*accmst)) > + return -EIO; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked); > + > +/** > * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment) > * @master: master doing the DAA > * > @@ -1554,11 +1616,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, > struct i3c_dev_desc *i3cdev; > int ret; > > - 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) > + if (master->op_mode == I3C_MASTER_MODE && > + !i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr)) Please place this test in a separate if branch, as they are not really related. Ok. > return -EINVAL; > > if (master->this) > @@ -1569,7 +1628,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, > return PTR_ERR(i3cdev); > > master->this = i3cdev; > - master->bus.cur_master = master->this; > + if (master->op_mode == I3C_MASTER_MODE) > + master->bus.cur_master = master->this; When you hare a secondary master, you should make ->cur_master point to an i3c_dev_desc describing the current/main master. Should be possible thanks to the DEFSLVS info. As I said before it's not so easy with DEFSLVS. We can consider creating device list without full information. Do you agree? > > ret = i3c_master_attach_i3c_dev(master, i3cdev); > if (ret) > @@ -1727,6 +1787,12 @@ 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. > + */ Hm, I'm not sure how that's supposed to work. What if the secondary master was initialized by the bootloader. Don't you need to reset something and maybe trigger a MSTREQ+DAA or a HotJoin? Even if secondary master does MSTREQ, he can't do DAA. He can't manage the bus, only main master can do that. I'll point this information in spec below. I'm wondering about HotJoin. I can't find this use case in my mind. > + if (master->secondary) > + return 0; Add a blank like here. Ok. > + /* > * Reset all dynamic address that may have been assigned before > * (assigned by the bootloader for example). > */ > @@ -1738,6 +1804,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > ret = i3c_master_disec_locked(master, I3C_BROADCAST_ADDR, > I3C_CCC_EVENT_SIR | I3C_CCC_EVENT_MR | > I3C_CCC_EVENT_HJ); > + Drop this blank line. Ok. > if (ret && ret != I3C_ERROR_M2) > goto err_bus_cleanup; > > @@ -1789,6 +1856,44 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) > return NULL; > } > > +int i3c_master_add_i2c_dev(struct i3c_master_controller *master, > + struct i2c_dev_boardinfo *info) > +{ > + enum i3c_addr_slot_status status; > + struct device *dev = &master->dev; > + struct i2c_dev_boardinfo *boardinfo; > + struct i2c_dev_desc *i2cdev; > + int ret; > + > + status = i3c_bus_get_addr_slot_status(&master->bus, > + info->base.addr); > + if (status != I3C_ADDR_SLOT_FREE) > + return -EBUSY; > + > + boardinfo = devm_kzalloc(dev, sizeof(*boardinfo), GFP_KERNEL); > + if (!boardinfo) > + return -ENOMEM; > + > + i3c_bus_set_addr_slot_status(&master->bus, > + info->base.addr, > + I3C_ADDR_SLOT_I2C_DEV); > + > + boardinfo->base.addr = info->base.addr; > + boardinfo->lvr = info->lvr; > + > + i2cdev = i3c_master_alloc_i2c_dev(master, boardinfo); > + if (IS_ERR(i2cdev)) > + return PTR_ERR(i2cdev); > + > + ret = i3c_master_attach_i2c_dev(master, i2cdev); > + if (ret) { > + i3c_master_free_i2c_dev(i2cdev); > + return ret; > + } > + return 0; > +} > +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 > @@ -1832,6 +1937,15 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > if (ret) > goto err_free_dev; > > + if (I3C_BCR_DEVICE_ROLE(newdev->info.bcr) == I3C_BCR_I3C_MASTER && > + master->ops->enable_mastership) { > + if (master->ops->enable_mastership(newdev)) { > + dev_warn(&master->dev, > + "Failed to enable mastership for device: %d%llx", > + master->bus.id, newdev->info.pid); > + } > + } > + > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > if (olddev) { > newdev->boardinfo = olddev->boardinfo; > @@ -2102,6 +2216,11 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap, > return -ENOTSUPP; > } > > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) Shouldn't this be called with the lock held? Oh, and we might have a race here if the master gains bus ownership and looses it just after that and before it was able to the transfer. I think we should: 1/ send a GETACCMST 2/ disable MR EVENTS 3/ do the I3C/I2C transfers 4/ enable MR EVENTS 1 and 2 should be sent in the same transaction to avoid cases where another master tries to acquire the bus, or if that's not possible, the master that just gained ownership of the bus should reject any incoming MR until DISEC(MR) has been sent. I strongly agree with your opinion. I'll implement it this way. > + return -EIO; > + } > + > i3c_bus_normaluse_lock(&master->bus); > dev = i3c_master_find_i2c_dev_by_addr(master, addr); > if (!dev) > @@ -2393,15 +2512,31 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops) > return 0; > } > > +static void i3c_master_bus_takeover(struct work_struct *work) > +{ > + struct i3c_master_controller *master; > + > + master = container_of(work, struct i3c_master_controller, mastership); > + > + i3c_bus_maintenance_lock(&master->bus); > + master->ops->update_devs(master); > + i3c_bus_maintenance_unlock(&master->bus); Blank like here please. Ok > + /* > + * We can register I3C devices received from master by DEFSLVS. > + */ > + master->init_done = true; I think this should be set before that, in the i3c_master_register() function. When a secondary master is initialized, it should populate the dev list based on a previous DEFSLVS frame it has received before the ->probe() call, or just start with an empty list. Same here. I do not populate list of devices until I'm able to get full information from devices. > + i3c_bus_normaluse_lock(&master->bus); > + i3c_master_register_new_i3c_devs(master); > + i3c_bus_normaluse_unlock(&master->bus); > +} > + > /** > * 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: > * > @@ -2424,10 +2559,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; > @@ -2439,6 +2570,7 @@ int i3c_master_register(struct i3c_master_controller *master, > master->dev.release = i3c_masterdev_release; > master->ops = ops; > master->secondary = secondary; > + master->op_mode = secondary ? I3C_SLAVE_MODE : I3C_MASTER_MODE; > INIT_LIST_HEAD(&master->boardinfo.i2c); > INIT_LIST_HEAD(&master->boardinfo.i3c); > > @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master, > goto err_del_dev; > > /* > + * We can stop here. Devices will be attached after bus takeover. > + * Secondary masters can't do DAA. > + */ > + if (master->secondary) > + return 0; See, I don't think we should stop here. We should provide a hook to let secondary slaves populate the dev list based on data they might have received before that. Do we want to populate list with data received by DEFSLVS? If yes, I can populate list but we are not able to register devices because we don’t have PID yet and device drivers are matched by PID. > + > + /* > * We're done initializing the bus and the controller, we can now > * register I3C devices dicovered during the initial DAA. > */ > @@ -2521,6 +2660,95 @@ int i3c_master_register(struct i3c_master_controller *master, > EXPORT_SYMBOL_GPL(i3c_master_register); > > /** > + * i3c_master_switch_operation_mode() - changes operation mode of the controller > + * @master: master used to send frames on the bus > + * @new_master: the device that takes control of the bus > + * @op_mode: the master new operation mode > + * > + * Return: 0 in case of success, a negative error code otherwise. > + */ > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master, > + struct i3c_dev_desc *new_master, > + enum i3c_op_mode new_op_mode) > +{ > + if (master->op_mode == new_op_mode) > + return -EEXIST; > + > + master->op_mode = new_op_mode; > + > + if (new_op_mode == I3C_SLAVE_MODE) { > + master->bus.cur_master = new_master; > + } else { > + master->bus.cur_master = master->this; This is done too early. You should update cur->master only after the GETACCMST operation succeeds. Indeed. > + INIT_WORK(&master->mastership, i3c_master_bus_takeover); INIT_WORK() should be called once at init time, not everytime you switch from one mode to another. My mistake. > + queue_work(master->wq, &master->mastership); > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode); > + > +/** > + * i3c_master_mastership_ack() - performs operations before bus handover. > + * @master: master used to send frames on the bus > + * @info: I3C device information > + * > + * 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, > + const struct i3c_device_info *info) > +{ > + int ret; > + > + i3c_bus_maintenance_lock(&master->bus); > + ret = i3c_master_defslvs_locked(master); > + i3c_bus_maintenance_unlock(&master->bus); > + if (ret) > + return ret; > + Hm, this should have been sent during DAA, no need to do it again here, right? I thought that there could be the case when something has changed on the bus but now I think device list on secondary master side is up-to-date without that. I'll drop this. > + i3c_bus_maintenance_lock(&master->bus); > + ret = i3c_master_get_accmst_locked(master, info); > + i3c_bus_maintenance_unlock(&master->bus); > + if (ret) > + return ret; Hm, will this really work? I thought the mastership handover procedure had to be done atomically (using Sr between each transfer to avoid external interruptions). I might be wrong though (need to read the spec to refresh my memory). There wouldn't be interruption. Secondary master may ack or nack this command. If he acks, it's done, he became current master. If he nacks or transmits incorrect address, he will not acquire mastership. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); > + > +static void i3c_secondary_master_bus_init(struct work_struct *work) > +{ > + struct i3c_master_controller *master; > + > + master = container_of(work, struct i3c_master_controller, mastership); > + > + if (i3c_master_request_mastership(master)) > + dev_err(&master->dev, "Mastership failed."); > +} > + > +/** > + * i3c_secondary_master_events_enabled() - event from current master > + * @master: master used to send frames on the bus > + * @evts: enabled events > + * > + * This function allows to perform required operations after current > + * master enables particular events on the bus. > + */ > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, > + u8 evts) > +{ > + if ((evts & I3C_CCC_EVENT_MR) && > + !master->init_done) { > + INIT_WORK(&master->mastership, i3c_secondary_master_bus_init); > + queue_work(master->wq, &master->mastership); > + } > +} > +EXPORT_SYMBOL_GPL(i3c_secondary_master_events_enabled); Hm, so you're trying to standardize events handling. I had initially left that to the driver as it's likely to be controller specific. I think that I3C spec describes common set of events. > + > +/** > * i3c_master_unregister() - unregister an I3C master > * @master: master used to send frames on the bus > * > @@ -2552,6 +2780,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > if (!master || !xfers) > return -EINVAL; > > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) > + return -EIO; > + } > + > if (!master->ops->priv_xfers) > return -ENOTSUPP; > > @@ -2657,5 +2890,6 @@ static void __exit i3c_exit(void) > module_exit(i3c_exit); > > MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); > +MODULE_AUTHOR("Przemyslaw Gaj <pgaj@cadence.com>"); Please do that in a separate patch. Ok. > MODULE_DESCRIPTION("I3C core"); > MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index f13fd8b..ada956a 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -282,6 +282,16 @@ enum i3c_addr_slot_status { > }; > > /** > + * enum i3c_op_mode - I3C controller operation mode > + * @I3C_SLAVE_MODE: I3C controller operates in slave mode > + * @I3C_MASTER_MODE: I3C controller operates in master mode > + */ > +enum i3c_op_mode { > + I3C_SLAVE_MODE, > + I3C_MASTER_MODE > +}; > + > +/** > * struct i3c_bus - I3C bus object > * @cur_master: I3C master currently driving the bus. Since I3C is multi-master > * this can change over the time. Will be used to let a master > @@ -418,6 +428,20 @@ struct i3c_bus { > * for a future IBI > * This method is mandatory only if ->request_ibi is not > * NULL. > + * @update_devs: updates device list. Called after bus takeover. Secondary > + * master can't perform DAA procedure. Is that really true? Can you point me to the relevant section in the spec describing this constraint? Sure, I3C Devices Roles vs Responsibilities, Table in spec. Dynamic Address Assignment, Secondary master is not capable to do that. Please also take a look at Hot-Join Dynamic Address Assignment. Some of controllers may have HJ-DAA implemented in the hardware, maybe it's better to send DEFSLVS right before GETACCMST? > This function allows to > + * update devices received from previous bus owner in DEFSLVS > + * command. Useful also when new device joins the bus controlled > + * by secondary master, main master will be able to add > + * this device after mastership takeover. > + * @request_mastership: requests bus mastership. By default called by secondary > + * master after ENEC CCC is received and when devices were > + * not fully initialized yet. Mastership is also requested > + * when device driver wants to transfer data using master > + * in slave mode. > + * @enable_mastership: enable the Mastership for specified device. Mastership > + * does not require handler. Mastership is enabled for all > + * masters by default. Maybe ->enable_mr_events() (->enable_mastership() is just unclear to me). Oh, and if you have an enable function you should have a disable one too. I agree. > */ > struct i3c_master_controller_ops { > int (*bus_init)(struct i3c_master_controller *master); > @@ -445,6 +469,9 @@ 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_mastership)(struct i3c_dev_desc *dev); > }; > > /** > @@ -458,6 +485,7 @@ struct i3c_master_controller_ops { > * @ops: master operations. See &struct i3c_master_controller_ops > * @secondary: true if the master is a secondary master > * @init_done: true when the bus initialization is done > + * @op_mode: controller operation mode As said earlier, I don't think this is needed. Just check ->cur_master to know whether the master owns the bus or not. I'll try. > * @boardinfo.i3c: list of I3C boardinfo objects > * @boardinfo.i2c: list of I2C boardinfo objects > * @boardinfo: board-level information attached to devices connected on the bus > @@ -467,6 +495,7 @@ struct i3c_master_controller_ops { > * in a thread context. Typical examples are Hot Join processing which > * requires taking the bus lock in maintenance, which in turn, can only > * be done from a sleep-able context > + * @mastership: work for switching operation mode after bus takeover How about mr_work or something like that. mastership is a bit ambiguous, and it's not really clear that it's representing a work object. I agree. > * > * A &struct i3c_master_controller has to be registered to the I3C subsystem > * through i3c_master_register(). None of &struct i3c_master_controller fields > @@ -480,12 +509,14 @@ struct i3c_master_controller { > const struct i3c_master_controller_ops *ops; > unsigned int secondary : 1; > unsigned int init_done : 1; > + enum i3c_op_mode op_mode; > struct { > struct list_head i3c; > struct list_head i2c; > } boardinfo; > struct i3c_bus bus; > struct workqueue_struct *wq; > + struct work_struct mastership; > }; > > /** > @@ -523,7 +554,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, > + struct i2c_dev_boardinfo *info); > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > u8 addr); > int i3c_master_do_daa(struct i3c_master_controller *master); > @@ -536,6 +568,15 @@ 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, > + const struct i3c_device_info *info); > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master, > + struct i3c_dev_desc *new_master, > + enum i3c_op_mode new_op_mode); > +int i3c_master_mastership_ack(struct i3c_master_controller *master, > + const struct i3c_device_info *info); > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, > + u8 evts); > > /** > * i3c_dev_get_master_data() - get master private data attached to an I3C Thanks, Przemek _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem 2018-12-14 7:14 ` Przemyslaw Gaj @ 2018-12-14 8:24 ` Boris Brezillon 2018-12-14 13:47 ` Przemyslaw Gaj 0 siblings, 1 reply; 9+ messages in thread From: Boris Brezillon @ 2018-12-14 8:24 UTC (permalink / raw) To: Przemyslaw Gaj; +Cc: linux-i3c, Przemyslaw Sroka, Rafal Ciepiela, vitor.soares Hi Przemek, On Fri, 14 Dec 2018 07:14:44 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > > Mastership request occurs in the following cases: > > - Mastership is requested automatically after secondary master > > receives mastership ENEC event. This allows secondary masters to > > initialize their bus > > I guess this only happens when the secondary master received a DEFSLVS > packet, right? > > Actually, this happens when current master enables MR event. > He should take care of providing devices list. > Of course, we can consider adding a flag which indicates if slaves are already defined or not. i3c_master_add_i3c_dev_locked() already handles this "device is already known/registered" case, so that shouldn't be a problem. > > static const char * const i3c_bus_mode_strings[] = { > > [I3C_BUS_MODE_PURE] = "pure", > > [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", > > @@ -491,8 +500,15 @@ static ssize_t current_master_show(struct device *dev, > > char *buf) > > { > > struct i3c_bus *i3cbus = dev_to_i3cbus(dev); > > + struct i3c_master_controller *master; > > ssize_t ret; > > > > + if (!i3cbus->cur_master) { > > + master = container_of(i3cbus, struct i3c_master_controller, bus); > > + if (i3c_master_request_mastership(master)) > > + return sprintf(buf, "unknown\n"); > > + } > > Hm, why is that needed? When you are a secondary master ->cur_master > should be set to the i3c_dev_desc representing the main master at init > time. > > I do not add devices at initialization time, so secondary master does not have i3c_dev_desc object describing main master. > DEFSLVS is providing us incomplete information, we don’t have PID yet. PID is retrieved from device when secondary master adds device. > This is possible only if current master is ready to relinquish bus control. > > I decided to set cur_master after secondary master completely initialized his bus. What do you think? Maybe we should not initialize the bus until we have DEFSLVS information then. > > > return -EINVAL; > > > > if (master->this) > > @@ -1569,7 +1628,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, > > return PTR_ERR(i3cdev); > > > > master->this = i3cdev; > > - master->bus.cur_master = master->this; > > + if (master->op_mode == I3C_MASTER_MODE) > > + master->bus.cur_master = master->this; > > When you hare a secondary master, you should make ->cur_master point to > an i3c_dev_desc describing the current/main master. Should be possible > thanks to the DEFSLVS info. > > As I said before it's not so easy with DEFSLVS. > We can consider creating device list without full information. Do you agree? We can just leave ->cur_master to NULL at first and have the following function: static bool i3c_master_owns_bus(struct i3c_master_controller *master) { return master->bus->cur_master == master->this; } Regarding the proposal to have a partially discovered device list, why not, depends what we want to do with that. > > > > > ret = i3c_master_attach_i3c_dev(master, i3cdev); > > if (ret) > > @@ -1727,6 +1787,12 @@ 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. > > + */ > > Hm, I'm not sure how that's supposed to work. What if the secondary > master was initialized by the bootloader. Don't you need to reset > something and maybe trigger a MSTREQ+DAA or a HotJoin? > > Even if secondary master does MSTREQ, he can't do DAA. He can't manage the bus, only main master can do that. > I'll point this information in spec below. Thanks. > > I'm wondering about HotJoin. I can't find this use case in my mind. > > + /* > > + * We can register I3C devices received from master by DEFSLVS. > > + */ > > + master->init_done = true; > > I think this should be set before that, in the i3c_master_register() > function. When a secondary master is initialized, it should populate > the dev list based on a previous DEFSLVS frame it has received before > the ->probe() call, or just start with an empty list. > > Same here. I do not populate list of devices until I'm able to get full information from devices. Correct. But ->init_done should be set before that IMO, unless we decide to defer bus initialization after DEFSLVS/ENEC(MR). > > > + i3c_bus_normaluse_lock(&master->bus); > > + i3c_master_register_new_i3c_devs(master); > > + i3c_bus_normaluse_unlock(&master->bus); > > +} > > + > > /** > > * 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: > > * > > @@ -2424,10 +2559,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; > > @@ -2439,6 +2570,7 @@ int i3c_master_register(struct i3c_master_controller *master, > > master->dev.release = i3c_masterdev_release; > > master->ops = ops; > > master->secondary = secondary; > > + master->op_mode = secondary ? I3C_SLAVE_MODE : I3C_MASTER_MODE; > > INIT_LIST_HEAD(&master->boardinfo.i2c); > > INIT_LIST_HEAD(&master->boardinfo.i3c); > > > > @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master, > > goto err_del_dev; > > > > /* > > + * We can stop here. Devices will be attached after bus takeover. > > + * Secondary masters can't do DAA. > > + */ > > + if (master->secondary) > > + return 0; > > See, I don't think we should stop here. We should provide a hook to let > secondary slaves populate the dev list based on data they might have > received before that. > > Do we want to populate list with data received by DEFSLVS? I guess we do. I mean, we shouldn't populate the list directly, but we should request bus ownership, and then add I3C devices one by one using i3c_master_add_i3c_dev_locked(). This function will do the rest (query PID, BCR, DCR, ... and then add the device to the list). > If yes, I can populate list but we are not able to register devices because we don’t have > PID yet and device drivers are matched by PID. Now I understand why you want it to be driven by ENEC(MR). Can't we have a situation where i3c_master_register() is called after it has received both DEFSLV and ENEC(MR). Shouldn't we request bus ownership directly at init time in this case. Looks like all of this will be I3C master controller driven though, so maybe it's just better to let master drivers handle all of that on their own. > > + i3c_bus_maintenance_lock(&master->bus); > > + ret = i3c_master_get_accmst_locked(master, info); > > + i3c_bus_maintenance_unlock(&master->bus); > > + if (ret) > > + return ret; > > Hm, will this really work? I thought the mastership handover procedure > had to be done atomically (using Sr between each transfer to avoid > external interruptions). I might be wrong though (need to read the > spec to refresh my memory). > > There wouldn't be interruption. Secondary master may ack or nack this command. > If he acks, it's done, he became current master. > If he nacks or transmits incorrect address, he will not acquire mastership. Yep, I realized that afterwards, when reading the spec again. > > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); > > + > > +static void i3c_secondary_master_bus_init(struct work_struct *work) > > +{ > > + struct i3c_master_controller *master; > > + > > + master = container_of(work, struct i3c_master_controller, mastership); > > + > > + if (i3c_master_request_mastership(master)) > > + dev_err(&master->dev, "Mastership failed."); > > +} > > + > > +/** > > + * i3c_secondary_master_events_enabled() - event from current master > > + * @master: master used to send frames on the bus > > + * @evts: enabled events > > + * > > + * This function allows to perform required operations after current > > + * master enables particular events on the bus. > > + */ > > +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, > > + u8 evts) > > +{ > > + if ((evts & I3C_CCC_EVENT_MR) && > > + !master->init_done) { > > + INIT_WORK(&master->mastership, i3c_secondary_master_bus_init); > > + queue_work(master->wq, &master->mastership); > > + } > > +} > > +EXPORT_SYMBOL_GPL(i3c_secondary_master_events_enabled); > > Hm, so you're trying to standardize events handling. I had initially > left that to the driver as it's likely to be controller specific. > > I think that I3C spec describes common set of events. Yes, and those common events are already defined in ccc.h. What I meant is that handling MR or HotJoin events is likely to be controller specific, not sure we need to define an mr_work at the i3c_master_controller level, we can just let I3C drivers define their own work if they need one (like I did to support HJ in the Cadence driver). > > +/** > > * struct i3c_bus - I3C bus object > > * @cur_master: I3C master currently driving the bus. Since I3C is multi-master > > * this can change over the time. Will be used to let a master > > @@ -418,6 +428,20 @@ struct i3c_bus { > > * for a future IBI > > * This method is mandatory only if ->request_ibi is not > > * NULL. > > + * @update_devs: updates device list. Called after bus takeover. Secondary > > + * master can't perform DAA procedure. > > Is that really true? Can you point me to the relevant section in the > spec describing this constraint? > > Sure, I3C Devices Roles vs Responsibilities, Table in spec. Dynamic Address > Assignment, Secondary master is not capable to do that. Thanks for pointing this out, I didn't notice. > > Please also take a look at Hot-Join Dynamic Address Assignment. Some of controllers may > have HJ-DAA implemented in the hardware, maybe it's better to send DEFSLVS right before > GETACCMST? It's the responsibility of the master doing a DAA to send a DEFSLVS frame if there are new secondary masters on the bus. So DEFSLVS should be sent just after the HJ-DAA in this case, not before GETACCMST. Regards, Boris _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem 2018-12-14 8:24 ` Boris Brezillon @ 2018-12-14 13:47 ` Przemyslaw Gaj 2018-12-19 14:16 ` Boris Brezillon 0 siblings, 1 reply; 9+ messages in thread From: Przemyslaw Gaj @ 2018-12-14 13:47 UTC (permalink / raw) To: Boris Brezillon; +Cc: linux-i3c, Przemyslaw Sroka, Rafal Ciepiela, vitor.soares Hi Boris, > On Dec 14, 2018, at 9:24 AM, Boris Brezillon <bbrezillon@kernel.org> wrote: > > EXTERNAL MAIL > > > Hi Przemek, > > On Fri, 14 Dec 2018 07:14:44 +0000 > Przemyslaw Gaj <pgaj@cadence.com> wrote: > >>> Mastership request occurs in the following cases: >>> - Mastership is requested automatically after secondary master >>> receives mastership ENEC event. This allows secondary masters to >>> initialize their bus >> >> I guess this only happens when the secondary master received a DEFSLVS >> packet, right? >> >> Actually, this happens when current master enables MR event. >> He should take care of providing devices list. >> Of course, we can consider adding a flag which indicates if slaves are already defined or not. > > i3c_master_add_i3c_dev_locked() already handles this "device is already > known/registered" case, so that shouldn't be a problem. I meant defined by DEFSLVS command when I wrote "slaves are already defined". I know i3c_master_add_i3c_dev_locked() handles such cases. This flag could indicate that we already got DEFSLVS and then (after ENEC(MR)) we could check if slaves were defined and finish bus initialization. > >>> static const char * const i3c_bus_mode_strings[] = { >>> [I3C_BUS_MODE_PURE] = "pure", >>> [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast", >>> @@ -491,8 +500,15 @@ static ssize_t current_master_show(struct device *dev, >>> char *buf) >>> { >>> struct i3c_bus *i3cbus = dev_to_i3cbus(dev); >>> + struct i3c_master_controller *master; >>> ssize_t ret; >>> >>> + if (!i3cbus->cur_master) { >>> + master = container_of(i3cbus, struct i3c_master_controller, bus); >>> + if (i3c_master_request_mastership(master)) >>> + return sprintf(buf, "unknown\n"); >>> + } >> >> Hm, why is that needed? When you are a secondary master ->cur_master >> should be set to the i3c_dev_desc representing the main master at init >> time. >> >> I do not add devices at initialization time, so secondary master does not have i3c_dev_desc object describing main master. >> DEFSLVS is providing us incomplete information, we don’t have PID yet. PID is retrieved from device when secondary master adds device. >> This is possible only if current master is ready to relinquish bus control. >> >> I decided to set cur_master after secondary master completely initialized his bus. What do you think? > > Maybe we should not initialize the bus until we have DEFSLVS > information then. It is good idea. We can use DEFSLVS instead of ENEC(MR) event as a trigger for secondary master bus initialization or both. I assumed that main master will not send ENEC(MR) without sending DEFSLVS and ENEC(MR) will let secondary master know that not only slave list is defined, but also he can request mastership. > >> >>> return -EINVAL; >>> >>> if (master->this) >>> @@ -1569,7 +1628,8 @@ int i3c_master_set_info(struct i3c_master_controller *master, >>> return PTR_ERR(i3cdev); >>> >>> master->this = i3cdev; >>> - master->bus.cur_master = master->this; >>> + if (master->op_mode == I3C_MASTER_MODE) >>> + master->bus.cur_master = master->this; >> >> When you hare a secondary master, you should make ->cur_master point to >> an i3c_dev_desc describing the current/main master. Should be possible >> thanks to the DEFSLVS info. >> >> As I said before it's not so easy with DEFSLVS. >> We can consider creating device list without full information. Do you agree? > > We can just leave ->cur_master to NULL at first and have the following > function: > > static bool i3c_master_owns_bus(struct i3c_master_controller *master) > { > return master->bus->cur_master == master->this; > } > > Regarding the proposal to have a partially discovered device list, why > not, depends what we want to do with that. > >> >>> >>> ret = i3c_master_attach_i3c_dev(master, i3cdev); >>> if (ret) >>> @@ -1727,6 +1787,12 @@ 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. >>> + */ >> >> Hm, I'm not sure how that's supposed to work. What if the secondary >> master was initialized by the bootloader. Don't you need to reset >> something and maybe trigger a MSTREQ+DAA or a HotJoin? >> >> Even if secondary master does MSTREQ, he can't do DAA. He can't manage the bus, only main master can do that. >> I'll point this information in spec below. > > Thanks. > >> >> I'm wondering about HotJoin. I can't find this use case in my mind. >>> + /* >>> + * We can register I3C devices received from master by DEFSLVS. >>> + */ >>> + master->init_done = true; >> >> I think this should be set before that, in the i3c_master_register() >> function. When a secondary master is initialized, it should populate >> the dev list based on a previous DEFSLVS frame it has received before >> the ->probe() call, or just start with an empty list. >> >> Same here. I do not populate list of devices until I'm able to get full information from devices. > > Correct. But ->init_done should be set before that IMO, unless we > decide to defer bus initialization after DEFSLVS/ENEC(MR). Yes, we need decision to know which path to choose :) > >> >>> + i3c_bus_normaluse_lock(&master->bus); >>> + i3c_master_register_new_i3c_devs(master); >>> + i3c_bus_normaluse_unlock(&master->bus); >>> +} >>> + >>> /** >>> * 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: >>> * >>> @@ -2424,10 +2559,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; >>> @@ -2439,6 +2570,7 @@ int i3c_master_register(struct i3c_master_controller *master, >>> master->dev.release = i3c_masterdev_release; >>> master->ops = ops; >>> master->secondary = secondary; >>> + master->op_mode = secondary ? I3C_SLAVE_MODE : I3C_MASTER_MODE; >>> INIT_LIST_HEAD(&master->boardinfo.i2c); >>> INIT_LIST_HEAD(&master->boardinfo.i3c); >>> >>> @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master, >>> goto err_del_dev; >>> >>> /* >>> + * We can stop here. Devices will be attached after bus takeover. >>> + * Secondary masters can't do DAA. >>> + */ >>> + if (master->secondary) >>> + return 0; >> >> See, I don't think we should stop here. We should provide a hook to let >> secondary slaves populate the dev list based on data they might have >> received before that. >> >> Do we want to populate list with data received by DEFSLVS? > > I guess we do. I mean, we shouldn't populate the list directly, but > we should request bus ownership, and then add I3C devices one by one > using i3c_master_add_i3c_dev_locked(). This function will do the rest > (query PID, BCR, DCR, ... and then add the device to the list). > >> If yes, I can populate list but we are not able to register devices because we don’t have >> PID yet and device drivers are matched by PID. > > Now I understand why you want it to be driven by ENEC(MR). Can't we > have a situation where i3c_master_register() is called after it has > received both DEFSLV and ENEC(MR). Shouldn't we request bus ownership > directly at init time in this case. I think it's possible and I think we should request mastership and finish initialization in this case. And what if MR is disabled during initialization? Do we want to wait for DEFSLVS or ENEC(MR) event and then finish initialization? I just wanted to have generic solution for both cases. > > Looks like all of this will be I3C master controller driven though, so > maybe it's just better to let master drivers handle all of that on > their own. So, I can try adding devices received by DEFSLVS on driver side and then call i3c_master_register(). Is that what you meant? I'm sorry if I misunderstood something. > >>> + i3c_bus_maintenance_lock(&master->bus); >>> + ret = i3c_master_get_accmst_locked(master, info); >>> + i3c_bus_maintenance_unlock(&master->bus); >>> + if (ret) >>> + return ret; >> >> Hm, will this really work? I thought the mastership handover procedure >> had to be done atomically (using Sr between each transfer to avoid >> external interruptions). I might be wrong though (need to read the >> spec to refresh my memory). >> >> There wouldn't be interruption. Secondary master may ack or nack this command. >> If he acks, it's done, he became current master. >> If he nacks or transmits incorrect address, he will not acquire mastership. > > Yep, I realized that afterwards, when reading the spec again. > >> >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack); >>> + >>> +static void i3c_secondary_master_bus_init(struct work_struct *work) >>> +{ >>> + struct i3c_master_controller *master; >>> + >>> + master = container_of(work, struct i3c_master_controller, mastership); >>> + >>> + if (i3c_master_request_mastership(master)) >>> + dev_err(&master->dev, "Mastership failed."); >>> +} >>> + >>> +/** >>> + * i3c_secondary_master_events_enabled() - event from current master >>> + * @master: master used to send frames on the bus >>> + * @evts: enabled events >>> + * >>> + * This function allows to perform required operations after current >>> + * master enables particular events on the bus. >>> + */ >>> +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master, >>> + u8 evts) >>> +{ >>> + if ((evts & I3C_CCC_EVENT_MR) && >>> + !master->init_done) { >>> + INIT_WORK(&master->mastership, i3c_secondary_master_bus_init); >>> + queue_work(master->wq, &master->mastership); >>> + } >>> +} >>> +EXPORT_SYMBOL_GPL(i3c_secondary_master_events_enabled); >> >> Hm, so you're trying to standardize events handling. I had initially >> left that to the driver as it's likely to be controller specific. >> >> I think that I3C spec describes common set of events. > > Yes, and those common events are already defined in ccc.h. What I meant > is that handling MR or HotJoin events is likely to be controller > specific, not sure we need to define an mr_work at the > i3c_master_controller level, we can just let I3C drivers define their > own work if they need one (like I did to support HJ in the Cadence > driver). Ok, I'll think about it. > >>> +/** >>> * struct i3c_bus - I3C bus object >>> * @cur_master: I3C master currently driving the bus. Since I3C is multi-master >>> * this can change over the time. Will be used to let a master >>> @@ -418,6 +428,20 @@ struct i3c_bus { >>> * for a future IBI >>> * This method is mandatory only if ->request_ibi is not >>> * NULL. >>> + * @update_devs: updates device list. Called after bus takeover. Secondary >>> + * master can't perform DAA procedure. >> >> Is that really true? Can you point me to the relevant section in the >> spec describing this constraint? >> >> Sure, I3C Devices Roles vs Responsibilities, Table in spec. Dynamic Address >> Assignment, Secondary master is not capable to do that. > > Thanks for pointing this out, I didn't notice. > >> >> Please also take a look at Hot-Join Dynamic Address Assignment. Some of controllers may >> have HJ-DAA implemented in the hardware, maybe it's better to send DEFSLVS right before >> GETACCMST? > > It's the responsibility of the master doing a DAA to send a DEFSLVS > frame if there are new secondary masters on the bus. So DEFSLVS should > be sent just after the HJ-DAA in this case, not before GETACCMST. > > Regards, > > Boris Regards, Przemek _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem 2018-12-14 13:47 ` Przemyslaw Gaj @ 2018-12-19 14:16 ` Boris Brezillon 0 siblings, 0 replies; 9+ messages in thread From: Boris Brezillon @ 2018-12-19 14:16 UTC (permalink / raw) To: Przemyslaw Gaj; +Cc: linux-i3c, Przemyslaw Sroka, Rafal Ciepiela, vitor.soares On Fri, 14 Dec 2018 13:47:39 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > >>> @@ -2497,6 +2629,13 @@ int i3c_master_register(struct i3c_master_controller *master, > >>> goto err_del_dev; > >>> > >>> /* > >>> + * We can stop here. Devices will be attached after bus takeover. > >>> + * Secondary masters can't do DAA. > >>> + */ > >>> + if (master->secondary) > >>> + return 0; > >> > >> See, I don't think we should stop here. We should provide a hook to let > >> secondary slaves populate the dev list based on data they might have > >> received before that. > >> > >> Do we want to populate list with data received by DEFSLVS? > > > > I guess we do. I mean, we shouldn't populate the list directly, but > > we should request bus ownership, and then add I3C devices one by one > > using i3c_master_add_i3c_dev_locked(). This function will do the rest > > (query PID, BCR, DCR, ... and then add the device to the list). > > > >> If yes, I can populate list but we are not able to register devices because we don’t have > >> PID yet and device drivers are matched by PID. > > > > Now I understand why you want it to be driven by ENEC(MR). Can't we > > have a situation where i3c_master_register() is called after it has > > received both DEFSLV and ENEC(MR). Shouldn't we request bus ownership > > directly at init time in this case. > > I think it's possible and I think we should request mastership and finish > initialization in this case. And what if MR is disabled during initialization? Then you'd have to delay this part until ENEC(MR) is received, so maybe it's just easier to let master controller drivers handle that part on their own. > Do we want to wait for DEFSLVS or ENEC(MR) event and then finish > initialization? I just wanted to have generic solution for both cases. It really depends what we mean by "bus initialization". If we decide that secondary master bus init is just about initializing the i3c_master_controller and i3c_bus structs (and maybe enable the controller and send an HJ event on the bus) and nothing more, then we should let I3C master controller drivers register I3C dev on their own. > > > > > Looks like all of this will be I3C master controller driven though, so > > maybe it's just better to let master drivers handle all of that on > > their own. > > So, I can try adding devices received by DEFSLVS on driver side and > then call i3c_master_register(). Is that what you meant? I'm sorry if I > misunderstood something. I don't know. I was just thinking out loud. Right now, I can't tell which option is best: 1/ provide a minimal secondary master init routine and let drivers do all the hard work 2/ provide an automated procedure to simplify driver's life and take the risk of making it too restrictive If I had to work on that, I'd probably go for option #1 and try to refine things when we start having more controllers, but I might be wrong. _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] i3c: master: cdns: add support for mastership request to Cadence I3C master driver. 2018-12-13 12:27 [PATCH 0/2] Add the I3C mastership request Przemyslaw Gaj 2018-12-13 12:28 ` [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj @ 2018-12-13 12:28 ` Przemyslaw Gaj 2018-12-13 12:47 ` [PATCH 0/2] Add the I3C mastership request Boris Brezillon 2 siblings, 0 replies; 9+ messages in thread From: Przemyslaw Gaj @ 2018-12-13 12:28 UTC (permalink / raw) To: bbrezillon, linux-i3c; +Cc: Przemyslaw Gaj, psroka, rafalc, vitor.soares This patch adds support for mastership request to Cadence I3C master driver. Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> --- drivers/i3c/master/i3c-master-cdns.c | 316 ++++++++++++++++++++++++++++++----- 1 file changed, 271 insertions(+), 45 deletions(-) diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c index 02a8297..a33f3a6 100644 --- a/drivers/i3c/master/i3c-master-cdns.c +++ b/drivers/i3c/master/i3c-master-cdns.c @@ -157,6 +157,7 @@ #define SLV_IMR 0x48 #define SLV_ICR 0x4c #define SLV_ISR 0x50 +#define SLV_INT_DEFSLVS BIT(21) #define SLV_INT_TM BIT(20) #define SLV_INT_ERROR BIT(19) #define SLV_INT_EVENT_UP BIT(18) @@ -390,6 +391,10 @@ struct cdns_i3c_xfer { struct cdns_i3c_master { struct work_struct hj_work; + struct { + struct work_struct handover_work; + u32 ibir; + } mastership; struct i3c_master_controller base; u32 free_rr_slots; unsigned int maxdevs; @@ -664,6 +669,89 @@ static void cdns_i3c_master_unqueue_xfer(struct cdns_i3c_master *master, spin_unlock_irqrestore(&master->xferqueue.lock, flags); } +static void +cdns_i3c_master_i3c_dev_rr_to_info(struct cdns_i3c_master *master, + unsigned int slot, + struct i3c_device_info *info) +{ + u32 rr; + + memset(info, 0, sizeof(*info)); + rr = readl(master->regs + DEV_ID_RR0(slot)); + info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr); + rr = readl(master->regs + DEV_ID_RR2(slot)); + info->dcr = rr; + info->bcr = rr >> 8; + info->pid = rr >> 16; + info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16; +} + +static void +cdns_i3c_master_i2c_dev_rr_to_boardinfo(struct cdns_i3c_master *master, + unsigned int slot, + struct i2c_dev_boardinfo *info) +{ + u32 rr; + + memset(info, 0, sizeof(*info)); + rr = readl(master->regs + DEV_ID_RR0(slot)); + info->base.addr = DEV_ID_RR0_GET_DEV_ADDR(rr); + rr = readl(master->regs + DEV_ID_RR2(slot)); + info->lvr = rr; +} + +static +int cdns_i3c_sec_master_request_mastership(struct i3c_master_controller *m) +{ + struct cdns_i3c_master *master = to_cdns_i3c_master(m); + u32 status; + int ret; + + status = readl(master->regs + MST_STATUS0); + if (status & MST_STATUS0_MASTER_MODE) + return -EEXIST; + + status = readl(master->regs + SLV_STATUS1); + if (status & SLV_STATUS1_MR_DIS) + return -EBUSY; + + writel(readl(master->regs + CTRL) | CTRL_MST_INIT | CTRL_MST_ACK, + master->regs + CTRL); + + writel(SLV_INT_MR_DONE, master->regs + SLV_IER); + + ret = readl_poll_timeout(master->regs + MST_STATUS0, status, + status & MST_STATUS0_MASTER_MODE, 100, + 1000000); + + return ret; +} + +static void cdns_i3c_master_update_devs(struct i3c_master_controller *m) +{ + struct cdns_i3c_master *master = to_cdns_i3c_master(m); + u32 val, newdevs; + int slot; + struct i3c_device_info i3c_info; + struct i2c_dev_boardinfo i2c_info; + + newdevs = readl(master->regs + DEVS_CTRL) & DEVS_CTRL_DEVS_ACTIVE_MASK; + for (slot = 1; slot <= master->maxdevs; slot++) { + val = readl(master->regs + DEV_ID_RR0(slot)); + + if ((newdevs & BIT(slot)) && (val & DEV_ID_RR0_IS_I3C)) { + cdns_i3c_master_i3c_dev_rr_to_info(master, slot, + &i3c_info); + i3c_master_add_i3c_dev_locked(m, i3c_info.dyn_addr); + } else if ((newdevs & BIT(slot)) && + !(val & DEV_ID_RR0_IS_I3C)) { + cdns_i3c_master_i2c_dev_rr_to_boardinfo(master, slot, + &i2c_info); + i3c_master_add_i2c_dev(m, &i2c_info); + } + } +} + static enum i3c_error_code cdns_i3c_cmd_get_err(struct cdns_i3c_cmd *cmd) { switch (cmd->error) { @@ -945,6 +1033,8 @@ static int cdns_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev, return 0; } +static int cdns_i3c_master_enable_mastership(struct i3c_dev_desc *dev); + static int cdns_i3c_master_attach_i3c_dev(struct i3c_dev_desc *dev) { struct i3c_master_controller *m = i3c_dev_get_master(dev); @@ -1045,22 +1135,6 @@ static void cdns_i3c_master_bus_cleanup(struct i3c_master_controller *m) cdns_i3c_master_disable(master); } -static void cdns_i3c_master_dev_rr_to_info(struct cdns_i3c_master *master, - unsigned int slot, - struct i3c_device_info *info) -{ - u32 rr; - - memset(info, 0, sizeof(*info)); - rr = readl(master->regs + DEV_ID_RR0(slot)); - info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr); - rr = readl(master->regs + DEV_ID_RR2(slot)); - info->dcr = rr; - info->bcr = rr >> 8; - info->pid = rr >> 16; - info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16; -} - static void cdns_i3c_master_upd_i3c_scl_lim(struct cdns_i3c_master *master) { struct i3c_master_controller *m = &master->base; @@ -1259,15 +1333,17 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m) prescl1 = PRESCL_CTRL1_OD_LOW(ncycles); writel(prescl1, master->regs + PRESCL_CTRL1); - /* Get an address for the master. */ - ret = i3c_master_get_free_addr(m, 0); - if (ret < 0) - return ret; + if (!m->secondary) { + /* Get an address for the master. */ + ret = i3c_master_get_free_addr(m, 0); + if (ret < 0) + return ret; - writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C, - master->regs + DEV_ID_RR0(0)); + writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C, + master->regs + DEV_ID_RR0(0)); + } - cdns_i3c_master_dev_rr_to_info(master, 0, &info); + cdns_i3c_master_i3c_dev_rr_to_info(master, 0, &info); if (info.bcr & I3C_BCR_HDR_CAP) info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR); @@ -1289,6 +1365,23 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m) return 0; } +static +struct i3c_dev_desc *cdns_i3c_get_ibi_device(struct cdns_i3c_master *master, + u32 ibir) +{ + struct i3c_dev_desc *dev; + u32 id = IBIR_SLVID(ibir); + + if (id >= master->ibi.num_slots || (ibir & IBIR_ERROR)) + return NULL; + + dev = master->ibi.slots[id]; + if (!dev) + return NULL; + + return dev; +} + static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master, u32 ibir) { @@ -1366,27 +1459,79 @@ static void cnds_i3c_master_demux_ibis(struct cdns_i3c_master *master) case IBIR_TYPE_MR: WARN_ON(IBIR_XFER_BYTES(ibir) || (ibir & IBIR_ERROR)); + master->mastership.ibir = ibir; + queue_work(master->base.wq, + &master->mastership.handover_work); + break; default: break; } } } +static void cdns_i3c_master_bus_handoff(struct cdns_i3c_master *master) +{ + struct i3c_dev_desc *dev; + + dev = cdns_i3c_get_ibi_device(master, master->mastership.ibir); + + writel(MST_INT_MR_DONE, master->regs + MST_ICR); + i3c_master_switch_operation_mode(&master->base, dev, I3C_SLAVE_MODE); +} + +static void cdns_i3c_sec_master_takeover(struct cdns_i3c_master *master) +{ + writel(SLV_INT_MR_DONE, master->regs + SLV_ICR); + i3c_master_switch_operation_mode(&master->base, + master->base.this, I3C_MASTER_MODE); +} + +static void cdns_i3c_sec_master_event_up(struct cdns_i3c_master *master) +{ + u32 status; + u8 evts = 0; + + writel(SLV_INT_EVENT_UP, master->regs + SLV_ICR); + status = readl(master->regs + SLV_STATUS1); + if (!(status & SLV_STATUS1_MR_DIS)) + evts |= I3C_CCC_EVENT_MR; + if (!(status & SLV_STATUS1_HJ_DIS)) + evts |= I3C_CCC_EVENT_HJ; + + i3c_secondary_master_events_enabled(&master->base, evts); +} + static irqreturn_t cdns_i3c_master_interrupt(int irq, void *data) { struct cdns_i3c_master *master = data; u32 status; - status = readl(master->regs + MST_ISR); - if (!(status & readl(master->regs + MST_IMR))) - return IRQ_NONE; + if (master->base.op_mode == I3C_SLAVE_MODE) { + status = readl(master->regs + SLV_ISR); - spin_lock(&master->xferqueue.lock); - cdns_i3c_master_end_xfer_locked(master, status); - spin_unlock(&master->xferqueue.lock); + if (!(status & readl(master->regs + SLV_IMR))) + return IRQ_NONE; - if (status & MST_INT_IBIR_THR) - cnds_i3c_master_demux_ibis(master); + if (status & SLV_INT_MR_DONE) + cdns_i3c_sec_master_takeover(master); + + if (status & SLV_INT_EVENT_UP) + cdns_i3c_sec_master_event_up(master); + } else { + status = readl(master->regs + MST_ISR); + if (!(status & readl(master->regs + MST_IMR))) + return IRQ_NONE; + + spin_lock(&master->xferqueue.lock); + cdns_i3c_master_end_xfer_locked(master, status); + spin_unlock(&master->xferqueue.lock); + + if (status & MST_INT_IBIR_THR) + cnds_i3c_master_demux_ibis(master); + + if (status & MST_INT_MR_DONE) + cdns_i3c_master_bus_handoff(master); + } return IRQ_HANDLED; } @@ -1455,30 +1600,46 @@ static int cdns_i3c_master_enable_ibi(struct i3c_dev_desc *dev) return ret; } -static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev, - const struct i3c_ibi_setup *req) +static int cdns_i3c_master_find_ibi_slot(struct cdns_i3c_master *master, + struct i3c_dev_desc *dev, + s16 *slot) { - struct i3c_master_controller *m = i3c_dev_get_master(dev); - struct cdns_i3c_master *master = to_cdns_i3c_master(m); - struct cdns_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev); unsigned long flags; unsigned int i; - - data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req); - if (IS_ERR(data->ibi_pool)) - return PTR_ERR(data->ibi_pool); + int ret = -ENOENT; spin_lock_irqsave(&master->ibi.lock, flags); for (i = 0; i < master->ibi.num_slots; i++) { - if (!master->ibi.slots[i]) { - data->ibi = i; + /* + * We only need 'SIR' slots to describe IBI-capable devices. + * This slot may be used by mastership request interrupt, + * We can ruse the same 'SIR' map entry. + */ + if (!master->ibi.slots[i] || + master->ibi.slots[i] == dev) { master->ibi.slots[i] = dev; + *slot = i; + ret = 0; break; } } spin_unlock_irqrestore(&master->ibi.lock, flags); - if (i < master->ibi.num_slots) + return ret; +} + +static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev, + const struct i3c_ibi_setup *req) +{ + struct i3c_master_controller *m = i3c_dev_get_master(dev); + struct cdns_i3c_master *master = to_cdns_i3c_master(m); + struct cdns_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev); + + data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req); + if (IS_ERR(data->ibi_pool)) + return PTR_ERR(data->ibi_pool); + + if (cdns_i3c_master_find_ibi_slot(master, dev, &data->ibi) == 0) return 0; i3c_generic_ibi_free_pool(data->ibi_pool); @@ -1487,6 +1648,37 @@ static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev, return -ENOSPC; } +static int cdns_i3c_master_enable_mastership(struct i3c_dev_desc *dev) +{ + struct i3c_master_controller *m = i3c_dev_get_master(dev); + struct cdns_i3c_master *master = to_cdns_i3c_master(m); + struct cdns_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev); + unsigned long flags; + u32 sircfg, sirmap; + int ret; + + ret = cdns_i3c_master_find_ibi_slot(master, dev, &data->ibi); + if (ret) + return -ENOSPC; + + spin_lock_irqsave(&master->ibi.lock, flags); + sirmap = readl(master->regs + SIR_MAP_DEV_REG(data->ibi)); + sirmap &= ~SIR_MAP_DEV_CONF_MASK(data->ibi); + sircfg = SIR_MAP_DEV_ROLE(dev->info.bcr >> 6) | + SIR_MAP_DEV_DA(dev->info.dyn_addr) | + SIR_MAP_DEV_PL(dev->info.max_ibi_len) | + SIR_MAP_DEV_ACK; + + if (dev->info.bcr & I3C_BCR_MAX_DATA_SPEED_LIM) + sircfg |= SIR_MAP_DEV_SLOW; + + sirmap |= SIR_MAP_DEV_CONF(data->ibi, sircfg); + writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi)); + spin_unlock_irqrestore(&master->ibi.lock, flags); + + return 0; +} + static void cdns_i3c_master_free_ibi(struct i3c_dev_desc *dev) { struct i3c_master_controller *m = i3c_dev_get_master(dev); @@ -1529,6 +1721,9 @@ static const struct i3c_master_controller_ops cdns_i3c_master_ops = { .request_ibi = cdns_i3c_master_request_ibi, .free_ibi = cdns_i3c_master_free_ibi, .recycle_ibi_slot = cdns_i3c_master_recycle_ibi_slot, + .request_mastership = cdns_i3c_sec_master_request_mastership, + .update_devs = cdns_i3c_master_update_devs, + .enable_mastership = cdns_i3c_master_enable_mastership }; static void cdns_i3c_master_hj(struct work_struct *work) @@ -1537,13 +1732,31 @@ static void cdns_i3c_master_hj(struct work_struct *work) struct cdns_i3c_master, hj_work); - i3c_master_do_daa(&master->base); + if (!master->base.secondary) + i3c_master_do_daa(&master->base); +} + +static void cdns_i3c_master_mastership_handover(struct work_struct *work) +{ + struct cdns_i3c_master *master = container_of(work, + struct cdns_i3c_master, + mastership.handover_work); + struct i3c_dev_desc *dev; + u32 ibir = master->mastership.ibir; + + dev = cdns_i3c_get_ibi_device(master, ibir); + if (!dev) + return; + + if (i3c_master_mastership_ack(&master->base, &dev->info)) + dev_err(&master->base.dev, "Mastership handover failed\n"); } static int cdns_i3c_master_probe(struct platform_device *pdev) { struct cdns_i3c_master *master; struct resource *res; + bool secondary = false; int ret, irq; u32 val; @@ -1585,15 +1798,22 @@ static int cdns_i3c_master_probe(struct platform_device *pdev) INIT_LIST_HEAD(&master->xferqueue.list); INIT_WORK(&master->hj_work, cdns_i3c_master_hj); + INIT_WORK(&master->mastership.handover_work, + cdns_i3c_master_mastership_handover); writel(0xffffffff, master->regs + MST_IDR); writel(0xffffffff, master->regs + SLV_IDR); ret = devm_request_irq(&pdev->dev, irq, cdns_i3c_master_interrupt, 0, dev_name(&pdev->dev), master); + if (ret) goto err_disable_sysclk; platform_set_drvdata(pdev, master); + val = readl(master->regs + MST_STATUS0); + if (!(val & MST_STATUS0_MASTER_MODE)) + secondary = true; + val = readl(master->regs + CONF_STATUS0); /* Device ID0 is reserved to describe this master. */ @@ -1609,6 +1829,7 @@ static int cdns_i3c_master_probe(struct platform_device *pdev) spin_lock_init(&master->ibi.lock); master->ibi.num_slots = CONF_STATUS1_IBI_HW_RES(val); + master->ibi.slots = devm_kcalloc(&pdev->dev, master->ibi.num_slots, sizeof(*master->ibi.slots), GFP_KERNEL); @@ -1620,10 +1841,14 @@ static int cdns_i3c_master_probe(struct platform_device *pdev) writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL); ret = i3c_master_register(&master->base, &pdev->dev, - &cdns_i3c_master_ops, false); + &cdns_i3c_master_ops, secondary); if (ret) goto err_disable_sysclk; + if (secondary) + writel(SLV_INT_EVENT_UP, + master->regs + SLV_IER); + return 0; err_disable_sysclk: @@ -1666,6 +1891,7 @@ static struct platform_driver cdns_i3c_master = { module_platform_driver(cdns_i3c_master); MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); +MODULE_AUTHOR("Przemyslaw Gaj <pgaj@cadence.com>"); MODULE_DESCRIPTION("Cadence I3C master driver"); MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:cdns-i3c-master"); -- 2.4.5 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Add the I3C mastership request 2018-12-13 12:27 [PATCH 0/2] Add the I3C mastership request Przemyslaw Gaj 2018-12-13 12:28 ` [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj 2018-12-13 12:28 ` [PATCH 2/2] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj @ 2018-12-13 12:47 ` Boris Brezillon 2 siblings, 0 replies; 9+ messages in thread From: Boris Brezillon @ 2018-12-13 12:47 UTC (permalink / raw) To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares Hi Przemek, When you resend a patch or a series, you should prefix it with [PATCH RESEND] and explain why you're resending it. On Thu, 13 Dec 2018 12:27:59 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > This patch series adds support for mastership request to I3C > subsystem and Cadence I3C master driver. Mastership request > allows slave to become the master of the I3C bus. Thanks for working on that. > > Przemyslaw Gaj (2): > i3c: Add support for mastership request to I3C subsystem > i3c: master: cdns: add support for mastership request to Cadence I3C > master driver. > > drivers/i3c/master.c | 260 ++++++++++++++++++++++++++-- > drivers/i3c/master/i3c-master-cdns.c | 316 ++++++++++++++++++++++++++++++----- > include/linux/i3c/master.h | 43 ++++- > 3 files changed, 560 insertions(+), 59 deletions(-) > _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-12-19 14:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-13 12:27 [PATCH 0/2] Add the I3C mastership request Przemyslaw Gaj 2018-12-13 12:28 ` [PATCH 1/2] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj 2018-12-13 13:50 ` Boris Brezillon 2018-12-14 7:14 ` Przemyslaw Gaj 2018-12-14 8:24 ` Boris Brezillon 2018-12-14 13:47 ` Przemyslaw Gaj 2018-12-19 14:16 ` Boris Brezillon 2018-12-13 12:28 ` [PATCH 2/2] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj 2018-12-13 12:47 ` [PATCH 0/2] Add the I3C mastership request Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).