[PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

We've had generic code handling module sysconfig and OCP reset registers
for omap variants for many years now and all the drivers really needs to
do is just call runtime PM functions.

Looks like the omap-hdq driver got only partially updated over the years
to use runtime PM, and still has lots of custom PM code left.

We can replace all the custom code for sysconfig, OCP reset, and PM with
just a few lines of runtime PM autosuspend code.

In order to set the device mode properly when pm_runtime_get_sync() is
called during probe, we need to also move parsing of "ti,mode" to happen
earlier before we call pm_runtime_enable().

Since we now disable interrupts lazily in omap_hdq_runtime_suspend(), we
must remove the call to hdq_disable_interrupt() in omap_w1_read_byte().
And we must clear irqstatus calling wait_event_timeout() on it, so let's
add hdq_reset_irqstatus() for that.

Note that the earlier driver specific usage count limit of four seems
completely artificial and should not be an issue in normal use.

Cc: Adam Ford <aford173@gmail.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Andreas Kemnade <andreas@kemnade.info>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Vignesh R <vigneshr@ti.com>
Tested-by: Andreas Kemnade <andreas@kemnade.info> # gta04
Tested-by: Adam Ford <aford173@gmail.com> #logicpd-torpedo-37xx-devkit
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v2:

- Added hdq_reset_irqstatus() and fixed up the calls so ti,mode = "1w"
  works

- Kept earlier Tested-by from Adam and Andreas as the hdq_reset_irqstatus()
  fix only affected "1w" mode and not "hdq" mode

Changes since v1:

- Drop call to hdq_disable_interrupt() as we now disable interrupts
  lazily un runtime_suspend

- Parse "ti,mode" earlier before first pm_runtime_get_sync() so the
  mode gets set properly also during the probe


 drivers/w1/masters/omap_hdq.c | 348 +++++++++++-----------------------
 1 file changed, 113 insertions(+), 235 deletions(-)

diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
--- a/drivers/w1/masters/omap_hdq.c
+++ b/drivers/w1/masters/omap_hdq.c
@@ -38,12 +38,6 @@
 #define OMAP_HDQ_INT_STATUS_TXCOMPLETE		BIT(2)
 #define OMAP_HDQ_INT_STATUS_RXCOMPLETE		BIT(1)
 #define OMAP_HDQ_INT_STATUS_TIMEOUT		BIT(0)
-#define OMAP_HDQ_SYSCONFIG			0x14
-#define OMAP_HDQ_SYSCONFIG_SOFTRESET		BIT(1)
-#define OMAP_HDQ_SYSCONFIG_AUTOIDLE		BIT(0)
-#define OMAP_HDQ_SYSCONFIG_NOIDLE		0x0
-#define OMAP_HDQ_SYSSTATUS			0x18
-#define OMAP_HDQ_SYSSTATUS_RESETDONE		BIT(0)
 
 #define OMAP_HDQ_FLAG_CLEAR			0
 #define OMAP_HDQ_FLAG_SET			1
@@ -62,17 +56,9 @@ struct hdq_data {
 	void __iomem		*hdq_base;
 	/* lock status update */
 	struct  mutex		hdq_mutex;
-	int			hdq_usecount;
 	u8			hdq_irqstatus;
 	/* device lock */
 	spinlock_t		hdq_spinlock;
-	/*
-	 * Used to control the call to omap_hdq_get and omap_hdq_put.
-	 * HDQ Protocol: Write the CMD|REG_address first, followed by
-	 * the data wrire or read.
-	 */
-	int			init_trans;
-	int                     rrw;
 	/* mode: 0-HDQ 1-W1 */
 	int                     mode;
 
@@ -99,15 +85,6 @@ static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
 	return new_val;
 }
 
-static void hdq_disable_interrupt(struct hdq_data *hdq_data, u32 offset,
-				  u32 mask)
-{
-	u32 ie;
-
-	ie = readl(hdq_data->hdq_base + offset);
-	writel(ie & mask, hdq_data->hdq_base + offset);
-}
-
 /*
  * Wait for one or more bits in flag change.
  * HDQ_FLAG_SET: wait until any bit in the flag is set.
@@ -142,22 +119,24 @@ static int hdq_wait_for_flag(struct hdq_data *hdq_data, u32 offset,
 	return ret;
 }
 
+/* Clear saved irqstatus after using an interrupt */
+static void hdq_reset_irqstatus(struct hdq_data *hdq_data)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
+	hdq_data->hdq_irqstatus = 0;
+	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
+}
+
 /* write out a byte and fill *status with HDQ_INT_STATUS */
 static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
 {
 	int ret;
 	u8 tmp_status;
-	unsigned long irqflags;
 
 	*status = 0;
 
-	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
-	/* clear interrupt flags via a dummy read */
-	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
-	/* ISR loads it with new INT_STATUS */
-	hdq_data->hdq_irqstatus = 0;
-	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
-
 	hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val);
 
 	/* set the GO bit */
@@ -191,6 +170,7 @@ static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
 	}
 
 out:
+	hdq_reset_irqstatus(hdq_data);
 	return ret;
 }
 
@@ -237,47 +217,11 @@ static void omap_w1_search_bus(void *_hdq, struct w1_master *master_dev,
 	slave_found(master_dev, id);
 }
 
-static int _omap_hdq_reset(struct hdq_data *hdq_data)
-{
-	int ret;
-	u8 tmp_status;
-
-	hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
-		    OMAP_HDQ_SYSCONFIG_SOFTRESET);
-	/*
-	 * Select HDQ/1W mode & enable clocks.
-	 * It is observed that INT flags can't be cleared via a read and GO/INIT
-	 * won't return to zero if interrupt is disabled. So we always enable
-	 * interrupt.
-	 */
-	hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
-		OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
-		OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
-
-	/* wait for reset to complete */
-	ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_SYSSTATUS,
-		OMAP_HDQ_SYSSTATUS_RESETDONE, OMAP_HDQ_FLAG_SET, &tmp_status);
-	if (ret)
-		dev_dbg(hdq_data->dev, "timeout waiting HDQ reset, %x",
-				tmp_status);
-	else {
-		hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
-			OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
-			OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
-			hdq_data->mode);
-		hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
-			OMAP_HDQ_SYSCONFIG_AUTOIDLE);
-	}
-
-	return ret;
-}
-
 /* Issue break pulse to the device */
 static int omap_hdq_break(struct hdq_data *hdq_data)
 {
 	int ret = 0;
 	u8 tmp_status;
-	unsigned long irqflags;
 
 	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
 	if (ret < 0) {
@@ -286,13 +230,6 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
 		goto rtn;
 	}
 
-	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
-	/* clear interrupt flags via a dummy read */
-	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
-	/* ISR loads it with new INT_STATUS */
-	hdq_data->hdq_irqstatus = 0;
-	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
-
 	/* set the INIT and GO bit */
 	hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
 		OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
@@ -341,6 +278,7 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
 			" return to zero, %x", tmp_status);
 
 out:
+	hdq_reset_irqstatus(hdq_data);
 	mutex_unlock(&hdq_data->hdq_mutex);
 rtn:
 	return ret;
@@ -357,7 +295,7 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 		goto rtn;
 	}
 
-	if (!hdq_data->hdq_usecount) {
+	if (pm_runtime_suspended(hdq_data->dev)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -388,86 +326,13 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 	/* the data is ready. Read it in! */
 	*val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
 out:
+	hdq_reset_irqstatus(hdq_data);
 	mutex_unlock(&hdq_data->hdq_mutex);
 rtn:
 	return ret;
 
 }
 
-/* Enable clocks and set the controller to HDQ/1W mode */
-static int omap_hdq_get(struct hdq_data *hdq_data)
-{
-	int ret = 0;
-
-	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
-	if (ret < 0) {
-		ret = -EINTR;
-		goto rtn;
-	}
-
-	if (OMAP_HDQ_MAX_USER == hdq_data->hdq_usecount) {
-		dev_dbg(hdq_data->dev, "attempt to exceed the max use count");
-		ret = -EINVAL;
-		goto out;
-	} else {
-		hdq_data->hdq_usecount++;
-		try_module_get(THIS_MODULE);
-		if (1 == hdq_data->hdq_usecount) {
-
-			pm_runtime_get_sync(hdq_data->dev);
-
-			/* make sure HDQ/1W is out of reset */
-			if (!(hdq_reg_in(hdq_data, OMAP_HDQ_SYSSTATUS) &
-				OMAP_HDQ_SYSSTATUS_RESETDONE)) {
-				ret = _omap_hdq_reset(hdq_data);
-				if (ret)
-					/* back up the count */
-					hdq_data->hdq_usecount--;
-			} else {
-				/* select HDQ/1W mode & enable clocks */
-				hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
-					OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
-					OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
-					hdq_data->mode);
-				hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
-					OMAP_HDQ_SYSCONFIG_NOIDLE);
-				hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
-			}
-		}
-	}
-
-out:
-	mutex_unlock(&hdq_data->hdq_mutex);
-rtn:
-	return ret;
-}
-
-/* Disable clocks to the module */
-static int omap_hdq_put(struct hdq_data *hdq_data)
-{
-	int ret = 0;
-
-	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
-	if (ret < 0)
-		return -EINTR;
-
-	hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
-		    OMAP_HDQ_SYSCONFIG_AUTOIDLE);
-	if (0 == hdq_data->hdq_usecount) {
-		dev_dbg(hdq_data->dev, "attempt to decrement use count"
-			" when it is zero");
-		ret = -EINVAL;
-	} else {
-		hdq_data->hdq_usecount--;
-		module_put(THIS_MODULE);
-		if (0 == hdq_data->hdq_usecount)
-			pm_runtime_put_sync(hdq_data->dev);
-	}
-	mutex_unlock(&hdq_data->hdq_mutex);
-
-	return ret;
-}
-
 /*
  * W1 triplet callback function - used for searching ROM addresses.
  * Registered only when controller is in 1-wire mode.
@@ -482,7 +347,12 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
 		  OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK;
 	u8 mask = ctrl | OMAP_HDQ_CTRL_STATUS_DIR;
 
-	omap_hdq_get(_hdq);
+	err = pm_runtime_get_sync(hdq_data->dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(hdq_data->dev);
+
+		return err;
+	}
 
 	err = mutex_lock_interruptible(&hdq_data->hdq_mutex);
 	if (err < 0) {
@@ -490,7 +360,6 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
 		goto rtn;
 	}
 
-	hdq_data->hdq_irqstatus = 0;
 	/* read id_bit */
 	hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS,
 		      ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask);
@@ -504,7 +373,9 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
 	}
 	id_bit = (hdq_reg_in(_hdq, OMAP_HDQ_RX_DATA) & 0x01);
 
-	hdq_data->hdq_irqstatus = 0;
+	/* Must clear irqstatus for another RXCOMPLETE interrupt */
+	hdq_reset_irqstatus(hdq_data);
+
 	/* read comp_bit */
 	hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS,
 		      ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask);
@@ -547,18 +418,33 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
 		      OMAP_HDQ_CTRL_STATUS_SINGLE);
 
 out:
+	hdq_reset_irqstatus(hdq_data);
 	mutex_unlock(&hdq_data->hdq_mutex);
 rtn:
-	omap_hdq_put(_hdq);
+	pm_runtime_mark_last_busy(hdq_data->dev);
+	pm_runtime_put_autosuspend(hdq_data->dev);
+
 	return ret;
 }
 
 /* reset callback */
 static u8 omap_w1_reset_bus(void *_hdq)
 {
-	omap_hdq_get(_hdq);
-	omap_hdq_break(_hdq);
-	omap_hdq_put(_hdq);
+	struct hdq_data *hdq_data = _hdq;
+	int err;
+
+	err = pm_runtime_get_sync(hdq_data->dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(hdq_data->dev);
+
+		return err;
+	}
+
+	omap_hdq_break(hdq_data);
+
+	pm_runtime_mark_last_busy(hdq_data->dev);
+	pm_runtime_put_autosuspend(hdq_data->dev);
+
 	return 0;
 }
 
@@ -569,37 +455,19 @@ static u8 omap_w1_read_byte(void *_hdq)
 	u8 val = 0;
 	int ret;
 
