lkml.org 
[lkml]   [2022]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 20/49] regmap-irq: Fix inverted handling of unmask registers
    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@gmail.com>
    > ---
    > 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
    >

    \
     
     \ /
      Last update: 2022-06-24 02:22    [W:3.649 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site