[RFC 0/2] Add secondary master

[RFC 0/2] Add secondary master

[Vitor Soares]

As agreed I'm sharing with you my secondary master approach based on
[1] and [2]. It is not my goal to use this as alternative solution to
[1] and [2] but to provide you feedback about my challenges. Futher the
following patch should be considered as final work.

During proposal of [1] and [2] I was't able to compile the code directly and
dad need to adjust them to be able to compile dw-i3c-master and found that
they didn'd address my use case.

Apart of what already was done IMO:
  - We may have a secondary master struct to embbed master and slave rules.
  - Algorithm to define the way masters share bus ownership
  - Delay a tranfers when secondary master doesn't bus ownership.

[1] https://patchwork.kernel.org/patch/11011447/
[2] https://patchwork.kernel.org/patch/10846309/

Vitor Soares (2):
  i3c: Add prliminary support for secondary master
  i3c: dw: add preliminary support for sec master

 drivers/i3c/master.c               | 365 +++++++++++++++++++++-
 drivers/i3c/master/dw-i3c-master.c | 619 +++++++++++++++++++++++++++++++++----
 include/linux/i3c/master.h         |  39 +++
 3 files changed, 959 insertions(+), 64 deletions(-)

-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 1/2] i3c: Add preliminary support for secondary master

[Vitor Soares]

This patch adds the preliminary support for secondary master feature to
i3c Framework for testing purposes.

Key points for consideration:
  - mastership_[show/store] are only used for testing
  - secondary master registration is made in two steps, one in
  i3c_master_register() and another in i3c_sec_master_bus_init() when
  secondary master became current master first time. This is made in this
  way to get all dt declared boardinfo list, create defslvs list and
  provide work_queue.
  - When the current master wants to deliver_mastership it necessary to
  disable all in-band events to avoid unwanted interrupt during bus
  ownership exchange. For now this patch doesn't reflect all
  improvements/changes made in v1.1 I3C Bus spec. But it can be extended
  by adding some commands and checks to the flow.
  - i3c_defslvs_info: The DEFSLVS info can be differently stored in
  diferen HC. Hence it is used a defslvs list similar to boardinfo list in
  the bus structure to hold this data. Them HC is taccking over the bus
  ownership can initialize each device of that list. For now, this not
  address the i2c devices since they are only statically described.
  - [request/deliver]_mastership(): Mastership request deliver may be done
  differently in different HC, hence the need to have a call back for each
  process.
  - Add dr_mode to DT: Similar to USB, HC can be programmed to Master only
  mode, Slave only mode or Secondary Master which aren't necessarily
  hardcoded.
  - bus_mode definition: The bus mode is defined even without defslvs
  information with DT info since the definition of i2c devices are those
  that have impact on bus_mode definition and need to statically declared.
  The only use case that may cause issues is when i2c devices aren't
  declared in secondary master side and bus mode doesn't match the
  main master. Anyway this can be solde without extra complexity.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/master.c       | 365 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/i3c/master.h |  39 +++++
 2 files changed, 396 insertions(+), 8 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 0436916..b398d77 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -449,6 +449,46 @@ static ssize_t mode_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(mode);
 
+static ssize_t
+mastership_show(struct device *dev, struct device_attribute *da, char *buf)
+{
+	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
+	ssize_t ret;
+
+	if (master->secondary)
+		ret = sprintf(buf, "Secondary Master\n");
+	else
+		ret = sprintf(buf, "Master\n");
+
+	return ret;
+}
+
+static ssize_t
+mastership_store(struct device *dev, struct device_attribute *attr,
+		 const char *buf, size_t count)
+{
+	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
+	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
+
+	if (i3cbus->cur_master == master->this) {
+		dev_err(dev, "I'm current mater.");
+		return count;
+	}
+
+	if (!master->ops->request_mastership) {
+		dev_err(dev, "mastership_request not supported.");
+		return count;
+	}
+
+	if (master->ops->request_mastership(master))
+		dev_err(dev, "mastership_request failed");
+	else
+		dev_err(dev, "mastership_request success");
+
+	return count;
+}
+static DEVICE_ATTR_RW(mastership);
+
 static ssize_t current_master_show(struct device *dev,
 				   struct device_attribute *da,
 				   char *buf)
@@ -457,8 +497,11 @@ static ssize_t current_master_show(struct device *dev,
 	ssize_t ret;
 
 	i3c_bus_normaluse_lock(i3cbus);
-	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
-		      i3cbus->cur_master->info.pid);
+	if (i3cbus->cur_master)
+		ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
+			      i3cbus->cur_master->info.pid);
+	else
+		ret = sprintf(buf, "Not Current Master\n");
 	i3c_bus_normaluse_unlock(i3cbus);
 
 	return ret;
@@ -497,6 +540,7 @@ static DEVICE_ATTR_RO(i2c_scl_frequency);
 
 static struct attribute *i3c_masterdev_attrs[] = {
 	&dev_attr_mode.attr,
+	&dev_attr_mastership.attr,
 	&dev_attr_current_master.attr,
 	&dev_attr_i3c_scl_frequency.attr,
 	&dev_attr_i2c_scl_frequency.attr,
@@ -854,6 +898,53 @@ int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
 EXPORT_SYMBOL_GPL(i3c_master_enec_locked);
 
 /**
+ * i3c_master_getaccmst_locked() - send an GETACCMST CCC command
+ * @master: master used to send frames on the bus
+ * @addr: a valid I3C slave address
+ *
+ * Sends an GETACCMST CCC command to offer bus Mastership to an
+ * I3C Secondary Master.
+ *
+ * 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_getaccmst_locked(struct i3c_master_controller *master, u8 addr)
+{
+	enum i3c_addr_slot_status addrstat;
+	struct i3c_ccc_getaccmst *accmst;
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	if (!master)
+		return -EINVAL;
+
+	addrstat = i3c_bus_get_addr_slot_status(&master->bus, addr);
+	if (addr == I3C_BROADCAST_ADDR || addrstat != I3C_ADDR_SLOT_I3C_DEV)
+		return -EINVAL;
+
+	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
+	if (!accmst)
+		return -ENOMEM;
+
+	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	if (ret)
+		goto out;
+
+	if (accmst->newmaster >> 1 != addr)
+		ret = -EIO;
+out:
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_getaccmst_locked);
+
+/**
  * i3c_master_defslvs_locked() - send a DEFSLVS CCC command
  * @master: master used to send frames on the bus
  *
@@ -1542,8 +1633,7 @@ int i3c_master_set_info(struct i3c_master_controller *master,
 	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
 		return -EINVAL;
 
-	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
-	    master->secondary)
+	if (I3C_BCR_DEVICE_ROLE(info->bcr) != I3C_BCR_I3C_MASTER)
 		return -EINVAL;
 
 	if (master->this)
@@ -2381,6 +2471,81 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
 	return 0;
 }
 
+int i3c_sec_master_bus_init(struct i3c_master_controller *master)
+{
+	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
+	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
+	enum i3c_bus_mode mode = i3cbus->mode;
+	struct i3c_defslvs_info *defslvsinfo;
+	int ret = 0;
+
+	if (master->init_done)
+		return -EINVAL;
+
+	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
+		if (defslvsinfo->slave.dyn_addr)
+			continue;
+
+		switch (defslvsinfo->slave.lvr & I3C_LVR_I2C_INDEX_MASK) {
+		case I3C_LVR_I2C_INDEX(0):
+			if (mode < I3C_BUS_MODE_MIXED_FAST)
+				mode = I3C_BUS_MODE_MIXED_FAST;
+			break;
+		case I3C_LVR_I2C_INDEX(1):
+		case I3C_LVR_I2C_INDEX(2):
+			if (mode < I3C_BUS_MODE_MIXED_SLOW)
+				mode = I3C_BUS_MODE_MIXED_SLOW;
+			break;
+		default:
+			ret = -EINVAL;
+			goto err_put_dev;
+		}
+		if (defslvsinfo->slave.lvr & I3C_LVR_I2C_FM_MODE)
+			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
+	}
+
+	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
+	if (ret)
+		goto err_put_dev;
+
+	/*
+	 * Now execute the controller specific ->bus_init() routine, which
+	 * might configure its internal logic to match the bus limitations.
+	 */
+	ret = master->ops->bus_init(master);
+	if (ret)
+		goto err_put_dev;
+
+	/*
+	 * The master device should have been instantiated in ->bus_init(),
+	 * complain if this was not the case.
+	 */
+	if (!master->this) {
+		dev_err(&master->dev,
+			"master_set_info() was not called in ->bus_init()\n");
+		ret = -EINVAL;
+		goto err_put_dev;
+	}
+
+	ret = device_add(&master->dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * Expose our I3C bus as an I2C adapter so that I2C devices are exposed
+	 * through the I2C subsystem.
+	 */
+	ret = i3c_master_i2c_adapter_init(master);
+	if (ret)
+		goto err_put_dev;
+
+	master->init_done = true;
+
+err_put_dev:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_sec_master_bus_init);
+
 /**
  * i3c_master_register() - register an I3C master
  * @master: master used to send frames on the bus
@@ -2413,10 +2578,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;
@@ -2430,6 +2591,7 @@ int i3c_master_register(struct i3c_master_controller *master,
 	master->secondary = secondary;
 	INIT_LIST_HEAD(&master->boardinfo.i2c);
 	INIT_LIST_HEAD(&master->boardinfo.i3c);
+	INIT_LIST_HEAD(&master->defslvs);
 
 	ret = i3c_bus_init(i3cbus);
 	if (ret)
@@ -2475,6 +2637,9 @@ int i3c_master_register(struct i3c_master_controller *master,
 		goto err_put_dev;
 	}
 
+	if (secondary)
+		return 0;
+
 	ret = i3c_master_bus_init(master);
 	if (ret)
 		goto err_put_dev;
@@ -2547,6 +2712,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
 	if (!master || !xfers)
 		return -EINVAL;
 
+	if (master->bus.cur_master != master->this) {
+		master->ops->request_mastership(master);
+		return -EBUSY;
+	}
+
 	if (!master->ops->priv_xfers)
 		return -ENOTSUPP;
 
@@ -2638,6 +2808,185 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
 	dev->ibi = NULL;
 }
 
+static const char *const i3c_dr_modes[] = {
+	[I3C_DR_MODE_MASTER]		= "master",
+	[I3C_DR_MODE_SEC_MASTER]	= "sec-master",
+	[I3C_DR_MODE_SLAVE]		= "slave",
+};
+
+static enum i3c_dr_mode i3c_get_dr_mode_from_string(const char *str)
+{
+	int ret;
+
+	ret = match_string(i3c_dr_modes, ARRAY_SIZE(i3c_dr_modes), str);
+	return (ret < 0) ? I3C_DR_MODE_MASTER : ret;
+}
+
+enum i3c_dr_mode i3c_get_dr_mode(struct device *dev)
+{
+	const char *dr_mode;
+	int err;
+
+	err = device_property_read_string(dev, "dr-mode", &dr_mode);
+	if (err < 0)
+		return I3C_DR_MODE_MASTER;
+
+	return i3c_get_dr_mode_from_string(dr_mode);
+}
+EXPORT_SYMBOL_GPL(i3c_get_dr_mode);
+
+int i3c_sec_master_request_mastership(struct i3c_master_controller *master)
+{
+	int ret;
+
+	i3c_bus_normaluse_lock(&master->bus);
+	ret = master->ops->request_mastership(master);
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_sec_master_request_mastership);
+
+int i3c_master_deliver_mastership(struct i3c_master_controller *master, u8 addr)
+{
+	struct i3c_dev_desc *dev;
+	int ret;
+
+	i3c_bus_normaluse_lock(&master->bus);
+	i3c_bus_for_each_i3cdev(&master->bus, dev) {
+		if (dev->ibi) {
+			mutex_lock(&dev->ibi_lock);
+			i3c_dev_disable_ibi_locked(dev);
+			mutex_unlock(&dev->ibi_lock);
+		}
+	}
+	i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
+				I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
+
+	ret = master->ops->deliver_mastership(master, addr);
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_deliver_mastership);
+
+int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
+			     struct i3c_ccc_dev_desc defslvs)
+{
+	struct i3c_defslvs_info *defslvsinfo;
+	struct device *dev = &master->dev;
+
+	i3c_bus_maintenance_lock(&master->bus);
+	defslvsinfo = devm_kzalloc(dev, sizeof(*defslvsinfo), GFP_KERNEL);
+	if (!defslvsinfo)
+		return -ENOMEM;
+
+	defslvsinfo->slave = defslvs;
+
+	list_add_tail(&defslvsinfo->node, &master->defslvs);
+
+	i3c_bus_maintenance_unlock(&master->bus);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(defslvs_populate_i3c_bus);
+
+static void i3c_master_add_new_defslvs(struct i3c_master_controller *master)
+{
+	struct i3c_defslvs_info *defslvsinfo;
+
+	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
+		/* TODO: add i2c devices to the bus */
+		if (!defslvsinfo->slave.dyn_addr)
+			continue;
+
+		if (defslvsinfo->slave.dyn_addr == master->this->info.dyn_addr)
+			continue;
+
+		if (!i3c_bus_dev_addr_is_avail(&master->bus,
+					       defslvsinfo->slave.dyn_addr))
+			continue;
+
+		i3c_master_add_i3c_dev_locked(master, defslvsinfo->slave.dyn_addr);
+	}
+
+	while (!list_empty(&master->defslvs)) {
+		defslvsinfo = list_first_entry(&master->defslvs,
+					       struct i3c_defslvs_info, node);
+		list_del(&defslvsinfo->node);
+	}
+}
+
+int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
+				     bool secondary)
+{
+	struct i3c_dev_desc *dev;
+	int ret;
+
+	if (master->secondary == secondary)
+		return -EEXIST;
+
+	/* TODO: get the current master information */
+	if (secondary)
+		master->bus.cur_master = NULL;
+	else
+		master->bus.cur_master = master->this;
+
+	if (!master->init_done && !secondary)
+		i3c_sec_master_bus_init(master);
+
+	master->secondary = secondary;
+
+	dev_info(&master->dev, "changing to %s\n",
+		 master->secondary ? "Sec Master" : "Master");
+
+	/* TODO: Analyse the use of maintenan_lock for everything */
+	if (!list_empty(&master->defslvs) && !secondary) {
+		i3c_bus_maintenance_lock(&master->bus);
+		i3c_master_add_new_defslvs(master);
+		i3c_bus_maintenance_unlock(&master->bus);
+
+		i3c_bus_normaluse_lock(&master->bus);
+		i3c_master_register_new_i3c_devs(master);
+		i3c_bus_normaluse_unlock(&master->bus);
+	}
+
+	if (!secondary) {
+		i3c_bus_normaluse_lock(&master->bus);
+		i3c_bus_for_each_i3cdev(&master->bus, dev) {
+			if (dev->ibi) {
+				mutex_lock(&dev->ibi_lock);
+				ret = i3c_dev_enable_ibi_locked(dev);
+				if (ret)
+					dev_err(&master->dev,
+						"Failed to re-enable IBI on device %d-%llx",
+						master->bus.id, dev->info.pid);
+				mutex_unlock(&dev->ibi_lock);
+				}
+		}
+
+		/* TODO: Enable MR only for the elegible devices */
+		i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
+					I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
+		i3c_bus_normaluse_unlock(&master->bus);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
+
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
+{
+	int res;
+
+	mutex_lock(&i3c_core_lock);
+	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
+	mutex_unlock(&i3c_core_lock);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(i3c_for_each_dev);
+
 static int __init i3c_init(void)
 {
 	return bus_register(&i3c_bus_type);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 9cb39d9..09bd99c 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -426,6 +426,8 @@ struct i3c_bus {
  *		      for a future IBI
  *		      This method is mandatory only if ->request_ibi is not
  *		      NULL.
+ * @request_mastership: Request mastership.
+ * @deliver_mastership: Deliver mastership
  */
 struct i3c_master_controller_ops {
 	int (*bus_init)(struct i3c_master_controller *master);
@@ -452,6 +454,21 @@ 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);
+	int (*request_mastership)(struct i3c_master_controller *master);
+	int (*deliver_mastership)(struct i3c_master_controller *master,
+				  u8 addr);
+};
+
+/**
+ * struct i3c_defslvs_info - defslvs information object
+ * @node: used to insert the defslvs object in the  list
+ * @slave: I3C/I2C device descriptor used for DEFSLVS
+ *
+ * This structure is used to hold defslvs information on Secondary Master.
+ */
+struct i3c_defslvs_info {
+	struct list_head node;
+	struct i3c_ccc_dev_desc slave;
 };
 
 /**
@@ -468,6 +485,7 @@ struct i3c_master_controller_ops {
  * @boardinfo.i3c: list of I3C  boardinfo objects
  * @boardinfo.i2c: list of I2C boardinfo objects
  * @boardinfo: board-level information attached to devices connected on the bus
+ * @defslvs: List of defslvs objects
  * @bus: I3C bus exposed by this master
  * @wq: workqueue used to execute IBI handlers. Can also be used by master
  *	drivers if they need to postpone operations that need to take place
@@ -491,6 +509,7 @@ struct i3c_master_controller {
 		struct list_head i3c;
 		struct list_head i2c;
 	} boardinfo;
+	struct list_head defslvs;
 	struct i3c_bus bus;
 	struct workqueue_struct *wq;
 };
@@ -525,6 +544,7 @@ int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr,
 			    u8 evts);
 int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
 			   u8 evts);
+int i3c_master_getaccmst_locked(struct i3c_master_controller *master, u8 addr);
 int i3c_master_entdaa_locked(struct i3c_master_controller *master);
 int i3c_master_defslvs_locked(struct i3c_master_controller *master);
 
@@ -652,4 +672,23 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
 
 struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
 
+enum i3c_dr_mode {
+	I3C_DR_MODE_MASTER,
+	I3C_DR_MODE_SEC_MASTER,
+	I3C_DR_MODE_SLAVE,
+};
+
+enum i3c_dr_mode i3c_get_dr_mode(struct device *dev);
+
+int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
+				     bool secondary);
+
+int i3c_sec_master_request_mastership(struct i3c_master_controller *master);
+int i3c_master_deliver_mastership(struct i3c_master_controller *master,
+				  u8 addr);
+
+int i3c_sec_master_bus_init(struct i3c_master_controller *master);
+int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
+			     struct i3c_ccc_dev_desc defslvs);
+
 #endif /* I3C_MASTER_H */
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[RFC 2/2] i3c: dw: add preliminary support for sec master

[Vitor Soares]

The preliminary support of sec master aims to provide all the logic of
secondary master feature only for testing purposes assuming the controller has
support for it and ignoring all necessary verifications.

Key points on secondary master support:
  - if secondary master receives MR enable or Defslvs CCC it will try to
  get bus ownership for eitheir for device initializations or master
  registation. The request is only made if both conditions are met.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
 drivers/i3c/master/dw-i3c-master.c | 619 +++++++++++++++++++++++++++++++++----
 1 file changed, 563 insertions(+), 56 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index b0ff0e1..7099f5f 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -24,7 +24,8 @@
 #define DEVICE_CTRL			0x0
 #define DEV_CTRL_ENABLE			BIT(31)
 #define DEV_CTRL_RESUME			BIT(30)
-#define DEV_CTRL_HOT_JOIN_NACK		BIT(8)
+#define ADAPTIVE_I2C_I3C		BIT(27)
+#define DEV_CTRL_HJ_NACK		BIT(8)
 #define DEV_CTRL_I2C_SLAVE_PRESENT	BIT(7)
 
 #define DEVICE_ADDR			0x4
@@ -74,7 +75,17 @@
 
 #define RX_TX_DATA_PORT			0x14
 #define IBI_QUEUE_STATUS		0x18
+#define IBI_QUEUE_IBI_ID(x)		(((x) & GENMASK(15, 8)) >> 8)
+#define IBI_QUEUE_IBI_ADDR(x)		((x) >> 1)
+#define IBI_TYPE_HJ(x)			\
+	(IBI_QUEUE_IBI_ADDR((x)) == I3C_HOT_JOIN_ADDR && !((x) & BIT(0)))
+#define IBI_TYPE_MR(x)			\
+	(IBI_QUEUE_IBI_ADDR((x)) != I3C_HOT_JOIN_ADDR && !((x) & BIT(0)))
+#define IBI_TYPE_SIR(x)			\
+	(IBI_QUEUE_IBI_ADDR((x)) != I3C_HOT_JOIN_ADDR && ((x) & BIT(0)))
+
 #define QUEUE_THLD_CTRL			0x1c
+#define QUEUE_THLD_CTRL_IBI_STS_MASK	GENMASK(31, 24)
 #define QUEUE_THLD_CTRL_RESP_BUF_MASK	GENMASK(15, 8)
 #define QUEUE_THLD_CTRL_RESP_BUF(x)	(((x) - 1) << 8)
 
@@ -85,6 +96,8 @@
 #define IBI_MR_REQ_REJECT		0x2C
 #define IBI_SIR_REQ_REJECT		0x30
 #define IBI_REQ_REJECT_ALL		GENMASK(31, 0)
+#define IBI_SIR_REQ_ID(x)		((((x) & GENMASK(6, 5)) >> 5) + \
+					((x) & GENMASK(4, 0)))
 
 #define RESET_CTRL			0x34
 #define RESET_CTRL_IBI_QUEUE		BIT(5)
@@ -95,6 +108,12 @@
 #define RESET_CTRL_SOFT			BIT(0)
 
 #define SLV_EVENT_CTRL			0x38
+#define SLV_EVENT_MWL_UPDATED		BIT(7)
+#define SLV_EVENT_MRL_UPDATED		BIT(6)
+#define SLV_EVENT_HJ_INTR_REQ		BIT(3)
+#define SLV_EVENT_MASTER_INTR_REQ	BIT(1)
+#define SLV_EVENT_SLAVE_INTR_REQ	BIT(0)
+
 #define INTR_STATUS			0x3c
 #define INTR_STATUS_EN			0x40
 #define INTR_SIGNAL_EN			0x44
@@ -126,8 +145,15 @@
 					INTR_TX_THLD_STAT |		\
 					INTR_RX_THLD_STAT)
 
-#define INTR_MASTER_MASK		(INTR_TRANSFER_ERR_STAT |	\
-					 INTR_RESP_READY_STAT)
+#define INTR_MASTER_MASK		(INTR_BUSOWNER_UPDATE_STAT |	\
+					 INTR_TRANSFER_ERR_STAT |	\
+					 INTR_RESP_READY_STAT |		\
+					 INTR_IBI_THLD_STAT)
+
+#define INTR_SLAVE_MASK			(INTR_BUSOWNER_UPDATE_STAT |	\
+					 INTR_DEFSLV_STAT |		\
+					 INTR_TRANSFER_ERR_STAT |	\
+					 INTR_CCC_UPDATED_STAT)
 
 #define QUEUE_STATUS_LEVEL		0x4c
 #define QUEUE_STATUS_IBI_STATUS_CNT(x)	(((x) & GENMASK(28, 24)) >> 24)
@@ -140,20 +166,34 @@
 
 #define PRESENT_STATE			0x54
 #define CCC_DEVICE_STATUS		0x58
-#define DEVICE_ADDR_TABLE_POINTER	0x5c
-#define DEVICE_ADDR_TABLE_DEPTH(x)	(((x) & GENMASK(31, 16)) >> 16)
-#define DEVICE_ADDR_TABLE_ADDR(x)	((x) & GENMASK(7, 0))
+#define DEV_ADDR_TABLE_POINTER		0x5c
+#define DEV_ADDR_TABLE_DEPTH(x)		(((x) & GENMASK(31, 16)) >> 16)
+#define DEV_ADDR_TABLE_ADDR(x)		((x) & GENMASK(15, 0))
 
 #define DEV_CHAR_TABLE_POINTER		0x60
+#define DEV_CHAR_TABLE_RST_INDEX	(~GENMASK(24, 19))
+#define DEV_CHAR_TABLE_DEPTH(x)		(((x) & GENMASK(18, 12)) >> 12)
+#define DEV_CHAR_TABLE_ADDR(x)		((x) & GENMASK(11, 0))
+
 #define VENDOR_SPECIFIC_REG_POINTER	0x6c
 #define SLV_PID_VALUE			0x74
 #define SLV_CHAR_CTRL			0x78