-	/* First write to initialize the transfer */
-	if (hdq_data->init_trans == 0)
-		omap_hdq_get(hdq_data);
+	ret = pm_runtime_get_sync(hdq_data->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(hdq_data->dev);
 
-	ret = hdq_read_byte(hdq_data, &val);
-	if (ret) {
-		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
-		if (ret < 0) {
-			dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
-			return -EINTR;
-		}
-		hdq_data->init_trans = 0;
-		mutex_unlock(&hdq_data->hdq_mutex);
-		omap_hdq_put(hdq_data);
 		return -1;
 	}
 
-	hdq_disable_interrupt(hdq_data, OMAP_HDQ_CTRL_STATUS,
-			      ~OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+	ret = hdq_read_byte(hdq_data, &val);
+	if (ret)
+		ret = -1;
 
-	/* Write followed by a read, release the module */
-	if (hdq_data->init_trans) {
-		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
-		if (ret < 0) {
-			dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
-			return -EINTR;
-		}
-		hdq_data->init_trans = 0;
-		mutex_unlock(&hdq_data->hdq_mutex);
-		omap_hdq_put(hdq_data);
-	}
+	pm_runtime_mark_last_busy(hdq_data->dev);
+	pm_runtime_put_autosuspend(hdq_data->dev);
 
 	return val;
 }
@@ -611,9 +479,12 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
 	int ret;
 	u8 status;
 
-	/* First write to initialize the transfer */
-	if (hdq_data->init_trans == 0)
-		omap_hdq_get(hdq_data);
+	ret = pm_runtime_get_sync(hdq_data->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(hdq_data->dev);
+
+		return;
+	}
 
 	/*
 	 * We need to reset the slave before
@@ -623,31 +494,15 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
 	if (byte == W1_SKIP_ROM)
 		omap_hdq_break(hdq_data);
 
-	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
-	if (ret < 0) {
-		dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
-		return;
-	}
-	hdq_data->init_trans++;
-	mutex_unlock(&hdq_data->hdq_mutex);
-
 	ret = hdq_write_byte(hdq_data, byte, &status);
 	if (ret < 0) {
 		dev_dbg(hdq_data->dev, "TX failure:Ctrl status %x\n", status);
-		return;
+		goto out_err;
 	}
 
-	/* Second write, data transferred. Release the module */
-	if (hdq_data->init_trans > 1) {
-		omap_hdq_put(hdq_data);
-		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
-		if (ret < 0) {
-			dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
-			return;
-		}
-		hdq_data->init_trans = 0;
-		mutex_unlock(&hdq_data->hdq_mutex);
-	}
+out_err:
+	pm_runtime_mark_last_busy(hdq_data->dev);
+	pm_runtime_put_autosuspend(hdq_data->dev);
 }
 
 static struct w1_bus_master omap_w1_master = {
@@ -656,6 +511,35 @@ static struct w1_bus_master omap_w1_master = {
 	.reset_bus	= omap_w1_reset_bus,
 };
 
+static int __maybe_unused omap_hdq_runtime_suspend(struct device *dev)
+{
+	struct hdq_data *hdq_data = dev_get_drvdata(dev);
+
+	hdq_reg_out(hdq_data, 0, hdq_data->mode);
+	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
+
+	return 0;
+}
+
+static int __maybe_unused omap_hdq_runtime_resume(struct device *dev)
+{
+	struct hdq_data *hdq_data = dev_get_drvdata(dev);
+
+	/* select HDQ/1W mode & enable clocks */
+	hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
+		    OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
+		    OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
+		    hdq_data->mode);
+	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
+
+	return 0;
+}
+
+static const struct dev_pm_ops omap_hdq_pm_ops = {
+	SET_RUNTIME_PM_OPS(omap_hdq_runtime_suspend,
+			   omap_hdq_runtime_resume, NULL)
+};
+
 static int omap_hdq_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -677,23 +561,27 @@ static int omap_hdq_probe(struct platform_device *pdev)
 	if (IS_ERR(hdq_data->hdq_base))
 		return PTR_ERR(hdq_data->hdq_base);
 
-	hdq_data->hdq_usecount = 0;
-	hdq_data->rrw = 0;
 	mutex_init(&hdq_data->hdq_mutex);
 
+	ret = of_property_read_string(pdev->dev.of_node, "ti,mode", &mode);
+	if (ret < 0 || !strcmp(mode, "hdq")) {
+		hdq_data->mode = 0;
+		omap_w1_master.search = omap_w1_search_bus;
+	} else {
+		hdq_data->mode = 1;
+		omap_w1_master.triplet = omap_w1_triplet;
+	}
+
 	pm_runtime_enable(&pdev->dev);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
 	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
 		dev_dbg(&pdev->dev, "pm_runtime_get_sync failed\n");
 		goto err_w1;
 	}
 
