[PATCH v3 0/3] bus: sunxi-rsb: Fix poweroff issues

[PATCH v3 0/3] bus: sunxi-rsb: Fix poweroff issues

[Samuel Holland]

This series fixes a couple of issues that occur when powering off a
board using a PMIC attached to the RSB bus.

These issues only affected 32-bit platforms, because 64-bit platforms
use PSCI for pm_power_off(), and the PSCI firmware reinitializes the
RSB controller.

Changes in v3:
 - Adjust patch 1 commit message
 - Clear the interrupt status register after polling
 - Add a patch refactoring how the status bits are cleared

Changes in v2:
 - Add Fixes tag to patch 2
 - Only check for specific status bits when polling

Samuel Holland (3):
  bus: sunxi-rsb: Remove the shutdown callback
  bus: sunxi-rsb: Support atomic transfers
  bus: sunxi-rsb: Clear interrupt status before each transfer

 drivers/bus/sunxi-rsb.c | 56 ++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

-- 
2.37.3

[PATCH v3 0/3] bus: sunxi-rsb: Fix poweroff issues

[Samuel Holland]

This series fixes a couple of issues that occur when powering off a
board using a PMIC attached to the RSB bus.

These issues only affected 32-bit platforms, because 64-bit platforms
use PSCI for pm_power_off(), and the PSCI firmware reinitializes the
RSB controller.

Changes in v3:
 - Adjust patch 1 commit message
 - Clear the interrupt status register after polling
 - Add a patch refactoring how the status bits are cleared

Changes in v2:
 - Add Fixes tag to patch 2
 - Only check for specific status bits when polling

Samuel Holland (3):
  bus: sunxi-rsb: Remove the shutdown callback
  bus: sunxi-rsb: Support atomic transfers
  bus: sunxi-rsb: Clear interrupt status before each transfer

 drivers/bus/sunxi-rsb.c | 56 ++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

-- 
2.37.3


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

[PATCH v3 1/3] bus: sunxi-rsb: Remove the shutdown callback

[Samuel Holland]

Shutting down the RSB controller prevents communicating with a PMIC
inside pm_power_off(), since that gets called after device_shutdown(),
so it breaks system poweroff on some boards.

Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Fixes: 843107498f91 ("bus: sunxi-rsb: Implement suspend/resume/shutdown callbacks")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Adjust patch 1 commit message

 drivers/bus/sunxi-rsb.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 4cd2e127946e..17343cd75338 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -812,14 +812,6 @@ static int sunxi_rsb_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static void sunxi_rsb_shutdown(struct platform_device *pdev)
-{
-	struct sunxi_rsb *rsb = platform_get_drvdata(pdev);
-
-	pm_runtime_disable(&pdev->dev);
-	sunxi_rsb_hw_exit(rsb);
-}
-
 static const struct dev_pm_ops sunxi_rsb_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(sunxi_rsb_runtime_suspend,
 			   sunxi_rsb_runtime_resume, NULL)
