Re: [PATCH v7 3/6] mfd: bd9576: Add IRQ support
Hello Again Lee & All, On Wed, 2021-02-10 at 09:01 +0200, Matti Vaittinen wrote: > On Tue, 2021-02-09 at 15:25 +, Lee Jones wrote: > > On Fri, 22 Jan 2021, Matti Vaittinen wrote: > > + /* BD9573 only supports fatal IRQs which we do not > > > handle */ > > > > Why not? > > Because 'fatal' in the context of this comment means that when this > condition occurs the PMIC will do emergency shut down for power > outputs After having a fresh look at this I see the wisdom in your comment. The HW provides IRQs and if they are listed in DT for BD9573 does not mean we should stop here. Thanks for pointing it out. Best Regards Matti Vaittinen
Re: [PATCH v7 3/6] mfd: bd9576: Add IRQ support
Hello Lee,
I appreciate your thorough reviews :) Thanks.
On Tue, 2021-02-09 at 15:25 +, Lee Jones wrote:
> On Fri, 22 Jan 2021, Matti Vaittinen wrote:
>
> > BD9573 and BD9576 support set of "protection" interrupts for
> > "fatal"
> > issues. Those lead to SOC reset as PMIC shuts the power outputs.
> > Thus
> > there is no relevant IRQ handling for them.
> >
> > Few "detection" interrupts were added to the BD9576 with the idea
> > that
> > SOC could take some recovery-action before error gets
> > unrecoverable.
> >
> > Unfortunately the BD9576 interrupt logic was not re-evaluated. IRQs
> > are not designed to be properly acknowleged - and IRQ line is kept
> > active for whole duration of error condition (in comparison to
> > informing only about state change).
> >
> > For above reason, do not consider missing IRQ as error.
> >
> > Signed-off-by: Matti Vaittinen
> > case ROHM_CHIP_TYPE_BD9573:
> > mfd = bd9573_mfd_cells;
> > cells = ARRAY_SIZE(bd9573_mfd_cells);
> > + /* BD9573 only supports fatal IRQs which we do not
> > handle */
>
> Why not?
Because 'fatal' in the context of this comment means that when this
condition occurs the PMIC will do emergency shut down for power outputs
- which means the processor will not be able to handle the IRQ as it
loses the power. Maybe I'd better clarify the meaning of 'fatal' here.
+ /*
> > +* BD9576 behaves badly. It kepts IRQ asserted for the whole
>
> This is solution is less than pretty.
Um, sorry, What are you referring to?
> > +* duration of detected HW condition (like over temp). This
> > does
>
> "over-temperature"
Right. Thanks :)
> > +* not play nicely under any condition but we can work around
> > it
> > +* except when we have shared IRQs. So we don't require IRQ to
> > be
> > +* populated to help those poor sods who did connect IRQ to
> > shared pin.
>
> No swearing in comments please.
Ok. This is actually a good reminder for me that I can't know how
something sounds like for a reader. (That phrase sounds quite innocent
to me but I've no idea how 'severe' swearing that is for the rest of
the world). I'll clean this up.
> How do you know if an IRQ is shared?
I don't. This is something that board designer does know. And my
thinking here was to allow board designer to omit the IRQ information
from DT if he prefers to not use these IRQs. I just tried to explain
that the driver does not _require_ IRQ information to be populated.
>
> > +* If IRQ information is not given, then we mask all IRQs and
> > do not
> > +* provide IRQ resources to regulator driver - which then just
> > omits
> > +* the notifiers.
> > +*/
>
> This situation doesn't sound totally crazy. Is there no way to
> handle
> 'persistent IRQ' conditions in the kernel?
Actually there is. Even for shared IRQs in this case. I made a mistake
at the beginning when I noticed that not all of these IRQs have mask
bits in the sub-IRQ registers. So I thought that for these IRQs the
device can't be told to revert IRQ back to normal. That would have
meaned that only way to prevent IRQ storm was to disable IRQs from the
processor end. But I was mistaken. All of the IRQs can be masked from
the 'main IRQ' level register. So we can mask the whole set of IRQs
form BD9576 when IRQ triggers - and then we can get the BD9576 to
restore the IRQ line.
So yes - we can make this to somehow work. And more importantly, we
don't completely spoil the shared IRQs. Still, the IRQ handling for
BD9576 is ... how to put it ... hacky. And I think few of the setups
might not actually have use for the notifications - so making IRQs
optional just sounded like the best course of action (to me).
+ } else {
> > + ret = regmap_update_bits(regmap,
> > BD957X_REG_INT_MAIN_MASK,
> > +BD957X_MASK_INT_ALL,
> > +BD957X_MASK_INT_ALL);
>
> What's the default state of the interrupts? Unmasked?
I've learned that I'd rather not assume the default state with ROHM
ICs. I've seen all kinds of defaults changing between IC revisions. And
occasionally I've seen same IC versions having different set of
defaults depending on the OTP version. I guess this comes from
traditional operation model where ICs have been tailored to meet needs
of the different customers.
> > diff --git a/include/linux/mfd/rohm-bd957x.h
> > b/include/linux/mfd/rohm-bd957x.h
> > index 3e7ca6fe5d4f..4fa632d8467a 100644
> > --- a/include/linux/mfd/rohm-bd957x.h
> > +++ b/include/linux/mfd/rohm-bd957x.h
> > @@ -13,47 +13,109 @@ enum {
> > BD957X_VOUTS1,
> > };
> >
> > +/* The BD9576 has own IRQ 'blocks' for:
>
> Comments start on line 2.
Do you mean I should move this comment block top of the file? The idea
of this comment is to clarify the IRQs in the hardware. Hence I placed
it in the section where IRQ definitions dwell.
> > + * I2C/THERMAL,
>
> Does this precede each line?
You
Re: [PATCH v7 3/6] mfd: bd9576: Add IRQ support
On Fri, 22 Jan 2021, Matti Vaittinen wrote:
> BD9573 and BD9576 support set of "protection" interrupts for "fatal"
> issues. Those lead to SOC reset as PMIC shuts the power outputs. Thus
> there is no relevant IRQ handling for them.
>
> Few "detection" interrupts were added to the BD9576 with the idea that
> SOC could take some recovery-action before error gets unrecoverable.
>
> Unfortunately the BD9576 interrupt logic was not re-evaluated. IRQs
> are not designed to be properly acknowleged - and IRQ line is kept
> active for whole duration of error condition (in comparison to
> informing only about state change).
>
> For above reason, do not consider missing IRQ as error.
>
> Signed-off-by: Matti Vaittinen
> ---
> Changes since v6:
> - new patch
>
> drivers/mfd/rohm-bd9576.c | 80 +-
> include/linux/mfd/rohm-bd957x.h | 140 +++-
> 2 files changed, 180 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/mfd/rohm-bd9576.c b/drivers/mfd/rohm-bd9576.c
> index f4dd9e438427..99fa0f333277 100644
> --- a/drivers/mfd/rohm-bd9576.c
> +++ b/drivers/mfd/rohm-bd9576.c
> @@ -16,12 +16,27 @@
> #include
> #include
>
> +/*
> + * Due to the BD9576MUF nasty IRQ behaiour we don't always populate IRQs.
> + * These will be added to regulator resources only if IRQ information for the
> + * PMIC is populated in device-tree.
> + */
> +static const struct resource bd9576_regulator_irqs[] = {
> + DEFINE_RES_IRQ_NAMED(BD9576_INT_THERM, "bd9576-temp"),
> + DEFINE_RES_IRQ_NAMED(BD9576_INT_OVD, "bd9576-ovd"),
> + DEFINE_RES_IRQ_NAMED(BD9576_INT_UVD, "bd9576-uvd"),
> +};
> +
> static struct mfd_cell bd9573_mfd_cells[] = {
> { .name = "bd9573-pmic", },
> { .name = "bd9576-wdt", },
> };
>
> static struct mfd_cell bd9576_mfd_cells[] = {
> + /*
> + * Please keep regulators as first cell as resources may be overwritten
> + * from probe and the code expects regulators to be at index 0.
> + */
> { .name = "bd9576-pmic", },
> { .name = "bd9576-wdt", },
> };
> @@ -48,6 +63,29 @@ static struct regmap_config bd957x_regmap = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> +static struct regmap_irq bd9576_irqs[] = {
> + REGMAP_IRQ_REG(BD9576_INT_THERM, 0, BD957X_MASK_INT_MAIN_THERM),
> + REGMAP_IRQ_REG(BD9576_INT_OVP, 0, BD957X_MASK_INT_MAIN_OVP),
> + REGMAP_IRQ_REG(BD9576_INT_SCP, 0, BD957X_MASK_INT_MAIN_SCP),
> + REGMAP_IRQ_REG(BD9576_INT_OCP, 0, BD957X_MASK_INT_MAIN_OCP),
> + REGMAP_IRQ_REG(BD9576_INT_OVD, 0, BD957X_MASK_INT_MAIN_OVD),
> + REGMAP_IRQ_REG(BD9576_INT_UVD, 0, BD957X_MASK_INT_MAIN_UVD),
> + REGMAP_IRQ_REG(BD9576_INT_UVP, 0, BD957X_MASK_INT_MAIN_UVP),
> + REGMAP_IRQ_REG(BD9576_INT_SYS, 0, BD957X_MASK_INT_MAIN_SYS),
> +};
> +
> +static struct regmap_irq_chip bd9576_irq_chip = {
> + .name = "bd9576_irq",
> + .irqs = &bd9576_irqs[0],
> + .num_irqs = ARRAY_SIZE(bd9576_irqs),
> + .status_base = BD957X_REG_INT_MAIN_STAT,
> + .mask_base = BD957X_REG_INT_MAIN_MASK,
> + .ack_base = BD957X_REG_INT_MAIN_STAT,
> + .init_ack_masked = true,
> + .num_regs = 1,
> + .irq_reg_stride = 1,
> +};
> +
> static int bd957x_i2c_probe(struct i2c_client *i2c,
>const struct i2c_device_id *id)
> {
> @@ -56,6 +94,7 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
> struct mfd_cell *mfd;
> int cells;
> unsigned long chip_type;
> + struct irq_domain *domain;
>
> chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
>
> @@ -67,6 +106,11 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
> case ROHM_CHIP_TYPE_BD9573:
> mfd = bd9573_mfd_cells;
> cells = ARRAY_SIZE(bd9573_mfd_cells);
> + /* BD9573 only supports fatal IRQs which we do not handle */
Why not?
> + if (i2c->irq) {
> + dev_err(&i2c->dev, "no supported irqs on BD9573\n");
"IRQs"
> + return -EINVAL;
> + }
> break;
> default:
> dev_err(&i2c->dev, "Unknown device type");
> @@ -78,9 +122,43 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
> dev_err(&i2c->dev, "Failed to initialize Regmap\n");
> return PTR_ERR(regmap);
> }
> + /*
> + * BD9576 behaves badly. It kepts IRQ asserted for the whole
This is solution is less than pretty.
> + * duration of detected HW condition (like over temp). This does
"over-temperature"
> + * not play nicely under any condition but we can work around it
> + * except when we have shared IRQs. So we don't require IRQ to be
> + * populated to help those poor sods who did connect IRQ to shared pin.
No swearing in comments please.
How do you know if an IRQ is shared?
> + * If IRQ information is not given, then we mask all IRQs and do not
> + * provide IRQ resources to regulator d
[PATCH v7 3/6] mfd: bd9576: Add IRQ support
BD9573 and BD9576 support set of "protection" interrupts for "fatal"
issues. Those lead to SOC reset as PMIC shuts the power outputs. Thus
there is no relevant IRQ handling for them.
Few "detection" interrupts were added to the BD9576 with the idea that
SOC could take some recovery-action before error gets unrecoverable.
Unfortunately the BD9576 interrupt logic was not re-evaluated. IRQs
are not designed to be properly acknowleged - and IRQ line is kept
active for whole duration of error condition (in comparison to
informing only about state change).
For above reason, do not consider missing IRQ as error.
Signed-off-by: Matti Vaittinen
---
Changes since v6:
- new patch
drivers/mfd/rohm-bd9576.c | 80 +-
include/linux/mfd/rohm-bd957x.h | 140 +++-
2 files changed, 180 insertions(+), 40 deletions(-)
diff --git a/drivers/mfd/rohm-bd9576.c b/drivers/mfd/rohm-bd9576.c
index f4dd9e438427..99fa0f333277 100644
--- a/drivers/mfd/rohm-bd9576.c
+++ b/drivers/mfd/rohm-bd9576.c
@@ -16,12 +16,27 @@
#include
#include
+/*
+ * Due to the BD9576MUF nasty IRQ behaiour we don't always populate IRQs.
+ * These will be added to regulator resources only if IRQ information for the
+ * PMIC is populated in device-tree.
+ */
+static const struct resource bd9576_regulator_irqs[] = {
+ DEFINE_RES_IRQ_NAMED(BD9576_INT_THERM, "bd9576-temp"),
+ DEFINE_RES_IRQ_NAMED(BD9576_INT_OVD, "bd9576-ovd"),
+ DEFINE_RES_IRQ_NAMED(BD9576_INT_UVD, "bd9576-uvd"),
+};
+
static struct mfd_cell bd9573_mfd_cells[] = {
{ .name = "bd9573-pmic", },
{ .name = "bd9576-wdt", },
};
static struct mfd_cell bd9576_mfd_cells[] = {
+ /*
+* Please keep regulators as first cell as resources may be overwritten
+* from probe and the code expects regulators to be at index 0.
+*/
{ .name = "bd9576-pmic", },
{ .name = "bd9576-wdt", },
};
@@ -48,6 +63,29 @@ static struct regmap_config bd957x_regmap = {
.cache_type = REGCACHE_RBTREE,
};
+static struct regmap_irq bd9576_irqs[] = {
+ REGMAP_IRQ_REG(BD9576_INT_THERM, 0, BD957X_MASK_INT_MAIN_THERM),
+ REGMAP_IRQ_REG(BD9576_INT_OVP, 0, BD957X_MASK_INT_MAIN_OVP),
+ REGMAP_IRQ_REG(BD9576_INT_SCP, 0, BD957X_MASK_INT_MAIN_SCP),
+ REGMAP_IRQ_REG(BD9576_INT_OCP, 0, BD957X_MASK_INT_MAIN_OCP),
+ REGMAP_IRQ_REG(BD9576_INT_OVD, 0, BD957X_MASK_INT_MAIN_OVD),
+ REGMAP_IRQ_REG(BD9576_INT_UVD, 0, BD957X_MASK_INT_MAIN_UVD),
+ REGMAP_IRQ_REG(BD9576_INT_UVP, 0, BD957X_MASK_INT_MAIN_UVP),
+ REGMAP_IRQ_REG(BD9576_INT_SYS, 0, BD957X_MASK_INT_MAIN_SYS),
+};
+
+static struct regmap_irq_chip bd9576_irq_chip = {
+ .name = "bd9576_irq",
+ .irqs = &bd9576_irqs[0],
+ .num_irqs = ARRAY_SIZE(bd9576_irqs),
+ .status_base = BD957X_REG_INT_MAIN_STAT,
+ .mask_base = BD957X_REG_INT_MAIN_MASK,
+ .ack_base = BD957X_REG_INT_MAIN_STAT,
+ .init_ack_masked = true,
+ .num_regs = 1,
+ .irq_reg_stride = 1,
+};
+
static int bd957x_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
@@ -56,6 +94,7 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
struct mfd_cell *mfd;
int cells;
unsigned long chip_type;
+ struct irq_domain *domain;
chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
@@ -67,6 +106,11 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
case ROHM_CHIP_TYPE_BD9573:
mfd = bd9573_mfd_cells;
cells = ARRAY_SIZE(bd9573_mfd_cells);
+ /* BD9573 only supports fatal IRQs which we do not handle */
+ if (i2c->irq) {
+ dev_err(&i2c->dev, "no supported irqs on BD9573\n");
+ return -EINVAL;
+ }
break;
default:
dev_err(&i2c->dev, "Unknown device type");
@@ -78,9 +122,43 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
dev_err(&i2c->dev, "Failed to initialize Regmap\n");
return PTR_ERR(regmap);
}
+ /*
+* BD9576 behaves badly. It kepts IRQ asserted for the whole
+* duration of detected HW condition (like over temp). This does
+* not play nicely under any condition but we can work around it
+* except when we have shared IRQs. So we don't require IRQ to be
+* populated to help those poor sods who did connect IRQ to shared pin.
+* If IRQ information is not given, then we mask all IRQs and do not
+* provide IRQ resources to regulator driver - which then just omits
+* the notifiers.
+*/
+ if (i2c->irq) {
+ struct regmap_irq_chip_data *irq_data;
+ struct mfd_cell *regulators = &bd9576_mfd_cells[0];
+
+ regulators->resources = bd9576_regulator_irqs;
+ regulators->num_resour