-	ret = _omap_hdq_reset(hdq_data);
-	if (ret) {
-		dev_dbg(&pdev->dev, "reset failed\n");
-		goto err_irq;
-	}
-
 	rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
 	dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
 		(rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
@@ -715,16 +603,8 @@ static int omap_hdq_probe(struct platform_device *pdev)
 
 	omap_hdq_break(hdq_data);
 
-	pm_runtime_put_sync(&pdev->dev);
-
-	ret = of_property_read_string(pdev->dev.of_node, "ti,mode", &mode);
-	if (ret < 0 || !strcmp(mode, "hdq")) {
-		hdq_data->mode = 0;
-		omap_w1_master.search = omap_w1_search_bus;
-	} else {
-		hdq_data->mode = 1;
-		omap_w1_master.triplet = omap_w1_triplet;
-	}
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
 
 	omap_w1_master.data = hdq_data;
 
@@ -739,6 +619,7 @@ static int omap_hdq_probe(struct platform_device *pdev)
 err_irq:
 	pm_runtime_put_sync(&pdev->dev);
 err_w1:
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return ret;
@@ -746,23 +627,19 @@ static int omap_hdq_probe(struct platform_device *pdev)
 
 static int omap_hdq_remove(struct platform_device *pdev)
 {
-	struct hdq_data *hdq_data = platform_get_drvdata(pdev);
+	int active;
 
-	mutex_lock(&hdq_data->hdq_mutex);
-
-	if (hdq_data->hdq_usecount) {
-		dev_dbg(&pdev->dev, "removed when use count is not zero\n");
-		mutex_unlock(&hdq_data->hdq_mutex);
-		return -EBUSY;
-	}
+	active = pm_runtime_get_sync(&pdev->dev);
+	if (active < 0)
+		pm_runtime_put_noidle(&pdev->dev);
 
-	mutex_unlock(&hdq_data->hdq_mutex);
+	w1_remove_master_device(&omap_w1_master);
 
-	/* remove module dependency */
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	if (active >= 0)
+		pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
-	w1_remove_master_device(&omap_w1_master);
-
 	return 0;
 }
 
@@ -779,6 +656,7 @@ static struct platform_driver omap_hdq_driver = {
 	.driver = {
 		.name =	"omap_hdq",
 		.of_match_table = omap_hdq_dt_ids,
+		.pm = &omap_hdq_pm_ops,
 	},
 };
 module_platform_driver(omap_hdq_driver);
-- 
2.24.1

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi Tony,
it looks as if something with this patch is broken on GTA04. For v5.6 and v5.7-rc1.

HDQ battery access times out after ca. 15 seconds and I get temperature of -273.1°C...

Reverting this patch and everything is ok again.

What is "ti,mode" about? Do we have that (indirectly) in gta04.dtsi?
Or does this patch need some CONFIGs we do not happen to have?

BR and thanks,
Nikolaus

root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=0
POWER_SUPPLY_CURRENT_NOW=0
POWER_SUPPLY_CAPACITY=0
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=-2731
POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
POWER_SUPPLY_TIME_TO_EMPTY_AVG=0
POWER_SUPPLY_TIME_TO_FULL_NOW=0
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=0
POWER_SUPPLY_CHARGE_NOW=0
POWER_SUPPLY_CHARGE_FULL_DESIGN=0
POWER_SUPPLY_CYCLE_COUNT=0
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_POWER_AVG=0
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

real    0m15.761s
user    0m0.001s
sys     0m0.025s
root@letux:~# 
root@letux:~# dmesg|fgrep -i hdq
[   20.252136] omap_hdq 480b2000.1w: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
root@letux:~#

> Am 17.12.2019 um 01:40 schrieb Tony Lindgren <tony@atomide.com>:
> 
> We've had generic code handling module sysconfig and OCP reset registers
> for omap variants for many years now and all the drivers really needs to
> do is just call runtime PM functions.
> 
> Looks like the omap-hdq driver got only partially updated over the years
> to use runtime PM, and still has lots of custom PM code left.
> 
> We can replace all the custom code for sysconfig, OCP reset, and PM with
> just a few lines of runtime PM autosuspend code.
> 
> In order to set the device mode properly when pm_runtime_get_sync() is
> called during probe, we need to also move parsing of "ti,mode" to happen
> earlier before we call pm_runtime_enable().
> 
> Since we now disable interrupts lazily in omap_hdq_runtime_suspend(), we
> must remove the call to hdq_disable_interrupt() in omap_w1_read_byte().
> And we must clear irqstatus calling wait_event_timeout() on it, so let's
> add hdq_reset_irqstatus() for that.
> 
> Note that the earlier driver specific usage count limit of four seems
> completely artificial and should not be an issue in normal use.
> 
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Andreas Kemnade <andreas@kemnade.info>
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Vignesh R <vigneshr@ti.com>
> Tested-by: Andreas Kemnade <andreas@kemnade.info> # gta04
> Tested-by: Adam Ford <aford173@gmail.com> #logicpd-torpedo-37xx-devkit
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Changes since v2:
> 
> - Added hdq_reset_irqstatus() and fixed up the calls so ti,mode = "1w"
>  works
> 
> - Kept earlier Tested-by from Adam and Andreas as the hdq_reset_irqstatus()
>  fix only affected "1w" mode and not "hdq" mode
> 
> Changes since v1:
> 
> - Drop call to hdq_disable_interrupt() as we now disable interrupts
>  lazily un runtime_suspend
> 
> - Parse "ti,mode" earlier before first pm_runtime_get_sync() so the
>  mode gets set properly also during the probe
> 
> 
> drivers/w1/masters/omap_hdq.c | 348 +++++++++++-----------------------
> 1 file changed, 113 insertions(+), 235 deletions(-)
> 
> diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
> --- a/drivers/w1/masters/omap_hdq.c
> +++ b/drivers/w1/masters/omap_hdq.c
> @@ -38,12 +38,6 @@
> #define OMAP_HDQ_INT_STATUS_TXCOMPLETE		BIT(2)
> #define OMAP_HDQ_INT_STATUS_RXCOMPLETE		BIT(1)
> #define OMAP_HDQ_INT_STATUS_TIMEOUT		BIT(0)
> -#define OMAP_HDQ_SYSCONFIG			0x14
> -#define OMAP_HDQ_SYSCONFIG_SOFTRESET		BIT(1)
> -#define OMAP_HDQ_SYSCONFIG_AUTOIDLE		BIT(0)
> -#define OMAP_HDQ_SYSCONFIG_NOIDLE		0x0
> -#define OMAP_HDQ_SYSSTATUS			0x18
> -#define OMAP_HDQ_SYSSTATUS_RESETDONE		BIT(0)
> 
> #define OMAP_HDQ_FLAG_CLEAR			0
> #define OMAP_HDQ_FLAG_SET			1
> @@ -62,17 +56,9 @@ struct hdq_data {
> 	void __iomem		*hdq_base;
> 	/* lock status update */
> 	struct  mutex		hdq_mutex;
> -	int			hdq_usecount;
> 	u8			hdq_irqstatus;
> 	/* device lock */
> 	spinlock_t		hdq_spinlock;
> -	/*
> -	 * Used to control the call to omap_hdq_get and omap_hdq_put.
> -	 * HDQ Protocol: Write the CMD|REG_address first, followed by
> -	 * the data wrire or read.
> -	 */
> -	int			init_trans;
> -	int                     rrw;
> 	/* mode: 0-HDQ 1-W1 */
> 	int                     mode;
> 
> @@ -99,15 +85,6 @@ static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
> 	return new_val;
> }
> 
> -static void hdq_disable_interrupt(struct hdq_data *hdq_data, u32 offset,
> -				  u32 mask)
> -{
> -	u32 ie;
> -
> -	ie = readl(hdq_data->hdq_base + offset);
> -	writel(ie & mask, hdq_data->hdq_base + offset);
> -}
> -
> /*
>  * Wait for one or more bits in flag change.
>  * HDQ_FLAG_SET: wait until any bit in the flag is set.
> @@ -142,22 +119,24 @@ static int hdq_wait_for_flag(struct hdq_data *hdq_data, u32 offset,
> 	return ret;
> }
> 
> +/* Clear saved irqstatus after using an interrupt */
> +static void hdq_reset_irqstatus(struct hdq_data *hdq_data)
> +{
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +	hdq_data->hdq_irqstatus = 0;
> +	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +}
> +
> /* write out a byte and fill *status with HDQ_INT_STATUS */
> static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
> {
> 	int ret;
> 	u8 tmp_status;
> -	unsigned long irqflags;
> 
> 	*status = 0;
> 
> -	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> -	/* clear interrupt flags via a dummy read */
> -	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> -	/* ISR loads it with new INT_STATUS */
> -	hdq_data->hdq_irqstatus = 0;
> -	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> -
> 	hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val);
> 
> 	/* set the GO bit */
> @@ -191,6 +170,7 @@ static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
> 	}
> 
> out:
> +	hdq_reset_irqstatus(hdq_data);
> 	return ret;
> }
> 
> @@ -237,47 +217,11 @@ static void omap_w1_search_bus(void *_hdq, struct w1_master *master_dev,
> 	slave_found(master_dev, id);
> }
> 
> -static int _omap_hdq_reset(struct hdq_data *hdq_data)
> -{
> -	int ret;
> -	u8 tmp_status;
> -
> -	hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> -		    OMAP_HDQ_SYSCONFIG_SOFTRESET);
> -	/*
> -	 * Select HDQ/1W mode & enable clocks.
> -	 * It is observed that INT flags can't be cleared via a read and GO/INIT
> -	 * won't return to zero if interrupt is disabled. So we always enable
> -	 * interrupt.
> -	 */
> -	hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> -		OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> -		OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
> -
> -	/* wait for reset to complete */
> -	ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_SYSSTATUS,
> -		OMAP_HDQ_SYSSTATUS_RESETDONE, OMAP_HDQ_FLAG_SET, &tmp_status);
> -	if (ret)
> -		dev_dbg(hdq_data->dev, "timeout waiting HDQ reset, %x",
> -				tmp_status);
> -	else {
> -		hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> -			OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> -			OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
> -			hdq_data->mode);
> -		hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> -			OMAP_HDQ_SYSCONFIG_AUTOIDLE);
> -	}
> -
> -	return ret;
> -}
> -
> /* Issue break pulse to the device */
> static int omap_hdq_break(struct hdq_data *hdq_data)
> {
> 	int ret = 0;
> 	u8 tmp_status;
> -	unsigned long irqflags;
> 
> 	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> 	if (ret < 0) {
> @@ -286,13 +230,6 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
> 		goto rtn;
> 	}
> 
> -	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> -	/* clear interrupt flags via a dummy read */
> -	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> -	/* ISR loads it with new INT_STATUS */
> -	hdq_data->hdq_irqstatus = 0;
> -	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> -
> 	/* set the INIT and GO bit */
> 	hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
> 		OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
> @@ -341,6 +278,7 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
> 			" return to zero, %x", tmp_status);
> 
> out:
> +	hdq_reset_irqstatus(hdq_data);
> 	mutex_unlock(&hdq_data->hdq_mutex);
> rtn:
> 	return ret;
> @@ -357,7 +295,7 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
> 		goto rtn;
> 	}
> 
> -	if (!hdq_data->hdq_usecount) {
> +	if (pm_runtime_suspended(hdq_data->dev)) {
> 		ret = -EINVAL;
> 		goto out;
> 	}
> @@ -388,86 +326,13 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
> 	/* the data is ready. Read it in! */
> 	*val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
> out:
> +	hdq_reset_irqstatus(hdq_data);
> 	mutex_unlock(&hdq_data->hdq_mutex);
> rtn:
> 	return ret;
> 
> }
> 
> -/* Enable clocks and set the controller to HDQ/1W mode */
> -static int omap_hdq_get(struct hdq_data *hdq_data)
> -{
> -	int ret = 0;
> -
> -	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> -	if (ret < 0) {
> -		ret = -EINTR;
> -		goto rtn;
> -	}
> -
> -	if (OMAP_HDQ_MAX_USER == hdq_data->hdq_usecount) {
> -		dev_dbg(hdq_data->dev, "attempt to exceed the max use count");
> -		ret = -EINVAL;
> -		goto out;
> -	} else {
> -		hdq_data->hdq_usecount++;
> -		try_module_get(THIS_MODULE);
> -		if (1 == hdq_data->hdq_usecount) {
> -
> -			pm_runtime_get_sync(hdq_data->dev);
> -
> -			/* make sure HDQ/1W is out of reset */
> -			if (!(hdq_reg_in(hdq_data, OMAP_HDQ_SYSSTATUS) &
> -				OMAP_HDQ_SYSSTATUS_RESETDONE)) {
> -				ret = _omap_hdq_reset(hdq_data);
> -				if (ret)
> -					/* back up the count */
> -					hdq_data->hdq_usecount--;
> -			} else {
> -				/* select HDQ/1W mode & enable clocks */
> -				hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> -					OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> -					OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
> -					hdq_data->mode);
> -				hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> -					OMAP_HDQ_SYSCONFIG_NOIDLE);
> -				hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> -			}
> -		}
> -	}
> -
> -out:
> -	mutex_unlock(&hdq_data->hdq_mutex);
> -rtn:
> -	return ret;
> -}
> -
> -/* Disable clocks to the module */
> -static int omap_hdq_put(struct hdq_data *hdq_data)
> -{
> -	int ret = 0;
> -
> -	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> -	if (ret < 0)
> -		return -EINTR;
> -
> -	hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> -		    OMAP_HDQ_SYSCONFIG_AUTOIDLE);
> -	if (0 == hdq_data->hdq_usecount) {
> -		dev_dbg(hdq_data->dev, "attempt to decrement use count"
> -			" when it is zero");
> -		ret = -EINVAL;
> -	} else {
> -		hdq_data->hdq_usecount--;
> -		module_put(THIS_MODULE);
> -		if (0 == hdq_data->hdq_usecount)
> -			pm_runtime_put_sync(hdq_data->dev);
> -	}
> -	mutex_unlock(&hdq_data->hdq_mutex);
> -
> -	return ret;
> -}
> -
> /*
>  * W1 triplet callback function - used for searching ROM addresses.
>  * Registered only when controller is in 1-wire mode.
> @@ -482,7 +347,12 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
> 		  OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK;
> 	u8 mask = ctrl | OMAP_HDQ_CTRL_STATUS_DIR;
> 
> -	omap_hdq_get(_hdq);
> +	err = pm_runtime_get_sync(hdq_data->dev);
> +	if (err < 0) {
> +		pm_runtime_put_noidle(hdq_data->dev);
> +
> +		return err;
> +	}
> 
> 	err = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> 	if (err < 0) {
> @@ -490,7 +360,6 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
> 		goto rtn;
> 	}
> 
> -	hdq_data->hdq_irqstatus = 0;
> 	/* read id_bit */
> 	hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS,
> 		      ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask);
> @@ -504,7 +373,9 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
> 	}
> 	id_bit = (hdq_reg_in(_hdq, OMAP_HDQ_RX_DATA) & 0x01);
> 
> -	hdq_data->hdq_irqstatus = 0;
> +	/* Must clear irqstatus for another RXCOMPLETE interrupt */
> +	hdq_reset_irqstatus(hdq_data);
> +
> 	/* read comp_bit */
> 	hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS,
> 		      ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask);
> @@ -547,18 +418,33 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
> 		      OMAP_HDQ_CTRL_STATUS_SINGLE);
> 
> out:
> +	hdq_reset_irqstatus(hdq_data);
> 	mutex_unlock(&hdq_data->hdq_mutex);
> rtn:
> -	omap_hdq_put(_hdq);
> +	pm_runtime_mark_last_busy(hdq_data->dev);
> +	pm_runtime_put_autosuspend(hdq_data->dev);
> +
> 	return ret;
> }
> 
> /* reset callback */
> static u8 omap_w1_reset_bus(void *_hdq)
> {
> -	omap_hdq_get(_hdq);
> -	omap_hdq_break(_hdq);
> -	omap_hdq_put(_hdq);
> +	struct hdq_data *hdq_data = _hdq;
> +	int err;
> +
> +	err = pm_runtime_get_sync(hdq_data->dev);
> +	if (err < 0) {
> +		pm_runtime_put_noidle(hdq_data->dev);
> +
> +		return err;
> +	}
> +
> +	omap_hdq_break(hdq_data);
> +
> +	pm_runtime_mark_last_busy(hdq_data->dev);
> +	pm_runtime_put_autosuspend(hdq_data->dev);
> +
> 	return 0;
> }
> 
> @@ -569,37 +455,19 @@ static u8 omap_w1_read_byte(void *_hdq)
> 	u8 val = 0;
> 	int ret;
> 
> -	/* First write to initialize the transfer */
> -	if (hdq_data->init_trans == 0)
> -		omap_hdq_get(hdq_data);
> +	ret = pm_runtime_get_sync(hdq_data->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(hdq_data->dev);
> 
> -	ret = hdq_read_byte(hdq_data, &val);
> -	if (ret) {
> -		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> -		if (ret < 0) {
> -			dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> -			return -EINTR;
> -		}
> -		hdq_data->init_trans = 0;
> -		mutex_unlock(&hdq_data->hdq_mutex);
> -		omap_hdq_put(hdq_data);
> 		return -1;
> 	}
> 
> -	hdq_disable_interrupt(hdq_data, OMAP_HDQ_CTRL_STATUS,
> -			      ~OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
> +	ret = hdq_read_byte(hdq_data, &val);
> +	if (ret)
> +		ret = -1;
> 
> -	/* Write followed by a read, release the module */
> -	if (hdq_data->init_trans) {
> -		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> -		if (ret < 0) {
> -			dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> -			return -EINTR;
> -		}
> -		hdq_data->init_trans = 0;
> -		mutex_unlock(&hdq_data->hdq_mutex);
> -		omap_hdq_put(hdq_data);
> -	}
> +	pm_runtime_mark_last_busy(hdq_data->dev);
> +	pm_runtime_put_autosuspend(hdq_data->dev);
> 
> 	return val;
> }
> @@ -611,9 +479,12 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
> 	int ret;
> 	u8 status;
> 
> -	/* First write to initialize the transfer */
> -	if (hdq_data->init_trans == 0)
> -		omap_hdq_get(hdq_data);
> +	ret = pm_runtime_get_sync(hdq_data->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(hdq_data->dev);
> +
> +		return;
> +	}
> 
> 	/*
> 	 * We need to reset the slave before
> @@ -623,31 +494,15 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
> 	if (byte == W1_SKIP_ROM)
> 		omap_hdq_break(hdq_data);
> 
> -	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> -	if (ret < 0) {
> -		dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> -		return;
> -	}
> -	hdq_data->init_trans++;
> -	mutex_unlock(&hdq_data->hdq_mutex);
> -
> 	ret = hdq_write_byte(hdq_data, byte, &status);
> 	if (ret < 0) {
> 		dev_dbg(hdq_data->dev, "TX failure:Ctrl status %x\n", status);
> -		return;
> +		goto out_err;
> 	}
> 
> -	/* Second write, data transferred. Release the module */
> -	if (hdq_data->init_trans > 1) {
> -		omap_hdq_put(hdq_data);
> -		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> -		if (ret < 0) {
> -			dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> -			return;
> -		}
> -		hdq_data->init_trans = 0;
> -		mutex_unlock(&hdq_data->hdq_mutex);
> -	}
> +out_err:
> +	pm_runtime_mark_last_busy(hdq_data->dev);
> +	pm_runtime_put_autosuspend(hdq_data->dev);
> }
> 
> static struct w1_bus_master omap_w1_master = {
> @@ -656,6 +511,35 @@ static struct w1_bus_master omap_w1_master = {
> 	.reset_bus	= omap_w1_reset_bus,
> };
> 
> +static int __maybe_unused omap_hdq_runtime_suspend(struct device *dev)
> +{
> +	struct hdq_data *hdq_data = dev_get_drvdata(dev);
> +
> +	hdq_reg_out(hdq_data, 0, hdq_data->mode);
> +	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused omap_hdq_runtime_resume(struct device *dev)
> +{
> +	struct hdq_data *hdq_data = dev_get_drvdata(dev);
> +
> +	/* select HDQ/1W mode & enable clocks */
> +	hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +		    OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> +		    OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
> +		    hdq_data->mode);
> +	hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops omap_hdq_pm_ops = {
> +	SET_RUNTIME_PM_OPS(omap_hdq_runtime_suspend,
> +			   omap_hdq_runtime_resume, NULL)
> +};
> +
> static int omap_hdq_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
> @@ -677,23 +561,27 @@ static int omap_hdq_probe(struct platform_device *pdev)
> 	if (IS_ERR(hdq_data->hdq_base))
> 		return PTR_ERR(hdq_data->hdq_base);
> 
> -	hdq_data->hdq_usecount = 0;
> -	hdq_data->rrw = 0;
> 	mutex_init(&hdq_data->hdq_mutex);
> 
> +	ret = of_property_read_string(pdev->dev.of_node, "ti,mode", &mode);
> +	if (ret < 0 || !strcmp(mode, "hdq")) {
> +		hdq_data->mode = 0;
> +		omap_w1_master.search = omap_w1_search_bus;
> +	} else {
> +		hdq_data->mode = 1;
> +		omap_w1_master.triplet = omap_w1_triplet;
> +	}
> +
> 	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
> 	ret = pm_runtime_get_sync(&pdev->dev);
> 	if (ret < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
> 		dev_dbg(&pdev->dev, "pm_runtime_get_sync failed\n");
> 		goto err_w1;
> 	}
> 
> -	ret = _omap_hdq_reset(hdq_data);
> -	if (ret) {
> -		dev_dbg(&pdev->dev, "reset failed\n");
> -		goto err_irq;
> -	}
> -
> 	rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
> 	dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
> 		(rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
> @@ -715,16 +603,8 @@ static int omap_hdq_probe(struct platform_device *pdev)
> 
> 	omap_hdq_break(hdq_data);
> 
> -	pm_runtime_put_sync(&pdev->dev);
> -
> -	ret = of_property_read_string(pdev->dev.of_node, "ti,mode", &mode);
> -	if (ret < 0 || !strcmp(mode, "hdq")) {
> -		hdq_data->mode = 0;
> -		omap_w1_master.search = omap_w1_search_bus;
> -	} else {
> -		hdq_data->mode = 1;
> -		omap_w1_master.triplet = omap_w1_triplet;
> -	}
> +	pm_runtime_mark_last_busy(&pdev->dev);
> +	pm_runtime_put_autosuspend(&pdev->dev);
> 
> 	omap_w1_master.data = hdq_data;
> 
> @@ -739,6 +619,7 @@ static int omap_hdq_probe(struct platform_device *pdev)
> err_irq:
> 	pm_runtime_put_sync(&pdev->dev);
> err_w1:
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> 
> 	return ret;
> @@ -746,23 +627,19 @@ static int omap_hdq_probe(struct platform_device *pdev)
> 
> static int omap_hdq_remove(struct platform_device *pdev)
> {
> -	struct hdq_data *hdq_data = platform_get_drvdata(pdev);
> +	int active;
> 
> -	mutex_lock(&hdq_data->hdq_mutex);
> -
> -	if (hdq_data->hdq_usecount) {
> -		dev_dbg(&pdev->dev, "removed when use count is not zero\n");
> -		mutex_unlock(&hdq_data->hdq_mutex);
> -		return -EBUSY;
> -	}
> +	active = pm_runtime_get_sync(&pdev->dev);
> +	if (active < 0)
> +		pm_runtime_put_noidle(&pdev->dev);
> 
> -	mutex_unlock(&hdq_data->hdq_mutex);
> +	w1_remove_master_device(&omap_w1_master);
> 
> -	/* remove module dependency */
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +	if (active >= 0)
> +		pm_runtime_put_sync(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> 
> -	w1_remove_master_device(&omap_w1_master);
> -
> 	return 0;
> }
> 
> @@ -779,6 +656,7 @@ static struct platform_driver omap_hdq_driver = {
> 	.driver = {
> 		.name =	"omap_hdq",
> 		.of_match_table = omap_hdq_dt_ids,
> +		.pm = &omap_hdq_pm_ops,
> 	},
> };
> module_platform_driver(omap_hdq_driver);
> -- 
> 2.24.1

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200416 15:04]:
> Hi Tony,
> it looks as if something with this patch is broken on GTA04. For v5.6 and v5.7-rc1.
> 
> HDQ battery access times out after ca. 15 seconds and I get temperature of -273.1°C...
> 
> Reverting this patch and everything is ok again.

Hmm OK interesting.

> What is "ti,mode" about? Do we have that (indirectly) in gta04.dtsi?
> Or does this patch need some CONFIGs we do not happen to have?

Sounds like you have things working though so there should be no
need for having ti,mode = "1w" in the dts.

> > 	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);

Care to check if changing pm_runtime_set_autosuspend_delay value
to -1 in probe makes the issue go away? Or change it manually
to -1 via sysfs.

If that helps, likely we have a missing pm_runtime_get_sync()
somewhere in the driver.

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Andreas Kemnade]

On Thu, 16 Apr 2020 11:46:38 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * H. Nikolaus Schaller <hns@goldelico.com> [200416 15:04]:
> > Hi Tony,
> > it looks as if something with this patch is broken on GTA04. For v5.6 and v5.7-rc1.
> > 
> > HDQ battery access times out after ca. 15 seconds and I get temperature of -273.1°C...
> > 
> > Reverting this patch and everything is ok again.  
> 
> Hmm OK interesting.
> 
> > What is "ti,mode" about? Do we have that (indirectly) in gta04.dtsi?
> > Or does this patch need some CONFIGs we do not happen to have?  
> 
> Sounds like you have things working though so there should be no
> need for having ti,mode = "1w" in the dts.
> 
> > > 	pm_runtime_enable(&pdev->dev);
> > > +	pm_runtime_use_autosuspend(&pdev->dev);
> > > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);  
> 
> Care to check if changing pm_runtime_set_autosuspend_delay value
> to -1 in probe makes the issue go away? Or change it manually
> to -1 via sysfs.
> 
> If that helps, likely we have a missing pm_runtime_get_sync()
> somewhere in the driver.
> 
I have not tested yet with v5.7-rc1 (it is compiling right now),
but I have not seen any problems with init=/bin/bash on v5.6
and only a minimal set of modules loaded on gta04. I have seen that
42 for IDLEST