+#define SLV_CHAR_CTRL_HDR_CAP(x)	(((x) & GENMASK(23, 16)) >> 16)
+#define SLV_CHAR_CTRL_DCR(x)		(((x) & GENMASK(15, 8)) >> 8)
+#define SLV_CHAR_CTRL_BCR(x)		((x) & GENMASK(7, 0))
+
 #define SLV_MAX_LEN			0x7c
 #define MAX_READ_TURNAROUND		0x80
 #define MAX_DATA_SPEED			0x84
 #define SLV_DEBUG_STATUS		0x88
+
 #define SLV_INTR_REQ			0x8c
+#define SLV_INTR_REQ_MIR		BIT(3)
+#define SLV_INTR_REQ_SIR		BIT(0)
+
 #define DEVICE_CTRL_EXTENDED		0xb0
+#define DEV_OP_MODE_SLAVE	BIT(0)
+
 #define SCL_I3C_OD_TIMING		0xb4
 #define SCL_I3C_PP_TIMING		0xb8
 #define SCL_I3C_TIMING_HCNT(x)		(((x) << 16) & GENMASK(23, 16))
@@ -176,9 +216,12 @@
 
 #define SCL_EXT_TERMN_LCNT_TIMING	0xcc
 #define BUS_FREE_TIMING			0xd4
+#define BUS_IBI_FREE(x)			(((x) << 16) & GENMASK(31, 16))
 #define BUS_I3C_MST_FREE(x)		((x) & GENMASK(15, 0))
 
 #define BUS_IDLE_TIMING			0xd8
+#define BUS_IDLE_TIME(x)		((x) & GENMASK(19, 0))
+
 #define I3C_VER_ID			0xe0
 #define I3C_VER_TYPE			0xe4
 #define EXTENDED_CAPABILITY		0xe8
@@ -189,8 +232,16 @@
 #define DEV_ADDR_TABLE_STATIC_ADDR(x)	((x) & GENMASK(6, 0))
 #define DEV_ADDR_TABLE_LOC(start, idx)	((start) + ((idx) << 2))
 
+#define DEV_CHAR_TABLE_LOC(start, idx, loc) \
+	((start) + ((idx) << 4) + ((loc) << 2))
+
+#define SEC_DEV_CHAR_TABLE_LOC(start, idx) \
+	((start) + ((idx) << 2))
+
 #define MAX_DEVS 32
 
+#define I3C_BUS_AVAIL_TIME_NS		1000
+#define I3C_BUS_IDLE_TIME_NS		1000000
 #define I3C_BUS_SDR1_SCL_RATE		8000000
 #define I3C_BUS_SDR2_SCL_RATE		6000000
 #define I3C_BUS_SDR3_SCL_RATE		4000000
@@ -201,7 +252,18 @@
 
 #define XFER_TIMEOUT (msecs_to_jiffies(1000))
 
+struct dw_i3c_defslvs_dct {
+	u8 static_addr;
+	u8 bcr;
+	u8 dcr_lvr;
+	u8 dyn_addr;
+};
+
 struct dw_i3c_master_caps {
+	u16 datstartaddr;
+	u16 datdepth;
+	u16 dctstartaddr;
+	u8 dctdepth;
 	u8 cmdfifodepth;
 	u8 datafifodepth;
 };
@@ -224,16 +286,32 @@ struct dw_i3c_xfer {
 	struct dw_i3c_cmd cmds[0];
 };
 
+enum type {
+	REQUEST = 1,
+	DELIVER,
+	HANDOFF,
+	TAKEOVER,
+};
+
 struct dw_i3c_master {
 	struct i3c_master_controller base;
-	u16 maxdevs;
-	u16 datstartaddr;
+	struct work_struct hj_work;
+	enum i3c_dr_mode dr_mode;
+	struct {
+		struct work_struct mr_work;
+		enum type type;
+		u8 addr;
+	} mastership;
 	u32 free_pos;
 	struct {
 		struct list_head list;
 		struct dw_i3c_xfer *cur;
 		spinlock_t lock;
 	} xferqueue;
+	struct {
+		struct i3c_dev_desc *slots[MAX_DEVS];
+		spinlock_t lock; /* spinlock for ibi handling */
+	} ibi;
 	struct dw_i3c_master_caps caps;
 	void __iomem *regs;
 	struct reset_control *core_rst;
@@ -244,7 +322,9 @@ struct dw_i3c_master {
 };
 
 struct dw_i3c_i2c_dev_data {
+	struct i3c_generic_ibi_pool *ibi_pool;
 	u8 index;
+	s8 ibi;
 };
 
 static u8 even_parity(u8 p)
@@ -276,6 +356,7 @@ static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
 	case I3C_CCC_SETMRL(true):
 	case I3C_CCC_SETMRL(false):
 	case I3C_CCC_ENTHDR(0):
+	case I3C_CCC_DEFSLVS:
 	case I3C_CCC_SETDASA:
 	case I3C_CCC_SETNEWDA:
 	case I3C_CCC_GETMWL:
@@ -283,6 +364,7 @@ static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
 	case I3C_CCC_GETPID:
 	case I3C_CCC_GETBCR:
 	case I3C_CCC_GETDCR:
+	case I3C_CCC_GETACCMST:
 	case I3C_CCC_GETSTATUS:
 	case I3C_CCC_GETMXDS:
 	case I3C_CCC_GETHDRCAP:
@@ -314,7 +396,7 @@ static int dw_i3c_master_get_addr_pos(struct dw_i3c_master *master, u8 addr)
 {
 	int pos;
 
-	for (pos = 0; pos < master->maxdevs; pos++) {
+	for (pos = 0; pos < master->caps.datdepth; pos++) {
 		if (addr == master->addrs[pos])
 			return pos;
 	}
@@ -324,12 +406,37 @@ static int dw_i3c_master_get_addr_pos(struct dw_i3c_master *master, u8 addr)
 
 static int dw_i3c_master_get_free_pos(struct dw_i3c_master *master)
 {
-	if (!(master->free_pos & GENMASK(master->maxdevs - 1, 0)))
+	if (!(master->free_pos & GENMASK(master->caps.datdepth - 1, 0)))
 		return -ENOSPC;
 
 	return ffs(master->free_pos) - 1;
 }
 
+static void
+dw_i3c_master_add_defslvs(struct dw_i3c_master *master, int ndevs)
+{
+	struct i3c_ccc_dev_desc defslvs;
+	struct dw_i3c_defslvs_dct dct;
+	u32 *ptr = (u32 *)&dct;
+	int i;
+
+	for (i = 0; i < ndevs; i++) {
+		*ptr = readl(master->regs +
+			SEC_DEV_CHAR_TABLE_LOC(master->caps.dctstartaddr, i));
+
+		defslvs.dyn_addr = dct.dyn_addr >> 1;
+		if (dct.dyn_addr)
+			defslvs.dcr = dct.dcr_lvr;
+		else
+			defslvs.lvr = dct.dcr_lvr;
+
+		defslvs.bcr = dct.bcr;
+		defslvs.static_addr = dct.static_addr >> 1;
+
+		defslvs_populate_i3c_bus(&master->base, defslvs);
+	}
+}
+
 static void dw_i3c_master_wr_tx_fifo(struct dw_i3c_master *master,
 				     const u8 *bytes, int nbytes)
 {
@@ -539,7 +646,8 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
 	writel(scl_timing, master->regs + SCL_I3C_PP_TIMING);
 
 	if (!(readl(master->regs + DEVICE_CTRL) & DEV_CTRL_I2C_SLAVE_PRESENT))
-		writel(BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING);
+		writel(readl(master->regs + BUS_FREE_TIMING) |
+		       BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING);
 
 	lcnt = DIV_ROUND_UP(I3C_BUS_TLOW_OD_MIN_NS, core_period);
 	scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt);
@@ -582,7 +690,8 @@ static int dw_i2c_clk_cfg(struct dw_i3c_master *master)
 		     SCL_I2C_FM_TIMING_LCNT(lcnt);
 	writel(scl_timing, master->regs + SCL_I2C_FM_TIMING);
 
-	writel(BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING);
+	writel(readl(master->regs + BUS_FREE_TIMING) |
+	       BUS_I3C_MST_FREE(lcnt), master->regs + BUS_FREE_TIMING);
 	writel(readl(master->regs + DEVICE_CTRL) | DEV_CTRL_I2C_SLAVE_PRESENT,
 	       master->regs + DEVICE_CTRL);
 
@@ -594,9 +703,11 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
 	struct dw_i3c_master *master = to_dw_i3c_master(m);
 	struct i3c_bus *bus = i3c_master_get_bus(m);
 	struct i3c_device_info info = { };
-	u32 thld_ctrl;
+	u32 thld_ctrl, char_ctrl;
 	int ret;
 
+	dw_i3c_master_disable(master);
+
 	switch (bus->mode) {
 	case I3C_BUS_MODE_MIXED_FAST:
 	case I3C_BUS_MODE_MIXED_LIMITED:
@@ -613,38 +724,43 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
 		return -EINVAL;
 	}
 
-	thld_ctrl = readl(master->regs + QUEUE_THLD_CTRL);
-	thld_ctrl &= ~QUEUE_THLD_CTRL_RESP_BUF_MASK;
-	writel(thld_ctrl, master->regs + QUEUE_THLD_CTRL);
+	if (!m->secondary) {
+		thld_ctrl = readl(master->regs + QUEUE_THLD_CTRL);
+		thld_ctrl &= ~QUEUE_THLD_CTRL_RESP_BUF_MASK &
+			     ~QUEUE_THLD_CTRL_IBI_STS_MASK;
+		writel(thld_ctrl, master->regs + QUEUE_THLD_CTRL);
 
-	thld_ctrl = readl(master->regs + DATA_BUFFER_THLD_CTRL);
-	thld_ctrl &= ~DATA_BUFFER_THLD_CTRL_RX_BUF;
-	writel(thld_ctrl, master->regs + DATA_BUFFER_THLD_CTRL);
+		thld_ctrl = readl(master->regs + DATA_BUFFER_THLD_CTRL);
+		thld_ctrl &= ~DATA_BUFFER_THLD_CTRL_RX_BUF;
+		writel(thld_ctrl, master->regs + DATA_BUFFER_THLD_CTRL);
 
-	writel(INTR_ALL, master->regs + INTR_STATUS);
-	writel(INTR_MASTER_MASK, master->regs + INTR_STATUS_EN);
-	writel(INTR_MASTER_MASK, master->regs + INTR_SIGNAL_EN);
+		writel(INTR_ALL, master->regs + INTR_STATUS);
+		writel(INTR_MASTER_MASK, master->regs + INTR_STATUS_EN);
+		writel(INTR_MASTER_MASK, master->regs + INTR_SIGNAL_EN);
 
-	ret = i3c_master_get_free_addr(m, 0);
-	if (ret < 0)
-		return ret;
+		ret = i3c_master_get_free_addr(m, 0);
+		if (ret < 0)
+			return ret;
 
-	writel(DEV_ADDR_DYNAMIC_ADDR_VALID | DEV_ADDR_DYNAMIC(ret),
-	       master->regs + DEVICE_ADDR);
+		writel(DEV_ADDR_DYNAMIC_ADDR_VALID | DEV_ADDR_DYNAMIC(ret),
+		       master->regs + DEVICE_ADDR);
+	}
+
+	if (!(readl(master->regs + DEVICE_ADDR) & DEV_ADDR_DYNAMIC_ADDR_VALID))
+		return -EINVAL;
 
 	memset(&info, 0, sizeof(info));
-	info.dyn_addr = ret;
+	char_ctrl = readl(master->regs + SLV_CHAR_CTRL);
+	info.hdr_cap = SLV_CHAR_CTRL_HDR_CAP(char_ctrl);
+	info.dcr = SLV_CHAR_CTRL_DCR(char_ctrl);
+	info.bcr = SLV_CHAR_CTRL_BCR(char_ctrl);
+	info.dyn_addr = readl(master->regs + DEVICE_ADDR) >> 16 & GENMASK(6, 0);
 
 	ret = i3c_master_set_info(&master->base, &info);
 	if (ret)
 		return ret;
 
 	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_SIR_REQ_REJECT);
-	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
-
-	/* For now don't support Hot-Join */
-	writel(readl(master->regs + DEVICE_CTRL) | DEV_CTRL_HOT_JOIN_NACK,
-	       master->regs + DEVICE_CTRL);
 
 	dw_i3c_master_enable(master);
 
@@ -770,7 +886,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 	olddevs = ~(master->free_pos);
 
 	/* Prepare DAT before launching DAA. */
-	for (pos = 0; pos < master->maxdevs; pos++) {
+	for (pos = 0; pos < master->caps.datdepth; pos++) {
 		if (olddevs & BIT(pos))
 			continue;
 
@@ -785,7 +901,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 
 		writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(ret),
 		       master->regs +
-		       DEV_ADDR_TABLE_LOC(master->datstartaddr, pos));
+		       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr, pos));
 	}
 
 	xfer = dw_i3c_master_alloc_xfer(master, 1);
@@ -795,7 +911,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 	pos = dw_i3c_master_get_free_pos(master);
 	cmd = &xfer->cmds[0];
 	cmd->cmd_hi = 0x1;
-	cmd->cmd_lo = COMMAND_PORT_DEV_COUNT(master->maxdevs - pos) |
+	cmd->cmd_lo = COMMAND_PORT_DEV_COUNT(master->caps.datdepth - pos) |
 		      COMMAND_PORT_DEV_INDEX(pos) |
 		      COMMAND_PORT_CMD(I3C_CCC_ENTDAA) |
 		      COMMAND_PORT_ADDR_ASSGN_CMD |
@@ -806,20 +922,20 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 	if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
 		dw_i3c_master_dequeue_xfer(master, xfer);
 
-	newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);
+	newdevs = GENMASK(master->caps.datdepth - cmd->rx_len - 1, 0);
 	newdevs &= ~olddevs;
 
-	for (pos = 0; pos < master->maxdevs; pos++) {
+	for (pos = 0; pos < master->caps.datdepth; pos++) {
 		if (newdevs & BIT(pos))
 			i3c_master_add_i3c_dev_locked(m, master->addrs[pos]);
 	}
 
 	dw_i3c_master_free_xfer(xfer);
 
-	i3c_master_disec_locked(m, I3C_BROADCAST_ADDR,
+	i3c_master_defslvs_locked(&master->base);
+	i3c_master_enec_locked(m, I3C_BROADCAST_ADDR,
 				I3C_CCC_EVENT_HJ |
-				I3C_CCC_EVENT_MR |
-				I3C_CCC_EVENT_SIR);
+				I3C_CCC_EVENT_MR);
 
 	return 0;
 }
@@ -899,10 +1015,27 @@ static int dw_i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	int pos;
+
+	pos = dw_i3c_master_get_free_pos(master);
+
+	if (data->index > pos && pos > 0) {
+		writel(0,
+		       master->regs +
+		       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr,
+					  data->index));
+
+		master->addrs[data->index] = 0;
+		master->free_pos |= BIT(data->index);
+
+		data->index = pos;
+		master->addrs[pos] = dev->info.dyn_addr;
+		master->free_pos &= ~BIT(pos);
+	}
 
 	writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(dev->info.dyn_addr),
 	       master->regs +
-	       DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+	       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr, data->index));
 
 	master->addrs[data->index] = dev->info.dyn_addr;
 
@@ -931,7 +1064,7 @@ static int dw_i3c_master_attach_i3c_dev(struct i3c_dev_desc *dev)
 
 	writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(master->addrs[pos]),
 	       master->regs +
-	       DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+	       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr, data->index));
 
 	return 0;
 }
@@ -944,7 +1077,7 @@ static void dw_i3c_master_detach_i3c_dev(struct i3c_dev_desc *dev)
 
 	writel(0,
 	       master->regs +
-	       DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+	       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr, data->index));
 
 	i3c_dev_set_master_data(dev, NULL);
 	master->addrs[data->index] = 0;
@@ -1040,7 +1173,7 @@ static int dw_i3c_master_attach_i2c_dev(struct i2c_dev_desc *dev)
 	writel(DEV_ADDR_TABLE_LEGACY_I2C_DEV |
 	       DEV_ADDR_TABLE_STATIC_ADDR(dev->addr),
 	       master->regs +
-	       DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+	       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr, data->index));
 
 	return 0;
 }
@@ -1053,7 +1186,7 @@ static void dw_i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 
 	writel(0,
 	       master->regs +
-	       DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
+	       DEV_ADDR_TABLE_LOC(master->caps.datstartaddr, data->index));
 
 	i2c_dev_set_master_data(dev, NULL);
 	master->addrs[data->index] = 0;
@@ -1061,7 +1194,153 @@ static void dw_i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 	kfree(data);
 }
 
-static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
+static void dw_i3c_master_handle_sir(struct dw_i3c_master *master, u8 addr)
+{
+	struct dw_i3c_i2c_dev_data *data;
+	struct i3c_dev_desc *dev;
+	struct i3c_ibi_slot *slot;
+	int pos;
+
+	pos = dw_i3c_master_get_addr_pos(master, addr);
+	if (pos < 0)
+		return;
+
+	dev = master->ibi.slots[pos];
+
+	spin_lock(&master->ibi.lock);
+	data = i3c_dev_get_master_data(dev);
+	slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
+	if (!slot) {
+		spin_unlock(&master->ibi.lock);
+		return;
+	}
+	i3c_master_queue_ibi(dev, slot);
+	spin_unlock(&master->ibi.lock);
+}
+
+static void dw_i3c_master_demux_ibis(struct dw_i3c_master *master)
+{
+	u32 nibis;
+	int i;
+
+	nibis = readl(master->regs + QUEUE_STATUS_LEVEL);
+	nibis = QUEUE_STATUS_IBI_BUF_BLR(nibis);
+
+	for (i = 0; i < nibis; i++) {
+		u32 ibir = readl(master->regs + IBI_QUEUE_STATUS);
+		u8 id = IBI_QUEUE_IBI_ID(ibir);
+
+		if (IBI_TYPE_HJ(id))
+			queue_work(master->base.wq, &master->hj_work);
+		else if (IBI_TYPE_SIR(id))
+			dw_i3c_master_handle_sir(master,
+						 IBI_QUEUE_IBI_ADDR(id));
+		else if (IBI_TYPE_MR(id)) {
+			master->mastership.addr = IBI_QUEUE_IBI_ADDR(id);
+			master->mastership.type = DELIVER;
+			queue_work(master->base.wq,
+				   &master->mastership.mr_work);
+		}
+	}
+}
+
+static void dw_i3c_master_bus_handoff(struct dw_i3c_master *master)
+{
+	u32 thld_ctrl;
+
+	writel(RESET_CTRL_RX_FIFO | RESET_CTRL_TX_FIFO |
+	       RESET_CTRL_RESP_QUEUE | RESET_CTRL_CMD_QUEUE,
+	       master->regs + RESET_CTRL);
+
+	thld_ctrl = readl(master->regs + QUEUE_THLD_CTRL);
+	thld_ctrl &= ~QUEUE_THLD_CTRL_RESP_BUF_MASK;
+	writel(thld_ctrl, master->regs + QUEUE_THLD_CTRL);
+
+	writel(INTR_ALL, master->regs + INTR_STATUS);
+	writel(INTR_SLAVE_MASK, master->regs + INTR_STATUS_EN);
+	writel(INTR_SLAVE_MASK, master->regs + INTR_SIGNAL_EN);
+
+	writel(readl(master->regs + DEVICE_CTRL) | DEV_CTRL_RESUME,
+	       master->regs + DEVICE_CTRL);
+
+	master->mastership.type = HANDOFF;
+	queue_work(master->base.wq, &master->mastership.mr_work);
+}
+
+static void dw_i3c_sec_master_bus_takeover(struct dw_i3c_master *master)
+{
+	u32 thld_ctrl;
+
+	writel(RESET_CTRL_RX_FIFO | RESET_CTRL_TX_FIFO |
+	       RESET_CTRL_RESP_QUEUE | RESET_CTRL_CMD_QUEUE,
+	       master->regs + RESET_CTRL);
+
+	thld_ctrl = readl(master->regs + QUEUE_THLD_CTRL);
+	thld_ctrl &= ~QUEUE_THLD_CTRL_RESP_BUF_MASK &
+		     ~QUEUE_THLD_CTRL_IBI_STS_MASK;
+	writel(thld_ctrl, master->regs + QUEUE_THLD_CTRL);
+
+	thld_ctrl = readl(master->regs + DATA_BUFFER_THLD_CTRL);
+	thld_ctrl &= ~DATA_BUFFER_THLD_CTRL_RX_BUF;
+	writel(thld_ctrl, master->regs + DATA_BUFFER_THLD_CTRL);
+
+	writel(INTR_ALL, master->regs + INTR_STATUS);
+	writel(INTR_MASTER_MASK, master->regs + INTR_STATUS_EN);
+	writel(INTR_MASTER_MASK, master->regs + INTR_SIGNAL_EN);
+
+	writel(readl(master->regs + DEVICE_CTRL) | DEV_CTRL_RESUME,
+	       master->regs + DEVICE_CTRL);
+
+	master->mastership.type = TAKEOVER;
+	queue_work(master->base.wq, &master->mastership.mr_work);
+}
+
+static void
+dw_i3c_sec_master_irq_handler(struct dw_i3c_master *master, u32 status)
+{
+	bool do_mr = false;
+	u32 ret;
+
+	if (status & INTR_TRANSFER_ERR_STAT)
+		writel(INTR_TRANSFER_ERR_STAT, master->regs + INTR_STATUS);
+
+	if (status & INTR_DEFSLV_STAT) {
+		ret = readl(master->regs + RESPONSE_QUEUE_PORT);
+		dw_i3c_master_add_defslvs(master, RESPONSE_PORT_DATA_LEN(ret));
+		writel(INTR_DEFSLV_STAT, master->regs + INTR_STATUS);
+		do_mr = true;
+	}
+
+	if (status & INTR_CCC_UPDATED_STAT) {
+		do_mr = true;
+		writel(INTR_CCC_UPDATED_STAT, master->regs + INTR_STATUS);
+	}
+
+	if (status & INTR_BUSOWNER_UPDATE_STAT) {
+		dw_i3c_master_bus_handoff(master);
+		writel(INTR_BUSOWNER_UPDATE_STAT, master->regs + INTR_STATUS);
+	}
+
+	if (!list_empty(&master->base.defslvs) && do_mr &&
+	    (readl(master->regs + SLV_EVENT_CTRL) & SLV_EVENT_MASTER_INTR_REQ)) {
+		master->mastership.type = REQUEST;
+		queue_work(master->base.wq, &master->mastership.mr_work);
+	}
+}
+
+static void
+dw_i3c_master_irq_handler(struct dw_i3c_master *master, u32 status)
+{
+	if (status & INTR_IBI_THLD_STAT)
+		dw_i3c_master_demux_ibis(master);
+
+	if (status & INTR_BUSOWNER_UPDATE_STAT) {
+		dw_i3c_sec_master_bus_takeover(master);
+		writel(INTR_BUSOWNER_UPDATE_STAT, master->regs + INTR_STATUS);
+	}
+}
+
+static irqreturn_t dw_i3c_irq_handler(int irq, void *dev_id)
 {
 	struct dw_i3c_master *master = dev_id;
 	u32 status;
@@ -1079,9 +1358,143 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
 		writel(INTR_TRANSFER_ERR_STAT, master->regs + INTR_STATUS);
 	spin_unlock(&master->xferqueue.lock);
 
+	if (readl(master->regs + DEVICE_CTRL_EXTENDED) & DEV_OP_MODE_SLAVE)
+		dw_i3c_sec_master_irq_handler(master, status);
+	else
+		dw_i3c_master_irq_handler(master, status);
+
 	return IRQ_HANDLED;
 }
 
