[PATCH v4 0/6] Add the I3C mastership request

[PATCH v4 0/6] Add the I3C mastership request

[Przemyslaw Gaj]

This patch series adds support for mastership request to I3C
subsystem and Cadence I3C master driver. Mastership request
allows slave to become the master of the I3C bus.

Main changes between v3 and v4 are:
- Reworked acquire bus ownership
- Refactored the code

Main changes between v2 and v3 are:
- Added DEFSLVS devices are registered from master driver
- Reworked I2C registering on secondary master side
- Reworked Mastership event is enabled/disabled globally (for all devices)

Main changes between initial version and v2 are:
- Reworked devices registration on secondary master side
- Reworked mastership event disabling/enabling
- Reworked bus locking during mastership takeover process
- Added DEFSLVS devices registration during initialization
- Fixed style issues

Przemyslaw Gaj (6):
  i3c: add addr and lvr to i2c_dev_desc structure
  i3c: export bus maintenance lock and unlock functions
  i3c: Add support for mastership request to I3C subsystem
  i3c: master: cdns: add support for mastership request to Cadence I3C
    master driver.
  i3c: master: Add module author
  MAINTAINERS: add myself as co-maintainer of i3c subsystem

 MAINTAINERS                          |   1 +
 drivers/i3c/device.c                 |  26 ++
 drivers/i3c/internals.h              |   4 +
 drivers/i3c/master.c                 | 425 ++++++++++++++++++++++++------
 drivers/i3c/master/i3c-master-cdns.c | 495 +++++++++++++++++++++++++++++++----
 include/linux/i3c/master.h           |  25 ++
 6 files changed, 849 insertions(+), 127 deletions(-)

-- 
2.8.3


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

[PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Przemyslaw Gaj]

I need to store address and lvr value for I2C devices without static definition
in DT. This allows secondary master to transmit DEFSLVS command properly.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
 include/linux/i3c/master.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index eca8337..3c27d9f 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
  * @common: common part of the I2C device descriptor
  * @boardinfo: pointer to the boardinfo attached to this I2C device
  * @dev: I2C device object registered to the I2C framework
+ * @addr: I2C device address
+ * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
+ *	 the I2C device limitations
  *
  * Each I2C device connected on the bus will have an i2c_dev_desc.
  * This object is created by the core and later attached to the controller
@@ -84,6 +87,8 @@ struct i2c_dev_desc {
 	struct i3c_i2c_dev_desc common;
 	const struct i2c_dev_boardinfo *boardinfo;
 	struct i2c_client *dev;
+	u16 addr;
+	u8 lvr;
 };
 
 /**
-- 
2.8.3


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

[PATCH v4 2/6] i3c: export bus maintenance lock and unlock functions

[Przemyslaw Gaj]

Secondary master driver has to gather device information using GETPID/GETBCR
and GETDCR. Mostly GETPID, DEFSLVS command does not provide device PID. Because
devices are registered from master controller driver, it has to lock the bus
for maintenance.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
 drivers/i3c/master.c       | 6 ++++--
 include/linux/i3c/master.h | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5b3adb3..aea4309 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -38,10 +38,11 @@ static DEFINE_MUTEX(i3c_core_lock);
  * logic to rely on I3C device information that could be changed behind their
  * back.
  */
-static void i3c_bus_maintenance_lock(struct i3c_bus *bus)
+void i3c_bus_maintenance_lock(struct i3c_bus *bus)
 {
 	down_write(&bus->lock);
 }
+EXPORT_SYMBOL_GPL(i3c_bus_maintenance_lock);
 
 /**
  * i3c_bus_maintenance_unlock - Release the bus lock after a maintenance
@@ -52,10 +53,11 @@ static void i3c_bus_maintenance_lock(struct i3c_bus *bus)
  * i3c_bus_maintenance_lock() for more details on what these maintenance
  * operations are.
  */
-static void i3c_bus_maintenance_unlock(struct i3c_bus *bus)
+void i3c_bus_maintenance_unlock(struct i3c_bus *bus)
 {
 	up_write(&bus->lock);
 }
+EXPORT_SYMBOL_GPL(i3c_bus_maintenance_unlock);
 
 /**
  * i3c_bus_normaluse_lock - Lock the bus for a normal operation
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 3c27d9f..42bb215 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -647,4 +647,7 @@ 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);
 
+void i3c_bus_maintenance_lock(struct i3c_bus *bus);
+void i3c_bus_maintenance_unlock(struct i3c_bus *bus);
+
 #endif /* I3C_MASTER_H */
-- 
2.8.3


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

[PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Przemyslaw Gaj]

This patch adds support for mastership request to I3C subsystem.

Mastership event is enabled globally.

Mastership is requested automatically when device driver
tries to transfer data using master controller in slave mode.

There is still some limitation:
- I2C devices are registered on secondary master side if boardinfo
entry matching the info transmitted through the DEFSLVS frame.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>

---

Main changes between v3 and v4:
- Add i3c_master_acquire_bus_ownership to acquire the bus
- Refactored the code

Main changes between v2 and v3:
- Add i3c_bus_downgrade_maintenance_lock() for downgrading the bus
lock from maintenance to normal use
- Add additional fields to i2c_dev_desc for DEFSLVS purpose (addr, lvr)
- Add i3c_master_register_new_i2c_devs() function to register I2C devices
- Reworked I2C devices registration on secondary master side

Changes in v2:
- Add mastership disable event hook
- Changed name of mastership enable event hook
- Add function to test if master owns the bus
- Add function to test if master owns the bus
- Removed op_mode
- Changed parameter of i3c_master_get_accmst_locked, no need to
pass full i3c_device_info
- Changed name of mastership enable event hook
- Add function to test if master owns the bus
- Removed op_mode
- Changed parameter of i3c_master_get_accmst_locked, no need to
pass full i3c_device_info
- Removed redundant DEFSLVS command before GETACCMST
- Add i3c_master_bus_takeover function. There is a need to lock
the bus before adding devices and no matter of the controller
devices have to be added after mastership takeover.
- Add device registration during initialization on secondary master
side. Devices received by DEFSLVS (if occured). If not, device
initialization is deffered untill next mastership request.
---
 drivers/i3c/device.c       |  26 +++
 drivers/i3c/internals.h    |   4 +
 drivers/i3c/master.c       | 417 +++++++++++++++++++++++++++++++++++++--------
 include/linux/i3c/master.h |  17 ++
 4 files changed, 391 insertions(+), 73 deletions(-)

diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 69cc040..b60f637 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -43,7 +43,13 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
 	}
 
 	i3c_bus_normaluse_lock(dev->bus);
+	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
+	if (ret)
+		goto err_unlock_bus;
+
 	ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
+
+err_unlock_bus:
 	i3c_bus_normaluse_unlock(dev->bus);
 
 	return ret;
@@ -114,11 +120,17 @@ int i3c_device_enable_ibi(struct i3c_device *dev)
 	int ret = -ENOENT;
 
 	i3c_bus_normaluse_lock(dev->bus);
+	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
+	if (ret)
+		goto err_unlock_bus;
+
 	if (dev->desc) {
 		mutex_lock(&dev->desc->ibi_lock);
 		ret = i3c_dev_enable_ibi_locked(dev->desc);
 		mutex_unlock(&dev->desc->ibi_lock);
 	}
+
+err_unlock_bus:
 	i3c_bus_normaluse_unlock(dev->bus);
 
 	return ret;
@@ -145,11 +157,17 @@ int i3c_device_request_ibi(struct i3c_device *dev,
 		return -EINVAL;
 
 	i3c_bus_normaluse_lock(dev->bus);
+	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
+	if (ret)
+		goto err_unlock_bus;
+
 	if (dev->desc) {
 		mutex_lock(&dev->desc->ibi_lock);
 		ret = i3c_dev_request_ibi_locked(dev->desc, req);
 		mutex_unlock(&dev->desc->ibi_lock);
 	}
+
+err_unlock_bus:
 	i3c_bus_normaluse_unlock(dev->bus);
 
 	return ret;
@@ -166,12 +184,20 @@ EXPORT_SYMBOL_GPL(i3c_device_request_ibi);
  */
 void i3c_device_free_ibi(struct i3c_device *dev)
 {
+	int ret;
+
 	i3c_bus_normaluse_lock(dev->bus);
+	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
+	if (ret)
+		goto err_unlock_bus;
+
 	if (dev->desc) {
 		mutex_lock(&dev->desc->ibi_lock);
 		i3c_dev_free_ibi_locked(dev->desc);
 		mutex_unlock(&dev->desc->ibi_lock);
 	}
+
+err_unlock_bus:
 	i3c_bus_normaluse_unlock(dev->bus);
 }
 EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 86b7b44..929ca6b 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type;
 
 void i3c_bus_normaluse_lock(struct i3c_bus *bus);
 void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
+void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus);
+int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master);
+int i3c_master_request_mastership_locked(struct i3c_master_controller *master);
+
 
 int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
 				 struct i3c_priv_xfer *xfers,
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index aea4309..7a84158 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
 	up_read(&bus->lock);
 }
 
+/**
+ * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal
+ * operation
+ * @bus: I3C bus to downgrade the lock on
+ *
+ * Should be called when a maintenance operation is done and normal
+ * operation is planned. See i3c_bus_maintenance_lock() and
+ * i3c_bus_normaluse_lock() for more details.
+ */
+void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus)
+{
+	downgrade_write(&bus->lock);
+}
+
 static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
 {
 	return container_of(dev, struct i3c_master_controller, dev);
@@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev)
 	return driver->probe(i3cdev);
 }
 
+static void i3c_master_enable_mr_events(struct i3c_master_controller *master)
+{
+	if (!master->ops->enable_mr_events)
+		return;
+
+	master->ops->enable_mr_events(master);
+}
+
+static void i3c_master_disable_mr_events(struct i3c_master_controller *master)
+{
+	if (!master->ops->disable_mr_events)
+		return;
+
+	master->ops->disable_mr_events(master);
+}
+
 static int i3c_device_remove(struct device *dev)
 {
 	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
@@ -462,6 +492,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
 	return 0;
 }
 
+int i3c_master_request_mastership_locked(struct i3c_master_controller *master)
+{
+	if (WARN_ON(master->init_done &&
+	    !rwsem_is_locked(&master->bus.lock)))
+		return -EINVAL;
+
+	if (!master->ops->request_mastership)
+		return -ENOTSUPP;
+
+	return master->ops->request_mastership(master);
+}
+
+int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master)
+{
+	int ret;
+
+	if (master->bus.cur_master != master->this) {
+		i3c_bus_normaluse_unlock(&master->bus);
+		i3c_bus_maintenance_lock(&master->bus);
+
+		ret = i3c_master_request_mastership_locked(master);
+		if (ret) {
+			i3c_bus_maintenance_unlock(&master->bus);
+			return ret;
+		}
+		i3c_bus_downgrade_maintenance_lock(&master->bus);
+	}
+	return 0;
+}
+
 static const char * const i3c_bus_mode_strings[] = {
 	[I3C_BUS_MODE_PURE] = "pure",
 	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
@@ -620,6 +680,25 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master,
 
 	dev->common.master = master;
 	dev->boardinfo = boardinfo;
+	dev->addr = boardinfo->base.addr;
+	dev->lvr = boardinfo->lvr;
+
+	return dev;
+}
+
+static struct i2c_dev_desc *
+i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master,
+				      u16 addr, u8 lvr)
+{
+	struct i2c_dev_desc *dev;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
+
+	dev->common.master = master;
+	dev->addr = addr;
+	dev->lvr = lvr;
 
 	return dev;
 }
@@ -693,6 +772,9 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
 	struct i2c_dev_desc *dev;
 
 	i3c_bus_for_each_i2cdev(&master->bus, dev) {
+		if (!dev->boardinfo)
+			continue;
+
 		if (dev->boardinfo->base.addr == addr)
 			return dev;
 	}
@@ -939,8 +1021,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master)
 
 	desc = defslvs->slaves;
 	i3c_bus_for_each_i2cdev(bus, i2cdev) {
-		desc->lvr = i2cdev->boardinfo->lvr;
-		desc->static_addr = i2cdev->boardinfo->base.addr << 1;
+		desc->lvr = i2cdev->lvr;
+		desc->static_addr = i2cdev->addr << 1;
 		desc++;
 	}
 
@@ -1492,6 +1574,83 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
 	}
 }
 
+static struct i2c_dev_boardinfo *
+i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master,
+			      u16 addr, u8 lvr)
+{
+	struct i2c_dev_boardinfo *i2cboardinfo;
+
+	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
+		if (i2cboardinfo->base.addr == addr &&
+		    i2cboardinfo->lvr == lvr)
+			return i2cboardinfo;
+	}
+
+	return NULL;
+}
+
+static void
+i3c_master_register_new_i2c_devs(struct i3c_master_controller *master)
+{
+	struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master);
+	struct i2c_dev_desc *i2cdev;
+
+	if (!master->init_done)
+		return;
+
+	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
+		if (i2cdev->dev)
+			continue;
+
+		if (!i2cdev->boardinfo)
+			continue;
+
+		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
+	}
+}
+
+/**
+ * i3c_master_get_accmst_locked() - send a GETACCMST CCC command
+ * @master: master used to send frames on the bus
+ * @info: I3C device information
+ *
+ * Send a GETACCMST CCC command.
+ *
+ * This should be called if the curent master acknowledges bus takeover.
+ *
+ * This function must be called with the bus lock held in write mode.
+ *
+ * Return: 0 in case of success, a positive I3C error code if the error is
+ * one of the official Mx error codes, and a negative error code otherwise.
+ */
+int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
+				 u8 addr)
+{
+	struct i3c_ccc_getaccmst *accmst;
+	struct i3c_ccc_cmd_dest dest;
+	struct i3c_ccc_cmd cmd;
+	int ret;
+
+	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
+	if (!accmst)
+		return -ENOMEM;
+
+	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
+
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	if (ret)
+		goto out;
+
+	if (dest.payload.len != sizeof(*accmst))
+		ret = -EIO;
+
+out:
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
+
 /**
  * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
  * @master: master doing the DAA
@@ -1559,10 +1718,6 @@ int i3c_master_set_info(struct i3c_master_controller *master,
 	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
 		return -EINVAL;
 
-	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
-	    master->secondary)
-		return -EINVAL;
-
 	if (master->this)
 		return -EINVAL;
 
@@ -1607,43 +1762,13 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
 				 common.node) {
 		i3c_master_detach_i2c_dev(i2cdev);
 		i3c_bus_set_addr_slot_status(&master->bus,
-					i2cdev->boardinfo->base.addr,
-					I3C_ADDR_SLOT_FREE);
+					     i2cdev->addr,
+					     I3C_ADDR_SLOT_FREE);
 		i3c_master_free_i2c_dev(i2cdev);
 	}
 }
 
-/**
- * i3c_master_bus_init() - initialize an I3C bus
- * @master: main master initializing the bus
- *
- * This function is following all initialisation steps described in the I3C
- * specification:
- *
- * 1. Attach I2C and statically defined I3C devs to the master so that the
- *    master can fill its internal device table appropriately
- *
- * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
- *    the master controller. That's usually where the bus mode is selected
- *    (pure bus or mixed fast/slow bus)
- *
- * 3. Instruct all devices on the bus to drop their dynamic address. This is
- *    particularly important when the bus was previously configured by someone
- *    else (for example the bootloader)
- *
- * 4. Disable all slave events.
- *
- * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
- *    devices that have a static address
- *
- * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
- *    remaining I3C devices
- *
- * Once this is done, all I3C and I2C devices should be usable.
- *
- * Return: a 0 in case of success, an negative error code otherwise.
- */
-static int i3c_master_bus_init(struct i3c_master_controller *master)
+static int i3c_master_attach_static_devs(struct i3c_master_controller *master)
 {
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
@@ -1652,32 +1777,24 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 
-	/*
-	 * First attach all devices with static definitions provided by the
-	 * FW.
-	 */
 	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
 		status = i3c_bus_get_addr_slot_status(&master->bus,
 						      i2cboardinfo->base.addr);
-		if (status != I3C_ADDR_SLOT_FREE) {
-			ret = -EBUSY;
-			goto err_detach_devs;
-		}
+		if (status != I3C_ADDR_SLOT_FREE)
+			return -EBUSY;
 
 		i3c_bus_set_addr_slot_status(&master->bus,
 					     i2cboardinfo->base.addr,
 					     I3C_ADDR_SLOT_I2C_DEV);
 
 		i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo);
-		if (IS_ERR(i2cdev)) {
-			ret = PTR_ERR(i2cdev);
-			goto err_detach_devs;
-		}
+		if (IS_ERR(i2cdev))
+			return PTR_ERR(i2cdev);
 
 		ret = i3c_master_attach_i2c_dev(master, i2cdev);
 		if (ret) {
 			i3c_master_free_i2c_dev(i2cdev);
-			goto err_detach_devs;
+			return ret;
 		}
 	}
 	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
@@ -1687,28 +1804,71 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 
 		if (i3cboardinfo->init_dyn_addr) {
 			status = i3c_bus_get_addr_slot_status(&master->bus,
-						i3cboardinfo->init_dyn_addr);
-			if (status != I3C_ADDR_SLOT_FREE) {
-				ret = -EBUSY;
-				goto err_detach_devs;
-			}
+							      i3cboardinfo->init_dyn_addr);
+			if (status != I3C_ADDR_SLOT_FREE)
+				return -EBUSY;
 		}
 
 		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
-		if (IS_ERR(i3cdev)) {
-			ret = PTR_ERR(i3cdev);
-			goto err_detach_devs;
-		}
+		if (IS_ERR(i3cdev))
+			return PTR_ERR(i3cdev);
 
 		i3cdev->boardinfo = i3cboardinfo;
 
 		ret = i3c_master_attach_i3c_dev(master, i3cdev);
 		if (ret) {
 			i3c_master_free_i3c_dev(i3cdev);
-			goto err_detach_devs;
+			return ret;
 		}
 	}
 
+	return 0;
+}
+
+/**
+ * i3c_master_bus_init() - initialize an I3C bus
+ * @master: main master initializing the bus
+ *
+ * This function is following all initialisation steps described in the I3C
+ * specification:
+ *
+ * 1. Attach I2C and statically defined I3C devs to the master so that the
+ *    master can fill its internal device table appropriately
+ *
+ * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
+ *    the master controller. That's usually where the bus mode is selected
+ *    (pure bus or mixed fast/slow bus)
+ *
+ * 3. Instruct all devices on the bus to drop their dynamic address. This is
+ *    particularly important when the bus was previously configured by someone
+ *    else (for example the bootloader)
+ *
+ * 4. Disable all slave events.
+ *
+ * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
+ *    devices that have a static address
+ *
+ * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
+ *    remaining I3C devices
+ *
+ * Once this is done, all I3C and I2C devices should be usable.
+ *
+ * Return: a 0 in case of success, an negative error code otherwise.
+ */
+static int i3c_master_bus_init(struct i3c_master_controller *master)
+{
+	struct i3c_dev_desc *i3cdev;
+	int ret;
+
+	/*
+	 * First attach all devices with static definitions provided by the
+	 * FW.
+	 */
+	if (!master->secondary) {
+		ret = i3c_master_attach_static_devs(master);
+		if (ret)
+			goto err_detach_devs;
+	}
 	/*
 	 * Now execute the controller specific ->bus_init() routine, which
 	 * might configure its internal logic to match the bus limitations.
@@ -1729,6 +1889,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	}
 
 	/*
+	 * Don't reset addresses if this is secondary master.
+	 * Secondary masters can't do DAA.
+	 */
+	if (master->secondary)
+		return 0;
+
+	/*
 	 * Reset all dynamic address that may have been assigned before
 	 * (assigned by the bootloader for example).
 	 */
@@ -1791,6 +1958,49 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
 	return NULL;
 }
 
+int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
+			   u16 addr, u8 lvr)
+{
+	enum i3c_addr_slot_status status;
+	struct i2c_dev_desc *i2cdev;
+	int ret;
+
+	if (!master)
+		return -EINVAL;
+
+	status = i3c_bus_get_addr_slot_status(&master->bus,
+					      addr);
+	if (status != I3C_ADDR_SLOT_FREE)
+		return -EBUSY;
+
+	i3c_bus_set_addr_slot_status(&master->bus, addr,
+				     I3C_ADDR_SLOT_I2C_DEV);
+
+	i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr);
+	if (IS_ERR(i2cdev)) {
+		ret = PTR_ERR(i2cdev);
+		goto err_free_dev;
+	}
+
+	i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr);
+
+	ret = i3c_master_attach_i2c_dev(master, i2cdev);
+	if (ret) {
+		ret = PTR_ERR(i2cdev);
+		goto err_free_dev;
+	}
+
+	return 0;
+
+err_free_dev:
+	i3c_bus_set_addr_slot_status(&master->bus, addr,
+				     I3C_ADDR_SLOT_FREE);
+	i3c_master_free_i2c_dev(i2cdev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev);
+
 /**
  * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
  * @master: master used to send frames on the bus
@@ -2113,11 +2323,17 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
 	}
 
 	i3c_bus_normaluse_lock(&master->bus);
+	ret = i3c_master_acquire_bus_ownership(master);
+	if (ret)
+		goto err_unlock_bus;
+
 	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
 	if (!dev)
 		ret = -ENOENT;
 	else
 		ret = master->ops->i2c_xfers(dev, xfers, nxfers);
+
+err_unlock_bus:
 	i3c_bus_normaluse_unlock(&master->bus);
 
 	return ret ? ret : nxfers;
@@ -2151,8 +2367,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master)
 	 * We silently ignore failures here. The bus should keep working
 	 * correctly even if one or more i2c devices are not registered.
 	 */
-	i3c_bus_for_each_i2cdev(&master->bus, i2cdev)
+	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
+		if (!i2cdev->boardinfo)
+			continue;
+
 		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
+	}
 
 	return 0;
 }
@@ -2393,18 +2613,43 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
 	     !ops->recycle_ibi_slot))
 		return -EINVAL;
 
+	if (ops->request_mastership &&
+	    (!ops->enable_mr_events || !ops->disable_mr_events))
+		return -EINVAL;
+
 	return 0;
 }
 
 /**
+ * i3c_master_register_new_devs() - register new devices
+ * @master: master used to send frames on the bus
+ *
+ * This function is useful when devices were not added
+ * during initialization or when new device joined the bus
+ * which was under control of different master.
+ */
+void i3c_master_register_new_devs(struct i3c_master_controller *master)
+{
+	/*
+	 * We can register I3C devices received from master by DEFSLVS.
+	 */
+	i3c_bus_normaluse_lock(&master->bus);
+	i3c_master_register_new_i3c_devs(master);
+	i3c_bus_normaluse_unlock(&master->bus);
+
+	i3c_bus_normaluse_lock(&master->bus);
+	i3c_master_register_new_i2c_devs(master);
+	i3c_bus_normaluse_unlock(&master->bus);
+}
+EXPORT_SYMBOL_GPL(i3c_master_register_new_devs);
+
+/**
  * i3c_master_register() - register an I3C master
  * @master: master used to send frames on the bus
  * @parent: the parent device (the one that provides this I3C master
  *	    controller)
  * @ops: the master controller operations
- * @secondary: true if you are registering a secondary master. Will return
- *	       -ENOTSUPP if set to true since secondary masters are not yet
- *	       supported
+ * @secondary: true if you are registering a secondary master.
  *
  * This function takes care of everything for you:
  *
@@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller *master,
 	struct i2c_dev_boardinfo *i2cbi;
 	int ret;
 
-	/* We do not support secondary masters yet. */
-	if (secondary)
-		return -ENOTSUPP;
-
 	ret = i3c_master_check_ops(ops);
 	if (ret)
 		return ret;
@@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller *master,
 	 * register I3C devices dicovered during the initial DAA.
 	 */
 	master->init_done = true;
-	i3c_bus_normaluse_lock(&master->bus);
-	i3c_master_register_new_i3c_devs(master);
-	i3c_bus_normaluse_unlock(&master->bus);
+	i3c_master_register_new_devs(master);
+
+	i3c_bus_maintenance_lock(&master->bus);
+	i3c_master_enable_mr_events(master);
+	i3c_bus_maintenance_unlock(&master->bus);
 
 	return 0;
 
