Re: [PATCH v8 3/6] mfd: bd9576: Add IRQ support

From: Lee Jones
Date: Mon Mar 08 2021 - 08:45:34 EST

Next message: Lee Jones: "Re: [PATCH v8 6/6] mfd: bd9576: Add safety limit/monitoring registers"
Previous message: Andy Shevchenko: "Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 11 Feb 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 <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> ---
> Changes since v7:
> - Do not fail probe is BD9573 IRQ information is populated
> - Comment clean-up/clarifications as suggested by Lee
>
> drivers/mfd/rohm-bd9576.c | 80 ++++++++++++++++++++++++++++++++-
> include/linux/mfd/rohm-bd957x.h | 62 +++++++++++++++++++++++++
> 2 files changed, 141 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/rohm-bd9576.c b/drivers/mfd/rohm-bd9576.c
> index efd439677c9e..6084f9a0aa1c 100644
> --- a/drivers/mfd/rohm-bd9576.c
> +++ b/drivers/mfd/rohm-bd9576.c
> @@ -17,12 +17,27 @@
> #include <linux/regmap.h>
> #include <linux/types.h>
>
> +/*
> + * 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-regulator", },
> { .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.
> + */

Thought I'd mentioned this before (must have slipped my mind).

This is not acceptable as it's much too fragile.

Please use defined indexes to look into the array entries instead.

[BD9576_REGULATOR] = { .name = "bd9576-regulator" }

Etc.

