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

[Lee Jones <lee.jones@linaro.org>]

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 <matti.vaittinen@fi.rohmeurope.com>
> ---
> 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 <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-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 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?

> +	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_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_info(&i2c->dev, "Using IRQs for BD9576MUF\n");
> +	} 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?

> +		if (ret)
> +			return ret;
> +		domain = NULL;
> +	}
>  
>  	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
> -				   NULL, 0, NULL);
> +				   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 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.

> + * I2C/THERMAL,

Does this precede each line?

> + * Over voltage protection
> + * Short-circuit protection
> + * Over current protection
> + * Over voltage detection
> + * Under voltage detection
> + * Under voltage protection
> + * and '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 cleared
> + *    in HW. This is _really_ annoying for IRQs like the one informing high
> + *    temperature. Best mitigation for high temperature for sure is to
> + *    keep the processor in IRQ loop, right? (NO!)

No sarcasm in comments please it can be easily lost in translation.

Please stay helpful and keep to the facts.

> + * So - 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
> -#define BD957X_REG_INT_THERM_MASK 0x24
> -#define BD957X_REG_INT_OVP_STAT 0x25
> -#define BD957X_REG_INT_SCP_STAT 0x26
> -#define BD957X_REG_INT_OCP_STAT 0x27
> -#define BD957X_REG_INT_OVD_STAT 0x28
> -#define BD957X_REG_INT_UVD_STAT 0x29
> -#define BD957X_REG_INT_UVP_STAT 0x2a
> -#define BD957X_REG_INT_SYS_STAT 0x2b
> -#define BD957X_REG_INT_SYS_MASK 0x2c
> -#define BD957X_REG_INT_MAIN_STAT 0x30
> -#define BD957X_REG_INT_MAIN_MASK 0x31
> -
> -#define BD957X_REG_WDT_CONF 0x16
> -
> -#define BD957X_REG_POW_TRIGGER1 0x41
> -#define BD957X_REG_POW_TRIGGER2 0x42
> -#define BD957X_REG_POW_TRIGGER3 0x43
> -#define BD957X_REG_POW_TRIGGER4 0x44
> -#define BD957X_REG_POW_TRIGGERL1 0x45
> -#define BD957X_REG_POW_TRIGGERS1 0x46
> -
> -#define BD957X_REGULATOR_EN_MASK 0xff
> -#define BD957X_REGULATOR_DIS_VAL 0xff
> -
> -#define BD957X_VSEL_REG_MASK	0xff
> -
> -#define BD957X_MASK_VOUT1_TUNE	0x87
> -#define BD957X_MASK_VOUT2_TUNE	0x87
> -#define BD957X_MASK_VOUT3_TUNE	0x1f
> -#define BD957X_MASK_VOUT4_TUNE	0x1f
> -#define BD957X_MASK_VOUTL1_TUNE	0x87
> -
> -#define BD957X_REG_VOUT1_TUNE	0x50
> -#define BD957X_REG_VOUT2_TUNE	0x53
> -#define BD957X_REG_VOUT3_TUNE	0x56
> -#define BD957X_REG_VOUT4_TUNE	0x59
> -#define BD957X_REG_VOUTL1_TUNE	0x5c
> -
> -#define BD957X_MAX_REGISTER 0x61
> +#define BD957X_REG_INT_THERM_MASK	0x24
> +#define BD957X_REG_INT_OVP_STAT		0x25
> +#define BD957X_REG_INT_SCP_STAT		0x26
> +#define BD957X_REG_INT_OCP_STAT		0x27
> +#define BD957X_REG_INT_OVD_STAT		0x28
> +#define BD957X_REG_INT_UVD_STAT		0x29
> +#define BD957X_REG_INT_UVP_STAT		0x2a
> +#define BD957X_REG_INT_SYS_STAT		0x2b
> +#define BD957X_REG_INT_SYS_MASK		0x2c
> +#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
> +#define BD957X_REG_POW_TRIGGER2		0x42
> +#define BD957X_REG_POW_TRIGGER3		0x43
> +#define BD957X_REG_POW_TRIGGER4		0x44
> +#define BD957X_REG_POW_TRIGGERL1	0x45
> +#define BD957X_REG_POW_TRIGGERS1	0x46
> +
> +#define BD957X_REGULATOR_EN_MASK	0xff
> +#define BD957X_REGULATOR_DIS_VAL	0xff
> +
> +#define BD957X_VSEL_REG_MASK		0xff
> +
> +#define BD957X_MASK_VOUT1_TUNE		0x87
> +#define BD957X_MASK_VOUT2_TUNE		0x87
> +#define BD957X_MASK_VOUT3_TUNE		0x1f
> +#define BD957X_MASK_VOUT4_TUNE		0x1f
> +#define BD957X_MASK_VOUTL1_TUNE		0x87
> +
> +#define BD957X_REG_VOUT1_TUNE		0x50
> +#define BD957X_REG_VOUT2_TUNE		0x53
> +#define BD957X_REG_VOUT3_TUNE		0x56
> +#define BD957X_REG_VOUT4_TUNE		0x59
> +#define BD957X_REG_VOUTL1_TUNE		0x5c
> +
> +#define BD957X_MAX_REGISTER		0x61

Line all these up please.

>  #endif

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