@@ -835,7 +827,6 @@ MODULE_DEVICE_TABLE(of, sunxi_rsb_of_match_table);
 static struct platform_driver sunxi_rsb_driver = {
 	.probe = sunxi_rsb_probe,
 	.remove	= sunxi_rsb_remove,
-	.shutdown = sunxi_rsb_shutdown,
 	.driver	= {
 		.name = RSB_CTRL_NAME,
 		.of_match_table = sunxi_rsb_of_match_table,
-- 
2.37.3

[PATCH v3 1/3] bus: sunxi-rsb: Remove the shutdown callback

[Samuel Holland]

Shutting down the RSB controller prevents communicating with a PMIC
inside pm_power_off(), since that gets called after device_shutdown(),
so it breaks system poweroff on some boards.

Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Fixes: 843107498f91 ("bus: sunxi-rsb: Implement suspend/resume/shutdown callbacks")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Adjust patch 1 commit message

 drivers/bus/sunxi-rsb.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 4cd2e127946e..17343cd75338 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -812,14 +812,6 @@ static int sunxi_rsb_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static void sunxi_rsb_shutdown(struct platform_device *pdev)
-{
-	struct sunxi_rsb *rsb = platform_get_drvdata(pdev);
-
-	pm_runtime_disable(&pdev->dev);
-	sunxi_rsb_hw_exit(rsb);
-}
-
 static const struct dev_pm_ops sunxi_rsb_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(sunxi_rsb_runtime_suspend,
 			   sunxi_rsb_runtime_resume, NULL)
@@ -835,7 +827,6 @@ MODULE_DEVICE_TABLE(of, sunxi_rsb_of_match_table);
 static struct platform_driver sunxi_rsb_driver = {
 	.probe = sunxi_rsb_probe,
 	.remove	= sunxi_rsb_remove,
-	.shutdown = sunxi_rsb_shutdown,
 	.driver	= {
 		.name = RSB_CTRL_NAME,
 		.of_match_table = sunxi_rsb_of_match_table,
-- 
2.37.3


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

[PATCH v3 2/3] bus: sunxi-rsb: Support atomic transfers

[Samuel Holland]

When communicating with a PMIC during system poweroff (pm_power_off()),
IRQs are disabled and we are in a RCU read-side critical section, so we
cannot use wait_for_completion_io_timeout(). Instead, poll the status
register for transfer completion.

Fixes: d787dcdb9c8f ("bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Clear the interrupt status register after polling

Changes in v2:
 - Add Fixes tag to patch 2
 - Only check for specific status bits when polling

 drivers/bus/sunxi-rsb.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 17343cd75338..3aa91aed3bf7 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -267,6 +267,9 @@ EXPORT_SYMBOL_GPL(sunxi_rsb_driver_register);
 /* common code that starts a transfer */
 static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 {
+	u32 int_mask, status;
+	bool timeout;
+
 	if (readl(rsb->regs + RSB_CTRL) & RSB_CTRL_START_TRANS) {
 		dev_dbg(rsb->dev, "RSB transfer still in progress\n");
 		return -EBUSY;
@@ -274,13 +277,23 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 
 	reinit_completion(&rsb->complete);
 
-	writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER,
-	       rsb->regs + RSB_INTE);
+	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER;
+	writel(int_mask, rsb->regs + RSB_INTE);
 	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
 	       rsb->regs + RSB_CTRL);
 
-	if (!wait_for_completion_io_timeout(&rsb->complete,
-					    msecs_to_jiffies(100))) {
+	if (irqs_disabled()) {
+		timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS,
+						    status, (status & int_mask),
+						    10, 100000);
+		writel(status, rsb->regs + RSB_INTS);
+	} else {
+		timeout = !wait_for_completion_io_timeout(&rsb->complete,
+							  msecs_to_jiffies(100));
+		status = rsb->status;
+	}
+
+	if (timeout) {
 		dev_dbg(rsb->dev, "RSB timeout\n");
 
 		/* abort the transfer */
@@ -292,18 +305,18 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 		return -ETIMEDOUT;
 	}
 
-	if (rsb->status & RSB_INTS_LOAD_BSY) {
+	if (status & RSB_INTS_LOAD_BSY) {
 		dev_dbg(rsb->dev, "RSB busy\n");
 		return -EBUSY;
 	}
 
-	if (rsb->status & RSB_INTS_TRANS_ERR) {
-		if (rsb->status & RSB_INTS_TRANS_ERR_ACK) {
+	if (status & RSB_INTS_TRANS_ERR) {
+		if (status & RSB_INTS_TRANS_ERR_ACK) {
 			dev_dbg(rsb->dev, "RSB slave nack\n");
 			return -EINVAL;
 		}
 
-		if (rsb->status & RSB_INTS_TRANS_ERR_DATA) {
+		if (status & RSB_INTS_TRANS_ERR_DATA) {
 			dev_dbg(rsb->dev, "RSB transfer data error\n");
 			return -EIO;
 		}
-- 
2.37.3

[PATCH v3 2/3] bus: sunxi-rsb: Support atomic transfers

[Samuel Holland]

When communicating with a PMIC during system poweroff (pm_power_off()),
IRQs are disabled and we are in a RCU read-side critical section, so we
cannot use wait_for_completion_io_timeout(). Instead, poll the status
register for transfer completion.

Fixes: d787dcdb9c8f ("bus: sunxi-rsb: Add driver for Allwinner Reduced Serial Bus")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Clear the interrupt status register after polling

Changes in v2:
 - Add Fixes tag to patch 2
 - Only check for specific status bits when polling

 drivers/bus/sunxi-rsb.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 17343cd75338..3aa91aed3bf7 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -267,6 +267,9 @@ EXPORT_SYMBOL_GPL(sunxi_rsb_driver_register);
 /* common code that starts a transfer */
 static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 {
+	u32 int_mask, status;
+	bool timeout;
+
 	if (readl(rsb->regs + RSB_CTRL) & RSB_CTRL_START_TRANS) {
 		dev_dbg(rsb->dev, "RSB transfer still in progress\n");
 		return -EBUSY;
@@ -274,13 +277,23 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 
 	reinit_completion(&rsb->complete);
 
-	writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER,
-	       rsb->regs + RSB_INTE);
+	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER;
+	writel(int_mask, rsb->regs + RSB_INTE);
 	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
 	       rsb->regs + RSB_CTRL);
 
-	if (!wait_for_completion_io_timeout(&rsb->complete,
-					    msecs_to_jiffies(100))) {
+	if (irqs_disabled()) {
+		timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS,
+						    status, (status & int_mask),
+						    10, 100000);
+		writel(status, rsb->regs + RSB_INTS);
+	} else {
+		timeout = !wait_for_completion_io_timeout(&rsb->complete,
+							  msecs_to_jiffies(100));
+		status = rsb->status;
+	}
+
+	if (timeout) {
 		dev_dbg(rsb->dev, "RSB timeout\n");
 
 		/* abort the transfer */
@@ -292,18 +305,18 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 		return -ETIMEDOUT;
 	}
 
-	if (rsb->status & RSB_INTS_LOAD_BSY) {
+	if (status & RSB_INTS_LOAD_BSY) {
 		dev_dbg(rsb->dev, "RSB busy\n");
 		return -EBUSY;
 	}
 
-	if (rsb->status & RSB_INTS_TRANS_ERR) {
-		if (rsb->status & RSB_INTS_TRANS_ERR_ACK) {
+	if (status & RSB_INTS_TRANS_ERR) {
+		if (status & RSB_INTS_TRANS_ERR_ACK) {
 			dev_dbg(rsb->dev, "RSB slave nack\n");
 			return -EINVAL;
 		}
 
-		if (rsb->status & RSB_INTS_TRANS_ERR_DATA) {
+		if (status & RSB_INTS_TRANS_ERR_DATA) {
 			dev_dbg(rsb->dev, "RSB transfer data error\n");
 			return -EIO;
 		}
-- 
2.37.3


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

[PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Samuel Holland]

Currently, the driver clears the interrupt status bits after anything
could have set them. However, this requires duplicating the same logic
in several places.

Instead of clearing the status flags in the interrupt handler, disable
all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.
Then we can delay the status register write until the start of the next
transfer, so it only has to be done in one place.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Add a patch refactoring how the status bits are cleared

 drivers/bus/sunxi-rsb.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 3aa91aed3bf7..cb622e60897b 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 
 	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER;
 	writel(int_mask, rsb->regs + RSB_INTE);
+	writel(int_mask, rsb->regs + RSB_INTS);
 	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
 	       rsb->regs + RSB_CTRL);
 
@@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 		timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS,
 						    status, (status & int_mask),
 						    10, 100000);
-		writel(status, rsb->regs + RSB_INTS);
 	} else {
 		timeout = !wait_for_completion_io_timeout(&rsb->complete,
 							  msecs_to_jiffies(100));
@@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 	if (timeout) {
 		dev_dbg(rsb->dev, "RSB timeout\n");
 
-		/* abort the transfer */
+		/* abort the transfer and disable interrupts */
 		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
 
-		/* clear any interrupt flags */
-		writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
-
 		return -ETIMEDOUT;
 	}
 
@@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
 static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
 {
 	struct sunxi_rsb *rsb = dev_id;
-	u32 status;
 
-	status = readl(rsb->regs + RSB_INTS);
-	rsb->status = status;
+	/* disable interrupts */
+	writel(0, rsb->regs + RSB_CTRL);
 
-	/* Clear interrupts */
-	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
-		   RSB_INTS_TRANS_OVER);
-	writel(status, rsb->regs + RSB_INTS);
+	rsb->status = readl(rsb->regs + RSB_INTS);
 
 	complete(&rsb->complete);
 
@@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb *rsb)
 	if (reg & RSB_DMCR_DEVICE_START)
 		ret = -ETIMEDOUT;
 
-	/* clear interrupt status bits */
-	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
-
 	return ret;
 }
 
-- 
2.37.3

[PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Samuel Holland]

Currently, the driver clears the interrupt status bits after anything
could have set them. However, this requires duplicating the same logic
in several places.

Instead of clearing the status flags in the interrupt handler, disable
all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.
Then we can delay the status register write until the start of the next
transfer, so it only has to be done in one place.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Add a patch refactoring how the status bits are cleared

 drivers/bus/sunxi-rsb.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 3aa91aed3bf7..cb622e60897b 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 
 	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER;
 	writel(int_mask, rsb->regs + RSB_INTE);
+	writel(int_mask, rsb->regs + RSB_INTS);
 	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
 	       rsb->regs + RSB_CTRL);
 
@@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 		timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS,
 						    status, (status & int_mask),
 						    10, 100000);
-		writel(status, rsb->regs + RSB_INTS);
 	} else {
 		timeout = !wait_for_completion_io_timeout(&rsb->complete,
 							  msecs_to_jiffies(100));
@@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
 	if (timeout) {
 		dev_dbg(rsb->dev, "RSB timeout\n");
 
-		/* abort the transfer */
+		/* abort the transfer and disable interrupts */
 		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
 
-		/* clear any interrupt flags */
-		writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
-
 		return -ETIMEDOUT;
 	}
 
@@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
 static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
 {
 	struct sunxi_rsb *rsb = dev_id;
-	u32 status;
 
-	status = readl(rsb->regs + RSB_INTS);
-	rsb->status = status;
+	/* disable interrupts */
+	writel(0, rsb->regs + RSB_CTRL);
 
-	/* Clear interrupts */
-	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
-		   RSB_INTS_TRANS_OVER);
-	writel(status, rsb->regs + RSB_INTS);
+	rsb->status = readl(rsb->regs + RSB_INTS);
 
 	complete(&rsb->complete);
 
@@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb *rsb)
 	if (reg & RSB_DMCR_DEVICE_START)
 		ret = -ETIMEDOUT;
 
-	/* clear interrupt status bits */
-	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
-
 	return ret;
 }
 
-- 
2.37.3


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

Re: [PATCH v3 2/3] bus: sunxi-rsb: Support atomic transfers

[Jernej Škrabec]

Dne ponedeljek, 14. november 2022 ob 02:57:48 CET je Samuel Holland 
napisal(a):
> When communicating with a PMIC during system poweroff (pm_power_off()),
> IRQs are disabled and we are in a RCU read-side critical section, so we
> cannot use wait_for_completion_io_timeout(). Instead, poll the status
> register for transfer completion.
> 
> Fixes: d787dcdb9c8f ("bus: sunxi-rsb: Add driver for Allwinner Reduced
> Serial Bus") Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

Re: [PATCH v3 2/3] bus: sunxi-rsb: Support atomic transfers

[Jernej Škrabec]

Dne ponedeljek, 14. november 2022 ob 02:57:48 CET je Samuel Holland 
napisal(a):
> When communicating with a PMIC during system poweroff (pm_power_off()),
> IRQs are disabled and we are in a RCU read-side critical section, so we
> cannot use wait_for_completion_io_timeout(). Instead, poll the status
> register for transfer completion.
> 
> Fixes: d787dcdb9c8f ("bus: sunxi-rsb: Add driver for Allwinner Reduced
> Serial Bus") Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej



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

Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Jernej Škrabec]

Hi Samuel,

Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland 
napisal(a):
> Currently, the driver clears the interrupt status bits after anything
> could have set them. However, this requires duplicating the same logic
> in several places.
> 
> Instead of clearing the status flags in the interrupt handler, disable
> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.

where is this bit cleared?

> Then we can delay the status register write until the start of the next
> transfer, so it only has to be done in one place.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Add a patch refactoring how the status bits are cleared
> 
>  drivers/bus/sunxi-rsb.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 3aa91aed3bf7..cb622e60897b 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
> 
>  	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | 
RSB_INTS_TRANS_OVER;
>  	writel(int_mask, rsb->regs + RSB_INTE);
> +	writel(int_mask, rsb->regs + RSB_INTS);

Wouldn't be better to clear status before enabling interrupts? Unless global 
interrupt flag is cleared beforehand, but I don't see that anywhere.

Best regards,
Jernej

>  	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
>  	       rsb->regs + RSB_CTRL);
> 
> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>  		timeout = readl_poll_timeout_atomic(rsb->regs + 
RSB_INTS,
>  						    status, 
(status & int_mask),
>  						    10, 
100000);
> -		writel(status, rsb->regs + RSB_INTS);
>  	} else {
>  		timeout = !wait_for_completion_io_timeout(&rsb-
>complete,
>  							  
msecs_to_jiffies(100));
> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>  	if (timeout) {
>  		dev_dbg(rsb->dev, "RSB timeout\n");
> 
> -		/* abort the transfer */
> +		/* abort the transfer and disable interrupts */
>  		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
> 
> -		/* clear any interrupt flags */
> -		writel(readl(rsb->regs + RSB_INTS), rsb->regs + 
RSB_INTS);
> -
>  		return -ETIMEDOUT;
>  	}
> 
> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
>  static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
>  {
>  	struct sunxi_rsb *rsb = dev_id;
> -	u32 status;
> 
> -	status = readl(rsb->regs + RSB_INTS);
> -	rsb->status = status;
> +	/* disable interrupts */
> +	writel(0, rsb->regs + RSB_CTRL);
> 
> -	/* Clear interrupts */
> -	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
> -		   RSB_INTS_TRANS_OVER);
> -	writel(status, rsb->regs + RSB_INTS);
> +	rsb->status = readl(rsb->regs + RSB_INTS);
> 
>  	complete(&rsb->complete);
> 
> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb
> *rsb) if (reg & RSB_DMCR_DEVICE_START)
>  		ret = -ETIMEDOUT;
> 
> -	/* clear interrupt status bits */
> -	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
> -
>  	return ret;
>  }

Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Jernej Škrabec]

Hi Samuel,

Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland 
napisal(a):
> Currently, the driver clears the interrupt status bits after anything
> could have set them. However, this requires duplicating the same logic
> in several places.
> 
> Instead of clearing the status flags in the interrupt handler, disable
> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.

where is this bit cleared?

> Then we can delay the status register write until the start of the next
> transfer, so it only has to be done in one place.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Add a patch refactoring how the status bits are cleared
> 
>  drivers/bus/sunxi-rsb.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 3aa91aed3bf7..cb622e60897b 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
> 
>  	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | 
RSB_INTS_TRANS_OVER;
>  	writel(int_mask, rsb->regs + RSB_INTE);
> +	writel(int_mask, rsb->regs + RSB_INTS);

Wouldn't be better to clear status before enabling interrupts? Unless global 
interrupt flag is cleared beforehand, but I don't see that anywhere.

Best regards,
Jernej

>  	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
>  	       rsb->regs + RSB_CTRL);
> 
> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>  		timeout = readl_poll_timeout_atomic(rsb->regs + 
RSB_INTS,
>  						    status, 
(status & int_mask),
>  						    10, 
100000);
> -		writel(status, rsb->regs + RSB_INTS);
>  	} else {
>  		timeout = !wait_for_completion_io_timeout(&rsb-
>complete,
>  							  
msecs_to_jiffies(100));
> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>  	if (timeout) {
>  		dev_dbg(rsb->dev, "RSB timeout\n");
> 
> -		/* abort the transfer */
> +		/* abort the transfer and disable interrupts */
>  		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
> 
> -		/* clear any interrupt flags */
> -		writel(readl(rsb->regs + RSB_INTS), rsb->regs + 
RSB_INTS);
> -
>  		return -ETIMEDOUT;
>  	}
> 
> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
>  static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
>  {
>  	struct sunxi_rsb *rsb = dev_id;
> -	u32 status;
> 
> -	status = readl(rsb->regs + RSB_INTS);
> -	rsb->status = status;
> +	/* disable interrupts */
> +	writel(0, rsb->regs + RSB_CTRL);
> 
> -	/* Clear interrupts */
> -	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
> -		   RSB_INTS_TRANS_OVER);
> -	writel(status, rsb->regs + RSB_INTS);
> +	rsb->status = readl(rsb->regs + RSB_INTS);
> 
>  	complete(&rsb->complete);
> 
> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb
> *rsb) if (reg & RSB_DMCR_DEVICE_START)
>  		ret = -ETIMEDOUT;
> 
> -	/* clear interrupt status bits */
> -	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
> -
>  	return ret;
>  }





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

Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Samuel Holland]