> { .name = "bd9576-regulator", },
> { .name = "bd9576-wdt", },
> };
> @@ -49,6 +64,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)
> {
> @@ -57,6 +95,8 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
> struct mfd_cell *cells;
> int num_cells;
> unsigned long chip_type;
> + struct irq_domain *domain;
> + bool usable_irqs;
>
> chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
>
> @@ -64,10 +104,16 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
> case ROHM_CHIP_TYPE_BD9576:
> cells = bd9576_mfd_cells;
> num_cells = ARRAY_SIZE(bd9576_mfd_cells);
> + usable_irqs = !!i2c->irq;
> break;
> case ROHM_CHIP_TYPE_BD9573:
> cells = bd9573_mfd_cells;
> num_cells = ARRAY_SIZE(bd9573_mfd_cells);
> + /*
> + * BD9573 only supports fatal IRQs which we can not handle
> + * because SoC is going to lose the power.
> + */
> + usable_irqs = false;
> break;
> default:
> dev_err(&i2c->dev, "Unknown device type");
> @@ -79,9 +125,41 @@ static int bd957x_i2c_probe(struct i2c_client *i2c,
> dev_err(&i2c->dev, "Failed to initialize Regmap\n");
> return PTR_ERR(regmap);
> }

Nit: '\n'

> + /*
> + * BD9576 behaves badly. It kepts IRQ line asserted for the whole
> + * duration of detected HW condition (like over temperature). So we
> + * don't require IRQ 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.
> + */
> + if (usable_irqs) {
> + struct regmap_irq_chip_data *irq_data;
> + struct mfd_cell *regulators = &bd9576_mfd_cells[0];
> +
> + regulators->resources = bd9576_regulator_irqs;
> + regulators->num_resources = ARRAY_SIZE(bd9576_regulator_irqs);
> +
> + ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, i2c->irq,
> + IRQF_ONESHOT, 0,
> + &bd9576_irq_chip, &irq_data);
> + if (ret) {
> + dev_err(&i2c->dev, "Failed to add IRQ chip\n");
> + return ret;
> + }
> + domain = regmap_irq_get_domain(irq_data);
> + dev_dbg(&i2c->dev, "Using IRQs for BD9576MUF\n");

Doesn't seem overly helpful. Please remove it.

> + } else {
> + ret = regmap_update_bits(regmap, BD957X_REG_INT_MAIN_MASK,
> + BD957X_MASK_INT_ALL,
> + BD957X_MASK_INT_ALL);
> + if (ret)
> + return ret;
> + domain = NULL;
> + }
>
> ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, cells,
> - num_cells, NULL, 0, NULL);
> + num_cells, NULL, 0, domain);
> if (ret)
> dev_err(&i2c->dev, "Failed to create subdevices\n");
>
> diff --git a/include/linux/mfd/rohm-bd957x.h b/include/linux/mfd/rohm-bd957x.h
> index ae59c0f7188d..3f351a1ae4ab 100644
> --- a/include/linux/mfd/rohm-bd957x.h
> +++ b/include/linux/mfd/rohm-bd957x.h
> @@ -13,6 +13,55 @@ enum {
> BD957X_VOUTS1,
> };
>
> +/*
> + * The BD9576 has own IRQ 'blocks' for:
> + * - I2C/thermal,
> + * - Over voltage protection
> + * - Short-circuit protection
> + * - Over current protection
> + * - Over voltage detection
> + * - Under voltage detection
> + * - Under voltage protection
> + * - 'system interrupt'.
> + *
> + * Each of the blocks have a status register giving more accurate IRQ source
> + * information - for example which of the regulators have over-voltage.
> + *
> + * On top of this, there is "main IRQ" status register where each bit indicates
> + * which of sub-blocks have active IRQs. Fine. That would fit regmap-irq main
> + * status handling. Except that:
> + * - Only some sub-IRQs can be masked.
> + * - The IRQ informs us about fault-condition, not when fault state changes.
> + * The IRQ line it is kept asserted until the detected condition is acked
> + * AND cleared in HW. This is annoying for IRQs like the one informing high
> + * temperature because if IRQ is not disabled it keeps the CPU in IRQ
> + * handling loop.
> + *
> + * For now we do just use the main-IRQ register as source for our IRQ
> + * information and bind the regmap-irq to this. We leave fine-grained sub-IRQ
> + * register handling to handlers in sub-devices. The regulator driver shall
> + * read which regulators are source for problem - or if the detected error is
> + * regulator temperature error. The sub-drivers do also handle masking of "sub-
> + * IRQs" if this is supported/needed.
> + *
> + * To overcome the problem with HW keeping IRQ asserted we do call
> + * disable_irq_nosync() from sub-device handler and add a delayed work to
> + * re-enable IRQ roughly 1 second later. This should keep our CPU out of
> + * busy-loop.
> + */
> +#define IRQS_SILENT_MS 1000
> +
> +enum {
> + BD9576_INT_THERM,
> + BD9576_INT_OVP,
> + BD9576_INT_SCP,
> + BD9576_INT_OCP,
> + BD9576_INT_OVD,
> + BD9576_INT_UVD,
> + BD9576_INT_UVP,
> + BD9576_INT_SYS,
> +};
> +
> #define BD957X_REG_SMRB_ASSERT 0x15
> #define BD957X_REG_PMIC_INTERNAL_STAT 0x20
> #define BD957X_REG_INT_THERM_STAT 0x23
> @@ -28,6 +77,19 @@ enum {
> #define BD957X_REG_INT_MAIN_STAT 0x30
> #define BD957X_REG_INT_MAIN_MASK 0x31
>
> +#define UVD_IRQ_VALID_MASK 0x6F
> +#define OVD_IRQ_VALID_MASK 0x2F
> +
> +#define BD957X_MASK_INT_MAIN_THERM BIT(0)
> +#define BD957X_MASK_INT_MAIN_OVP BIT(1)
> +#define BD957X_MASK_INT_MAIN_SCP BIT(2)
> +#define BD957X_MASK_INT_MAIN_OCP BIT(3)
> +#define BD957X_MASK_INT_MAIN_OVD BIT(4)
> +#define BD957X_MASK_INT_MAIN_UVD BIT(5)
> +#define BD957X_MASK_INT_MAIN_UVP BIT(6)
> +#define BD957X_MASK_INT_MAIN_SYS BIT(7)
> +#define BD957X_MASK_INT_ALL 0xff
> +
> #define BD957X_REG_WDT_CONF 0x16
>
> #define BD957X_REG_POW_TRIGGER1 0x41

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Next message: Lee Jones: "Re: [PATCH v8 6/6] mfd: bd9576: Add safety limit/monitoring registers"
Previous message: Andy Shevchenko: "Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]