So might be something a bit more weird.

Regards,
Andreas

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [200416 20:05]:
> On Thu, 16 Apr 2020 11:46:38 -0700
> Tony Lindgren <tony@atomide.com> wrote:
> > If that helps, likely we have a missing pm_runtime_get_sync()
> > somewhere in the driver.
> > 
> I have not tested yet with v5.7-rc1 (it is compiling right now),
> but I have not seen any problems with init=/bin/bash on v5.6
> and only a minimal set of modules loaded on gta04. I have seen that
> 42 for IDLEST

Yeah I think you did test this change on gta04 earlier based
on the Tested-by from you :)

> So might be something a bit more weird.

OK please let us know what you find out.

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

> Am 16.04.2020 um 22:33 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * Andreas Kemnade <andreas@kemnade.info> [200416 20:05]:
>> On Thu, 16 Apr 2020 11:46:38 -0700
>> Tony Lindgren <tony@atomide.com> wrote:
>>> If that helps, likely we have a missing pm_runtime_get_sync()
>>> somewhere in the driver.
>>> 
>> I have not tested yet with v5.7-rc1 (it is compiling right now),
>> but I have not seen any problems with init=/bin/bash on v5.6
>> and only a minimal set of modules loaded on gta04. I have seen that
>> 42 for IDLEST
> 
> Yeah I think you did test this change on gta04 earlier based
> on the Tested-by from you :)

I think Andreas may have simply missed my test situation.

> 
>> So might be something a bit more weird.
> 
> OK please let us know what you find out.
> 
> Regards,
> 
> Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

> Am 16.04.2020 um 20:46 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [200416 15:04]:
>> Hi Tony,
>> it looks as if something with this patch is broken on GTA04. For v5.6 and v5.7-rc1.
>> 
>> HDQ battery access times out after ca. 15 seconds and I get temperature of -273.1°C...
>> 
>> Reverting this patch and everything is ok again.
> 
> Hmm OK interesting.
> 
>> What is "ti,mode" about? Do we have that (indirectly) in gta04.dtsi?
>> Or does this patch need some CONFIGs we do not happen to have?
> 
> Sounds like you have things working though so there should be no
> need for having ti,mode = "1w" in the dts.
> 
>>> 	pm_runtime_enable(&pdev->dev);
>>> +	pm_runtime_use_autosuspend(&pdev->dev);
>>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
> 
> Care to check if changing pm_runtime_set_autosuspend_delay value
> to -1 in probe makes the issue go away? Or change it manually
> to -1 via sysfs.
> 
> If that helps, likely we have a missing pm_runtime_get_sync()
> somewhere in the driver.

Yes, it does! It suffices to set it to -1 for one readout.
Aything else I can test?

root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=4049000
POWER_SUPPLY_CURRENT_NOW=233478
POWER_SUPPLY_CAPACITY=0
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=-2731
POWER_SUPPLY_TIME_TO_EMPTY_NOW=0
POWER_SUPPLY_TIME_TO_EMPTY_AVG=0
POWER_SUPPLY_TIME_TO_FULL_NOW=0
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=0
POWER_SUPPLY_CHARGE_NOW=755788
POWER_SUPPLY_CHARGE_FULL_DESIGN=1233792
POWER_SUPPLY_CYCLE_COUNT=80
POWER_SUPPLY_ENERGY_NOW=0
POWER_SUPPLY_POWER_AVG=941700
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

real    0m8.910s
user    0m0.001s
sys     0m0.028s
root@letux:~# echo -1 >/sys/bus/platform/drivers/omap_hdq/480b2000.1w/power/autosuspend_delay_ms
root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=3985000
POWER_SUPPLY_CURRENT_NOW=231871
POWER_SUPPLY_CAPACITY=78
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=354
POWER_SUPPLY_TIME_TO_EMPTY_NOW=10440
POWER_SUPPLY_TIME_TO_EMPTY_AVG=9900
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=858138
POWER_SUPPLY_CHARGE_NOW=670170
POWER_SUPPLY_CHARGE_FULL_DESIGN=1233792
POWER_SUPPLY_CYCLE_COUNT=80
POWER_SUPPLY_ENERGY_NOW=2544780
POWER_SUPPLY_POWER_AVG=922720
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

real    0m0.232s
user    0m0.001s
sys     0m0.023s
root@letux:~# echo 300 >/sys/bus/platform/drivers/omap_hdq/480b2000.1w/power/autosuspend_delay_ms
root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=3985000
POWER_SUPPLY_CURRENT_NOW=234727
POWER_SUPPLY_CAPACITY=78
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=354
POWER_SUPPLY_TIME_TO_EMPTY_NOW=10620
POWER_SUPPLY_TIME_TO_EMPTY_AVG=10140
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=858138
POWER_SUPPLY_CHARGE_NOW=669102
POWER_SUPPLY_CHARGE_FULL_DESIGN=1233792
POWER_SUPPLY_CYCLE_COUNT=80
POWER_SUPPLY_ENERGY_NOW=2541860
POWER_SUPPLY_POWER_AVG=903740
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

real    0m0.178s
user    0m0.000s
sys     0m0.029s
root@letux:~# 

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Andreas Kemnade]

On Fri, 17 Apr 2020 16:22:47 +0200
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> > Am 16.04.2020 um 20:46 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > * H. Nikolaus Schaller <hns@goldelico.com> [200416 15:04]:  
> >> Hi Tony,
> >> it looks as if something with this patch is broken on GTA04. For v5.6 and v5.7-rc1.
> >> 
> >> HDQ battery access times out after ca. 15 seconds and I get temperature of -273.1°C...
> >> 
> >> Reverting this patch and everything is ok again.  
> > 
> > Hmm OK interesting.
> >   
> >> What is "ti,mode" about? Do we have that (indirectly) in gta04.dtsi?
> >> Or does this patch need some CONFIGs we do not happen to have?  
> > 
> > Sounds like you have things working though so there should be no
> > need for having ti,mode = "1w" in the dts.
> >   
> >>> 	pm_runtime_enable(&pdev->dev);
> >>> +	pm_runtime_use_autosuspend(&pdev->dev);
> >>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);  
> > 
> > Care to check if changing pm_runtime_set_autosuspend_delay value
> > to -1 in probe makes the issue go away? Or change it manually
> > to -1 via sysfs.
> > 
> > If that helps, likely we have a missing pm_runtime_get_sync()
> > somewhere in the driver.  
> 
> Yes, it does! It suffices to set it to -1 for one readout.
> Aything else I can test?
> 
How does it depend on loaded drivers?
Is it really mainline kernel + config + devicetree or something else?

Can you reproduce the problem with init=/bin/bash
and then mount sysfs and modprobe omap_hdq?

Regarding pm_runtime stuff I thought I have the worst case scenario.

Regards,
Andreas

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

> Am 17.04.2020 um 16:43 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> On Fri, 17 Apr 2020 16:22:47 +0200
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>>> Am 16.04.2020 um 20:46 schrieb Tony Lindgren <tony@atomide.com>:
>>> 
>>> * H. Nikolaus Schaller <hns@goldelico.com> [200416 15:04]:  
>>>> Hi Tony,
>>>> it looks as if something with this patch is broken on GTA04. For v5.6 and v5.7-rc1.
>>>> 
>>>> HDQ battery access times out after ca. 15 seconds and I get temperature of -273.1°C...
>>>> 
>>>> Reverting this patch and everything is ok again.  
>>> 
>>> Hmm OK interesting.
>>> 
>>>> What is "ti,mode" about? Do we have that (indirectly) in gta04.dtsi?
>>>> Or does this patch need some CONFIGs we do not happen to have?  
>>> 
>>> Sounds like you have things working though so there should be no
>>> need for having ti,mode = "1w" in the dts.
>>> 
>>>>> 	pm_runtime_enable(&pdev->dev);
>>>>> +	pm_runtime_use_autosuspend(&pdev->dev);
>>>>> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);  
>>> 
>>> Care to check if changing pm_runtime_set_autosuspend_delay value
>>> to -1 in probe makes the issue go away? Or change it manually
>>> to -1 via sysfs.
>>> 
>>> If that helps, likely we have a missing pm_runtime_get_sync()
>>> somewhere in the driver.  
>> 
>> Yes, it does! It suffices to set it to -1 for one readout.
>> Aything else I can test?
>> 
> How does it depend on loaded drivers?
> Is it really mainline kernel + config + devicetree or something else?

Well, I can revert the patch on the same
kernel (5.6 or 5.7-rc1) + config + devicetree + user-space
and the problem is gone.

This means that something is different between the old and the new
version which makes the hdq access delayed and failing. Of course I
don't know the reason for it and what does influence it.

> 
> Can you reproduce the problem with init=/bin/bash
> and then mount sysfs and modprobe omap_hdq?

I am not sure how quickly I can test such a setup.

> Regarding pm_runtime stuff I thought I have the worst case scenario.

What may make a difference is the sequence in which drivers are loaded.

BR,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200417 14:53]:
> 
> > Am 17.04.2020 um 16:43 schrieb Andreas Kemnade <andreas@kemnade.info>:
> > 
> > On Fri, 17 Apr 2020 16:22:47 +0200
> > "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> > 
> >>> Am 16.04.2020 um 20:46 schrieb Tony Lindgren <tony@atomide.com>:
> >>> Care to check if changing pm_runtime_set_autosuspend_delay value
> >>> to -1 in probe makes the issue go away? Or change it manually
> >>> to -1 via sysfs.
> >>> 
> >>> If that helps, likely we have a missing pm_runtime_get_sync()
> >>> somewhere in the driver.  
> >> 
> >> Yes, it does! It suffices to set it to -1 for one readout.
> >> Aything else I can test?

You could sprinkle dev_info(dev, "%s\n", __func__) to the
omap_hdq_runtime_suspend() and omap_hdq_runtime_resume()
functions.

> > How does it depend on loaded drivers?
> > Is it really mainline kernel + config + devicetree or something else?
> 
> Well, I can revert the patch on the same
> kernel (5.6 or 5.7-rc1) + config + devicetree + user-space
> and the problem is gone.
> 
> This means that something is different between the old and the new
> version which makes the hdq access delayed and failing. Of course I
> don't know the reason for it and what does influence it.
> 
> > 
> > Can you reproduce the problem with init=/bin/bash
> > and then mount sysfs and modprobe omap_hdq?
> 
> I am not sure how quickly I can test such a setup.
> 
> > Regarding pm_runtime stuff I thought I have the worst case scenario.
> 
> What may make a difference is the sequence in which drivers are loaded.

Well to me it seems that we have PM runtime handling properly
implemented for all the functions in w1_bus_master omap_w1_master,
so we should not have any consumers calling into the driver
bypassing PM runtime.

Maybe the PM runtime usecounts get unbalanced somewhere in the
driver where we end up with driver permanently in disabled state?

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [200417 15:08]:
> Maybe the PM runtime usecounts get unbalanced somewhere in the
> driver where we end up with driver permanently in disabled state?