On 11/14/22 15:00, Jernej Škrabec wrote:
> Hi Samuel,
> 
> Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland 
> napisal(a):
>> Currently, the driver clears the interrupt status bits after anything
>> could have set them. However, this requires duplicating the same logic
>> in several places.
>>
>> Instead of clearing the status flags in the interrupt handler, disable
>> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.
> 
> where is this bit cleared?

It is cleared by any write to RSB_CTRL that does not include it. I noted
it below with the "disable interrupts" comments.

>> Then we can delay the status register write until the start of the next
>> transfer, so it only has to be done in one place.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Changes in v3:
>>  - Add a patch refactoring how the status bits are cleared
>>
>>  drivers/bus/sunxi-rsb.c | 20 +++++---------------
>>  1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
>> index 3aa91aed3bf7..cb622e60897b 100644
>> --- a/drivers/bus/sunxi-rsb.c
>> +++ b/drivers/bus/sunxi-rsb.c
>> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>>
>>  	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER;
>>  	writel(int_mask, rsb->regs + RSB_INTE);
>> +	writel(int_mask, rsb->regs + RSB_INTS);
> 
> Wouldn't be better to clear status before enabling interrupts? Unless global 
> interrupt flag is cleared beforehand, but I don't see that anywhere.

Indeed the intention was that the global interrupt flag is cleared
beforehand, and only enabled on the next line below. However, I realize
I missed disabling it for the new atomic case.

