Re: [PATCH 20/49] regmap-irq: Fix inverted handling of unmask registers

[Guru Das Srinagesh <quic_gurus@xxxxxxxxxxx>]

On Mon, Jun 20, 2022 at 09:06:15PM +0100, Aidan MacDonald wrote:
> To me "unmask" suggests that we write 1s to the register when
> an interrupt is enabled. This also makes sense because it's the
> opposite of what the "mask" register does (write 1s to disable
> an interrupt).
> 
> But regmap-irq does the opposite: for a disabled interrupt, it
> writes 1s to "unmask" and 0s to "mask". This is surprising and
> deviates from the usual way mask registers are handled.

Thank you for fixing this.

> 
> Additionally, mask_invert didn't interact with unmask registers
> properly -- it caused them to be ignored entirely.
> 
> Fix this by making mask and unmask registers orthogonal, using
> the following behavior:
> 
> * Mask registers are written with 1s for disabled interrupts.
> * Unmask registers are written with 1s for enabled interrupts.

This is more in line with my understanding of the semantics as well.

> 
> This behavior supports both normal or inverted mask registers
> and separate set/clear registers via different combinations of
> mask_base/unmask_base. The mask_invert flag is made redundant,
> since an inverted mask register can be described more directly
> as an unmask register.
> 
> To cope with existing drivers that rely on the old "backward"
> behavior, check for the broken_mask_unmask flag and swap the
> roles of mask/unmask registers. This is a compatibility measure
> which can be dropped once the drivers are updated to use the
> new, more consistent behavior.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>
> ---
>  drivers/base/regmap/regmap-irq.c | 96 +++++++++++++++++---------------
>  include/linux/regmap.h           |  7 ++-
>  2 files changed, 55 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index 875415fc3133..082a2981120c 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -30,6 +30,9 @@ struct regmap_irq_chip_data {
>  	int irq;
>  	int wake_count;
>  
> +	unsigned int mask_base;
> +	unsigned int unmask_base;
> +
>  	void *status_reg_buf;
>  	unsigned int *main_status_buf;
>  	unsigned int *status_buf;
> @@ -95,7 +98,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>  	struct regmap *map = d->map;
>  	int i, j, ret;
>  	u32 reg;
> -	u32 unmask_offset;
>  	u32 val;
>  
>  	if (d->chip->runtime_pm) {
> @@ -124,35 +126,23 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>  	 * suppress pointless writes.
>  	 */
>  	for (i = 0; i < d->chip->num_regs; i++) {
> -		if (!d->chip->mask_base)
> -			continue;
> -
> -		reg = sub_irq_reg(d, d->chip->mask_base, i);
> -		if (d->chip->mask_invert) {
> +		if (d->mask_base) {
> +			reg = sub_irq_reg(d, d->mask_base, i);
>  			ret = regmap_irq_update_mask_bits(d, reg,
> -					 d->mask_buf_def[i], ~d->mask_buf[i]);
> -		} else if (d->chip->unmask_base) {
> -			/* set mask with mask_base register */
> +					d->mask_buf_def[i], d->mask_buf[i]);
> +			if (ret != 0)
> +				dev_err(d->map->dev, "Failed to sync masks in %x\n",
> +					reg);
> +		}
> +
> +		if (d->unmask_base) {
> +			reg = sub_irq_reg(d, d->unmask_base, i);
>  			ret = regmap_irq_update_mask_bits(d, reg,
>  					d->mask_buf_def[i], ~d->mask_buf[i]);
> -			if (ret < 0)
> -				dev_err(d->map->dev,
> -					"Failed to sync unmasks in %x\n",
> +			if (ret != 0)
> +				dev_err(d->map->dev, "Failed to sync masks in %x\n",
>  					reg);
> -			unmask_offset = d->chip->unmask_base -
> -							d->chip->mask_base;
> -			/* clear mask with unmask_base register */
> -			ret = regmap_irq_update_mask_bits(d,
> -					reg + unmask_offset,
> -					d->mask_buf_def[i],
> -					d->mask_buf[i]);
> -		} else {
> -			ret = regmap_irq_update_mask_bits(d, reg,
> -					 d->mask_buf_def[i], d->mask_buf[i]);
>  		}
> -		if (ret != 0)
> -			dev_err(d->map->dev, "Failed to sync masks in %x\n",
> -				reg);
>  
>  		reg = sub_irq_reg(d, d->chip->wake_base, i);
>  		if (d->wake_buf) {
> @@ -634,7 +624,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>  	int i;
>  	int ret = -ENOMEM;
>  	u32 reg;
> -	u32 unmask_offset;
>  
>  	if (chip->num_regs <= 0)
>  		return -EINVAL;
> @@ -732,6 +721,24 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>  	d->chip = chip;
>  	d->irq_base = irq_base;
>  
> +	/*
> +	 * Swap role of mask_base and unmask_base if mask bits are inverted.
> +	 *
> +	 * Historically, chips that specify both mask_base and unmask_base
> +	 * got inverted mask behavior; this was arguably a bug in regmap-irq
> +	 * and there was no way to get the normal, non-inverted behavior.
> +	 * Those chips will set the broken_mask_unmask flag. They don't set
> +	 * mask_invert so there is no need to worry about interactions with
> +	 * that flag.
> +	 */
> +	if (chip->mask_invert || chip->broken_mask_unmask) {

I'm not able to comment on whether mask_invert belongs here.

> +		d->mask_base = chip->unmask_base;
> +		d->unmask_base = chip->mask_base;
> +	} else {
> +		d->mask_base = chip->mask_base;
> +		d->unmask_base = chip->unmask_base;
> +	}
> +
>  	if (chip->irq_reg_stride)
>  		d->irq_reg_stride = chip->irq_reg_stride;
>  	else
> @@ -755,28 +762,27 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
>  	/* Mask all the interrupts by default */
>  	for (i = 0; i < chip->num_regs; i++) {
>  		d->mask_buf[i] = d->mask_buf_def[i];
> -		if (!chip->mask_base)
> -			continue;
>  
> -		reg = sub_irq_reg(d, d->chip->mask_base, i);
> -
> -		if (chip->mask_invert)
> +		if (d->mask_base) {
> +			reg = sub_irq_reg(d, d->mask_base, i);
>  			ret = regmap_irq_update_mask_bits(d, reg,
> -					 d->mask_buf[i], ~d->mask_buf[i]);
> -		else if (d->chip->unmask_base) {
> -			unmask_offset = d->chip->unmask_base -
> -					d->chip->mask_base;
> -			ret = regmap_irq_update_mask_bits(d,
> -					reg + unmask_offset,
> -					d->mask_buf[i],
> -					d->mask_buf[i]);
> -		} else
> +					d->mask_buf_def[i], d->mask_buf[i]);
> +			if (ret != 0) {
> +				dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
> +					reg, ret);
> +				goto err_alloc;
> +			}
> +		}
> +
> +		if (d->unmask_base) {

This makes a lot of sense. unmask_base really needed to be handled separately
and not as an offset from mask_base.

> +			reg = sub_irq_reg(d, d->unmask_base, i);
>  			ret = regmap_irq_update_mask_bits(d, reg,
> -					 d->mask_buf[i], d->mask_buf[i]);
> -		if (ret != 0) {
> -			dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
> -				reg, ret);
> -			goto err_alloc;
> +					d->mask_buf_def[i], ~d->mask_buf[i]);
> +			if (ret != 0) {
> +				dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
> +					reg, ret);
> +				goto err_alloc;
> +			}
>  		}
>  
>  		if (!chip->init_ack_masked)
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 21a70fd99493..0cf3c4a66946 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1451,10 +1451,11 @@ struct regmap_irq_sub_irq_map {
>   *		   main_status set.
>   *
>   * @status_base: Base status register address.
> - * @mask_base:   Base mask register address.
> + * @mask_base:   Base mask register address. Mask bits are set to 1 when an
> + *               interrupt is masked, 0 when unmasked.
>   * @mask_writeonly: Base mask register is write only.
> - * @unmask_base:  Base unmask register address. for chips who have
> - *                separate mask and unmask registers
> + * @unmask_base:  Base unmask register address. Unmask bits are set to 1 when
> + *                an interrupt is unmasked and 0 when masked.
>   * @ack_base:    Base ack address. If zero then the chip is clear on read.
>   *               Using zero value is possible with @use_ack bit.
>   * @wake_base:   Base address for wake enables.  If zero unsupported.
> -- 
> 2.35.1
>