| Message ID | 20220620200644.1961936-1-aidanmacdonald.0x0@gmail.com |
|---|---|
| Headers | show |
| Series | regmap-irq cleanups and refactoring | expand |
On Mon, Jun 20, 2022 at 10:07 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > Hi Mark, > > Here's a bunch of cleanups for regmap-irq focused on simplifying the API > and generalizing it a bit. It's broken up into three refactors, focusing > on one area at a time. > > * Patches 01 and 02 are straightforward bugfixes, independent of the > rest of the series. Neither of the bugs are triggered by in-tree > drivers but they might be worth picking up early anyhow. > > * Patches 03-13 clean up everything related to configuring IRQ types. > > * Patches 14-45 deal with mask/unmask registers. First, make unmask > registers behave more intuitively and usefully, and get rid of the > mask_invert flag in favor of describing inverted mask registers as > unmask registers. Second, make the mask_writeonly flag more useful > and enable it for two chips where it makes sense. > > * Patches 46-49 refactor sub_irq_reg() as a get_irq_reg() callback, > and use that to eliminate the not_fixed_stride flag. > > The approach I used when refactoring is pretty simple: (1) introduce new > functionality in regmap-irq, (2) convert the drivers, and (3) remove any > old code. Nothing should break in the middle. > > The patches can be re-ordered to some extent if that's preferable, but > it's best to add get_irq_reg() last to avoid having to think about how > it interacts with features that'll be removed anyway. > > I can't test most of the devices affected by this series so a lot of the > code is only build tested. I've tested on real hardware with my AXP192 > patchset[1], although it only provides limited code coverage. > > qcom-pm8008 in particular deserves careful testing - it used all of the > features touched by the refactors and required the most changes. Other > drivers only required trivial changes but there are three of them worth > mentioning: wcd943x, wcd9335, and wcd938x. They have suspicious looking > IRQ type definitions and I'm pretty sure aren't working properly, but > I can't fix them myself. The refactor shouldn't affect their behavior > so how / when / if they get fixed shouldn't be much of an issue. > > Oh, and I added the 'mask_writeonly' flag and volatile ranges to the > stpmic1 driver based on its datasheet[2] as a small optimization. It's > probably fine but testing would be a good idea. > > [1]: https://lore.kernel.org/linux-iio/20220618214009.2178567-1-aidanmacdonald.0x0@gmailcom/ > [2]: https://www.st.com/resource/en/datasheet/stpmic1.pdf Cool series, thanks for cleaning this up!
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > No drivers currently use mask_writeonly, and in its current form > it seems a bit misleading. When set, mask registers will be > updated with regmap_write_bits() instead of regmap_update_bits(), > but regmap_write_bits() still does a read-modify-write under the > hood. It's not a write-only operation. > > Performing a simple regmap_write() is probably more useful, since > it can be used for chips that have separate set & clear registers > for controlling mask bits. Such registers are normally volatile > and read as 0, so avoiding a register read minimizes bus traffic. Reading your explanations and the code, I would rather think about fixing the regmap_write_bits() to be writeonly op. Otherwise it's unclear what's the difference between regmap_write_bits() vs. regmap_update_bits(). ... > if (d->chip->mask_writeonly) > - return regmap_write_bits(d->map, reg, mask, val); > + return regmap_write(d->map, reg, val & mask); > else > return regmap_update_bits(d->map, reg, mask, val);
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > This flag is necessary to prepare for fixing the behavior of unmask > registers. Existing chips that set mask_base and unmask_base must > set broken_mask_unmask=1 to declare that they expect the mask bits Boolean should take true/false. > will be inverted in both registers, contrary to the usual behavior > of mask registers. > diff --git a/include/linux/regmap.h b/include/linux/regmap.h > index ee2567a0465c..21a70fd99493 100644 > --- a/include/linux/regmap.h > +++ b/include/linux/regmap.h > @@ -1523,6 +1523,7 @@ struct regmap_irq_chip { > bool clear_on_unmask:1; > bool not_fixed_stride:1; > bool status_invert:1; > + bool broken_mask_unmask:1; Looking at the given context, I would group it with clean_on_unmask above. The above is weird enough on its own. Can you prepare a precursor patch that either drops the bit fields of booleans or moves them to unsigned int? Note, bit fields in C are beasts when it goes to concurrent access. It would be nice to ensure these are not the cases of a such.
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > The STPMIC1 has a normal "1 to disable" mask register with > separate set and clear registers. It's relying on masks and > unmasks being inverted from their intuitive meaning, so it > needs the broken_mask_unmask flag. Same comment as per previous patch and continues to all patches of a kind.
On Mon, Jun 20, 2022 at 10:10 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > An inverted mask register can be described more directly > as an unmask register. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > drivers/gpio/gpio-sl28cpld.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-sl28cpld.c b/drivers/gpio/gpio-sl28cpld.c > index 52404736ac86..2195f88c2048 100644 > --- a/drivers/gpio/gpio-sl28cpld.c > +++ b/drivers/gpio/gpio-sl28cpld.c > @@ -70,8 +70,7 @@ static int sl28cpld_gpio_irq_init(struct platform_device *pdev, > irq_chip->num_irqs = ARRAY_SIZE(sl28cpld_gpio_irqs); > irq_chip->num_regs = 1; > irq_chip->status_base = base + GPIO_REG_IP; > - irq_chip->mask_base = base + GPIO_REG_IE; > - irq_chip->mask_invert = true; > + irq_chip->unmask_base = base + GPIO_REG_IE; > irq_chip->ack_base = base + GPIO_REG_IP; > > ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), > -- > 2.35.1 >
On Mon, Jun 20, 2022 at 09:05:55PM +0100, Aidan MacDonald wrote: > Here's a bunch of cleanups for regmap-irq focused on simplifying the API > and generalizing it a bit. It's broken up into three refactors, focusing > on one area at a time. This series is very large and the way it is interleaving patches for several different subsystems adds to the difficulty managing it. As you've identified there's several different subserieses in here, if you need to resend any of this (I've not even started looking at the actual patches yet) it would be easier to digest with some combination of sending as separate serieses and reordering things so that all the things for each subsystem are grouped together. That'd help with both review and with merging, both large serieses and cross subsystem dependencies tend to slow things down.
Mark Brown <broonie@kernel.org> writes: > On Mon, Jun 20, 2022 at 09:05:55PM +0100, Aidan MacDonald wrote: > >> Here's a bunch of cleanups for regmap-irq focused on simplifying the API >> and generalizing it a bit. It's broken up into three refactors, focusing >> on one area at a time. > > This series is very large and the way it is interleaving patches for > several different subsystems adds to the difficulty managing it. As > you've identified there's several different subserieses in here, if you > need to resend any of this (I've not even started looking at the actual > patches yet) it would be easier to digest with some combination of > sending as separate serieses and reordering things so that all the > things for each subsystem are grouped together. That'd help with both > review and with merging, both large serieses and cross subsystem > dependencies tend to slow things down. Thanks for the advice. After reading this and some of Andy's comments I think I understand how to organize this better. I'll send a trimmed down series including only regmap changes, and instead of removing things right away I'll just mark them deprecated. Then the driver patches can go by subsystem (one series per subsystem?) and once everything is merged the deprecated stuff in regmap-irq can be removed at a later date.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: >> >> This flag is necessary to prepare for fixing the behavior of unmask >> registers. Existing chips that set mask_base and unmask_base must >> set broken_mask_unmask=1 to declare that they expect the mask bits > > Boolean should take true/false. > >> will be inverted in both registers, contrary to the usual behavior >> of mask registers. > >> diff --git a/include/linux/regmap.h b/include/linux/regmap.h >> index ee2567a0465c..21a70fd99493 100644 >> --- a/include/linux/regmap.h >> +++ b/include/linux/regmap.h >> @@ -1523,6 +1523,7 @@ struct regmap_irq_chip { >> bool clear_on_unmask:1; >> bool not_fixed_stride:1; >> bool status_invert:1; >> + bool broken_mask_unmask:1; > > Looking at the given context, I would group it with clean_on_unmask above. > > The above is weird enough on its own. Can you prepare a precursor > patch that either drops the bit fields of booleans or moves them to > unsigned int? Sure. > Note, bit fields in C are beasts when it goes to concurrent access. It > would be nice to ensure these are not the cases of a such. These are read-only so there's no danger here.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: >> >> No drivers currently use mask_writeonly, and in its current form >> it seems a bit misleading. When set, mask registers will be >> updated with regmap_write_bits() instead of regmap_update_bits(), >> but regmap_write_bits() still does a read-modify-write under the >> hood. It's not a write-only operation. >> >> Performing a simple regmap_write() is probably more useful, since >> it can be used for chips that have separate set & clear registers >> for controlling mask bits. Such registers are normally volatile >> and read as 0, so avoiding a register read minimizes bus traffic. > > Reading your explanations and the code, I would rather think about > fixing the regmap_write_bits() to be writeonly op. That's impossible without special hardware support. > Otherwise it's unclear what's the difference between > regmap_write_bits() vs. regmap_update_bits(). This was not obvious to me either. They're the same except in how they issue the low-level write op -- regmap_update_bits() will only do the write if the new value differs from the current one. regmap_write_bits() will always do a write, even if the new value is the same. I think the problem is lack of documentation. I only figured this out by reading the implementation. >> if (d->chip->mask_writeonly) >> - return regmap_write_bits(d->map, reg, mask, val); >> + return regmap_write(d->map, reg, val & mask); >> else >> return regmap_update_bits(d->map, reg, mask, val);
Am 2022-06-20 22:06, schrieb Aidan MacDonald: > An inverted mask register can be described more directly > as an unmask register. > > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> Reviewed-by: Michael Walle <michael@walle.cc>
On Tue, Jun 21, 2022 at 10:04:59PM +0100, Aidan MacDonald wrote: > Thanks for the advice. After reading this and some of Andy's comments I > think I understand how to organize this better. > I'll send a trimmed down series including only regmap changes, and > instead of removing things right away I'll just mark them deprecated. > Then the driver patches can go by subsystem (one series per subsystem?) > and once everything is merged the deprecated stuff in regmap-irq can be > removed at a later date. That sounds like it should help a lot, thanks.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Tuesday, June 21, 2022, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: >> Andy Shevchenko <andy.shevchenko@gmail.com> writes: >> >> > On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald >> > <aidanmacdonald.0x0@gmail.com> wrote: >> >> >> >> No drivers currently use mask_writeonly, and in its current form >> >> it seems a bit misleading. When set, mask registers will be >> >> updated with regmap_write_bits() instead of regmap_update_bits(), >> >> but regmap_write_bits() still does a read-modify-write under the >> >> hood. It's not a write-only operation. >> >> >> >> Performing a simple regmap_write() is probably more useful, since >> >> it can be used for chips that have separate set & clear registers >> >> for controlling mask bits. Such registers are normally volatile >> >> and read as 0, so avoiding a register read minimizes bus traffic. >> > >> > Reading your explanations and the code, I would rather think about >> > fixing the regmap_write_bits() to be writeonly op. >> >> That's impossible without special hardware support. >> >> > Otherwise it's unclear what's the difference between >> > regmap_write_bits() vs. regmap_update_bits(). >> >> This was not obvious to me either. They're the same except in how they >> issue the low-level write op -- regmap_update_bits() will only do the >> write if the new value differs from the current one. regmap_write_bits() >> will always do a write, even if the new value is the same. > > Okay, it makes a lot of sense for W1C type of bits in the register. > Also, “reading” might imply to restore last value from cache, no? Maybe there needs to be some explanation of what the typical use case is and why you'd choose write_bits() over update_bits(), because the more I think about it the less clear it is. You're right that the read could be served from a cache. But I'm not sure if a cache would be safe if even one bit in the register is volatile, and I can't really see a use case for write_bits() that doesn't involve volatile behavior of some sort. In any event, I'm just going to drop this patch and the related driver patches in favor of removing mask_writeonly entirely, since it looks like it was never used, and after thinking about it I'm not sure what I did helps much. If some driver needs write_bits() for mask registers down the road it's not a big deal to add this back. >> I think the problem is lack of documentation. I only figured this out >> by reading the implementation. >> >> >> if (d->chip->mask_writeonly) >> >> - return regmap_write_bits(d->map, reg, mask, val); >> >> + return regmap_write(d->map, reg, val & mask); >> >> else >> >> return regmap_update_bits(d->map, reg, mask, val);