@@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller *master,
 EXPORT_SYMBOL_GPL(i3c_master_register);
 
 /**
+ * i3c_master_mastership_ack() - performs operations before bus handover.
+ * @master: master used to send frames on the bus
+ * @addr: I3C device address
+ *
+ * Basically, it sends DEFSLVS command to ensure new master is taking
+ * the bus with complete list of devices and then acknowledges bus takeover.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int i3c_master_mastership_ack(struct i3c_master_controller *master,
+			      u8 addr)
+{
+	int ret;
+
+	i3c_bus_maintenance_lock(&master->bus);
+	ret = i3c_master_get_accmst_locked(master, addr);
+	i3c_bus_maintenance_unlock(&master->bus);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i3c_master_mastership_ack);
+
+
+/**
  * i3c_master_unregister() - unregister an I3C master
  * @master: master used to send frames on the bus
  *
@@ -2533,6 +2800,10 @@ EXPORT_SYMBOL_GPL(i3c_master_register);
  */
 int i3c_master_unregister(struct i3c_master_controller *master)
 {
+	i3c_bus_maintenance_lock(&master->bus);
+	i3c_master_disable_mr_events(master);
+	i3c_bus_maintenance_unlock(&master->bus);
+
 	i3c_master_i2c_adapter_cleanup(master);
 	i3c_master_unregister_i3c_devs(master);
 	i3c_master_bus_cleanup(master);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 42bb215..f32afd3 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -421,6 +421,12 @@ struct i3c_bus {
  *		      for a future IBI
  *		      This method is mandatory only if ->request_ibi is not
  *		      NULL.
+ * @request_mastership: requests bus mastership. Mastership is requested
+ *                      automatically when device driver wants to transfer
+ *                      data using master in slave mode.
+ * @enable_mr_events: enable the Mastership event.
+ *                    Mastership does not require handler.
+ * @disable_mr_events: disable the Mastership event.
  */
 struct i3c_master_controller_ops {
 	int (*bus_init)(struct i3c_master_controller *master);
@@ -447,6 +453,10 @@ struct i3c_master_controller_ops {
 	int (*disable_ibi)(struct i3c_dev_desc *dev);
 	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
 				 struct i3c_ibi_slot *slot);
+	void (*update_devs)(struct i3c_master_controller *master);
+	int (*request_mastership)(struct i3c_master_controller *master);
+	int (*enable_mr_events)(struct i3c_master_controller *master);
+	int (*disable_mr_events)(struct i3c_master_controller *master);
 };
 
 /**
@@ -526,6 +536,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master);
 int i3c_master_get_free_addr(struct i3c_master_controller *master,
 			     u8 start_addr);
 
+int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
+			   u16 addr, u8 lvr);
 int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 				  u8 addr);
 int i3c_master_do_daa(struct i3c_master_controller *master);
@@ -538,6 +550,11 @@ int i3c_master_register(struct i3c_master_controller *master,
 			const struct i3c_master_controller_ops *ops,
 			bool secondary);
 int i3c_master_unregister(struct i3c_master_controller *master);
+int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
+				 u8 addr);
+int i3c_master_mastership_ack(struct i3c_master_controller *master,
+			      u8 addr);
+void i3c_master_register_new_devs(struct i3c_master_controller *master);
 
 /**
  * i3c_dev_get_master_data() - get master private data attached to an I3C
-- 
2.8.3


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

[PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.

[Przemyslaw Gaj]

This patch adds support for mastership request to Cadence I3C master driver.

Mastership is requested automatically after secondary master
receives mastership ENEC event. This allows secondary masters to
initialize their bus.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>

---

Main changes between v3 and v4:
- Refactored the code

Main changes between v2 and v3:
- Add mastership type
- Add postponed master registration
- Add update device definition on bus initialization time
- Removed redundant mastership work structs
- Reworked IBI slot lookup
- Reworked Mastership event enabling/disabling

Changes in v2:
- Add work structs for mastership purpose
- Add missing mastership disable feature
---
 drivers/i3c/master/i3c-master-cdns.c | 495 +++++++++++++++++++++++++++++++----
 1 file changed, 443 insertions(+), 52 deletions(-)

diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index 237f24a..61d6416 100644
--- a/drivers/i3c/master/i3c-master-cdns.c
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -157,6 +157,7 @@
 #define SLV_IMR				0x48
 #define SLV_ICR				0x4c
 #define SLV_ISR				0x50
+#define SLV_INT_DEFSLVS			BIT(21)
 #define SLV_INT_TM			BIT(20)
 #define SLV_INT_ERROR			BIT(19)
 #define SLV_INT_EVENT_UP		BIT(18)
@@ -388,8 +389,19 @@ struct cdns_i3c_xfer {
 	struct cdns_i3c_cmd cmds[0];
 };
 
+enum cdns_i3c_mr {
+	REQUEST,
+	HANDOFF,
+	TAKEOVER
+};
+
 struct cdns_i3c_master {
 	struct work_struct hj_work;
+	struct {
+		struct work_struct work;
+		enum cdns_i3c_mr mr_type;
+		u32 ibir;
+	} mastership;
 	struct i3c_master_controller base;
 	u32 free_rr_slots;
 	unsigned int maxdevs;
@@ -408,6 +420,8 @@ struct cdns_i3c_master {
 	struct clk *pclk;
 	struct cdns_i3c_master_caps caps;
 	unsigned long i3c_scl_lim;
+	struct device *parent;
+	struct workqueue_struct *wq;
 };
 
 static inline struct cdns_i3c_master *
@@ -663,6 +677,88 @@ static void cdns_i3c_master_unqueue_xfer(struct cdns_i3c_master *master,
 	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
 }
 
+static void
+cdns_i3c_master_i3c_dev_rr_to_info(struct cdns_i3c_master *master,
+				   unsigned int slot,
+				   struct i3c_device_info *info)
+{
+	u32 rr;
+
+	memset(info, 0, sizeof(*info));
+	rr = readl(master->regs + DEV_ID_RR0(slot));
+	info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
+	rr = readl(master->regs + DEV_ID_RR2(slot));
+	info->dcr = rr;
+	info->bcr = rr >> 8;
+	info->pid = rr >> 16;
+	info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16;
+}
+
+static void
+cdns_i3c_master_i2c_dev_rr_to_info(struct cdns_i3c_master *master,
+				   unsigned int slot,
+				   u16 *addr, u8 *lvr)
+{
+	u32 rr;
+
+	rr = readl(master->regs + DEV_ID_RR0(slot));
+	*addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
+	rr = readl(master->regs + DEV_ID_RR2(slot));
+	*lvr = rr;
+}
+
+static
+int cdns_i3c_sec_master_request_mastership(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	u32 status;
+	int ret;
+
+	status = readl(master->regs + MST_STATUS0);
+	if (WARN_ON(status & MST_STATUS0_MASTER_MODE))
+		return -EEXIST;
+
+	status = readl(master->regs + SLV_STATUS1);
+	if (status & SLV_STATUS1_MR_DIS)
+		return -EACCES;
+
+	writel(SLV_INT_MR_DONE, master->regs + SLV_IER);
+	writel(readl(master->regs + CTRL) | CTRL_MST_INIT | CTRL_MST_ACK,
+	       master->regs + CTRL);
+
+	ret = readl_poll_timeout(master->regs + MST_STATUS0, status,
+				 status & MST_STATUS0_MASTER_MODE, 100,
+				 100000);
+	return ret;
+}
+
+static void cdns_i3c_master_update_devs(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	u32 val, newdevs;
+	u16 addr;
+	u8 lvr;
+	int slot;
+	struct i3c_device_info i3c_info;
+
+	newdevs = readl(master->regs + DEVS_CTRL) & DEVS_CTRL_DEVS_ACTIVE_MASK;
+	for (slot = 1; slot <= master->maxdevs; slot++) {
+		val = readl(master->regs + DEV_ID_RR0(slot));
+
+		if ((newdevs & BIT(slot)) && (val & DEV_ID_RR0_IS_I3C)) {
+			cdns_i3c_master_i3c_dev_rr_to_info(master, slot,
+							   &i3c_info);
+
+			i3c_master_add_i3c_dev_locked(m, i3c_info.dyn_addr);
+		} else if ((newdevs & BIT(slot)) &&
+			   !(val & DEV_ID_RR0_IS_I3C)) {
+			cdns_i3c_master_i2c_dev_rr_to_info(master, slot,
+							   &addr, &lvr);
+			i3c_master_add_i2c_dev(m, addr, lvr);
+		}
+	}
+}
+
 static enum i3c_error_code cdns_i3c_cmd_get_err(struct cdns_i3c_cmd *cmd)
 {
 	switch (cmd->error) {
@@ -913,6 +1009,7 @@ static int cdns_i3c_master_get_rr_slot(struct cdns_i3c_master *master,
 		return ffs(master->free_rr_slots) - 1;
 	}
 
+
 	activedevs = readl(master->regs + DEVS_CTRL) &
 		     DEVS_CTRL_DEVS_ACTIVE_MASK;
 
@@ -1005,9 +1102,9 @@ static int cdns_i3c_master_attach_i2c_dev(struct i2c_dev_desc *dev)
 	master->free_rr_slots &= ~BIT(slot);
 	i2c_dev_set_master_data(dev, data);
 
-	writel(prepare_rr0_dev_address(dev->boardinfo->base.addr),
+	writel(prepare_rr0_dev_address(dev->addr),
 	       master->regs + DEV_ID_RR0(data->id));
-	writel(dev->boardinfo->lvr, master->regs + DEV_ID_RR2(data->id));
+	writel(dev->lvr, master->regs + DEV_ID_RR2(data->id));
 	writel(readl(master->regs + DEVS_CTRL) |
 	       DEVS_CTRL_DEV_ACTIVE(data->id),
 	       master->regs + DEVS_CTRL);
@@ -1037,22 +1134,6 @@ static void cdns_i3c_master_bus_cleanup(struct i3c_master_controller *m)
 	cdns_i3c_master_disable(master);
 }
 
-static void cdns_i3c_master_dev_rr_to_info(struct cdns_i3c_master *master,
-					   unsigned int slot,
-					   struct i3c_device_info *info)
-{
-	u32 rr;
-
-	memset(info, 0, sizeof(*info));
-	rr = readl(master->regs + DEV_ID_RR0(slot));
-	info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
-	rr = readl(master->regs + DEV_ID_RR2(slot));
-	info->dcr = rr;
-	info->bcr = rr >> 8;
-	info->pid = rr >> 16;
-	info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16;
-}
-
 static void cdns_i3c_master_upd_i3c_scl_lim(struct cdns_i3c_master *master)
 {
 	struct i3c_master_controller *m = &master->base;
@@ -1180,10 +1261,6 @@ static int cdns_i3c_master_do_daa(struct i3c_master_controller *m)
 
 	cdns_i3c_master_upd_i3c_scl_lim(master);
 
-	/* Unmask Hot-Join and Mastership request interrupts. */
-	i3c_master_enec_locked(m, I3C_BROADCAST_ADDR,
-			       I3C_CCC_EVENT_HJ | I3C_CCC_EVENT_MR);
-
 	return 0;
 }
 
@@ -1247,15 +1324,17 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
 	prescl1 = PRESCL_CTRL1_OD_LOW(ncycles);
 	writel(prescl1, master->regs + PRESCL_CTRL1);
 
-	/* Get an address for the master. */
-	ret = i3c_master_get_free_addr(m, 0);
-	if (ret < 0)
-		return ret;
+	if (!m->secondary) {
+		/* Get an address for the master. */
+		ret = i3c_master_get_free_addr(m, 0);
+		if (ret < 0)
+			return ret;
 
-	writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
-	       master->regs + DEV_ID_RR0(0));
+		writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
+		       master->regs + DEV_ID_RR0(0));
+	}
 
-	cdns_i3c_master_dev_rr_to_info(master, 0, &info);
+	cdns_i3c_master_i3c_dev_rr_to_info(master, 0, &info);
 	if (info.bcr & I3C_BCR_HDR_CAP)
 		info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR);
 
@@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
 
 	cdns_i3c_master_enable(master);
 
+	if (m->secondary) {
+		i3c_bus_maintenance_lock(&master->base.bus);
+		cdns_i3c_master_update_devs(&master->base);
+		i3c_bus_maintenance_unlock(&master->base.bus);
+	}
+
 	return 0;
 }
 
+static
+struct i3c_dev_desc *cdns_i3c_get_ibi_device(struct cdns_i3c_master *master,
+					     u32 ibir)
+{
+	struct i3c_dev_desc *dev;
+	u32 id = IBIR_SLVID(ibir);
+
+	if (id >= master->ibi.num_slots || (ibir & IBIR_ERROR))
+		return NULL;
+
+	dev = master->ibi.slots[id];
+	if (!dev)
+		return NULL;
+
+	return dev;
+}
+
 static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
 				       u32 ibir)
 {
@@ -1354,27 +1456,100 @@ static void cnds_i3c_master_demux_ibis(struct cdns_i3c_master *master)
 
 		case IBIR_TYPE_MR:
 			WARN_ON(IBIR_XFER_BYTES(ibir) || (ibir & IBIR_ERROR));
+			master->mastership.ibir = ibir;
+			master->mastership.mr_type = HANDOFF;
+			queue_work(master->base.wq,
+				   &master->mastership.work);
+			break;
 		default:
 			break;
 		}
 	}
 }
 
+static void cdns_i3c_master_bus_handoff(struct cdns_i3c_master *master)
+{
+	struct i3c_dev_desc *dev;
+
+	dev = cdns_i3c_get_ibi_device(master, master->mastership.ibir);
+
+	writel(MST_INT_MR_DONE, master->regs + MST_ICR);
+
+	master->base.bus.cur_master = dev;
+}
+
+static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
+{
+	if (master->base.init_done) {
+		i3c_bus_maintenance_lock(&master->base.bus);
+		cdns_i3c_master_update_devs(&master->base);
+		i3c_bus_maintenance_unlock(&master->base.bus);
+
+		i3c_master_register_new_devs(&master->base);
+	}
+
+	writel(readl(master->regs + CTRL) & ~CTRL_MST_ACK, master->regs + CTRL);
+}
+
+static void cdns_i3c_sec_master_event_up(struct cdns_i3c_master *master)
+{
+	u32 status;
+
+	writel(SLV_INT_EVENT_UP, master->regs + SLV_ICR);
+	status = readl(master->regs + SLV_STATUS1);
+	if (!(status & SLV_STATUS1_MR_DIS) &&
+	    !master->base.this) {
+		master->mastership.mr_type = REQUEST;
+		queue_work(master->wq, &master->mastership.work);
+	}
+}
+
+static void cdns_i3c_sec_mastership_done(struct cdns_i3c_master *master)
+{
+	writel(SLV_INT_MR_DONE, master->regs + SLV_ICR);
+
+	master->base.bus.cur_master = master->base.this;
+
+	if (master->base.this) {
+		master->mastership.mr_type = TAKEOVER;
+		queue_work(master->base.wq, &master->mastership.work);
+	}
+}
+
 static irqreturn_t cdns_i3c_master_interrupt(int irq, void *data)
 {
 	struct cdns_i3c_master *master = data;
 	u32 status;
 
-	status = readl(master->regs + MST_ISR);
-	if (!(status & readl(master->regs + MST_IMR)))
-		return IRQ_NONE;
+	status = readl(master->regs + MST_STATUS0);
+
+	if (!master->base.this ||
+	    master->base.this != master->base.bus.cur_master) {
+		status = readl(master->regs + SLV_ISR);
+
+		if (!(status & readl(master->regs + SLV_IMR)))
+			return IRQ_NONE;
+
+		if (status & SLV_INT_MR_DONE)
+			cdns_i3c_sec_mastership_done(master);
 
-	spin_lock(&master->xferqueue.lock);
-	cdns_i3c_master_end_xfer_locked(master, status);
-	spin_unlock(&master->xferqueue.lock);
+		if (status & SLV_INT_EVENT_UP)
+			cdns_i3c_sec_master_event_up(master);
+	} else {
+		status = readl(master->regs + MST_ISR);
+		if (!(status & readl(master->regs + MST_IMR)))
+			return IRQ_NONE;
+
+		spin_lock(&master->xferqueue.lock);
+		cdns_i3c_master_end_xfer_locked(master, status);
+		spin_unlock(&master->xferqueue.lock);
 
-	if (status & MST_INT_IBIR_THR)
-		cnds_i3c_master_demux_ibis(master);
+		if (status & MST_INT_IBIR_THR)
+			cnds_i3c_master_demux_ibis(master);
+
+		if (status & MST_INT_MR_DONE)
+			cdns_i3c_master_bus_handoff(master);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -1443,30 +1618,54 @@ static int cdns_i3c_master_enable_ibi(struct i3c_dev_desc *dev)
 	return ret;
 }
 
-static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev,
-				       const struct i3c_ibi_setup *req)
+static int cdns_i3c_master_find_ibi_slot(struct cdns_i3c_master *master,
+					 struct i3c_dev_desc *dev,
+					 s16 *slot)
 {
-	struct i3c_master_controller *m = i3c_dev_get_master(dev);
-	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
-	struct cdns_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	unsigned long flags;
 	unsigned int i;
-
-	data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req);
-	if (IS_ERR(data->ibi_pool))
-		return PTR_ERR(data->ibi_pool);
+	int ret = -ENOENT;
 
 	spin_lock_irqsave(&master->ibi.lock, flags);
 	for (i = 0; i < master->ibi.num_slots; i++) {
-		if (!master->ibi.slots[i]) {
-			data->ibi = i;
-			master->ibi.slots[i] = dev;
+		/*
+		 * We only need 'SIR' slots to describe IBI-capable devices.
+		 * This slot may be used by mastership request interrupt,
+		 * We can ruse the same 'SIR' map entry.
+		 */
+		if (master->ibi.slots[i] == dev) {
+			*slot = i;
+			ret = 0;
 			break;
 		}
 	}
+
+	if (ret)
+		for (i = 0; i < master->ibi.num_slots; i++) {
+			if (!master->ibi.slots[i]) {
+				master->ibi.slots[i] = dev;
+				*slot = i;
+				ret = 0;
+				break;
+			}
+		}
 	spin_unlock_irqrestore(&master->ibi.lock, flags);
 
-	if (i < master->ibi.num_slots)
+	return ret;
+}
+
+static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev,
+				       const struct i3c_ibi_setup *req)
+{
+	struct i3c_master_controller *m = i3c_dev_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+
+	data->ibi_pool = i3c_generic_ibi_alloc_pool(dev, req);
+	if (IS_ERR(data->ibi_pool))
+		return PTR_ERR(data->ibi_pool);
+
+	if (cdns_i3c_master_find_ibi_slot(master, dev, &data->ibi) == 0)
 		return 0;
 
 	i3c_generic_ibi_free_pool(data->ibi_pool);
@@ -1475,6 +1674,51 @@ static int cdns_i3c_master_request_ibi(struct i3c_dev_desc *dev,
 	return -ENOSPC;
 }
 
+static int
+cdns_i3c_master_enable_mastership_events(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data;
+	struct i3c_dev_desc *i3cdev;
+	unsigned long flags;
+	u32 sircfg, sirmap;
+	int ret;
+
+	i3c_bus_for_each_i3cdev(&m->bus, i3cdev) {
+		if (I3C_BCR_DEVICE_ROLE(i3cdev->info.bcr) != I3C_BCR_I3C_MASTER)
+			continue;
+
+		data = i3c_dev_get_master_data(i3cdev);
+		if (!data)
+			continue;
+
+		ret = cdns_i3c_master_find_ibi_slot(master, i3cdev, &data->ibi);
+		if (ret)
+			continue;
+
+		spin_lock_irqsave(&master->ibi.lock, flags);
+		sirmap = readl(master->regs + SIR_MAP_DEV_REG(data->ibi));
+		sirmap &= ~SIR_MAP_DEV_CONF_MASK(data->ibi);
+		sircfg = SIR_MAP_DEV_ROLE(i3cdev->info.bcr >> 6) |
+			SIR_MAP_DEV_DA(i3cdev->info.dyn_addr) |
+			SIR_MAP_DEV_PL(i3cdev->info.max_ibi_len) |
+			SIR_MAP_DEV_ACK;
+
+		if (i3cdev->info.bcr & I3C_BCR_MAX_DATA_SPEED_LIM)
+			sircfg |= SIR_MAP_DEV_SLOW;
+
+		sirmap |= SIR_MAP_DEV_CONF(data->ibi, sircfg);
+		writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
+		spin_unlock_irqrestore(&master->ibi.lock, flags);
+	}
+
+	/* Unmask Hot-Join and Mastership request interrupts. */
+	i3c_master_enec_locked(&master->base, I3C_BROADCAST_ADDR,
+			       I3C_CCC_EVENT_HJ | I3C_CCC_EVENT_MR);
+
+	return 0;
+}
+
 static void cdns_i3c_master_free_ibi(struct i3c_dev_desc *dev)
 {
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
@@ -1490,6 +1734,52 @@ static void cdns_i3c_master_free_ibi(struct i3c_dev_desc *dev)
 	i3c_generic_ibi_free_pool(data->ibi_pool);
 }
 
+static int
+cdns_i3c_master_disable_mastership_events(struct i3c_master_controller *m)
+{
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	struct cdns_i3c_i2c_dev_data *data;
+	struct i3c_dev_desc *i3cdev;
+	unsigned long flags;
+	u32 sirmap;
+	int ret;
+
+	ret = i3c_master_disec_locked(m, I3C_BROADCAST_ADDR,
+				      I3C_CCC_EVENT_MR);
+	if (ret)
+		return ret;
+
+	i3c_bus_for_each_i3cdev(&m->bus, i3cdev) {
+		if (I3C_BCR_DEVICE_ROLE(i3cdev->info.bcr) != I3C_BCR_I3C_MASTER)
+			continue;
+
+		data = i3c_dev_get_master_data(i3cdev);
+
+		ret = cdns_i3c_master_find_ibi_slot(master, i3cdev, &data->ibi);
+		if (ret)
+			continue;
+
+		/*
+		 * Do not modify SIR register and cleanup slots
+		 * if regular IBI is enabled for this device.
+		 */
+		if (master->ibi.slots[data->ibi]->ibi->handler)
+			continue;
+
+		spin_lock_irqsave(&master->ibi.lock, flags);
+		sirmap = readl(master->regs + SIR_MAP_DEV_REG(data->ibi));
+		sirmap &= ~SIR_MAP_DEV_CONF_MASK(data->ibi);
+		sirmap |= SIR_MAP_DEV_CONF(data->ibi,
+					SIR_MAP_DEV_DA(I3C_BROADCAST_ADDR));
+		writel(sirmap, master->regs + SIR_MAP_DEV_REG(data->ibi));
+		spin_unlock_irqrestore(&master->ibi.lock, flags);
+
+		cdns_i3c_master_free_ibi(i3cdev);
+	}
+
+	return ret;
+}
+
 static void cdns_i3c_master_recycle_ibi_slot(struct i3c_dev_desc *dev,
 					     struct i3c_ibi_slot *slot)
 {
@@ -1516,6 +1806,9 @@ static const struct i3c_master_controller_ops cdns_i3c_master_ops = {
 	.request_ibi = cdns_i3c_master_request_ibi,
 	.free_ibi = cdns_i3c_master_free_ibi,
 	.recycle_ibi_slot = cdns_i3c_master_recycle_ibi_slot,
+	.request_mastership = cdns_i3c_sec_master_request_mastership,
+	.enable_mr_events = cdns_i3c_master_enable_mastership_events,
+	.disable_mr_events = cdns_i3c_master_disable_mastership_events
 };
 
 static void cdns_i3c_master_hj(struct work_struct *work)
@@ -1527,10 +1820,80 @@ static void cdns_i3c_master_hj(struct work_struct *work)
 	i3c_master_do_daa(&master->base);
 }
 
+static int cdns_i3c_sec_master_bus_init(struct cdns_i3c_master *master)
+{
+	u32 val;
+	int ret;
+
+	val = readl(master->regs + SLV_STATUS1);
+
+	if (!(val & SLV_STATUS1_HAS_DA))
+		return -EFAULT;
+
+	i3c_bus_maintenance_lock(&master->base.bus);
+	ret = cdns_i3c_sec_master_request_mastership(&master->base);
+	i3c_bus_maintenance_unlock(&master->base.bus);
+
+	return ret;
+}
+
+static void
+cdns_i3c_master_mastership_request(struct cdns_i3c_master *master)
+{
+	int ret;
+
+	ret = cdns_i3c_sec_master_bus_init(master);
+	if (ret)
+		return;
+
+	ret = i3c_master_register(&master->base, master->parent,
+				  &cdns_i3c_master_ops, true);
+	if (ret)
+		dev_err(&master->base.dev, "Master register failed\n");
+}
+
+static void cdns_i3c_master_mastership_handoff(struct cdns_i3c_master *master)
+{
+	int ret;
+
+	struct i3c_dev_desc *dev;
+	u32 ibir = master->mastership.ibir;
+
+	dev = cdns_i3c_get_ibi_device(master, ibir);
+	if (!dev)
+		return;
+
+	ret = i3c_master_mastership_ack(&master->base, dev->info.dyn_addr);
+	if (ret)
+		dev_err(&master->base.dev, "Mastership handoff failed\n");
+}
+
+static void cdns_i3c_master_mastership(struct work_struct *work)
+{
+	struct cdns_i3c_master *master = container_of(work,
+						      struct cdns_i3c_master,
+						      mastership.work);
+
+	switch (master->mastership.mr_type) {
+	case REQUEST:
+		cdns_i3c_master_mastership_request(master);
+		break;
+	case HANDOFF:
+		cdns_i3c_master_mastership_handoff(master);
+		break;
+	case TAKEOVER:
+		cdns_i3c_master_mastership_takeover(master);
+		break;
+	default:
+		break;
+	}
+}
+
 static int cdns_i3c_master_probe(struct platform_device *pdev)
 {
 	struct cdns_i3c_master *master;
 	struct resource *res;
+	bool secondary = false;
 	int ret, irq;
 	u32 val;
 
@@ -1572,6 +1935,9 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&master->xferqueue.list);
 
 	INIT_WORK(&master->hj_work, cdns_i3c_master_hj);
+	INIT_WORK(&master->mastership.work,
+		  cdns_i3c_master_mastership);
+
 	writel(0xffffffff, master->regs + MST_IDR);
 	writel(0xffffffff, master->regs + SLV_IDR);
 	ret = devm_request_irq(&pdev->dev, irq, cdns_i3c_master_interrupt, 0,
@@ -1581,6 +1947,10 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, master);
 
+	val = readl(master->regs + MST_STATUS0);
+	if (!(val & MST_STATUS0_MASTER_MODE))
+		secondary = true;
+
 	val = readl(master->regs + CONF_STATUS0);
 
 	/* Device ID0 is reserved to describe this master. */
@@ -1596,6 +1966,7 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
 
 	spin_lock_init(&master->ibi.lock);
 	master->ibi.num_slots = CONF_STATUS1_IBI_HW_RES(val);
+
 	master->ibi.slots = devm_kcalloc(&pdev->dev, master->ibi.num_slots,
 					 sizeof(*master->ibi.slots),
 					 GFP_KERNEL);
@@ -1604,15 +1975,31 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
 
 	writel(IBIR_THR(1), master->regs + CMD_IBI_THR_CTRL);
 	writel(MST_INT_IBIR_THR, master->regs + MST_IER);
-	writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
+
+	if (secondary) {
+		ret = cdns_i3c_sec_master_bus_init(master);
+		if (ret)
+			goto err_postpone_init;
+	} else
+		writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
 
 	ret = i3c_master_register(&master->base, &pdev->dev,
-				  &cdns_i3c_master_ops, false);
+				  &cdns_i3c_master_ops, secondary);
 	if (ret)
 		goto err_disable_sysclk;
 
 	return 0;
 