I'm not so sure anymore that this patch is an improvement. What do you
think? I can send a v4 with a fix, or I am fine with skipping this
patch. I would at least like the other two to be merged for -fixes.

Regards,
Samuel

>>  	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
>>  	       rsb->regs + RSB_CTRL);
>>
>> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>>  		timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS,
>>  						    status, (status & int_mask),
>>  						    10, 100000);
>> -		writel(status, rsb->regs + RSB_INTS);
>>  	} else {
>>  		timeout = !wait_for_completion_io_timeout(&rsb-
>> complete,
>>  							  msecs_to_jiffies(100));
>> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>>  	if (timeout) {
>>  		dev_dbg(rsb->dev, "RSB timeout\n");
>>
>> -		/* abort the transfer */
>> +		/* abort the transfer and disable interrupts */
>>  		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
>>
>> -		/* clear any interrupt flags */
>> -		writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
>> -
>>  		return -ETIMEDOUT;
>>  	}
>>
>> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
>>  static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
>>  {
>>  	struct sunxi_rsb *rsb = dev_id;
>> -	u32 status;
>>
>> -	status = readl(rsb->regs + RSB_INTS);
>> -	rsb->status = status;
>> +	/* disable interrupts */
>> +	writel(0, rsb->regs + RSB_CTRL);
>>
>> -	/* Clear interrupts */
>> -	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
>> -		   RSB_INTS_TRANS_OVER);
>> -	writel(status, rsb->regs + RSB_INTS);
>> +	rsb->status = readl(rsb->regs + RSB_INTS);
>>
>>  	complete(&rsb->complete);
>>
>> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb
>> *rsb) if (reg & RSB_DMCR_DEVICE_START)
>>  		ret = -ETIMEDOUT;
>>
>> -	/* clear interrupt status bits */
>> -	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
>> -
>>  	return ret;
>>  }

Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Samuel Holland]