Or it could be that with omap_hdq.c no longer blocking SoC
deeper idle states, omap_hdq.c now loses context if you have
omap3 off mode enabled during idle?

If so, this can easily be fixed by adding a cpu_pm notifier
along the lines of what we already have for few drivers:

$ git grep -e CLUSTER_PM_ENTER -e CLUSTER_PM_EXIT

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Andreas Kemnade]

On Fri, 17 Apr 2020 08:14:47 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * Tony Lindgren <tony@atomide.com> [200417 15:08]:
> > Maybe the PM runtime usecounts get unbalanced somewhere in the
> > driver where we end up with driver permanently in disabled state?  
> 
> Or it could be that with omap_hdq.c no longer blocking SoC
> deeper idle states, omap_hdq.c now loses context if you have
> omap3 off mode enabled during idle?
> 
I run twice (to get mmc access cached):

root@(none):/# sleep 15 ; /usr/local/bin/idledump >/run/idledump ; cat /sys/cla
ss/power_supply/bq27000-battery/current_now ; cat /run/idledump 
32665
     CM_IDLEST1_CORE 00000042
     CM_IDLEST3_CORE 00000000
     CM_FCLKEN1_CORE 00000000
     CM_FCLKEN3_CORE 00000002
     CM_CLKSTST_CORE 00000003
     CM_IDLEST_CKGEN 00000001
    CM_IDLEST2_CKGEN 00000000
       CM_FCLKEN_DSS 00000000
       CM_IDLEST_DSS 00000000
       CM_FCLKEN_CAM 00000000
       CM_IDLEST_CAM 00000000
       CM_FCLKEN_PER 00000000
       CM_IDLEST_PER 00030000
root@(none):/# 
root@(none):/# cat /sys/kernel/debug/pm_debug/count 
usbhost_pwrdm (ON),OFF:754,RET:9148,INA:0,ON:9903,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:1,ON:2,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:237,RET:1202,INA:0,ON:1440,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm (ON),OFF:754,RET:9148,INA:0,ON:9903,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (OFF),OFF:1,RET:1,INA:0,ON:2,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:638,RET:9015,INA:250,ON:9904,RET-LOGIC-OFF:0
mpu_pwrdm (ON),OFF:638,RET:9014,INA:250,ON:9903,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm (OFF),OFF:1,RET:1,INA:0,ON:2,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0
usbhost_clkdm->usbhost_pwrdm (1)
sgx_clkdm->sgx_pwrdm (0)
per_clkdm->per_pwrdm (16)
cam_clkdm->cam_pwrdm (0)
dss_clkdm->dss_pwrdm (1)
d2d_clkdm->core_pwrdm (0)
iva2_clkdm->iva2_pwrdm (0)
mpu_clkdm->mpu_pwrdm (0)
core_l4_clkdm->core_pwrdm (21)
core_l3_clkdm->core_pwrdm (1)
neon_clkdm->neon_pwrdm (0)

Regards,
Andreas

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi Tony,

> Am 17.04.2020 um 17:07 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [200417 14:53]:
>> 
>>> Am 17.04.2020 um 16:43 schrieb Andreas Kemnade <andreas@kemnade.info>:
>>> 
>>> On Fri, 17 Apr 2020 16:22:47 +0200
>>> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
>>> 
>>>>> Am 16.04.2020 um 20:46 schrieb Tony Lindgren <tony@atomide.com>:
>>>>> Care to check if changing pm_runtime_set_autosuspend_delay value
>>>>> to -1 in probe makes the issue go away? Or change it manually
>>>>> to -1 via sysfs.
>>>>> 
>>>>> If that helps, likely we have a missing pm_runtime_get_sync()
>>>>> somewhere in the driver.  
>>>> 
>>>> Yes, it does! It suffices to set it to -1 for one readout.
>>>> Aything else I can test?
> 
> You could sprinkle dev_info(dev, "%s\n", __func__) to the
> omap_hdq_runtime_suspend() and omap_hdq_runtime_resume()
> functions.

I have done it incl. adding dev_info to hdq_read_byte(). It
looks as if the read attempts already time out during boot,
but after omap_hdq_runtime_resume(). And omap_hdq_runtime_resume()
is done well after the last one. This looks ok.

echo -1 >autosuspend_delay_ms

Does omap_hdq_runtime_resume() once.

To me it looks as if reading hqd too quickly after omap_hdq_runtime_resume()
may be part of the problem, although it is 0.4 seconds between [   18.355163]
and [   18.745269]. So I am not sure about my interpretation.

A different attempt for interpretation may be that trying to read the
slave triggers omap_hdq_runtime_resume() just before doing the
first hdq_read_byte().

This may be different context from separating these steps into different
processes/threads (echo + cat).

I.e.

[   18.355163] omap_hdq 480b2000.1w: omap_hdq_runtime_resume
[   18.432403] omap_hdq 480b2000.1w: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
[   18.745269] omap_hdq 480b2000.1w: hdq_read_byte

somehow differs from

root@letux:~# echo -1 >/sys/bus/platform/drivers/omap_hdq/480b2000.1w/power/autosuspend_delay_ms
[  544.714904] omap_hdq 480b2000.1w: omap_hdq_runtime_resume
root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
[  551.704498] omap_hdq 480b2000.1w: hdq_read_byte

BR,
Nikolaus


root@letux:~# dmesg|fgrep hdq
[   18.355163] omap_hdq 480b2000.1w: omap_hdq_runtime_resume
[   18.432403] omap_hdq 480b2000.1w: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
[   18.745269] omap_hdq 480b2000.1w: hdq_read_byte
[   19.163055] omap_hdq 480b2000.1w: hdq_read_byte
[   19.583099] omap_hdq 480b2000.1w: hdq_read_byte
[   20.003051] omap_hdq 480b2000.1w: hdq_read_byte
[   20.422973] omap_hdq 480b2000.1w: hdq_read_byte
[   20.843109] omap_hdq 480b2000.1w: hdq_read_byte
[   21.262908] omap_hdq 480b2000.1w: hdq_read_byte
[   21.682922] omap_hdq 480b2000.1w: hdq_read_byte
[   22.103149] omap_hdq 480b2000.1w: hdq_read_byte
[   22.523040] omap_hdq 480b2000.1w: hdq_read_byte
[   22.963012] omap_hdq 480b2000.1w: hdq_read_byte
[   23.390197] omap_hdq 480b2000.1w: hdq_read_byte
[   23.812988] omap_hdq 480b2000.1w: hdq_read_byte
[   24.232971] omap_hdq 480b2000.1w: hdq_read_byte
[   24.653167] omap_hdq 480b2000.1w: hdq_read_byte
[   25.073028] omap_hdq 480b2000.1w: hdq_read_byte
[   25.493011] omap_hdq 480b2000.1w: hdq_read_byte
[   25.923095] omap_hdq 480b2000.1w: hdq_read_byte
[   26.133392] omap_hdq 480b2000.1w: hdq_read_byte
[   26.553009] omap_hdq 480b2000.1w: hdq_read_byte
[   26.973083] omap_hdq 480b2000.1w: hdq_read_byte
[   27.393035] omap_hdq 480b2000.1w: hdq_read_byte
[   27.813354] omap_hdq 480b2000.1w: hdq_read_byte
[   28.233367] omap_hdq 480b2000.1w: hdq_read_byte
[   28.673309] omap_hdq 480b2000.1w: hdq_read_byte
[   29.093322] omap_hdq 480b2000.1w: hdq_read_byte
[   29.513366] omap_hdq 480b2000.1w: hdq_read_byte
[   29.948089] omap_hdq 480b2000.1w: hdq_read_byte
[   30.403076] omap_hdq 480b2000.1w: hdq_read_byte
[   30.823303] omap_hdq 480b2000.1w: hdq_read_byte
[   31.243408] omap_hdq 480b2000.1w: hdq_read_byte
[   31.663085] omap_hdq 480b2000.1w: hdq_read_byte
[   32.083312] omap_hdq 480b2000.1w: hdq_read_byte
[   32.503326] omap_hdq 480b2000.1w: hdq_read_byte
[   32.923309] omap_hdq 480b2000.1w: hdq_read_byte
[   33.343353] omap_hdq 480b2000.1w: hdq_read_byte
[   33.763305] omap_hdq 480b2000.1w: hdq_read_byte
[   33.814392] Modules linked in: wl18xx wlcore mac80211 cfg80211 libarc4 omapdrm cmac bnep panel_tpo_td028ttec1 pps_gpio snd_soc_simple_card snd_soc_simple_card_utils pps_core simple_bridge wwan_on_off snd_soc_omap_twl4030 snd_soc_w2cbw003_bt snd_soc_gtm601 display_connector generic_adc_battery pwm_bl pwm_omap_dmtimer omap_aes_driver crypto_engine omap_crypto wlcore_sdio omap3_isp videobuf2_dma_contig omap_sham videobuf2_memops videobuf2_v4l2 bq27xxx_battery_hdq bq27xxx_battery videobuf2_common omap2430 omap_hdq bmp280_spi snd_soc_omap_mcbsp snd_soc_ti_sdma ov9655 bmp280_i2c v4l2_fwnode bmp280 bmg160_i2c bmg160_core at24 tsc2007 videodev leds_tca6507 mc bno055 bmc150_magn_i2c bmc150_accel_i2c bmc150_magn bmc150_accel_core industrialio_triggered_buffer snd_soc_si47xx kfifo_buf phy_twl4030_usb musb_hdrc twl4030_pwrbutton twl4030_vibra snd_soc_twl4030 hci_uart btbcm twl4030_charger twl4030_madc industrialio input_polldev gnss_sirf bluetooth gnss ecdh_generic ecc ehci_omap omapdss omapdss_base
[   34.363281] omap_hdq 480b2000.1w: hdq_read_byte
[   34.783142] omap_hdq 480b2000.1w: hdq_read_byte
[   35.203124] omap_hdq 480b2000.1w: hdq_read_byte
[   35.623382] omap_hdq 480b2000.1w: hdq_read_byte
[   36.043273] omap_hdq 480b2000.1w: hdq_read_byte
[   36.463317] omap_hdq 480b2000.1w: hdq_read_byte
[   36.883392] omap_hdq 480b2000.1w: hdq_read_byte
[   37.303314] omap_hdq 480b2000.1w: hdq_read_byte
[   37.723327] omap_hdq 480b2000.1w: hdq_read_byte
[   38.143341] omap_hdq 480b2000.1w: hdq_read_byte
[   38.563323] omap_hdq 480b2000.1w: hdq_read_byte
[   38.983306] omap_hdq 480b2000.1w: hdq_read_byte
[   39.403350] omap_hdq 480b2000.1w: hdq_read_byte
[   39.823303] omap_hdq 480b2000.1w: hdq_read_byte
[   40.243347] omap_hdq 480b2000.1w: hdq_read_byte
[   40.663299] omap_hdq 480b2000.1w: hdq_read_byte
[   41.083374] omap_hdq 480b2000.1w: hdq_read_byte
[   41.503295] omap_hdq 480b2000.1w: hdq_read_byte
[   41.923400] omap_hdq 480b2000.1w: hdq_read_byte
[   42.343292] omap_hdq 480b2000.1w: hdq_read_byte
[   42.763305] omap_hdq 480b2000.1w: hdq_read_byte
[   43.183319] omap_hdq 480b2000.1w: hdq_read_byte
[   43.603332] omap_hdq 480b2000.1w: hdq_read_byte
[   44.188964] omap_hdq 480b2000.1w: omap_hdq_runtime_suspend
root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
[  383.654083] omap_hdq 480b2000.1w: omap_hdq_runtime_resume
[  383.868103] omap_hdq 480b2000.1w: hdq_read_byte
[  384.288055] omap_hdq 480b2000.1w: hdq_read_byte
[  384.708129] omap_hdq 480b2000.1w: hdq_read_byte
[  385.128173] omap_hdq 480b2000.1w: hdq_read_byte
[  385.548156] omap_hdq 480b2000.1w: hdq_read_byte
[  385.968139] omap_hdq 480b2000.1w: hdq_read_byte
[  385.973052] omap_hdq 480b2000.1w: hdq_read_byte
[  386.200683] omap_hdq 480b2000.1w: hdq_read_byte
[  386.215606] omap_hdq 480b2000.1w: hdq_read_byte
[  386.223358] omap_hdq 480b2000.1w: hdq_read_byte
[  386.241729] omap_hdq 480b2000.1w: hdq_read_byte
[  386.251129] omap_hdq 480b2000.1w: hdq_read_byte
[  386.260986] omap_hdq 480b2000.1w: hdq_read_byte
[  386.272460] omap_hdq 480b2000.1w: hdq_read_byte
[  386.281402] omap_hdq 480b2000.1w: hdq_read_byte
[  386.286865] omap_hdq 480b2000.1w: hdq_read_byte
[  386.510711] omap_hdq 480b2000.1w: hdq_read_byte
[  386.526153] omap_hdq 480b2000.1w: hdq_read_byte
[  386.533905] omap_hdq 480b2000.1w: hdq_read_byte
[  386.551818] omap_hdq 480b2000.1w: hdq_read_byte
[  386.561065] omap_hdq 480b2000.1w: hdq_read_byte
[  386.570922] omap_hdq 480b2000.1w: hdq_read_byte
[  386.582641] omap_hdq 480b2000.1w: hdq_read_byte
[  386.591339] omap_hdq 480b2000.1w: hdq_read_byte
[  386.602508] omap_hdq 480b2000.1w: hdq_read_byte
[  386.610900] omap_hdq 480b2000.1w: hdq_read_byte
[  386.620941] omap_hdq 480b2000.1w: hdq_read_byte
[  386.632476] omap_hdq 480b2000.1w: hdq_read_byte
[  386.641387] omap_hdq 480b2000.1w: hdq_read_byte
[  386.652709] omap_hdq 480b2000.1w: hdq_read_byte
[  386.660949] omap_hdq 480b2000.1w: hdq_read_byte
[  386.671051] omap_hdq 480b2000.1w: hdq_read_byte
[  386.680908] omap_hdq 480b2000.1w: hdq_read_byte
[  386.691986] omap_hdq 480b2000.1w: hdq_read_byte
[  386.701324] omap_hdq 480b2000.1w: hdq_read_byte
[  386.709045] omap_hdq 480b2000.1w: hdq_read_byte
[  386.720886] omap_hdq 480b2000.1w: hdq_read_byte
[  386.731872] omap_hdq 480b2000.1w: hdq_read_byte
[  386.741424] omap_hdq 480b2000.1w: hdq_read_byte
[  386.749053] omap_hdq 480b2000.1w: hdq_read_byte
[  386.761413] omap_hdq 480b2000.1w: hdq_read_byte
[  386.768798] omap_hdq 480b2000.1w: hdq_read_byte
[  386.780853] omap_hdq 480b2000.1w: hdq_read_byte
[  386.791839] omap_hdq 480b2000.1w: hdq_read_byte
[  386.801635] omap_hdq 480b2000.1w: hdq_read_byte
[  386.808746] omap_hdq 480b2000.1w: hdq_read_byte
[  386.820831] omap_hdq 480b2000.1w: hdq_read_byte
[  386.828124] omap_hdq 480b2000.1w: hdq_read_byte
[  386.840362] omap_hdq 480b2000.1w: hdq_read_byte
[  386.847534] omap_hdq 480b2000.1w: hdq_read_byte
[  386.859680] omap_hdq 480b2000.1w: hdq_read_byte
[  386.866882] omap_hdq 480b2000.1w: hdq_read_byte
[  386.882263] omap_hdq 480b2000.1w: hdq_read_byte
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_PRE[  386.897064] omap_hdq 480b2000.1w: hdq_read_byte
SENT=1
POWER_SUPPLY_VOLTAGE_NOW=4017000
POWER_SUPPLY_CURRENT_NOW=226873
POWER_SUPPLY_CAPACITY=83
POWER_SUPPL[  386.911407] omap_hdq 480b2000.1w: hdq_read_byte
Y_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=-2731
POWER_SUPPLY_T[  386.922393] omap_hdq 480b2000.1w: hdq_read_byte
IME_TO_EMPTY_NOW=900
POWER_SUPPLY_TIME_TO_EMPTY_AVG=10860
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=858138
POW[  386.937408] omap_hdq 480b2000.1w: hdq_read_byte
ER_SUPPLY_CHARGE_NOW=716806
POWER_SUPPLY_CHARGE_FULL_DESIGN=1233792
POWER_SUPPLY_CYCLE_COUNT=80
POWER_SUPPLY_[  386.951812] omap_hdq 480b2000.1w: hdq_read_byte
ENERGY_NOW=2718520
POWER_SUPPLY_POWER_AVG=900820
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas Instruments