+err_postpone_init:
+	master->wq = alloc_workqueue("%s", 0, 0, pdev->name);
+	if (!master->wq)
+		return -ENOMEM;
+
+	master->parent = &pdev->dev;
+	writel(SLV_INT_EVENT_UP, master->regs + SLV_IER);
+
+	return 0;
+
 err_disable_sysclk:
 	clk_disable_unprepare(master->sysclk);
 
@@ -1627,6 +2014,9 @@ static int cdns_i3c_master_remove(struct platform_device *pdev)
 	struct cdns_i3c_master *master = platform_get_drvdata(pdev);
 	int ret;
 
+	if (master->wq)
+		destroy_workqueue(master->wq);
+
 	ret = i3c_master_unregister(&master->base);
 	if (ret)
 		return ret;
@@ -1653,6 +2043,7 @@ static struct platform_driver cdns_i3c_master = {
 module_platform_driver(cdns_i3c_master);
 
 MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
+MODULE_AUTHOR("Przemyslaw Gaj <pgaj@cadence.com>");
 MODULE_DESCRIPTION("Cadence I3C master driver");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:cdns-i3c-master");
-- 
2.8.3


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

[PATCH v4 5/6] i3c: master: Add module author

[Przemyslaw Gaj]

This adds myself as an author of I3C framework.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
 drivers/i3c/master.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 7a84158..8d68001 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2018 Cadence Design Systems Inc.
  *
  * Author: Boris Brezillon <boris.brezillon@bootlin.com>
+ * Author: Przemyslaw Gaj <pgaj@cadence.com>
  */
 
 #include <linux/atomic.h>
@@ -2931,5 +2932,6 @@ static void __exit i3c_exit(void)
 module_exit(i3c_exit);
 
 MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>");
+MODULE_AUTHOR("Przemyslaw Gaj <pgaj@cadence.com>");
 MODULE_DESCRIPTION("I3C core");
 MODULE_LICENSE("GPL v2");
-- 
2.8.3


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

[PATCH v4 6/6] MAINTAINERS: add myself as co-maintainer of i3c subsystem

[Przemyslaw Gaj]

As discussed with Boris Brezillon - I'm adding myself as the co-maintainer.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dce5c09..a5003bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7175,6 +7175,7 @@ F:	drivers/i2c/i2c-stub.c
 
 I3C SUBSYSTEM
 M:	Boris Brezillon <bbrezillon@kernel.org>
+M:	Przemyslaw Gaj <pgaj@cadence.com>
 L:	linux-i3c@lists.infradead.org
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux.git
 S:	Maintained
-- 
2.8.3


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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Boris Brezillon]

On Sun, 10 Mar 2019 13:58:38 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> I need to store address and lvr value for I2C devices without static definition
> in DT. This allows secondary master to transmit DEFSLVS command properly.
> 
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> ---
>  include/linux/i3c/master.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index eca8337..3c27d9f 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
>   * @common: common part of the I2C device descriptor
>   * @boardinfo: pointer to the boardinfo attached to this I2C device
>   * @dev: I2C device object registered to the I2C framework
> + * @addr: I2C device address
> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> + *	 the I2C device limitations
>   *
>   * Each I2C device connected on the bus will have an i2c_dev_desc.
>   * This object is created by the core and later attached to the controller
> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
>  	struct i3c_i2c_dev_desc common;
>  	const struct i2c_dev_boardinfo *boardinfo;
>  	struct i2c_client *dev;
> +	u16 addr;
> +	u8 lvr;

You also need to remove lvr from i2c_dev_boardinfo and adjust the code
to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
patch 3.

>  };
>  
>  /**


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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon]

Hi Przemek,

Sorry for the late review.

On Sun, 10 Mar 2019 13:58:40 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> This patch adds support for mastership request to I3C subsystem.
> 
> Mastership event is enabled globally.
> 
> Mastership is requested automatically when device driver
> tries to transfer data using master controller in slave mode.
> 
> There is still some limitation:
> - I2C devices are registered on secondary master side if boardinfo
> entry matching the info transmitted through the DEFSLVS frame.
> 
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> 
> ---
> 
> Main changes between v3 and v4:
> - Add i3c_master_acquire_bus_ownership to acquire the bus
> - Refactored the code
> 
> Main changes between v2 and v3:
> - Add i3c_bus_downgrade_maintenance_lock() for downgrading the bus
> lock from maintenance to normal use
> - Add additional fields to i2c_dev_desc for DEFSLVS purpose (addr, lvr)
> - Add i3c_master_register_new_i2c_devs() function to register I2C devices
> - Reworked I2C devices registration on secondary master side
> 
> Changes in v2:
> - Add mastership disable event hook
> - Changed name of mastership enable event hook
> - Add function to test if master owns the bus
> - Add function to test if master owns the bus
> - Removed op_mode
> - Changed parameter of i3c_master_get_accmst_locked, no need to
> pass full i3c_device_info
> - Changed name of mastership enable event hook
> - Add function to test if master owns the bus
> - Removed op_mode
> - Changed parameter of i3c_master_get_accmst_locked, no need to
> pass full i3c_device_info
> - Removed redundant DEFSLVS command before GETACCMST
> - Add i3c_master_bus_takeover function. There is a need to lock
> the bus before adding devices and no matter of the controller
> devices have to be added after mastership takeover.
> - Add device registration during initialization on secondary master
> side. Devices received by DEFSLVS (if occured). If not, device
> initialization is deffered untill next mastership request.
> ---
>  drivers/i3c/device.c       |  26 +++
>  drivers/i3c/internals.h    |   4 +
>  drivers/i3c/master.c       | 417 +++++++++++++++++++++++++++++++++++++--------
>  include/linux/i3c/master.h |  17 ++
>  4 files changed, 391 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index 69cc040..b60f637 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -43,7 +43,13 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
>  	}
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  
>  	return ret;
> @@ -114,11 +120,17 @@ int i3c_device_enable_ibi(struct i3c_device *dev)
>  	int ret = -ENOENT;
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	if (dev->desc) {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		ret = i3c_dev_enable_ibi_locked(dev->desc);
>  		mutex_unlock(&dev->desc->ibi_lock);
>  	}
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  
>  	return ret;
> @@ -145,11 +157,17 @@ int i3c_device_request_ibi(struct i3c_device *dev,
>  		return -EINVAL;
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	if (dev->desc) {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		ret = i3c_dev_request_ibi_locked(dev->desc, req);
>  		mutex_unlock(&dev->desc->ibi_lock);
>  	}
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  
>  	return ret;
> @@ -166,12 +184,20 @@ EXPORT_SYMBOL_GPL(i3c_device_request_ibi);
>   */
>  void i3c_device_free_ibi(struct i3c_device *dev)
>  {
> +	int ret;
> +
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	if (dev->desc) {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		i3c_dev_free_ibi_locked(dev->desc);
>  		mutex_unlock(&dev->desc->ibi_lock);
>  	}
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  }
>  EXPORT_SYMBOL_GPL(i3c_device_free_ibi);
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 86b7b44..929ca6b 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type;
>  
>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus);
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master);
> +int i3c_master_request_mastership_locked(struct i3c_master_controller *master);
> +
>  
>  int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>  				 struct i3c_priv_xfer *xfers,
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index aea4309..7a84158 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
>  	up_read(&bus->lock);
>  }
>  
> +/**
> + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal
> + * operation
> + * @bus: I3C bus to downgrade the lock on
> + *
> + * Should be called when a maintenance operation is done and normal
> + * operation is planned. See i3c_bus_maintenance_lock() and
> + * i3c_bus_normaluse_lock() for more details.
> + */
> +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus)

This function is only used in this file, please make it static and
don't expose it in internals.h. Also, the comment above the function
should not be part of the doc, as its supposed to be a private
function. Just make it a normal comment (/* instead of /**) and don't
document the input param.

> +{
> +	downgrade_write(&bus->lock);
> +}
> +
>  static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
>  {
>  	return container_of(dev, struct i3c_master_controller, dev);
> @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev)
>  	return driver->probe(i3cdev);
>  }
>  
> +static void i3c_master_enable_mr_events(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->enable_mr_events)
> +		return;
> +
> +	master->ops->enable_mr_events(master);
> +}
> +
> +static void i3c_master_disable_mr_events(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->disable_mr_events)
> +		return;
> +
> +	master->ops->disable_mr_events(master);
> +}
> +
>  static int i3c_device_remove(struct device *dev)
>  {
>  	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> @@ -462,6 +492,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
>  	return 0;
>  }
>  
> +int i3c_master_request_mastership_locked(struct i3c_master_controller *master)
> +{
> +	if (WARN_ON(master->init_done &&
> +	    !rwsem_is_locked(&master->bus.lock)))
> +		return -EINVAL;
> +
> +	if (!master->ops->request_mastership)
> +		return -ENOTSUPP;
> +
> +	return master->ops->request_mastership(master);
> +}

Same goes for this function. Please make it private.

> +
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master)
> +{
> +	int ret;
> +
> +	if (master->bus.cur_master != master->this) {
> +		i3c_bus_normaluse_unlock(&master->bus);
> +		i3c_bus_maintenance_lock(&master->bus);
> +
> +		ret = i3c_master_request_mastership_locked(master);
> +		if (ret) {
> +			i3c_bus_maintenance_unlock(&master->bus);
> +			return ret;
> +		}
> +		i3c_bus_downgrade_maintenance_lock(&master->bus);
> +	}
> +	return 0;
> +}
> +
>  static const char * const i3c_bus_mode_strings[] = {
>  	[I3C_BUS_MODE_PURE] = "pure",
>  	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> @@ -620,6 +680,25 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master,
>  
>  	dev->common.master = master;
>  	dev->boardinfo = boardinfo;
> +	dev->addr = boardinfo->base.addr;
> +	dev->lvr = boardinfo->lvr;
> +
> +	return dev;
> +}

This code should be part of patch 1.

> +
> +static struct i2c_dev_desc *
> +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master,
> +				      u16 addr, u8 lvr)
> +{
> +	struct i2c_dev_desc *dev;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev->common.master = master;
> +	dev->addr = addr;
> +	dev->lvr = lvr;
>  
>  	return dev;
>  }
> @@ -693,6 +772,9 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
>  	struct i2c_dev_desc *dev;
>  
>  	i3c_bus_for_each_i2cdev(&master->bus, dev) {
> +		if (!dev->boardinfo)
> +			continue;
> +
>  		if (dev->boardinfo->base.addr == addr)
>  			return dev;
>  	}
> @@ -939,8 +1021,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master)
>  
>  	desc = defslvs->slaves;
>  	i3c_bus_for_each_i2cdev(bus, i2cdev) {
> -		desc->lvr = i2cdev->boardinfo->lvr;
> -		desc->static_addr = i2cdev->boardinfo->base.addr << 1;
> +		desc->lvr = i2cdev->lvr;
> +		desc->static_addr = i2cdev->addr << 1;
>  		desc++;
>  	}
>  
> @@ -1492,6 +1574,83 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  	}
>  }
>  
> +static struct i2c_dev_boardinfo *
> +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master,
> +			      u16 addr, u8 lvr)
> +{
> +	struct i2c_dev_boardinfo *i2cboardinfo;
> +
> +	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
> +		if (i2cboardinfo->base.addr == addr &&
> +		    i2cboardinfo->lvr == lvr)
> +			return i2cboardinfo;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void
> +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master)
> +{
> +	struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master);
> +	struct i2c_dev_desc *i2cdev;
> +
> +	if (!master->init_done)
> +		return;
> +
> +	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +		if (i2cdev->dev)
> +			continue;
> +
> +		if (!i2cdev->boardinfo)
> +			continue;
> +
> +		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
> +	}
> +}
> +
> +/**
> + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command
> + * @master: master used to send frames on the bus
> + * @info: I3C device information
> + *
> + * Send a GETACCMST CCC command.
> + *
> + * This should be called if the curent master acknowledges bus takeover.
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: 0 in case of success, a positive I3C error code if the error is
> + * one of the official Mx error codes, and a negative error code otherwise.
> + */
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +				 u8 addr)

Looks like you call the i3c_master_mastership_ack() wrapper in the
cadence driver, so please keep this function private.

> +{
> +	struct i3c_ccc_getaccmst *accmst;
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> +	if (!accmst)
> +		return -ENOMEM;
> +
> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	if (ret)
> +		goto out;
> +
> +	if (dest.payload.len != sizeof(*accmst))
> +		ret = -EIO;
> +
> +out:
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
> +
>  /**
>   * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
>   * @master: master doing the DAA
> @@ -1559,10 +1718,6 @@ int i3c_master_set_info(struct i3c_master_controller *master,
>  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
>  		return -EINVAL;
>  
> -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> -	    master->secondary)
> -		return -EINVAL;
> -
>  	if (master->this)
>  		return -EINVAL;
>  
> @@ -1607,43 +1762,13 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
>  				 common.node) {
>  		i3c_master_detach_i2c_dev(i2cdev);
>  		i3c_bus_set_addr_slot_status(&master->bus,
> -					i2cdev->boardinfo->base.addr,
> -					I3C_ADDR_SLOT_FREE);
> +					     i2cdev->addr,
> +					     I3C_ADDR_SLOT_FREE);

Should be part of patch 1.

>  		i3c_master_free_i2c_dev(i2cdev);
>  	}
>  }
>  
> -/**
> - * i3c_master_bus_init() - initialize an I3C bus
> - * @master: main master initializing the bus
> - *
> - * This function is following all initialisation steps described in the I3C
> - * specification:
> - *
> - * 1. Attach I2C and statically defined I3C devs to the master so that the
> - *    master can fill its internal device table appropriately
> - *
> - * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
> - *    the master controller. That's usually where the bus mode is selected
> - *    (pure bus or mixed fast/slow bus)
> - *
> - * 3. Instruct all devices on the bus to drop their dynamic address. This is
> - *    particularly important when the bus was previously configured by someone
> - *    else (for example the bootloader)
> - *
> - * 4. Disable all slave events.
> - *
> - * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> - *    devices that have a static address
> - *
> - * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> - *    remaining I3C devices
> - *
> - * Once this is done, all I3C and I2C devices should be usable.
> - *
> - * Return: a 0 in case of success, an negative error code otherwise.
> - */
> -static int i3c_master_bus_init(struct i3c_master_controller *master)
> +static int i3c_master_attach_static_devs(struct i3c_master_controller *master)
>  {
>  	enum i3c_addr_slot_status status;
>  	struct i2c_dev_boardinfo *i2cboardinfo;
> @@ -1652,32 +1777,24 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	struct i2c_dev_desc *i2cdev;
>  	int ret;
>  
> -	/*
> -	 * First attach all devices with static definitions provided by the
> -	 * FW.
> -	 */
>  	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
>  		status = i3c_bus_get_addr_slot_status(&master->bus,
>  						      i2cboardinfo->base.addr);
> -		if (status != I3C_ADDR_SLOT_FREE) {
> -			ret = -EBUSY;
> -			goto err_detach_devs;
> -		}
> +		if (status != I3C_ADDR_SLOT_FREE)
> +			return -EBUSY;
>  
>  		i3c_bus_set_addr_slot_status(&master->bus,
>  					     i2cboardinfo->base.addr,
>  					     I3C_ADDR_SLOT_I2C_DEV);
>  
>  		i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo);
> -		if (IS_ERR(i2cdev)) {
> -			ret = PTR_ERR(i2cdev);
> -			goto err_detach_devs;
> -		}
> +		if (IS_ERR(i2cdev))
> +			return PTR_ERR(i2cdev);
>  
>  		ret = i3c_master_attach_i2c_dev(master, i2cdev);
>  		if (ret) {
>  			i3c_master_free_i2c_dev(i2cdev);
> -			goto err_detach_devs;
> +			return ret;
>  		}
>  	}
>  	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> @@ -1687,28 +1804,71 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  
>  		if (i3cboardinfo->init_dyn_addr) {
>  			status = i3c_bus_get_addr_slot_status(&master->bus,
> -						i3cboardinfo->init_dyn_addr);
> -			if (status != I3C_ADDR_SLOT_FREE) {
> -				ret = -EBUSY;
> -				goto err_detach_devs;
> -			}
> +							      i3cboardinfo->init_dyn_addr);
> +			if (status != I3C_ADDR_SLOT_FREE)
> +				return -EBUSY;
>  		}
>  
>  		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> -		if (IS_ERR(i3cdev)) {
> -			ret = PTR_ERR(i3cdev);
> -			goto err_detach_devs;
> -		}
> +		if (IS_ERR(i3cdev))
> +			return PTR_ERR(i3cdev);
>  
>  		i3cdev->boardinfo = i3cboardinfo;
>  
>  		ret = i3c_master_attach_i3c_dev(master, i3cdev);
>  		if (ret) {
>  			i3c_master_free_i3c_dev(i3cdev);
> -			goto err_detach_devs;
> +			return ret;
>  		}
>  	}
>  
> +	return 0;
> +}
> +
> +/**
> + * i3c_master_bus_init() - initialize an I3C bus
> + * @master: main master initializing the bus
> + *
> + * This function is following all initialisation steps described in the I3C
> + * specification:
> + *
> + * 1. Attach I2C and statically defined I3C devs to the master so that the
> + *    master can fill its internal device table appropriately
> + *
> + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
> + *    the master controller. That's usually where the bus mode is selected
> + *    (pure bus or mixed fast/slow bus)
> + *
> + * 3. Instruct all devices on the bus to drop their dynamic address. This is
> + *    particularly important when the bus was previously configured by someone
> + *    else (for example the bootloader)
> + *
> + * 4. Disable all slave events.
> + *
> + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> + *    devices that have a static address
> + *
> + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> + *    remaining I3C devices
> + *
> + * Once this is done, all I3C and I2C devices should be usable.
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +static int i3c_master_bus_init(struct i3c_master_controller *master)
> +{
> +	struct i3c_dev_desc *i3cdev;
> +	int ret;
> +
> +	/*
> +	 * First attach all devices with static definitions provided by the
> +	 * FW.
> +	 */
> +	if (!master->secondary) {
> +		ret = i3c_master_attach_static_devs(master);
> +		if (ret)
> +			goto err_detach_devs;
> +	}
>  	/*
>  	 * Now execute the controller specific ->bus_init() routine, which
>  	 * might configure its internal logic to match the bus limitations.
> @@ -1729,6 +1889,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	}
>  
>  	/*
> +	 * Don't reset addresses if this is secondary master.
> +	 * Secondary masters can't do DAA.
> +	 */
> +	if (master->secondary)
> +		return 0;
> +
> +	/*
>  	 * Reset all dynamic address that may have been assigned before
>  	 * (assigned by the bootloader for example).
>  	 */
> @@ -1791,6 +1958,49 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
> +			   u16 addr, u8 lvr)

Documentation is missing for this function. And I guess the lock must
be held, so please suffix it with _locked.

> +{
> +	enum i3c_addr_slot_status status;
> +	struct i2c_dev_desc *i2cdev;
> +	int ret;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	status = i3c_bus_get_addr_slot_status(&master->bus,
> +					      addr);
> +	if (status != I3C_ADDR_SLOT_FREE)
> +		return -EBUSY;
> +
> +	i3c_bus_set_addr_slot_status(&master->bus, addr,
> +				     I3C_ADDR_SLOT_I2C_DEV);
> +
> +	i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr);
> +	if (IS_ERR(i2cdev)) {
> +		ret = PTR_ERR(i2cdev);
> +		goto err_free_dev;
> +	}
> +
> +	i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr);
> +
> +	ret = i3c_master_attach_i2c_dev(master, i2cdev);
> +	if (ret) {
> +		ret = PTR_ERR(i2cdev);
> +		goto err_free_dev;
> +	}
> +
> +	return 0;
> +
> +err_free_dev:
> +	i3c_bus_set_addr_slot_status(&master->bus, addr,
> +				     I3C_ADDR_SLOT_FREE);
> +	i3c_master_free_i2c_dev(i2cdev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev);
> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -2113,11 +2323,17 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
>  	}
>  
>  	i3c_bus_normaluse_lock(&master->bus);
> +	ret = i3c_master_acquire_bus_ownership(master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
>  	if (!dev)
>  		ret = -ENOENT;
>  	else
>  		ret = master->ops->i2c_xfers(dev, xfers, nxfers);
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(&master->bus);
>  
>  	return ret ? ret : nxfers;
> @@ -2151,8 +2367,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master)
>  	 * We silently ignore failures here. The bus should keep working
>  	 * correctly even if one or more i2c devices are not registered.
>  	 */
> -	i3c_bus_for_each_i2cdev(&master->bus, i2cdev)
> +	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +		if (!i2cdev->boardinfo)
> +			continue;
> +
>  		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
> +	}
>  
>  	return 0;
>  }
> @@ -2393,18 +2613,43 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>  	     !ops->recycle_ibi_slot))
>  		return -EINVAL;
>  
> +	if (ops->request_mastership &&
> +	    (!ops->enable_mr_events || !ops->disable_mr_events))
> +		return -EINVAL;

Can you maybe add a comment explaining why ->enable/disable_mr_events()
are required?

> +
>  	return 0;
>  }
>  
>  /**
> + * i3c_master_register_new_devs() - register new devices
> + * @master: master used to send frames on the bus
> + *
> + * This function is useful when devices were not added
> + * during initialization or when new device joined the bus
> + * which was under control of different master.
> + */
> +void i3c_master_register_new_devs(struct i3c_master_controller *master)
> +{
> +	/*
> +	 * We can register I3C devices received from master by DEFSLVS.
> +	 */
> +	i3c_bus_normaluse_lock(&master->bus);
> +	i3c_master_register_new_i3c_devs(master);
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	i3c_bus_normaluse_lock(&master->bus);

Why do you release+take the lock here? Can't you keep it?

> +	i3c_master_register_new_i2c_devs(master);
> +	i3c_bus_normaluse_unlock(&master->bus);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs);
> +
> +/**
>   * i3c_master_register() - register an I3C master
>   * @master: master used to send frames on the bus
>   * @parent: the parent device (the one that provides this I3C master
>   *	    controller)
>   * @ops: the master controller operations
> - * @secondary: true if you are registering a secondary master. Will return
> - *	       -ENOTSUPP if set to true since secondary masters are not yet
> - *	       supported
> + * @secondary: true if you are registering a secondary master.
>   *
>   * This function takes care of everything for you:
>   *
> @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller *master,
>  	struct i2c_dev_boardinfo *i2cbi;
>  	int ret;
>  
> -	/* We do not support secondary masters yet. */
> -	if (secondary)
> -		return -ENOTSUPP;
> -
>  	ret = i3c_master_check_ops(ops);
>  	if (ret)
>  		return ret;
> @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller *master,
>  	 * register I3C devices dicovered during the initial DAA.
>  	 */
>  	master->init_done = true;
> -	i3c_bus_normaluse_lock(&master->bus);
> -	i3c_master_register_new_i3c_devs(master);
> -	i3c_bus_normaluse_unlock(&master->bus);
> +	i3c_master_register_new_devs(master);
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	i3c_master_enable_mr_events(master);