On 11/14/22 15:00, Jernej Škrabec wrote:
> Hi Samuel,
> 
> Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland 
> napisal(a):
>> Currently, the driver clears the interrupt status bits after anything
>> could have set them. However, this requires duplicating the same logic
>> in several places.
>>
>> Instead of clearing the status flags in the interrupt handler, disable
>> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.
> 
> where is this bit cleared?

It is cleared by any write to RSB_CTRL that does not include it. I noted
it below with the "disable interrupts" comments.

>> Then we can delay the status register write until the start of the next
>> transfer, so it only has to be done in one place.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Changes in v3:
>>  - Add a patch refactoring how the status bits are cleared
>>
>>  drivers/bus/sunxi-rsb.c | 20 +++++---------------
>>  1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
>> index 3aa91aed3bf7..cb622e60897b 100644
>> --- a/drivers/bus/sunxi-rsb.c
>> +++ b/drivers/bus/sunxi-rsb.c
>> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>>
>>  	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER;
>>  	writel(int_mask, rsb->regs + RSB_INTE);
>> +	writel(int_mask, rsb->regs + RSB_INTS);
> 
> Wouldn't be better to clear status before enabling interrupts? Unless global 
> interrupt flag is cleared beforehand, but I don't see that anywhere.

Indeed the intention was that the global interrupt flag is cleared
beforehand, and only enabled on the next line below. However, I realize
I missed disabling it for the new atomic case.

