All of lore.kernel.org
 help / color / mirror / Atom feed
From: Parshuram Thombare <pthombar@cadence.com>
To: <bbrezillon@kernel.org>, <vitor.soares@synopsys.com>
Cc: <pgaj@cadence.com>, <linux-i3c@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <mparab@cadence.com>,
	<praneeth@ti.com>, Parshuram Thombare <pthombar@cadence.com>
Subject: [PATCH] i3c: master: fix for SETDASA and DAA process
Date: Thu, 14 May 2020 18:30:09 +0200	[thread overview]
Message-ID: <1589473809-16708-1-git-send-email-pthombar@cadence.com> (raw)

This patch fix following issues.
1. Controller slots blocked for devices with static_addr
   but no init_dyn_addr may limit the number of I3C devices
   on the bus which gets dynamic address in DAA. So
   instead of attaching all the devices with static_addr,
   now we only attach the devices which successfully
   complete SETDASA. Remaining devices are handled in DAA.
2. Since we alreay handled devices with init_dyn_addr, removed
   it's handling from i3c_master_add_i3c_dev_locked().
   Now only case handled is devices already with dyn_addr
   participated in DAA, and again got new dyn_addr with an
   extra slot in the master controller.
3. Removed unnecessary i3c_master_reattach_i3c_dev() from
   i3c_master_add_i3c_dev_locked(), right away after finding
   if duplicate device exists in the I3C list.
   In case of different new and old dyn_addr
   i3c_master_reattach_i3c_dev() will fail which is wrong,
   and in case of same dyn_addr it doesn't add anything new.

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
 drivers/i3c/master.c | 111 ++++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 65 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52121fe..f1d929b58549 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1375,6 +1375,11 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 		i3c_bus_set_addr_slot_status(&master->bus,
 					     dev->info.dyn_addr,
 					     I3C_ADDR_SLOT_I3C_DEV);
+
+		if (old_dyn_addr)
+			i3c_bus_set_addr_slot_status(&master->bus,
+						     old_dyn_addr,
+						     I3C_ADDR_SLOT_FREE);
 	}
 
 	if (master->ops->reattach_i3c_dev) {
@@ -1426,33 +1431,52 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static void i3c_master_pre_assign_dyn_addr(struct i3c_master_controller *master,
+					   struct i3c_dev_boardinfo *boardinfo)
 {
-	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_device_info info = {
+		.static_addr = boardinfo->static_addr,
+	};
+	struct i3c_dev_desc *i3cdev;
 	int ret;
 
-	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
-	    !dev->boardinfo->static_addr)
+	/*
+	 * We anyway don't attach devices which are not addressable
+	 * (no static_addr and dyn_addr) and devices with static_addr
+	 * but no init_dyn_addr will participate in DAA.
+	 */
+	if (!boardinfo->static_addr || !boardinfo->init_dyn_addr)
+		return;
+
+	i3cdev = i3c_master_alloc_i3c_dev(master, &info);
+	if (IS_ERR(i3cdev))
 		return;
 
-	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
-					dev->boardinfo->init_dyn_addr);
+	i3cdev->boardinfo = boardinfo;
+
+	ret = i3c_master_attach_i3c_dev(master, i3cdev);
 	if (ret)
 		return;
 
-	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
-	ret = i3c_master_reattach_i3c_dev(dev, 0);
+	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
+					i3cdev->boardinfo->init_dyn_addr);
 	if (ret)
-		goto err_rstdaa;
+		goto err_setdasa;
 
-	ret = i3c_master_retrieve_dev_info(dev);
+	i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
+	ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	ret = i3c_master_retrieve_dev_info(i3cdev);
+	if (ret)
+		goto err_rstdaa;
 
 err_rstdaa:
-	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+	i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
+err_setdasa:
+	i3c_master_detach_i3c_dev(i3cdev);
+	i3c_master_free_i3c_dev(i3cdev);
 }
 
 static void
@@ -1619,8 +1643,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
  * 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
+ * 1. Attach I2C 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
@@ -1647,7 +1671,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 
@@ -1679,34 +1702,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 			goto err_detach_devs;
 		}
 	}
-	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
-		struct i3c_device_info info = {
-			.static_addr = i3cboardinfo->static_addr,
-		};
-
-		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;
-			}
-		}
-
-		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
-		if (IS_ERR(i3cdev)) {
-			ret = PTR_ERR(i3cdev);
-			goto err_detach_devs;
-		}
-
-		i3cdev->boardinfo = i3cboardinfo;
-
-		ret = i3c_master_attach_i3c_dev(master, i3cdev);
-		if (ret) {
-			i3c_master_free_i3c_dev(i3cdev);
-			goto err_detach_devs;
-		}
-	}
 
 	/*
 	 * Now execute the controller specific ->bus_init() routine, which
@@ -1746,8 +1741,8 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node)
+		i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
 
 	ret = i3c_master_do_daa(master);
 	if (ret)
@@ -1811,7 +1806,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 {
 	struct i3c_device_info info = { .dyn_addr = addr };
 	struct i3c_dev_desc *newdev, *olddev;
-	u8 old_dyn_addr = addr, expected_dyn_addr;
+	u8 old_dyn_addr = addr;
 	struct i3c_ibi_setup ibireq = { };
 	bool enable_ibi = false;
 	int ret;
@@ -1866,39 +1861,25 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 		i3c_master_free_i3c_dev(olddev);
 	}
 
-	ret = i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
-	if (ret)
-		goto err_detach_dev;
-
 	/*
 	 * Depending on our previous state, the expected dynamic address might
 	 * differ:
 	 * - if the device already had a dynamic address assigned, let's try to
-	 *   re-apply this one
-	 * - if the device did not have a dynamic address and the firmware
-	 *   requested a specific address, pick this one
+	 *   re-apply this one. Device with dyn_addr participated in DAA ?
 	 * - in any other case, keep the address automatically assigned by the
 	 *   master
 	 */