i3c_master_enable_mr_events() should be suffixed with _locked. And
please add a comment explaining why you enable MR events here.

> +	i3c_bus_maintenance_unlock(&master->bus);
>  
>  	return 0;
>  
> @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller *master,
>  EXPORT_SYMBOL_GPL(i3c_master_register);
>  
>  /**
> + * i3c_master_mastership_ack() - performs operations before bus handover.
> + * @master: master used to send frames on the bus
> + * @addr: I3C device address
> + *
> + * Basically, it sends DEFSLVS command to ensure new master is taking

Are you sure of that? It seems to only send GETACCMST, and DEFSLVS has
probably been sent early, when the secondary master was discovered
(during DAA).

> + * the bus with complete list of devices and then acknowledges bus takeover.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> +			      u8 addr)
> +{
> +	int ret;
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	ret = i3c_master_get_accmst_locked(master, addr);
> +	i3c_bus_maintenance_unlock(&master->bus);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack);
> +
> +
> +/**
>   * i3c_master_unregister() - unregister an I3C master
>   * @master: master used to send frames on the bus
>   *
> @@ -2533,6 +2800,10 @@ EXPORT_SYMBOL_GPL(i3c_master_register);
>   */
>  int i3c_master_unregister(struct i3c_master_controller *master)
>  {
> +	i3c_bus_maintenance_lock(&master->bus);
> +	i3c_master_disable_mr_events(master);
> +	i3c_bus_maintenance_unlock(&master->bus);
> +
>  	i3c_master_i2c_adapter_cleanup(master);
>  	i3c_master_unregister_i3c_devs(master);
>  	i3c_master_bus_cleanup(master);
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 42bb215..f32afd3 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -421,6 +421,12 @@ struct i3c_bus {
>   *		      for a future IBI
>   *		      This method is mandatory only if ->request_ibi is not
>   *		      NULL.
> + * @request_mastership: requests bus mastership. Mastership is requested
> + *                      automatically when device driver wants to transfer
> + *                      data using master in slave mode.

				      ^ a master that does not currently
				      owns the bus

> + * @enable_mr_events: enable the Mastership event.
> + *                    Mastership does not require handler.

I don't understand this comment. Also, I think it deserves some extra
details, like what drivers are supposed to do here (send ENEC(MR) +
possibly enable auto-ack?)

> + * @disable_mr_events: disable the Mastership event.

Same here, we need extra details.

>   */
>  struct i3c_master_controller_ops {
>  	int (*bus_init)(struct i3c_master_controller *master);
> @@ -447,6 +453,10 @@ struct i3c_master_controller_ops {
>  	int (*disable_ibi)(struct i3c_dev_desc *dev);
>  	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
>  				 struct i3c_ibi_slot *slot);
> +	void (*update_devs)(struct i3c_master_controller *master);

This one is no longer used AFAICS.

> +	int (*request_mastership)(struct i3c_master_controller *master);
> +	int (*enable_mr_events)(struct i3c_master_controller *master);
> +	int (*disable_mr_events)(struct i3c_master_controller *master);
>  };
>  
>  /**
> @@ -526,6 +536,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master);
>  int i3c_master_get_free_addr(struct i3c_master_controller *master,
>  			     u8 start_addr);
>  
> +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
> +			   u16 addr, u8 lvr);
>  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  				  u8 addr);
>  int i3c_master_do_daa(struct i3c_master_controller *master);
> @@ -538,6 +550,11 @@ int i3c_master_register(struct i3c_master_controller *master,
>  			const struct i3c_master_controller_ops *ops,
>  			bool secondary);
>  int i3c_master_unregister(struct i3c_master_controller *master);
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +				 u8 addr);
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> +			      u8 addr);
> +void i3c_master_register_new_devs(struct i3c_master_controller *master);
>  
>  /**
>   * i3c_dev_get_master_data() - get master private data attached to an I3C


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

Re: [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.

[Boris Brezillon]

On Sun, 10 Mar 2019 13:58:41 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> This patch adds support for mastership request to Cadence I3C master driver.
> 
> Mastership is requested automatically after secondary master
> receives mastership ENEC event. This allows secondary masters to
> initialize their bus.
> 
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> 
> ---
> 
> Main changes between v3 and v4:
> - Refactored the code
> 
> Main changes between v2 and v3:
> - Add mastership type
> - Add postponed master registration
> - Add update device definition on bus initialization time
> - Removed redundant mastership work structs
> - Reworked IBI slot lookup
> - Reworked Mastership event enabling/disabling
> 
> Changes in v2:
> - Add work structs for mastership purpose
> - Add missing mastership disable feature
> ---
>  drivers/i3c/master/i3c-master-cdns.c | 495 +++++++++++++++++++++++++++++++----
>  1 file changed, 443 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index 237f24a..61d6416 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -157,6 +157,7 @@
>  #define SLV_IMR				0x48
>  #define SLV_ICR				0x4c
>  #define SLV_ISR				0x50
> +#define SLV_INT_DEFSLVS			BIT(21)
>  #define SLV_INT_TM			BIT(20)
>  #define SLV_INT_ERROR			BIT(19)
>  #define SLV_INT_EVENT_UP		BIT(18)
> @@ -388,8 +389,19 @@ struct cdns_i3c_xfer {
>  	struct cdns_i3c_cmd cmds[0];
>  };
>  
> +enum cdns_i3c_mr {
> +	REQUEST,
> +	HANDOFF,
> +	TAKEOVER
> +};
> +
>  struct cdns_i3c_master {
>  	struct work_struct hj_work;
> +	struct {
> +		struct work_struct work;
> +		enum cdns_i3c_mr mr_type;
> +		u32 ibir;
> +	} mastership;
>  	struct i3c_master_controller base;
>  	u32 free_rr_slots;
>  	unsigned int maxdevs;
> @@ -408,6 +420,8 @@ struct cdns_i3c_master {
>  	struct clk *pclk;
>  	struct cdns_i3c_master_caps caps;
>  	unsigned long i3c_scl_lim;
> +	struct device *parent;
> +	struct workqueue_struct *wq;
>  };
>  
>  static inline struct cdns_i3c_master *
> @@ -663,6 +677,88 @@ static void cdns_i3c_master_unqueue_xfer(struct cdns_i3c_master *master,
>  	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
>  }
>  
> +static void
> +cdns_i3c_master_i3c_dev_rr_to_info(struct cdns_i3c_master *master,

cdns_i3c_master_dev_rr_to_i3c_info(...


> +				   unsigned int slot,
> +				   struct i3c_device_info *info)
> +{
> +	u32 rr;
> +
> +	memset(info, 0, sizeof(*info));
> +	rr = readl(master->regs + DEV_ID_RR0(slot));
> +	info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
> +	rr = readl(master->regs + DEV_ID_RR2(slot));
> +	info->dcr = rr;
> +	info->bcr = rr >> 8;
> +	info->pid = rr >> 16;
> +	info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16;
> +}
> +
> +static void
> +cdns_i3c_master_i2c_dev_rr_to_info(struct cdns_i3c_master *master,

cdns_i3c_master_dev_rr_to_i2c_info(...

> +				   unsigned int slot,
> +				   u16 *addr, u8 *lvr)
> +{
> +	u32 rr;
> +
> +	rr = readl(master->regs + DEV_ID_RR0(slot));
> +	*addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
> +	rr = readl(master->regs + DEV_ID_RR2(slot));
> +	*lvr = rr;
> +}
> +
> +static
> +int cdns_i3c_sec_master_request_mastership(struct i3c_master_controller *m)

It's also used for the primary master to get ownership back, right? In
that case, don't prefix things with _sec_master.

> +{
> +	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
> +	u32 status;
> +	int ret;
> +
> +	status = readl(master->regs + MST_STATUS0);
> +	if (WARN_ON(status & MST_STATUS0_MASTER_MODE))
> +		return -EEXIST;
> +
> +	status = readl(master->regs + SLV_STATUS1);
> +	if (status & SLV_STATUS1_MR_DIS)
> +		return -EACCES;
> +
> +	writel(SLV_INT_MR_DONE, master->regs + SLV_IER);

Hm, you seem to poll the master mode status, what's the point of
enabling this interrupt?

> +	writel(readl(master->regs + CTRL) | CTRL_MST_INIT | CTRL_MST_ACK,
> +	       master->regs + CTRL);
> +
> +	ret = readl_poll_timeout(master->regs + MST_STATUS0, status,
> +				 status & MST_STATUS0_MASTER_MODE, 100,
> +				 100000);
> +	return ret;
> +}
> +
> +static void cdns_i3c_master_update_devs(struct i3c_master_controller *m)
> +{
> +	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
> +	u32 val, newdevs;
> +	u16 addr;
> +	u8 lvr;
> +	int slot;
> +	struct i3c_device_info i3c_info;
> +
> +	newdevs = readl(master->regs + DEVS_CTRL) & DEVS_CTRL_DEVS_ACTIVE_MASK;
> +	for (slot = 1; slot <= master->maxdevs; slot++) {
> +		val = readl(master->regs + DEV_ID_RR0(slot));
> +
> +		if ((newdevs & BIT(slot)) && (val & DEV_ID_RR0_IS_I3C)) {
> +			cdns_i3c_master_i3c_dev_rr_to_info(master, slot,
> +							   &i3c_info);
> +
> +			i3c_master_add_i3c_dev_locked(m, i3c_info.dyn_addr);
> +		} else if ((newdevs & BIT(slot)) &&
> +			   !(val & DEV_ID_RR0_IS_I3C)) {
> +			cdns_i3c_master_i2c_dev_rr_to_info(master, slot,
> +							   &addr, &lvr);
> +			i3c_master_add_i2c_dev(m, addr, lvr);
> +		}
> +	}
> +}
> +
>  static enum i3c_error_code cdns_i3c_cmd_get_err(struct cdns_i3c_cmd *cmd)
>  {
>  	switch (cmd->error) {
> @@ -913,6 +1009,7 @@ static int cdns_i3c_master_get_rr_slot(struct cdns_i3c_master *master,
>  		return ffs(master->free_rr_slots) - 1;
>  	}
>  
> +
>  	activedevs = readl(master->regs + DEVS_CTRL) &
>  		     DEVS_CTRL_DEVS_ACTIVE_MASK;
>  
> @@ -1005,9 +1102,9 @@ static int cdns_i3c_master_attach_i2c_dev(struct i2c_dev_desc *dev)
>  	master->free_rr_slots &= ~BIT(slot);
>  	i2c_dev_set_master_data(dev, data);
>  
> -	writel(prepare_rr0_dev_address(dev->boardinfo->base.addr),
> +	writel(prepare_rr0_dev_address(dev->addr),
>  	       master->regs + DEV_ID_RR0(data->id));
> -	writel(dev->boardinfo->lvr, master->regs + DEV_ID_RR2(data->id));
> +	writel(dev->lvr, master->regs + DEV_ID_RR2(data->id));
>  	writel(readl(master->regs + DEVS_CTRL) |
>  	       DEVS_CTRL_DEV_ACTIVE(data->id),
>  	       master->regs + DEVS_CTRL);

Should be part of patch 1, and you need to patch the designware driver
too.

> @@ -1037,22 +1134,6 @@ static void cdns_i3c_master_bus_cleanup(struct i3c_master_controller *m)
>  	cdns_i3c_master_disable(master);
>  }
>  
> -static void cdns_i3c_master_dev_rr_to_info(struct cdns_i3c_master *master,
> -					   unsigned int slot,
> -					   struct i3c_device_info *info)
> -{
> -	u32 rr;
> -
> -	memset(info, 0, sizeof(*info));
> -	rr = readl(master->regs + DEV_ID_RR0(slot));
> -	info->dyn_addr = DEV_ID_RR0_GET_DEV_ADDR(rr);
> -	rr = readl(master->regs + DEV_ID_RR2(slot));
> -	info->dcr = rr;
> -	info->bcr = rr >> 8;
> -	info->pid = rr >> 16;
> -	info->pid |= (u64)readl(master->regs + DEV_ID_RR1(slot)) << 16;
> -}
> -
>  static void cdns_i3c_master_upd_i3c_scl_lim(struct cdns_i3c_master *master)
>  {
>  	struct i3c_master_controller *m = &master->base;
> @@ -1180,10 +1261,6 @@ static int cdns_i3c_master_do_daa(struct i3c_master_controller *m)
>  
>  	cdns_i3c_master_upd_i3c_scl_lim(master);
>  
> -	/* Unmask Hot-Join and Mastership request interrupts. */
> -	i3c_master_enec_locked(m, I3C_BROADCAST_ADDR,
> -			       I3C_CCC_EVENT_HJ | I3C_CCC_EVENT_MR);
> -
>  	return 0;
>  }
>  
> @@ -1247,15 +1324,17 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
>  	prescl1 = PRESCL_CTRL1_OD_LOW(ncycles);
>  	writel(prescl1, master->regs + PRESCL_CTRL1);
>  
> -	/* Get an address for the master. */
> -	ret = i3c_master_get_free_addr(m, 0);
> -	if (ret < 0)
> -		return ret;
> +	if (!m->secondary) {
> +		/* Get an address for the master. */
> +		ret = i3c_master_get_free_addr(m, 0);
> +		if (ret < 0)
> +			return ret;
>  
> -	writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
> -	       master->regs + DEV_ID_RR0(0));
> +		writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
> +		       master->regs + DEV_ID_RR0(0));
> +	}
>  
> -	cdns_i3c_master_dev_rr_to_info(master, 0, &info);
> +	cdns_i3c_master_i3c_dev_rr_to_info(master, 0, &info);
>  	if (info.bcr & I3C_BCR_HDR_CAP)
>  		info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR);
>  
> @@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
>  
>  	cdns_i3c_master_enable(master);
>  
> +	if (m->secondary) {
> +		i3c_bus_maintenance_lock(&master->base.bus);
> +		cdns_i3c_master_update_devs(&master->base);
> +		i3c_bus_maintenance_unlock(&master->base.bus);
> +	}

Okay, I changed my mind on this solution (it's not the first time that
happens, and unfortunately won't be the last time either :-)). I think
I don't like the idea of exposing the i3c_bus_maintenance_unlock/lock()
functions in the end.

I'd like to reconsider what you initially proposed: having an
->update_devs() hook that is called by the core, except I'd call it
->populate_bus().

BTW, there's still something that's unclear to me. You seem to populate
the bus here and also when acquiring bus ownership. Is this correct?
I'd expect it to be 'partially' populated at bus-init+defslvs time,
which would trigger a mastership request if I3C devices are present (as
we need to send a GETPID to know more about I3C devs).

Also, what happens if i3c_master_add_i3c_dev_locked() fails? You
don't seem to handle that case at all.

> +
>  	return 0;
>  }
>  
> +static
> +struct i3c_dev_desc *cdns_i3c_get_ibi_device(struct cdns_i3c_master *master,
> +					     u32 ibir)
> +{
> +	struct i3c_dev_desc *dev;
> +	u32 id = IBIR_SLVID(ibir);
> +
> +	if (id >= master->ibi.num_slots || (ibir & IBIR_ERROR))
> +		return NULL;
> +
> +	dev = master->ibi.slots[id];
> +	if (!dev)
> +		return NULL;
> +
> +	return dev;
> +}
> +
>  static void cdns_i3c_master_handle_ibi(struct cdns_i3c_master *master,
>  				       u32 ibir)
>  {
> @@ -1354,27 +1456,100 @@ static void cnds_i3c_master_demux_ibis(struct cdns_i3c_master *master)
>  
>  		case IBIR_TYPE_MR:
>  			WARN_ON(IBIR_XFER_BYTES(ibir) || (ibir & IBIR_ERROR));
> +			master->mastership.ibir = ibir;
> +			master->mastership.mr_type = HANDOFF;
> +			queue_work(master->base.wq,
> +				   &master->mastership.work);
> +			break;
>  		default:
>  			break;
>  		}
>  	}
>  }
>  
> +static void cdns_i3c_master_bus_handoff(struct cdns_i3c_master *master)
> +{
> +	struct i3c_dev_desc *dev;
> +
> +	dev = cdns_i3c_get_ibi_device(master, master->mastership.ibir);
> +
> +	writel(MST_INT_MR_DONE, master->regs + MST_ICR);
> +
> +	master->base.bus.cur_master = dev;
> +}
> +
> +static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
> +{
> +	if (master->base.init_done) {

Can this really happen that init_done is not set when you reach this
point.

> +		i3c_bus_maintenance_lock(&master->base.bus);
> +		cdns_i3c_master_update_devs(&master->base);
> +		i3c_bus_maintenance_unlock(&master->base.bus);
> +
> +		i3c_master_register_new_devs(&master->base);

The core will somehow be informed that this master now owns the bus, so
it can call i3c_master_register_new_devs() for us, right?

But as said above, I'm not even sure this is correct to do it from
here. I'd expect this to happen at DEFSLVS or BUS init time.

> +	}
> +
> +	writel(readl(master->regs + CTRL) & ~CTRL_MST_ACK, master->regs + CTRL);
> +}
> +
> +static void cdns_i3c_sec_master_event_up(struct cdns_i3c_master *master)

Again, not specific to secondary masters, the primary master can go
through that path too.

> +{
> +	u32 status;
> +
> +	writel(SLV_INT_EVENT_UP, master->regs + SLV_ICR);
> +	status = readl(master->regs + SLV_STATUS1);
> +	if (!(status & SLV_STATUS1_MR_DIS) &&
> +	    !master->base.this) {
> +		master->mastership.mr_type = REQUEST;
> +		queue_work(master->wq, &master->mastership.work);
> +	}
> +}

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[vitor]

Hi,

On 30/03/19 14:36, Boris Brezillon wrote:
> On Sun, 10 Mar 2019 13:58:38 +0000
> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>
>> I need to store address and lvr value for I2C devices without static definition
>> in DT. This allows secondary master to transmit DEFSLVS command properly.
>>
>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>> ---
>>  include/linux/i3c/master.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>> index eca8337..3c27d9f 100644
>> --- a/include/linux/i3c/master.h
>> +++ b/include/linux/i3c/master.h
>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
>>   * @common: common part of the I2C device descriptor
>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
>>   * @dev: I2C device object registered to the I2C framework
>> + * @addr: I2C device address
>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
>> + *	 the I2C device limitations
>>   *
>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
>>   * This object is created by the core and later attached to the controller
>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
>>  	struct i3c_i2c_dev_desc common;
>>  	const struct i2c_dev_boardinfo *boardinfo;
>>  	struct i2c_client *dev;
>> +	u16 addr;
>> +	u8 lvr;
> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> patch 3.
>

Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?
>>  };
>>  
>>  /**


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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Boris Brezillon]

On Mon, 1 Apr 2019 19:17:03 +0100
vitor <vitor.soares@synopsys.com> wrote:

> Hi,
> 
> On 30/03/19 14:36, Boris Brezillon wrote:
> > On Sun, 10 Mar 2019 13:58:38 +0000
> > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >  
> >> I need to store address and lvr value for I2C devices without static definition
> >> in DT. This allows secondary master to transmit DEFSLVS command properly.
> >>
> >> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> >> ---
> >>  include/linux/i3c/master.h | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> >> index eca8337..3c27d9f 100644
> >> --- a/include/linux/i3c/master.h
> >> +++ b/include/linux/i3c/master.h
> >> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
> >>   * @common: common part of the I2C device descriptor
> >>   * @boardinfo: pointer to the boardinfo attached to this I2C device
> >>   * @dev: I2C device object registered to the I2C framework
> >> + * @addr: I2C device address
> >> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> >> + *	 the I2C device limitations
> >>   *
> >>   * Each I2C device connected on the bus will have an i2c_dev_desc.
> >>   * This object is created by the core and later attached to the controller
> >> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
> >>  	struct i3c_i2c_dev_desc common;
> >>  	const struct i2c_dev_boardinfo *boardinfo;
> >>  	struct i2c_client *dev;
> >> +	u16 addr;
> >> +	u8 lvr;  
> > You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> > to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> > patch 3.
> >  
> 
> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?

Because i2c_dev_boardinfo is extracted from the DT and the secondary
slaves does not necessarily have this description. The idea is to keep
reserving the address slot for the I2C device even if we don't expose
it to the upper layers.

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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[vitor]

Hi Przemek,

On 10/03/19 13:58, Przemyslaw Gaj wrote:
> This patch adds support for mastership request to I3C subsystem.
>
> Mastership event is enabled globally.
>
> Mastership is requested automatically when device driver
> tries to transfer data using master controller in slave mode.
>
> There is still some limitation:
> - I2C devices are registered on secondary master side if boardinfo
> entry matching the info transmitted through the DEFSLVS frame.
>
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>
> ---
>
> Main changes between v3 and v4:
> - Add i3c_master_acquire_bus_ownership to acquire the bus
> - Refactored the code
>
> Main changes between v2 and v3:
> - Add i3c_bus_downgrade_maintenance_lock() for downgrading the bus
> lock from maintenance to normal use
> - Add additional fields to i2c_dev_desc for DEFSLVS purpose (addr, lvr)
> - Add i3c_master_register_new_i2c_devs() function to register I2C devices
> - Reworked I2C devices registration on secondary master side
>
> Changes in v2:
> - Add mastership disable event hook
> - Changed name of mastership enable event hook
> - Add function to test if master owns the bus
> - Add function to test if master owns the bus
> - Removed op_mode
> - Changed parameter of i3c_master_get_accmst_locked, no need to
> pass full i3c_device_info
> - Changed name of mastership enable event hook
> - Add function to test if master owns the bus
> - Removed op_mode
> - Changed parameter of i3c_master_get_accmst_locked, no need to
> pass full i3c_device_info
> - Removed redundant DEFSLVS command before GETACCMST
> - Add i3c_master_bus_takeover function. There is a need to lock
> the bus before adding devices and no matter of the controller
> devices have to be added after mastership takeover.
> - Add device registration during initialization on secondary master
> side. Devices received by DEFSLVS (if occured). If not, device
> initialization is deffered untill next mastership request.
> ---
>  drivers/i3c/device.c       |  26 +++
>  drivers/i3c/internals.h    |   4 +
>  drivers/i3c/master.c       | 417 +++++++++++++++++++++++++++++++++++++--------
>  include/linux/i3c/master.h |  17 ++
>  4 files changed, 391 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index 69cc040..b60f637 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -43,7 +43,13 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
>  	}
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  
>  	return ret;
> @@ -114,11 +120,17 @@ int i3c_device_enable_ibi(struct i3c_device *dev)
>  	int ret = -ENOENT;
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	if (dev->desc) {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		ret = i3c_dev_enable_ibi_locked(dev->desc);
>  		mutex_unlock(&dev->desc->ibi_lock);
>  	}
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  
>  	return ret;
> @@ -145,11 +157,17 @@ int i3c_device_request_ibi(struct i3c_device *dev,
>  		return -EINVAL;
>  
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	if (dev->desc) {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		ret = i3c_dev_request_ibi_locked(dev->desc, req);
>  		mutex_unlock(&dev->desc->ibi_lock);
>  	}
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  
>  	return ret;
> @@ -166,12 +184,20 @@ EXPORT_SYMBOL_GPL(i3c_device_request_ibi);
>   */
>  void i3c_device_free_ibi(struct i3c_device *dev)
>  {
> +	int ret;
> +
>  	i3c_bus_normaluse_lock(dev->bus);
> +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	if (dev->desc) {
>  		mutex_lock(&dev->desc->ibi_lock);
>  		i3c_dev_free_ibi_locked(dev->desc);
>  		mutex_unlock(&dev->desc->ibi_lock);
>  	}
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(dev->bus);
>  }
>  EXPORT_SYMBOL_GPL(i3c_device_free_ibi);

Can't we verify if it is the current master no master.c side?

> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 86b7b44..929ca6b 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type;
>  
>  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
>  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus);
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master);
> +int i3c_master_request_mastership_locked(struct i3c_master_controller *master);
> +
>  
>  int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>  				 struct i3c_priv_xfer *xfers,
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index aea4309..7a84158 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
>  	up_read(&bus->lock);
>  }
>  
> +/**
> + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal
> + * operation
> + * @bus: I3C bus to downgrade the lock on
> + *
> + * Should be called when a maintenance operation is done and normal
> + * operation is planned. See i3c_bus_maintenance_lock() and
> + * i3c_bus_normaluse_lock() for more details.
> + */
> +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus)
> +{
> +	downgrade_write(&bus->lock);
> +}
> +
>  static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
>  {
>  	return container_of(dev, struct i3c_master_controller, dev);
> @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev)
>  	return driver->probe(i3cdev);
>  }
>  
> +static void i3c_master_enable_mr_events(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->enable_mr_events)
> +		return;
> +
> +	master->ops->enable_mr_events(master);
> +}
> +
> +static void i3c_master_disable_mr_events(struct i3c_master_controller *master)
> +{
> +	if (!master->ops->disable_mr_events)
> +		return;
> +
> +	master->ops->disable_mr_events(master);
> +}
> +

do we want to do this in broadcast? Is it save to give mastership capabilities to all devices?

>  static int i3c_device_remove(struct device *dev)
>  {
>  	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> @@ -462,6 +492,36 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
>  	return 0;
>  }
>  
> +int i3c_master_request_mastership_locked(struct i3c_master_controller *master)
> +{
> +	if (WARN_ON(master->init_done &&
> +	    !rwsem_is_locked(&master->bus.lock)))
> +		return -EINVAL;
> +
> +	if (!master->ops->request_mastership)
> +		return -ENOTSUPP;
> +
> +	return master->ops->request_mastership(master);
> +}
> +
> +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master)
> +{
> +	int ret;
> +
> +	if (master->bus.cur_master != master->this) {
> +		i3c_bus_normaluse_unlock(&master->bus);
> +		i3c_bus_maintenance_lock(&master->bus);
> +
> +		ret = i3c_master_request_mastership_locked(master);
> +		if (ret) {
> +			i3c_bus_maintenance_unlock(&master->bus);
> +			return ret;
> +		}
> +		i3c_bus_downgrade_maintenance_lock(&master->bus);
> +	}
> +	return 0;
> +}
> +
>  static const char * const i3c_bus_mode_strings[] = {
>  	[I3C_BUS_MODE_PURE] = "pure",
>  	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> @@ -620,6 +680,25 @@ i3c_master_alloc_i2c_dev(struct i3c_master_controller *master,
>  
>  	dev->common.master = master;
>  	dev->boardinfo = boardinfo;
> +	dev->addr = boardinfo->base.addr;
> +	dev->lvr = boardinfo->lvr;
> +
> +	return dev;
> +}
> +
> +static struct i2c_dev_desc *
> +i3c_master_alloc_i2c_dev_no_boardinfo(struct i3c_master_controller *master,
> +				      u16 addr, u8 lvr)
> +{
> +	struct i2c_dev_desc *dev;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev->common.master = master;
> +	dev->addr = addr;
> +	dev->lvr = lvr;
>  
>  	return dev;
>  }
> @@ -693,6 +772,9 @@ i3c_master_find_i2c_dev_by_addr(const struct i3c_master_controller *master,
>  	struct i2c_dev_desc *dev;
>  
>  	i3c_bus_for_each_i2cdev(&master->bus, dev) {
> +		if (!dev->boardinfo)
> +			continue;
> +
>  		if (dev->boardinfo->base.addr == addr)
>  			return dev;
>  	}
> @@ -939,8 +1021,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master)
>  
>  	desc = defslvs->slaves;
>  	i3c_bus_for_each_i2cdev(bus, i2cdev) {
> -		desc->lvr = i2cdev->boardinfo->lvr;
> -		desc->static_addr = i2cdev->boardinfo->base.addr << 1;
> +		desc->lvr = i2cdev->lvr;
> +		desc->static_addr = i2cdev->addr << 1;
>  		desc++;
>  	}
>  
> @@ -1492,6 +1574,83 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>  	}
>  }
>  
> +static struct i2c_dev_boardinfo *
> +i3c_master_find_i2c_boardinfo(const struct i3c_master_controller *master,
> +			      u16 addr, u8 lvr)
> +{
> +	struct i2c_dev_boardinfo *i2cboardinfo;
> +
> +	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
> +		if (i2cboardinfo->base.addr == addr &&
> +		    i2cboardinfo->lvr == lvr)
> +			return i2cboardinfo;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void
> +i3c_master_register_new_i2c_devs(struct i3c_master_controller *master)
> +{
> +	struct i2c_adapter *adap = i3c_master_to_i2c_adapter(master);
> +	struct i2c_dev_desc *i2cdev;
> +
> +	if (!master->init_done)
> +		return;
> +
> +	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +		if (i2cdev->dev)
> +			continue;
> +
> +		if (!i2cdev->boardinfo)
> +			continue;
> +
> +		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
> +	}
> +}
> +
> +/**
> + * i3c_master_get_accmst_locked() - send a GETACCMST CCC command
> + * @master: master used to send frames on the bus
> + * @info: I3C device information
> + *
> + * Send a GETACCMST CCC command.
> + *
> + * This should be called if the curent master acknowledges bus takeover.
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * Return: 0 in case of success, a positive I3C error code if the error is
> + * one of the official Mx error codes, and a negative error code otherwise.
> + */
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +				 u8 addr)
> +{
> +	struct i3c_ccc_getaccmst *accmst;
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	accmst = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*accmst));
> +	if (!accmst)
> +		return -ENOMEM;
> +
> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETACCMST, &dest, 1);
> +
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	if (ret)
> +		goto out;
> +
> +	if (dest.payload.len != sizeof(*accmst))
> +		ret = -EIO;
> +
> +out:
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_get_accmst_locked);
> +
>  /**
>   * i3c_master_do_daa() - do a DAA (Dynamic Address Assignment)
>   * @master: master doing the DAA
> @@ -1559,10 +1718,6 @@ int i3c_master_set_info(struct i3c_master_controller *master,
>  	if (!i3c_bus_dev_addr_is_avail(&master->bus, info->dyn_addr))
>  		return -EINVAL;
>  
> -	if (I3C_BCR_DEVICE_ROLE(info->bcr) == I3C_BCR_I3C_MASTER &&
> -	    master->secondary)
> -		return -EINVAL;
> -
>  	if (master->this)
>  		return -EINVAL;
>  
> @@ -1607,43 +1762,13 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
>  				 common.node) {
>  		i3c_master_detach_i2c_dev(i2cdev);
>  		i3c_bus_set_addr_slot_status(&master->bus,
> -					i2cdev->boardinfo->base.addr,
> -					I3C_ADDR_SLOT_FREE);
> +					     i2cdev->addr,
> +					     I3C_ADDR_SLOT_FREE);
>  		i3c_master_free_i2c_dev(i2cdev);
>  	}
>  }
>  
> -/**
> - * i3c_master_bus_init() - initialize an I3C bus
> - * @master: main master initializing the bus
> - *
> - * This function is following all initialisation steps described in the I3C
> - * specification:
> - *
> - * 1. Attach I2C and statically defined I3C devs to the master so that the
> - *    master can fill its internal device table appropriately
> - *
> - * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
> - *    the master controller. That's usually where the bus mode is selected
> - *    (pure bus or mixed fast/slow bus)
> - *
> - * 3. Instruct all devices on the bus to drop their dynamic address. This is
> - *    particularly important when the bus was previously configured by someone
> - *    else (for example the bootloader)
> - *
> - * 4. Disable all slave events.
> - *
> - * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> - *    devices that have a static address
> - *
> - * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> - *    remaining I3C devices
> - *
> - * Once this is done, all I3C and I2C devices should be usable.
> - *
> - * Return: a 0 in case of success, an negative error code otherwise.
> - */
> -static int i3c_master_bus_init(struct i3c_master_controller *master)
> +static int i3c_master_attach_static_devs(struct i3c_master_controller *master)
>  {
>  	enum i3c_addr_slot_status status;
>  	struct i2c_dev_boardinfo *i2cboardinfo;
> @@ -1652,32 +1777,24 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	struct i2c_dev_desc *i2cdev;
>  	int ret;
>  
> -	/*
> -	 * First attach all devices with static definitions provided by the
> -	 * FW.
> -	 */
>  	list_for_each_entry(i2cboardinfo, &master->boardinfo.i2c, node) {
>  		status = i3c_bus_get_addr_slot_status(&master->bus,
>  						      i2cboardinfo->base.addr);
> -		if (status != I3C_ADDR_SLOT_FREE) {
> -			ret = -EBUSY;
> -			goto err_detach_devs;
> -		}
> +		if (status != I3C_ADDR_SLOT_FREE)
> +			return -EBUSY;
>  
>  		i3c_bus_set_addr_slot_status(&master->bus,
>  					     i2cboardinfo->base.addr,
>  					     I3C_ADDR_SLOT_I2C_DEV);
>  
>  		i2cdev = i3c_master_alloc_i2c_dev(master, i2cboardinfo);
> -		if (IS_ERR(i2cdev)) {
> -			ret = PTR_ERR(i2cdev);
> -			goto err_detach_devs;
> -		}
> +		if (IS_ERR(i2cdev))
> +			return PTR_ERR(i2cdev);
>  
>  		ret = i3c_master_attach_i2c_dev(master, i2cdev);
>  		if (ret) {
>  			i3c_master_free_i2c_dev(i2cdev);
> -			goto err_detach_devs;
> +			return ret;
>  		}
>  	}
>  	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> @@ -1687,28 +1804,71 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  
>  		if (i3cboardinfo->init_dyn_addr) {
>  			status = i3c_bus_get_addr_slot_status(&master->bus,
> -						i3cboardinfo->init_dyn_addr);
> -			if (status != I3C_ADDR_SLOT_FREE) {
> -				ret = -EBUSY;
> -				goto err_detach_devs;
> -			}
> +							      i3cboardinfo->init_dyn_addr);
> +			if (status != I3C_ADDR_SLOT_FREE)
> +				return -EBUSY;
>  		}
>  
>  		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> -		if (IS_ERR(i3cdev)) {
> -			ret = PTR_ERR(i3cdev);
> -			goto err_detach_devs;
> -		}
> +		if (IS_ERR(i3cdev))
> +			return PTR_ERR(i3cdev);
>  
>  		i3cdev->boardinfo = i3cboardinfo;
>  
>  		ret = i3c_master_attach_i3c_dev(master, i3cdev);
>  		if (ret) {
>  			i3c_master_free_i3c_dev(i3cdev);
> -			goto err_detach_devs;
> +			return ret;
>  		}
>  	}
>  
> +	return 0;
> +}
> +
> +/**
> + * i3c_master_bus_init() - initialize an I3C bus
> + * @master: main master initializing the bus
> + *
> + * This function is following all initialisation steps described in the I3C
> + * specification:
> + *
> + * 1. Attach I2C and statically defined I3C devs to the master so that the
> + *    master can fill its internal device table appropriately
> + *
> + * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
> + *    the master controller. That's usually where the bus mode is selected
> + *    (pure bus or mixed fast/slow bus)
> + *
> + * 3. Instruct all devices on the bus to drop their dynamic address. This is
> + *    particularly important when the bus was previously configured by someone
> + *    else (for example the bootloader)
> + *
> + * 4. Disable all slave events.
> + *
> + * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
> + *    devices that have a static address
> + *
> + * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> + *    remaining I3C devices
> + *
> + * Once this is done, all I3C and I2C devices should be usable.
> + *
> + * Return: a 0 in case of success, an negative error code otherwise.
> + */
> +static int i3c_master_bus_init(struct i3c_master_controller *master)
> +{
> +	struct i3c_dev_desc *i3cdev;
> +	int ret;
> +
> +	/*
> +	 * First attach all devices with static definitions provided by the
> +	 * FW.
> +	 */
> +	if (!master->secondary) {
> +		ret = i3c_master_attach_static_devs(master);
> +		if (ret)
> +			goto err_detach_devs;
> +	}

When do we get the i3c_dev_info()?

>  	/*
>  	 * Now execute the controller specific ->bus_init() routine, which
>  	 * might configure its internal logic to match the bus limitations.
> @@ -1729,6 +1889,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	}
>  
>  	/*
> +	 * Don't reset addresses if this is secondary master.
> +	 * Secondary masters can't do DAA.
> +	 */
> +	if (master->secondary)
> +		return 0;
> +
> +	/*
>  	 * Reset all dynamic address that may have been assigned before
>  	 * (assigned by the bootloader for example).
>  	 */
> @@ -1791,6 +1958,49 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
> +			   u16 addr, u8 lvr)
> +{
> +	enum i3c_addr_slot_status status;
> +	struct i2c_dev_desc *i2cdev;
> +	int ret;
> +
> +	if (!master)
> +		return -EINVAL;
> +
> +	status = i3c_bus_get_addr_slot_status(&master->bus,
> +					      addr);
> +	if (status != I3C_ADDR_SLOT_FREE)
> +		return -EBUSY;
> +
> +	i3c_bus_set_addr_slot_status(&master->bus, addr,
> +				     I3C_ADDR_SLOT_I2C_DEV);
> +
> +	i2cdev = i3c_master_alloc_i2c_dev_no_boardinfo(master, addr, lvr);
> +	if (IS_ERR(i2cdev)) {
> +		ret = PTR_ERR(i2cdev);
> +		goto err_free_dev;
> +	}
> +
> +	i2cdev->boardinfo = i3c_master_find_i2c_boardinfo(master, addr, lvr);
> +
> +	ret = i3c_master_attach_i2c_dev(master, i2cdev);
> +	if (ret) {
> +		ret = PTR_ERR(i2cdev);
> +		goto err_free_dev;
> +	}
> +
> +	return 0;
> +
> +err_free_dev:
> +	i3c_bus_set_addr_slot_status(&master->bus, addr,
> +				     I3C_ADDR_SLOT_FREE);
> +	i3c_master_free_i2c_dev(i2cdev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_add_i2c_dev);
> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -2113,11 +2323,17 @@ static int i3c_master_i2c_adapter_xfer(struct i2c_adapter *adap,
>  	}
>  
>  	i3c_bus_normaluse_lock(&master->bus);
> +	ret = i3c_master_acquire_bus_ownership(master);
> +	if (ret)
> +		goto err_unlock_bus;
> +
>  	dev = i3c_master_find_i2c_dev_by_addr(master, addr);
>  	if (!dev)
>  		ret = -ENOENT;
>  	else
>  		ret = master->ops->i2c_xfers(dev, xfers, nxfers);
> +
> +err_unlock_bus:
>  	i3c_bus_normaluse_unlock(&master->bus);
>  
>  	return ret ? ret : nxfers;
> @@ -2151,8 +2367,12 @@ static int i3c_master_i2c_adapter_init(struct i3c_master_controller *master)
>  	 * We silently ignore failures here. The bus should keep working
>  	 * correctly even if one or more i2c devices are not registered.
>  	 */
> -	i3c_bus_for_each_i2cdev(&master->bus, i2cdev)
> +	i3c_bus_for_each_i2cdev(&master->bus, i2cdev) {
> +		if (!i2cdev->boardinfo)
> +			continue;
> +
>  		i2cdev->dev = i2c_new_device(adap, &i2cdev->boardinfo->base);
> +	}
>  
>  	return 0;
>  }
> @@ -2393,18 +2613,43 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>  	     !ops->recycle_ibi_slot))
>  		return -EINVAL;
>  
> +	if (ops->request_mastership &&
> +	    (!ops->enable_mr_events || !ops->disable_mr_events))
> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
>  /**
> + * i3c_master_register_new_devs() - register new devices
> + * @master: master used to send frames on the bus
> + *
> + * This function is useful when devices were not added
> + * during initialization or when new device joined the bus
> + * which was under control of different master.
> + */
> +void i3c_master_register_new_devs(struct i3c_master_controller *master)
> +{
> +	/*
> +	 * We can register I3C devices received from master by DEFSLVS.
> +	 */
> +	i3c_bus_normaluse_lock(&master->bus);
> +	i3c_master_register_new_i3c_devs(master);
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +	i3c_master_register_new_i2c_devs(master);
> +	i3c_bus_normaluse_unlock(&master->bus);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs);
> +

 I would say a function to add DEVSLVS. From the master driver you can pack all
 received slaves into i3c_ccc_defslvs struct and rely the task of add those
 devices to the subsystem.