I'm not so sure anymore that this patch is an improvement. What do you
think? I can send a v4 with a fix, or I am fine with skipping this
patch. I would at least like the other two to be merged for -fixes.

Regards,
Samuel

>>  	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
>>  	       rsb->regs + RSB_CTRL);
>>
>> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>>  		timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS,
>>  						    status, (status & int_mask),
>>  						    10, 100000);
>> -		writel(status, rsb->regs + RSB_INTS);
>>  	} else {
>>  		timeout = !wait_for_completion_io_timeout(&rsb-
>> complete,
>>  							  msecs_to_jiffies(100));
>> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
>>  	if (timeout) {
>>  		dev_dbg(rsb->dev, "RSB timeout\n");
>>
>> -		/* abort the transfer */
>> +		/* abort the transfer and disable interrupts */
>>  		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
>>
>> -		/* clear any interrupt flags */
>> -		writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
>> -
>>  		return -ETIMEDOUT;
>>  	}
>>
>> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
>>  static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
>>  {
>>  	struct sunxi_rsb *rsb = dev_id;
>> -	u32 status;
>>
>> -	status = readl(rsb->regs + RSB_INTS);
>> -	rsb->status = status;
>> +	/* disable interrupts */
>> +	writel(0, rsb->regs + RSB_CTRL);
>>
>> -	/* Clear interrupts */
>> -	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
>> -		   RSB_INTS_TRANS_OVER);
>> -	writel(status, rsb->regs + RSB_INTS);
>> +	rsb->status = readl(rsb->regs + RSB_INTS);
>>
>>  	complete(&rsb->complete);
>>
>> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb
>> *rsb) if (reg & RSB_DMCR_DEVICE_START)
>>  		ret = -ETIMEDOUT;
>>
>> -	/* clear interrupt status bits */
>> -	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
>> -
>>  	return ret;
>>  }


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

Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Jernej Škrabec]

Dne torek, 15. november 2022 ob 07:08:12 CET je Samuel Holland napisal(a):
> On 11/14/22 15:00, Jernej Škrabec wrote:
> > Hi Samuel,
> > 
> > Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland
> > 
> > napisal(a):
> >> Currently, the driver clears the interrupt status bits after anything
> >> could have set them. However, this requires duplicating the same logic
> >> in several places.
> >> 
> >> Instead of clearing the status flags in the interrupt handler, disable
> >> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.
> > 
> > where is this bit cleared?
> 
> It is cleared by any write to RSB_CTRL that does not include it. I noted
> it below with the "disable interrupts" comments.

Right.

> 
> >> Then we can delay the status register write until the start of the next
> >> transfer, so it only has to be done in one place.
> >> 
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >> 
> >> Changes in v3:
> >>  - Add a patch refactoring how the status bits are cleared
> >>  
> >>  drivers/bus/sunxi-rsb.c | 20 +++++---------------
> >>  1 file changed, 5 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> >> index 3aa91aed3bf7..cb622e60897b 100644
> >> --- a/drivers/bus/sunxi-rsb.c
> >> +++ b/drivers/bus/sunxi-rsb.c
> >> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
> >> 
> >>  	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
> >>  	RSB_INTS_TRANS_OVER;
> >>  	writel(int_mask, rsb->regs + RSB_INTE);
> >> 
> >> +	writel(int_mask, rsb->regs + RSB_INTS);
> > 
> > Wouldn't be better to clear status before enabling interrupts? Unless
> > global interrupt flag is cleared beforehand, but I don't see that
> > anywhere.
> Indeed the intention was that the global interrupt flag is cleared
> beforehand, and only enabled on the next line below. However, I realize
> I missed disabling it for the new atomic case.
> 
> I'm not so sure anymore that this patch is an improvement. What do you
> think? I can send a v4 with a fix, or I am fine with skipping this
> patch. I would at least like the other two to be merged for -fixes.