real[  386.968322] omap_hdq 480b2000.1w: hdq_read_byte
        0m3.267s
user    0m0.002s
sys     0m0.230s
[  386.981445] omap_hdq 480b2000.1w: hdq_read_byte
[  386.991394] omap_hdq 480b2000.1w: hdq_read_byte
root@letux:~# [  387.001861] omap_hdq 480b2000.1w: hdq_read_byte
[  387.012512] omap_hdq 480b2000.1w: hdq_read_byte
[  387.020996] omap_hdq 480b2000.1w: hdq_read_byte
[  387.378234] omap_hdq 480b2000.1w: omap_hdq_runtime_suspend

root@letux:~# 
root@letux:~# echo -1 >/sys/bus/platform/drivers/omap_hdq/480b2000.1w/power/autosuspend_delay_ms
[  544.714904] omap_hdq 480b2000.1w: omap_hdq_runtime_resume
root@letux:~# time cat /sys/class/power_supply/bq27000-battery/uevent
[  551.704498] omap_hdq 480b2000.1w: hdq_read_byte
[  551.711944] omap_hdq 480b2000.1w: hdq_read_byte
[  551.727844] omap_hdq 480b2000.1w: hdq_read_byte
[  551.735015] omap_hdq 480b2000.1w: hdq_read_byte
[  551.748260] omap_hdq 480b2000.1w: hdq_read_byte
[  551.755432] omap_hdq 480b2000.1w: hdq_read_byte
[  551.768615] omap_hdq 480b2000.1w: hdq_read_byte
[  551.775787] omap_hdq 480b2000.1w: hdq_read_byte
[  551.788604] omap_hdq 480b2000.1w: hdq_read_byte
[  551.795776] omap_hdq 480b2000.1w: hdq_read_byte
[  551.808715] omap_hdq 480b2000.1w: hdq_read_byte
[  551.815887] omap_hdq 480b2000.1w: hdq_read_byte
[  551.828735] omap_hdq 480b2000.1w: hdq_read_byte
[  551.839477] omap_hdq 480b2000.1w: hdq_read_byte
[  551.849578] omap_hdq 480b2000.1w: hdq_read_byte
[  551.859802] omap_hdq 480b2000.1w: hdq_read_byte
[  551.869720] omap_hdq 480b2000.1w: hdq_read_byte
[  551.879913] omap_hdq 480b2000.1w: hdq_read_byte
[  551.889739] omap_hdq 480b2000.1w: hdq_read_byte
[  551.899780] omap_hdq 480b2000.1w: hdq_read_byte
[  551.909667] omap_hdq 480b2000.1w: hdq_read_byte
[  551.919677] omap_hdq 480b2000.1w: hdq_read_byte
[  551.929748] omap_hdq 480b2000.1w: hdq_read_byte
[  551.939727] omap_hdq 480b2000.1w: hdq_read_byte
[  551.949768] omap_hdq 480b2000.1w: hdq_read_byte
[  551.959716] omap_hdq 480b2000.1w: hdq_read_byte
[  551.969787] omap_hdq 480b2000.1w: hdq_read_byte
[  551.979888] omap_hdq 480b2000.1w: hdq_read_byte
[  551.987060] omap_hdq 480b2000.1w: hdq_read_byte
[  551.999176] omap_hdq 480b2000.1w: hdq_read_byte
[  552.007049] omap_hdq 480b2000.1w: hdq_read_byte
[  552.019042] omap_hdq 480b2000.1w: hdq_read_byte
[  552.026977] omap_hdq 480b2000.1w: hdq_read_byte
[  552.039184] omap_hdq 480b2000.1w: hdq_read_byte
[  552.046844] omap_hdq 480b2000.1w: hdq_read_byte
[  552.058288] omap_hdq 480b2000.1w: hdq_read_byte
[  552.065521] omap_hdq 480b2000.1w: hdq_read_byte
[  552.078613] omap_hdq 480b2000.1w: hdq_read_byte
[  552.085815] omap_hdq 480b2000.1w: hdq_read_byte
[  552.098785] omap_hdq 480b2000.1w: hdq_read_byte
[  552.106170] omap_hdq 480b2000.1w: hdq_read_byte
[  552.118682] omap_hdq 480b2000.1w: hdq_read_byte
[  552.126037] omap_hdq 480b2000.1w: hdq_read_byte
[  552.138702] omap_hdq 480b2000.1w: hdq_read_byte
[  552.146667] omap_hdq 480b2000.1w: hdq_read_byte
[  552.159271] omap_hdq 480b2000.1w: hdq_read_byte
[  552.166900] omap_hdq 480b2000.1w: hdq_read_byte
[  552.181365] omap_hdq 480b2000.1w: hdq_read_byte
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_PRESENT=1
POWER_S[  552.199462] omap_hdq 480b2000.1w: hdq_read_byte
UPPLY_VOLTAGE_NOW=4012000
POWER_SUPPLY_CURRENT_NOW=220447
POWE[  552.210998] omap_hdq 480b2000.1w: hdq_read_byte
R_SUPPLY_CAPACITY=82
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=289
POWER_SUPPLY_TIME_TO_EMPTY_NOW=11580
POWER_SUPPLY_TIME_TO_EMPT[  552.227203] omap_hdq 480b2000.1w: hdq_read_byte
Y_AVG=11040
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL=858138
POWER_SUPPLY_CHARGE_NOW=706660
PO[  552.241851] omap_hdq 480b2000.1w: hdq_read_byte
WER_SUPPLY_CHARGE_FULL_DESIGN=1233792
POWER_SUPPLY_CYCLE_COUNT=80
POWER_SUPPLY_ENERGY_NOW=2680560
POWER_SUPPLY_POWER_AVG=8760[  552.257995] omap_hdq 480b2000.1w: hdq_read_byte
00
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_MANUFACTURER=Texas In[  552.269348] omap_hdq 480b2000.1w: hdq_read_byte
struments

real    0m0.510s
user    0m0.001s
sys     0m0.192s
root@letux:~# [  552.286468] omap_hdq 480b2000.1w: hdq_read_byte
[  552.294067] omap_hdq 480b2000.1w: hdq_read_byte
[  552.305786] omap_hdq 480b2000.1w: hdq_read_byte
[  552.317443] omap_hdq 480b2000.1w: hdq_read_byte
[  552.324645] omap_hdq 480b2000.1w: hdq_read_byte

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200417 21:04]:
> To me it looks as if reading hqd too quickly after omap_hdq_runtime_resume()
> may be part of the problem, although it is 0.4 seconds between [   18.355163]
> and [   18.745269]. So I am not sure about my interpretation.
> 
> A different attempt for interpretation may be that trying to read the
> slave triggers omap_hdq_runtime_resume() just before doing the
> first hdq_read_byte().

Hmm so I wonder if adding msleep(100) at the end of
omap_hdq_runtime_resume() might help?

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi Tony,

> Am 20.04.2020 um 17:08 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [200417 21:04]:
>> To me it looks as if reading hqd too quickly after omap_hdq_runtime_resume()
>> may be part of the problem, although it is 0.4 seconds between [   18.355163]
>> and [   18.745269]. So I am not sure about my interpretation.
>> 
>> A different attempt for interpretation may be that trying to read the
>> slave triggers omap_hdq_runtime_resume() just before doing the
>> first hdq_read_byte().
> 
> Hmm so I wonder if adding msleep(100) at the end of
> omap_hdq_runtime_resume() might help?

I have tried and initially it did boot and work once.
But after the second boot/reboot the effect was back.

This is something I have observed previously, that the issue
is there in ca. 9 or 10 boot attempts. So I would assume
some race condition with udev reading the uevent file of the
bq27xxx bus client and hence through hdq.

I already had noticed some hqd_read activity right after probing
success.

I had also tried to change pm_runtime_set_autosuspend_delay(, 1000)
with no success. And I tried to call omap_hdq_runtime_resume() at the
end of probe.

The only maybe important observation was when I disabled all
kernel modules except *hdq*.ko and *bq27*.ko. Then I did only
get an emergency shell so that it is quite similar to the
scenario Andreas has tested. With this setup it did work.

I then tried to reenable other kernel modules but the result
wasn't convincing that it gives a reliable result.

So I have still no clear indication when the problem occurs and
when not.

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Andreas Kemnade]

On Mon, 20 Apr 2020 23:11:18 +0200
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> Hi Tony,
> 
> > Am 20.04.2020 um 17:08 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > * H. Nikolaus Schaller <hns@goldelico.com> [200417 21:04]:  
> >> To me it looks as if reading hqd too quickly after omap_hdq_runtime_resume()
> >> may be part of the problem, although it is 0.4 seconds between [   18.355163]
> >> and [   18.745269]. So I am not sure about my interpretation.
> >> 
> >> A different attempt for interpretation may be that trying to read the
> >> slave triggers omap_hdq_runtime_resume() just before doing the
> >> first hdq_read_byte().  
> > 
> > Hmm so I wonder if adding msleep(100) at the end of
> > omap_hdq_runtime_resume() might help?  
> 
> I have tried and initially it did boot and work once.
> But after the second boot/reboot the effect was back.
> 
> This is something I have observed previously, that the issue
> is there in ca. 9 or 10 boot attempts. So I would assume
> some race condition with udev reading the uevent file of the
> bq27xxx bus client and hence through hdq.
> 
> I already had noticed some hqd_read activity right after probing
> success.
> 
> I had also tried to change pm_runtime_set_autosuspend_delay(, 1000)
> with no success. And I tried to call omap_hdq_runtime_resume() at the
> end of probe.
> 
> The only maybe important observation was when I disabled all
> kernel modules except *hdq*.ko and *bq27*.ko. Then I did only
> get an emergency shell so that it is quite similar to the
> scenario Andreas has tested. With this setup it did work.
> 
So I guess without idling uarts?

> I then tried to reenable other kernel modules but the result
> wasn't convincing that it gives a reliable result.
> 
> So I have still no clear indication when the problem occurs and
> when not.
> 
Hmm, last summer I had problems even without that patch reading
temperature while doing umts transfers. Maybe there are some
connections,
maybe not. For that scenario we might have emc issues, thermal problems
or a real kernel problem.

Regards,
Andreas

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [200421 06:54]:
> On Mon, 20 Apr 2020 23:11:18 +0200
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> > The only maybe important observation was when I disabled all
> > kernel modules except *hdq*.ko and *bq27*.ko. Then I did only
> > get an emergency shell so that it is quite similar to the
> > scenario Andreas has tested. With this setup it did work.
> > 
> So I guess without idling uarts?
> 
> > I then tried to reenable other kernel modules but the result
> > wasn't convincing that it gives a reliable result.
> > 
> > So I have still no clear indication when the problem occurs and
> > when not.
> > 
> Hmm, last summer I had problems even without that patch reading
> temperature while doing umts transfers. Maybe there are some
> connections,
> maybe not. For that scenario we might have emc issues, thermal problems
> or a real kernel problem.