> +/**
>   * i3c_master_register() - register an I3C master
>   * @master: master used to send frames on the bus
>   * @parent: the parent device (the one that provides this I3C master
>   *	    controller)
>   * @ops: the master controller operations
> - * @secondary: true if you are registering a secondary master. Will return
> - *	       -ENOTSUPP if set to true since secondary masters are not yet
> - *	       supported
> + * @secondary: true if you are registering a secondary master.
>   *
>   * This function takes care of everything for you:
>   *
> @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller *master,
>  	struct i2c_dev_boardinfo *i2cbi;
>  	int ret;
>  
> -	/* We do not support secondary masters yet. */
> -	if (secondary)
> -		return -ENOTSUPP;
> -
>  	ret = i3c_master_check_ops(ops);
>  	if (ret)
>  		return ret;
> @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller *master,
>  	 * register I3C devices dicovered during the initial DAA.
>  	 */
>  	master->init_done = true;
> -	i3c_bus_normaluse_lock(&master->bus);
> -	i3c_master_register_new_i3c_devs(master);
> -	i3c_bus_normaluse_unlock(&master->bus);
> +	i3c_master_register_new_devs(master);
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	i3c_master_enable_mr_events(master);
> +	i3c_bus_maintenance_unlock(&master->bus);
>  
>  	return 0;
>  
> @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller *master,
>  EXPORT_SYMBOL_GPL(i3c_master_register);
>  
>  /**
> + * i3c_master_mastership_ack() - performs operations before bus handover.
> + * @master: master used to send frames on the bus
> + * @addr: I3C device address
> + *
> + * Basically, it sends DEFSLVS command to ensure new master is taking
> + * the bus with complete list of devices and then acknowledges bus takeover.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> +			      u8 addr)
> +{
> +	int ret;
> +
> +	i3c_bus_maintenance_lock(&master->bus);
> +	ret = i3c_master_get_accmst_locked(master, addr);
> +	i3c_bus_maintenance_unlock(&master->bus);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_mastership_ack);
> +
> +
> +/**
>   * i3c_master_unregister() - unregister an I3C master
>   * @master: master used to send frames on the bus
>   *
> @@ -2533,6 +2800,10 @@ EXPORT_SYMBOL_GPL(i3c_master_register);
>   */
>  int i3c_master_unregister(struct i3c_master_controller *master)
>  {
> +	i3c_bus_maintenance_lock(&master->bus);
> +	i3c_master_disable_mr_events(master);
> +	i3c_bus_maintenance_unlock(&master->bus);
> +
>  	i3c_master_i2c_adapter_cleanup(master);
>  	i3c_master_unregister_i3c_devs(master);
>  	i3c_master_bus_cleanup(master);
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 42bb215..f32afd3 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -421,6 +421,12 @@ struct i3c_bus {
>   *		      for a future IBI
>   *		      This method is mandatory only if ->request_ibi is not
>   *		      NULL.
> + * @request_mastership: requests bus mastership. Mastership is requested
> + *                      automatically when device driver wants to transfer
> + *                      data using master in slave mode.
> + * @enable_mr_events: enable the Mastership event.
> + *                    Mastership does not require handler.
> + * @disable_mr_events: disable the Mastership event.
>   */
>  struct i3c_master_controller_ops {
>  	int (*bus_init)(struct i3c_master_controller *master);
> @@ -447,6 +453,10 @@ struct i3c_master_controller_ops {
>  	int (*disable_ibi)(struct i3c_dev_desc *dev);
>  	void (*recycle_ibi_slot)(struct i3c_dev_desc *dev,
>  				 struct i3c_ibi_slot *slot);
> +	void (*update_devs)(struct i3c_master_controller *master);
> +	int (*request_mastership)(struct i3c_master_controller *master);
> +	int (*enable_mr_events)(struct i3c_master_controller *master);
> +	int (*disable_mr_events)(struct i3c_master_controller *master);
>  };
>  
>  /**
> @@ -526,6 +536,8 @@ int i3c_master_defslvs_locked(struct i3c_master_controller *master);
>  int i3c_master_get_free_addr(struct i3c_master_controller *master,
>  			     u8 start_addr);
>  
> +int i3c_master_add_i2c_dev(struct i3c_master_controller *master,
> +			   u16 addr, u8 lvr);
>  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  				  u8 addr);
>  int i3c_master_do_daa(struct i3c_master_controller *master);
> @@ -538,6 +550,11 @@ int i3c_master_register(struct i3c_master_controller *master,
>  			const struct i3c_master_controller_ops *ops,
>  			bool secondary);
>  int i3c_master_unregister(struct i3c_master_controller *master);
> +int i3c_master_get_accmst_locked(struct i3c_master_controller *master,
> +				 u8 addr);
> +int i3c_master_mastership_ack(struct i3c_master_controller *master,
> +			      u8 addr);
> +void i3c_master_register_new_devs(struct i3c_master_controller *master);
>  
>  /**
>   * i3c_dev_get_master_data() - get master private data attached to an I3C

Best regards,
Vitor Soares

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[vitor]

Hi,

On 01/04/19 19:31, Boris Brezillon wrote:
> On Mon, 1 Apr 2019 19:17:03 +0100
> vitor <vitor.soares@synopsys.com> wrote:
>
>> Hi,
>>
>> On 30/03/19 14:36, Boris Brezillon wrote:
>>> On Sun, 10 Mar 2019 13:58:38 +0000
>>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>>>  
>>>> I need to store address and lvr value for I2C devices without static definition
>>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
>>>>
>>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>>>> ---
>>>>  include/linux/i3c/master.h | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>>> index eca8337..3c27d9f 100644
>>>> --- a/include/linux/i3c/master.h
>>>> +++ b/include/linux/i3c/master.h
>>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
>>>>   * @common: common part of the I2C device descriptor
>>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
>>>>   * @dev: I2C device object registered to the I2C framework
>>>> + * @addr: I2C device address
>>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
>>>> + *	 the I2C device limitations
>>>>   *
>>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
>>>>   * This object is created by the core and later attached to the controller
>>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
>>>>  	struct i3c_i2c_dev_desc common;
>>>>  	const struct i2c_dev_boardinfo *boardinfo;
>>>>  	struct i2c_client *dev;
>>>> +	u16 addr;
>>>> +	u8 lvr;  
>>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
>>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
>>> patch 3.
>>>  
>> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?
> Because i2c_dev_boardinfo is extracted from the DT and the secondary
> slaves does not necessarily have this description. The idea is to keep
> reserving the address slot for the I2C device even if we don't expose
> it to the upper layers.

So you are using i2c_dev_boardinfo just for DT devices? Because at end we need to register the new i2c devs.

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Przemyslaw Gaj]

Hi Vitor,

The 04/01/2019 19:48, vitor wrote:
> 
> Hi,
> 
> On 01/04/19 19:31, Boris Brezillon wrote:
> > On Mon, 1 Apr 2019 19:17:03 +0100
> > vitor <vitor.soares@synopsys.com> wrote:
> >
> >> Hi,
> >>
> >> On 30/03/19 14:36, Boris Brezillon wrote:
> >>> On Sun, 10 Mar 2019 13:58:38 +0000
> >>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >>>  
> >>>> I need to store address and lvr value for I2C devices without static definition
> >>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
> >>>>
> >>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> >>>> ---
> >>>>  include/linux/i3c/master.h | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> >>>> index eca8337..3c27d9f 100644
> >>>> --- a/include/linux/i3c/master.h
> >>>> +++ b/include/linux/i3c/master.h
> >>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
> >>>>   * @common: common part of the I2C device descriptor
> >>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
> >>>>   * @dev: I2C device object registered to the I2C framework
> >>>> + * @addr: I2C device address
> >>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> >>>> + *	 the I2C device limitations
> >>>>   *
> >>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
> >>>>   * This object is created by the core and later attached to the controller
> >>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
> >>>>  	struct i3c_i2c_dev_desc common;
> >>>>  	const struct i2c_dev_boardinfo *boardinfo;
> >>>>  	struct i2c_client *dev;
> >>>> +	u16 addr;
> >>>> +	u8 lvr;  
> >>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> >>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> >>> patch 3.
> >>>  
> >> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?
> > Because i2c_dev_boardinfo is extracted from the DT and the secondary
> > slaves does not necessarily have this description. The idea is to keep
> > reserving the address slot for the I2C device even if we don't expose
> > it to the upper layers.
> 
> So you are using i2c_dev_boardinfo just for DT devices? Because at end we need to register the new i2c devs.

Yes, exactly. On secondary master side we are registering only I2C devices with
DT definition.

-- 
-- 
Przemyslaw Gaj

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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon]

On Mon, 1 Apr 2019 19:41:39 +0100
vitor <vitor.soares@synopsys.com> wrote:

> >  void i3c_device_free_ibi(struct i3c_device *dev)
> >  {
> > +	int ret;
> > +
> >  	i3c_bus_normaluse_lock(dev->bus);
> > +	ret = i3c_master_acquire_bus_ownership(dev->desc->common.master);
> > +	if (ret)
> > +		goto err_unlock_bus;
> > +
> >  	if (dev->desc) {
> >  		mutex_lock(&dev->desc->ibi_lock);
> >  		i3c_dev_free_ibi_locked(dev->desc);
> >  		mutex_unlock(&dev->desc->ibi_lock);
> >  	}
> > +
> > +err_unlock_bus:
> >  	i3c_bus_normaluse_unlock(dev->bus);
> >  }
> >  EXPORT_SYMBOL_GPL(i3c_device_free_ibi);  
> 
> Can't we verify if it is the current master no master.c side?

You mean acquiring the bus in i3c_dev_free_ibi_locked()? It doesn't
work if we want things to play well with lockdep (which tracks locking
order to make sure deadlocks can't happen). To acquire the bus you need
to take the bus lock in write/maintenance mode and downgrade it to a
read/normaluse lock once you're done. If you release/acquire the lock
inside i3c_dev_free_ibi_locked() that means some path will have

LOCK(bus);
LOCK(ibi);
UNLOCK(bus);
LOCK(bus);
UNLOCK(ibi);
UNLOCK(bus);

and others

LOCK(bus);
LOCK(ibi);
UNLOCK(ibi);
UNLOCK(bus);

BTW, it won't make a difference in term of LoC to move that to logic
master.c, and the i3c_master_acquire_bus_ownership() helper already
hides most of the complexity.

> 
> > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> > index 86b7b44..929ca6b 100644
> > --- a/drivers/i3c/internals.h
> > +++ b/drivers/i3c/internals.h
> > @@ -14,6 +14,10 @@ extern struct bus_type i3c_bus_type;
> >  
> >  void i3c_bus_normaluse_lock(struct i3c_bus *bus);
> >  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus);
> > +int i3c_master_acquire_bus_ownership(struct i3c_master_controller *master);
> > +int i3c_master_request_mastership_locked(struct i3c_master_controller *master);
> > +
> >  
> >  int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> >  				 struct i3c_priv_xfer *xfers,
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index aea4309..7a84158 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -93,6 +93,20 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus)
> >  	up_read(&bus->lock);
> >  }
> >  
> > +/**
> > + * i3c_bus_downgrade_maintenance_lock - Downgrade the bus lock to normal
> > + * operation
> > + * @bus: I3C bus to downgrade the lock on
> > + *
> > + * Should be called when a maintenance operation is done and normal
> > + * operation is planned. See i3c_bus_maintenance_lock() and
> > + * i3c_bus_normaluse_lock() for more details.
> > + */
> > +void i3c_bus_downgrade_maintenance_lock(struct i3c_bus *bus)
> > +{
> > +	downgrade_write(&bus->lock);
> > +}
> > +
> >  static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev)
> >  {
> >  	return container_of(dev, struct i3c_master_controller, dev);
> > @@ -341,6 +355,22 @@ static int i3c_device_probe(struct device *dev)
> >  	return driver->probe(i3cdev);
> >  }
> >  
> > +static void i3c_master_enable_mr_events(struct i3c_master_controller *master)
> > +{
> > +	if (!master->ops->enable_mr_events)
> > +		return;
> > +
> > +	master->ops->enable_mr_events(master);
> > +}
> > +
> > +static void i3c_master_disable_mr_events(struct i3c_master_controller *master)
> > +{
> > +	if (!master->ops->disable_mr_events)
> > +		return;
> > +
> > +	master->ops->disable_mr_events(master);
> > +}
> > +  
> 
> do we want to do this in broadcast? Is it save to give mastership capabilities to all devices?

Good question. Sounds like a policy decision that we should leave to the
user (kernel parameter, sysfs iface?). Regarding the
i3c_master_controller_ops iface, if we want to allow MR on a per-master
basis, that's something we could store in the i3c_dev_desc struct and
let masters iterate over this list to decide who they should send
ENEC() events to. We probably also want a default policy like reject or
accept all.

Anyway, those are things we can work on once this patch series has
landed.

> >  /**
> > + * i3c_master_register_new_devs() - register new devices
> > + * @master: master used to send frames on the bus
> > + *
> > + * This function is useful when devices were not added
> > + * during initialization or when new device joined the bus
> > + * which was under control of different master.
> > + */
> > +void i3c_master_register_new_devs(struct i3c_master_controller *master)
> > +{
> > +	/*
> > +	 * We can register I3C devices received from master by DEFSLVS.
> > +	 */
> > +	i3c_bus_normaluse_lock(&master->bus);
> > +	i3c_master_register_new_i3c_devs(master);
> > +	i3c_bus_normaluse_unlock(&master->bus);
> > +
> > +	i3c_bus_normaluse_lock(&master->bus);
> > +	i3c_master_register_new_i2c_devs(master);
> > +	i3c_bus_normaluse_unlock(&master->bus);
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_register_new_devs);
> > +  
> 
>  I would say a function to add DEVSLVS. From the master driver you can pack all
>  received slaves into i3c_ccc_defslvs struct and rely the task of add those
>  devices to the subsystem.

Unfortunately, controllers might have dedicated logic parsing the
DEFSLV frame and populating the device table (AFAIR Cadence controller
is working this way). Your solution would imply reconstructing a
DEFSLVS frame which I don't think is worth it. If we have several
controllers exposing directly the DEFSLVS frames, then we can add an
helper to do what you suggest though.



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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Boris Brezillon]

On Mon, 1 Apr 2019 19:48:45 +0100
vitor <vitor.soares@synopsys.com> wrote:

> Hi,
> 
> On 01/04/19 19:31, Boris Brezillon wrote:
> > On Mon, 1 Apr 2019 19:17:03 +0100
> > vitor <vitor.soares@synopsys.com> wrote:
> >  
> >> Hi,
> >>
> >> On 30/03/19 14:36, Boris Brezillon wrote:  
> >>> On Sun, 10 Mar 2019 13:58:38 +0000
> >>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >>>    
> >>>> I need to store address and lvr value for I2C devices without static definition
> >>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
> >>>>
> >>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> >>>> ---
> >>>>  include/linux/i3c/master.h | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> >>>> index eca8337..3c27d9f 100644
> >>>> --- a/include/linux/i3c/master.h
> >>>> +++ b/include/linux/i3c/master.h
> >>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
> >>>>   * @common: common part of the I2C device descriptor
> >>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
> >>>>   * @dev: I2C device object registered to the I2C framework
> >>>> + * @addr: I2C device address
> >>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> >>>> + *	 the I2C device limitations
> >>>>   *
> >>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
> >>>>   * This object is created by the core and later attached to the controller
> >>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
> >>>>  	struct i3c_i2c_dev_desc common;
> >>>>  	const struct i2c_dev_boardinfo *boardinfo;
> >>>>  	struct i2c_client *dev;
> >>>> +	u16 addr;
> >>>> +	u8 lvr;    
> >>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> >>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> >>> patch 3.
> >>>    
> >> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?  
> > Because i2c_dev_boardinfo is extracted from the DT and the secondary
> > slaves does not necessarily have this description. The idea is to keep
> > reserving the address slot for the I2C device even if we don't expose
> > it to the upper layers.  
> 
> So you are using i2c_dev_boardinfo just for DT devices?

All devices that are described in some way, can be through DT, ACPI or
board-file desc (only DT is supported right now, but we can easily add
support for other formats).

> Because at end we need to register the new i2c devs.

No, only those that have a valid description, because there's nothing
you can expose if you only have the address and LVR values. You at
least need to know which driver this dev should be attached to
(compatible string in your DT) and most drivers also need extra
information (basically all the DT props that are described a the DT
binding).

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[vitor]

Hi,


On 01/04/19 20:24, Boris Brezillon wrote:
> On Mon, 1 Apr 2019 19:48:45 +0100
> vitor <vitor.soares@synopsys.com> wrote:
>
>> Hi,
>>
>> On 01/04/19 19:31, Boris Brezillon wrote:
>>> On Mon, 1 Apr 2019 19:17:03 +0100
>>> vitor <vitor.soares@synopsys.com> wrote:
>>>  
>>>> Hi,
>>>>
>>>> On 30/03/19 14:36, Boris Brezillon wrote:  
>>>>> On Sun, 10 Mar 2019 13:58:38 +0000
>>>>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>>>>>    
>>>>>> I need to store address and lvr value for I2C devices without static definition
>>>>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
>>>>>>
>>>>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>>>>>> ---
>>>>>>  include/linux/i3c/master.h | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>>>>> index eca8337..3c27d9f 100644
>>>>>> --- a/include/linux/i3c/master.h
>>>>>> +++ b/include/linux/i3c/master.h
>>>>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
>>>>>>   * @common: common part of the I2C device descriptor
>>>>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
>>>>>>   * @dev: I2C device object registered to the I2C framework
>>>>>> + * @addr: I2C device address
>>>>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
>>>>>> + *	 the I2C device limitations
>>>>>>   *
>>>>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
>>>>>>   * This object is created by the core and later attached to the controller
>>>>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
>>>>>>  	struct i3c_i2c_dev_desc common;
>>>>>>  	const struct i2c_dev_boardinfo *boardinfo;
>>>>>>  	struct i2c_client *dev;
>>>>>> +	u16 addr;
>>>>>> +	u8 lvr;    
>>>>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
>>>>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
>>>>> patch 3.
>>>>>    
>>>> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?  
>>> Because i2c_dev_boardinfo is extracted from the DT and the secondary
>>> slaves does not necessarily have this description. The idea is to keep
>>> reserving the address slot for the I2C device even if we don't expose
>>> it to the upper layers.  
>> So you are using i2c_dev_boardinfo just for DT devices?
> All devices that are described in some way, can be through DT, ACPI or
> board-file desc (only DT is supported right now, but we can easily add
> support for other formats).

We can drop i2c_dev_boardinfo.

>
>> Because at end we need to register the new i2c devs.
> No, only those that have a valid description, because there's nothing
> you can expose if you only have the address and LVR values. You at
> least need to know which driver this dev should be attached to
> (compatible string in your DT) and most drivers also need extra
> information (basically all the DT props that are described a the DT
> binding).

In any case the HC need to know the bus mode, how is this done?

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Przemyslaw Gaj]

Hi,

The 04/02/2019 12:10, vitor wrote:
> 
> Hi,
> 
> 
> On 01/04/19 20:24, Boris Brezillon wrote:
> > On Mon, 1 Apr 2019 19:48:45 +0100
> > vitor <vitor.soares@synopsys.com> wrote:
> >
> >> Hi,
> >>
> >> On 01/04/19 19:31, Boris Brezillon wrote:
> >>> On Mon, 1 Apr 2019 19:17:03 +0100
> >>> vitor <vitor.soares@synopsys.com> wrote:
> >>>  
> >>>> Hi,
> >>>>
> >>>> On 30/03/19 14:36, Boris Brezillon wrote:  
> >>>>> On Sun, 10 Mar 2019 13:58:38 +0000
> >>>>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >>>>>    
> >>>>>> I need to store address and lvr value for I2C devices without static definition
> >>>>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
> >>>>>>
> >>>>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> >>>>>> ---
> >>>>>>  include/linux/i3c/master.h | 5 +++++
> >>>>>>  1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> >>>>>> index eca8337..3c27d9f 100644
> >>>>>> --- a/include/linux/i3c/master.h
> >>>>>> +++ b/include/linux/i3c/master.h
> >>>>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
> >>>>>>   * @common: common part of the I2C device descriptor
> >>>>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
> >>>>>>   * @dev: I2C device object registered to the I2C framework
> >>>>>> + * @addr: I2C device address
> >>>>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> >>>>>> + *	 the I2C device limitations
> >>>>>>   *
> >>>>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
> >>>>>>   * This object is created by the core and later attached to the controller
> >>>>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
> >>>>>>  	struct i3c_i2c_dev_desc common;
> >>>>>>  	const struct i2c_dev_boardinfo *boardinfo;
> >>>>>>  	struct i2c_client *dev;
> >>>>>> +	u16 addr;
> >>>>>> +	u8 lvr;    
> >>>>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> >>>>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> >>>>> patch 3.
> >>>>>    
> >>>> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?  
> >>> Because i2c_dev_boardinfo is extracted from the DT and the secondary
> >>> slaves does not necessarily have this description. The idea is to keep
> >>> reserving the address slot for the I2C device even if we don't expose
> >>> it to the upper layers.  
> >> So you are using i2c_dev_boardinfo just for DT devices?
> > All devices that are described in some way, can be through DT, ACPI or
> > board-file desc (only DT is supported right now, but we can easily add
> > support for other formats).
> 
> We can drop i2c_dev_boardinfo.
>

Why you think we can drop it? We are still using it for DT devices.

> >
> >> Because at end we need to register the new i2c devs.
> > No, only those that have a valid description, because there's nothing
> > you can expose if you only have the address and LVR values. You at
> > least need to know which driver this dev should be attached to
> > (compatible string in your DT) and most drivers also need extra
> > information (basically all the DT props that are described a the DT
> > binding).
> 
> In any case the HC need to know the bus mode, how is this done?

LVR is passed through DEFSLVS command.

-- 
-- 
Przemyslaw Gaj

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[vitor]

On 02/04/19 12:22, Przemyslaw Gaj wrote:
> Hi,
>
> The 04/02/2019 12:10, vitor wrote:
>> Hi,
>>
>>
>> On 01/04/19 20:24, Boris Brezillon wrote:
>>> On Mon, 1 Apr 2019 19:48:45 +0100
>>> vitor <vitor.soares@synopsys.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> On 01/04/19 19:31, Boris Brezillon wrote:
>>>>> On Mon, 1 Apr 2019 19:17:03 +0100
>>>>> vitor <vitor.soares@synopsys.com> wrote:
>>>>>  
>>>>>> Hi,
>>>>>>
>>>>>> On 30/03/19 14:36, Boris Brezillon wrote:  
>>>>>>> On Sun, 10 Mar 2019 13:58:38 +0000
>>>>>>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
>>>>>>>    
>>>>>>>> I need to store address and lvr value for I2C devices without static definition
>>>>>>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
>>>>>>>>
>>>>>>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
>>>>>>>> ---
>>>>>>>>  include/linux/i3c/master.h | 5 +++++
>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>>>>>>> index eca8337..3c27d9f 100644
>>>>>>>> --- a/include/linux/i3c/master.h
>>>>>>>> +++ b/include/linux/i3c/master.h
>>>>>>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
>>>>>>>>   * @common: common part of the I2C device descriptor
>>>>>>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
>>>>>>>>   * @dev: I2C device object registered to the I2C framework
>>>>>>>> + * @addr: I2C device address
>>>>>>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
>>>>>>>> + *	 the I2C device limitations
>>>>>>>>   *
>>>>>>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
>>>>>>>>   * This object is created by the core and later attached to the controller
>>>>>>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
>>>>>>>>  	struct i3c_i2c_dev_desc common;
>>>>>>>>  	const struct i2c_dev_boardinfo *boardinfo;
>>>>>>>>  	struct i2c_client *dev;
>>>>>>>> +	u16 addr;
>>>>>>>> +	u8 lvr;    
>>>>>>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
>>>>>>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
>>>>>>> patch 3.
>>>>>>>    
>>>>>> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?  
>>>>> Because i2c_dev_boardinfo is extracted from the DT and the secondary
>>>>> slaves does not necessarily have this description. The idea is to keep
>>>>> reserving the address slot for the I2C device even if we don't expose
>>>>> it to the upper layers.  
>>>> So you are using i2c_dev_boardinfo just for DT devices?
>>> All devices that are described in some way, can be through DT, ACPI or
>>> board-file desc (only DT is supported right now, but we can easily add
>>> support for other formats).
>> We can drop i2c_dev_boardinfo.
>>
> Why you think we can drop it? We are still using it for DT devices.

Until now you need it to hold LVR, if you pass it to i2c_dev_desc, the i2c_board_info can be used for DT.

>
>>>> Because at end we need to register the new i2c devs.
>>> No, only those that have a valid description, because there's nothing
>>> you can expose if you only have the address and LVR values. You at
>>> least need to know which driver this dev should be attached to
>>> (compatible string in your DT) and most drivers also need extra
>>> information (basically all the DT props that are described a the DT
>>> binding).
>> In any case the HC need to know the bus mode, how is this done?
> LVR is passed through DEFSLVS command.

Is HC driver who set the bus mode? Shouldn't be the subsystem when you register all devices?


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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Boris Brezillon]

On Sat, 30 Mar 2019 15:36:18 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Sun, 10 Mar 2019 13:58:38 +0000
> Przemyslaw Gaj <pgaj@cadence.com> wrote:
> 
> > I need to store address and lvr value for I2C devices without static definition
> > in DT. This allows secondary master to transmit DEFSLVS command properly.
> > 
> > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> > ---
> >  include/linux/i3c/master.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > index eca8337..3c27d9f 100644
> > --- a/include/linux/i3c/master.h
> > +++ b/include/linux/i3c/master.h
> > @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
> >   * @common: common part of the I2C device descriptor
> >   * @boardinfo: pointer to the boardinfo attached to this I2C device
> >   * @dev: I2C device object registered to the I2C framework
> > + * @addr: I2C device address
> > + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> > + *	 the I2C device limitations
> >   *
> >   * Each I2C device connected on the bus will have an i2c_dev_desc.
> >   * This object is created by the core and later attached to the controller
> > @@ -84,6 +87,8 @@ struct i2c_dev_desc {
> >  	struct i3c_i2c_dev_desc common;
> >  	const struct i2c_dev_boardinfo *boardinfo;
> >  	struct i2c_client *dev;
> > +	u16 addr;
> > +	u8 lvr;  
> 
> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> patch 3.

Hm, looks like I was wrong, you must keep i2c_dev_boardinfo->lvr as
i2c_dev_desc instances and i2c_dev_boardinfo objects are created
separately.

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

Re: [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure

[Boris Brezillon]

On Tue, 2 Apr 2019 12:32:41 +0100
vitor <vitor.soares@synopsys.com> wrote:

> On 02/04/19 12:22, Przemyslaw Gaj wrote:
> > Hi,
> >
> > The 04/02/2019 12:10, vitor wrote:  
> >> Hi,
> >>
> >>
> >> On 01/04/19 20:24, Boris Brezillon wrote:  
> >>> On Mon, 1 Apr 2019 19:48:45 +0100
> >>> vitor <vitor.soares@synopsys.com> wrote:
> >>>  
> >>>> Hi,
> >>>>
> >>>> On 01/04/19 19:31, Boris Brezillon wrote:  
> >>>>> On Mon, 1 Apr 2019 19:17:03 +0100
> >>>>> vitor <vitor.soares@synopsys.com> wrote:
> >>>>>    
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 30/03/19 14:36, Boris Brezillon wrote:    
> >>>>>>> On Sun, 10 Mar 2019 13:58:38 +0000
> >>>>>>> Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >>>>>>>      
> >>>>>>>> I need to store address and lvr value for I2C devices without static definition
> >>>>>>>> in DT. This allows secondary master to transmit DEFSLVS command properly.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> >>>>>>>> ---
> >>>>>>>>  include/linux/i3c/master.h | 5 +++++
> >>>>>>>>  1 file changed, 5 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> >>>>>>>> index eca8337..3c27d9f 100644
> >>>>>>>> --- a/include/linux/i3c/master.h
> >>>>>>>> +++ b/include/linux/i3c/master.h
> >>>>>>>> @@ -71,6 +71,9 @@ struct i2c_dev_boardinfo {
> >>>>>>>>   * @common: common part of the I2C device descriptor
> >>>>>>>>   * @boardinfo: pointer to the boardinfo attached to this I2C device
> >>>>>>>>   * @dev: I2C device object registered to the I2C framework
> >>>>>>>> + * @addr: I2C device address
> >>>>>>>> + * @lvr: LVR (Legacy Virtual Register) needed by the I3C core to know about
> >>>>>>>> + *	 the I2C device limitations
> >>>>>>>>   *
> >>>>>>>>   * Each I2C device connected on the bus will have an i2c_dev_desc.
> >>>>>>>>   * This object is created by the core and later attached to the controller
> >>>>>>>> @@ -84,6 +87,8 @@ struct i2c_dev_desc {
> >>>>>>>>  	struct i3c_i2c_dev_desc common;
> >>>>>>>>  	const struct i2c_dev_boardinfo *boardinfo;
> >>>>>>>>  	struct i2c_client *dev;
> >>>>>>>> +	u16 addr;
> >>>>>>>> +	u8 lvr;      
> >>>>>>> You also need to remove lvr from i2c_dev_boardinfo and adjust the code
> >>>>>>> to use i2c_dev_desc->addr and i2c_dev_desc->lvr in this patch, not in
> >>>>>>> patch 3.
> >>>>>>>      
> >>>>>> Why can't we keep the lvr and addr in i2c_dev_boardinfo and need this information on i2c_dev_desc?    
> >>>>> Because i2c_dev_boardinfo is extracted from the DT and the secondary
> >>>>> slaves does not necessarily have this description. The idea is to keep
> >>>>> reserving the address slot for the I2C device even if we don't expose
> >>>>> it to the upper layers.    
> >>>> So you are using i2c_dev_boardinfo just for DT devices?  
> >>> All devices that are described in some way, can be through DT, ACPI or
> >>> board-file desc (only DT is supported right now, but we can easily add
> >>> support for other formats).  
> >> We can drop i2c_dev_boardinfo.
> >>  
> > Why you think we can drop it? We are still using it for DT devices.  
> 
> Until now you need it to hold LVR, if you pass it to i2c_dev_desc, the i2c_board_info can be used for DT.

It also contains a list node which we need to attach the object to the
i2c boardinfo list. Anyway, looks like I was wrong and
i2c_board_info->lvr is still needed.

> 
> >  
> >>>> Because at end we need to register the new i2c devs.  
> >>> No, only those that have a valid description, because there's nothing
> >>> you can expose if you only have the address and LVR values. You at
> >>> least need to know which driver this dev should be attached to
> >>> (compatible string in your DT) and most drivers also need extra
> >>> information (basically all the DT props that are described a the DT
> >>> binding).  
> >> In any case the HC need to know the bus mode, how is this done?  
> > LVR is passed through DEFSLVS command.  
> 
> Is HC driver who set the bus mode? Shouldn't be the subsystem when you register all devices?
> 

The framework selects the mode based on the devices attached to the bus.
The reception of a DEFSLVS frame should trigger the creation of I3C/I2C
devices which will in turn update the bus mode. Of course, the HC
driver must update the bus config accordingly.

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

RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Vitor Soares]

Hi Przemek,

From my analyses you do i3c_master_register for secondary master in follow cases:
- HC has already an dynamic address and it is the owner of the bus.
Or 
- when it receive MR event enable.

Is this correct?

From: vitor <soares@synopsys.com>
Date: Mon, Apr 01, 2019 at 19:41:39

> Hi Przemek,
> 
> > +/**
> >   * i3c_master_register() - register an I3C master
> >   * @master: master used to send frames on the bus
> >   * @parent: the parent device (the one that provides this I3C master
> >   *	    controller)
> >   * @ops: the master controller operations
> > - * @secondary: true if you are registering a secondary master. Will return
> > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > - *	       supported
> > + * @secondary: true if you are registering a secondary master.
> >   *
> >   * This function takes care of everything for you:
> >   *
> > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller 
> *master,
> >  	struct i2c_dev_boardinfo *i2cbi;
> >  	int ret;
> >  
> > -	/* We do not support secondary masters yet. */
> > -	if (secondary)
> > -		return -ENOTSUPP;
> > -
> >  	ret = i3c_master_check_ops(ops);
> >  	if (ret)
> >  		return ret;

The bus mode is set before the sec master does i3c_master_add_i3c_dev_locked() and
i3c_master_add_i2c_dev() and after that it isn't updated. This can cause troubles for
HDR-TS modes and when you have i2c devices INDEX(2) on the bus.

> > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller 
> *master,
> >  	 * register I3C devices dicovered during the initial DAA.
> >  	 */
> >  	master->init_done = true;
> > -	i3c_bus_normaluse_lock(&master->bus);
> > -	i3c_master_register_new_i3c_devs(master);
> > -	i3c_bus_normaluse_unlock(&master->bus);
> > +	i3c_master_register_new_devs(master);
> > +
> > +	i3c_bus_maintenance_lock(&master->bus);
> > +	i3c_master_enable_mr_events(master);

Why are you enabling MR events here? As a standard function this might case issues
because the master can support ENEC/DISEC commands but not multi-master typologies.

> > +	i3c_bus_maintenance_unlock(&master->bus);
> >  
> >  	return 0;
> >  
> > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller 
> *master,
> >  EXPORT_SYMBOL_GPL(i3c_master_register);
> >  

I'm thinking if isn't better to instantiate the bus apart and them register the secondary master.
In this way you are able to add DEFSLVS even before the HC has enable MR events like it is done
with dt devices.

Best regards,
Vitor Soares








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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Przemyslaw Gaj]

Hi Vitor,

The 04/09/2019 14:31, Vitor Soares wrote:
> 
> Hi Przemek,
> 
> From my analyses you do i3c_master_register for secondary master in follow cases:
> - HC has already an dynamic address and it is the owner of the bus.

Correct. It requests bus ownership before registration, if DA is assigned.

> Or 
> - when it receive MR event enabled

Actually, when MR event is enabled and secondary master is not initialized yet.

> 
> Is this correct?
> 
> From: vitor <soares@synopsys.com>
> Date: Mon, Apr 01, 2019 at 19:41:39
> 
> > Hi Przemek,
> > 
> > > +/**
> > >   * i3c_master_register() - register an I3C master
> > >   * @master: master used to send frames on the bus
> > >   * @parent: the parent device (the one that provides this I3C master
> > >   *	    controller)
> > >   * @ops: the master controller operations
> > > - * @secondary: true if you are registering a secondary master. Will return
> > > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > > - *	       supported
> > > + * @secondary: true if you are registering a secondary master.
> > >   *
> > >   * This function takes care of everything for you:
> > >   *
> > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct i3c_master_controller 
> > *master,
> > >  	struct i2c_dev_boardinfo *i2cbi;
> > >  	int ret;
> > >  
> > > -	/* We do not support secondary masters yet. */
> > > -	if (secondary)
> > > -		return -ENOTSUPP;
> > > -
> > >  	ret = i3c_master_check_ops(ops);
> > >  	if (ret)
> > >  		return ret;
> 
> The bus mode is set before the sec master does i3c_master_add_i3c_dev_locked() and
> i3c_master_add_i2c_dev() and after that it isn't updated. This can cause troubles for
> HDR-TS modes and when you have i2c devices INDEX(2) on the bus.

Yes, I see this now. It may happen before HC driver updates device list. I
should update device list right before i3c_master_register().

> 
> > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct i3c_master_controller 
> > *master,
> > >  	 * register I3C devices dicovered during the initial DAA.
> > >  	 */
> > >  	master->init_done = true;
> > > -	i3c_bus_normaluse_lock(&master->bus);
> > > -	i3c_master_register_new_i3c_devs(master);
> > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > +	i3c_master_register_new_devs(master);
> > > +
> > > +	i3c_bus_maintenance_lock(&master->bus);
> > > +	i3c_master_enable_mr_events(master);
> 
> Why are you enabling MR events here? As a standard function this might case issues
> because the master can support ENEC/DISEC commands but not multi-master typologies.

I enable it at the end of master registration to be sure that current master is
ready for bus mastership relinquish. If controller does not support secondary
master role it can just do nothing with it.

> 
> > > +	i3c_bus_maintenance_unlock(&master->bus);
> > >  
> > >  	return 0;
> > >  
> > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct i3c_master_controller 
> > *master,
> > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > >  
> 
> I'm thinking if isn't better to instantiate the bus apart and them register the secondary master.

It looked like that before but we decided to register everything or nothing.

> In this way you are able to add DEFSLVS even before the HC has enable MR events like it is done
> with dt devices.

I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received from
current master right after ENTDAA. Could you please explain?

> 
> Best regards,
> Vitor Soares
> 
> 
> 
> 
> 
> 
> 
> 

-- 
-- 
Przemyslaw Gaj

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

RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Vitor Soares]

Hi Przemek,

From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Tue, Apr 09, 2019 at 16:20:30

> Hi Vitor,
> 
> The 04/09/2019 14:31, Vitor Soares wrote:
> > 
> > Hi Przemek,
> > 
> > From my analyses you do i3c_master_register for secondary master in follow 
> cases:
> > - HC has already an dynamic address and it is the owner of the bus.
> 
> Correct. It requests bus ownership before registration, if DA is assigned.
> 
> > Or 
> > - when it receive MR event enabled
> 
> Actually, when MR event is enabled and secondary master is not initialized 
> yet.

I didn't find where you do this verification, can you point me?

> 
> > 
> > Is this correct?
> > 
> > From: vitor <soares@synopsys.com>
> > Date: Mon, Apr 01, 2019 at 19:41:39
> > 
> > > Hi Przemek,
> > > 
> > > > +/**
> > > >   * i3c_master_register() - register an I3C master
> > > >   * @master: master used to send frames on the bus
> > > >   * @parent: the parent device (the one that provides this I3C master
> > > >   *	    controller)
> > > >   * @ops: the master controller operations
> > > > - * @secondary: true if you are registering a secondary master. Will 
> return
> > > > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > > > - *	       supported
> > > > + * @secondary: true if you are registering a secondary master.
> > > >   *
> > > >   * This function takes care of everything for you:
> > > >   *
> > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct 
> i3c_master_controller 
> > > *master,
> > > >  	struct i2c_dev_boardinfo *i2cbi;
> > > >  	int ret;
> > > >  
> > > > -	/* We do not support secondary masters yet. */
> > > > -	if (secondary)
> > > > -		return -ENOTSUPP;
> > > > -
> > > >  	ret = i3c_master_check_ops(ops);
> > > >  	if (ret)
> > > >  		return ret;
> > 
> > The bus mode is set before the sec master does 
> i3c_master_add_i3c_dev_locked() and
> > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause 
> troubles for
> > HDR-TS modes and when you have i2c devices INDEX(2) on the bus.
> 
> Yes, I see this now. It may happen before HC driver updates device list. I
> should update device list right before i3c_master_register().
> 
> > 
> > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct 
> i3c_master_controller 
> > > *master,
> > > >  	 * register I3C devices dicovered during the initial DAA.
> > > >  	 */
> > > >  	master->init_done = true;
> > > > -	i3c_bus_normaluse_lock(&master->bus);
> > > > -	i3c_master_register_new_i3c_devs(master);
> > > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > > +	i3c_master_register_new_devs(master);
> > > > +
> > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > +	i3c_master_enable_mr_events(master);
> > 
> > Why are you enabling MR events here? As a standard function this might case 
> issues
> > because the master can support ENEC/DISEC commands but not multi-master 
> typologies.
> 
> I enable it at the end of master registration to be sure that current master 
> is
> ready for bus mastership relinquish. If controller does not support secondary
> master role it can just do nothing with it.

I think it isn't good idea to have this here because you are forcing HC driver to
verify it. If it is to be done in subsystem side probably it is better to have a
master/slave functionalities structure.

> 
> > 
> > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > >  
> > > >  	return 0;
> > > >  
> > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct 
> i3c_master_controller 
> > > *master,
> > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > >  
> > 
> > I'm thinking if isn't better to instantiate the bus apart and them register 
> the secondary master.
> 
> It looked like that before but we decided to register everything or nothing.

I see your point, but for the future, slave implementation, we can have a
function to instantiate just the bus and another for master or slave.

> 
> > In this way you are able to add DEFSLVS even before the HC has enable MR 
> events like it is done
> > with dt devices.
> 
> I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> from
> current master right after ENTDAA. Could you please explain?

So the current master receive an hot join and send the DEFSLVS, when do you
add them to the bus? Will you go for the all process to get all dev info and so on?

> 
> > 
> > Best regards,
> > Vitor Soares
> 
> -- 
> -- 
> Przemyslaw Gaj
> 

Best regards,
Vitor Soares

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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Przemyslaw Gaj]