-	if (old_dyn_addr && old_dyn_addr != newdev->info.dyn_addr)
-		expected_dyn_addr = old_dyn_addr;
-	else if (newdev->boardinfo && newdev->boardinfo->init_dyn_addr)
-		expected_dyn_addr = newdev->boardinfo->init_dyn_addr;
-	else
-		expected_dyn_addr = newdev->info.dyn_addr;
-
-	if (newdev->info.dyn_addr != expected_dyn_addr) {
+	if (old_dyn_addr && old_dyn_addr != newdev->info.dyn_addr) {
 		/*
 		 * Try to apply the expected dynamic address. If it fails, keep
 		 * the address assigned by the master.
 		 */
 		ret = i3c_master_setnewda_locked(master,
 						 newdev->info.dyn_addr,
-						 expected_dyn_addr);
+						 old_dyn_addr);
 		if (!ret) {
-			old_dyn_addr = newdev->info.dyn_addr;
-			newdev->info.dyn_addr = expected_dyn_addr;
-			i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
+			newdev->info.dyn_addr = old_dyn_addr;
+			i3c_master_reattach_i3c_dev(newdev, addr);
 		} else {
 			dev_err(&master->dev,
 				"Failed to assign reserved/old address to device %d%llx",
-- 
2.17.1


WARNING: multiple messages have this Message-ID (diff)
From: Parshuram Thombare <pthombar@cadence.com>
To: <bbrezillon@kernel.org>, <vitor.soares@synopsys.com>
Cc: mparab@cadence.com, Parshuram Thombare <pthombar@cadence.com>,
	praneeth@ti.com, linux-kernel@vger.kernel.org, pgaj@cadence.com,
	linux-i3c@lists.infradead.org
Subject: [PATCH] i3c: master: fix for SETDASA and DAA process
Date: Thu, 14 May 2020 18:30:09 +0200	[thread overview]
Message-ID: <1589473809-16708-1-git-send-email-pthombar@cadence.com> (raw)

This patch fix following issues.
1. Controller slots blocked for devices with static_addr
   but no init_dyn_addr may limit the number of I3C devices
   on the bus which gets dynamic address in DAA. So
   instead of attaching all the devices with static_addr,
   now we only attach the devices which successfully
   complete SETDASA. Remaining devices are handled in DAA.
2. Since we alreay handled devices with init_dyn_addr, removed
   it's handling from i3c_master_add_i3c_dev_locked().
   Now only case handled is devices already with dyn_addr
   participated in DAA, and again got new dyn_addr with an
   extra slot in the master controller.
3. Removed unnecessary i3c_master_reattach_i3c_dev() from
   i3c_master_add_i3c_dev_locked(), right away after finding
   if duplicate device exists in the I3C list.
   In case of different new and old dyn_addr
   i3c_master_reattach_i3c_dev() will fail which is wrong,
   and in case of same dyn_addr it doesn't add anything new.

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
 drivers/i3c/master.c | 111 ++++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 65 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52121fe..f1d929b58549 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1375,6 +1375,11 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 		i3c_bus_set_addr_slot_status(&master->bus,
 					     dev->info.dyn_addr,
 					     I3C_ADDR_SLOT_I3C_DEV);
+
+		if (old_dyn_addr)
+			i3c_bus_set_addr_slot_status(&master->bus,
+						     old_dyn_addr,
+						     I3C_ADDR_SLOT_FREE);
 	}
 
 	if (master->ops->reattach_i3c_dev) {
@@ -1426,33 +1431,52 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static void i3c_master_pre_assign_dyn_addr(struct i3c_master_controller *master,
+					   struct i3c_dev_boardinfo *boardinfo)
 {
-	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_device_info info = {
+		.static_addr = boardinfo->static_addr,
+	};
+	struct i3c_dev_desc *i3cdev;
 	int ret;
 
-	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
-	    !dev->boardinfo->static_addr)
+	/*
+	 * We anyway don't attach devices which are not addressable
+	 * (no static_addr and dyn_addr) and devices with static_addr
+	 * but no init_dyn_addr will participate in DAA.
+	 */
+	if (!boardinfo->static_addr || !boardinfo->init_dyn_addr)
+		return;
+
+	i3cdev = i3c_master_alloc_i3c_dev(master, &info);
+	if (IS_ERR(i3cdev))
 		return;
 
-	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
-					dev->boardinfo->init_dyn_addr);
+	i3cdev->boardinfo = boardinfo;
+
+	ret = i3c_master_attach_i3c_dev(master, i3cdev);
 	if (ret)
 		return;
 
-	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
-	ret = i3c_master_reattach_i3c_dev(dev, 0);
+	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
+					i3cdev->boardinfo->init_dyn_addr);
 	if (ret)
-		goto err_rstdaa;
+		goto err_setdasa;
 
-	ret = i3c_master_retrieve_dev_info(dev);
+	i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
+	ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	ret = i3c_master_retrieve_dev_info(i3cdev);
+	if (ret)
+		goto err_rstdaa;
 
 err_rstdaa:
-	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+	i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
+err_setdasa:
+	i3c_master_detach_i3c_dev(i3cdev);
+	i3c_master_free_i3c_dev(i3cdev);
 }
 
 static void
