* [PATCH v2 0/3] Add the I3C mastership request
@ 2019-01-11 17:43 Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-11 17:43 UTC (permalink / raw)
To: bbrezillon; +Cc: linux-i3c, 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.
Main changes between initial version and v2 are:
- Reworked devices registration on secondary master side
- Reworked mastership event disabling/enabling
- Reworked bus locking related to mastership takeover process
- Added DEFSLVS devices registration during initialization
- Fixed style issues
Przemyslaw Gaj (3):
i3c: Add support for mastership request to I3C subsystem
i3c: master: cdns: add support for mastership request to Cadence I3C
master driver.
i3c: master: Add module author
drivers/i3c/master.c | 258 +++++++++++++++++++++--
drivers/i3c/master/i3c-master-cdns.c | 385 ++++++++++++++++++++++++++++++-----
include/linux/i3c/master.h | 29 ++-
3 files changed, 611 insertions(+), 61 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] 14+ messages in thread
* [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-11 17:43 [PATCH v2 0/3] Add the I3C mastership request Przemyslaw Gaj
@ 2019-01-11 17:43 ` Przemyslaw Gaj
2019-01-15 21:09 ` Boris Brezillon
2019-01-11 17:43 ` [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 3/3] i3c: master: Add module author Przemyslaw Gaj
2 siblings, 1 reply; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-11 17:43 UTC (permalink / raw)
To: bbrezillon; +Cc: linux-i3c, 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>
Changes in v2:
- Add mastership disable event hook
- Changed name of mastership enable event hook
- Add function to test if master owns the bus
- Removed op_mode
- Changed parameter of i3c_master_get_accmst_locked, no need to
pass full i3c_device_info
- Removed redundant DEFSLVS command before GETACCMST
- Add i3c_master_bus_takeover function. There is a need to lock
the bus before adding devices and no matter of the controller
devices have to be added after mastership takeover.
- Add device registration during initialization on secondary master
side. Devices received by DEFSLVS (if occured). If not, device
initialization is deffered untill next mastership request.
---
drivers/i3c/master.c | 256 ++++++++++++++++++++++++++++++++++++++++++---
include/linux/i3c/master.h | 29 ++++-
2 files changed, 270 insertions(+), 15 deletions(-)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 68d6553..320d905 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -339,6 +339,37 @@ static int i3c_device_probe(struct device *dev)
return driver->probe(i3cdev);
}
+static void i3c_master_enable_mr_events(struct i3c_dev_desc *i3cdev)
+{
+ struct i3c_master_controller *master = i3c_dev_get_master(i3cdev);
+
+ if (!i3cdev)
+ return;
+
+ if (!master->ops->enable_mr_events)
+ return;
+
+ if (master->ops->enable_mr_events(i3cdev))
+ dev_warn(&master->dev,
+ "Failed to enable mastership for device: %d%llx",
+ master->bus.id, i3cdev->info.pid);
+}
+
+static void i3c_master_disable_mr_events(struct i3c_dev_desc *i3cdev)
+{
+ struct i3c_master_controller *master = i3c_dev_get_master(i3cdev);
+
+ if (!i3cdev)
+ return;
+
+ if (!master->ops->disable_mr_events)
+ return;
+
+ if (master->ops->disable_mr_events(i3cdev))
+ dev_warn(&master->dev,
+ "Failed to disable mastership for device: %d%llx",
+ master->bus.id, i3cdev->info.pid);
+}
static int i3c_device_remove(struct device *dev)
{
struct i3c_device *i3cdev = dev_to_i3cdev(dev);
@@ -351,6 +382,9 @@ static int i3c_device_remove(struct device *dev)
i3c_device_free_ibi(i3cdev);
+ if (I3C_BCR_DEVICE_ROLE(i3cdev->desc->info.bcr) == I3C_BCR_I3C_MASTER)
+ i3c_master_disable_mr_events(i3cdev->desc);
+
return ret;
}
@@ -460,6 +494,26 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
return 0;
}
+static int i3c_master_request_mastership(struct i3c_master_controller *master)
+{
+ if (WARN_ON(master->init_done &&
+ !rwsem_is_locked(&master->bus.lock)))
+ return -EINVAL;
+
+ if (!master->ops->request_mastership)
+ return -ENOTSUPP;
+
+ if (master->ops->request_mastership(master))
+ return -EIO;
+
+ return 0;
+}
+
+static bool i3c_master_owns_bus(struct i3c_master_controller *master)
+{
+ return master->bus.cur_master == master->this;
+}
+
static const char * const i3c_bus_mode_strings[] = {
[I3C_BUS_MODE_PURE] = "pure",
[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
@@ -491,8 +545,19 @@ 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);
+ i3c_bus_normaluse_lock(i3cbus);
+ ret = i3c_master_request_mastership(master);
+ if (ret)
+ return sprintf(buf, "unknown\n");
+ i3c_bus_normaluse_unlock(i3cbus);
+ }
+
i3c_bus_normaluse_lock(i3cbus);
ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
i3cbus->cur_master->info.pid);
@@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
!rwsem_is_locked(&master->bus.lock)))
return -EINVAL;
+ if (!i3c_master_owns_bus(master)) {
+ ret = i3c_master_request_mastership(master);
+ if (ret)
+ return ret;
+ }
+
if (!master->ops->send_ccc_cmd)
return -ENOTSUPP;
@@ -1491,6 +1562,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,
+ u8 addr)
+{
+ struct i3c_ccc_getaccmst *accmst;
+ struct i3c_ccc_cmd_dest dest;
+ struct i3c_ccc_cmd cmd;
+ int ret;
+
+ accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
+ if (!accmst)
+ return -ENOMEM;
+
+ i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
+
+ ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+
+ if (ret)
+ 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,12 +1665,9 @@ 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)
- return -EINVAL;
+ if (!master->secondary)
+ if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
+ return -EINVAL;
if (master->this)
return -EINVAL;
@@ -1569,7 +1677,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->secondary)
+ master->bus.cur_master = master->this;
ret = i3c_master_attach_i3c_dev(master, i3cdev);
if (ret)
@@ -1727,6 +1836,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
}
/*
+ * Don't reset addresses if this is secondary master.
+ * Secondary masters can't do DAA.
+ */
+ if (master->secondary)
+ return 0;
+
+ /*
* Reset all dynamic address that may have been assigned before
* (assigned by the bootloader for example).
*/
@@ -1789,6 +1905,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 +1986,9 @@ 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)
+ i3c_master_enable_mr_events(newdev);
+
olddev = i3c_master_search_i3c_dev_duplicate(newdev);
if (olddev) {
newdev->boardinfo = olddev->boardinfo;
@@ -2103,6 +2260,12 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
}
i3c_bus_normaluse_lock(&master->bus);
+ if (!i3c_master_owns_bus(master)) {
+ ret = i3c_master_request_mastership(master);
+ if (ret)
+ return ret;
+ }
+
dev = i3c_master_find_i2c_dev_by_addr(master, addr);
if (!dev)
ret = -ENOENT;
@@ -2390,18 +2553,44 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
!ops->recycle_ibi_slot))
return -EINVAL;
+ if (ops->request_mastership &&
+ (!ops->update_devs || !ops->enable_mr_events ||
+ !ops->disable_mr_events))
+ return -EINVAL;
+
return 0;
}
/**
+ * i3c_master_bus_takeover() - register new I3C devices on bus takeover
+ * @master: master used to send frames on the bus
+ *
+ * This function is useful when devices were not added
+ * during initialization or when new device joined the bus
+ * which was under control of different master.
+ */
+void i3c_master_bus_takeover(struct i3c_master_controller *master)
+{
+ 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.
+ */
+ i3c_bus_normaluse_lock(&master->bus);
+ i3c_master_register_new_i3c_devs(master);
+ i3c_bus_normaluse_unlock(&master->bus);
+}
+EXPORT_SYMBOL_GPL(i3c_master_bus_takeover);
+
+/**
* 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 +2613,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;
@@ -2497,6 +2682,16 @@ int i3c_master_register(struct i3c_master_controller *master,
goto err_del_dev;
/*
+ * Try to attach devices received by DEFSLVS.
+ * Secondary masters can't do DAA.
+ */
+ if (master->secondary) {
+ i3c_bus_normaluse_lock(&master->bus);
+ master->ops->update_devs(master);
+ i3c_bus_normaluse_unlock(&master->bus);
+ }
+
+ /*
* We're done initializing the bus and the controller, we can now
* register I3C devices dicovered during the initial DAA.
*/
@@ -2521,6 +2716,32 @@ int i3c_master_register(struct i3c_master_controller *master,
EXPORT_SYMBOL_GPL(i3c_master_register);
/**
+ * i3c_master_mastership_ack() - performs operations before bus handover.
+ * @master: master used to send frames on the bus
+ * @addr: I3C device address
+ *
+ * Basically, it sends DEFSLVS command to ensure new master is taking
+ * the bus with complete list of devices and then acknowledges bus takeover.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int i3c_master_mastership_ack(struct i3c_master_controller *master,
+ u8 addr)
+{
+ int ret;
+
+ i3c_bus_maintenance_lock(&master->bus);
+ ret = i3c_master_get_accmst_locked(master, addr);
+ i3c_bus_maintenance_unlock(&master->bus);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(i3c_master_mastership_ack);
+
+
+/**
* i3c_master_unregister() - unregister an I3C master
* @master: master used to send frames on the bus
*
@@ -2544,6 +2765,7 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
int nxfers)
{
struct i3c_master_controller *master;
+ int ret;
if (!dev)
return -ENOENT;
@@ -2552,6 +2774,12 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
if (!master || !xfers)
return -EINVAL;
+ if (!i3c_master_owns_bus(master)) {
+ ret = i3c_master_request_mastership(master);
+ if (ret)
+ return ret;
+ }
+
if (!master->ops->priv_xfers)
return -ENOTSUPP;
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index f13fd8b..16e7995 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -418,6 +418,21 @@ 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_mr_events: enable the Mastership event for specified device.
+ * Mastership does not require handler. Mastership is enabled
+ * for all masters by default.
+ * @disable_mr_events: disable the Mastership event for specified device.
*/
struct i3c_master_controller_ops {
int (*bus_init)(struct i3c_master_controller *master);
@@ -445,6 +460,10 @@ struct i3c_master_controller_ops {
int (*disable_ibi)(struct i3c_dev_desc *dev);
void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
struct i3c_ibi_slot *slot);
+ void (*update_devs)(struct i3c_master_controller *master);
+ int (*request_mastership)(struct i3c_master_controller *master);
+ int (*enable_mr_events)(struct i3c_dev_desc *dev);
+ int (*disable_mr_events)(struct i3c_dev_desc *dev);
};
/**
@@ -523,7 +542,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 +556,13 @@ int i3c_master_register(struct i3c_master_controller *master,
const struct i3c_master_controller_ops *ops,
bool secondary);
int i3c_master_unregister(struct i3c_master_controller *master);
+int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
+ u8 addr);
+int i3c_master_mastership_ack(struct i3c_master_controller *master,
+ u8 addr);
+void i3c_secondary_master_events_enabled(struct i3c_master_controller *master,
+ u8 evts);
+void i3c_master_bus_takeover(struct i3c_master_controller *master);
/**
* 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] 14+ messages in thread
* [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.
2019-01-11 17:43 [PATCH v2 0/3] Add the I3C mastership request Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
@ 2019-01-11 17:43 ` Przemyslaw Gaj
2019-01-15 21:36 ` Boris Brezillon
2019-01-11 17:43 ` [PATCH v2 3/3] i3c: master: Add module author Przemyslaw Gaj
2 siblings, 1 reply; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-11 17:43 UTC (permalink / raw)
To: bbrezillon; +Cc: linux-i3c, 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>
Changes in v2:
- Add work structs for mastership purpose
- Add missing mastership disable feature
---
drivers/i3c/master/i3c-master-cdns.c | 385 ++++++++++++++++++++++++++++++-----
1 file changed, 339 insertions(+), 46 deletions(-)
diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index 02a8297..578fc1e 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,12 @@ struct cdns_i3c_xfer {
struct cdns_i3c_master {
struct work_struct hj_work;
+ struct {
+ struct work_struct handover_work;
+ struct work_struct takeover_work;
+ struct work_struct request_work;
+ u32 ibir;
+ } mastership;
struct i3c_master_controller base;
u32 free_rr_slots;
unsigned int maxdevs;
@@ -664,6 +671,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) {
@@ -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,105 @@ 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);
+
+ master->base.bus.cur_master = dev;
+}
+
+static void cdns_i3c_master_mastership_takeover(struct work_struct *work)
+{
+ struct cdns_i3c_master *master = container_of(work,
+ struct cdns_i3c_master,
+ mastership.takeover_work);
+
+ writel(SLV_INT_MR_DONE, master->regs + SLV_ICR);
+
+ master->base.bus.cur_master = master->base.this;
+
+ if (master->base.init_done)
+ i3c_master_bus_takeover(&master->base);
+
+ writel(readl(master->regs + CTRL) & ~CTRL_MST_ACK, master->regs + CTRL);
+}
+
+static void cdns_i3c_sec_master_mastership_request(struct work_struct *work)
+{
+ struct cdns_i3c_master *master = container_of(work,
+ struct cdns_i3c_master,
+ mastership.request_work);
+
+ if (cdns_i3c_sec_master_request_mastership(&master->base))
+ dev_err(&master->base.dev, "Mastership takeover failed\n");
+ else
+ dev_err(&master->base.dev, "Mastership takeover succeed\n");
+}
+
+static void cdns_i3c_sec_master_event_up(struct cdns_i3c_master *master)
+{
+ u32 status;
+
+ writel(SLV_INT_EVENT_UP, master->regs + SLV_ICR);
+ status = readl(master->regs + SLV_STATUS1);
+ if (!(status & SLV_STATUS1_MR_DIS) &&
+ !master->base.bus.cur_master) {
+ queue_work(master->base.wq, &master->mastership.request_work);
+ }
+}
+
+static void cdns_i3c_sec_mastership_done(struct cdns_i3c_master *master)
+{
+ writel(SLV_INT_MR_DONE, master->regs + SLV_ICR);
+
+ queue_work(master->base.wq, &master->mastership.takeover_work);
+}
+
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.bus.cur_master != master->base.this) {
+ 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 & SLV_INT_MR_DONE)
+ cdns_i3c_sec_mastership_done(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;
- if (status & MST_INT_IBIR_THR)
- cnds_i3c_master_demux_ibis(master);
+ 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 +1626,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 +1674,37 @@ static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev,
return -ENOSPC;
}
+static int cdns_i3c_master_enable_mastership_events(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);
@@ -1502,6 +1720,39 @@ static void cdns_i3c_master_free_ibi(struct i3c_dev_desc *dev)
i3c_generic_ibi_free_pool(data->ibi_pool);
}
+static int cdns_i3c_master_disable_mastership_events(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 sirmap;
+ int ret;
+
+ ret = i3c_master_disec_locked(m, dev->info.dyn_addr,
+ I3C_CCC_EVENT_MR);
+ if (ret)
+ return ret;
+
+ if (data->ibi < 0)
+ return -ENOENT;
+
+ if (master->ibi.slots[data->ibi]->ibi->handler)
+ return 0;
+
+ 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);
+ sirmap |= SIR_MAP_DEV_CONF(data->ibi,
+ SIR_MAP_DEV_DA(I3C_BROADCAST_ADDR));
+ writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
+ spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+ cdns_i3c_master_free_ibi(dev);
+
+ return ret;
+}
+
static void cdns_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
struct i3c_ibi_slot *slot)
{
@@ -1529,6 +1780,10 @@ 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_mr_events = cdns_i3c_master_enable_mastership_events,
+ .disable_mr_events = cdns_i3c_master_disable_mastership_events
};
static void cdns_i3c_master_hj(struct work_struct *work)
@@ -1537,13 +1792,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.dyn_addr))
+ 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 +1858,27 @@ 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);
+ INIT_WORK(&master->mastership.takeover_work,
+ cdns_i3c_master_mastership_takeover);
+ INIT_WORK(&master->mastership.request_work,
+ cdns_i3c_sec_master_mastership_request);
+
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 +1894,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);
@@ -1617,13 +1903,19 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
writel(IBIR_THR(1), master->regs + CMD_IBI_THR_CTRL);
writel(MST_INT_IBIR_THR, master->regs + MST_IER);
- writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
+
+ if (!secondary)
+ 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 +1958,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] 14+ messages in thread
* [PATCH v2 3/3] i3c: master: Add module author
2019-01-11 17:43 [PATCH v2 0/3] Add the I3C mastership request Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
@ 2019-01-11 17:43 ` Przemyslaw Gaj
2019-01-15 21:11 ` Boris Brezillon
2 siblings, 1 reply; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-11 17:43 UTC (permalink / raw)
To: bbrezillon; +Cc: linux-i3c, Przemyslaw Gaj, psroka, rafalc, vitor.soares
Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
drivers/i3c/master.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 320d905..72b1711 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -3,6 +3,7 @@
* Copyright (C) 2018 Cadence Design Systems Inc.
*
* Author: Boris Brezillon <boris.brezillon@bootlin.com>
+ * Author: Przemyslaw Gaj <pgaj@cadence.com>
*/
#include <linux/atomic.h>
@@ -2885,5 +2886,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");
--
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] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-11 17:43 ` [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
@ 2019-01-15 21:09 ` Boris Brezillon
2019-01-28 10:37 ` Przemyslaw Gaj
2019-01-29 18:13 ` Przemyslaw Gaj
0 siblings, 2 replies; 14+ messages in thread
From: Boris Brezillon @ 2019-01-15 21:09 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Fri, 11 Jan 2019 17:43:35 +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
> - 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.
Should be pretty easy to find the boardinfo entry matching the info
transmitted through the DEFSLVS frame, and if there's no match, we
simply don't expose the I2C dev.
>
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>
> Changes in v2:
> - Add mastership disable event hook
> - Changed name of mastership enable event hook
> - Add function to test if master owns the bus
> - Removed op_mode
> - Changed parameter of i3c_master_get_accmst_locked, no need to
> pass full i3c_device_info
> - Removed redundant DEFSLVS command before GETACCMST
> - Add i3c_master_bus_takeover function. There is a need to lock
> the bus before adding devices and no matter of the controller
> devices have to be added after mastership takeover.
> - Add device registration during initialization on secondary master
> side. Devices received by DEFSLVS (if occured). If not, device
> initialization is deffered untill next mastership request.
> ---
> drivers/i3c/master.c | 256 ++++++++++++++++++++++++++++++++++++++++++---
> include/linux/i3c/master.h | 29 ++++-
> 2 files changed, 270 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 68d6553..320d905 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -339,6 +339,37 @@ static int i3c_device_probe(struct device *dev)
> return driver->probe(i3cdev);
> }
>
> +static void i3c_master_enable_mr_events(struct i3c_dev_desc *i3cdev)
Do we have a good reason for enabling MR events on a per-device basis?
I guess we could limit the MR requests to a sub-set of the available
secondary masters, but I don't see a use for that right now. What we
actually need is a way to accept/reject MR requests globally.
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(i3cdev);
> +
> + if (!i3cdev)
> + return;
> +
> + if (!master->ops->enable_mr_events)
> + return;
> +
> + if (master->ops->enable_mr_events(i3cdev))
> + dev_warn(&master->dev,
> + "Failed to enable mastership for device: %d%llx",
> + master->bus.id, i3cdev->info.pid);
> +}
> +
> +static void i3c_master_disable_mr_events(struct i3c_dev_desc *i3cdev)
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(i3cdev);
> +
> + if (!i3cdev)
> + return;
> +
> + if (!master->ops->disable_mr_events)
> + return;
> +
> + if (master->ops->disable_mr_events(i3cdev))
> + dev_warn(&master->dev,
> + "Failed to disable mastership for device: %d%llx",
> + master->bus.id, i3cdev->info.pid);
> +}
Missing blank line.
> static int i3c_device_remove(struct device *dev)
> {
> struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> @@ -351,6 +382,9 @@ static int i3c_device_remove(struct device *dev)
>
> i3c_device_free_ibi(i3cdev);
>
> + if (I3C_BCR_DEVICE_ROLE(i3cdev->desc->info.bcr) == I3C_BCR_I3C_MASTER)
> + i3c_master_disable_mr_events(i3cdev->desc);
> +
> return ret;
> }
>
> @@ -460,6 +494,26 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
> return 0;
> }
>
> +static int i3c_master_request_mastership(struct i3c_master_controller *master)
Looks like you always call this function with the lock held. Please add
a _lock suffix to make that clear.
> +{
> + if (WARN_ON(master->init_done &&
> + !rwsem_is_locked(&master->bus.lock)))
> + return -EINVAL;
> +
> + if (!master->ops->request_mastership)
> + return -ENOTSUPP;
> +
> + if (master->ops->request_mastership(master))
> + return -EIO;
Please propagate the ret code returned by ->request_mastership()
instead of EIO.
> +
> + return 0;
> +}
> +
> +static bool i3c_master_owns_bus(struct i3c_master_controller *master)
> +{
> + return master->bus.cur_master == master->this;
> +}
> +
> static const char * const i3c_bus_mode_strings[] = {
> [I3C_BUS_MODE_PURE] = "pure",
> [I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> @@ -491,8 +545,19 @@ 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);
> + i3c_bus_normaluse_lock(i3cbus);
I think we should acquire the lock in maintenance mode here.
> + ret = i3c_master_request_mastership(master);
> + if (ret)
> + return sprintf(buf, "unknown\n");
We should restore ownership back to the initial bus owner, or simply
return unknown when ->cur_master is NULL instead of trying to acquire
the bus.
> + i3c_bus_normaluse_unlock(i3cbus);
> + }
> +
> i3c_bus_normaluse_lock(i3cbus);
> ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> i3cbus->cur_master->info.pid);
> @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> !rwsem_is_locked(&master->bus.lock)))
> return -EINVAL;
>
> + if (!i3c_master_owns_bus(master)) {
> + ret = i3c_master_request_mastership(master);
> + if (ret)
> + return ret;
> + }
As I said above, I think bus ownership should be requested in
maintenance mode, which means it has to be done before entering this
function. You can probably start acquiring the lock in write (AKA
maintenance) mode and downgrade it to read (AKA normal) mode before we
start sending the frame (CCC, SDR, I2C or HDR).
> +
> if (!master->ops->send_ccc_cmd)
> return -ENOTSUPP;
>
> @@ -1491,6 +1562,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,
> + u8 addr)
> +{
> + struct i3c_ccc_getaccmst *accmst;
> + struct i3c_ccc_cmd_dest dest;
> + struct i3c_ccc_cmd cmd;
> + int ret;
> +
> + accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> + if (!accmst)
> + return -ENOMEM;
> +
> + i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> +
> + ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +
Remove this blank line.
> + if (ret)
> + return ret;
> +
> + if (dest.payload.len != sizeof(*accmst))
> + return -EIO;
> +
Missing i3c_ccc_cmd_dest_cleanup().
> + 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,12 +1665,9 @@ 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)
> - return -EINVAL;
> + if (!master->secondary)
> + if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
> + return -EINVAL;
>
> if (master->this)
> return -EINVAL;
> @@ -1569,7 +1677,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->secondary)
> + master->bus.cur_master = master->this;
>
> ret = i3c_master_attach_i3c_dev(master, i3cdev);
> if (ret)
> @@ -1727,6 +1836,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> }
>
> /*
> + * Don't reset addresses if this is secondary master.
When is the bitmap initialization happening then? I really think
secondary master init should happen early on and be limited to SW-side
object initialization. We can have a separate function to do that, and
maybe a separate register function too
(i3c_{main,secondary}_master_register()).
> + * 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).
> */
> @@ -1789,6 +1905,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;
I don't think you should allocate the boardinfo here since you're
missing important info like, which device this is.
> +
> + i3c_bus_set_addr_slot_status(&master->bus,
> + info->base.addr,
> + I3C_ADDR_SLOT_I2C_DEV);
The only thing you can do when the I2C dev is added by a master is
reserve the address in the I2C/I3C address space and search in the list
of boardinfo definitions (those extracted from the DT for instance) if
you have one that matches the LVR+I2C-address props. If there's match,
you can allocate and attach the i2c dev, otherwise the I2C address is
just reserved and no devices are exposed.
> +
> + 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 +1986,9 @@ 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)
> + i3c_master_enable_mr_events(newdev);
> +
> olddev = i3c_master_search_i3c_dev_duplicate(newdev);
> if (olddev) {
> newdev->boardinfo = olddev->boardinfo;
> @@ -2103,6 +2260,12 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
> }
>
> i3c_bus_normaluse_lock(&master->bus);
> + if (!i3c_master_owns_bus(master)) {
> + ret = i3c_master_request_mastership(master);
> + if (ret)
> + return ret;
> + }
> +
Same comment as above: this should be done in maintenance mode.
> dev = i3c_master_find_i2c_dev_by_addr(master, addr);
> if (!dev)
> ret = -ENOENT;
> @@ -2390,18 +2553,44 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
> !ops->recycle_ibi_slot))
> return -EINVAL;
>
> + if (ops->request_mastership &&
> + (!ops->update_devs || !ops->enable_mr_events ||
> + !ops->disable_mr_events))
> + return -EINVAL;
> +
> return 0;
> }
>
> /**
> + * i3c_master_bus_takeover() - register new I3C devices on bus takeover
> + * @master: master used to send frames on the bus
> + *
> + * This function is useful when devices were not added
> + * during initialization or when new device joined the bus
> + * which was under control of different master.
> + */
> +void i3c_master_bus_takeover(struct i3c_master_controller *master)
> +{
> + 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.
> + */
> + i3c_bus_normaluse_lock(&master->bus);
> + i3c_master_register_new_i3c_devs(master);
> + i3c_bus_normaluse_unlock(&master->bus);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_bus_takeover);
> +
> +/**
> * 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 +2613,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;
> @@ -2497,6 +2682,16 @@ int i3c_master_register(struct i3c_master_controller *master,
> goto err_del_dev;
>
> /*
> + * Try to attach devices received by DEFSLVS.
> + * Secondary masters can't do DAA.
> + */
> + if (master->secondary) {
> + i3c_bus_normaluse_lock(&master->bus);
> + master->ops->update_devs(master);
> + i3c_bus_normaluse_unlock(&master->bus);
> + }
> +
> + /*
> * We're done initializing the bus and the controller, we can now
> * register I3C devices dicovered during the initial DAA.
> */
> @@ -2521,6 +2716,32 @@ int i3c_master_register(struct i3c_master_controller *master,
> EXPORT_SYMBOL_GPL(i3c_master_register);
>
> /**
> + * i3c_master_mastership_ack() - performs operations before bus handover.
> + * @master: master used to send frames on the bus
> + * @addr: I3C device address
> + *
> + * Basically, it sends DEFSLVS command to ensure new master is taking
> + * the bus with complete list of devices and then acknowledges bus takeover.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> + u8 addr)
> +{
> + int ret;
> +
> + i3c_bus_maintenance_lock(&master->bus);
> + ret = i3c_master_get_accmst_locked(master, addr);
> + i3c_bus_maintenance_unlock(&master->bus);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack);
> +
> +
> +/**
> * i3c_master_unregister() - unregister an I3C master
> * @master: master used to send frames on the bus
> *
> @@ -2544,6 +2765,7 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> int nxfers)
> {
> struct i3c_master_controller *master;
> + int ret;
>
> if (!dev)
> return -ENOENT;
> @@ -2552,6 +2774,12 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> if (!master || !xfers)
> return -EINVAL;
>
> + if (!i3c_master_owns_bus(master)) {
> + ret = i3c_master_request_mastership(master);
> + if (ret)
> + return ret;
> + }
> +
> if (!master->ops->priv_xfers)
> return -ENOTSUPP;
>
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index f13fd8b..16e7995 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -418,6 +418,21 @@ 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.
Do we really need this hook? AFAIU, this is only called from the master
controller's work which is responsible with mastership handover, so
this can be done entirely from the master driver without requiring help
from the framework (just need to do it with the maintenance lock held).
Am I missing something?
> + * @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.
...using a master which is currently
operating in slave mode.
> + * @enable_mr_events: enable the Mastership event for specified device.
> + * Mastership does not require handler. Mastership is enabled
> + * for all masters by default.
> + * @disable_mr_events: disable the Mastership event for specified device.
Unless you have a good reason for having a per-device granularity, I'd
recommend adding hooks that disable/enable MR globally. We can later
add fine grained control if needed.
> */
> struct i3c_master_controller_ops {
> int (*bus_init)(struct i3c_master_controller *master);
> @@ -445,6 +460,10 @@ struct i3c_master_controller_ops {
> int (*disable_ibi)(struct i3c_dev_desc *dev);
> void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
> struct i3c_ibi_slot *slot);
> + void (*update_devs)(struct i3c_master_controller *master);
> + int (*request_mastership)(struct i3c_master_controller *master);
> + int (*enable_mr_events)(struct i3c_dev_desc *dev);
> + int (*disable_mr_events)(struct i3c_dev_desc *dev);
> };
>
> /**
> @@ -523,7 +542,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 +556,13 @@ int i3c_master_register(struct i3c_master_controller *master,
> const struct i3c_master_controller_ops *ops,
> bool secondary);
> int i3c_master_unregister(struct i3c_master_controller *master);
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> + u8 addr);
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> + u8 addr);
> +void i3c_secondary_master_events_enabled(struct i3c_master_controller *master,
> + u8 evts);
> +void i3c_master_bus_takeover(struct i3c_master_controller *master);
>
> /**
> * 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] 14+ messages in thread
* Re: [PATCH v2 3/3] i3c: master: Add module author
2019-01-11 17:43 ` [PATCH v2 3/3] i3c: master: Add module author Przemyslaw Gaj
@ 2019-01-15 21:11 ` Boris Brezillon
2019-01-15 21:40 ` Boris Brezillon
0 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2019-01-15 21:11 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Fri, 11 Jan 2019 17:43:37 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
Please add a commit message, even a short one is better than none.
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> ---
> drivers/i3c/master.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 320d905..72b1711 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -3,6 +3,7 @@
> * Copyright (C) 2018 Cadence Design Systems Inc.
> *
> * Author: Boris Brezillon <boris.brezillon@bootlin.com>
> + * Author: Przemyslaw Gaj <pgaj@cadence.com>
> */
>
> #include <linux/atomic.h>
> @@ -2885,5 +2886,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");
You should probably add an entry for this driver in MAINTAINERS.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.
2019-01-11 17:43 ` [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
@ 2019-01-15 21:36 ` Boris Brezillon
0 siblings, 0 replies; 14+ messages in thread
From: Boris Brezillon @ 2019-01-15 21:36 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Fri, 11 Jan 2019 17:43:36 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> This patch adds support for mastership request to Cadence I3C master driver.
>
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>
> Changes in v2:
> - Add work structs for mastership purpose
> - Add missing mastership disable feature
> ---
> drivers/i3c/master/i3c-master-cdns.c | 385 ++++++++++++++++++++++++++++++-----
> 1 file changed, 339 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index 02a8297..578fc1e 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,12 @@ struct cdns_i3c_xfer {
>
> struct cdns_i3c_master {
> struct work_struct hj_work;
> + struct {
> + struct work_struct handover_work;
> + struct work_struct takeover_work;
> + struct work_struct request_work;
Can't we use the same work for all of them and just have a state or
type field reflecting the kind of MR request?
> + u32 ibir;
> + } mastership;
> struct i3c_master_controller base;
> u32 free_rr_slots;
> unsigned int maxdevs;
> @@ -664,6 +671,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;
This deserves a WARN_ON() as we should not hit that case unless the
driver or core is buggy.
> +
> + status = readl(master->regs + SLV_STATUS1);
> + if (status & SLV_STATUS1_MR_DIS)
> + return -EBUSY;
Maybe EACCES instead of 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) {
> @@ -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,105 @@ 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);
> +
> + master->base.bus.cur_master = dev;
> +}
> +
> +static void cdns_i3c_master_mastership_takeover(struct work_struct *work)
> +{
> + struct cdns_i3c_master *master = container_of(work,
> + struct cdns_i3c_master,
> + mastership.takeover_work);
> +
> + writel(SLV_INT_MR_DONE, master->regs + SLV_ICR);
> +
> + master->base.bus.cur_master = master->base.this;
> +
> + if (master->base.init_done)
> + i3c_master_bus_takeover(&master->base);
> +
> + writel(readl(master->regs + CTRL) & ~CTRL_MST_ACK, master->regs + CTRL);
> +}
> +
> +static void cdns_i3c_sec_master_mastership_request(struct work_struct *work)
> +{
> + struct cdns_i3c_master *master = container_of(work,
> + struct cdns_i3c_master,
> + mastership.request_work);
> +
> + if (cdns_i3c_sec_master_request_mastership(&master->base))
> + dev_err(&master->base.dev, "Mastership takeover failed\n");
> + else
> + dev_err(&master->base.dev, "Mastership takeover succeed\n");
> +}
> +
> +static void cdns_i3c_sec_master_event_up(struct cdns_i3c_master *master)
> +{
> + u32 status;
> +
> + writel(SLV_INT_EVENT_UP, master->regs + SLV_ICR);
> + status = readl(master->regs + SLV_STATUS1);
> + if (!(status & SLV_STATUS1_MR_DIS) &&
> + !master->base.bus.cur_master) {
Hm, don't we need to make sure we actually have something to do? I
mean, we could receive ENEC(MR) event for the second time and actually
have no devices to register. In this case we probably don't want to
send an MR event...
> + queue_work(master->base.wq, &master->mastership.request_work);
> + }
> +}
> +
> +static void cdns_i3c_sec_mastership_done(struct cdns_i3c_master *master)
> +{
> + writel(SLV_INT_MR_DONE, master->regs + SLV_ICR);
> +
> + queue_work(master->base.wq, &master->mastership.takeover_work);
> +}
> +
> 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.bus.cur_master != master->base.this) {
> + 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 & SLV_INT_MR_DONE)
> + cdns_i3c_sec_mastership_done(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;
>
> - if (status & MST_INT_IBIR_THR)
> - cnds_i3c_master_demux_ibis(master);
> + 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 +1626,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) {
Doesn't work if you have empty slots in the middle. You should first
iterate over all entries searching for slots[i] == dev and if there's
no match, pick the first empty entry.
> 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)
if (!....)
> return 0;
>
> i3c_generic_ibi_free_pool(data->ibi_pool);
> @@ -1487,6 +1674,37 @@ static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev,
> return -ENOSPC;
> }
>
> +static int cdns_i3c_master_enable_mastership_events(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;
Please propagate the error instead of returning 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);
> +
Where is i3c_master_enec_locked() called?
> + return 0;
> +}
> +
> static void cdns_i3c_master_free_ibi(struct i3c_dev_desc *dev)
> {
> struct i3c_master_controller *m = i3c_dev_get_master(dev);
> @@ -1502,6 +1720,39 @@ static void cdns_i3c_master_free_ibi(struct i3c_dev_desc *dev)
> i3c_generic_ibi_free_pool(data->ibi_pool);
> }
>
> +static int cdns_i3c_master_disable_mastership_events(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 sirmap;
> + int ret;
> +
> + ret = i3c_master_disec_locked(m, dev->info.dyn_addr,
> + I3C_CCC_EVENT_MR);
> + if (ret)
> + return ret;
> +
> + if (data->ibi < 0)
> + return -ENOENT;
> +
> + if (master->ibi.slots[data->ibi]->ibi->handler)
> + return 0;
> +
> + 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);
> + sirmap |= SIR_MAP_DEV_CONF(data->ibi,
> + SIR_MAP_DEV_DA(I3C_BROADCAST_ADDR));
> + writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
> + spin_unlock_irqrestore(&master->ibi.lock, flags);
> +
> + cdns_i3c_master_free_ibi(dev);
Looks like you're also disabling regular IBIs here, is that really
what you want?
> +
> + return ret;
> +}
> +
> static void cdns_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
> struct i3c_ibi_slot *slot)
> {
> @@ -1529,6 +1780,10 @@ 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_mr_events = cdns_i3c_master_enable_mastership_events,
> + .disable_mr_events = cdns_i3c_master_disable_mastership_events
> };
>
> static void cdns_i3c_master_hj(struct work_struct *work)
> @@ -1537,13 +1792,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.dyn_addr))
> + 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 +1858,27 @@ 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);
> + INIT_WORK(&master->mastership.takeover_work,
> + cdns_i3c_master_mastership_takeover);
> + INIT_WORK(&master->mastership.request_work,
> + cdns_i3c_sec_master_mastership_request);
> +
> 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);
> +
Please do not add blank lines between ret assignments and the following
ret check.
> 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 +1894,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);
> @@ -1617,13 +1903,19 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
>
> writel(IBIR_THR(1), master->regs + CMD_IBI_THR_CTRL);
> writel(MST_INT_IBIR_THR, master->regs + MST_IER);
> - writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
> +
> + if (!secondary)
> + 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 +1958,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");
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] i3c: master: Add module author
2019-01-15 21:11 ` Boris Brezillon
@ 2019-01-15 21:40 ` Boris Brezillon
0 siblings, 0 replies; 14+ messages in thread
From: Boris Brezillon @ 2019-01-15 21:40 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Tue, 15 Jan 2019 22:11:19 +0100
Boris Brezillon <bbrezillon@kernel.org> wrote:
> On Fri, 11 Jan 2019 17:43:37 +0000
> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>
> Please add a commit message, even a short one is better than none.
>
> > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> > ---
> > drivers/i3c/master.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 320d905..72b1711 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -3,6 +3,7 @@
> > * Copyright (C) 2018 Cadence Design Systems Inc.
> > *
> > * Author: Boris Brezillon <boris.brezillon@bootlin.com>
> > + * Author: Przemyslaw Gaj <pgaj@cadence.com>
> > */
> >
> > #include <linux/atomic.h>
> > @@ -2885,5 +2886,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");
>
> You should probably add an entry for this driver in MAINTAINERS.
Didn't notice you were updating a core file here, so please just add
your name+address to the existing I3C entry.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-15 21:09 ` Boris Brezillon
@ 2019-01-28 10:37 ` Przemyslaw Gaj
2019-01-28 12:08 ` Boris Brezillon
2019-01-29 18:13 ` Przemyslaw Gaj
1 sibling, 1 reply; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-28 10:37 UTC (permalink / raw)
To: Boris Brezillon; +Cc: linux-i3c, psroka, rafalc, vitor.soares
The 01/15/2019 22:09, Boris Brezillon wrote:
> EXTERNAL MAIL
>
>
> > @@ -1727,6 +1836,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> > }
> >
> > /*
> > + * Don't reset addresses if this is secondary master.
>
> When is the bitmap initialization happening then? I really think
What do you mean bitmap? Are you asking about slot statuses?
> secondary master init should happen early on and be limited to SW-side
> object initialization. We can have a separate function to do that, and
> maybe a separate register function too
> (i3c_{main,secondary}_master_register()).
Ok, I get it. Because of lack of information at secondary master init time, I
decided to register devices with full information. PID is blocking me. I'll try
to separate register routines.
>
> > + * 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).
> > */
> > index f13fd8b..16e7995 100644
> > --- a/include/linux/i3c/master.h
> > +++ b/include/linux/i3c/master.h
> > @@ -418,6 +418,21 @@ 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.
>
> Do we really need this hook? AFAIU, this is only called from the master
> controller's work which is responsible with mastership handover, so
> this can be done entirely from the master driver without requiring help
> from the framework (just need to do it with the maintenance lock held).
> Am I missing something?
>
Actually, I use this hook at secondary master init time (in framework) to
register devices which could be received by DEFSLVS command. I can register
those devices in the driver. Separate secondary_master_register routine should
help in this case.
--
--
Przemyslaw Gaj
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-28 10:37 ` Przemyslaw Gaj
@ 2019-01-28 12:08 ` Boris Brezillon
0 siblings, 0 replies; 14+ messages in thread
From: Boris Brezillon @ 2019-01-28 12:08 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Mon, 28 Jan 2019 10:37:15 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> The 01/15/2019 22:09, Boris Brezillon wrote:
> > EXTERNAL MAIL
> >
> >
> > > @@ -1727,6 +1836,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> > > }
> > >
> > > /*
> > > + * Don't reset addresses if this is secondary master.
> >
> > When is the bitmap initialization happening then? I really think
>
> What do you mean bitmap? Are you asking about slot statuses?
Yes.
>
> > secondary master init should happen early on and be limited to SW-side
> > object initialization. We can have a separate function to do that, and
> > maybe a separate register function too
> > (i3c_{main,secondary}_master_register()).
>
> Ok, I get it. Because of lack of information at secondary master init time, I
> decided to register devices with full information. PID is blocking me. I'll try
> to separate register routines.
I was talking about master registration, not i3c devices (slaves
connected to the master) registration.
>
> >
> > > + * 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).
> > > */
> > > index f13fd8b..16e7995 100644
> > > --- a/include/linux/i3c/master.h
> > > +++ b/include/linux/i3c/master.h
> > > @@ -418,6 +418,21 @@ 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.
> >
> > Do we really need this hook? AFAIU, this is only called from the master
> > controller's work which is responsible with mastership handover, so
> > this can be done entirely from the master driver without requiring help
> > from the framework (just need to do it with the maintenance lock held).
> > Am I missing something?
> >
>
> Actually, I use this hook at secondary master init time (in framework) to
> register devices which could be received by DEFSLVS command.
Hm, so you're considering the case where DEFSLVS has been received
before the secondary master was registered. Can't you just add those
drivers from the ->bus_init() implementation? I mean, you know when
you're acting as a secondary master, so it shouldn't be hard to have
one ->bus_init() per master type (secondary vs main) and in the
secondary master ->bus_init() add devices reported by the previous
DEFSLVS frame.
> I can register
> those devices in the driver. Separate secondary_master_register routine should
> help in this case.
Yes, having separate register functions should make things clearer and
allow you to customize each patch (for instance, no DAA in case of a
secondary master).
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-15 21:09 ` Boris Brezillon
2019-01-28 10:37 ` Przemyslaw Gaj
@ 2019-01-29 18:13 ` Przemyslaw Gaj
2019-01-30 7:56 ` Boris Brezillon
1 sibling, 1 reply; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-29 18:13 UTC (permalink / raw)
To: Boris Brezillon; +Cc: linux-i3c, psroka, rafalc, vitor.soares
The 01/15/2019 22:09, Boris Brezillon wrote:
> EXTERNAL MAIL
>
>
> On Fri, 11 Jan 2019 17:43:35 +0000
> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>
> > + i3c_bus_normaluse_unlock(i3cbus);
> > + }
> > +
> > i3c_bus_normaluse_lock(i3cbus);
> > ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > i3cbus->cur_master->info.pid);
> > @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> > !rwsem_is_locked(&master->bus.lock)))
> > return -EINVAL;
> >
> > + if (!i3c_master_owns_bus(master)) {
> > + ret = i3c_master_request_mastership(master);
> > + if (ret)
> > + return ret;
> > + }
>
> As I said above, I think bus ownership should be requested in
> maintenance mode, which means it has to be done before entering this
> function. You can probably start acquiring the lock in write (AKA
> maintenance) mode and downgrade it to read (AKA normal) mode before we
> start sending the frame (CCC, SDR, I2C or HDR).
>
So, I'll have to call all the _locked functions with maintenance lock.
Inside i3c_master_send_ccc_cmd_locked() I will downgrade the lock to read
(normal use). Isn't it inconsistent that after _locked functions I will
unlock the bus from normal use, even if I locked it for maintenance?
Also, I will have to downgrade the lock in case of failure in all _locked
functions to unlock the bus properly using i3c_bus_normaluse_lock(). DO you
think this looks good?
--
--
Przemyslaw Gaj
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-29 18:13 ` Przemyslaw Gaj
@ 2019-01-30 7:56 ` Boris Brezillon
2019-01-30 8:29 ` Przemyslaw Gaj
0 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2019-01-30 7:56 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Tue, 29 Jan 2019 18:13:37 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> The 01/15/2019 22:09, Boris Brezillon wrote:
> > EXTERNAL MAIL
> >
> >
> > On Fri, 11 Jan 2019 17:43:35 +0000
> > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >
> > > + i3c_bus_normaluse_unlock(i3cbus);
> > > + }
> > > +
> > > i3c_bus_normaluse_lock(i3cbus);
> > > ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > i3cbus->cur_master->info.pid);
> > > @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> > > !rwsem_is_locked(&master->bus.lock)))
> > > return -EINVAL;
> > >
> > > + if (!i3c_master_owns_bus(master)) {
> > > + ret = i3c_master_request_mastership(master);
> > > + if (ret)
> > > + return ret;
> > > + }
> >
> > As I said above, I think bus ownership should be requested in
> > maintenance mode, which means it has to be done before entering this
> > function. You can probably start acquiring the lock in write (AKA
> > maintenance) mode and downgrade it to read (AKA normal) mode before we
> > start sending the frame (CCC, SDR, I2C or HDR).
> >
>
> So, I'll have to call all the _locked functions with maintenance lock.
> Inside i3c_master_send_ccc_cmd_locked() I will downgrade the lock to read
> (normal use).
No, definitely not. The prefix _locked() means that the lock has been
acquired by the caller, and the function itself is not responsible for
downgrading the lock. What I meant is that the
i3c_master_owns_bus()+i3c_master_request_mastership() sequence does not
belong in xxxx_locked() funcs. It should be done by the caller with the
lock held in write/maintenance mode, and then downgraded to a read/normal
lock once the bus has been acquired. i3c_master_send_ccc_cmd_locked()
was probably not the best example, as this function might be
intentionally called with the lock held in write/maintenance mode
sometimes (depends on the CCC command), but for other funcs, it should
be pretty easy to do:
int i3c_master_acquire_bus_ownership_locked(struct i3c_master_controller *master)
{
if (i3c_master_owns_bus(master))
return 0;
return i3c_master_request_mastership(master);
}
int i3c_device_do_priv_xfers(struct i3c_device *dev,
struct i3c_priv_xfer *xfers,
int nxfers)
{
...
i3c_bus_maintenance_lock(dev->bus);
ret = i3c_master_acquire_bus_ownership(master);
if (ret) {
i3c_bus_maintenance_unlock(dev->bus);
return ret;
}
i3c_bus_downgrade_maintenance_lock(dev->bus);
ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
i3c_bus_normaluse_unlock(dev->bus);
return ret;
}
> Isn't it inconsistent that after _locked functions I will
> unlock the bus from normal use, even if I locked it for maintenance?>
> Also, I will have to downgrade the lock in case of failure in all _locked
> functions to unlock the bus properly using i3c_bus_normaluse_lock(). DO you
> think this looks good?
No, see above for an explanation.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-30 7:56 ` Boris Brezillon
@ 2019-01-30 8:29 ` Przemyslaw Gaj
2019-01-30 8:38 ` Boris Brezillon
0 siblings, 1 reply; 14+ messages in thread
From: Przemyslaw Gaj @ 2019-01-30 8:29 UTC (permalink / raw)
To: Boris Brezillon; +Cc: linux-i3c, psroka, rafalc, vitor.soares
The 01/30/2019 08:56, Boris Brezillon wrote:
> EXTERNAL MAIL
>
>
> On Tue, 29 Jan 2019 18:13:37 +0000
> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>
> > The 01/15/2019 22:09, Boris Brezillon wrote:
> > > EXTERNAL MAIL
> > >
> > >
> > > On Fri, 11 Jan 2019 17:43:35 +0000
> > > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> > >
> > > > + i3c_bus_normaluse_unlock(i3cbus);
> > > > + }
> > > > +
> > > > i3c_bus_normaluse_lock(i3cbus);
> > > > ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > > i3cbus->cur_master->info.pid);
> > > > @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> > > > !rwsem_is_locked(&master->bus.lock)))
> > > > return -EINVAL;
> > > >
> > > > + if (!i3c_master_owns_bus(master)) {
> > > > + ret = i3c_master_request_mastership(master);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > >
> > > As I said above, I think bus ownership should be requested in
> > > maintenance mode, which means it has to be done before entering this
> > > function. You can probably start acquiring the lock in write (AKA
> > > maintenance) mode and downgrade it to read (AKA normal) mode before we
> > > start sending the frame (CCC, SDR, I2C or HDR).
> > >
> >
> > So, I'll have to call all the _locked functions with maintenance lock.
> > Inside i3c_master_send_ccc_cmd_locked() I will downgrade the lock to read
> > (normal use).
>
> No, definitely not. The prefix _locked() means that the lock has been
> acquired by the caller, and the function itself is not responsible for
> downgrading the lock. What I meant is that the
> i3c_master_owns_bus()+i3c_master_request_mastership() sequence does not
> belong in xxxx_locked() funcs. It should be done by the caller with the
> lock held in write/maintenance mode, and then downgraded to a read/normal
> lock once the bus has been acquired. i3c_master_send_ccc_cmd_locked()
> was probably not the best example, as this function might be
> intentionally called with the lock held in write/maintenance mode
> sometimes (depends on the CCC command), but for other funcs, it should
> be pretty easy to do:
>
> int i3c_master_acquire_bus_ownership_locked(struct i3c_master_controller *master)
> {
> if (i3c_master_owns_bus(master))
> return 0;
>
> return i3c_master_request_mastership(master);
> }
>
> int i3c_device_do_priv_xfers(struct i3c_device *dev,
> struct i3c_priv_xfer *xfers,
> int nxfers)
> {
> ...
>
> i3c_bus_maintenance_lock(dev->bus);
> ret = i3c_master_acquire_bus_ownership(master);
> if (ret) {
> i3c_bus_maintenance_unlock(dev->bus);
> return ret;
> }
>
> i3c_bus_downgrade_maintenance_lock(dev->bus);
> ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
> i3c_bus_normaluse_unlock(dev->bus);
>
> return ret;
> }
>
> > Isn't it inconsistent that after _locked functions I will
> > unlock the bus from normal use, even if I locked it for maintenance?>
> > Also, I will have to downgrade the lock in case of failure in all _locked
> > functions to unlock the bus properly using i3c_bus_normaluse_lock(). DO you
> > think this looks good?
>
> No, see above for an explanation.
Thank you for the explanation. It's clear now.
--
--
Przemyslaw Gaj
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
2019-01-30 8:29 ` Przemyslaw Gaj
@ 2019-01-30 8:38 ` Boris Brezillon
0 siblings, 0 replies; 14+ messages in thread
From: Boris Brezillon @ 2019-01-30 8:38 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares
On Wed, 30 Jan 2019 08:29:45 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> The 01/30/2019 08:56, Boris Brezillon wrote:
> > EXTERNAL MAIL
> >
> >
> > On Tue, 29 Jan 2019 18:13:37 +0000
> > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >
> > > The 01/15/2019 22:09, Boris Brezillon wrote:
> > > > EXTERNAL MAIL
> > > >
> > > >
> > > > On Fri, 11 Jan 2019 17:43:35 +0000
> > > > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> > > >
> > > > > + i3c_bus_normaluse_unlock(i3cbus);
> > > > > + }
> > > > > +
> > > > > i3c_bus_normaluse_lock(i3cbus);
> > > > > ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > > > i3cbus->cur_master->info.pid);
> > > > > @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> > > > > !rwsem_is_locked(&master->bus.lock)))
> > > > > return -EINVAL;
> > > > >
> > > > > + if (!i3c_master_owns_bus(master)) {
> > > > > + ret = i3c_master_request_mastership(master);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + }
> > > >
> > > > As I said above, I think bus ownership should be requested in
> > > > maintenance mode, which means it has to be done before entering this
> > > > function. You can probably start acquiring the lock in write (AKA
> > > > maintenance) mode and downgrade it to read (AKA normal) mode before we
> > > > start sending the frame (CCC, SDR, I2C or HDR).
> > > >
> > >
> > > So, I'll have to call all the _locked functions with maintenance lock.
> > > Inside i3c_master_send_ccc_cmd_locked() I will downgrade the lock to read
> > > (normal use).
> >
> > No, definitely not. The prefix _locked() means that the lock has been
> > acquired by the caller, and the function itself is not responsible for
> > downgrading the lock. What I meant is that the
> > i3c_master_owns_bus()+i3c_master_request_mastership() sequence does not
> > belong in xxxx_locked() funcs. It should be done by the caller with the
> > lock held in write/maintenance mode, and then downgraded to a read/normal
> > lock once the bus has been acquired. i3c_master_send_ccc_cmd_locked()
> > was probably not the best example, as this function might be
> > intentionally called with the lock held in write/maintenance mode
> > sometimes (depends on the CCC command), but for other funcs, it should
> > be pretty easy to do:
> >
> > int i3c_master_acquire_bus_ownership_locked(struct i3c_master_controller *master)
> > {
> > if (i3c_master_owns_bus(master))
> > return 0;
> >
> > return i3c_master_request_mastership(master);
> > }
> >
> > int i3c_device_do_priv_xfers(struct i3c_device *dev,
> > struct i3c_priv_xfer *xfers,
> > int nxfers)
> > {
> > ...
> >
> > i3c_bus_maintenance_lock(dev->bus);
> > ret = i3c_master_acquire_bus_ownership(master);
> > if (ret) {
> > i3c_bus_maintenance_unlock(dev->bus);
> > return ret;
> > }
> >
> > i3c_bus_downgrade_maintenance_lock(dev->bus);
> > ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
> > i3c_bus_normaluse_unlock(dev->bus);
> >
> > return ret;
> > }
> >
> > > Isn't it inconsistent that after _locked functions I will
> > > unlock the bus from normal use, even if I locked it for maintenance?>
> > > Also, I will have to downgrade the lock in case of failure in all _locked
> > > functions to unlock the bus properly using i3c_bus_normaluse_lock(). DO you
> > > think this looks good?
> >
> > No, see above for an explanation.
>
> Thank you for the explanation. It's clear now.
>
Just realize we want a fast path for the likely "bus is already owned"
case, so we should actually do something like:
int i3c_device_do_priv_xfers(struct i3c_device *dev,
struct i3c_priv_xfer *xfers,
int nxfers)
{
...
i3c_bus_normaluse_lock(dev->bus);
if (!i3c_master_owns_bus(master)) {
i3c_bus_normaluse_unlock(dev->bus);
i3c_bus_maintenance_lock(dev->bus);
ret = i3c_master_request_mastership(master);
if (ret) {
i3c_bus_maintenance_unlock(dev->bus);
return ret;
}
i3c_bus_downgrade_maintenance_lock(dev->bus);
}
ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
i3c_bus_normaluse_unlock(dev->bus);
return ret;
}
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-01-30 8:38 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 17:43 [PATCH v2 0/3] Add the I3C mastership request Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-01-15 21:09 ` Boris Brezillon
2019-01-28 10:37 ` Przemyslaw Gaj
2019-01-28 12:08 ` Boris Brezillon
2019-01-29 18:13 ` Przemyslaw Gaj
2019-01-30 7:56 ` Boris Brezillon
2019-01-30 8:29 ` Przemyslaw Gaj
2019-01-30 8:38 ` Boris Brezillon
2019-01-11 17:43 ` [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-01-15 21:36 ` Boris Brezillon
2019-01-11 17:43 ` [PATCH v2 3/3] i3c: master: Add module author Przemyslaw Gaj
2019-01-15 21:11 ` Boris Brezillon
2019-01-15 21:40 ` 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).