Hi Vitor,

The 04/09/2019 15:46, Vitor Soares wrote:
> 
> Hi Przemek,
> 
> From: Przemyslaw Gaj <pgaj@cadence.com>
> Date: Tue, Apr 09, 2019 at 16:20:30
> 
> > Hi Vitor,
> > 
> > The 04/09/2019 14:31, Vitor Soares wrote:
> > > 
> > > Hi Przemek,
> > > 
> > > From my analyses you do i3c_master_register for secondary master in follow 
> > cases:
> > > - HC has already an dynamic address and it is the owner of the bus.
> > 
> > Correct. It requests bus ownership before registration, if DA is assigned.
> > 
> > > Or 
> > > - when it receive MR event enabled
> > 
> > Actually, when MR event is enabled and secondary master is not initialized 
> > yet.
> 
> I didn't find where you do this verification, can you point me?

In the driver, cdns_i3c_sec_master_event_up(). I'm requesting mastership and
registering the master when event is enabled and master is not registered yet.

> 
> > 
> > > 
> > > Is this correct?
> > > 
> > > From: vitor <soares@synopsys.com>
> > > Date: Mon, Apr 01, 2019 at 19:41:39
> > > 
> > > > Hi Przemek,
> > > > 
> > > > > +/**
> > > > >   * i3c_master_register() - register an I3C master
> > > > >   * @master: master used to send frames on the bus
> > > > >   * @parent: the parent device (the one that provides this I3C master
> > > > >   *	    controller)
> > > > >   * @ops: the master controller operations
> > > > > - * @secondary: true if you are registering a secondary master. Will 
> > return
> > > > > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > > > > - *	       supported
> > > > > + * @secondary: true if you are registering a secondary master.
> > > > >   *
> > > > >   * This function takes care of everything for you:
> > > > >   *
> > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct 
> > i3c_master_controller 
> > > > *master,
> > > > >  	struct i2c_dev_boardinfo *i2cbi;
> > > > >  	int ret;
> > > > >  
> > > > > -	/* We do not support secondary masters yet. */
> > > > > -	if (secondary)
> > > > > -		return -ENOTSUPP;
> > > > > -
> > > > >  	ret = i3c_master_check_ops(ops);
> > > > >  	if (ret)
> > > > >  		return ret;
> > > 
> > > The bus mode is set before the sec master does 
> > i3c_master_add_i3c_dev_locked() and
> > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause 
> > troubles for
> > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus.
> > 
> > Yes, I see this now. It may happen before HC driver updates device list. I
> > should update device list right before i3c_master_register().
> > 
> > > 
> > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct 
> > i3c_master_controller 
> > > > *master,
> > > > >  	 * register I3C devices dicovered during the initial DAA.
> > > > >  	 */
> > > > >  	master->init_done = true;
> > > > > -	i3c_bus_normaluse_lock(&master->bus);
> > > > > -	i3c_master_register_new_i3c_devs(master);
> > > > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > > > +	i3c_master_register_new_devs(master);
> > > > > +
> > > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > > +	i3c_master_enable_mr_events(master);
> > > 
> > > Why are you enabling MR events here? As a standard function this might case 
> > issues
> > > because the master can support ENEC/DISEC commands but not multi-master 
> > typologies.
> > 
> > I enable it at the end of master registration to be sure that current master 
> > is
> > ready for bus mastership relinquish. If controller does not support secondary
> > master role it can just do nothing with it.
> 
> I think it isn't good idea to have this here because you are forcing HC driver to
> verify it. If it is to be done in subsystem side probably it is better to have a
> master/slave functionalities structure.

If multi-master topology is not supported, request_mastership() hook won't be
implemented and also you will not implement enable/disable_mr_events() hooks.
You don't have to verify it in the driver.

> 
> > 
> > > 
> > > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > >  
> > > > >  	return 0;
> > > > >  
> > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct 
> > i3c_master_controller 
> > > > *master,
> > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > >  
> > > 
> > > I'm thinking if isn't better to instantiate the bus apart and them register 
> > the secondary master.
> > 
> > It looked like that before but we decided to register everything or nothing.
> 
> I see your point, but for the future, slave implementation, we can have a
> function to instantiate just the bus and another for master or slave.
> 

We can go the same way with slave, and register bus and slave at once. If this
is the case.

> > 
> > > In this way you are able to add DEFSLVS even before the HC has enable MR 
> > events like it is done
> > > with dt devices.
> > 
> > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > from
> > current master right after ENTDAA. Could you please explain?
> 
> So the current master receive an hot join and send the DEFSLVS, when do you
> add them to the bus? Will you go for the all process to get all dev info and so on?
> 

When current master receives an hot-join, do ENTDAA to enumerate device which
joined the bus and then sends DEFSLVS. This data is stored in secondary master
controller and if secondary master can request mastership, collects all
required informations and adds the devices. It has to collect PID, DEFSLVS does
not provide this information.

> > 
> > > 
> > > Best regards,
> > > Vitor Soares
> > 
> > -- 
> > -- 
> > Przemyslaw Gaj
> > 
> 
> Best regards,
> Vitor Soares
> 

-- 
-- 
Przemyslaw Gaj

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

RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Vitor Soares]

Hi Przemek,

From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Wed, Apr 10, 2019 at 07:53:40

> Hi Vitor,
> 
> The 04/09/2019 15:46, Vitor Soares wrote:
> > 
> > Hi Przemek,
> > 
> > From: Przemyslaw Gaj <pgaj@cadence.com>
> > Date: Tue, Apr 09, 2019 at 16:20:30
> > 
> > > Hi Vitor,
> > > 
> > > The 04/09/2019 14:31, Vitor Soares wrote:
> > > > 
> > > > Hi Przemek,
> > > > 
> > > > From my analyses you do i3c_master_register for secondary master in follow 
> > > cases:
> > > > - HC has already an dynamic address and it is the owner of the bus.
> > > 
> > > Correct. It requests bus ownership before registration, if DA is assigned.
> > > 
> > > > Or 
> > > > - when it receive MR event enabled
> > > 
> > > Actually, when MR event is enabled and secondary master is not initialized 
> > > yet.
> > 
> > I didn't find where you do this verification, can you point me?
> 
> In the driver, cdns_i3c_sec_master_event_up(). I'm requesting mastership and
> registering the master when event is enabled and master is not registered yet.
> 

Sorry, I really don't see that. I assume it is some logic in the 
controller.

> > 
> > > 
> > > > 
> > > > Is this correct?
> > > > 
> > > > From: vitor <soares@synopsys.com>
> > > > Date: Mon, Apr 01, 2019 at 19:41:39
> > > > 
> > > > > Hi Przemek,
> > > > > 
> > > > > > +/**
> > > > > >   * i3c_master_register() - register an I3C master
> > > > > >   * @master: master used to send frames on the bus
> > > > > >   * @parent: the parent device (the one that provides this I3C master
> > > > > >   *	    controller)
> > > > > >   * @ops: the master controller operations
> > > > > > - * @secondary: true if you are registering a secondary master. Will 
> > > return
> > > > > > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > > > > > - *	       supported
> > > > > > + * @secondary: true if you are registering a secondary master.
> > > > > >   *
> > > > > >   * This function takes care of everything for you:
> > > > > >   *
> > > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct 
> > > i3c_master_controller 
> > > > > *master,
> > > > > >  	struct i2c_dev_boardinfo *i2cbi;
> > > > > >  	int ret;
> > > > > >  
> > > > > > -	/* We do not support secondary masters yet. */
> > > > > > -	if (secondary)
> > > > > > -		return -ENOTSUPP;
> > > > > > -
> > > > > >  	ret = i3c_master_check_ops(ops);
> > > > > >  	if (ret)
> > > > > >  		return ret;
> > > > 
> > > > The bus mode is set before the sec master does 
> > > i3c_master_add_i3c_dev_locked() and
> > > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause 
> > > troubles for
> > > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus.
> > > 
> > > Yes, I see this now. It may happen before HC driver updates device list. I
> > > should update device list right before i3c_master_register().
> > > 
> > > > 
> > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct 
> > > i3c_master_controller 
> > > > > *master,
> > > > > >  	 * register I3C devices dicovered during the initial DAA.
> > > > > >  	 */
> > > > > >  	master->init_done = true;
> > > > > > -	i3c_bus_normaluse_lock(&master->bus);
> > > > > > -	i3c_master_register_new_i3c_devs(master);
> > > > > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > > > > +	i3c_master_register_new_devs(master);
> > > > > > +
> > > > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > > > +	i3c_master_enable_mr_events(master);
> > > > 
> > > > Why are you enabling MR events here? As a standard function this might case 
> > > issues
> > > > because the master can support ENEC/DISEC commands but not multi-master 
> > > typologies.
> > > 
> > > I enable it at the end of master registration to be sure that current master 
> > > is
> > > ready for bus mastership relinquish. If controller does not support secondary
> > > master role it can just do nothing with it.
> > 
> > I think it isn't good idea to have this here because you are forcing HC driver to
> > verify it. If it is to be done in subsystem side probably it is better to have a
> > master/slave functionalities structure.
> 
> If multi-master topology is not supported, request_mastership() hook won't be
> implemented and also you will not implement enable/disable_mr_events() hooks.
> You don't have to verify it in the driver.

In that case we will need a driver for each role/set of available 
features and duplicate the code otherwise this need to be checked in HC 
side.

> 
> > 
> > > 
> > > > 
> > > > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > > >  
> > > > > >  	return 0;
> > > > > >  
> > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct 
> > > i3c_master_controller 
> > > > > *master,
> > > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > >  
> > > > 
> > > > I'm thinking if isn't better to instantiate the bus apart and them register 
> > > the secondary master.
> > > 
> > > It looked like that before but we decided to register everything or nothing.
> > 
> > I see your point, but for the future, slave implementation, we can have a
> > function to instantiate just the bus and another for master or slave.
> > 
> 
> We can go the same way with slave, and register bus and slave at once. If this
> is the case.

What if the slave is a secondary master? In your opinion what should be 
the flow?

> 
> > > 
> > > > In this way you are able to add DEFSLVS even before the HC has enable MR 
> > > events like it is done
> > > > with dt devices.
> > > 
> > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > from
> > > current master right after ENTDAA. Could you please explain?
> > 
> > So the current master receive an hot join and send the DEFSLVS, when do you
> > add them to the bus? Will you go for the all process to get all dev info and so on?
> > 
> 
> When current master receives an hot-join, do ENTDAA to enumerate device which
> joined the bus and then sends DEFSLVS. This data is stored in secondary master
> controller and if secondary master can request mastership, collects all
> required informations and adds the devices. It has to collect PID, DEFSLVS does
> not provide this information.

I see now. You need to take care here because when the secondary master 
add the i3c_dev it might change the address.
This is one of the reasons I would prefer a dedicated function to add the 
DEFSLVS.

Another point is what happen if the current master received MR request 
during this registration?

Best regards,
Vitor Soares

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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon]

On Wed, 10 Apr 2019 10:05:33 +0000
Vitor Soares <vitor.soares@synopsys.com> wrote:


> > > > >   
> > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct   
> > > > i3c_master_controller   
> > > > > > *master,  
> > > > > > >  	 * register I3C devices dicovered during the initial DAA.
> > > > > > >  	 */
> > > > > > >  	master->init_done = true;
> > > > > > > -	i3c_bus_normaluse_lock(&master->bus);
> > > > > > > -	i3c_master_register_new_i3c_devs(master);
> > > > > > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > > > > > +	i3c_master_register_new_devs(master);
> > > > > > > +
> > > > > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > > > > +	i3c_master_enable_mr_events(master);  
> > > > > 
> > > > > Why are you enabling MR events here? As a standard function this might case   
> > > > issues  
> > > > > because the master can support ENEC/DISEC commands but not multi-master   
> > > > typologies.
> > > > 
> > > > I enable it at the end of master registration to be sure that current master 
> > > > is
> > > > ready for bus mastership relinquish. If controller does not support secondary
> > > > master role it can just do nothing with it.  
> > > 
> > > I think it isn't good idea to have this here because you are forcing HC driver to
> > > verify it. If it is to be done in subsystem side probably it is better to have a
> > > master/slave functionalities structure.  
> > 
> > If multi-master topology is not supported, request_mastership() hook won't be
> > implemented and also you will not implement enable/disable_mr_events() hooks.
> > You don't have to verify it in the driver.  
> 
> In that case we will need a driver for each role/set of available 
> features and duplicate the code otherwise this need to be checked in HC 
> side.

Not really, just 2 set of ops, one with the hooks set and another one
with the hooks left unassigned (NULL). But I guess we can also add
a caps field where we'd list i3c master caps (secondary master,
supports MR, ...).

> 
> >   
> > >   
> > > >   
> > > > >   
> > > > > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > > > >  
> > > > > > >  	return 0;
> > > > > > >  
> > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct   
> > > > i3c_master_controller   
> > > > > > *master,  
> > > > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > > >    
> > > > > 
> > > > > I'm thinking if isn't better to instantiate the bus apart and them register   
> > > > the secondary master.
> > > > 
> > > > It looked like that before but we decided to register everything or nothing.  
> > > 
> > > I see your point, but for the future, slave implementation, we can have a
> > > function to instantiate just the bus and another for master or slave.
> > >   
> > 
> > We can go the same way with slave, and register bus and slave at once. If this
> > is the case.  

The slave framework isn't there yet, and I don't think we should expose
the bus concept to slave drivers anyway. If a master can act both as a
slave (I mean a slave that can do more than just send MR requests)  and
a master (secondary), the driver should register to both framework.

> 
> What if the slave is a secondary master? In your opinion what should be 
> the flow?

i3c_slave_regiter(&ctrl->slave);
i3c_master_regiter(&ctrl->master);

The order is arbitrary, and those might actually be called from
different path if it makes more sense. I'm just pointing out that
registering a slave and registering a master are 2 different things,
and the master side of the framework should probably not automate slave
registration, at least not until we have a better idea of what the
slave API will look like.

> 
> >   
> > > >   
> > > > > In this way you are able to add DEFSLVS even before the HC has enable MR   
> > > > events like it is done  
> > > > > with dt devices.  
> > > > 
> > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > > from
> > > > current master right after ENTDAA. Could you please explain?  
> > > 
> > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > >   
> > 
> > When current master receives an hot-join, do ENTDAA to enumerate device which
> > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > controller and if secondary master can request mastership, collects all
> > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > not provide this information.  
> 
> I see now. You need to take care here because when the secondary master 
> add the i3c_dev it might change the address.
> This is one of the reasons I would prefer a dedicated function to add the 
> DEFSLVS.

IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
automatically filled based on that. We should only add the DEFSLVS
parsing helper once we start seeing a need for it (probably when adding
secondary slave support since you seem to insist on this aspect :-)).

> 
> Another point is what happen if the current master received MR request 
> during this registration?

You mean when registering the primary I3C master? The driver should
take care of disabling MR interrupts and if possible reject all
incoming MR. MR should only be re-enabled when ->enable_mr_events() is
called by the core.

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

RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Vitor Soares]

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Wed, Apr 10, 2019 at 11:36:03

> On Wed, 10 Apr 2019 10:05:33 +0000
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> 
> > > > > >   
> > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct   
> > > > > i3c_master_controller   
> > > > > > > *master,  
> > > > > > > >  	 * register I3C devices dicovered during the initial DAA.
> > > > > > > >  	 */
> > > > > > > >  	master->init_done = true;
> > > > > > > > -	i3c_bus_normaluse_lock(&master->bus);
> > > > > > > > -	i3c_master_register_new_i3c_devs(master);
> > > > > > > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > > > > > > +	i3c_master_register_new_devs(master);
> > > > > > > > +
> > > > > > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > > > > > +	i3c_master_enable_mr_events(master);  
> > > > > > 
> > > > > > Why are you enabling MR events here? As a standard function this might case   
> > > > > issues  
> > > > > > because the master can support ENEC/DISEC commands but not multi-master   
> > > > > typologies.
> > > > > 
> > > > > I enable it at the end of master registration to be sure that current master 
> > > > > is
> > > > > ready for bus mastership relinquish. If controller does not support secondary
> > > > > master role it can just do nothing with it.  
> > > > 
> > > > I think it isn't good idea to have this here because you are forcing HC driver to
> > > > verify it. If it is to be done in subsystem side probably it is better to have a
> > > > master/slave functionalities structure.  
> > > 
> > > If multi-master topology is not supported, request_mastership() hook won't be
> > > implemented and also you will not implement enable/disable_mr_events() hooks.
> > > You don't have to verify it in the driver.  
> > 
> > In that case we will need a driver for each role/set of available 
> > features and duplicate the code otherwise this need to be checked in HC 
> > side.
> 
> Not really, just 2 set of ops, one with the hooks set and another one
> with the hooks left unassigned (NULL). But I guess we can also add
> a caps field where we'd list i3c master caps (secondary master,
> supports MR, ...).

Yes we can do this, but what I see is to have several ops structures 
based on HC capabilities.