Sure, first two patches will go in regardless. I'm not convinced of value of 
this patch either. I guess we can skip it.

Best regards,
Jernej

> 
> Regards,
> Samuel
> 
> >>  	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
> >>  	
> >>  	       rsb->regs + RSB_CTRL);
> >> 
> >> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
> >> 
> >>  		timeout = readl_poll_timeout_atomic(rsb->regs + 
RSB_INTS,
> >>  		
> >>  						    
status, (status & int_mask),
> >>  						    10, 
100000);
> >> 
> >> -		writel(status, rsb->regs + RSB_INTS);
> >> 
> >>  	} else {
> >>  	
> >>  		timeout = !wait_for_completion_io_timeout(&rsb-
> >> 
> >> complete,
> >> 
> >>  							
  msecs_to_jiffies(100));
> >> 
> >> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb
> >> *rsb)
> >> 
> >>  	if (timeout) {
> >>  	
> >>  		dev_dbg(rsb->dev, "RSB timeout\n");
> >> 
> >> -		/* abort the transfer */
> >> +		/* abort the transfer and disable interrupts */
> >> 
> >>  		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
> >> 
> >> -		/* clear any interrupt flags */
> >> -		writel(readl(rsb->regs + RSB_INTS), rsb->regs + 
RSB_INTS);
> >> -
> >> 
> >>  		return -ETIMEDOUT;
> >>  	
> >>  	}
> >> 
> >> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
> >> 
> >>  static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
> >>  {
> >>  
> >>  	struct sunxi_rsb *rsb = dev_id;
> >> 
> >> -	u32 status;
> >> 
> >> -	status = readl(rsb->regs + RSB_INTS);
> >> -	rsb->status = status;
> >> +	/* disable interrupts */
> >> +	writel(0, rsb->regs + RSB_CTRL);
> >> 
> >> -	/* Clear interrupts */
> >> -	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
> >> -		   RSB_INTS_TRANS_OVER);
> >> -	writel(status, rsb->regs + RSB_INTS);
> >> +	rsb->status = readl(rsb->regs + RSB_INTS);
> >> 
> >>  	complete(&rsb->complete);
> >> 
> >> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct
> >> sunxi_rsb
> >> *rsb) if (reg & RSB_DMCR_DEVICE_START)
> >> 
> >>  		ret = -ETIMEDOUT;
> >> 
> >> -	/* clear interrupt status bits */
> >> -	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
> >> -
> >> 
> >>  	return ret;
> >>  
> >>  }

Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer

[Jernej Škrabec]

Dne torek, 15. november 2022 ob 07:08:12 CET je Samuel Holland napisal(a):
> On 11/14/22 15:00, Jernej Škrabec wrote:
> > Hi Samuel,
> > 
> > Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland
> > 
> > napisal(a):
> >> Currently, the driver clears the interrupt status bits after anything
> >> could have set them. However, this requires duplicating the same logic
> >> in several places.
> >> 
> >> Instead of clearing the status flags in the interrupt handler, disable
> >> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit.
> > 
> > where is this bit cleared?
> 
> It is cleared by any write to RSB_CTRL that does not include it. I noted
> it below with the "disable interrupts" comments.

Right.

> 
> >> Then we can delay the status register write until the start of the next
> >> transfer, so it only has to be done in one place.
> >> 
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >> 
> >> Changes in v3:
> >>  - Add a patch refactoring how the status bits are cleared
> >>  
> >>  drivers/bus/sunxi-rsb.c | 20 +++++---------------
> >>  1 file changed, 5 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> >> index 3aa91aed3bf7..cb622e60897b 100644
> >> --- a/drivers/bus/sunxi-rsb.c
> >> +++ b/drivers/bus/sunxi-rsb.c
> >> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
> >> 
> >>  	int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
> >>  	RSB_INTS_TRANS_OVER;
> >>  	writel(int_mask, rsb->regs + RSB_INTE);
> >> 
> >> +	writel(int_mask, rsb->regs + RSB_INTS);
> > 
> > Wouldn't be better to clear status before enabling interrupts? Unless
> > global interrupt flag is cleared beforehand, but I don't see that
> > anywhere.
> Indeed the intention was that the global interrupt flag is cleared
> beforehand, and only enabled on the next line below. However, I realize
> I missed disabling it for the new atomic case.
> 
> I'm not so sure anymore that this patch is an improvement. What do you
> think? I can send a v4 with a fix, or I am fine with skipping this
> patch. I would at least like the other two to be merged for -fixes.