+static int dw_i3c_master_disable_ibi(struct i3c_dev_desc *dev)
+{
+	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+	struct i3c_master_controller *m = i3c_dev_get_master(dev);
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	unsigned long flags;
+	u32 sirmap;
+	int ret;
+
+	ret = i3c_master_disec_locked(m, dev->info.dyn_addr,
+				      I3C_CCC_EVENT_SIR);
+	if (ret)
+		return ret;
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	sirmap = readl(master->regs + IBI_SIR_REQ_REJECT);
+	sirmap |= BIT(data->ibi);
+	writel(sirmap, master->regs + IBI_SIR_REQ_REJECT);
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	return ret;
+}
+
+static int dw_i3c_master_enable_ibi(struct i3c_dev_desc *dev)
+{
+	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+	struct i3c_master_controller *m = i3c_dev_get_master(dev);
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	unsigned long flags;
+	u32 sirmap;
+	int ret;
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	sirmap = readl(master->regs + IBI_SIR_REQ_REJECT);
+	sirmap &= ~BIT(data->ibi);
+	writel(sirmap, master->regs + IBI_SIR_REQ_REJECT);
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	ret = i3c_master_enec_locked(m, dev->info.dyn_addr,
+				     I3C_CCC_EVENT_SIR);
+	if (ret) {
+		spin_lock_irqsave(&master->ibi.lock, flags);
+		sirmap = readl(master->regs + IBI_SIR_REQ_REJECT);
+		sirmap |= BIT(data->ibi);
+		writel(sirmap, master->regs + IBI_SIR_REQ_REJECT);
+		spin_unlock_irqrestore(&master->ibi.lock, flags);
+	}
+
+	return ret;
+}
+
+static int dw_i3c_master_request_ibi(struct i3c_dev_desc *dev,
+				     const struct i3c_ibi_setup *req)
+{
+	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+	struct i3c_master_controller *m = i3c_dev_get_master(dev);
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	unsigned long flags;
+
+	if (req->max_payload_len > 0)
+		return -ENOTSUPP;
+
+	data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req);
+	if (IS_ERR(data->ibi_pool))
+		return PTR_ERR(data->ibi_pool);
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	data->ibi = IBI_SIR_REQ_ID(dev->info.dyn_addr);
+	master->ibi.slots[data->index] = dev;
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	return 0;
+}
+
+static void dw_i3c_master_free_ibi(struct i3c_dev_desc *dev)
+{
+	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+	struct i3c_master_controller *m = i3c_dev_get_master(dev);
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->ibi.lock, flags);
+	master->ibi.slots[data->index] = NULL;
+	data->ibi = -1;
+	spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+	i3c_generic_ibi_free_pool(data->ibi_pool);
+}
+
+static void dw_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
+					   struct i3c_ibi_slot *slot)
+{
+	struct dw_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+
+	i3c_generic_ibi_recycle_slot(data->ibi_pool, slot);
+}
+
+static int
+dw_i3c_sec_master_request_mastership(struct i3c_master_controller *m)
+{
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	u32 status;
+	int ret;
+
+	if (!m->secondary)
+		return -EINVAL;
+
+	if (!(readl(master->regs + DEVICE_ADDR) & DEV_ADDR_DYNAMIC_ADDR_VALID))
+		return -EINVAL;
+
+	if (!(readl(master->regs + SLV_EVENT_CTRL) & SLV_EVENT_MASTER_INTR_REQ))
+		return -EINVAL;
+
+	writel(readl(master->regs + SLV_INTR_REQ) |
+	       SLV_INTR_REQ_MIR, master->regs + SLV_INTR_REQ);
+
+	ret = readl_poll_timeout(master->regs + SLV_INTR_REQ, status,
+				 ((status & BIT(8)) && !(status & BIT(9))),
+				 100, 1000000);
+
+	return ret;
+}
+
+static int
+dw_i3c_master_deliver_mastership(struct i3c_master_controller *m, u8 addr)
+{
+	return i3c_master_getaccmst_locked(m, addr);
+}
+
 static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
 	.bus_init = dw_i3c_master_bus_init,
 	.bus_cleanup = dw_i3c_master_bus_cleanup,
@@ -1095,12 +1508,74 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
 	.attach_i2c_dev = dw_i3c_master_attach_i2c_dev,
 	.detach_i2c_dev = dw_i3c_master_detach_i2c_dev,
 	.i2c_xfers = dw_i3c_master_i2c_xfers,
+	.enable_ibi = dw_i3c_master_enable_ibi,
+	.disable_ibi = dw_i3c_master_disable_ibi,
+	.request_ibi = dw_i3c_master_request_ibi,
+	.free_ibi = dw_i3c_master_free_ibi,
+	.recycle_ibi_slot = dw_i3c_master_recycle_ibi_slot,
+	.request_mastership = dw_i3c_sec_master_request_mastership,
+	.deliver_mastership = dw_i3c_master_deliver_mastership,
 };
 
+static void dw_i3c_master_hj(struct work_struct *work)
+{
+	struct dw_i3c_master *master = container_of(work, struct dw_i3c_master,
+						    hj_work);
+
+	i3c_master_do_daa(&master->base);
+}
+
+static void dw_i3c_master_mr(struct work_struct *work)
+{
+	struct dw_i3c_master *master = container_of(work,
+						   struct dw_i3c_master,
+						   mastership.mr_work);
+
+	switch (master->mastership.type) {
+	case REQUEST:
+		i3c_sec_master_request_mastership(&master->base);
+		break;
+	case DELIVER:
+		i3c_master_deliver_mastership(&master->base,
+					      master->mastership.addr);
+		break;
+	case HANDOFF:
+		i3c_master_switch_operation_mode(&master->base, true);
+		break;
+	case TAKEOVER:
+		i3c_master_switch_operation_mode(&master->base, false);
+		break;
+	default:
+		break;
+	}
+}
+
+static int dw_i3c_sec_master_init(struct dw_i3c_master *master)
+{
+	u32 ret;
+
+	ret = readl(master->regs + QUEUE_THLD_CTRL);
+	ret &= ~QUEUE_THLD_CTRL_RESP_BUF_MASK;
+	writel(ret, master->regs + QUEUE_THLD_CTRL);
+
+	writel(INTR_SLAVE_MASK, master->regs + INTR_STATUS_EN);
+	writel(INTR_SLAVE_MASK, master->regs + INTR_SIGNAL_EN);
+
+	writel(ADAPTIVE_I2C_I3C, master->regs + DEVICE_CTRL);
+	writel(DEV_OP_MODE_SLAVE, master->regs + DEVICE_CTRL_EXTENDED);
+
+	dw_i3c_master_enable(master);
+
+	return 0;
+}
+
 static int dw_i3c_probe(struct platform_device *pdev)
 {
+	unsigned long core_rate, core_period;
 	struct dw_i3c_master *master;
 	struct resource *res;
+	u16 avail_timing;
+	u32 idle_timing;
 	int ret, irq;
 
 	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
@@ -1127,13 +1602,16 @@ static int dw_i3c_probe(struct platform_device *pdev)
 
 	reset_control_deassert(master->core_rst);
 
+	spin_lock_init(&master->ibi.lock);
 	spin_lock_init(&master->xferqueue.lock);
 	INIT_LIST_HEAD(&master->xferqueue.list);
+	INIT_WORK(&master->hj_work, dw_i3c_master_hj);
+	INIT_WORK(&master->mastership.mr_work, dw_i3c_master_mr);
 
 	writel(INTR_ALL, master->regs + INTR_STATUS);
 	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(&pdev->dev, irq,
-			       dw_i3c_master_irq_handler, 0,
+			       dw_i3c_irq_handler, 0,
 			       dev_name(&pdev->dev), master);
 	if (ret)
 		goto err_assert_rst;
@@ -1147,15 +1625,44 @@ static int dw_i3c_probe(struct platform_device *pdev)
 	ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL);
 	master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret);
 
-	ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER);
-	master->datstartaddr = ret;
-	master->maxdevs = ret >> 16;
-	master->free_pos = GENMASK(master->maxdevs - 1, 0);
+	ret = readl(master->regs + DEV_ADDR_TABLE_POINTER);
+	master->caps.datstartaddr = DEV_ADDR_TABLE_ADDR(ret);
+	master->caps.datdepth = DEV_ADDR_TABLE_DEPTH(ret);
 
-	ret = i3c_master_register(&master->base, &pdev->dev,
-				  &dw_mipi_i3c_ops, false);
-	if (ret)
-		goto err_assert_rst;
+	ret = readl(master->regs + DEV_CHAR_TABLE_POINTER);
+	master->caps.dctstartaddr = DEV_CHAR_TABLE_ADDR(ret);
+	master->caps.dctdepth = DEV_CHAR_TABLE_DEPTH(ret);
+
+	master->free_pos = GENMASK(master->caps.datdepth - 1, 0);
+
+	core_rate = clk_get_rate(master->core_clk);
+	if (!core_rate)
+		return -EINVAL;
+
+	core_period = DIV_ROUND_UP(1000000000, core_rate);
+
+	avail_timing = DIV_ROUND_UP(I3C_BUS_AVAIL_TIME_NS, core_period);
+	writel(readl(master->regs + BUS_FREE_TIMING) |
+	       BUS_IBI_FREE(avail_timing), master->regs + BUS_FREE_TIMING);
+
+	idle_timing = DIV_ROUND_UP(I3C_BUS_IDLE_TIME_NS, core_period);
+	writel(readl(master->regs + BUS_IDLE_TIMING) |
+	       BUS_IDLE_TIME(idle_timing), master->regs + BUS_IDLE_TIMING);
+
+	master->dr_mode = i3c_get_dr_mode(&pdev->dev);
+
+	if (master->dr_mode == I3C_DR_MODE_MASTER) {
+		if (i3c_master_register(&master->base, &pdev->dev,
+					&dw_mipi_i3c_ops, false))
+			goto err_assert_rst;
+	} else {
+		if (dw_i3c_sec_master_init(master))
+			goto err_assert_rst;
+
+		if (i3c_master_register(&master->base, &pdev->dev,
+					&dw_mipi_i3c_ops, true))
+			goto err_assert_rst;
+	}
 
 	return 0;
 
-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 1/2] i3c: Add preliminary support for secondary master

[Przemyslaw Gaj]

Hi Vitor,

First, you woke up my son and he couldn't sleep the rest of the night
:-)
I appreciate you sent that so we can discuss it.