> 
> > 
> > >   
> > > >   
> > > > >   
> > > > > >   
> > > > > > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > > > > >  
> > > > > > > >  	return 0;
> > > > > > > >  
> > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct   
> > > > > i3c_master_controller   
> > > > > > > *master,  
> > > > > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > > > >    
> > > > > > 
> > > > > > I'm thinking if isn't better to instantiate the bus apart and them register   
> > > > > the secondary master.
> > > > > 
> > > > > It looked like that before but we decided to register everything or nothing.  
> > > > 
> > > > I see your point, but for the future, slave implementation, we can have a
> > > > function to instantiate just the bus and another for master or slave.
> > > >   
> > > 
> > > We can go the same way with slave, and register bus and slave at once. If this
> > > is the case.  
> 
> The slave framework isn't there yet, and I don't think we should expose
> the bus concept to slave drivers anyway. If a master can act both as a
> slave (I mean a slave that can do more than just send MR requests)  and
> a master (secondary), the driver should register to both framework.
> 
> > 
> > What if the slave is a secondary master? In your opinion what should be 
> > the flow?
> 
> i3c_slave_regiter(&ctrl->slave);
> i3c_master_regiter(&ctrl->master);
> 
> The order is arbitrary, and those might actually be called from
> different path if it makes more sense. I'm just pointing out that
> registering a slave and registering a master are 2 different things,
> and the master side of the framework should probably not automate slave
> registration, at least not until we have a better idea of what the
> slave API will look like.

We need to understand better your point.

The secondary master can have both roles and even not implementing all 
slave function it is a slave when is not a master.
For me still keep the idea:

        bus
         / \
       /     \
slave    master

and just one role is active at time.

I didn't get why the bus shouldn't be instantiated for slave. Can you 
explain?

In any case we will need a common point to switch the roles.

> 
> > 
> > >   
> > > > >   
> > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR   
> > > > > events like it is done  
> > > > > > with dt devices.  
> > > > > 
> > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > > > from
> > > > > current master right after ENTDAA. Could you please explain?  
> > > > 
> > > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > > >   
> > > 
> > > When current master receives an hot-join, do ENTDAA to enumerate device which
> > > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > > controller and if secondary master can request mastership, collects all
> > > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > > not provide this information.  
> > 
> > I see now. You need to take care here because when the secondary master 
> > add the i3c_dev it might change the address.
> > This is one of the reasons I would prefer a dedicated function to add the 
> > DEFSLVS.
> 
> IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
> automatically filled based on that. We should only add the DEFSLVS
> parsing helper once we start seeing a need for it (probably when adding
> secondary slave support since you seem to insist on this aspect :-)).

For me the subsystem should hold and handle all this information. Anyway, 
this information is received and needed before the mastership takeover.
Basically I want to avoid to put this type of logistics in HC driver 
layer and call the maintenance_lock.

I'm insisting because it seems that I have a different use case to 
address and I don't see how this fit on it.

> 
> > 
> > Another point is what happen if the current master received MR request 
> > during this registration?
> 
> You mean when registering the primary I3C master? The driver should
> take care of disabling MR interrupts and if possible reject all
> incoming MR. MR should only be re-enabled when ->enable_mr_events() is
> called by the core.

I'm aware of that I just didn't see any disable events.


Best regards,
Vitor Soares

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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon]

On Fri, 12 Apr 2019 14:28:26 +0000
Vitor Soares <vitor.soares@synopsys.com> wrote:

> > > What if the slave is a secondary master? In your opinion what should be 
> > > the flow?  
> > 
> > i3c_slave_regiter(&ctrl->slave);
> > i3c_master_regiter(&ctrl->master);
> > 
> > The order is arbitrary, and those might actually be called from
> > different path if it makes more sense. I'm just pointing out that
> > registering a slave and registering a master are 2 different things,
> > and the master side of the framework should probably not automate slave
> > registration, at least not until we have a better idea of what the
> > slave API will look like.  
> 
> We need to understand better your point.
> 
> The secondary master can have both roles and even not implementing all 
> slave function it is a slave when is not a master.
> For me still keep the idea:
> 
>         bus
>          / \
>        /     \
> slave    master
> 
> and just one role is active at time.
> 
> I didn't get why the bus shouldn't be instantiated for slave. Can you 
> explain?

Because, from a SW PoV, pure slave devices don't care about the bus
concept. Do you have use cases where you'd need to know what bus the
slave is connected to?

AFAICT, all you can do is reply to master requests (probably with some
predefined messages, like values stored in a regmap or data queued in
a FIFO).

> 
> In any case we will need a common point to switch the roles.

We'd need a way to relinquish bus ownership, that's all. When the
master is not the current master, it automatically becomes a slave, and
if it has any "I3C slave" profile registered, it can reply to requests
coming from the current master.

> > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR     
> > > > > > events like it is done    
> > > > > > > with dt devices.    
> > > > > > 
> > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > > > > from
> > > > > > current master right after ENTDAA. Could you please explain?    
> > > > > 
> > > > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > > > >     
> > > > 
> > > > When current master receives an hot-join, do ENTDAA to enumerate device which
> > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > > > controller and if secondary master can request mastership, collects all
> > > > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > > > not provide this information.    
> > > 
> > > I see now. You need to take care here because when the secondary master 
> > > add the i3c_dev it might change the address.
> > > This is one of the reasons I would prefer a dedicated function to add the 
> > > DEFSLVS.  
> > 
> > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
> > automatically filled based on that. We should only add the DEFSLVS
> > parsing helper once we start seeing a need for it (probably when adding
> > secondary slave support since you seem to insist on this aspect :-)).  
> 
> For me the subsystem should hold and handle all this information. Anyway, 
> this information is received and needed before the mastership takeover.

It is, and we already have a bunch of helpers to add new devices. Maybe
we need a few more, I'm just saying that forging a DEFSLVS frame to then
pass it to the core is not the right solution IMO. If you need an helper
that automatically parses a DEFSLVS frame and add the new devices, then
fine, add this helper to the framework and use it in your driver, but
don't force other drivers to use this method.

> Basically I want to avoid to put this type of logistics in HC driver 
> layer and call the maintenance_lock.

And yet, I don't think forging a DEFSLVS frame is the right way to
provide the kind of abstraction you're talking about. Note that I said
a few things should be provided as helpers in my review.

> 
> I'm insisting because it seems that I have a different use case to 
> address and I don't see how this fit on it.

Can you be more specific, because we don't know about your use cases.

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

RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Vitor Soares]

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Apr 12, 
2019 at 15:57:19

> On Fri, 12 Apr 2019 14:28:26 +0000
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > > > What if the slave is a secondary master? In your opinion what should be 
> > > > the flow?  
> > > 
> > > i3c_slave_regiter(&ctrl->slave);
> > > i3c_master_regiter(&ctrl->master);
> > > 
> > > The order is arbitrary, and those might actually be called from
> > > different path if it makes more sense. I'm just pointing out that
> > > registering a slave and registering a master are 2 different things,
> > > and the master side of the framework should probably not automate slave
> > > registration, at least not until we have a better idea of what the
> > > slave API will look like.  
> > 
> > We need to understand better your point.
> > 
> > The secondary master can have both roles and even not implementing all 
> > slave function it is a slave when is not a master.
> > For me still keep the idea:
> > 
> >         bus
> >          / \
> >        /     \
> > slave    master
> > 
> > and just one role is active at time.
> > 
> > I didn't get why the bus shouldn't be instantiated for slave. Can you 
> > explain?
> 
> Because, from a SW PoV, pure slave devices don't care about the bus
> concept. Do you have use cases where you'd need to know what bus the
> slave is connected to?
> 

In device model I expect that the slave is under a bus. You may not need 
to know the bus topologies and the devices connect but at least we might 
need to know the bus is alive.

The I2C subsystem implement that and it is working.

I still don't understand your position, is there any other reasons?

> AFAICT, all you can do is reply to master requests (probably with some
> predefined messages, like values stored in a regmap or data queued in
> a FIFO).

We can list it:
  - RX/TX SDR data
  - RX/TX HDR data
  - SIR
  - HJ
  - MR (case of sec master)
  - CCC 
  - and perhaps timing control.

> 
> > 
> > In any case we will need a common point to switch the roles.
> 
> We'd need a way to relinquish bus ownership, that's all. When the
> master is not the current master, it automatically becomes a slave, and
> if it has any "I3C slave" profile registered, it can reply to requests
> coming from the current master.

This is too abstract to me. Can you point a current example? Because for 
me we need a common layer for both master slave structures.

> 
> > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR     
> > > > > > > events like it is done    
> > > > > > > > with dt devices.    
> > > > > > > 
> > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > > > > > from
> > > > > > > current master right after ENTDAA. Could you please explain?    
> > > > > > 
> > > > > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > > > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > > > > >     
> > > > > 
> > > > > When current master receives an hot-join, do ENTDAA to enumerate device which
> > > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > > > > controller and if secondary master can request mastership, collects all
> > > > > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > > > > not provide this information.    
> > > > 
> > > > I see now. You need to take care here because when the secondary master 
> > > > add the i3c_dev it might change the address.
> > > > This is one of the reasons I would prefer a dedicated function to add the 
> > > > DEFSLVS.  
> > > 
> > > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
> > > automatically filled based on that. We should only add the DEFSLVS
> > > parsing helper once we start seeing a need for it (probably when adding
> > > secondary slave support since you seem to insist on this aspect :-)).  
> > 
> > For me the subsystem should hold and handle all this information. Anyway, 
> > this information is received and needed before the mastership takeover.
> 
> It is, and we already have a bunch of helpers to add new devices. Maybe
> we need a few more, I'm just saying that forging a DEFSLVS frame to then
> pass it to the core is not the right solution IMO. 
> If you need an helper
> that automatically parses a DEFSLVS frame and add the new devices, then
> fine, add this helper to the framework and use it in your driver, but
> don't force other drivers to use this method.

This is not based in a need. I can also add the devices one by one.
If I have everything available why not send it straight away to the 
subsystem and let it do the management?
But ok, I understand your point and I can do the helper to parse the 
DEFSLVS.

Another question:
  How do we know what is the main master in case we want to send back the 
bus mastership?

> 
> > Basically I want to avoid to put this type of logistics in HC driver 
> > layer and call the maintenance_lock.
> 
> And yet, I don't think forging a DEFSLVS frame is the right way to
> provide the kind of abstraction you're talking about. 

Can you explain?

> Note that I said
> a few things should be provided as helpers in my review.
> 
> > 
> > I'm insisting because it seems that I have a different use case to 
> > address and I don't see how this fit on it.
> 
> Can you be more specific, because we don't know about your use cases.

According to the spec the sec master may have all function of a Main 
master and may have all function of a slave.

The implementation should made with this in mind and I'm asking 
flexibility for that. 

Best regards,
Vitor Soares

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

Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon]

Hi Vitor,

On Mon, 15 Apr 2019 12:02:02 +0000
Vitor Soares <vitor.soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Apr 12, 
> 2019 at 15:57:19
> 
> > On Fri, 12 Apr 2019 14:28:26 +0000
> > Vitor Soares <vitor.soares@synopsys.com> wrote:
> >   
> > > > > What if the slave is a secondary master? In your opinion what should be 
> > > > > the flow?    
> > > > 
> > > > i3c_slave_regiter(&ctrl->slave);
> > > > i3c_master_regiter(&ctrl->master);
> > > > 
> > > > The order is arbitrary, and those might actually be called from
> > > > different path if it makes more sense. I'm just pointing out that
> > > > registering a slave and registering a master are 2 different things,
> > > > and the master side of the framework should probably not automate slave
> > > > registration, at least not until we have a better idea of what the
> > > > slave API will look like.    
> > > 
> > > We need to understand better your point.
> > > 
> > > The secondary master can have both roles and even not implementing all 
> > > slave function it is a slave when is not a master.
> > > For me still keep the idea:
> > > 
> > >         bus
> > >          / \
> > >        /     \
> > > slave    master
> > > 
> > > and just one role is active at time.
> > > 
> > > I didn't get why the bus shouldn't be instantiated for slave. Can you 
> > > explain?  
> > 
> > Because, from a SW PoV, pure slave devices don't care about the bus
> > concept. Do you have use cases where you'd need to know what bus the
> > slave is connected to?
> >   
> 
> In device model I expect that the slave is under a bus.

Attached to a bus_type, maybe, under an I3C bus object, I'm not
convinced.

> You may not need 
> to know the bus topologies and the devices connect but at least we might 
> need to know the bus is alive.

Yes, AFAICT, the slave status (and potentially the I3C dynamic address)
are the only thing an I3C slave controller driver might need.

> 
> The I2C subsystem implement that and it is working.

But is it needed? That's what I'm questioning here. What would you put
in the bus object if the only thing you know about it is that the slave
controller is present on the bus. Does it make sense to re-use the same
logic/struct for the slave case?
Maybe my feeling about this problem is wrong, but as usual, I don't see
a real use case expressed or a PoC that shows me why you'd need to have
an i3c_bus attached to the slave. So it's hard to say who's right/wrong.
If you come up with an RFC for the slave API we'll have something
concrete to discuss about. Until this happens, I'd like to close this
topic. And of course, I'd appreciate that we keep focusing on the
mastership handover/secondary master case without thinking too much
about how things will be impacted by the slave API.

> 
> I still don't understand your position, is there any other reasons?

My position is: I don't see the point of adding complex concepts to
something that's otherwise way simpler than managing an I3C bus. You can
prove me wrong with a detailed example, or even better, a patch series
adding the slave API plus a reference driver to show how it's supposed
to work :P.

> 
> > AFAICT, all you can do is reply to master requests (probably with some
> > predefined messages, like values stored in a regmap or data queued in
> > a FIFO).  
> 
> We can list it:
>   - RX/TX SDR data
>   - RX/TX HDR data
>   - SIR
>   - HJ
>   - MR (case of sec master)

Yes, but that case falls in the I3C master API.

>   - CCC 
>   - and perhaps timing control.
> 
> >   
> > > 
> > > In any case we will need a common point to switch the roles.  
> > 
> > We'd need a way to relinquish bus ownership, that's all. When the
> > master is not the current master, it automatically becomes a slave, and
> > if it has any "I3C slave" profile registered, it can reply to requests
> > coming from the current master.  
> 
> This is too abstract to me. Can you point a current example? Because for 
> me we need a common layer for both master slave structures.

We need to share a few things (CCC defs and probably other things,
but I don't know which ones yet), yes, but I don't think the i3c_bus
struct is one of them.

I also don't know what the slave API will look like, and that's
actually something someone (you??) should work on before having
advanced discussions about what has to be shared between the
master/slave layers.

> 
> >   
> > > > > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR       
> > > > > > > > events like it is done      
> > > > > > > > > with dt devices.      
> > > > > > > > 
> > > > > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > > > > > > from
> > > > > > > > current master right after ENTDAA. Could you please explain?      
> > > > > > > 
> > > > > > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > > > > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > > > > > >       
> > > > > > 
> > > > > > When current master receives an hot-join, do ENTDAA to enumerate device which
> > > > > > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > > > > > controller and if secondary master can request mastership, collects all
> > > > > > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > > > > > not provide this information.      
> > > > > 
> > > > > I see now. You need to take care here because when the secondary master 
> > > > > add the i3c_dev it might change the address.
> > > > > This is one of the reasons I would prefer a dedicated function to add the 
> > > > > DEFSLVS.    
> > > > 
> > > > IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
> > > > automatically filled based on that. We should only add the DEFSLVS
> > > > parsing helper once we start seeing a need for it (probably when adding
> > > > secondary slave support since you seem to insist on this aspect :-)).    
> > > 
> > > For me the subsystem should hold and handle all this information. Anyway, 
> > > this information is received and needed before the mastership takeover.  
> > 
> > It is, and we already have a bunch of helpers to add new devices. Maybe
> > we need a few more, I'm just saying that forging a DEFSLVS frame to then
> > pass it to the core is not the right solution IMO. 
> > If you need an helper
> > that automatically parses a DEFSLVS frame and add the new devices, then
> > fine, add this helper to the framework and use it in your driver, but
> > don't force other drivers to use this method.  
> 
> This is not based in a need. I can also add the devices one by one.
> If I have everything available why not send it straight away to the 
> subsystem and let it do the management?

This is exactly what I recommend you to do: provide an helper that will
automate that, but don't force other drivers to use this helper. Since
the Cadence driver will not use this helper, it should not be
introduced in this patch series, that's all I said.

> But ok, I understand your point and I can do the helper to parse the 
> DEFSLVS.
> 
> Another question:
>   How do we know what is the main master in case we want to send back the 
> bus mastership?

There's a ->cur_master field in struct i3c_bus. It should be updated
when the bus owner changes. Not sure exactly how other masters can be
informed about that, but those doing the handshake know about the
change, that's for sure.

> 
> >   
> > > Basically I want to avoid to put this type of logistics in HC driver 
> > > layer and call the maintenance_lock.  
> > 
> > And yet, I don't think forging a DEFSLVS frame is the right way to
> > provide the kind of abstraction you're talking about.   
> 
> Can you explain?

I keep explaining you that reconstructing a fake DEFSLVS frame just for
the sake of having all drivers use the helper you want to add is not a
good idea.

To sum-up one last time: adding the
i3c_master_parse_defslvs_and_add_devs() helper is fine, forcing all
drivers to use it is not. Is that clear enough?

> 
> > Note that I said
> > a few things should be provided as helpers in my review.
> >   
> > > 
> > > I'm insisting because it seems that I have a different use case to 
> > > address and I don't see how this fit on it.  
> > 
> > Can you be more specific, because we don't know about your use cases.  
> 
> According to the spec the sec master may have all function of a Main 
> master and may have all function of a slave.

So you're back to the master/slave dual role thing?

> 
> The implementation should made with this in mind and I'm asking 
> flexibility for that. 

What you're asking for is the opposite of flexibility, and most
importantly it's based on assumptions about something that has not been
fully designed yet: the I3C slave API.

So let's have a plan for this discussion to actually lead to something
useful:

1/ propose an RFC for the slave API + a reference driver
2/ think about how dual role devs can implement both interfaces and the
things they need to share

See, we keep discussing #2 while #1 is not even ready yet, and it's
definitely a pre-requisite to #2. So please work #1 first.

Regards,

Boris

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

Re: [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.

[Przemyslaw Gaj]

Hi Boris,

I'm sorry for my late response. I hope you remember this thread :-)
I'm implementing this and have some questions.

The 03/30/2019 16:44, Boris Brezillon wrote:
> >  
> > @@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
> >  
> >  	cdns_i3c_master_enable(master);
> >  
> > +	if (m->secondary) {
> > +		i3c_bus_maintenance_lock(&master->base.bus);
> > +		cdns_i3c_master_update_devs(&master->base);
> > +		i3c_bus_maintenance_unlock(&master->base.bus);
> > +	}
> 
> Okay, I changed my mind on this solution (it's not the first time that
> happens, and unfortunately won't be the last time either :-)). I think
> I don't like the idea of exposing the i3c_bus_maintenance_unlock/lock()
> functions in the end.

Ok :-)

> 
> I'd like to reconsider what you initially proposed: having an
> ->update_devs() hook that is called by the core, except I'd call it
> ->populate_bus().

Ok, we can back to previous approach.

> 
> BTW, there's still something that's unclear to me. You seem to populate
> the bus here and also when acquiring bus ownership. Is this correct?

Yes, this is correct. I'm doing this here to register all the devices received
by DEFSLVS on master initialization time. I'm also populating new devices when
acquiring the bus because some device could join the bus dynamically and we
want to register this new devices on our side also.

> I'd expect it to be 'partially' populated at bus-init+defslvs time,
> which would trigger a mastership request if I3C devices are present (as
> we need to send a GETPID to know more about I3C devs).

So, you want to allocate and attach devices and then, when possible get devices
info and register them? I mean when mastership request is possible. If not,
just leave devices allocated and register them when ENEC(MR) received, correct?

Previously, I allocated and registered all the devices after successful
mastership request. Which way is better in your opinion?

>
> Also, what happens if i3c_master_add_i3c_dev_locked() fails? You
> don't seem to handle that case at all.

For now, I just skipped it silently.

> 
> > +
> > +static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
> > +{
> > +	if (master->base.init_done) {
> 
> Can this really happen that init_done is not set when you reach this
> point.

Yes, it was possible. Mastership was taken but master wasn't registered yet.
With new approach I think this won't happen.

> 
> > +		i3c_bus_maintenance_lock(&master->base.bus);
> > +		cdns_i3c_master_update_devs(&master->base);
> > +		i3c_bus_maintenance_unlock(&master->base.bus);
> > +
> > +		i3c_master_register_new_devs(&master->base);
> 
> The core will somehow be informed that this master now owns the bus, so
> it can call i3c_master_register_new_devs() for us, right?

I think it can. I'm sure it worked like that before. When HC driver changed
cur_master, new devices were populated.

> 
> But as said above, I'm not even sure this is correct to do it from
> here. I'd expect this to happen at DEFSLVS or BUS init time.
> 

Ok. New(Previous) approach allows that.

-- 
-- 
Przemyslaw Gaj

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

Re: [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.

[Boris Brezillon]

On Mon, 29 Apr 2019 11:36:42 +0100
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> Hi Boris,
> 
> I'm sorry for my late response. I hope you remember this thread :-)

Unfortunately not :-/, and it's now my turn to apologize for the late
reply.

> I'm implementing this and have some questions.
> 
> The 03/30/2019 16:44, Boris Brezillon wrote:
> > >  
> > > @@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
> > >  
> > >  	cdns_i3c_master_enable(master);
> > >  
> > > +	if (m->secondary) {
> > > +		i3c_bus_maintenance_lock(&master->base.bus);
> > > +		cdns_i3c_master_update_devs(&master->base);
> > > +		i3c_bus_maintenance_unlock(&master->base.bus);
> > > +	}  
> > 
> > Okay, I changed my mind on this solution (it's not the first time that
> > happens, and unfortunately won't be the last time either :-)). I think
> > I don't like the idea of exposing the i3c_bus_maintenance_unlock/lock()
> > functions in the end.  
> 
> Ok :-)
> 
> > 
> > I'd like to reconsider what you initially proposed: having an  
> > ->update_devs() hook that is called by the core, except I'd call it
> > ->populate_bus().  
> 
> Ok, we can back to previous approach.
> 
> > 
> > BTW, there's still something that's unclear to me. You seem to populate
> > the bus here and also when acquiring bus ownership. Is this correct?  
> 
> Yes, this is correct. I'm doing this here to register all the devices received
> by DEFSLVS on master initialization time. I'm also populating new devices when
> acquiring the bus because some device could join the bus dynamically and we
> want to register this new devices on our side also.

Hm, I don't get that part. I thought we were supposed to add devices as
soon as we know about them. In case of HJ and assuming your master is
not currently owning the bus, the active master should send you a
DEFSLVS which should serve as a trigger for registering new devs on all
non active masters. Since registering new devices will trigger a
mastership handover to query devs info (PID + other stuff) we should be
all good, right?

> 
> > I'd expect it to be 'partially' populated at bus-init+defslvs time,
> > which would trigger a mastership request if I3C devices are present (as
> > we need to send a GETPID to know more about I3C devs).  
> 
> So, you want to allocate and attach devices and then, when possible get devices
> info and register them? I mean when mastership request is possible. If not,
> just leave devices allocated and register them when ENEC(MR) received, correct?

Kind of, yes. We can probably just have a "want_to_acquire_bus" flag
set, and the partially registered/discovered devices present in the
master list would be registered automatically every time the master
acquires bus ownership. This way we can re-use this logic for any
operation that requires the master to own the bus to do something
specific.

> 
> Previously, I allocated and registered all the devices after successful
> mastership request. Which way is better in your opinion?

That's a solution too, but it feels like a lot of generic code is
open-coded in the driver if we do it this way. I know I'm the one who
suggested this approach, but now that I see the code, I realize I was
wrong (sorry about that).

> 
> >
> > Also, what happens if i3c_master_add_i3c_dev_locked() fails? You
> > don't seem to handle that case at all.  
> 
> For now, I just skipped it silently.

We should at least print an error/warning.

> 
> >   
> > > +
> > > +static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
> > > +{
> > > +	if (master->base.init_done) {  
> > 
> > Can this really happen that init_done is not set when you reach this
> > point.  
> 
> Yes, it was possible. Mastership was taken but master wasn't registered yet.
> With new approach I think this won't happen.

One more reason to switch to the new approach.

> 
> >   
> > > +		i3c_bus_maintenance_lock(&master->base.bus);
> > > +		cdns_i3c_master_update_devs(&master->base);
> > > +		i3c_bus_maintenance_unlock(&master->base.bus);
> > > +
> > > +		i3c_master_register_new_devs(&master->base);  
> > 
> > The core will somehow be informed that this master now owns the bus, so
> > it can call i3c_master_register_new_devs() for us, right?  
> 
> I think it can. I'm sure it worked like that before. When HC driver changed
> cur_master, new devices were populated.
> 
> > 
> > But as said above, I'm not even sure this is correct to do it from
> > here. I'd expect this to happen at DEFSLVS or BUS init time.
> >   
> 
> Ok. New(Previous) approach allows that.
> 

Ok, good.

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