I have confimed here that logicpd torpedo devkit with battery
works just fine while entering off mode during idle. I can see
the temperature just fine with:

# cat /sys/class/power_supply/bq27000-battery/uevent
POWER_SUPPLY_NAME=bq27000-battery
POWER_SUPPLY_STATUS=Charging
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_VOLTAGE_NOW=3829000
POWER_SUPPLY_CURRENT_NOW=-592084
POWER_SUPPLY_CAPACITY_LEVEL=Normal
POWER_SUPPLY_TEMP=306
POWER_SUPPLY_TECHNOLOGY=Li-ion
POWER_SUPPLY_CHARGE_FULL_DESIGN=2056320
POWER_SUPPLY_CYCLE_COUNT=0
POWER_SUPPLY_POWER_AVG=0
POWER_SUPPLY_MANUFACTURER=Texas Instruments

This is 37xx though, maybe you have 35xx and there's some errata
that we're not handling?

I'm only seeing "2.7. HDQTM/1-Wire® Communication Constraints"
for external pull-up resitor in 34xx errata at [0].

I wonder if wrong external pull could cause flakyeness after
enabling the hdq module?

If nothing else helps, you could try to block idle for hdq
module, but I have a feeling that's a workaround for something
else.

Regards,

Tony

[0] https://www.ti.com/pdfs/wtbu/SWPZ009A_OMAP4430_Errata_Public_vA.pdf

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi Tony,

> Am 21.04.2020 um 20:02 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * Andreas Kemnade <andreas@kemnade.info> [200421 06:54]:
>> On Mon, 20 Apr 2020 23:11:18 +0200
>> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
>>> The only maybe important observation was when I disabled all
>>> kernel modules except *hdq*.ko and *bq27*.ko. Then I did only
>>> get an emergency shell so that it is quite similar to the
>>> scenario Andreas has tested. With this setup it did work.
>>> 
>> So I guess without idling uarts?
>> 
>>> I then tried to reenable other kernel modules but the result
>>> wasn't convincing that it gives a reliable result.
>>> 
>>> So I have still no clear indication when the problem occurs and
>>> when not.
>>> 
>> Hmm, last summer I had problems even without that patch reading
>> temperature while doing umts transfers. Maybe there are some
>> connections,
>> maybe not. For that scenario we might have emc issues, thermal problems
>> or a real kernel problem.
> 
> I have confimed here that logicpd torpedo devkit with battery
> works just fine while entering off mode during idle. I can see
> the temperature just fine with:
> 
> # cat /sys/class/power_supply/bq27000-battery/uevent
> POWER_SUPPLY_NAME=bq27000-battery
> POWER_SUPPLY_STATUS=Charging
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_VOLTAGE_NOW=3829000
> POWER_SUPPLY_CURRENT_NOW=-592084
> POWER_SUPPLY_CAPACITY_LEVEL=Normal
> POWER_SUPPLY_TEMP=306
> POWER_SUPPLY_TECHNOLOGY=Li-ion
> POWER_SUPPLY_CHARGE_FULL_DESIGN=2056320
> POWER_SUPPLY_CYCLE_COUNT=0
> POWER_SUPPLY_POWER_AVG=0
> POWER_SUPPLY_MANUFACTURER=Texas Instruments
> 
> This is 37xx though, maybe you have 35xx and there's some errata
> that we're not handling?

No, it is dm3730 on three different units I have tried.

> I'm only seeing "2.7. HDQTM/1-Wire® Communication Constraints"
> for external pull-up resitor in 34xx errata at [0].
> 
> I wonder if wrong external pull could cause flakyeness after
> enabling the hdq module?

I have checked and we have 10 kOhm pullup to 1.8 V and a 470 Ohm
series resistor.

> 
> If nothing else helps, you could try to block idle for hdq
> module, but I have a feeling that's a workaround for something
> else.

Well, what helps is reverting the patch and using the old driver
(which did work for several years). So I would not assume that
there is a hardware influence. It seems to be something the new
driver is doing differently.

I need more time to understand and trace this issue on what it
depends... It may depend on the sequence some other modules are
loaded and what the user-space (udevd) is doing in the meantime.

> 
> Regards,
> 
> Tony
> 
> [0] https://www.ti.com/pdfs/wtbu/SWPZ009A_OMAP4430_Errata_Public_vA.pdf
> 

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200421 18:14]:
> > Am 21.04.2020 um 20:02 schrieb Tony Lindgren <tony@atomide.com>:
> > This is 37xx though, maybe you have 35xx and there's some errata
> > that we're not handling?
> 
> No, it is dm3730 on three different units I have tried.
> 
> > I'm only seeing "2.7. HDQTM/1-Wire® Communication Constraints"
> > for external pull-up resitor in 34xx errata at [0].
> > 
> > I wonder if wrong external pull could cause flakyeness after
> > enabling the hdq module?
> 
> I have checked and we have 10 kOhm pullup to 1.8 V and a 470 Ohm
> series resistor.

OK

> > If nothing else helps, you could try to block idle for hdq
> > module, but I have a feeling that's a workaround for something
> > else.
> 
> Well, what helps is reverting the patch and using the old driver
> (which did work for several years). So I would not assume that
> there is a hardware influence. It seems to be something the new
> driver is doing differently.

Well earlier hdq1w.c did not idle, now it does. If you just want
to keep it enabled like earlier, you can just add something like:

&hdqw1w {
	ti,no-idle;
};

> I need more time to understand and trace this issue on what it
> depends... It may depend on the sequence some other modules are
> loaded and what the user-space (udevd) is doing in the meantime.

Yes would be good to understand what goes wrong here before we
apply the ti,no-idle as that will block SoC deeper idle states.

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Andreas Kemnade]

On Tue, 21 Apr 2020 11:20:17 -0700
Tony Lindgren <tony@atomide.com> wrote:

> * H. Nikolaus Schaller <hns@goldelico.com> [200421 18:14]:
> > > Am 21.04.2020 um 20:02 schrieb Tony Lindgren <tony@atomide.com>:
> > > This is 37xx though, maybe you have 35xx and there's some errata
> > > that we're not handling?  
> > 
> > No, it is dm3730 on three different units I have tried.
> >   
> > > I'm only seeing "2.7. HDQTM/1-Wire® Communication Constraints"
> > > for external pull-up resitor in 34xx errata at [0].
> > > 
> > > I wonder if wrong external pull could cause flakyeness after
> > > enabling the hdq module?  
> > 
> > I have checked and we have 10 kOhm pullup to 1.8 V and a 470 Ohm
> > series resistor.  
> 
> OK
> 
> > > If nothing else helps, you could try to block idle for hdq
> > > module, but I have a feeling that's a workaround for something
> > > else.  
> > 
> > Well, what helps is reverting the patch and using the old driver
> > (which did work for several years). So I would not assume that
> > there is a hardware influence. It seems to be something the new
> > driver is doing differently.  
> 
> Well earlier hdq1w.c did not idle, now it does. If you just want
> to keep it enabled like earlier, you can just add something like:
> 
> &hdqw1w {
> 	ti,no-idle;
> };
> 
> > I need more time to understand and trace this issue on what it
> > depends... It may depend on the sequence some other modules are
> > loaded and what the user-space (udevd) is doing in the meantime.  
> 
> Yes would be good to understand what goes wrong here before we
> apply the ti,no-idle as that will block SoC deeper idle states.
>

hmm, he is testing without idling uarts, so I am a bit confused here,
the problem only seems to occur when more things are *active*.
Is something not handled in time.

Regards,
Andreas

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi Tony,

> Am 21.04.2020 um 20:20 schrieb Tony Lindgren <tony@atomide.com>:
> 
>> Well, what helps is reverting the patch and using the old driver
>> (which did work for several years). So I would not assume that
>> there is a hardware influence. It seems to be something the new
>> driver is doing differently.
> 
> Well earlier hdq1w.c did not idle, now it does.

Ah, I see!

> If you just want to keep it enabled like earlier, you can just add something like:

Well, I don't want it enabled, it just should work as before.
Ideally including all improvements :)

> 
> &hdqw1w {
> 	ti,no-idle;
> };

I have added that and there might be a slightly different pattern
(unless that is just by luck): the first two or three attempts to
read the bq27xx/uevent did still time out. But then the next ones
worked fine (with a response time of ca. 65 .. 230 ms).

So as if something needs to be shaken into the right position after
boot until it works.

Interestingly, after reinstalling the version without ti,no-idle;
it did work well on the first boot but not afterwards. Like there
is some memory surviving powerdown and battery removal... But again,
it started to work after 6 or 7 failed attempts.

If only it weren't so time-consuming to perform such tests...

>> I need more time to understand and trace this issue on what it
>> depends... It may depend on the sequence some other modules are
>> loaded and what the user-space (udevd) is doing in the meantime.
> 
> Yes would be good to understand what goes wrong here before we
> apply the ti,no-idle as that will block SoC deeper idle states.
> 
> Regards,
> 
> Tony

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Andreas Kemnade]

On Tue, 21 Apr 2020 22:40:39 +0200
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> Hi Tony,
> 
> > Am 21.04.2020 um 20:20 schrieb Tony Lindgren <tony@atomide.com>:
> >   
> >> Well, what helps is reverting the patch and using the old driver
> >> (which did work for several years). So I would not assume that
> >> there is a hardware influence. It seems to be something the new
> >> driver is doing differently.  
> > 
> > Well earlier hdq1w.c did not idle, now it does.  
> 
> Ah, I see!
> 
> > If you just want to keep it enabled like earlier, you can just add something like:  
> 
> Well, I don't want it enabled, it just should work as before.
> Ideally including all improvements :)
> 
> > 
> > &hdqw1w {
> > 	ti,no-idle;
> > };  
> 
> I have added that and there might be a slightly different pattern
> (unless that is just by luck): the first two or three attempts to
> read the bq27xx/uevent did still time out. But then the next ones
> worked fine (with a response time of ca. 65 .. 230 ms).
> 
> So as if something needs to be shaken into the right position after
> boot until it works.
> 

What about reading battery with just omap_hdq loaded and then continue
booting as usual, e.g. something like:

have script init-bat.sh
#!/bin/sh
modprobe omap_hdq
cat /sys/class/power_supply/bq27000_battery/uevent
exec /sbin/init "$@"

and then append to kernel parameters init=/init-bat.sh

Regards,
Andreas

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi,

> Am 22.04.2020 um 12:04 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> On Tue, 21 Apr 2020 22:40:39 +0200
> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
> 
>> Hi Tony,
>> 
>>> Am 21.04.2020 um 20:20 schrieb Tony Lindgren <tony@atomide.com>:
>>> 
>>>> Well, what helps is reverting the patch and using the old driver
>>>> (which did work for several years). So I would not assume that
>>>> there is a hardware influence. It seems to be something the new
>>>> driver is doing differently.  
>>> 
>>> Well earlier hdq1w.c did not idle, now it does.  
>> 
>> Ah, I see!
>> 
>>> If you just want to keep it enabled like earlier, you can just add something like:  
>> 
>> Well, I don't want it enabled, it just should work as before.
>> Ideally including all improvements :)
>> 
>>> 
>>> &hdqw1w {
>>> 	ti,no-idle;
>>> };  
>> 
>> I have added that and there might be a slightly different pattern
>> (unless that is just by luck): the first two or three attempts to
>> read the bq27xx/uevent did still time out. But then the next ones
>> worked fine (with a response time of ca. 65 .. 230 ms).
>> 
>> So as if something needs to be shaken into the right position after
>> boot until it works.
>> 
> 
> What about reading battery with just omap_hdq loaded and then continue
> booting as usual, e.g. something like:
> 
> have script init-bat.sh
> #!/bin/sh
> modprobe omap_hdq
> cat /sys/class/power_supply/bq27000_battery/uevent
> exec /sbin/init "$@"
> 
> and then append to kernel parameters init=/init-bat.sh

Interesting idea. But how would it help to identify the
issue?

I can confirm that after a while of attempting to read
the uevent it suddenly starts to work.

Then I can even swap the battery (and verify by looking
at the cycle count which changes).

I have some unconfirmed impression that it helps to run
two cat /sys/class/power_supply/bq27000_battery/uevent
in quick enough succession to get it out of the hickup mode.
This needs more test.

It would indicate that if a new omap_hdq_runtime_resume()
is called by the second open+read+close sequence before
the autosuspend (300ms) of the previous omap_hdq_runtime_suspend()
happens, something gets back in place for the second
omap_hdq_runtime_suspend().

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi,

> Am 24.04.2020 um 18:52 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Tony and Andreas,
> 
>> Am 22.04.2020 um 18:06 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>> 
>> Hi,
>> 
>>> Am 22.04.2020 um 12:04 schrieb Andreas Kemnade <andreas@kemnade.info>:
>>> 
>>> On Tue, 21 Apr 2020 22:40:39 +0200
>>> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
>>> 
>>>> Hi Tony,
>>>> 
>>>> 
>>>> I have added that and there might be a slightly different pattern
>>>> (unless that is just by luck): the first two or three attempts to
>>>> read the bq27xx/uevent did still time out. But then the next ones
>>>> worked fine (with a response time of ca. 65 .. 230 ms).
> 
> Now I have focussed on the hdq_read_byte() timeout and enabled more
> driver debugging.
> 
> Yes, there are a lot of "timeout waiting for RXCOMPLETE" reports.
> Sometimes with ",0" and sometimes with ",1" (hdq_data->hdq_irqstatus).
> 
...