The 11/28/2019 02:15, Vitor Soares wrote:
> 
> This patch adds the preliminary support for secondary master feature to
> i3c Framework for testing purposes.
> 
> Key points for consideration:
>   - mastership_[show/store] are only used for testing
>   - secondary master registration is made in two steps, one in
>   i3c_master_register() and another in i3c_sec_master_bus_init() when
>   secondary master became current master first time. This is made in this
>   way to get all dt declared boardinfo list, create defslvs list and
>   provide work_queue.
>   - When the current master wants to deliver_mastership it necessary to
>   disable all in-band events to avoid unwanted interrupt during bus
>   ownership exchange. For now this patch doesn't reflect all
>   improvements/changes made in v1.1 I3C Bus spec. But it can be extended
>   by adding some commands and checks to the flow.
>   - i3c_defslvs_info: The DEFSLVS info can be differently stored in
>   diferen HC. Hence it is used a defslvs list similar to boardinfo list in
>   the bus structure to hold this data. Them HC is taccking over the bus
>   ownership can initialize each device of that list. For now, this not
>   address the i2c devices since they are only statically described.
>   - [request/deliver]_mastership(): Mastership request deliver may be done
>   differently in different HC, hence the need to have a call back for each
>   process.
>   - Add dr_mode to DT: Similar to USB, HC can be programmed to Master only
>   mode, Slave only mode or Secondary Master which aren't necessarily
>   hardcoded.
>   - bus_mode definition: The bus mode is defined even without defslvs
>   information with DT info since the definition of i2c devices are those
>   that have impact on bus_mode definition and need to statically declared.
>   The only use case that may cause issues is when i2c devices aren't
>   declared in secondary master side and bus mode doesn't match the
>   main master. Anyway this can be solde without extra complexity.
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
>  drivers/i3c/master.c       | 365 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/i3c/master.h |  39 +++++
>  2 files changed, 396 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 0436916..b398d77 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -449,6 +449,46 @@ static ssize_t mode_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(mode);
>  
> +static ssize_t
> +mastership_show(struct device *dev, struct device_attribute *da, char *buf)
> +{
> +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> +	ssize_t ret;
> +
> +	if (master->secondary)
> +		ret = sprintf(buf, "Secondary Master\n");
> +	else
> +		ret = sprintf(buf, "Master\n");
> +
> +	return ret;
> +}
> +
> +static ssize_t
> +mastership_store(struct device *dev, struct device_attribute *attr,
> +		 const char *buf, size_t count)
> +{
> +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> +
> +	if (i3cbus->cur_master == master->this) {
> +		dev_err(dev, "I'm current mater.");
> +		return count;
> +	}
> +
> +	if (!master->ops->request_mastership) {
> +		dev_err(dev, "mastership_request not supported.");
> +		return count;
> +	}
> +
> +	if (master->ops->request_mastership(master))
> +		dev_err(dev, "mastership_request failed");
> +	else
> +		dev_err(dev, "mastership_request success");
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(mastership);
> +
>  static ssize_t current_master_show(struct device *dev,
>  				   struct device_attribute *da,
>  				   char *buf)
> @@ -457,8 +497,11 @@ static ssize_t current_master_show(struct device *dev,
>  	ssize_t ret;
>  
>  	i3c_bus_normaluse_lock(i3cbus);
> -	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> -		      i3cbus->cur_master->info.pid);
> +	if (i3cbus->cur_master)
> +		ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> +			      i3cbus->cur_master->info.pid);
> +	else
> +		ret = sprintf(buf, "Not Current Master\n");
>  	i3c_bus_normaluse_unlock(i3cbus);
>  
>  	return ret;
> @@ -497,6 +540,7 @@ static DEVICE_ATTR_RO(i2c_scl_frequency);
>  
>  static struct attribute *i3c_masterdev_attrs[] = {
>  	&dev_attr_mode.attr,
> +	&dev_attr_mastership.attr,
>  	&dev_attr_current_master.attr,
>  	&dev_attr_i3c_scl_frequency.attr,
>  	&dev_attr_i2c_scl_frequency.attr,
> @@ -854,6 +898,53 @@ int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
>  EXPORT_SYMBOL_GPL(i3c_master_enec_locked);
>  
>  /**
> + * i3c_master_getaccmst_locked() - send an GETACCMST CCC command
> + * @master: master used to send frames on the bus
> + * @addr: a valid I3C slave address
> + *
> + * Sends an GETACCMST CCC command to offer bus Mastership to an
> + * I3C Secondary Master.
> + *
> + * 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_getaccmst_locked(struct i3c_master_controller *master, u8 addr)
> +{
> +	enum i3c_addr_slot_status addrstat;
> +	struct i3c_ccc_getaccmst *accmst;
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	addrstat = i3c_bus_get_addr_slot_status(&master->bus, addr);
> +	if (addr == I3C_BROADCAST_ADDR || addrstat != I3C_ADDR_SLOT_I3C_DEV)
> +		return -EINVAL;
> +
> +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> +	if (!accmst)
> +		return -ENOMEM;
> +
> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	if (ret)
> +		goto out;
> +
> +	if (accmst->newmaster >> 1 != addr)

I really like this check. This is something I realized working
on next patch version.

> +		ret = -EIO;
> +out:
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_getaccmst_locked);
> +
> +/**
>   * i3c_master_defslvs_locked() - send a DEFSLVS CCC command
>   * @master: master used to send frames on the bus
>   *
> @@ -1542,8 +1633,7 @@ int i3c_master_set_info(struct i3c_master_controller *master,
>  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
>  		return -EINVAL;
>  
> -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> -	    master->secondary)
> +	if (I3C_BCR_DEVICE_ROLE(info->bcr) != I3C_BCR_I3C_MASTER)
>  		return -EINVAL;
>  
>  	if (master->this)
> @@ -2381,6 +2471,81 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>  	return 0;
>  }
>  
> +int i3c_sec_master_bus_init(struct i3c_master_controller *master)
> +{
> +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> +	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> +	enum i3c_bus_mode mode = i3cbus->mode;
> +	struct i3c_defslvs_info *defslvsinfo;
> +	int ret = 0;
> +
> +	if (master->init_done)
> +		return -EINVAL;
> +
> +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> +		if (defslvsinfo->slave.dyn_addr)
> +			continue;
> +
> +		switch (defslvsinfo->slave.lvr & I3C_LVR_I2C_INDEX_MASK) {
> +		case I3C_LVR_I2C_INDEX(0):
> +			if (mode < I3C_BUS_MODE_MIXED_FAST)
> +				mode = I3C_BUS_MODE_MIXED_FAST;
> +			break;
> +		case I3C_LVR_I2C_INDEX(1):
> +		case I3C_LVR_I2C_INDEX(2):
> +			if (mode < I3C_BUS_MODE_MIXED_SLOW)
> +				mode = I3C_BUS_MODE_MIXED_SLOW;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			goto err_put_dev;
> +		}
> +		if (defslvsinfo->slave.lvr & I3C_LVR_I2C_FM_MODE)
> +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
> +	}
> +
> +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
> +	if (ret)
> +		goto err_put_dev;
> +
> +	/*
> +	 * Now execute the controller specific ->bus_init() routine, which
> +	 * might configure its internal logic to match the bus limitations.
> +	 */
> +	ret = master->ops->bus_init(master);
> +	if (ret)
> +		goto err_put_dev;
> +
> +	/*
> +	 * The master device should have been instantiated in ->bus_init(),
> +	 * complain if this was not the case.
> +	 */
> +	if (!master->this) {
> +		dev_err(&master->dev,
> +			"master_set_info() was not called in ->bus_init()\n");
> +		ret = -EINVAL;
> +		goto err_put_dev;
> +	}
> +
> +	ret = device_add(&master->dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Expose our I3C bus as an I2C adapter so that I2C devices are exposed
> +	 * through the I2C subsystem.
> +	 */
> +	ret = i3c_master_i2c_adapter_init(master);
> +	if (ret)
> +		goto err_put_dev;
> +
> +	master->init_done = true;
> +
> +err_put_dev:
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_sec_master_bus_init);
> +
>  /**
>   * i3c_master_register() - register an I3C master
>   * @master: master used to send frames on the bus
> @@ -2413,10 +2578,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;
> @@ -2430,6 +2591,7 @@ int i3c_master_register(struct i3c_master_controller *master,
>  	master->secondary = secondary;
>  	INIT_LIST_HEAD(&master->boardinfo.i2c);
>  	INIT_LIST_HEAD(&master->boardinfo.i3c);
> +	INIT_LIST_HEAD(&master->defslvs);
>  
>  	ret = i3c_bus_init(i3cbus);
>  	if (ret)
> @@ -2475,6 +2637,9 @@ int i3c_master_register(struct i3c_master_controller *master,
>  		goto err_put_dev;
>  	}
>  
> +	if (secondary)
> +		return 0;
> +
>  	ret = i3c_master_bus_init(master);
>  	if (ret)
>  		goto err_put_dev;
> @@ -2547,6 +2712,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>  	if (!master || !xfers)
>  		return -EINVAL;
>  
> +	if (master->bus.cur_master != master->this) {
> +		master->ops->request_mastership(master);
> +		return -EBUSY;

I don't like this approach, so you have to re-trigger the operation when
this master becomes current master. It is not transparent. Especially,
HCI 1.1 describes the process in detail, even  on flow chart and you can 
see there that you should block all the transfers/tasks on your side and
wait for GETACCMST.

> +	}
> +
>  	if (!master->ops->priv_xfers)
>  		return -ENOTSUPP;
>  
> @@ -2638,6 +2808,185 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
>  	dev->ibi = NULL;
>  }
>  
> +static const char *const i3c_dr_modes[] = {
> +	[I3C_DR_MODE_MASTER]		= "master",
> +	[I3C_DR_MODE_SEC_MASTER]	= "sec-master",
> +	[I3C_DR_MODE_SLAVE]		= "slave",
> +};
> +
> +static enum i3c_dr_mode i3c_get_dr_mode_from_string(const char *str)
> +{
> +	int ret;
> +
> +	ret = match_string(i3c_dr_modes, ARRAY_SIZE(i3c_dr_modes), str);
> +	return (ret < 0) ? I3C_DR_MODE_MASTER : ret;
> +}
> +
> +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev)
> +{
> +	const char *dr_mode;
> +	int err;
> +
> +	err = device_property_read_string(dev, "dr-mode", &dr_mode);
> +	if (err < 0)
> +		return I3C_DR_MODE_MASTER;
> +
> +	return i3c_get_dr_mode_from_string(dr_mode);
> +}
> +EXPORT_SYMBOL_GPL(i3c_get_dr_mode);
> +
> +int i3c_sec_master_request_mastership(struct i3c_master_controller *master)
> +{
> +	int ret;
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +	ret = master->ops->request_mastership(master);
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_sec_master_request_mastership);
> +
> +int i3c_master_deliver_mastership(struct i3c_master_controller *master, u8 addr)

I agree, wa can introduce this now. But we decided to postpone it. As
you can see, it shouldn't be so hard.

> +{
> +	struct i3c_dev_desc *dev;
> +	int ret;
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +	i3c_bus_for_each_i3cdev(&master->bus, dev) {
> +		if (dev->ibi) {
> +			mutex_lock(&dev->ibi_lock);
> +			i3c_dev_disable_ibi_locked(dev);
> +			mutex_unlock(&dev->ibi_lock);
> +		}
> +	}
> +	i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
> +				I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> +
> +	ret = master->ops->deliver_mastership(master, addr);
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_deliver_mastership);
> +
> +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> +			     struct i3c_ccc_dev_desc defslvs)
> +{
> +	struct i3c_defslvs_info *defslvsinfo;
> +	struct device *dev = &master->dev;
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	defslvsinfo = devm_kzalloc(dev, sizeof(*defslvsinfo), GFP_KERNEL);
> +	if (!defslvsinfo)
> +		return -ENOMEM;
> +
> +	defslvsinfo->slave = defslvs;
> +
> +	list_add_tail(&defslvsinfo->node, &master->defslvs);

I don't get why can't we call i3c_master_add_i3c_dev_locked when
populating the bus. You have all the data on your plate (in HC driver)
when you are populating it from SEC_DEV_CHAR_TABLE_LOC.

I decided to do it similarly, but then Boris suggested to rework it and
we use only i3c_master_add_i3c_dev_locked.

> +
> +	i3c_bus_maintenance_unlock(&master->bus);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(defslvs_populate_i3c_bus);
> +
> +static void i3c_master_add_new_defslvs(struct i3c_master_controller *master)
> +{
> +	struct i3c_defslvs_info *defslvsinfo;
> +
> +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> +		/* TODO: add i2c devices to the bus */
> +		if (!defslvsinfo->slave.dyn_addr)
> +			continue;
> +
> +		if (defslvsinfo->slave.dyn_addr == master->this->info.dyn_addr)
> +			continue;
> +
> +		if (!i3c_bus_dev_addr_is_avail(&master->bus,
> +					       defslvsinfo->slave.dyn_addr))

We can add those checks also but we also have i3c_master_attach_i3c_dev
and i3c_master_get_i3c_addrs which takes care of this everything.

> +			continue;
> +
> +		i3c_master_add_i3c_dev_locked(master, defslvsinfo->slave.dyn_addr);
> +	}
> +
> +	while (!list_empty(&master->defslvs)) {
> +		defslvsinfo = list_first_entry(&master->defslvs,
> +					       struct i3c_defslvs_info, node);
> +		list_del(&defslvsinfo->node);

I feel like this code is redundant, we have to allocate it, then delete.

> +	}
> +}
> +
> +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> +				     bool secondary)
> +{
> +	struct i3c_dev_desc *dev;
> +	int ret;
> +
> +	if (master->secondary == secondary)
> +		return -EEXIST;
> +
> +	/* TODO: get the current master information */
> +	if (secondary)
> +		master->bus.cur_master = NULL;
> +	else
> +		master->bus.cur_master = master->this;
> +
> +	if (!master->init_done && !secondary)
> +		i3c_sec_master_bus_init(master);
> +
> +	master->secondary = secondary;
> +
> +	dev_info(&master->dev, "changing to %s\n",
> +		 master->secondary ? "Sec Master" : "Master");
> +
> +	/* TODO: Analyse the use of maintenan_lock for everything */
> +	if (!list_empty(&master->defslvs) && !secondary) {
> +		i3c_bus_maintenance_lock(&master->bus);
> +		i3c_master_add_new_defslvs(master);
> +		i3c_bus_maintenance_unlock(&master->bus);
> +
> +		i3c_bus_normaluse_lock(&master->bus);
> +		i3c_master_register_new_i3c_devs(master);

Take a look also at i3c_master_bus_takeover from my latest patch. BTW.
what about I2C devices? We worked on that also, and this is part of the
latest patch also. I'm testing it with I2C devices also.

> +		i3c_bus_normaluse_unlock(&master->bus);
> +	}
> +
> +	if (!secondary) {
> +		i3c_bus_normaluse_lock(&master->bus);
> +		i3c_bus_for_each_i3cdev(&master->bus, dev) {
> +			if (dev->ibi) {
> +				mutex_lock(&dev->ibi_lock);
> +				ret = i3c_dev_enable_ibi_locked(dev);
> +				if (ret)
> +					dev_err(&master->dev,
> +						"Failed to re-enable IBI on device %d-%llx",
> +						master->bus.id, dev->info.pid);
> +				mutex_unlock(&dev->ibi_lock);
> +				}
> +		}
> +
> +		/* TODO: Enable MR only for the elegible devices */

This was postponed also, but we had that before. We can add per-device
granularity to i3c_master_bus_takeover().

> +		i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
> +					I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> +		i3c_bus_normaluse_unlock(&master->bus);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
> +
> +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
> +{
> +	int res;
> +
> +	mutex_lock(&i3c_core_lock);
> +	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
> +	mutex_unlock(&i3c_core_lock);
> +
> +	return res;
> +}
> +EXPORT_SYMBOL_GPL(i3c_for_each_dev);
> +
>  static int __init i3c_init(void)
>  {
>  	return bus_register(&i3c_bus_type);
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 9cb39d9..09bd99c 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -426,6 +426,8 @@ struct i3c_bus {
>   *		      for a future IBI
>   *		      This method is mandatory only if ->request_ibi is not
>   *		      NULL.
> + * @request_mastership: Request mastership.
> + * @deliver_mastership: Deliver mastership
>   */
>  struct i3c_master_controller_ops {
>  	int (*bus_init)(struct i3c_master_controller *master);
> @@ -452,6 +454,21 @@ 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);
> +	int (*request_mastership)(struct i3c_master_controller *master);
> +	int (*deliver_mastership)(struct i3c_master_controller *master,
> +				  u8 addr);
> +};
> +
> +/**
> + * struct i3c_defslvs_info - defslvs information object
> + * @node: used to insert the defslvs object in the  list
> + * @slave: I3C/I2C device descriptor used for DEFSLVS
> + *
> + * This structure is used to hold defslvs information on Secondary Master.
> + */
> +struct i3c_defslvs_info {
> +	struct list_head node;
> +	struct i3c_ccc_dev_desc slave;
>  };
>  
>  /**
> @@ -468,6 +485,7 @@ struct i3c_master_controller_ops {
>   * @boardinfo.i3c: list of I3C  boardinfo objects
>   * @boardinfo.i2c: list of I2C boardinfo objects
>   * @boardinfo: board-level information attached to devices connected on the bus
> + * @defslvs: List of defslvs objects
>   * @bus: I3C bus exposed by this master
>   * @wq: workqueue used to execute IBI handlers. Can also be used by master
>   *	drivers if they need to postpone operations that need to take place
> @@ -491,6 +509,7 @@ struct i3c_master_controller {
>  		struct list_head i3c;
>  		struct list_head i2c;
>  	} boardinfo;
> +	struct list_head defslvs;
>  	struct i3c_bus bus;
>  	struct workqueue_struct *wq;
>  };
> @@ -525,6 +544,7 @@ int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr,
>  			    u8 evts);
>  int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
>  			   u8 evts);
> +int i3c_master_getaccmst_locked(struct i3c_master_controller *master, u8 addr);
>  int i3c_master_entdaa_locked(struct i3c_master_controller *master);
>  int i3c_master_defslvs_locked(struct i3c_master_controller *master);
>  
> @@ -652,4 +672,23 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
>  
>  struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
>  
> +enum i3c_dr_mode {
> +	I3C_DR_MODE_MASTER,
> +	I3C_DR_MODE_SEC_MASTER,
> +	I3C_DR_MODE_SLAVE,
> +};
> +
> +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev);
> +
> +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> +				     bool secondary);
> +
> +int i3c_sec_master_request_mastership(struct i3c_master_controller *master);
> +int i3c_master_deliver_mastership(struct i3c_master_controller *master,
> +				  u8 addr);
> +
> +int i3c_sec_master_bus_init(struct i3c_master_controller *master);
> +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> +			     struct i3c_ccc_dev_desc defslvs);
> +
>  #endif /* I3C_MASTER_H */
> -- 
> 2.7.4
> 

I feel like this is based on early approach which has evolved by the time
and I think some of the nice part are missing. The biggest parts to
discuass are:
- bus population using devslvs device list instead of using
  i3c_master_add_i3c_dev locked, have you tried that? If something does
  not work for you, wa can adjust it but I really want you to try the
  new approach

- the way how we are requesting mastership, IMO, we should wait untill
  the process is finished, as also described in HCI spec for example.
  When I introduce interrupt-based solution, everything should be fine.
  Could you do that also in your driver?

Again, good to have that. I'm really counting on a fair discussion :-)

-- 
--
Regards, 
Przemyslaw Gaj

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 1/2] i3c: Add preliminary support for secondary master

[Vitor Soares]

Hi,


From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Thu, Nov 28, 2019 at 05:50:08

> Hi Vitor,
> 
> First, you woke up my son and he couldn't sleep the rest of the night
> :-)

Sorry for that.

> I appreciate you sent that so we can discuss it.
> 
> The 11/28/2019 02:15, Vitor Soares wrote:
> > 
> > This patch adds the preliminary support for secondary master feature to
> > i3c Framework for testing purposes.
> > 
> > Key points for consideration:
> >   - mastership_[show/store] are only used for testing
> >   - secondary master registration is made in two steps, one in
> >   i3c_master_register() and another in i3c_sec_master_bus_init() when
> >   secondary master became current master first time. This is made in this
> >   way to get all dt declared boardinfo list, create defslvs list and
> >   provide work_queue.
> >   - When the current master wants to deliver_mastership it necessary to
> >   disable all in-band events to avoid unwanted interrupt during bus
> >   ownership exchange. For now this patch doesn't reflect all
> >   improvements/changes made in v1.1 I3C Bus spec. But it can be extended
> >   by adding some commands and checks to the flow.
> >   - i3c_defslvs_info: The DEFSLVS info can be differently stored in
> >   diferen HC. Hence it is used a defslvs list similar to boardinfo list in
> >   the bus structure to hold this data. Them HC is taccking over the bus
> >   ownership can initialize each device of that list. For now, this not
> >   address the i2c devices since they are only statically described.
> >   - [request/deliver]_mastership(): Mastership request deliver may be done
> >   differently in different HC, hence the need to have a call back for each
> >   process.
> >   - Add dr_mode to DT: Similar to USB, HC can be programmed to Master only
> >   mode, Slave only mode or Secondary Master which aren't necessarily
> >   hardcoded.
> >   - bus_mode definition: The bus mode is defined even without defslvs
> >   information with DT info since the definition of i2c devices are those
> >   that have impact on bus_mode definition and need to statically declared.
> >   The only use case that may cause issues is when i2c devices aren't
> >   declared in secondary master side and bus mode doesn't match the
> >   main master. Anyway this can be solde without extra complexity.
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> >  drivers/i3c/master.c       | 365 ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/i3c/master.h |  39 +++++
> >  2 files changed, 396 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 0436916..b398d77 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -449,6 +449,46 @@ static ssize_t mode_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(mode);
> >  
> > +static ssize_t
> > +mastership_show(struct device *dev, struct device_attribute *da, char *buf)
> > +{
> > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > +	ssize_t ret;
> > +
> > +	if (master->secondary)
> > +		ret = sprintf(buf, "Secondary Master\n");
> > +	else
> > +		ret = sprintf(buf, "Master\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static ssize_t
> > +mastership_store(struct device *dev, struct device_attribute *attr,
> > +		 const char *buf, size_t count)
> > +{
> > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> > +
> > +	if (i3cbus->cur_master == master->this) {
> > +		dev_err(dev, "I'm current mater.");
> > +		return count;
> > +	}
> > +
> > +	if (!master->ops->request_mastership) {
> > +		dev_err(dev, "mastership_request not supported.");
> > +		return count;
> > +	}
> > +
> > +	if (master->ops->request_mastership(master))
> > +		dev_err(dev, "mastership_request failed");
> > +	else
> > +		dev_err(dev, "mastership_request success");
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(mastership);
> > +
> >  static ssize_t current_master_show(struct device *dev,
> >  				   struct device_attribute *da,
> >  				   char *buf)
> > @@ -457,8 +497,11 @@ static ssize_t current_master_show(struct device *dev,
> >  	ssize_t ret;
> >  
> >  	i3c_bus_normaluse_lock(i3cbus);
> > -	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > -		      i3cbus->cur_master->info.pid);
> > +	if (i3cbus->cur_master)
> > +		ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > +			      i3cbus->cur_master->info.pid);
> > +	else
> > +		ret = sprintf(buf, "Not Current Master\n");
> >  	i3c_bus_normaluse_unlock(i3cbus);
> >  
> >  	return ret;
> > @@ -497,6 +540,7 @@ static DEVICE_ATTR_RO(i2c_scl_frequency);
> >  
> >  static struct attribute *i3c_masterdev_attrs[] = {
> >  	&dev_attr_mode.attr,
> > +	&dev_attr_mastership.attr,
> >  	&dev_attr_current_master.attr,
> >  	&dev_attr_i3c_scl_frequency.attr,
> >  	&dev_attr_i2c_scl_frequency.attr,
> > @@ -854,6 +898,53 @@ int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> >  EXPORT_SYMBOL_GPL(i3c_master_enec_locked);
> >  
> >  /**
> > + * i3c_master_getaccmst_locked() - send an GETACCMST CCC command
> > + * @master: master used to send frames on the bus
> > + * @addr: a valid I3C slave address
> > + *
> > + * Sends an GETACCMST CCC command to offer bus Mastership to an
> > + * I3C Secondary Master.
> > + *
> > + * 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_getaccmst_locked(struct i3c_master_controller *master, u8 addr)
> > +{
> > +	enum i3c_addr_slot_status addrstat;
> > +	struct i3c_ccc_getaccmst *accmst;
> > +	struct i3c_ccc_cmd_dest dest;
> > +	struct i3c_ccc_cmd cmd;
> > +	int ret;
> > +
> > +	if (!master)
> > +		return -EINVAL;
> > +
> > +	addrstat = i3c_bus_get_addr_slot_status(&master->bus, addr);
> > +	if (addr == I3C_BROADCAST_ADDR || addrstat != I3C_ADDR_SLOT_I3C_DEV)
> > +		return -EINVAL;
> > +
> > +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> > +	if (!accmst)
> > +		return -ENOMEM;
> > +
> > +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> > +
> > +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (accmst->newmaster >> 1 != addr)
> 
> I really like this check. This is something I realized working
> on next patch version.
> 
> > +		ret = -EIO;
> > +out:
> > +	i3c_ccc_cmd_dest_cleanup(&dest);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_getaccmst_locked);
> > +
> > +/**
> >   * i3c_master_defslvs_locked() - send a DEFSLVS CCC command
> >   * @master: master used to send frames on the bus
> >   *
> > @@ -1542,8 +1633,7 @@ int i3c_master_set_info(struct i3c_master_controller *master,
> >  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
> >  		return -EINVAL;
> >  
> > -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> > -	    master->secondary)
> > +	if (I3C_BCR_DEVICE_ROLE(info->bcr) != I3C_BCR_I3C_MASTER)
> >  		return -EINVAL;
> >  
> >  	if (master->this)
> > @@ -2381,6 +2471,81 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
> >  	return 0;
> >  }
> >  
> > +int i3c_sec_master_bus_init(struct i3c_master_controller *master)
> > +{
> > +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > +	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> > +	enum i3c_bus_mode mode = i3cbus->mode;
> > +	struct i3c_defslvs_info *defslvsinfo;
> > +	int ret = 0;
> > +
> > +	if (master->init_done)
> > +		return -EINVAL;
> > +
> > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > +		if (defslvsinfo->slave.dyn_addr)
> > +			continue;
> > +
> > +		switch (defslvsinfo->slave.lvr & I3C_LVR_I2C_INDEX_MASK) {
> > +		case I3C_LVR_I2C_INDEX(0):
> > +			if (mode < I3C_BUS_MODE_MIXED_FAST)
> > +				mode = I3C_BUS_MODE_MIXED_FAST;
> > +			break;
> > +		case I3C_LVR_I2C_INDEX(1):
> > +		case I3C_LVR_I2C_INDEX(2):
> > +			if (mode < I3C_BUS_MODE_MIXED_SLOW)
> > +				mode = I3C_BUS_MODE_MIXED_SLOW;
> > +			break;
> > +		default:
> > +			ret = -EINVAL;
> > +			goto err_put_dev;
> > +		}
> > +		if (defslvsinfo->slave.lvr & I3C_LVR_I2C_FM_MODE)
> > +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
> > +	}
> > +
> > +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
> > +	if (ret)
> > +		goto err_put_dev;
> > +
> > +	/*
> > +	 * Now execute the controller specific ->bus_init() routine, which
> > +	 * might configure its internal logic to match the bus limitations.
> > +	 */
> > +	ret = master->ops->bus_init(master);
> > +	if (ret)
> > +		goto err_put_dev;
> > +
> > +	/*
> > +	 * The master device should have been instantiated in ->bus_init(),
> > +	 * complain if this was not the case.
> > +	 */
> > +	if (!master->this) {
> > +		dev_err(&master->dev,
> > +			"master_set_info() was not called in ->bus_init()\n");
> > +		ret = -EINVAL;
> > +		goto err_put_dev;
> > +	}
> > +
> > +	ret = device_add(&master->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Expose our I3C bus as an I2C adapter so that I2C devices are exposed
> > +	 * through the I2C subsystem.
> > +	 */
> > +	ret = i3c_master_i2c_adapter_init(master);
> > +	if (ret)
> > +		goto err_put_dev;
> > +
> > +	master->init_done = true;
> > +
> > +err_put_dev:
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_sec_master_bus_init);
> > +
> >  /**
> >   * i3c_master_register() - register an I3C master
> >   * @master: master used to send frames on the bus
> > @@ -2413,10 +2578,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;
> > @@ -2430,6 +2591,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> >  	master->secondary = secondary;
> >  	INIT_LIST_HEAD(&master->boardinfo.i2c);
> >  	INIT_LIST_HEAD(&master->boardinfo.i3c);
> > +	INIT_LIST_HEAD(&master->defslvs);
> >  
> >  	ret = i3c_bus_init(i3cbus);
> >  	if (ret)
> > @@ -2475,6 +2637,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> >  		goto err_put_dev;
> >  	}
> >  
> > +	if (secondary)
> > +		return 0;
> > +
> >  	ret = i3c_master_bus_init(master);
> >  	if (ret)
> >  		goto err_put_dev;
> > @@ -2547,6 +2712,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> >  	if (!master || !xfers)
> >  		return -EINVAL;
> >  
> > +	if (master->bus.cur_master != master->this) {
> > +		master->ops->request_mastership(master);
> > +		return -EBUSY;
> 
> I don't like this approach, so you have to re-trigger the operation when
> this master becomes current master. It is not transparent. Especially,
> HCI 1.1 describes the process in detail, even  on flow chart and you can 
> see there that you should block all the transfers/tasks on your side and
> wait for GETACCMST.

I forgot to explain that this is part is not fully operational and my 
intention was to address this in near future.
To quickly head-up, what I had in my mind when did this was if a device 
wants to do a xfer and sec master is not current master the framework 
will request the bus ownership and them pass a EBUSY in case of success 
or another error in case current master Nacks the MR request.

> 
> > +	}
> > +
> >  	if (!master->ops->priv_xfers)
> >  		return -ENOTSUPP;
> >  
> > @@ -2638,6 +2808,185 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> >  	dev->ibi = NULL;
> >  }
> >  
> > +static const char *const i3c_dr_modes[] = {
> > +	[I3C_DR_MODE_MASTER]		= "master",
> > +	[I3C_DR_MODE_SEC_MASTER]	= "sec-master",
> > +	[I3C_DR_MODE_SLAVE]		= "slave",
> > +};
> > +
> > +static enum i3c_dr_mode i3c_get_dr_mode_from_string(const char *str)
> > +{
> > +	int ret;
> > +
> > +	ret = match_string(i3c_dr_modes, ARRAY_SIZE(i3c_dr_modes), str);
> > +	return (ret < 0) ? I3C_DR_MODE_MASTER : ret;
> > +}
> > +
> > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev)
> > +{
> > +	const char *dr_mode;
> > +	int err;
> > +
> > +	err = device_property_read_string(dev, "dr-mode", &dr_mode);
> > +	if (err < 0)
> > +		return I3C_DR_MODE_MASTER;
> > +
> > +	return i3c_get_dr_mode_from_string(dr_mode);
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_get_dr_mode);
> > +
> > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master)
> > +{
> > +	int ret;
> > +
> > +	i3c_bus_normaluse_lock(&master->bus);
> > +	ret = master->ops->request_mastership(master);
> > +	i3c_bus_normaluse_unlock(&master->bus);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_sec_master_request_mastership);
> > +
> > +int i3c_master_deliver_mastership(struct i3c_master_controller *master, u8 addr)
> 
> I agree, wa can introduce this now. But we decided to postpone it. As
> you can see, it shouldn't be so hard.

IMO this is important, I could use directly the CCC but I know that there 
is other HC that may use a different approach.

> 
> > +{
> > +	struct i3c_dev_desc *dev;
> > +	int ret;
> > +
> > +	i3c_bus_normaluse_lock(&master->bus);
> > +	i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > +		if (dev->ibi) {
> > +			mutex_lock(&dev->ibi_lock);
> > +			i3c_dev_disable_ibi_locked(dev);
> > +			mutex_unlock(&dev->ibi_lock);
> > +		}
> > +	}
> > +	i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
> > +				I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > +
> > +	ret = master->ops->deliver_mastership(master, addr);
> > +	i3c_bus_normaluse_unlock(&master->bus);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_deliver_mastership);
> > +
> > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > +			     struct i3c_ccc_dev_desc defslvs)
> > +{
> > +	struct i3c_defslvs_info *defslvsinfo;
> > +	struct device *dev = &master->dev;
> > +
> > +	i3c_bus_maintenance_lock(&master->bus);
> > +	defslvsinfo = devm_kzalloc(dev, sizeof(*defslvsinfo), GFP_KERNEL);
> > +	if (!defslvsinfo)
> > +		return -ENOMEM;
> > +
> > +	defslvsinfo->slave = defslvs;
> > +
> > +	list_add_tail(&defslvsinfo->node, &master->defslvs);
> 
> I don't get why can't we call i3c_master_add_i3c_dev_locked when
> populating the bus.

I want to avoid the populating bus call back.

> You have all the data on your plate (in HC driver)
> when you are populating it from SEC_DEV_CHAR_TABLE_LOC.

Yes, I told you that I have a table for that, yet I decide to not use it. 
My concern is about the HC that doesn't have?
For me passing this task for the framework is more universal. 

> 
> I decided to do it similarly, but then Boris suggested to rework it and
> we use only i3c_master_add_i3c_dev_locked.
> 
> > +
> > +	i3c_bus_maintenance_unlock(&master->bus);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(defslvs_populate_i3c_bus);
> > +
> > +static void i3c_master_add_new_defslvs(struct i3c_master_controller *master)
> > +{
> > +	struct i3c_defslvs_info *defslvsinfo;
> > +
> > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > +		/* TODO: add i2c devices to the bus */
> > +		if (!defslvsinfo->slave.dyn_addr)
> > +			continue;
> > +
> > +		if (defslvsinfo->slave.dyn_addr == master->this->info.dyn_addr)
> > +			continue;
> > +
> > +		if (!i3c_bus_dev_addr_is_avail(&master->bus,
> > +					       defslvsinfo->slave.dyn_addr))
> 
> We can add those checks also but we also have i3c_master_attach_i3c_dev
> and i3c_master_get_i3c_addrs which takes care of this everything.

Yes, but them you will be allocating and free devs unnecessarily.

> 
> > +			continue;
> > +
> > +		i3c_master_add_i3c_dev_locked(master, defslvsinfo->slave.dyn_addr);
> > +	}
> > +
> > +	while (!list_empty(&master->defslvs)) {
> > +		defslvsinfo = list_first_entry(&master->defslvs,
> > +					       struct i3c_defslvs_info, node);
> > +		list_del(&defslvsinfo->node);
> 
> I feel like this code is redundant, we have to allocate it, then delete.

No. you need to clean the list. I may receive another one in the future 
due a HJ or a dynamic address change.

> 
> > +	}
> > +}
> > +
> > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > +				     bool secondary)
> > +{
> > +	struct i3c_dev_desc *dev;
> > +	int ret;
> > +
> > +	if (master->secondary == secondary)
> > +		return -EEXIST;
> > +
> > +	/* TODO: get the current master information */
> > +	if (secondary)
> > +		master->bus.cur_master = NULL;
> > +	else
> > +		master->bus.cur_master = master->this;
> > +
> > +	if (!master->init_done && !secondary)
> > +		i3c_sec_master_bus_init(master);
> > +
> > +	master->secondary = secondary;
> > +
> > +	dev_info(&master->dev, "changing to %s\n",
> > +		 master->secondary ? "Sec Master" : "Master");
> > +
> > +	/* TODO: Analyse the use of maintenan_lock for everything */
> > +	if (!list_empty(&master->defslvs) && !secondary) {
> > +		i3c_bus_maintenance_lock(&master->bus);
> > +		i3c_master_add_new_defslvs(master);
> > +		i3c_bus_maintenance_unlock(&master->bus);
> > +
> > +		i3c_bus_normaluse_lock(&master->bus);
> > +		i3c_master_register_new_i3c_devs(master);
> 
> Take a look also at i3c_master_bus_takeover from my latest patch. BTW.
> what about I2C devices? We worked on that also, and this is part of the
> latest patch also. I'm testing it with I2C devices also.

Please check the comments for that. Anyway you can declare the i2c 
devices of DT on both sides, what I think it should be a good practice.

> 
> > +		i3c_bus_normaluse_unlock(&master->bus);
> > +	}
> > +
> > +	if (!secondary) {
> > +		i3c_bus_normaluse_lock(&master->bus);
> > +		i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > +			if (dev->ibi) {
> > +				mutex_lock(&dev->ibi_lock);
> > +				ret = i3c_dev_enable_ibi_locked(dev);
> > +				if (ret)
> > +					dev_err(&master->dev,
> > +						"Failed to re-enable IBI on device %d-%llx",
> > +						master->bus.id, dev->info.pid);
> > +				mutex_unlock(&dev->ibi_lock);
> > +				}
> > +		}
> > +
> > +		/* TODO: Enable MR only for the elegible devices */
> 
> This was postponed also, but we had that before. We can add per-device
> granularity to i3c_master_bus_takeover().

Here we need to do the same as for ibi and maybe get the eligible devices 
from DT or based on its DCR,BCR and PID.
This is something that should be address because it represent a big 
security gap.

> 
> > +		i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
> > +					I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > +		i3c_bus_normaluse_unlock(&master->bus);
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
> > +
> > +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
> > +{
> > +	int res;
> > +
> > +	mutex_lock(&i3c_core_lock);
> > +	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
> > +	mutex_unlock(&i3c_core_lock);
> > +
> > +	return res;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_for_each_dev);
> > +
> >  static int __init i3c_init(void)
> >  {
> >  	return bus_register(&i3c_bus_type);
> > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > index 9cb39d9..09bd99c 100644
> > --- a/include/linux/i3c/master.h
> > +++ b/include/linux/i3c/master.h
> > @@ -426,6 +426,8 @@ struct i3c_bus {
> >   *		      for a future IBI
> >   *		      This method is mandatory only if ->request_ibi is not
> >   *		      NULL.
> > + * @request_mastership: Request mastership.
> > + * @deliver_mastership: Deliver mastership
> >   */
> >  struct i3c_master_controller_ops {
> >  	int (*bus_init)(struct i3c_master_controller *master);
> > @@ -452,6 +454,21 @@ 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);
> > +	int (*request_mastership)(struct i3c_master_controller *master);
> > +	int (*deliver_mastership)(struct i3c_master_controller *master,
> > +				  u8 addr);
> > +};
> > +
> > +/**
> > + * struct i3c_defslvs_info - defslvs information object
> > + * @node: used to insert the defslvs object in the  list
> > + * @slave: I3C/I2C device descriptor used for DEFSLVS
> > + *
> > + * This structure is used to hold defslvs information on Secondary Master.
> > + */
> > +struct i3c_defslvs_info {
> > +	struct list_head node;
> > +	struct i3c_ccc_dev_desc slave;
> >  };
> >  
> >  /**
> > @@ -468,6 +485,7 @@ struct i3c_master_controller_ops {
> >   * @boardinfo.i3c: list of I3C  boardinfo objects
> >   * @boardinfo.i2c: list of I2C boardinfo objects
> >   * @boardinfo: board-level information attached to devices connected on the bus
> > + * @defslvs: List of defslvs objects
> >   * @bus: I3C bus exposed by this master
> >   * @wq: workqueue used to execute IBI handlers. Can also be used by master
> >   *	drivers if they need to postpone operations that need to take place
> > @@ -491,6 +509,7 @@ struct i3c_master_controller {
> >  		struct list_head i3c;
> >  		struct list_head i2c;
> >  	} boardinfo;
> > +	struct list_head defslvs;
> >  	struct i3c_bus bus;
> >  	struct workqueue_struct *wq;
> >  };
> > @@ -525,6 +544,7 @@ int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr,
> >  			    u8 evts);
> >  int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> >  			   u8 evts);
> > +int i3c_master_getaccmst_locked(struct i3c_master_controller *master, u8 addr);
> >  int i3c_master_entdaa_locked(struct i3c_master_controller *master);
> >  int i3c_master_defslvs_locked(struct i3c_master_controller *master);
> >  
> > @@ -652,4 +672,23 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
> >  
> >  struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
> >  
> > +enum i3c_dr_mode {
> > +	I3C_DR_MODE_MASTER,
> > +	I3C_DR_MODE_SEC_MASTER,
> > +	I3C_DR_MODE_SLAVE,
> > +};
> > +
> > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev);
> > +
> > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > +				     bool secondary);
> > +
> > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master);
> > +int i3c_master_deliver_mastership(struct i3c_master_controller *master,
> > +				  u8 addr);
> > +
> > +int i3c_sec_master_bus_init(struct i3c_master_controller *master);
> > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > +			     struct i3c_ccc_dev_desc defslvs);
> > +
> >  #endif /* I3C_MASTER_H */
> > -- 
> > 2.7.4
> > 
> 
> I feel like this is based on early approach which has evolved by the time
> and I think some of the nice part are missing. The biggest parts to
> discuass are:
> - bus population using devslvs device list instead of using
>   i3c_master_add_i3c_dev locked, have you tried that? If something does
>   not work for you, wa can adjust it but I really want you to try the
>   new approach

To me the i3c_master_add_i3c_dev_locked just need an improvement to 
propagate the boardinfo to all devices, something that already I started, 
and we need to discuss if on secondary master side we can change the 
devices addresses which imply to send DEFSLVS at the end of the process.

> 
> - the way how we are requesting mastership, IMO, we should wait untill
>   the process is finished, as also described in HCI spec for example.

I think you are confusing the concept here. I understand you want to do 
the xfer straight way after receive GETACCMST and I agree with that.
The thing is that you don't know what will happen on the bus between the 
time you send the request and you get the bus ownership. I just think we 
need to find another solution for that like defer the transfer to another 
time and when the controller switch the rule trigger all transfers in the 
pipeline (something like this) even before restore all ibi. If you block 
the bus you are not able to pass the defslvs to the framework.  

>   When I introduce interrupt-based solution, everything should be fine.
>   Could you do that also in your driver?

I did it but had issues and didn't fit in my use case. That the reason I 
did this approach.

> 
> Again, good to have that. I'm really counting on a fair discussion :-)
> 
> -- 
> --
> Regards, 
> Przemyslaw Gaj

As you can see this is based on your work. Off course it as some lose 
ends that should be addressed (like i2c devices, bus mode...) but don't 
have big impact in what I want to show.
For me it is important to rely on framework the way how bus ownership 
exchange is made because it will be easier to maintain in long term and 
we can introduce algorithms for the bus access in the future.

If you are using i3c-tree/next you can apply this directly and if you 
find anything else that isn't going ok please let me know.

Best regards,
Vitor Soares
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 1/2] i3c: Add preliminary support for secondary master

[Przemyslaw Gaj]

The 11/28/2019 12:20, Vitor Soares wrote:
> 
> Hi,
> 
> 
> From: Przemyslaw Gaj <pgaj@cadence.com>
> Date: Thu, Nov 28, 2019 at 05:50:08
> 
> > Hi Vitor,
> > 
> > First, you woke up my son and he couldn't sleep the rest of the night
> > :-)
> 
> Sorry for that.
> 
> > I appreciate you sent that so we can discuss it.
> > 
> > The 11/28/2019 02:15, Vitor Soares wrote:
> > > 
> > > This patch adds the preliminary support for secondary master feature to
> > > i3c Framework for testing purposes.
> > > 
> > > Key points for consideration:
> > >   - mastership_[show/store] are only used for testing
> > >   - secondary master registration is made in two steps, one in
> > >   i3c_master_register() and another in i3c_sec_master_bus_init() when
> > >   secondary master became current master first time. This is made in this
> > >   way to get all dt declared boardinfo list, create defslvs list and
> > >   provide work_queue.
> > >   - When the current master wants to deliver_mastership it necessary to
> > >   disable all in-band events to avoid unwanted interrupt during bus
> > >   ownership exchange. For now this patch doesn't reflect all
> > >   improvements/changes made in v1.1 I3C Bus spec. But it can be extended
> > >   by adding some commands and checks to the flow.
> > >   - i3c_defslvs_info: The DEFSLVS info can be differently stored in
> > >   diferen HC. Hence it is used a defslvs list similar to boardinfo list in
> > >   the bus structure to hold this data. Them HC is taccking over the bus
> > >   ownership can initialize each device of that list. For now, this not
> > >   address the i2c devices since they are only statically described.
> > >   - [request/deliver]_mastership(): Mastership request deliver may be done
> > >   differently in different HC, hence the need to have a call back for each
> > >   process.
> > >   - Add dr_mode to DT: Similar to USB, HC can be programmed to Master only
> > >   mode, Slave only mode or Secondary Master which aren't necessarily
> > >   hardcoded.
> > >   - bus_mode definition: The bus mode is defined even without defslvs
> > >   information with DT info since the definition of i2c devices are those
> > >   that have impact on bus_mode definition and need to statically declared.
> > >   The only use case that may cause issues is when i2c devices aren't
> > >   declared in secondary master side and bus mode doesn't match the
> > >   main master. Anyway this can be solde without extra complexity.
> > > 
> > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > ---
> > >  drivers/i3c/master.c       | 365 ++++++++++++++++++++++++++++++++++++++++++++-
> > >  include/linux/i3c/master.h |  39 +++++
> > >  2 files changed, 396 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index 0436916..b398d77 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -449,6 +449,46 @@ static ssize_t mode_show(struct device *dev,
> > >  }
> > >  static DEVICE_ATTR_RO(mode);
> > >  
> > > +static ssize_t
> > > +mastership_show(struct device *dev, struct device_attribute *da, char *buf)
> > > +{
> > > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > > +	ssize_t ret;
> > > +
> > > +	if (master->secondary)
> > > +		ret = sprintf(buf, "Secondary Master\n");
> > > +	else
> > > +		ret = sprintf(buf, "Master\n");
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static ssize_t
> > > +mastership_store(struct device *dev, struct device_attribute *attr,
> > > +		 const char *buf, size_t count)
> > > +{
> > > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > > +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> > > +
> > > +	if (i3cbus->cur_master == master->this) {
> > > +		dev_err(dev, "I'm current mater.");
> > > +		return count;
> > > +	}
> > > +
> > > +	if (!master->ops->request_mastership) {
> > > +		dev_err(dev, "mastership_request not supported.");
> > > +		return count;
> > > +	}
> > > +
> > > +	if (master->ops->request_mastership(master))
> > > +		dev_err(dev, "mastership_request failed");
> > > +	else
> > > +		dev_err(dev, "mastership_request success");
> > > +
> > > +	return count;
> > > +}
> > > +static DEVICE_ATTR_RW(mastership);
> > > +
> > >  static ssize_t current_master_show(struct device *dev,
> > >  				   struct device_attribute *da,
> > >  				   char *buf)
> > > @@ -457,8 +497,11 @@ static ssize_t current_master_show(struct device *dev,
> > >  	ssize_t ret;
> > >  
> > >  	i3c_bus_normaluse_lock(i3cbus);
> > > -	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > -		      i3cbus->cur_master->info.pid);
> > > +	if (i3cbus->cur_master)
> > > +		ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > +			      i3cbus->cur_master->info.pid);
> > > +	else
> > > +		ret = sprintf(buf, "Not Current Master\n");
> > >  	i3c_bus_normaluse_unlock(i3cbus);
> > >  
> > >  	return ret;
> > > @@ -497,6 +540,7 @@ static DEVICE_ATTR_RO(i2c_scl_frequency);
> > >  
> > >  static struct attribute *i3c_masterdev_attrs[] = {
> > >  	&dev_attr_mode.attr,
> > > +	&dev_attr_mastership.attr,
> > >  	&dev_attr_current_master.attr,
> > >  	&dev_attr_i3c_scl_frequency.attr,
> > >  	&dev_attr_i2c_scl_frequency.attr,
> > > @@ -854,6 +898,53 @@ int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> > >  EXPORT_SYMBOL_GPL(i3c_master_enec_locked);
> > >  
> > >  /**
> > > + * i3c_master_getaccmst_locked() - send an GETACCMST CCC command
> > > + * @master: master used to send frames on the bus
> > > + * @addr: a valid I3C slave address
> > > + *
> > > + * Sends an GETACCMST CCC command to offer bus Mastership to an
> > > + * I3C Secondary Master.
> > > + *
> > > + * 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_getaccmst_locked(struct i3c_master_controller *master, u8 addr)
> > > +{
> > > +	enum i3c_addr_slot_status addrstat;
> > > +	struct i3c_ccc_getaccmst *accmst;
> > > +	struct i3c_ccc_cmd_dest dest;
> > > +	struct i3c_ccc_cmd cmd;
> > > +	int ret;
> > > +
> > > +	if (!master)
> > > +		return -EINVAL;
> > > +
> > > +	addrstat = i3c_bus_get_addr_slot_status(&master->bus, addr);
> > > +	if (addr == I3C_BROADCAST_ADDR || addrstat != I3C_ADDR_SLOT_I3C_DEV)
> > > +		return -EINVAL;
> > > +
> > > +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> > > +	if (!accmst)
> > > +		return -ENOMEM;
> > > +
> > > +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> > > +
> > > +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	if (accmst->newmaster >> 1 != addr)
> > 
> > I really like this check. This is something I realized working
> > on next patch version.
> > 
> > > +		ret = -EIO;
> > > +out:
> > > +	i3c_ccc_cmd_dest_cleanup(&dest);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_master_getaccmst_locked);
> > > +
> > > +/**
> > >   * i3c_master_defslvs_locked() - send a DEFSLVS CCC command
> > >   * @master: master used to send frames on the bus
> > >   *
> > > @@ -1542,8 +1633,7 @@ int i3c_master_set_info(struct i3c_master_controller *master,
> > >  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
> > >  		return -EINVAL;
> > >  
> > > -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> > > -	    master->secondary)
> > > +	if (I3C_BCR_DEVICE_ROLE(info->bcr) != I3C_BCR_I3C_MASTER)
> > >  		return -EINVAL;
> > >  
> > >  	if (master->this)
> > > @@ -2381,6 +2471,81 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
> > >  	return 0;
> > >  }
> > >  
> > > +int i3c_sec_master_bus_init(struct i3c_master_controller *master)
> > > +{
> > > +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > +	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> > > +	enum i3c_bus_mode mode = i3cbus->mode;
> > > +	struct i3c_defslvs_info *defslvsinfo;
> > > +	int ret = 0;
> > > +
> > > +	if (master->init_done)
> > > +		return -EINVAL;
> > > +
> > > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > > +		if (defslvsinfo->slave.dyn_addr)
> > > +			continue;
> > > +
> > > +		switch (defslvsinfo->slave.lvr & I3C_LVR_I2C_INDEX_MASK) {
> > > +		case I3C_LVR_I2C_INDEX(0):
> > > +			if (mode < I3C_BUS_MODE_MIXED_FAST)
> > > +				mode = I3C_BUS_MODE_MIXED_FAST;
> > > +			break;
> > > +		case I3C_LVR_I2C_INDEX(1):
> > > +		case I3C_LVR_I2C_INDEX(2):
> > > +			if (mode < I3C_BUS_MODE_MIXED_SLOW)
> > > +				mode = I3C_BUS_MODE_MIXED_SLOW;
> > > +			break;
> > > +		default:
> > > +			ret = -EINVAL;
> > > +			goto err_put_dev;
> > > +		}
> > > +		if (defslvsinfo->slave.lvr & I3C_LVR_I2C_FM_MODE)
> > > +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
> > > +	}
> > > +
> > > +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
> > > +	if (ret)
> > > +		goto err_put_dev;
> > > +
> > > +	/*
> > > +	 * Now execute the controller specific ->bus_init() routine, which
> > > +	 * might configure its internal logic to match the bus limitations.
> > > +	 */
> > > +	ret = master->ops->bus_init(master);
> > > +	if (ret)
> > > +		goto err_put_dev;
> > > +
> > > +	/*
> > > +	 * The master device should have been instantiated in ->bus_init(),
> > > +	 * complain if this was not the case.
> > > +	 */
> > > +	if (!master->this) {
> > > +		dev_err(&master->dev,
> > > +			"master_set_info() was not called in ->bus_init()\n");
> > > +		ret = -EINVAL;
> > > +		goto err_put_dev;
> > > +	}
> > > +
> > > +	ret = device_add(&master->dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Expose our I3C bus as an I2C adapter so that I2C devices are exposed
> > > +	 * through the I2C subsystem.
> > > +	 */
> > > +	ret = i3c_master_i2c_adapter_init(master);
> > > +	if (ret)
> > > +		goto err_put_dev;
> > > +
> > > +	master->init_done = true;
> > > +
> > > +err_put_dev:
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_sec_master_bus_init);
> > > +
> > >  /**
> > >   * i3c_master_register() - register an I3C master
> > >   * @master: master used to send frames on the bus
> > > @@ -2413,10 +2578,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;
> > > @@ -2430,6 +2591,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > >  	master->secondary = secondary;
> > >  	INIT_LIST_HEAD(&master->boardinfo.i2c);
> > >  	INIT_LIST_HEAD(&master->boardinfo.i3c);
> > > +	INIT_LIST_HEAD(&master->defslvs);
> > >  
> > >  	ret = i3c_bus_init(i3cbus);
> > >  	if (ret)
> > > @@ -2475,6 +2637,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > >  		goto err_put_dev;
> > >  	}
> > >  
> > > +	if (secondary)
> > > +		return 0;
> > > +
> > >  	ret = i3c_master_bus_init(master);
> > >  	if (ret)
> > >  		goto err_put_dev;
> > > @@ -2547,6 +2712,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> > >  	if (!master || !xfers)
> > >  		return -EINVAL;
> > >  
> > > +	if (master->bus.cur_master != master->this) {
> > > +		master->ops->request_mastership(master);
> > > +		return -EBUSY;
> > 
> > I don't like this approach, so you have to re-trigger the operation when
> > this master becomes current master. It is not transparent. Especially,
> > HCI 1.1 describes the process in detail, even  on flow chart and you can 
> > see there that you should block all the transfers/tasks on your side and
> > wait for GETACCMST.
> 
> I forgot to explain that this is part is not fully operational and my 
> intention was to address this in near future.
> To quickly head-up, what I had in my mind when did this was if a device 
> wants to do a xfer and sec master is not current master the framework 
> will request the bus ownership and them pass a EBUSY in case of success 
> or another error in case current master Nacks the MR request.
> 
> > 
> > > +	}
> > > +
> > >  	if (!master->ops->priv_xfers)
> > >  		return -ENOTSUPP;
> > >  
> > > @@ -2638,6 +2808,185 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> > >  	dev->ibi = NULL;
> > >  }
> > >  
> > > +static const char *const i3c_dr_modes[] = {
> > > +	[I3C_DR_MODE_MASTER]		= "master",
> > > +	[I3C_DR_MODE_SEC_MASTER]	= "sec-master",
> > > +	[I3C_DR_MODE_SLAVE]		= "slave",
> > > +};
> > > +
> > > +static enum i3c_dr_mode i3c_get_dr_mode_from_string(const char *str)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = match_string(i3c_dr_modes, ARRAY_SIZE(i3c_dr_modes), str);
> > > +	return (ret < 0) ? I3C_DR_MODE_MASTER : ret;
> > > +}
> > > +
> > > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev)
> > > +{
> > > +	const char *dr_mode;
> > > +	int err;
> > > +
> > > +	err = device_property_read_string(dev, "dr-mode", &dr_mode);
> > > +	if (err < 0)
> > > +		return I3C_DR_MODE_MASTER;
> > > +
> > > +	return i3c_get_dr_mode_from_string(dr_mode);
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_get_dr_mode);
> > > +
> > > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master)
> > > +{
> > > +	int ret;
> > > +
> > > +	i3c_bus_normaluse_lock(&master->bus);
> > > +	ret = master->ops->request_mastership(master);
> > > +	i3c_bus_normaluse_unlock(&master->bus);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_sec_master_request_mastership);
> > > +
> > > +int i3c_master_deliver_mastership(struct i3c_master_controller *master, u8 addr)
> > 
> > I agree, wa can introduce this now. But we decided to postpone it. As
> > you can see, it shouldn't be so hard.
> 
> IMO this is important, I could use directly the CCC but I know that there 
> is other HC that may use a different approach.
> 
> > 
> > > +{
> > > +	struct i3c_dev_desc *dev;
> > > +	int ret;
> > > +
> > > +	i3c_bus_normaluse_lock(&master->bus);
> > > +	i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > > +		if (dev->ibi) {
> > > +			mutex_lock(&dev->ibi_lock);
> > > +			i3c_dev_disable_ibi_locked(dev);
> > > +			mutex_unlock(&dev->ibi_lock);
> > > +		}
> > > +	}
> > > +	i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
> > > +				I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > > +
> > > +	ret = master->ops->deliver_mastership(master, addr);
> > > +	i3c_bus_normaluse_unlock(&master->bus);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_master_deliver_mastership);
> > > +
> > > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > > +			     struct i3c_ccc_dev_desc defslvs)
> > > +{
> > > +	struct i3c_defslvs_info *defslvsinfo;
> > > +	struct device *dev = &master->dev;
> > > +
> > > +	i3c_bus_maintenance_lock(&master->bus);
> > > +	defslvsinfo = devm_kzalloc(dev, sizeof(*defslvsinfo), GFP_KERNEL);
> > > +	if (!defslvsinfo)
> > > +		return -ENOMEM;
> > > +
> > > +	defslvsinfo->slave = defslvs;
> > > +
> > > +	list_add_tail(&defslvsinfo->node, &master->defslvs);
> > 
> > I don't get why can't we call i3c_master_add_i3c_dev_locked when
> > populating the bus.
> 
> I want to avoid the populating bus call back.

Why?

> 
> > You have all the data on your plate (in HC driver)
> > when you are populating it from SEC_DEV_CHAR_TABLE_LOC.
> 
> Yes, I told you that I have a table for that, yet I decide to not use it. 
> My concern is about the HC that doesn't have?
> For me passing this task for the framework is more universal. 
> 

Looks like we can do the same in HCI.
HC driver has to know always the devices on the bus.

> > 
> > I decided to do it similarly, but then Boris suggested to rework it and
> > we use only i3c_master_add_i3c_dev_locked.
> > 
> > > +
> > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(defslvs_populate_i3c_bus);
> > > +
> > > +static void i3c_master_add_new_defslvs(struct i3c_master_controller *master)
> > > +{
> > > +	struct i3c_defslvs_info *defslvsinfo;
> > > +
> > > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > > +		/* TODO: add i2c devices to the bus */
> > > +		if (!defslvsinfo->slave.dyn_addr)
> > > +			continue;
> > > +
> > > +		if (defslvsinfo->slave.dyn_addr == master->this->info.dyn_addr)
> > > +			continue;
> > > +
> > > +		if (!i3c_bus_dev_addr_is_avail(&master->bus,
> > > +					       defslvsinfo->slave.dyn_addr))
> > 
> > We can add those checks also but we also have i3c_master_attach_i3c_dev
> > and i3c_master_get_i3c_addrs which takes care of this everything.
> 
> Yes, but them you will be allocating and free devs unnecessarily.
> 

Can you explain that?

> > 
> > > +			continue;
> > > +
> > > +		i3c_master_add_i3c_dev_locked(master, defslvsinfo->slave.dyn_addr);
> > > +	}
> > > +
> > > +	while (!list_empty(&master->defslvs)) {
> > > +		defslvsinfo = list_first_entry(&master->defslvs,
> > > +					       struct i3c_defslvs_info, node);
> > > +		list_del(&defslvsinfo->node);
> > 
> > I feel like this code is redundant, we have to allocate it, then delete.
> 
> No. you need to clean the list. I may receive another one in the future 
> due a HJ or a dynamic address change.
>

I like more the way when HC driver is updateing that. It makes API
simpler.

> > 
> > > +	}
> > > +}
> > > +
> > > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > > +				     bool secondary)
> > > +{
> > > +	struct i3c_dev_desc *dev;
> > > +	int ret;
> > > +
> > > +	if (master->secondary == secondary)
> > > +		return -EEXIST;
> > > +
> > > +	/* TODO: get the current master information */
> > > +	if (secondary)
> > > +		master->bus.cur_master = NULL;
> > > +	else
> > > +		master->bus.cur_master = master->this;
> > > +
> > > +	if (!master->init_done && !secondary)
> > > +		i3c_sec_master_bus_init(master);
> > > +
> > > +	master->secondary = secondary;
> > > +
> > > +	dev_info(&master->dev, "changing to %s\n",
> > > +		 master->secondary ? "Sec Master" : "Master");
> > > +
> > > +	/* TODO: Analyse the use of maintenan_lock for everything */
> > > +	if (!list_empty(&master->defslvs) && !secondary) {
> > > +		i3c_bus_maintenance_lock(&master->bus);
> > > +		i3c_master_add_new_defslvs(master);
> > > +		i3c_bus_maintenance_unlock(&master->bus);
> > > +
> > > +		i3c_bus_normaluse_lock(&master->bus);
> > > +		i3c_master_register_new_i3c_devs(master);
> > 
> > Take a look also at i3c_master_bus_takeover from my latest patch. BTW.
> > what about I2C devices? We worked on that also, and this is part of the
> > latest patch also. I'm testing it with I2C devices also.
> 
> Please check the comments for that. Anyway you can declare the i2c 
> devices of DT on both sides, what I think it should be a good practice.
> 

This is what we are doing, but we are comparing I2C devices from DEFSLVS
list with those defined in DT. We discussed that already.

> > 
> > > +		i3c_bus_normaluse_unlock(&master->bus);
> > > +	}
> > > +
> > > +	if (!secondary) {
> > > +		i3c_bus_normaluse_lock(&master->bus);
> > > +		i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > > +			if (dev->ibi) {
> > > +				mutex_lock(&dev->ibi_lock);
> > > +				ret = i3c_dev_enable_ibi_locked(dev);
> > > +				if (ret)
> > > +					dev_err(&master->dev,
> > > +						"Failed to re-enable IBI on device %d-%llx",
> > > +						master->bus.id, dev->info.pid);
> > > +				mutex_unlock(&dev->ibi_lock);
> > > +				}
> > > +		}
> > > +
> > > +		/* TODO: Enable MR only for the elegible devices */
> > 
> > This was postponed also, but we had that before. We can add per-device
> > granularity to i3c_master_bus_takeover().
> 
> Here we need to do the same as for ibi and maybe get the eligible devices 
> from DT or based on its DCR,BCR and PID.
> This is something that should be address because it represent a big 
> security gap.
> 
> > 
> > > +		i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
> > > +					I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > > +		i3c_bus_normaluse_unlock(&master->bus);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
> > > +
> > > +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
> > > +{
> > > +	int res;
> > > +
> > > +	mutex_lock(&i3c_core_lock);
> > > +	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
> > > +	mutex_unlock(&i3c_core_lock);
> > > +
> > > +	return res;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_for_each_dev);
> > > +
> > >  static int __init i3c_init(void)
> > >  {
> > >  	return bus_register(&i3c_bus_type);
> > > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > > index 9cb39d9..09bd99c 100644
> > > --- a/include/linux/i3c/master.h
> > > +++ b/include/linux/i3c/master.h
> > > @@ -426,6 +426,8 @@ struct i3c_bus {
> > >   *		      for a future IBI
> > >   *		      This method is mandatory only if ->request_ibi is not
> > >   *		      NULL.
> > > + * @request_mastership: Request mastership.
> > > + * @deliver_mastership: Deliver mastership
> > >   */
> > >  struct i3c_master_controller_ops {
> > >  	int (*bus_init)(struct i3c_master_controller *master);
> > > @@ -452,6 +454,21 @@ 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);
> > > +	int (*request_mastership)(struct i3c_master_controller *master);
> > > +	int (*deliver_mastership)(struct i3c_master_controller *master,
> > > +				  u8 addr);
> > > +};
> > > +
> > > +/**
> > > + * struct i3c_defslvs_info - defslvs information object
> > > + * @node: used to insert the defslvs object in the  list
> > > + * @slave: I3C/I2C device descriptor used for DEFSLVS
> > > + *
> > > + * This structure is used to hold defslvs information on Secondary Master.
> > > + */
> > > +struct i3c_defslvs_info {
> > > +	struct list_head node;
> > > +	struct i3c_ccc_dev_desc slave;
> > >  };
> > >  
> > >  /**
> > > @@ -468,6 +485,7 @@ struct i3c_master_controller_ops {
> > >   * @boardinfo.i3c: list of I3C  boardinfo objects
> > >   * @boardinfo.i2c: list of I2C boardinfo objects
> > >   * @boardinfo: board-level information attached to devices connected on the bus
> > > + * @defslvs: List of defslvs objects
> > >   * @bus: I3C bus exposed by this master
> > >   * @wq: workqueue used to execute IBI handlers. Can also be used by master
> > >   *	drivers if they need to postpone operations that need to take place
> > > @@ -491,6 +509,7 @@ struct i3c_master_controller {
> > >  		struct list_head i3c;
> > >  		struct list_head i2c;
> > >  	} boardinfo;
> > > +	struct list_head defslvs;
> > >  	struct i3c_bus bus;
> > >  	struct workqueue_struct *wq;
> > >  };
> > > @@ -525,6 +544,7 @@ int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr,
> > >  			    u8 evts);
> > >  int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> > >  			   u8 evts);
> > > +int i3c_master_getaccmst_locked(struct i3c_master_controller *master, u8 addr);
> > >  int i3c_master_entdaa_locked(struct i3c_master_controller *master);
> > >  int i3c_master_defslvs_locked(struct i3c_master_controller *master);
> > >  
> > > @@ -652,4 +672,23 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
> > >  
> > >  struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
> > >  
> > > +enum i3c_dr_mode {
> > > +	I3C_DR_MODE_MASTER,
> > > +	I3C_DR_MODE_SEC_MASTER,
> > > +	I3C_DR_MODE_SLAVE,
> > > +};
> > > +
> > > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev);
> > > +
> > > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > > +				     bool secondary);
> > > +
> > > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master);
> > > +int i3c_master_deliver_mastership(struct i3c_master_controller *master,
> > > +				  u8 addr);
> > > +
> > > +int i3c_sec_master_bus_init(struct i3c_master_controller *master);
> > > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > > +			     struct i3c_ccc_dev_desc defslvs);
> > > +
> > >  #endif /* I3C_MASTER_H */
> > > -- 
> > > 2.7.4
> > > 
> > 
> > I feel like this is based on early approach which has evolved by the time
> > and I think some of the nice part are missing. The biggest parts to
> > discuass are:
> > - bus population using devslvs device list instead of using
> >   i3c_master_add_i3c_dev locked, have you tried that? If something does
> >   not work for you, wa can adjust it but I really want you to try the
> >   new approach
> 
> To me the i3c_master_add_i3c_dev_locked just need an improvement to 
> propagate the boardinfo to all devices, something that already I started, 
> and we need to discuss if on secondary master side we can change the 
> devices addresses which imply to send DEFSLVS at the end of the process.

So let's improve it. Can you point me to that?

> 
> > 
> > - the way how we are requesting mastership, IMO, we should wait untill
> >   the process is finished, as also described in HCI spec for example.
> 
> I think you are confusing the concept here. I understand you want to do 
> the xfer straight way after receive GETACCMST and I agree with that.
> The thing is that you don't know what will happen on the bus between the 
> time you send the request and you get the bus ownership. I just think we 
> need to find another solution for that like defer the transfer to another 
> time and when the controller switch the rule trigger all transfers in the 
> pipeline (something like this) even before restore all ibi. If you block 
> the bus you are not able to pass the defslvs to the framework.  
> 

If the MR request was acked, we know that we'll get the control. And it
doesn't matter if current master will enable/disable something, change
the activity state, send another DEFSLVS, we still want to make a
transfer. 

In your approach it is possible that we get the control, we'll
not make the transfer and give control back to a diffent master. 
I have tests for more than 2 masters.

And also, I don't feel like queueing the transfers is good idea. When
some upper layer wants to transfer the data again, old data may be stale
but we still will transfer that.

> >   When I introduce interrupt-based solution, everything should be fine.
> >   Could you do that also in your driver?
> 
> I did it but had issues and didn't fit in my use case. That the reason I 
> did this approach.

Can you describe what issues you faced with this approach? What is your
use case? If we get the control, we just make the transfers, if not,
return exit code to upper layer and that's it.

> 
> > 
> > Again, good to have that. I'm really counting on a fair discussion :-)
> > 
> > -- 
> > --
> > Regards, 
> > Przemyslaw Gaj
> 
> As you can see this is based on your work. Off course it as some lose 
> ends that should be addressed (like i2c devices, bus mode...) but don't 
> have big impact in what I want to show.

Yes, but on my work in early stage and I think this approach wasn't
perfect. Then we decided to refork some parts which is not part of that.
I feel like we can't find common view.

> For me it is important to rely on framework the way how bus ownership 
> exchange is made because it will be easier to maintain in long term and 
> we can introduce algorithms for the bus access in the future.
> 

Yes, why not. My latest patchset does not break anything here.

> If you are using i3c-tree/next you can apply this directly and if you 
> find anything else that isn't going ok please let me know.
> 
> Best regards,
> Vitor Soares

-- 
-- 
Regards,
Przemyslaw Gaj

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

RE: [RFC 1/2] i3c: Add preliminary support for secondary master

[Vitor Soares]

Hi,

From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Thu, Nov 28, 2019 at 12:58:06

> The 11/28/2019 12:20, Vitor Soares wrote:
> > 
> > Hi,
> > 
> > 
> > From: Przemyslaw Gaj <pgaj@cadence.com>
> > Date: Thu, Nov 28, 2019 at 05:50:08
> > 
> > > Hi Vitor,
> > > 
> > > First, you woke up my son and he couldn't sleep the rest of the night
> > > :-)
> > 
> > Sorry for that.
> > 
> > > I appreciate you sent that so we can discuss it.
> > > 
> > > The 11/28/2019 02:15, Vitor Soares wrote:
> > > > 
> > > > This patch adds the preliminary support for secondary master feature to
> > > > i3c Framework for testing purposes.
> > > > 
> > > > Key points for consideration:
> > > >   - mastership_[show/store] are only used for testing
> > > >   - secondary master registration is made in two steps, one in
> > > >   i3c_master_register() and another in i3c_sec_master_bus_init() when
> > > >   secondary master became current master first time. This is made in this
> > > >   way to get all dt declared boardinfo list, create defslvs list and
> > > >   provide work_queue.
> > > >   - When the current master wants to deliver_mastership it necessary to
> > > >   disable all in-band events to avoid unwanted interrupt during bus
> > > >   ownership exchange. For now this patch doesn't reflect all
> > > >   improvements/changes made in v1.1 I3C Bus spec. But it can be extended
> > > >   by adding some commands and checks to the flow.
> > > >   - i3c_defslvs_info: The DEFSLVS info can be differently stored in
> > > >   diferen HC. Hence it is used a defslvs list similar to boardinfo list in
> > > >   the bus structure to hold this data. Them HC is taccking over the bus
> > > >   ownership can initialize each device of that list. For now, this not
> > > >   address the i2c devices since they are only statically described.
> > > >   - [request/deliver]_mastership(): Mastership request deliver may be done
> > > >   differently in different HC, hence the need to have a call back for each
> > > >   process.
> > > >   - Add dr_mode to DT: Similar to USB, HC can be programmed to Master only
> > > >   mode, Slave only mode or Secondary Master which aren't necessarily
> > > >   hardcoded.
> > > >   - bus_mode definition: The bus mode is defined even without defslvs
> > > >   information with DT info since the definition of i2c devices are those
> > > >   that have impact on bus_mode definition and need to statically declared.
> > > >   The only use case that may cause issues is when i2c devices aren't
> > > >   declared in secondary master side and bus mode doesn't match the
> > > >   main master. Anyway this can be solde without extra complexity.
> > > > 
> > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > > ---
> > > >  drivers/i3c/master.c       | 365 ++++++++++++++++++++++++++++++++++++++++++++-
> > > >  include/linux/i3c/master.h |  39 +++++
> > > >  2 files changed, 396 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > index 0436916..b398d77 100644
> > > > --- a/drivers/i3c/master.c
> > > > +++ b/drivers/i3c/master.c
> > > > @@ -449,6 +449,46 @@ static ssize_t mode_show(struct device *dev,
> > > >  }
> > > >  static DEVICE_ATTR_RO(mode);
> > > >  
> > > > +static ssize_t
> > > > +mastership_show(struct device *dev, struct device_attribute *da, char *buf)
> > > > +{
> > > > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > > > +	ssize_t ret;
> > > > +
> > > > +	if (master->secondary)
> > > > +		ret = sprintf(buf, "Secondary Master\n");
> > > > +	else
> > > > +		ret = sprintf(buf, "Master\n");
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static ssize_t
> > > > +mastership_store(struct device *dev, struct device_attribute *attr,
> > > > +		 const char *buf, size_t count)
> > > > +{
> > > > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > > > +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> > > > +
> > > > +	if (i3cbus->cur_master == master->this) {
> > > > +		dev_err(dev, "I'm current mater.");
> > > > +		return count;
> > > > +	}
> > > > +
> > > > +	if (!master->ops->request_mastership) {
> > > > +		dev_err(dev, "mastership_request not supported.");
> > > > +		return count;
> > > > +	}
> > > > +
> > > > +	if (master->ops->request_mastership(master))
> > > > +		dev_err(dev, "mastership_request failed");
> > > > +	else
> > > > +		dev_err(dev, "mastership_request success");
> > > > +
> > > > +	return count;
> > > > +}
> > > > +static DEVICE_ATTR_RW(mastership);
> > > > +
> > > >  static ssize_t current_master_show(struct device *dev,
> > > >  				   struct device_attribute *da,
> > > >  				   char *buf)
> > > > @@ -457,8 +497,11 @@ static ssize_t current_master_show(struct device *dev,
> > > >  	ssize_t ret;
> > > >  
> > > >  	i3c_bus_normaluse_lock(i3cbus);
> > > > -	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > > -		      i3cbus->cur_master->info.pid);
> > > > +	if (i3cbus->cur_master)
> > > > +		ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > > +			      i3cbus->cur_master->info.pid);
> > > > +	else
> > > > +		ret = sprintf(buf, "Not Current Master\n");
> > > >  	i3c_bus_normaluse_unlock(i3cbus);
> > > >  
> > > >  	return ret;
> > > > @@ -497,6 +540,7 @@ static DEVICE_ATTR_RO(i2c_scl_frequency);
> > > >  
> > > >  static struct attribute *i3c_masterdev_attrs[] = {
> > > >  	&dev_attr_mode.attr,
> > > > +	&dev_attr_mastership.attr,
> > > >  	&dev_attr_current_master.attr,
> > > >  	&dev_attr_i3c_scl_frequency.attr,
> > > >  	&dev_attr_i2c_scl_frequency.attr,
> > > > @@ -854,6 +898,53 @@ int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> > > >  EXPORT_SYMBOL_GPL(i3c_master_enec_locked);
> > > >  
> > > >  /**
> > > > + * i3c_master_getaccmst_locked() - send an GETACCMST CCC command
> > > > + * @master: master used to send frames on the bus
> > > > + * @addr: a valid I3C slave address
> > > > + *
> > > > + * Sends an GETACCMST CCC command to offer bus Mastership to an
> > > > + * I3C Secondary Master.
> > > > + *
> > > > + * 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_getaccmst_locked(struct i3c_master_controller *master, u8 addr)
> > > > +{
> > > > +	enum i3c_addr_slot_status addrstat;
> > > > +	struct i3c_ccc_getaccmst *accmst;
> > > > +	struct i3c_ccc_cmd_dest dest;
> > > > +	struct i3c_ccc_cmd cmd;
> > > > +	int ret;
> > > > +
> > > > +	if (!master)
> > > > +		return -EINVAL;
> > > > +
> > > > +	addrstat = i3c_bus_get_addr_slot_status(&master->bus, addr);
> > > > +	if (addr == I3C_BROADCAST_ADDR || addrstat != I3C_ADDR_SLOT_I3C_DEV)
> > > > +		return -EINVAL;
> > > > +
> > > > +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> > > > +	if (!accmst)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> > > > +
> > > > +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	if (accmst->newmaster >> 1 != addr)
> > > 
> > > I really like this check. This is something I realized working
> > > on next patch version.
> > > 
> > > > +		ret = -EIO;
> > > > +out:
> > > > +	i3c_ccc_cmd_dest_cleanup(&dest);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_master_getaccmst_locked);
> > > > +
> > > > +/**
> > > >   * i3c_master_defslvs_locked() - send a DEFSLVS CCC command
> > > >   * @master: master used to send frames on the bus
> > > >   *
> > > > @@ -1542,8 +1633,7 @@ int i3c_master_set_info(struct i3c_master_controller *master,
> > > >  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
> > > >  		return -EINVAL;
> > > >  
> > > > -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> > > > -	    master->secondary)
> > > > +	if (I3C_BCR_DEVICE_ROLE(info->bcr) != I3C_BCR_I3C_MASTER)
> > > >  		return -EINVAL;
> > > >  
> > > >  	if (master->this)
> > > > @@ -2381,6 +2471,81 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +int i3c_sec_master_bus_init(struct i3c_master_controller *master)
> > > > +{
> > > > +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > +	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> > > > +	enum i3c_bus_mode mode = i3cbus->mode;
> > > > +	struct i3c_defslvs_info *defslvsinfo;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (master->init_done)
> > > > +		return -EINVAL;
> > > > +
> > > > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > > > +		if (defslvsinfo->slave.dyn_addr)
> > > > +			continue;
> > > > +
> > > > +		switch (defslvsinfo->slave.lvr & I3C_LVR_I2C_INDEX_MASK) {
> > > > +		case I3C_LVR_I2C_INDEX(0):
> > > > +			if (mode < I3C_BUS_MODE_MIXED_FAST)
> > > > +				mode = I3C_BUS_MODE_MIXED_FAST;
> > > > +			break;
> > > > +		case I3C_LVR_I2C_INDEX(1):
> > > > +		case I3C_LVR_I2C_INDEX(2):
> > > > +			if (mode < I3C_BUS_MODE_MIXED_SLOW)
> > > > +				mode = I3C_BUS_MODE_MIXED_SLOW;
> > > > +			break;
> > > > +		default:
> > > > +			ret = -EINVAL;
> > > > +			goto err_put_dev;
> > > > +		}
> > > > +		if (defslvsinfo->slave.lvr & I3C_LVR_I2C_FM_MODE)
> > > > +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
> > > > +	}
> > > > +
> > > > +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
> > > > +	if (ret)
> > > > +		goto err_put_dev;
> > > > +
> > > > +	/*
> > > > +	 * Now execute the controller specific ->bus_init() routine, which
> > > > +	 * might configure its internal logic to match the bus limitations.
> > > > +	 */
> > > > +	ret = master->ops->bus_init(master);
> > > > +	if (ret)
> > > > +		goto err_put_dev;
> > > > +
> > > > +	/*
> > > > +	 * The master device should have been instantiated in ->bus_init(),
> > > > +	 * complain if this was not the case.
> > > > +	 */
> > > > +	if (!master->this) {
> > > > +		dev_err(&master->dev,
> > > > +			"master_set_info() was not called in ->bus_init()\n");
> > > > +		ret = -EINVAL;
> > > > +		goto err_put_dev;
> > > > +	}
> > > > +
> > > > +	ret = device_add(&master->dev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Expose our I3C bus as an I2C adapter so that I2C devices are exposed
> > > > +	 * through the I2C subsystem.
> > > > +	 */
> > > > +	ret = i3c_master_i2c_adapter_init(master);
> > > > +	if (ret)
> > > > +		goto err_put_dev;
> > > > +
> > > > +	master->init_done = true;
> > > > +
> > > > +err_put_dev:
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_sec_master_bus_init);
> > > > +
> > > >  /**
> > > >   * i3c_master_register() - register an I3C master
> > > >   * @master: master used to send frames on the bus
> > > > @@ -2413,10 +2578,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;
> > > > @@ -2430,6 +2591,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > >  	master->secondary = secondary;
> > > >  	INIT_LIST_HEAD(&master->boardinfo.i2c);
> > > >  	INIT_LIST_HEAD(&master->boardinfo.i3c);
> > > > +	INIT_LIST_HEAD(&master->defslvs);
> > > >  
> > > >  	ret = i3c_bus_init(i3cbus);
> > > >  	if (ret)
> > > > @@ -2475,6 +2637,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > >  		goto err_put_dev;
> > > >  	}
> > > >  
> > > > +	if (secondary)
> > > > +		return 0;
> > > > +
> > > >  	ret = i3c_master_bus_init(master);
> > > >  	if (ret)
> > > >  		goto err_put_dev;
> > > > @@ -2547,6 +2712,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> > > >  	if (!master || !xfers)
> > > >  		return -EINVAL;
> > > >  
> > > > +	if (master->bus.cur_master != master->this) {
> > > > +		master->ops->request_mastership(master);
> > > > +		return -EBUSY;
> > > 
> > > I don't like this approach, so you have to re-trigger the operation when
> > > this master becomes current master. It is not transparent. Especially,
> > > HCI 1.1 describes the process in detail, even  on flow chart and you can 
> > > see there that you should block all the transfers/tasks on your side and
> > > wait for GETACCMST.
> > 
> > I forgot to explain that this is part is not fully operational and my 
> > intention was to address this in near future.
> > To quickly head-up, what I had in my mind when did this was if a device 
> > wants to do a xfer and sec master is not current master the framework 
> > will request the bus ownership and them pass a EBUSY in case of success 
> > or another error in case current master Nacks the MR request.
> > 
> > > 
> > > > +	}
> > > > +
> > > >  	if (!master->ops->priv_xfers)
> > > >  		return -ENOTSUPP;
> > > >  
> > > > @@ -2638,6 +2808,185 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> > > >  	dev->ibi = NULL;
> > > >  }
> > > >  
> > > > +static const char *const i3c_dr_modes[] = {
> > > > +	[I3C_DR_MODE_MASTER]		= "master",
> > > > +	[I3C_DR_MODE_SEC_MASTER]	= "sec-master",
> > > > +	[I3C_DR_MODE_SLAVE]		= "slave",
> > > > +};
> > > > +
> > > > +static enum i3c_dr_mode i3c_get_dr_mode_from_string(const char *str)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = match_string(i3c_dr_modes, ARRAY_SIZE(i3c_dr_modes), str);
> > > > +	return (ret < 0) ? I3C_DR_MODE_MASTER : ret;
> > > > +}
> > > > +
> > > > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev)
> > > > +{
> > > > +	const char *dr_mode;
> > > > +	int err;
> > > > +
> > > > +	err = device_property_read_string(dev, "dr-mode", &dr_mode);
> > > > +	if (err < 0)
> > > > +		return I3C_DR_MODE_MASTER;
> > > > +
> > > > +	return i3c_get_dr_mode_from_string(dr_mode);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_get_dr_mode);
> > > > +
> > > > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	i3c_bus_normaluse_lock(&master->bus);
> > > > +	ret = master->ops->request_mastership(master);
> > > > +	i3c_bus_normaluse_unlock(&master->bus);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_sec_master_request_mastership);
> > > > +
> > > > +int i3c_master_deliver_mastership(struct i3c_master_controller *master, u8 addr)
> > > 
> > > I agree, wa can introduce this now. But we decided to postpone it. As
> > > you can see, it shouldn't be so hard.
> > 
> > IMO this is important, I could use directly the CCC but I know that there 
> > is other HC that may use a different approach.
> > 
> > > 
> > > > +{
> > > > +	struct i3c_dev_desc *dev;
> > > > +	int ret;
> > > > +
> > > > +	i3c_bus_normaluse_lock(&master->bus);
> > > > +	i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > > > +		if (dev->ibi) {
> > > > +			mutex_lock(&dev->ibi_lock);
> > > > +			i3c_dev_disable_ibi_locked(dev);
> > > > +			mutex_unlock(&dev->ibi_lock);
> > > > +		}
> > > > +	}
> > > > +	i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
> > > > +				I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > > > +
> > > > +	ret = master->ops->deliver_mastership(master, addr);
> > > > +	i3c_bus_normaluse_unlock(&master->bus);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_master_deliver_mastership);
> > > > +
> > > > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > > > +			     struct i3c_ccc_dev_desc defslvs)
> > > > +{
> > > > +	struct i3c_defslvs_info *defslvsinfo;
> > > > +	struct device *dev = &master->dev;
> > > > +
> > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > +	defslvsinfo = devm_kzalloc(dev, sizeof(*defslvsinfo), GFP_KERNEL);
> > > > +	if (!defslvsinfo)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	defslvsinfo->slave = defslvs;
> > > > +
> > > > +	list_add_tail(&defslvsinfo->node, &master->defslvs);
> > > 
> > > I don't get why can't we call i3c_master_add_i3c_dev_locked when
> > > populating the bus.
> > 
> > I want to avoid the populating bus call back.
> 
> Why?

When I developed this procedure the idea behind it was to avoid bus 
callbacks and offload this complexity to framework because we don't know 
how others HC handle DEFSLVS CCC, lets image the case where the DEFSLVS 
are stored in a place that can be rewriter  before sec master get bus 
ownership.
Another advantage that I see is, when someone write a new HC driver won't 
need to care about this because the framework already provide this 
facility.

> 
> > 
> > > You have all the data on your plate (in HC driver)
> > > when you are populating it from SEC_DEV_CHAR_TABLE_LOC.
> > 
> > Yes, I told you that I have a table for that, yet I decide to not use it. 
> > My concern is about the HC that doesn't have?
> > For me passing this task for the framework is more universal. 
> > 
> 
> Looks like we can do the same in HCI.
> HC driver has to know always the devices on the bus.

My view here is more like the HC driver provide a way of the framework 
interact with bus.

> 
> > > 
> > > I decided to do it similarly, but then Boris suggested to rework it and
> > > we use only i3c_master_add_i3c_dev_locked.
> > > 
> > > > +
> > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(defslvs_populate_i3c_bus);
> > > > +
> > > > +static void i3c_master_add_new_defslvs(struct i3c_master_controller *master)
> > > > +{
> > > > +	struct i3c_defslvs_info *defslvsinfo;
> > > > +
> > > > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > > > +		/* TODO: add i2c devices to the bus */
> > > > +		if (!defslvsinfo->slave.dyn_addr)
> > > > +			continue;
> > > > +
> > > > +		if (defslvsinfo->slave.dyn_addr == master->this->info.dyn_addr)
> > > > +			continue;
> > > > +
> > > > +		if (!i3c_bus_dev_addr_is_avail(&master->bus,
> > > > +					       defslvsinfo->slave.dyn_addr))
> > > 
> > > We can add those checks also but we also have i3c_master_attach_i3c_dev
> > > and i3c_master_get_i3c_addrs which takes care of this everything.
> > 
> > Yes, but them you will be allocating and free devs unnecessarily.
> > 
> 
> Can you explain that?

Yap. In i3c_master_add_i3c_dev_locked we 1st allocate the dev, then 
attach the device and if it fails we free the dev.
I think here we can check in upfront if the address is already in use and 
avoid extra steps.

> 
> > > 
> > > > +			continue;
> > > > +
> > > > +		i3c_master_add_i3c_dev_locked(master, defslvsinfo->slave.dyn_addr);
> > > > +	}
> > > > +
> > > > +	while (!list_empty(&master->defslvs)) {
> > > > +		defslvsinfo = list_first_entry(&master->defslvs,
> > > > +					       struct i3c_defslvs_info, node);
> > > > +		list_del(&defslvsinfo->node);
> > > 
> > > I feel like this code is redundant, we have to allocate it, then delete.
> > 
> > No. you need to clean the list. I may receive another one in the future 
> > due a HJ or a dynamic address change.
> >

In my previous comment I forgot to mention by doing the cleanup in API we 
avoid duplicated code in HC drivers.

> 
> I like more the way when HC driver is updateing that. It makes API
> simpler.

There is nothing against that 😉. In the end, the purpose of this DEFSLVS 
handling was to present an alternative solution and its advantages.

> 
> > > 
> > > > +	}
> > > > +}
> > > > +
> > > > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > > > +				     bool secondary)
> > > > +{
> > > > +	struct i3c_dev_desc *dev;
> > > > +	int ret;
> > > > +
> > > > +	if (master->secondary == secondary)
> > > > +		return -EEXIST;
> > > > +
> > > > +	/* TODO: get the current master information */
> > > > +	if (secondary)
> > > > +		master->bus.cur_master = NULL;
> > > > +	else
> > > > +		master->bus.cur_master = master->this;
> > > > +
> > > > +	if (!master->init_done && !secondary)
> > > > +		i3c_sec_master_bus_init(master);
> > > > +
> > > > +	master->secondary = secondary;
> > > > +
> > > > +	dev_info(&master->dev, "changing to %s\n",
> > > > +		 master->secondary ? "Sec Master" : "Master");
> > > > +
> > > > +	/* TODO: Analyse the use of maintenan_lock for everything */
> > > > +	if (!list_empty(&master->defslvs) && !secondary) {
> > > > +		i3c_bus_maintenance_lock(&master->bus);
> > > > +		i3c_master_add_new_defslvs(master);
> > > > +		i3c_bus_maintenance_unlock(&master->bus);
> > > > +
> > > > +		i3c_bus_normaluse_lock(&master->bus);
> > > > +		i3c_master_register_new_i3c_devs(master);
> > > 
> > > Take a look also at i3c_master_bus_takeover from my latest patch. BTW.
> > > what about I2C devices? We worked on that also, and this is part of the
> > > latest patch also. I'm testing it with I2C devices also.
> > 
> > Please check the comments for that. Anyway you can declare the i2c 
> > devices of DT on both sides, what I think it should be a good practice.
> > 
> 
> This is what we are doing, but we are comparing I2C devices from DEFSLVS
> list with those defined in DT. We discussed that already.

You are right.

> 
> > > 
> > > > +		i3c_bus_normaluse_unlock(&master->bus);
> > > > +	}
> > > > +
> > > > +	if (!secondary) {
> > > > +		i3c_bus_normaluse_lock(&master->bus);
> > > > +		i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > > > +			if (dev->ibi) {
> > > > +				mutex_lock(&dev->ibi_lock);
> > > > +				ret = i3c_dev_enable_ibi_locked(dev);
> > > > +				if (ret)
> > > > +					dev_err(&master->dev,
> > > > +						"Failed to re-enable IBI on device %d-%llx",
> > > > +						master->bus.id, dev->info.pid);
> > > > +				mutex_unlock(&dev->ibi_lock);
> > > > +				}
> > > > +		}
> > > > +
> > > > +		/* TODO: Enable MR only for the elegible devices */
> > > 
> > > This was postponed also, but we had that before. We can add per-device
> > > granularity to i3c_master_bus_takeover().
> > 
> > Here we need to do the same as for ibi and maybe get the eligible devices 
> > from DT or based on its DCR,BCR and PID.
> > This is something that should be address because it represent a big 
> > security gap.
> > 
> > > 
> > > > +		i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
> > > > +					I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > > > +		i3c_bus_normaluse_unlock(&master->bus);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
> > > > +
> > > > +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
> > > > +{
> > > > +	int res;
> > > > +
> > > > +	mutex_lock(&i3c_core_lock);
> > > > +	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
> > > > +	mutex_unlock(&i3c_core_lock);
> > > > +
> > > > +	return res;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(i3c_for_each_dev);
> > > > +
> > > >  static int __init i3c_init(void)
> > > >  {
> > > >  	return bus_register(&i3c_bus_type);
> > > > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > > > index 9cb39d9..09bd99c 100644
> > > > --- a/include/linux/i3c/master.h
> > > > +++ b/include/linux/i3c/master.h
> > > > @@ -426,6 +426,8 @@ struct i3c_bus {
> > > >   *		      for a future IBI
> > > >   *		      This method is mandatory only if ->request_ibi is not
> > > >   *		      NULL.
> > > > + * @request_mastership: Request mastership.
> > > > + * @deliver_mastership: Deliver mastership
> > > >   */
> > > >  struct i3c_master_controller_ops {
> > > >  	int (*bus_init)(struct i3c_master_controller *master);
> > > > @@ -452,6 +454,21 @@ 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);
> > > > +	int (*request_mastership)(struct i3c_master_controller *master);
> > > > +	int (*deliver_mastership)(struct i3c_master_controller *master,
> > > > +				  u8 addr);
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct i3c_defslvs_info - defslvs information object
> > > > + * @node: used to insert the defslvs object in the  list
> > > > + * @slave: I3C/I2C device descriptor used for DEFSLVS
> > > > + *
> > > > + * This structure is used to hold defslvs information on Secondary Master.
> > > > + */
> > > > +struct i3c_defslvs_info {
> > > > +	struct list_head node;
> > > > +	struct i3c_ccc_dev_desc slave;
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -468,6 +485,7 @@ struct i3c_master_controller_ops {
> > > >   * @boardinfo.i3c: list of I3C  boardinfo objects
> > > >   * @boardinfo.i2c: list of I2C boardinfo objects
> > > >   * @boardinfo: board-level information attached to devices connected on the bus
> > > > + * @defslvs: List of defslvs objects
> > > >   * @bus: I3C bus exposed by this master
> > > >   * @wq: workqueue used to execute IBI handlers. Can also be used by master
> > > >   *	drivers if they need to postpone operations that need to take place
> > > > @@ -491,6 +509,7 @@ struct i3c_master_controller {
> > > >  		struct list_head i3c;
> > > >  		struct list_head i2c;
> > > >  	} boardinfo;
> > > > +	struct list_head defslvs;
> > > >  	struct i3c_bus bus;
> > > >  	struct workqueue_struct *wq;
> > > >  };
> > > > @@ -525,6 +544,7 @@ int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr,
> > > >  			    u8 evts);
> > > >  int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> > > >  			   u8 evts);
> > > > +int i3c_master_getaccmst_locked(struct i3c_master_controller *master, u8 addr);
> > > >  int i3c_master_entdaa_locked(struct i3c_master_controller *master);
> > > >  int i3c_master_defslvs_locked(struct i3c_master_controller *master);
> > > >  
> > > > @@ -652,4 +672,23 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
> > > >  
> > > >  struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
> > > >  
> > > > +enum i3c_dr_mode {
> > > > +	I3C_DR_MODE_MASTER,
> > > > +	I3C_DR_MODE_SEC_MASTER,
> > > > +	I3C_DR_MODE_SLAVE,
> > > > +};
> > > > +
> > > > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev);
> > > > +
> > > > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > > > +				     bool secondary);
> > > > +
> > > > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master);
> > > > +int i3c_master_deliver_mastership(struct i3c_master_controller *master,
> > > > +				  u8 addr);
> > > > +
> > > > +int i3c_sec_master_bus_init(struct i3c_master_controller *master);
> > > > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > > > +			     struct i3c_ccc_dev_desc defslvs);
> > > > +
> > > >  #endif /* I3C_MASTER_H */
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > I feel like this is based on early approach which has evolved by the time
> > > and I think some of the nice part are missing. The biggest parts to
> > > discuass are:
> > > - bus population using devslvs device list instead of using
> > >   i3c_master_add_i3c_dev locked, have you tried that? If something does
> > >   not work for you, wa can adjust it but I really want you to try the
> > >   new approach
> > 
> > To me the i3c_master_add_i3c_dev_locked just need an improvement to 
> > propagate the boardinfo to all devices, something that already I started, 
> > and we need to discuss if on secondary master side we can change the 
> > devices addresses which imply to send DEFSLVS at the end of the process.
> 
> So let's improve it. Can you point me to that?