@@ -1619,8 +1643,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
  * 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
+ * 1. Attach I2C 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
@@ -1647,7 +1671,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 
@@ -1679,34 +1702,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 			goto err_detach_devs;
 		}
 	}
-	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
-		struct i3c_device_info info = {
-			.static_addr = i3cboardinfo->static_addr,
-		};
-
-		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;
-			}
-		}
-
-		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
-		if (IS_ERR(i3cdev)) {
-			ret = PTR_ERR(i3cdev);
-			goto err_detach_devs;
-		}
-
-		i3cdev->boardinfo = i3cboardinfo;
-
-		ret = i3c_master_attach_i3c_dev(master, i3cdev);
-		if (ret) {
-			i3c_master_free_i3c_dev(i3cdev);
-			goto err_detach_devs;
-		}
-	}
 
 	/*
 	 * Now execute the controller specific ->bus_init() routine, which
@@ -1746,8 +1741,8 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node)
+		i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
 
 	ret = i3c_master_do_daa(master);
 	if (ret)
@@ -1811,7 +1806,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 {
 	struct i3c_device_info info = { .dyn_addr = addr };
 	struct i3c_dev_desc *newdev, *olddev;
-	u8 old_dyn_addr = addr, expected_dyn_addr;
+	u8 old_dyn_addr = addr;
 	struct i3c_ibi_setup ibireq = { };
 	bool enable_ibi = false;
 	int ret;
@@ -1866,39 +1861,25 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 		i3c_master_free_i3c_dev(olddev);
 	}
 
-	ret = i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
-	if (ret)
-		goto err_detach_dev;
-
 	/*
 	 * Depending on our previous state, the expected dynamic address might
 	 * differ:
 	 * - if the device already had a dynamic address assigned, let's try to
-	 *   re-apply this one
-	 * - if the device did not have a dynamic address and the firmware
-	 *   requested a specific address, pick this one
+	 *   re-apply this one. Device with dyn_addr participated in DAA ?
 	 * - in any other case, keep the address automatically assigned by the
 	 *   master
 	 */
-	if (old_dyn_addr && old_dyn_addr != newdev->info.dyn_addr)
-		expected_dyn_addr = old_dyn_addr;
-	else if (newdev->boardinfo && newdev->boardinfo->init_dyn_addr)
-		expected_dyn_addr = newdev->boardinfo->init_dyn_addr;
-	else
-		expected_dyn_addr = newdev->info.dyn_addr;
-
-	if (newdev->info.dyn_addr != expected_dyn_addr) {
+	if (old_dyn_addr && old_dyn_addr != newdev->info.dyn_addr) {
 		/*
 		 * Try to apply the expected dynamic address. If it fails, keep
 		 * the address assigned by the master.
 		 */
 		ret = i3c_master_setnewda_locked(master,
 						 newdev->info.dyn_addr,
-						 expected_dyn_addr);
+						 old_dyn_addr);
 		if (!ret) {
-			old_dyn_addr = newdev->info.dyn_addr;
-			newdev->info.dyn_addr = expected_dyn_addr;
-			i3c_master_reattach_i3c_dev(newdev, old_dyn_addr);
+			newdev->info.dyn_addr = old_dyn_addr;
+			i3c_master_reattach_i3c_dev(newdev, addr);
 		} else {
 			dev_err(&master->dev,
 				"Failed to assign reserved/old address to device %d%llx",
-- 
2.17.1


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

             reply	other threads:[~2020-05-14 16:30 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-14 16:30 Parshuram Thombare [this message]
2020-05-14 16:30 ` [PATCH] i3c: master: fix for SETDASA and DAA process Parshuram Thombare
2020-05-20  8:58 ` Boris Brezillon
2020-05-20  8:58   ` Boris Brezillon
2020-05-20  9:20   ` Parshuram Raju Thombare
2020-05-20  9:20     ` Parshuram Raju Thombare

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1589473809-16708-1-git-send-email-pthombar@cadence.com \
    --to=pthombar@cadence.com \
    --cc=bbrezillon@kernel.org \
    --cc=linux-i3c@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mparab@cadence.com \
    --cc=pgaj@cadence.com \
    --cc=praneeth@ti.com \
    --cc=vitor.soares@synopsys.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.