> So I have no idea why it sometimes fails. A race?
> Something not locked? Interrupt not delivered in time?
> 
> The only thing I noticed in code is that hdq_write_byte() doesn't
> use the hdq_mutex although they share the hdq_irqstatus.
> 
> But hdq_write_byte() doesn't seem to be used for booting and
> reading the bq27xxx.

It is. I had a bug in my printk.

> 
> Another idea: there may occur two interrupts between call
> to wait_event_timeout(hdq_data->hdq_irqstatus) and
> status = hdq_data->hdq_irqstatus; and calling hdq_reset_irqstatus().
> 
> These changes hdq_irqstatus. And at the end of the read
> process hdq_irqstatus is set to 0 potentially wiping
> out interrupt flags.
> 
> Just two ideas where we may have missing some locking.
> 
> There is one more observation: the hdq_read_byte()
> code now also calls hdq_reset_irqstatus(hdq_data);
> in the success case which the old code didn't. So
> there may be an additional risk of loosing interrupts.

After enabling more debugging I got this interesting snapshot:

[ 1659.330627] omap_hdq 480b2000.1w: hdq_write_byte
[ 1659.349914] omap_hdq 480b2000.1w: hdq_isr: 0
[ 1659.563903] omap_hdq 480b2000.1w: TX wait elapsed
[ 1659.568939] omap_hdq 480b2000.1w: TX failure:Ctrl status 0
[ 1659.583831] omap_hdq 480b2000.1w: hdq_read_byte
[ 1659.588684] omap_hdq 480b2000.1w: hdq_isr: 0
[ 1659.793945] omap_hdq 480b2000.1w: timeout waiting for RXCOMPLETE, 0
[ 1659.800659] omap_hdq 480b2000.1w: hdq_write_byte
[ 1659.813812] omap_hdq 480b2000.1w: hdq_isr: 0
[ 1660.023956] omap_hdq 480b2000.1w: TX wait elapsed
[ 1660.028961] omap_hdq 480b2000.1w: TX failure:Ctrl status 0
[ 1660.043823] omap_hdq 480b2000.1w: hdq_read_byte
[ 1660.048675] omap_hdq 480b2000.1w: hdq_isr: 6
[ 1660.053253] omap_hdq 480b2000.1w: hdq_write_byte
[ 1660.074371] omap_hdq 480b2000.1w: hdq_isr: 2
[ 1660.078948] omap_hdq 480b2000.1w: timeout waiting for TXCOMPLETE/RXCOMPLETE, 2
[ 1660.103851] omap_hdq 480b2000.1w: TX failure:Ctrl status 2
[ 1660.109710] omap_hdq 480b2000.1w: hdq_read_byte
[ 1660.124267] omap_hdq 480b2000.1w: hdq_isr: 1
[ 1660.333953] omap_hdq 480b2000.1w: timeout waiting for RXCOMPLETE, 1
[ 1660.340637] omap_hdq 480b2000.1w: hdq_write_byte
[ 1660.355957] omap_hdq 480b2000.1w: hdq_isr: 4
[ 1660.360565] omap_hdq 480b2000.1w: hdq_read_byte
[ 1660.373809] omap_hdq 480b2000.1w: hdq_isr: 2
[ 1660.378356] omap_hdq 480b2000.1w: hdq_write_byte
[ 1660.385345] omap_hdq 480b2000.1w: hdq_isr: 4
[ 1660.394287] omap_hdq 480b2000.1w: hdq_read_byte
[ 1660.399078] omap_hdq 480b2000.1w: hdq_isr: 2
[ 1660.413787] omap_hdq 480b2000.1w: hdq_write_byte
[ 1660.420776] omap_hdq 480b2000.1w: hdq_isr: 4
[ 1660.425537] omap_hdq 480b2000.1w: hdq_read_byte
[ 1660.430297] omap_hdq 480b2000.1w: hdq_isr: 2

It looks as if the hdq_write_byte commands fail!
Then, there is of course no response from the bq27xxx
and a hdq_read_byte will also fail.

The things start to get "fixed" when the hdq_isr
jumps to 6 indicating

OMAP_HDQ_INT_STATUS_RXCOMPLETE | OMAP_HDQ_INT_STATUS_TXCOMPLETE

So I am getting more inclined to believe that it is
not a power management issue but some piggybacked
change to how interrupts are handled.
Especially hdq_reset_irqstatus.

So I will add a printk to hdq_reset_irqstatus
to see what value it had before being reset.

And I wonder what the spinlock around setting
hdq_reset_irqstatus = 0 is good for. This should
be an atomic operation anyways.

One more code observation: the goto out_err
in omap_w1_write_byte is not needed.

BR,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

> Am 25.04.2020 um 12:29 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> H
> The things start to get "fixed" when the hdq_isr
> jumps to 6 indicating
> 
> OMAP_HDQ_INT_STATUS_RXCOMPLETE | OMAP_HDQ_INT_STATUS_TXCOMPLETE
> 
> So I am getting more inclined to believe that it is
> not a power management issue but some piggybacked
> change to how interrupts are handled.
> Especially hdq_reset_irqstatus.
> 
> So I will add a printk to hdq_reset_irqstatus
> to see what value it had before being reset.

I now did check the log during boot and there is the
reverse situation. Initially it works but suddenly
hdq_isr becomes 6 and then trouble starts.

So the key problem is that both, the RX and the TX
interrupts may be set and then, the code resets
everything to 0 and looses either one.

I wonder if that is an issue by two processes reading
hdq in parallel.

Another question is how independent command-writes + result-reads
are properly serialized and locked so that they don't overlap?

Maybe this is handled outside of the omap_hdq code.

So what could be a fix?

BR,
Nikolaus
 
[   17.026916] omap_hdq 480b2000.1w: omap_hdq_probe
[   17.057739] omap_hdq 480b2000.1w: omap_hdq_probe 1
[   17.062866] omap_hdq 480b2000.1w: omap_hdq_runtime_resume
[   17.221374] omap_hdq 480b2000.1w: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
[   17.350860] omap_hdq 480b2000.1w: omap_hdq_probe 2
[   17.372863] omap_hdq 480b2000.1w: omap_hdq_probe 3
[   17.468444] omap_hdq 480b2000.1w: hdq_isr: 1
[   17.473876] omap_hdq 480b2000.1w: Presence bit not set
[   17.505249] omap_hdq 480b2000.1w: omap_hdq_probe 4
[   17.576690] omap_hdq 480b2000.1w: omap_hdq_probe done
[   17.697998] omap_hdq 480b2000.1w: hdq_write_byte
[   17.704986] omap_hdq 480b2000.1w: hdq_isr: 4
[   17.734954] omap_hdq 480b2000.1w: hdq_read_byte
[   17.747528] omap_hdq 480b2000.1w: hdq_isr: 2
[   17.752044] omap_hdq 480b2000.1w: hdq_write_byte
[   17.759033] omap_hdq 480b2000.1w: hdq_isr: 4
[   17.774414] omap_hdq 480b2000.1w: hdq_read_byte
[   17.798767] omap_hdq 480b2000.1w: hdq_isr: 2
[   17.803314] omap_hdq 480b2000.1w: hdq_write_byte
[   17.821807] omap_hdq 480b2000.1w: hdq_isr: 4
[   17.826385] omap_hdq 480b2000.1w: hdq_read_byte
[   17.837646] omap_hdq 480b2000.1w: hdq_isr: 2
[   17.842224] omap_hdq 480b2000.1w: hdq_write_byte
[   17.849212] omap_hdq 480b2000.1w: hdq_isr: 4
[   17.861877] omap_hdq 480b2000.1w: hdq_read_byte
[   17.887573] omap_hdq 480b2000.1w: hdq_isr: 2
[   17.892150] omap_hdq 480b2000.1w: hdq_write_byte
[   17.899139] omap_hdq 480b2000.1w: hdq_isr: 4
[   17.903686] omap_hdq 480b2000.1w: hdq_read_byte
[   17.926177] omap_hdq 480b2000.1w: hdq_isr: 2
[   17.945434] omap_hdq 480b2000.1w: hdq_write_byte
[   17.953979] omap_hdq 480b2000.1w: hdq_isr: 4
[   17.959503] omap_hdq 480b2000.1w: hdq_read_byte
[   17.964294] omap_hdq 480b2000.1w: hdq_isr: 2
[   17.984436] omap_hdq 480b2000.1w: hdq_write_byte
[   18.017578] omap_hdq 480b2000.1w: hdq_isr: 6
[   18.022521] omap_hdq 480b2000.1w: hdq_read_byte
[   18.027282] omap_hdq 480b2000.1w: hdq_isr: 0
[   18.287597] omap_hdq 480b2000.1w: timeout waiting for RXCOMPLETE, 0
[   18.294250] omap_hdq 480b2000.1w: hdq_write_byte
[   18.313720] omap_hdq 480b2000.1w: hdq_isr: 0
[   18.537536] omap_hdq 480b2000.1w: TX wait elapsed
[   18.542510] omap_hdq 480b2000.1w: TX failure:Ctrl status 0
[   18.577697] omap_hdq 480b2000.1w: hdq_read_byte
[   18.582489] omap_hdq 480b2000.1w: hdq_isr: 0
[   18.787597] omap_hdq 480b2000.1w: timeout waiting for RXCOMPLETE, 0

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi,

> Am 25.04.2020 um 12:37 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 
>> Am 25.04.2020 um 12:29 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>> 
>> H
>> The things start to get "fixed" when the hdq_isr
>> jumps to 6 indicating
>> 
>> OMAP_HDQ_INT_STATUS_RXCOMPLETE | OMAP_HDQ_INT_STATUS_TXCOMPLETE
>> 
>> So I am getting more inclined to believe that it is
>> not a power management issue but some piggybacked
>> change to how interrupts are handled.
>> Especially hdq_reset_irqstatus.
>> 
>> So I will add a printk to hdq_reset_irqstatus
>> to see what value it had before being reset.
> 
> I now did check the log during boot and there is the
> reverse situation. Initially it works but suddenly
> hdq_isr becomes 6 and then trouble starts.
> 
> So the key problem is that both, the RX and the TX
> interrupts may be set and then, the code resets
> everything to 0 and looses either one.
> 
> I wonder if that is an issue by two processes reading
> hdq in parallel.
> 
> Another question is how independent command-writes + result-reads
> are properly serialized and locked so that they don't overlap?

I have reworked the way the spinlocks, setting and resetting
of the hdq_irqstatus bits are done and now it works right from
start of boot. Without any timeouts or delays.

I am not exactly sure what went wrong, but it seems as if
the read is already done when the write interrupt status
bit is processed. Then, the old logic did wipe out both
bits by hdq_reset_irqstatus() and the read code did timeout
because it did not notice that the data had already been
available. This may depend on other system activities so
that it can explain why other tests didn't reveal it.

omap_hdq_runtime_resume() and omap_hdq_runtime_suspend()
also behave fine.

Before I can post something I need to clean up my hacks
and add similar fixes to omap_hdq_break() and omap_w1_triplet()
where I hope that I don't break those...

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200429 21:35]:
> I have reworked the way the spinlocks, setting and resetting
> of the hdq_irqstatus bits are done and now it works right from
> start of boot. Without any timeouts or delays.
> 
> I am not exactly sure what went wrong, but it seems as if
> the read is already done when the write interrupt status
> bit is processed. Then, the old logic did wipe out both
> bits by hdq_reset_irqstatus() and the read code did timeout
> because it did not notice that the data had already been
> available. This may depend on other system activities so
> that it can explain why other tests didn't reveal it.
> 
> omap_hdq_runtime_resume() and omap_hdq_runtime_suspend()
> also behave fine.
> 
> Before I can post something I need to clean up my hacks
> and add similar fixes to omap_hdq_break() and omap_w1_triplet()
> where I hope that I don't break those...

OK good to hear you were able to figure out what is
going on here.

Regards,

Tony

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[H. Nikolaus Schaller]

Hi Tony,

> Am 29.04.2020 um 23:38 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [200429 21:35]:
>> I have reworked the way the spinlocks, setting and resetting
>> of the hdq_irqstatus bits are done and now it works right from
>> start of boot. Without any timeouts or delays.
>> 
>> I am not exactly sure what went wrong, but it seems as if
>> the read is already done when the write interrupt status
>> bit is processed. Then, the old logic did wipe out both
>> bits by hdq_reset_irqstatus() and the read code did timeout
>> because it did not notice that the data had already been
>> available. This may depend on other system activities so
>> that it can explain why other tests didn't reveal it.
>> 
>> omap_hdq_runtime_resume() and omap_hdq_runtime_suspend()
>> also behave fine.
>> 
>> Before I can post something I need to clean up my hacks
>> and add similar fixes to omap_hdq_break() and omap_w1_triplet()
>> where I hope that I don't break those...
> 
> OK good to hear you were able to figure out what is
> going on here.

I have found another small bug and a dev_dbg format weakness
and now it seems to work well even if I remove or reinsert the
battery while read operations are ongoing.

Still I need more time to fix up the patch(es).

BR and thanks,
Nikolaus

Re: [PATCHv3] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Tony Lindgren]

* H. Nikolaus Schaller <hns@goldelico.com> [200509 11:48]:
> I have found another small bug and a dev_dbg format weakness
> and now it seems to work well even if I remove or reinsert the
> battery while read operations are ongoing.

OK great.

> Still I need more time to fix up the patch(es).

Well it's ready when it's ready :)

Tony