Regarding the improvements you can check the patches that touch in the 
API in this series [1].
During the development of v3 I started to thinking it is a good idea to 
pre-reserve all declared addresses in DT and not use them during the 
ENTDAA process. We can discuss this when I send the new version.


Regarding the secondary master, do you think it is a good idea to let the 
secondary master change the dynamic address of a device assigned by the 
main master?
For me we might need this kind of algorithms thing to define the 
secondary master behavior.

> 
> > 
> > > 
> > > - the way how we are requesting mastership, IMO, we should wait untill
> > >   the process is finished, as also described in HCI spec for example.
> > 
> > I think you are confusing the concept here. I understand you want to do 
> > the xfer straight way after receive GETACCMST and I agree with that.
> > The thing is that you don't know what will happen on the bus between the 
> > time you send the request and you get the bus ownership. I just think we 
> > need to find another solution for that like defer the transfer to another 
> > time and when the controller switch the rule trigger all transfers in the 
> > pipeline (something like this) even before restore all ibi. If you block 
> > the bus you are not able to pass the defslvs to the framework.  
> > 
> 
> If the MR request was acked, we know that we'll get the control. And it
> doesn't matter if current master will enable/disable something, change
> the activity state, send another DEFSLVS, we still want to make a
> transfer. 
> 
> In your approach it is possible that we get the control, we'll
> not make the transfer and give control back to a diffent master. 
> I have tests for more than 2 masters.

