| Message ID | 526562423eaa58b4075362083f561841f1d6956c.1615423027.git.gurus@codeaurora.org |
|---|---|
| State | Accepted |
| Commit | 1066cfbdfa3f5c401870fad577fe63d1171a5bcd |
| Headers | show |
| Series | Add support for Qualcomm MFD PMIC register layout | expand |
Hi All, On Wed, 2021-03-10 at 16:39 -0800, Guru Das Srinagesh wrote: > Qualcomm's MFD chips have a top level interrupt status register and > sub-irqs (peripherals). When a bit in the main status register goes > high, it means that the peripheral corresponding to that bit has an > unserviced interrupt. If the bit is not set, this means that the > corresponding peripheral does not. > > Commit a2d21848d9211d ("regmap: regmap-irq: Add main status register > support") introduced the sub-irq logic that is currently applied only > when reading status registers, but not for any other functions like > acking > or masking. Extend the use of sub-irq to all other functions, with > two > caveats regarding the specification of offsets: > > - Each member of the sub_reg_offsets array should be of length 1 > - The specified offsets should be the unequal strides for each sub- > irq > device. > > In QCOM's case, all the *_base registers are to be configured to the > base addresses of the first sub-irq group, with offsets of each > subsequent group calculated as a difference from these addresses. > > Continuing from the example mentioned in the cover letter: > > /* > * Address of MISC_INT_MASK = 0x1011 > * Address of TEMP_ALARM_INT_MASK = 0x2011 > * Address of GPIO01_INT_MASK = 0x3011 > * > * Calculate offsets as: > * offset_0 = 0x1011 - 0x1011 = 0 (to access MISC's > * registers) > * offset_1 = 0x2011 - 0x1011 = 0x1000 > * offset_2 = 0x3011 - 0x1011 = 0x2000 > */ > > static unsigned int sub_unit0_offsets[] = {0}; > static unsigned int sub_unit1_offsets[] = {0x1000}; > static unsigned int sub_unit2_offsets[] = {0x2000}; > > static struct regmap_irq_sub_irq_map chip_sub_irq_offsets[] = { > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), > }; > > static struct regmap_irq_chip chip_irq_chip = { > --------8<-------- > .not_fixed_stride = true, > .mask_base = MISC_INT_MASK, > .type_base = MISC_INT_TYPE, > .ack_base = MISC_INT_ACK, > .sub_reg_offsets = chip_sub_irq_offsets, > --------8<-------- > }; > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> > --- > drivers/base/regmap/regmap-irq.c | 81 ++++++++++++++++++++++++++-- > ------------ > include/linux/regmap.h | 7 ++++ > 2 files changed, 60 insertions(+), 28 deletions(-) > > diff --git a/drivers/base/regmap/regmap-irq.c > b/drivers/base/regmap/regmap-irq.c > index 19db764..e1d8fc9e 100644 > --- a/drivers/base/regmap/regmap-irq.c > +++ b/drivers/base/regmap/regmap-irq.c > @@ -45,6 +45,27 @@ struct regmap_irq_chip_data { > bool clear_status:1; > }; > Sorry that I am late with the "review" but I only now noticed this change when I was following the references from PM8008 PMIC patch mail. > > +static int sub_irq_reg(struct regmap_irq_chip_data *data, > + unsigned int base_reg, int i) Do I read this correctly - this function should map the main status bit (given in i) to the (sub)IRQ register, right? How does this work for cases where one bit corresponds to more than one sub-register? Or do I misunderstand the purpose of this function? (This is the case with both the BD70528 and BD71828). > +{ > + const struct regmap_irq_chip *chip = data->chip; > + struct regmap *map = data->map; > + struct regmap_irq_sub_irq_map *subreg; > + unsigned int offset; > + int reg = 0; > + > + if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { > + /* Assume linear mapping */ > + reg = base_reg + (i * map->reg_stride * data- > >irq_reg_stride); > + } else { > + subreg = &chip->sub_reg_offsets[i]; > + offset = subreg->offset[0]; > + reg = base_reg + offset; > + } > + > + return reg; > +} > + > static inline const > struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data > *data, > int irq) > @@ -87,8 +108,7 @@ static void regmap_irq_sync_unlock(struct irq_data > *data) > > if (d->clear_status) { > for (i = 0; i < d->chip->num_regs; i++) { > - reg = d->chip->status_base + > - (i * map->reg_stride * d- > >irq_reg_stride); > + reg = sub_irq_reg(d, d->chip->status_base, i); How does this work with the case where we have many subregs pointed by single main-status register bit? If I read this correctly, then the chip->num_regs can be greater than amount of meaningful main-status bits and thus greater than amount of map entries. I don't have BD71828 or BD70528 at home so I haven't tested this. So hopefully I misunderstand something - but if I don't, then this change will break the existing main-IRQ functionality. I will visit the office later this week and I'll see if I have a chance to test this - but I hope you can check your change still supports the case where one main status IRQ bit signals more than one sub-register with active IRQs. Best Regards Matti Vaittinen -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC K iviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
Hi Matti, On Mon, Apr 12, 2021 at 11:08:49AM +0000, Vaittinen, Matti wrote: > Hi, > > On Mon, 2021-04-12 at 09:05 +0300, Matti Vaittinen wrote: > > Hi All, > > > > On Wed, 2021-03-10 at 16:39 -0800, Guru Das Srinagesh wrote: > > > Qualcomm's MFD chips have a top level interrupt status register and > > > sub-irqs (peripherals). When a bit in the main status register > > > goes > > > high, it means that the peripheral corresponding to that bit has an > > > unserviced interrupt. If the bit is not set, this means that the > > > corresponding peripheral does not. > > > > > > Commit a2d21848d9211d ("regmap: regmap-irq: Add main status > > > register > > > support") introduced the sub-irq logic that is currently applied > > > only > > > when reading status registers, but not for any other functions like > > > acking > > > or masking. Extend the use of sub-irq to all other functions, with > > > two > > > caveats regarding the specification of offsets: > > > > > > - Each member of the sub_reg_offsets array should be of length 1 > > > - The specified offsets should be the unequal strides for each sub- > > > irq > > > device. > > > > > > In QCOM's case, all the *_base registers are to be configured to > > > the > > > base addresses of the first sub-irq group, with offsets of each > > > subsequent group calculated as a difference from these addresses. > > > > > > Continuing from the example mentioned in the cover letter: > > > > > > /* > > > * Address of MISC_INT_MASK = 0x1011 > > > * Address of TEMP_ALARM_INT_MASK = 0x2011 > > > * Address of GPIO01_INT_MASK = 0x3011 > > > * > > > * Calculate offsets as: > > > * offset_0 = 0x1011 - 0x1011 = 0 (to access MISC's > > > * registers) > > > * offset_1 = 0x2011 - 0x1011 = 0x1000 > > > * offset_2 = 0x3011 - 0x1011 = 0x2000 > > > */ > > > > > > static unsigned int sub_unit0_offsets[] = {0}; > > > static unsigned int sub_unit1_offsets[] = {0x1000}; > > > static unsigned int sub_unit2_offsets[] = {0x2000}; > > > > > > static struct regmap_irq_sub_irq_map chip_sub_irq_offsets[] = { > > > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), > > > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), > > > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), > > > }; > > > > > > static struct regmap_irq_chip chip_irq_chip = { > > > --------8<-------- > > > .not_fixed_stride = true, > > > .mask_base = MISC_INT_MASK, > > > .type_base = MISC_INT_TYPE, > > > .ack_base = MISC_INT_ACK, > > > .sub_reg_offsets = chip_sub_irq_offsets, > > > --------8<-------- > > > }; > > > > > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> > > > --- > > > drivers/base/regmap/regmap-irq.c | 81 ++++++++++++++++++++++++++-- > > > ------------ > > > include/linux/regmap.h | 7 ++++ > > > 2 files changed, 60 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/base/regmap/regmap-irq.c > > > b/drivers/base/regmap/regmap-irq.c > > > index 19db764..e1d8fc9e 100644 > > > --- a/drivers/base/regmap/regmap-irq.c > > > +++ b/drivers/base/regmap/regmap-irq.c > > > @@ -45,6 +45,27 @@ struct regmap_irq_chip_data { > > > bool clear_status:1; > > > }; > > > > > > > Sorry that I am late with the "review" but I only now noticed this > > change when I was following the references from PM8008 PMIC patch > > mail. > > > > > > > > > > +static int sub_irq_reg(struct regmap_irq_chip_data *data, > > > + unsigned int base_reg, int i) > > > > Do I read this correctly - this function should map the main status > > bit > > (given in i) to the (sub)IRQ register, right? How does this work for > > cases where one bit corresponds to more than one sub-register? Or do > > I > > misunderstand the purpose of this function? (This is the case with > > both > > the BD70528 and BD71828). > > I did some quick test with BD71815 which I had at home. And it seems to > be I did indeed misunderstand this :) This is not for converting the > main-IRQ bits to sub-irq registers - this is for getting the sub-IRQ > register address based on the 'sub IRQ register index'. Yes, that's right. With this change, the sub-irq concept which was initially introduced to map the main-irq bits to sub-irq registers is being extended and repurposed to cover the specific memory layout described in the commit message and cover letter. I've updated the comment block in the header file for `sub_reg_offsets` to make this clarification as well. The sub_irq_reg() function will not break existing functionality because the crux of it will get executed only if not_fixed_stride is set. > > So I do apologize the noise, it seems all is well and everything > (except my self confidence) keeps working as it did :) > > Thanks for the improvement Guru Das! Thanks for testing this out and providing confirmation, Matti :) Thank you. Guru Das.
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 19db764..e1d8fc9e 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -45,6 +45,27 @@ struct regmap_irq_chip_data { bool clear_status:1; }; +static int sub_irq_reg(struct regmap_irq_chip_data *data, + unsigned int base_reg, int i) +{ + const struct regmap_irq_chip *chip = data->chip; + struct regmap *map = data->map; + struct regmap_irq_sub_irq_map *subreg; + unsigned int offset; + int reg = 0; + + if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { + /* Assume linear mapping */ + reg = base_reg + (i * map->reg_stride * data->irq_reg_stride); + } else { + subreg = &chip->sub_reg_offsets[i]; + offset = subreg->offset[0]; + reg = base_reg + offset; + } + + return reg; +} + static inline const struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data, int irq) @@ -87,8 +108,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) if (d->clear_status) { for (i = 0; i < d->chip->num_regs; i++) { - reg = d->chip->status_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->status_base, i); ret = regmap_read(map, reg, &val); if (ret) @@ -108,8 +128,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) if (!d->chip->mask_base) continue; - reg = d->chip->mask_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->mask_base, i); if (d->chip->mask_invert) { ret = regmap_irq_update_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); @@ -136,8 +155,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) dev_err(d->map->dev, "Failed to sync masks in %x\n", reg); - reg = d->chip->wake_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->wake_base, i); if (d->wake_buf) { if (d->chip->wake_invert) ret = regmap_irq_update_bits(d, reg, @@ -161,8 +179,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data) * it'll be ignored in irq handler, then may introduce irq storm */ if (d->mask_buf[i] && (d->chip->ack_base || d->chip->use_ack)) { - reg = d->chip->ack_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->ack_base, i); + /* some chips ack by write 0 */ if (d->chip->ack_invert) ret = regmap_write(map, reg, ~d->mask_buf[i]); @@ -187,8 +205,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) for (i = 0; i < d->chip->num_type_reg; i++) { if (!d->type_buf_def[i]) continue; - reg = d->chip->type_base + - (i * map->reg_stride * d->type_reg_stride); + reg = sub_irq_reg(d, d->chip->type_base, i); if (d->chip->type_invert) ret = regmap_irq_update_bits(d, reg, d->type_buf_def[i], ~d->type_buf[i]); @@ -352,8 +369,15 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, for (i = 0; i < subreg->num_regs; i++) { unsigned int offset = subreg->offset[i]; - ret = regmap_read(map, chip->status_base + offset, - &data->status_buf[offset]); + if (chip->not_fixed_stride) + ret = regmap_read(map, + chip->status_base + offset, + &data->status_buf[b]); + else + ret = regmap_read(map, + chip->status_base + offset, + &data->status_buf[offset]); + if (ret) break; } @@ -474,10 +498,9 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) } else { for (i = 0; i < data->chip->num_regs; i++) { - ret = regmap_read(map, chip->status_base + - (i * map->reg_stride - * data->irq_reg_stride), - &data->status_buf[i]); + unsigned int reg = sub_irq_reg(data, + data->chip->status_base, i); + ret = regmap_read(map, reg, &data->status_buf[i]); if (ret != 0) { dev_err(map->dev, @@ -499,8 +522,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) data->status_buf[i] &= ~data->mask_buf[i]; if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) { - reg = chip->ack_base + - (i * map->reg_stride * data->irq_reg_stride); + reg = sub_irq_reg(data, data->chip->ack_base, i); + if (chip->ack_invert) ret = regmap_write(map, reg, ~data->status_buf[i]); @@ -605,6 +628,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, return -EINVAL; } + if (chip->not_fixed_stride) { + for (i = 0; i < chip->num_regs; i++) + if (chip->sub_reg_offsets[i].num_regs != 1) + return -EINVAL; + } + if (irq_base) { irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0); if (irq_base < 0) { @@ -700,8 +729,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (!chip->mask_base) continue; - reg = chip->mask_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->mask_base, i); + if (chip->mask_invert) ret = regmap_irq_update_bits(d, reg, d->mask_buf[i], ~d->mask_buf[i]); @@ -725,8 +754,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, continue; /* Ack masked but set interrupts */ - reg = chip->status_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->status_base, i); ret = regmap_read(map, reg, &d->status_buf[i]); if (ret != 0) { dev_err(map->dev, "Failed to read IRQ status: %d\n", @@ -735,8 +763,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, } if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) { - reg = chip->ack_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->ack_base, i); if (chip->ack_invert) ret = regmap_write(map, reg, ~(d->status_buf[i] & d->mask_buf[i])); @@ -765,8 +792,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (d->wake_buf) { for (i = 0; i < chip->num_regs; i++) { d->wake_buf[i] = d->mask_buf_def[i]; - reg = chip->wake_base + - (i * map->reg_stride * d->irq_reg_stride); + reg = sub_irq_reg(d, d->chip->wake_base, i); if (chip->wake_invert) ret = regmap_irq_update_bits(d, reg, @@ -786,8 +812,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (chip->num_type_reg && !chip->type_in_mask) { for (i = 0; i < chip->num_type_reg; ++i) { - reg = chip->type_base + - (i * map->reg_stride * d->type_reg_stride); + reg = sub_irq_reg(d, d->chip->type_base, i); ret = regmap_read(map, reg, &d->type_buf_def[i]); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 2cc4ecd..18910bd 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1378,6 +1378,9 @@ struct regmap_irq_sub_irq_map { * status_base. Should contain num_regs arrays. * Can be provided for chips with more complex mapping than * 1.st bit to 1.st sub-reg, 2.nd bit to 2.nd sub-reg, ... + * When used with not_fixed_stride, each one-element array + * member contains offset calculated as address from each + * peripheral to first peripheral. * @num_main_regs: Number of 'main status' irq registers for chips which have * main_status set. * @@ -1404,6 +1407,9 @@ struct regmap_irq_sub_irq_map { * @clear_on_unmask: For chips with interrupts cleared on read: read the status * registers before unmasking interrupts to clear any bits * set when they were masked. + * @not_fixed_stride: Used when chip peripherals are not laid out with fixed + * stride. Must be used with sub_reg_offsets containing the + * offsets to each peripheral. * @runtime_pm: Hold a runtime PM lock on the device when accessing it. * * @num_regs: Number of registers in each control bank. @@ -1450,6 +1456,7 @@ struct regmap_irq_chip { bool type_invert:1; bool type_in_mask:1; bool clear_on_unmask:1; + bool not_fixed_stride:1; int num_regs;
Qualcomm's MFD chips have a top level interrupt status register and sub-irqs (peripherals). When a bit in the main status register goes high, it means that the peripheral corresponding to that bit has an unserviced interrupt. If the bit is not set, this means that the corresponding peripheral does not. Commit a2d21848d9211d ("regmap: regmap-irq: Add main status register support") introduced the sub-irq logic that is currently applied only when reading status registers, but not for any other functions like acking or masking. Extend the use of sub-irq to all other functions, with two caveats regarding the specification of offsets: - Each member of the sub_reg_offsets array should be of length 1 - The specified offsets should be the unequal strides for each sub-irq device. In QCOM's case, all the *_base registers are to be configured to the base addresses of the first sub-irq group, with offsets of each subsequent group calculated as a difference from these addresses. Continuing from the example mentioned in the cover letter: /* * Address of MISC_INT_MASK = 0x1011 * Address of TEMP_ALARM_INT_MASK = 0x2011 * Address of GPIO01_INT_MASK = 0x3011 * * Calculate offsets as: * offset_0 = 0x1011 - 0x1011 = 0 (to access MISC's * registers) * offset_1 = 0x2011 - 0x1011 = 0x1000 * offset_2 = 0x3011 - 0x1011 = 0x2000 */ static unsigned int sub_unit0_offsets[] = {0}; static unsigned int sub_unit1_offsets[] = {0x1000}; static unsigned int sub_unit2_offsets[] = {0x2000}; static struct regmap_irq_sub_irq_map chip_sub_irq_offsets[] = { REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets), }; static struct regmap_irq_chip chip_irq_chip = { --------8<-------- .not_fixed_stride = true, .mask_base = MISC_INT_MASK, .type_base = MISC_INT_TYPE, .ack_base = MISC_INT_ACK, .sub_reg_offsets = chip_sub_irq_offsets, --------8<-------- }; Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> --- drivers/base/regmap/regmap-irq.c | 81 ++++++++++++++++++++++++++-------------- include/linux/regmap.h | 7 ++++ 2 files changed, 60 insertions(+), 28 deletions(-)