Sure, first two patches will go in regardless. I'm not convinced of value of 
this patch either. I guess we can skip it.

Best regards,
Jernej

> 
> Regards,
> Samuel
> 
> >>  	writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB,
> >>  	
> >>  	       rsb->regs + RSB_CTRL);
> >> 
> >> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb)
> >> 
> >>  		timeout = readl_poll_timeout_atomic(rsb->regs + 
RSB_INTS,
> >>  		
> >>  						    
status, (status & int_mask),
> >>  						    10, 
100000);
> >> 
> >> -		writel(status, rsb->regs + RSB_INTS);
> >> 
> >>  	} else {
> >>  	
> >>  		timeout = !wait_for_completion_io_timeout(&rsb-
> >> 
> >> complete,
> >> 
> >>  							
  msecs_to_jiffies(100));
> >> 
> >> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb
> >> *rsb)
> >> 
> >>  	if (timeout) {
> >>  	
> >>  		dev_dbg(rsb->dev, "RSB timeout\n");
> >> 
> >> -		/* abort the transfer */
> >> +		/* abort the transfer and disable interrupts */
> >> 
> >>  		writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL);
> >> 
> >> -		/* clear any interrupt flags */
> >> -		writel(readl(rsb->regs + RSB_INTS), rsb->regs + 
RSB_INTS);
> >> -
> >> 
> >>  		return -ETIMEDOUT;
> >>  	
> >>  	}
> >> 
> >> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb);
> >> 
> >>  static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id)
> >>  {
> >>  
> >>  	struct sunxi_rsb *rsb = dev_id;
> >> 
> >> -	u32 status;
> >> 
> >> -	status = readl(rsb->regs + RSB_INTS);
> >> -	rsb->status = status;
> >> +	/* disable interrupts */
> >> +	writel(0, rsb->regs + RSB_CTRL);
> >> 
> >> -	/* Clear interrupts */
> >> -	status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR |
> >> -		   RSB_INTS_TRANS_OVER);
> >> -	writel(status, rsb->regs + RSB_INTS);
> >> +	rsb->status = readl(rsb->regs + RSB_INTS);
> >> 
> >>  	complete(&rsb->complete);
> >> 
> >> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct
> >> sunxi_rsb
> >> *rsb) if (reg & RSB_DMCR_DEVICE_START)
> >> 
> >>  		ret = -ETIMEDOUT;
> >> 
> >> -	/* clear interrupt status bits */
> >> -	writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS);
> >> -
> >> 
> >>  	return ret;
> >>  
> >>  }





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

Re: [PATCH v3 0/3] bus: sunxi-rsb: Fix poweroff issues

[Jernej Škrabec]

Dne ponedeljek, 14. november 2022 ob 02:57:46 CET je Samuel Holland 
napisal(a):
> This series fixes a couple of issues that occur when powering off a
> board using a PMIC attached to the RSB bus.
> 
> These issues only affected 32-bit platforms, because 64-bit platforms
> use PSCI for pm_power_off(), and the PSCI firmware reinitializes the
> RSB controller.
> 
> Changes in v3:
>  - Adjust patch 1 commit message
>  - Clear the interrupt status register after polling
>  - Add a patch refactoring how the status bits are cleared
> 
> Changes in v2:
>  - Add Fixes tag to patch 2
>  - Only check for specific status bits when polling
> 
> Samuel Holland (3):
>   bus: sunxi-rsb: Remove the shutdown callback
>   bus: sunxi-rsb: Support atomic transfers
>   bus: sunxi-rsb: Clear interrupt status before each transfer

Applied patches 1 and 2.

Best regards,
Jernej

> 
>  drivers/bus/sunxi-rsb.c | 56 ++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> --
> 2.37.3

Re: [PATCH v3 0/3] bus: sunxi-rsb: Fix poweroff issues

[Jernej Škrabec]

Dne ponedeljek, 14. november 2022 ob 02:57:46 CET je Samuel Holland 
napisal(a):
> This series fixes a couple of issues that occur when powering off a
> board using a PMIC attached to the RSB bus.
> 
> These issues only affected 32-bit platforms, because 64-bit platforms
> use PSCI for pm_power_off(), and the PSCI firmware reinitializes the
> RSB controller.
> 
> Changes in v3:
>  - Adjust patch 1 commit message
>  - Clear the interrupt status register after polling
>  - Add a patch refactoring how the status bits are cleared
> 
> Changes in v2:
>  - Add Fixes tag to patch 2
>  - Only check for specific status bits when polling
> 
> Samuel Holland (3):
>   bus: sunxi-rsb: Remove the shutdown callback
>   bus: sunxi-rsb: Support atomic transfers
>   bus: sunxi-rsb: Clear interrupt status before each transfer

Applied patches 1 and 2.

Best regards,
Jernej

> 
>  drivers/bus/sunxi-rsb.c | 56 ++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> --
> 2.37.3



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