You are right.
This case I didn't address it yet. I have a upper layer that give bus 
access time to each master for now.

In your use case if you don't need the IBI in the secondary master side, 
I would say the best is to request mastership, do the transfer, and send 
back the mastership, without the logistics enable/disable IBI in 
secondary master.

> 
> And also, I don't feel like queueing the transfers is good idea. When
> some upper layer wants to transfer the data again, old data may be stale
> but we still will transfer that.

That might be a problem for both approach because the exchange of 
ownership can take so long that xfer data might not be valid anymore.
IMO it is also dangerous to have the timeout of the bus ownership 
exchange in HC driver because it will depend from use case to use case 
and it is not something predictable.

> 
> > >   When I introduce interrupt-based solution, everything should be fine.
> > >   Could you do that also in your driver?
> > 
> > I did it but had issues and didn't fit in my use case. That the reason I 
> > did this approach.
> 
> Can you describe what issues you faced with this approach? What is your
> use case? If we get the control, we just make the transfers, if not,
> return exit code to upper layer and that's it.

The way I have the code that is not possible. In fact it is something 
that I need help to let the xfer continue after sec master takeover the 
bus. 

> 
> > 
> > > 
> > > Again, good to have that. I'm really counting on a fair discussion :-)
> > > 
> > > -- 
> > > --
> > > Regards, 
> > > Przemyslaw Gaj
> > 
> > As you can see this is based on your work. Off course it as some lose 
> > ends that should be addressed (like i2c devices, bus mode...) but don't 
> > have big impact in what I want to show.
> 
> Yes, but on my work in early stage and I think this approach wasn't
> perfect. Then we decided to refork some parts which is not part of that.
> I feel like we can't find common view.
> 
> > For me it is important to rely on framework the way how bus ownership 
> > exchange is made because it will be easier to maintain in long term and 
> > we can introduce algorithms for the bus access in the future.
> > 
> 
> Yes, why not. My latest patchset does not break anything here.

