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 v2:
- Add Fixes tag to patch 2
- Only check for specific status bits when polling
Samuel Holland (2):
bus: sunxi-rsb: Remove shutdown callback
bus: sunxi-rsb: Support atomic transfers
drivers/bus/sunxi-rsb.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
--
2.37.3
Shutting down the RSB controller prevents communicating with a PMIC
inside pm_power_off(), so it breaks system poweroff on some boards.
Reported-by: Ivaylo Dimitrov <[email protected]>
Tested-by: Ivaylo Dimitrov <[email protected]>
Acked-by: Jernej Skrabec <[email protected]>
Fixes: 843107498f91 ("bus: sunxi-rsb: Implement suspend/resume/shutdown callbacks")
Signed-off-by: Samuel Holland <[email protected]>
---
(no changes since v1)
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
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 <[email protected]>
---
Changes in v2:
- Add Fixes tag to patch 2
- Only check for specific status bits when polling
drivers/bus/sunxi-rsb.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 17343cd75338..012e82f9b7b0 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,
+ 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);
+ } 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
On Sun, 6 Nov 2022 23:22:00 -0600
Samuel Holland <[email protected]> wrote:
Hi,
> 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 <[email protected]>
> ---
>
> Changes in v2:
> - Add Fixes tag to patch 2
> - Only check for specific status bits when polling
>
> drivers/bus/sunxi-rsb.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 17343cd75338..012e82f9b7b0 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,
> + 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);
So if I understand correctly, this mimics the operation of
sunxi_rsb_irq(), just replacing rsb->status with status.
But wouldn't that also mean that we need to clear the interrupt bits in
INTS, since we are about to handle them below?
It probably doesn't matter in practise, since we call this during power
down only, but looks like more robust to do, from a driver's perspective.
Cheers,
Andre
> + } 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;
> }
Dne ponedeljek, 07. november 2022 ob 12:30:29 CET je Andre Przywara
napisal(a):
> On Sun, 6 Nov 2022 23:22:00 -0600
> Samuel Holland <[email protected]> wrote:
>
> Hi,
>
> > 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 <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Add Fixes tag to patch 2
> > - Only check for specific status bits when polling
> >
> > drivers/bus/sunxi-rsb.c | 27 ++++++++++++++++++++-------
> > 1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> > index 17343cd75338..012e82f9b7b0 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,
> > + 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);
>
> So if I understand correctly, this mimics the operation of
> sunxi_rsb_irq(), just replacing rsb->status with status.
> But wouldn't that also mean that we need to clear the interrupt bits in
> INTS, since we are about to handle them below?
Yes, I pointed out that in review of v1.
Best regards,
Jernej
>
> It probably doesn't matter in practise, since we call this during power
> down only, but looks like more robust to do, from a driver's perspective.
>
> Cheers,
> Andre
>
> > + } 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;
> >
> > }