It already passed few months since them, but I remember to have issues 
during the master/secondary registration.

> 
> > If you are using i3c-tree/next you can apply this directly and if you 
> > find anything else that isn't going ok please let me know.
> > 
> > Best regards,
> > Vitor Soares
> 
> -- 
> -- 
> Regards,
> Przemyslaw Gaj

Best regards,
Vitor Soares

[1] https://patchwork.kernel.org/patch/11127661/

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 1/2] i3c: Add preliminary support for secondary master

[Przemyslaw Gaj]

Hi Vitor,

The 11/28/2019 15:50, Vitor Soares wrote:
> 
> Hi,
> 
> From: Przemyslaw Gaj <pgaj@cadence.com>
> Date: Thu, Nov 28, 2019 at 12:58:06
> 
> > The 11/28/2019 12:20, Vitor Soares wrote:
> > > 
> > > Hi,
> > > 
> > > 
> > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > Date: Thu, Nov 28, 2019 at 05:50:08
> > > 
> > > > Hi Vitor,
> > > > 
> > > > First, you woke up my son and he couldn't sleep the rest of the night
> > > > :-)
> > > 
> > > Sorry for that.
> > > 
> > > > I appreciate you sent that so we can discuss it.
> > > > 
> > > > The 11/28/2019 02:15, Vitor Soares wrote:
> > > > > 
> > > > > This patch adds the preliminary support for secondary master feature to
> > > > > i3c Framework for testing purposes.
> > > > > 
> > > > > Key points for consideration:
> > > > >   - mastership_[show/store] are only used for testing
> > > > >   - secondary master registration is made in two steps, one in
> > > > >   i3c_master_register() and another in i3c_sec_master_bus_init() when
> > > > >   secondary master became current master first time. This is made in this
> > > > >   way to get all dt declared boardinfo list, create defslvs list and
> > > > >   provide work_queue.
> > > > >   - When the current master wants to deliver_mastership it necessary to
> > > > >   disable all in-band events to avoid unwanted interrupt during bus
> > > > >   ownership exchange. For now this patch doesn't reflect all
> > > > >   improvements/changes made in v1.1 I3C Bus spec. But it can be extended
> > > > >   by adding some commands and checks to the flow.
> > > > >   - i3c_defslvs_info: The DEFSLVS info can be differently stored in
> > > > >   diferen HC. Hence it is used a defslvs list similar to boardinfo list in
> > > > >   the bus structure to hold this data. Them HC is taccking over the bus
> > > > >   ownership can initialize each device of that list. For now, this not
> > > > >   address the i2c devices since they are only statically described.
> > > > >   - [request/deliver]_mastership(): Mastership request deliver may be done
> > > > >   differently in different HC, hence the need to have a call back for each
> > > > >   process.
> > > > >   - Add dr_mode to DT: Similar to USB, HC can be programmed to Master only
> > > > >   mode, Slave only mode or Secondary Master which aren't necessarily
> > > > >   hardcoded.
> > > > >   - bus_mode definition: The bus mode is defined even without defslvs
> > > > >   information with DT info since the definition of i2c devices are those
> > > > >   that have impact on bus_mode definition and need to statically declared.
> > > > >   The only use case that may cause issues is when i2c devices aren't
> > > > >   declared in secondary master side and bus mode doesn't match the
> > > > >   main master. Anyway this can be solde without extra complexity.
> > > > > 
> > > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > > > ---
> > > > >  drivers/i3c/master.c       | 365 ++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  include/linux/i3c/master.h |  39 +++++
> > > > >  2 files changed, 396 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > > index 0436916..b398d77 100644
> > > > > --- a/drivers/i3c/master.c
> > > > > +++ b/drivers/i3c/master.c
> > > > > @@ -449,6 +449,46 @@ static ssize_t mode_show(struct device *dev,
> > > > >  }
> > > > >  static DEVICE_ATTR_RO(mode);
> > > > >  
> > > > > +static ssize_t
> > > > > +mastership_show(struct device *dev, struct device_attribute *da, char *buf)
> > > > > +{
> > > > > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > > > > +	ssize_t ret;
> > > > > +
> > > > > +	if (master->secondary)
> > > > > +		ret = sprintf(buf, "Secondary Master\n");
> > > > > +	else
> > > > > +		ret = sprintf(buf, "Master\n");
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static ssize_t
> > > > > +mastership_store(struct device *dev, struct device_attribute *attr,
> > > > > +		 const char *buf, size_t count)
> > > > > +{
> > > > > +	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> > > > > +	struct i3c_bus *i3cbus = dev_to_i3cbus(dev);
> > > > > +
> > > > > +	if (i3cbus->cur_master == master->this) {
> > > > > +		dev_err(dev, "I'm current mater.");
> > > > > +		return count;
> > > > > +	}
> > > > > +
> > > > > +	if (!master->ops->request_mastership) {
> > > > > +		dev_err(dev, "mastership_request not supported.");
> > > > > +		return count;
> > > > > +	}
> > > > > +
> > > > > +	if (master->ops->request_mastership(master))
> > > > > +		dev_err(dev, "mastership_request failed");
> > > > > +	else
> > > > > +		dev_err(dev, "mastership_request success");
> > > > > +
> > > > > +	return count;
> > > > > +}
> > > > > +static DEVICE_ATTR_RW(mastership);
> > > > > +
> > > > >  static ssize_t current_master_show(struct device *dev,
> > > > >  				   struct device_attribute *da,
> > > > >  				   char *buf)
> > > > > @@ -457,8 +497,11 @@ static ssize_t current_master_show(struct device *dev,
> > > > >  	ssize_t ret;
> > > > >  
> > > > >  	i3c_bus_normaluse_lock(i3cbus);
> > > > > -	ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > > > -		      i3cbus->cur_master->info.pid);
> > > > > +	if (i3cbus->cur_master)
> > > > > +		ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > > > +			      i3cbus->cur_master->info.pid);
> > > > > +	else
> > > > > +		ret = sprintf(buf, "Not Current Master\n");
> > > > >  	i3c_bus_normaluse_unlock(i3cbus);
> > > > >  
> > > > >  	return ret;
> > > > > @@ -497,6 +540,7 @@ static DEVICE_ATTR_RO(i2c_scl_frequency);
> > > > >  
> > > > >  static struct attribute *i3c_masterdev_attrs[] = {
> > > > >  	&dev_attr_mode.attr,
> > > > > +	&dev_attr_mastership.attr,
> > > > >  	&dev_attr_current_master.attr,
> > > > >  	&dev_attr_i3c_scl_frequency.attr,
> > > > >  	&dev_attr_i2c_scl_frequency.attr,
> > > > > @@ -854,6 +898,53 @@ int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> > > > >  EXPORT_SYMBOL_GPL(i3c_master_enec_locked);
> > > > >  
> > > > >  /**
> > > > > + * i3c_master_getaccmst_locked() - send an GETACCMST CCC command
> > > > > + * @master: master used to send frames on the bus
> > > > > + * @addr: a valid I3C slave address
> > > > > + *
> > > > > + * Sends an GETACCMST CCC command to offer bus Mastership to an
> > > > > + * I3C Secondary Master.
> > > > > + *
> > > > > + * 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_getaccmst_locked(struct i3c_master_controller *master, u8 addr)
> > > > > +{
> > > > > +	enum i3c_addr_slot_status addrstat;
> > > > > +	struct i3c_ccc_getaccmst *accmst;
> > > > > +	struct i3c_ccc_cmd_dest dest;
> > > > > +	struct i3c_ccc_cmd cmd;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!master)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	addrstat = i3c_bus_get_addr_slot_status(&master->bus, addr);
> > > > > +	if (addr == I3C_BROADCAST_ADDR || addrstat != I3C_ADDR_SLOT_I3C_DEV)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> > > > > +	if (!accmst)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> > > > > +
> > > > > +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> > > > > +	if (ret)
> > > > > +		goto out;
> > > > > +
> > > > > +	if (accmst->newmaster >> 1 != addr)
> > > > 
> > > > I really like this check. This is something I realized working
> > > > on next patch version.
> > > > 
> > > > > +		ret = -EIO;
> > > > > +out:
> > > > > +	i3c_ccc_cmd_dest_cleanup(&dest);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_master_getaccmst_locked);
> > > > > +
> > > > > +/**
> > > > >   * i3c_master_defslvs_locked() - send a DEFSLVS CCC command
> > > > >   * @master: master used to send frames on the bus
> > > > >   *
> > > > > @@ -1542,8 +1633,7 @@ int i3c_master_set_info(struct i3c_master_controller *master,
> > > > >  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
> > > > >  		return -EINVAL;
> > > > >  
> > > > > -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> > > > > -	    master->secondary)
> > > > > +	if (I3C_BCR_DEVICE_ROLE(info->bcr) != I3C_BCR_I3C_MASTER)
> > > > >  		return -EINVAL;
> > > > >  
> > > > >  	if (master->this)
> > > > > @@ -2381,6 +2471,81 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +int i3c_sec_master_bus_init(struct i3c_master_controller *master)
> > > > > +{
> > > > > +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > > +	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> > > > > +	enum i3c_bus_mode mode = i3cbus->mode;
> > > > > +	struct i3c_defslvs_info *defslvsinfo;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (master->init_done)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > > > > +		if (defslvsinfo->slave.dyn_addr)
> > > > > +			continue;
> > > > > +
> > > > > +		switch (defslvsinfo->slave.lvr & I3C_LVR_I2C_INDEX_MASK) {
> > > > > +		case I3C_LVR_I2C_INDEX(0):
> > > > > +			if (mode < I3C_BUS_MODE_MIXED_FAST)
> > > > > +				mode = I3C_BUS_MODE_MIXED_FAST;
> > > > > +			break;
> > > > > +		case I3C_LVR_I2C_INDEX(1):
> > > > > +		case I3C_LVR_I2C_INDEX(2):
> > > > > +			if (mode < I3C_BUS_MODE_MIXED_SLOW)
> > > > > +				mode = I3C_BUS_MODE_MIXED_SLOW;
> > > > > +			break;
> > > > > +		default:
> > > > > +			ret = -EINVAL;
> > > > > +			goto err_put_dev;
> > > > > +		}
> > > > > +		if (defslvsinfo->slave.lvr & I3C_LVR_I2C_FM_MODE)
> > > > > +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
> > > > > +	}
> > > > > +
> > > > > +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
> > > > > +	if (ret)
> > > > > +		goto err_put_dev;
> > > > > +
> > > > > +	/*
> > > > > +	 * Now execute the controller specific ->bus_init() routine, which
> > > > > +	 * might configure its internal logic to match the bus limitations.
> > > > > +	 */
> > > > > +	ret = master->ops->bus_init(master);
> > > > > +	if (ret)
> > > > > +		goto err_put_dev;
> > > > > +
> > > > > +	/*
> > > > > +	 * The master device should have been instantiated in ->bus_init(),
> > > > > +	 * complain if this was not the case.
> > > > > +	 */
> > > > > +	if (!master->this) {
> > > > > +		dev_err(&master->dev,
> > > > > +			"master_set_info() was not called in ->bus_init()\n");
> > > > > +		ret = -EINVAL;
> > > > > +		goto err_put_dev;
> > > > > +	}
> > > > > +
> > > > > +	ret = device_add(&master->dev);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	/*
> > > > > +	 * Expose our I3C bus as an I2C adapter so that I2C devices are exposed
> > > > > +	 * through the I2C subsystem.
> > > > > +	 */
> > > > > +	ret = i3c_master_i2c_adapter_init(master);
> > > > > +	if (ret)
> > > > > +		goto err_put_dev;
> > > > > +
> > > > > +	master->init_done = true;
> > > > > +
> > > > > +err_put_dev:
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_sec_master_bus_init);
> > > > > +
> > > > >  /**
> > > > >   * i3c_master_register() - register an I3C master
> > > > >   * @master: master used to send frames on the bus
> > > > > @@ -2413,10 +2578,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;
> > > > > @@ -2430,6 +2591,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > > >  	master->secondary = secondary;
> > > > >  	INIT_LIST_HEAD(&master->boardinfo.i2c);
> > > > >  	INIT_LIST_HEAD(&master->boardinfo.i3c);
> > > > > +	INIT_LIST_HEAD(&master->defslvs);
> > > > >  
> > > > >  	ret = i3c_bus_init(i3cbus);
> > > > >  	if (ret)
> > > > > @@ -2475,6 +2637,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > > >  		goto err_put_dev;
> > > > >  	}
> > > > >  
> > > > > +	if (secondary)
> > > > > +		return 0;
> > > > > +
> > > > >  	ret = i3c_master_bus_init(master);
> > > > >  	if (ret)
> > > > >  		goto err_put_dev;
> > > > > @@ -2547,6 +2712,11 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> > > > >  	if (!master || !xfers)
> > > > >  		return -EINVAL;
> > > > >  
> > > > > +	if (master->bus.cur_master != master->this) {
> > > > > +		master->ops->request_mastership(master);
> > > > > +		return -EBUSY;
> > > > 
> > > > I don't like this approach, so you have to re-trigger the operation when
> > > > this master becomes current master. It is not transparent. Especially,
> > > > HCI 1.1 describes the process in detail, even  on flow chart and you can 
> > > > see there that you should block all the transfers/tasks on your side and
> > > > wait for GETACCMST.
> > > 
> > > I forgot to explain that this is part is not fully operational and my 
> > > intention was to address this in near future.
> > > To quickly head-up, what I had in my mind when did this was if a device 
> > > wants to do a xfer and sec master is not current master the framework 
> > > will request the bus ownership and them pass a EBUSY in case of success 
> > > or another error in case current master Nacks the MR request.
> > > 
> > > > 
> > > > > +	}
> > > > > +
> > > > >  	if (!master->ops->priv_xfers)
> > > > >  		return -ENOTSUPP;
> > > > >  
> > > > > @@ -2638,6 +2808,185 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> > > > >  	dev->ibi = NULL;
> > > > >  }
> > > > >  
> > > > > +static const char *const i3c_dr_modes[] = {
> > > > > +	[I3C_DR_MODE_MASTER]		= "master",
> > > > > +	[I3C_DR_MODE_SEC_MASTER]	= "sec-master",
> > > > > +	[I3C_DR_MODE_SLAVE]		= "slave",
> > > > > +};
> > > > > +
> > > > > +static enum i3c_dr_mode i3c_get_dr_mode_from_string(const char *str)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = match_string(i3c_dr_modes, ARRAY_SIZE(i3c_dr_modes), str);
> > > > > +	return (ret < 0) ? I3C_DR_MODE_MASTER : ret;
> > > > > +}
> > > > > +
> > > > > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev)
> > > > > +{
> > > > > +	const char *dr_mode;
> > > > > +	int err;
> > > > > +
> > > > > +	err = device_property_read_string(dev, "dr-mode", &dr_mode);
> > > > > +	if (err < 0)
> > > > > +		return I3C_DR_MODE_MASTER;
> > > > > +
> > > > > +	return i3c_get_dr_mode_from_string(dr_mode);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_get_dr_mode);
> > > > > +
> > > > > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	i3c_bus_normaluse_lock(&master->bus);
> > > > > +	ret = master->ops->request_mastership(master);
> > > > > +	i3c_bus_normaluse_unlock(&master->bus);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_sec_master_request_mastership);
> > > > > +
> > > > > +int i3c_master_deliver_mastership(struct i3c_master_controller *master, u8 addr)
> > > > 
> > > > I agree, wa can introduce this now. But we decided to postpone it. As
> > > > you can see, it shouldn't be so hard.
> > > 
> > > IMO this is important, I could use directly the CCC but I know that there 
> > > is other HC that may use a different approach.
> > > 
> > > > 
> > > > > +{
> > > > > +	struct i3c_dev_desc *dev;
> > > > > +	int ret;
> > > > > +
> > > > > +	i3c_bus_normaluse_lock(&master->bus);
> > > > > +	i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > > > > +		if (dev->ibi) {
> > > > > +			mutex_lock(&dev->ibi_lock);
> > > > > +			i3c_dev_disable_ibi_locked(dev);
> > > > > +			mutex_unlock(&dev->ibi_lock);
> > > > > +		}
> > > > > +	}
> > > > > +	i3c_master_disec_locked(master, I3C_BROADCAST_ADDR,
> > > > > +				I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > > > > +
> > > > > +	ret = master->ops->deliver_mastership(master, addr);
> > > > > +	i3c_bus_normaluse_unlock(&master->bus);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_master_deliver_mastership);
> > > > > +
> > > > > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > > > > +			     struct i3c_ccc_dev_desc defslvs)
> > > > > +{
> > > > > +	struct i3c_defslvs_info *defslvsinfo;
> > > > > +	struct device *dev = &master->dev;
> > > > > +
> > > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > > +	defslvsinfo = devm_kzalloc(dev, sizeof(*defslvsinfo), GFP_KERNEL);
> > > > > +	if (!defslvsinfo)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	defslvsinfo->slave = defslvs;
> > > > > +
> > > > > +	list_add_tail(&defslvsinfo->node, &master->defslvs);
> > > > 
> > > > I don't get why can't we call i3c_master_add_i3c_dev_locked when
> > > > populating the bus.
> > > 
> > > I want to avoid the populating bus call back.
> > 
> > Why?
> 
> When I developed this procedure the idea behind it was to avoid bus 
> callbacks and offload this complexity to framework because we don't know 
> how others HC handle DEFSLVS CCC, lets image the case where the DEFSLVS 
> are stored in a place that can be rewriter  before sec master get bus 
> ownership.

Yes, it is theoretically possible. This is not the case for our
controller, HCI and as I can see for DW also not. As now we have only
few drivers, I think we can do it this way.

In HCI, this data is stored in DAT table and entries has to be populated
because index in DAT table is used for transfer descriptors. Without
that, HC is not able to identify target device. Again, we should be ok
with it. The key here is to make the API as straightforward as possible

> Another advantage that I see is, when someone write a new HC driver won't 
> need to care about this because the framework already provide this 
> facility.

We have 2 drivers for now, I think that those two + HCI should be
satisfied now.

> 
> > 
> > > 
> > > > You have all the data on your plate (in HC driver)
> > > > when you are populating it from SEC_DEV_CHAR_TABLE_LOC.
> > > 
> > > Yes, I told you that I have a table for that, yet I decide to not use it. 
> > > My concern is about the HC that doesn't have?
> > > For me passing this task for the framework is more universal. 
> > > 
> > 
> > Looks like we can do the same in HCI.
> > HC driver has to know always the devices on the bus.
> 
> My view here is more like the HC driver provide a way of the framework 
> interact with bus.
> 

API extends that way, and we are dupplicating the code. If we decide at
some point that we should provide information to framewoek this way, we
can change it easily.

> > 
> > > > 
> > > > I decided to do it similarly, but then Boris suggested to rework it and
> > > > we use only i3c_master_add_i3c_dev_locked.
> > > > 
> > > > > +
> > > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(defslvs_populate_i3c_bus);
> > > > > +
> > > > > +static void i3c_master_add_new_defslvs(struct i3c_master_controller *master)
> > > > > +{
> > > > > +	struct i3c_defslvs_info *defslvsinfo;
> > > > > +
> > > > > +	list_for_each_entry(defslvsinfo, &master->defslvs, node) {
> > > > > +		/* TODO: add i2c devices to the bus */
> > > > > +		if (!defslvsinfo->slave.dyn_addr)
> > > > > +			continue;
> > > > > +
> > > > > +		if (defslvsinfo->slave.dyn_addr == master->this->info.dyn_addr)
> > > > > +			continue;
> > > > > +
> > > > > +		if (!i3c_bus_dev_addr_is_avail(&master->bus,
> > > > > +					       defslvsinfo->slave.dyn_addr))
> > > > 
> > > > We can add those checks also but we also have i3c_master_attach_i3c_dev
> > > > and i3c_master_get_i3c_addrs which takes care of this everything.
> > > 
> > > Yes, but them you will be allocating and free devs unnecessarily.
> > > 
> > 
> > Can you explain that?
> 
> Yap. In i3c_master_add_i3c_dev_locked we 1st allocate the dev, then 
> attach the device and if it fails we free the dev.
> I think here we can check in upfront if the address is already in use and 
> avoid extra steps.

Yes, I agree. We can do that. We cand sent this patch earlier or later
to not to complicate mastership patchset. I want to keep it simple.

> 
> > 
> > > > 
> > > > > +			continue;
> > > > > +
> > > > > +		i3c_master_add_i3c_dev_locked(master, defslvsinfo->slave.dyn_addr);
> > > > > +	}
> > > > > +
> > > > > +	while (!list_empty(&master->defslvs)) {
> > > > > +		defslvsinfo = list_first_entry(&master->defslvs,
> > > > > +					       struct i3c_defslvs_info, node);
> > > > > +		list_del(&defslvsinfo->node);
> > > > 
> > > > I feel like this code is redundant, we have to allocate it, then delete.
> > > 
> > > No. you need to clean the list. I may receive another one in the future 
> > > due a HJ or a dynamic address change.
> > >
> 
> In my previous comment I forgot to mention by doing the cleanup in API we 
> avoid duplicated code in HC drivers.

When you call i3c_master_add_i3d_dev_locked directly, code is not
dupplicated. We can rework also device registration on HC side to not to
introduce dupplicates.

> 
> > 
> > I like more the way when HC driver is updateing that. It makes API
> > simpler.
> 
> There is nothing against that 😉. In the end, the purpose of this DEFSLVS 
> handling was to present an alternative solution and its advantages.

Maybe someday we'll be forced to introduce that, who knows. For now,
for everyone, key should be to keep it simple.

> 
> > 
> > > > 
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > > > > +				     bool secondary)
> > > > > +{
> > > > > +	struct i3c_dev_desc *dev;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (master->secondary == secondary)
> > > > > +		return -EEXIST;
> > > > > +
> > > > > +	/* TODO: get the current master information */
> > > > > +	if (secondary)
> > > > > +		master->bus.cur_master = NULL;
> > > > > +	else
> > > > > +		master->bus.cur_master = master->this;
> > > > > +
> > > > > +	if (!master->init_done && !secondary)
> > > > > +		i3c_sec_master_bus_init(master);
> > > > > +
> > > > > +	master->secondary = secondary;
> > > > > +
> > > > > +	dev_info(&master->dev, "changing to %s\n",
> > > > > +		 master->secondary ? "Sec Master" : "Master");
> > > > > +
> > > > > +	/* TODO: Analyse the use of maintenan_lock for everything */
> > > > > +	if (!list_empty(&master->defslvs) && !secondary) {
> > > > > +		i3c_bus_maintenance_lock(&master->bus);
> > > > > +		i3c_master_add_new_defslvs(master);
> > > > > +		i3c_bus_maintenance_unlock(&master->bus);
> > > > > +
> > > > > +		i3c_bus_normaluse_lock(&master->bus);
> > > > > +		i3c_master_register_new_i3c_devs(master);
> > > > 
> > > > Take a look also at i3c_master_bus_takeover from my latest patch. BTW.
> > > > what about I2C devices? We worked on that also, and this is part of the
> > > > latest patch also. I'm testing it with I2C devices also.
> > > 
> > > Please check the comments for that. Anyway you can declare the i2c 
> > > devices of DT on both sides, what I think it should be a good practice.
> > > 
> > 
> > This is what we are doing, but we are comparing I2C devices from DEFSLVS
> > list with those defined in DT. We discussed that already.
> 
> You are right.
> 
> > 
> > > > 
> > > > > +		i3c_bus_normaluse_unlock(&master->bus);
> > > > > +	}
> > > > > +
> > > > > +	if (!secondary) {
> > > > > +		i3c_bus_normaluse_lock(&master->bus);
> > > > > +		i3c_bus_for_each_i3cdev(&master->bus, dev) {
> > > > > +			if (dev->ibi) {
> > > > > +				mutex_lock(&dev->ibi_lock);
> > > > > +				ret = i3c_dev_enable_ibi_locked(dev);
> > > > > +				if (ret)
> > > > > +					dev_err(&master->dev,
> > > > > +						"Failed to re-enable IBI on device %d-%llx",
> > > > > +						master->bus.id, dev->info.pid);
> > > > > +				mutex_unlock(&dev->ibi_lock);
> > > > > +				}
> > > > > +		}
> > > > > +
> > > > > +		/* TODO: Enable MR only for the elegible devices */
> > > > 
> > > > This was postponed also, but we had that before. We can add per-device
> > > > granularity to i3c_master_bus_takeover().
> > > 
> > > Here we need to do the same as for ibi and maybe get the eligible devices 
> > > from DT or based on its DCR,BCR and PID.
> > > This is something that should be address because it represent a big 
> > > security gap.
> > > 
> > > > 
> > > > > +		i3c_master_enec_locked(master, I3C_BROADCAST_ADDR,
> > > > > +					I3C_CCC_EVENT_MR | I3C_CCC_EVENT_HJ);
> > > > > +		i3c_bus_normaluse_unlock(&master->bus);
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_master_switch_operation_mode);
> > > > > +
> > > > > +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
> > > > > +{
> > > > > +	int res;
> > > > > +
> > > > > +	mutex_lock(&i3c_core_lock);
> > > > > +	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
> > > > > +	mutex_unlock(&i3c_core_lock);
> > > > > +
> > > > > +	return res;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(i3c_for_each_dev);
> > > > > +
> > > > >  static int __init i3c_init(void)
> > > > >  {
> > > > >  	return bus_register(&i3c_bus_type);
> > > > > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > > > > index 9cb39d9..09bd99c 100644
> > > > > --- a/include/linux/i3c/master.h
> > > > > +++ b/include/linux/i3c/master.h
> > > > > @@ -426,6 +426,8 @@ struct i3c_bus {
> > > > >   *		      for a future IBI
> > > > >   *		      This method is mandatory only if ->request_ibi is not
> > > > >   *		      NULL.
> > > > > + * @request_mastership: Request mastership.
> > > > > + * @deliver_mastership: Deliver mastership
> > > > >   */
> > > > >  struct i3c_master_controller_ops {
> > > > >  	int (*bus_init)(struct i3c_master_controller *master);
> > > > > @@ -452,6 +454,21 @@ 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);
> > > > > +	int (*request_mastership)(struct i3c_master_controller *master);
> > > > > +	int (*deliver_mastership)(struct i3c_master_controller *master,
> > > > > +				  u8 addr);
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * struct i3c_defslvs_info - defslvs information object
> > > > > + * @node: used to insert the defslvs object in the  list
> > > > > + * @slave: I3C/I2C device descriptor used for DEFSLVS
> > > > > + *
> > > > > + * This structure is used to hold defslvs information on Secondary Master.
> > > > > + */
> > > > > +struct i3c_defslvs_info {
> > > > > +	struct list_head node;
> > > > > +	struct i3c_ccc_dev_desc slave;
> > > > >  };
> > > > >  
> > > > >  /**
> > > > > @@ -468,6 +485,7 @@ struct i3c_master_controller_ops {
> > > > >   * @boardinfo.i3c: list of I3C  boardinfo objects
> > > > >   * @boardinfo.i2c: list of I2C boardinfo objects
> > > > >   * @boardinfo: board-level information attached to devices connected on the bus
> > > > > + * @defslvs: List of defslvs objects
> > > > >   * @bus: I3C bus exposed by this master
> > > > >   * @wq: workqueue used to execute IBI handlers. Can also be used by master
> > > > >   *	drivers if they need to postpone operations that need to take place
> > > > > @@ -491,6 +509,7 @@ struct i3c_master_controller {
> > > > >  		struct list_head i3c;
> > > > >  		struct list_head i2c;
> > > > >  	} boardinfo;
> > > > > +	struct list_head defslvs;
> > > > >  	struct i3c_bus bus;
> > > > >  	struct workqueue_struct *wq;
> > > > >  };
> > > > > @@ -525,6 +544,7 @@ int i3c_master_disec_locked(struct i3c_master_controller *master, u8 addr,
> > > > >  			    u8 evts);
> > > > >  int i3c_master_enec_locked(struct i3c_master_controller *master, u8 addr,
> > > > >  			   u8 evts);
> > > > > +int i3c_master_getaccmst_locked(struct i3c_master_controller *master, u8 addr);
> > > > >  int i3c_master_entdaa_locked(struct i3c_master_controller *master);
> > > > >  int i3c_master_defslvs_locked(struct i3c_master_controller *master);
> > > > >  
> > > > > @@ -652,4 +672,23 @@ void i3c_master_queue_ibi(struct i3c_dev_desc *dev, struct i3c_ibi_slot *slot);
> > > > >  
> > > > >  struct i3c_ibi_slot *i3c_master_get_free_ibi_slot(struct i3c_dev_desc *dev);
> > > > >  
> > > > > +enum i3c_dr_mode {
> > > > > +	I3C_DR_MODE_MASTER,
> > > > > +	I3C_DR_MODE_SEC_MASTER,
> > > > > +	I3C_DR_MODE_SLAVE,
> > > > > +};
> > > > > +
> > > > > +enum i3c_dr_mode i3c_get_dr_mode(struct device *dev);
> > > > > +
> > > > > +int i3c_master_switch_operation_mode(struct i3c_master_controller *master,
> > > > > +				     bool secondary);
> > > > > +
> > > > > +int i3c_sec_master_request_mastership(struct i3c_master_controller *master);
> > > > > +int i3c_master_deliver_mastership(struct i3c_master_controller *master,
> > > > > +				  u8 addr);
> > > > > +
> > > > > +int i3c_sec_master_bus_init(struct i3c_master_controller *master);
> > > > > +int defslvs_populate_i3c_bus(struct i3c_master_controller *master,
> > > > > +			     struct i3c_ccc_dev_desc defslvs);
> > > > > +
> > > > >  #endif /* I3C_MASTER_H */
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > 
> > > > I feel like this is based on early approach which has evolved by the time
> > > > and I think some of the nice part are missing. The biggest parts to
> > > > discuass are:
> > > > - bus population using devslvs device list instead of using
> > > >   i3c_master_add_i3c_dev locked, have you tried that? If something does
> > > >   not work for you, wa can adjust it but I really want you to try the
> > > >   new approach
> > > 
> > > To me the i3c_master_add_i3c_dev_locked just need an improvement to 
> > > propagate the boardinfo to all devices, something that already I started, 
> > > and we need to discuss if on secondary master side we can change the 
> > > devices addresses which imply to send DEFSLVS at the end of the process.
> > 
> > So let's improve it. Can you point me to that?
> 
> Regarding the improvements you can check the patches that touch in the 
> API in this series [1].
> During the development of v3 I started to thinking it is a good idea to 
> pre-reserve all declared addresses in DT and not use them during the 
> ENTDAA process. We can discuss this when I send the new version.
> 

Ok, I'll look into that once again implementing the new version. But I
still feel like we can detach that but we should keep the information
about the device in the framework. Otherwise we are losing some
information which may be useful in the future.

> 
> Regarding the secondary master, do you think it is a good idea to let the 
> secondary master change the dynamic address of a device assigned by the 
> main master?
> For me we might need this kind of algorithms thing to define the 
> secondary master behavior.
> 

The I3C spec clearly defines that case. Secondary master can reconfigure
the bus. But, depending on PID we are looking for dupplicates and
reattaching the devices with new DAs. I'm testing that part also,
introduced that test after our last discussion. Of course, I'm forcing
it currently from secondary master side but this is the proof :-)

> > 
> > > 
> > > > 
> > > > - the way how we are requesting mastership, IMO, we should wait untill
> > > >   the process is finished, as also described in HCI spec for example.
> > > 
> > > I think you are confusing the concept here. I understand you want to do 
> > > the xfer straight way after receive GETACCMST and I agree with that.
> > > The thing is that you don't know what will happen on the bus between the 
> > > time you send the request and you get the bus ownership. I just think we 
> > > need to find another solution for that like defer the transfer to another 
> > > time and when the controller switch the rule trigger all transfers in the 
> > > pipeline (something like this) even before restore all ibi. If you block 
> > > the bus you are not able to pass the defslvs to the framework.  
> > > 
> > 
> > If the MR request was acked, we know that we'll get the control. And it
> > doesn't matter if current master will enable/disable something, change
> > the activity state, send another DEFSLVS, we still want to make a
> > transfer. 
> > 
> > In your approach it is possible that we get the control, we'll
> > not make the transfer and give control back to a diffent master. 
> > I have tests for more than 2 masters.
> 
> You are right.
> This case I didn't address it yet. I have a upper layer that give bus 
> access time to each master for now.
> 
> In your use case if you don't need the IBI in the secondary master side, 
> I would say the best is to request mastership, do the transfer, and send 
> back the mastership, without the logistics enable/disable IBI in 
> secondary master.
> 
> > 
> > And also, I don't feel like queueing the transfers is good idea. When
> > some upper layer wants to transfer the data again, old data may be stale
> > but we still will transfer that.
> 
> That might be a problem for both approach because the exchange of 
> ownership can take so long that xfer data might not be valid anymore.

But we shouldn't wait forever. And this should give upper layer the
control, if mastership is not possible, upper layer may just retry the
transfer later, if still needed. Also, MIPI spec describes this: "After
acknowledging the Mastership Request, the Current Master should prepare
for Handoff in a timely manner".

> IMO it is also dangerous to have the timeout of the bus ownership 
> exchange in HC driver because it will depend from use case to use case 
> and it is not something predictable.

Exactly, that's why I'm going to do one thing, I discussed that already
with Boris. I'll add timeout to the HC driver struct for now, and then
we can change it and pass that timeout to the function. This wy we can
potentialy expose that timeout through sysfs to let users tweak it
depending on their use case.

> 
> > 
> > > >   When I introduce interrupt-based solution, everything should be fine.
> > > >   Could you do that also in your driver?
> > > 
> > > I did it but had issues and didn't fit in my use case. That the reason I 
> > > did this approach.
> > 
> > Can you describe what issues you faced with this approach? What is your
> > use case? If we get the control, we just make the transfers, if not,
> > return exit code to upper layer and that's it.
> 
> The way I have the code that is not possible. In fact it is something 
> that I need help to let the xfer continue after sec master takeover the 
> bus. 

I think this approach makes it too complex. User can handle the cases
when MR wasn't possible. And the transfers will be handled
automatically.

> 
> > 
> > > 
> > > > 
> > > > Again, good to have that. I'm really counting on a fair discussion :-)
> > > > 
> > > > -- 
> > > > --
> > > > Regards, 
> > > > Przemyslaw Gaj
> > > 
> > > As you can see this is based on your work. Off course it as some lose 
> > > ends that should be addressed (like i2c devices, bus mode...) but don't 
> > > have big impact in what I want to show.
> > 
> > Yes, but on my work in early stage and I think this approach wasn't
> > perfect. Then we decided to refork some parts which is not part of that.
> > I feel like we can't find common view.
> > 
> > > For me it is important to rely on framework the way how bus ownership 
> > > exchange is made because it will be easier to maintain in long term and 
> > > we can introduce algorithms for the bus access in the future.
> > > 
> > 
> > Yes, why not. My latest patchset does not break anything here.
> 
> It already passed few months since them, but I remember to have issues 
> during the master/secondary registration.
> 

Ok, I'll take few things from here to next release but the question is,
can you take it after that and try to implement MR in DW based on my
work? It will really help, I hope I can count on you :-). Without that
we won't be able to find the solution.

> > 
> > > If you are using i3c-tree/next you can apply this directly and if you 
> > > find anything else that isn't going ok please let me know.
> > > 
> > > Best regards,
> > > Vitor Soares
> > 
> > -- 
> > -- 
> > Regards,
> > Przemyslaw Gaj
> 
> Best regards,
> Vitor Soares
> 
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_11127661_&d=DwIGaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=CMnAfM_OfpqcWZRfiqcRWw&m=PaFCjYnrERfxLlGyJ94Fp6dSCEEIT9dyKDOOHqenQW0&s=Nu8sd9WAbwad39vbwVYSc3PN5TmLRnlya2l-vujYFHo&e= 
> 

-- 
-- 
Regards,
Przemek

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [RFC 1/2] i3c: Add preliminary support for secondary master

[Przemyslaw Gaj]

Hi Vitor,

I would like to share the document with you. This should help us to come
up with better implementation / implementation which fits everyone's needs.

https://annuel.framapad.org/p/i3c-mastership-handover?lang=en

Please comment / add additional sections / ideas which you have in mind.

Regards,
Przemek


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c