| Message ID | 40581a58bd16442f03db1abea014ca1eedc94d3c.1603402280.git.gurus@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Qualcomm MFD PMIC register layout | expand |
On Thu, Oct 22, 2020 at 02:35:40PM -0700, Guru Das Srinagesh wrote: > Some MFD chips do not have the register space for their peripherals > mapped out with a fixed stride. Add peripheral address offsets to the > framework to support such address spaces. > In this new scheme, the regmap-irq client registering with the framework > shall have to define the *_base registers (e.g. status_base, mask_base, > type_base, etc.) as those of the very first peripheral in the chip, and > then specify address offsets of each subsequent peripheral so that their > corresponding *_base registers may be calculated by the framework. The > first element of the periph_offs array must be zero so that the first > peripherals' addresses may be accessed. > Some MFD chips define two registers in addition to the IRQ type > registers: POLARITY_HI and POLARITY_LO, so add support to manage their > data as well as write to them. It is difficult to follow what this change is supposed to do, in part because it looks like this is in fact two separate changes, one adding the _base feature and another adding the polarity feature. These should each be in a separate patch if that is the case, and I think each needs a clearer changelog - I'm not entirely sure what the polarity feature is supposed to do. Nothing here says what POLARITY_HI and POLARITY_LO are, how they interact or anything. For the address offsets I'm not sure that this is the best way to represent things. It looks like the hardware this is trying to describe is essentially a bunch of separate interrupt controllers that happen to share an upstream interrupt and I think that the code would be a lot clearer if at least the implementation looked like this. Instead of having to check for this array of offsets at every use point (which is going to be rarely used and hence prone to bugs) we'd have a set of separate regmap-irqs and then we'd mostly only have to loop through them on handling, the bulk of the implementation wouldn't have to worry about this special case. Historically genirq didn't support sharing threaded interrupts, if that's not changed we'd need to open code everything inside regmap-irq but it would be doable, or ideally genirq could grow this feature. If it's done inside regmap you'd have a separate API that took an array of regmap-irq configurations instead of just one and then when an interrupt is delivered just loops through all of them handling it. A quick scan through the interrupt code suggests it might be able to cope with shared IRQs now though which would make life easier.
Hi Mark, Sorry for the delay in my response. On Thu, Nov 12, 2020 at 07:33:12PM +0000, Mark Brown wrote: > It is difficult to follow what this change is supposed to do, in part > because it looks like this is in fact two separate changes, one adding > the _base feature and another adding the polarity feature. These should > each be in a separate patch if that is the case, and I think each needs > a clearer changelog - I'm not entirely sure what the polarity feature is > supposed to do. Nothing here says what POLARITY_HI and POLARITY_LO are, > how they interact or anything. Sure, I can split this into two patches for easier review. The POLARITY_HI and POLARITY_LO registers were described very briefly in the cover letter. If an interrupt is already configured as either edge- or level-triggered, setting the corresponding bit for it in the POLARITY_HI register further configures it as rising-edge or level-high triggered (as the case may be), while setting the same bit in POLARITY_LO further configures it as falling-edge or level-low triggered. I could certainly add this information to the commit message as well. > > For the address offsets I'm not sure that this is the best way to > represent things. It looks like the hardware this is trying to describe > is essentially a bunch of separate interrupt controllers that happen to > share an upstream interrupt Sorry but isn't this essentially the same as what the framework already knows as the "sub-irq" concept, with the key difference that the register stride is not fixed? Everything else is the same (except for a couple of minor points noted below) - a main IRQ register that indicates sub-irq blocks that have unhandled interrupts, as well as interrupt handling and servicing. The two minor differences are: - type_buf handling in regmap_irq_set_type() for IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW - Two extra registers: POLARITY_HI and POLARITY_LO > clearer if at least the implementation looked like this. Instead of > having to check for this array of offsets at every use point (which is > going to be rarely used and hence prone to bugs) Well, using irq_reg_stride already does exactly this - calculating the right register to access at every use point, as an offset from the _base register (status, ack, type, et c.). Peripheral offsets would just be another way of calculating the right register, that's all. And we could have a macro as well. > we'd have a set of separate regmap-irqs and then we'd mostly only have > to loop through them on handling, the bulk of the implementation > wouldn't have to worry about this special case. > > Historically genirq didn't support sharing threaded interrupts, if > that's not changed we'd need to open code everything inside regmap-irq > but it would be doable, or ideally genirq could grow this feature. If > it's done inside regmap you'd have a separate API that took an array of > regmap-irq configurations instead of just one and then when an interrupt > is delivered just loops through all of them handling it. A quick scan > through the interrupt code suggests it might be able to cope with shared > IRQs now though which would make life easier. Sure, I can look into how this approach would look like, but given that the QCOM register arrangement of main vs sub-irq is essentially the same as what the framework currently understands, couldn't we simply have a macro to change the way the right register offset is calculated (irq_reg_stride vs. peripheral offsets)? Also, could you elaborate more on the genirq route? I'm not sure where to start looking to evaluate one route vs the other. Thank you. Guru Das.
On Thu, Mar 04, 2021 at 10:27:35AM -0800, Guru Das Srinagesh wrote: > On Thu, Nov 12, 2020 at 07:33:12PM +0000, Mark Brown wrote: > > supposed to do. Nothing here says what POLARITY_HI and POLARITY_LO are, > > how they interact or anything. > The POLARITY_HI and POLARITY_LO registers were described very briefly in > the cover letter. If an interrupt is already configured as either edge- > or level-triggered, setting the corresponding bit for it in the > POLARITY_HI register further configures it as rising-edge or level-high > triggered (as the case may be), while setting the same bit in > POLARITY_LO further configures it as falling-edge or level-low > triggered. I could certainly add this information to the commit message > as well. So this is just a trigger type control that's in two discontiguous bits possibly in different registers or something? This doesn't sound like anything generic with the API you're describing, if that's what it is the interface should also handle things like four bits (one for each type) or having the different values mapped differently within the two bits that are spread out (eg, you could have one bit for polarity and another for edge/level). > > For the address offsets I'm not sure that this is the best way to > > represent things. It looks like the hardware this is trying to describe > > is essentially a bunch of separate interrupt controllers that happen to > > share an upstream interrupt > Sorry but isn't this essentially the same as what the framework already knows as > the "sub-irq" concept, with the key difference that the register stride > is not fixed? Everything else is the same (except for a couple of minor > points noted below) - a main IRQ register that indicates sub-irq blocks > that have unhandled interrupts, as well as interrupt handling and > servicing. Like I said in my original review it is extremely hard to tell from your patch what you are trying to implement, and it's now been more than four months since you sent it which isn't helping anything. This means it is also extremely hard to tell if the patch is doing the same thing as sub_irq. IIRC it appeared that there was no top level interrupt status register, the point with sub_irq is that we don't need to read all the status registers because there's a top level status register which says which of them have signals in them (including the possibility that more than one bit in the top level status might indicate the same leaf status register). If the device doesn't have that it doesn't have sub_irqs. Note that sub_irq only affects status register reads, it doesn't affect other things like acking or masking. On the other hand if this *is* the same thing as sub_irqs then why is it completely separate code and not sharing anything with it? As I said at the time you need to split this into at least two distinct patches with clear changelogs which explain what they are trying to implement, I don't think it's useful to discuss this further without that. I can't give you any clearer advice on how to implement whatever you are trying to implement without having some idea of what that is. > > clearer if at least the implementation looked like this. Instead of > > having to check for this array of offsets at every use point (which is > > going to be rarely used and hence prone to bugs) > Well, using irq_reg_stride already does exactly this - calculating the > right register to access at every use point, as an offset from the _base > register (status, ack, type, et c.). Peripheral offsets would just be > another way of calculating the right register, that's all. And we could > have a macro as well. The stride code is executed in all paths, it doesn't add conditional statements all over the place. This helps a lot, we know it's being run all the time as opposed to being a lot of separate code paths that are rarely run - the case without a stride is just a stride of 1. > Sure, I can look into how this approach would look like, but given that > the QCOM register arrangement of main vs sub-irq is essentially the same > as what the framework currently understands, couldn't we simply have a > macro to change the way the right register offset is calculated > (irq_reg_stride vs. peripheral offsets)? I'm not sure macros all over the place is going to be clearer than conditional statements all over the place. As with what you were saying about sub_irq if you think the two things are equivalent then why is one not implemented in terms of the other rather than doing conditional code on every single use? > Also, could you elaborate more on the genirq route? I'm not sure where > to start looking to evaluate one route vs the other. Register a separate regmap-irq for each of these perhiperals in your list, using the same parent interrupt for all of them and setting IRQF_SHARED. They will then be handled like any other shared interrupt, if the parent interrupt fires then genirq will go through and call each of the handlers until one reports that it handled an interrupt.
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index ad5c2de..dbf2c86 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -38,6 +38,8 @@ struct regmap_irq_chip_data { unsigned int *wake_buf; unsigned int *type_buf; unsigned int *type_buf_def; + unsigned int *polarity_hi_buf; + unsigned int *polarity_lo_buf; unsigned int irq_reg_stride; unsigned int type_reg_stride; @@ -87,8 +89,13 @@ 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); + if (d->chip->periph_offs) + reg = d->chip->status_base + + d->chip->periph_offs[i]; + else + reg = d->chip->status_base + + (i * map->reg_stride * + d->irq_reg_stride); ret = regmap_read(map, reg, &val); if (ret) @@ -108,8 +115,13 @@ 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); + if (d->chip->periph_offs) + reg = d->chip->mask_base + + d->chip->periph_offs[i]; + else + reg = d->chip->mask_base + + (i * map->reg_stride * d->irq_reg_stride); + if (d->chip->mask_invert) { ret = regmap_irq_update_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); @@ -136,8 +148,13 @@ 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); + if (d->chip->periph_offs) + reg = d->chip->wake_base + + d->chip->periph_offs[i]; + else + reg = d->chip->wake_base + + (i * map->reg_stride * d->irq_reg_stride); + if (d->wake_buf) { if (d->chip->wake_invert) ret = regmap_irq_update_bits(d, reg, @@ -161,8 +178,14 @@ 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); + if (d->chip->periph_offs) + reg = d->chip->ack_base + + d->chip->periph_offs[i]; + else + reg = d->chip->ack_base + + (i * map->reg_stride * + d->irq_reg_stride); + /* some chips ack by write 0 */ if (d->chip->ack_invert) ret = regmap_write(map, reg, ~d->mask_buf[i]); @@ -187,8 +210,14 @@ 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); + if (d->chip->periph_offs) + reg = d->chip->type_base + + d->chip->periph_offs[i]; + else + reg = d->chip->type_base + + (i * map->reg_stride * + d->type_reg_stride); + if (d->chip->type_invert) ret = regmap_irq_update_bits(d, reg, d->type_buf_def[i], ~d->type_buf[i]); @@ -198,6 +227,25 @@ static void regmap_irq_sync_unlock(struct irq_data *data) if (ret != 0) dev_err(d->map->dev, "Failed to sync type in %x\n", reg); + + if (!d->chip->periph_offs || + !d->chip->polarity_hi_base || + !d->chip->polarity_lo_base) + continue; + + reg = d->chip->polarity_hi_base + + d->chip->periph_offs[i]; + ret = regmap_write(map, reg, d->polarity_hi_buf[i]); + if (ret != 0) + dev_err(d->map->dev, "Failed to sync polarity hi in %x\n", + reg); + + reg = d->chip->polarity_lo_base + + d->chip->periph_offs[i]; + ret = regmap_write(map, reg, d->polarity_lo_buf[i]); + if (ret != 0) + dev_err(d->map->dev, "Failed to sync polarity lo in %x\n", + reg); } } @@ -280,23 +328,49 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type) switch (type) { case IRQ_TYPE_EDGE_FALLING: d->type_buf[reg] |= t->type_falling_val; + if (d->chip->periph_offs) { + d->polarity_hi_buf[reg] &= ~t->type_falling_val; + d->polarity_lo_buf[reg] |= t->type_falling_val; + } break; case IRQ_TYPE_EDGE_RISING: d->type_buf[reg] |= t->type_rising_val; + if (d->chip->periph_offs) { + d->polarity_hi_buf[reg] |= t->type_rising_val; + d->polarity_lo_buf[reg] &= ~t->type_rising_val; + } break; case IRQ_TYPE_EDGE_BOTH: d->type_buf[reg] |= (t->type_falling_val | t->type_rising_val); + if (d->chip->periph_offs) { + d->polarity_hi_buf[reg] |= (t->type_falling_val | + t->type_rising_val); + d->polarity_lo_buf[reg] |= (t->type_falling_val | + t->type_rising_val); + } break; case IRQ_TYPE_LEVEL_HIGH: - d->type_buf[reg] |= t->type_level_high_val; + if (!d->chip->periph_offs) { + d->type_buf[reg] |= t->type_level_high_val; + } else { + d->type_buf[reg] &= ~t->type_level_high_val; + d->polarity_hi_buf[reg] |= t->type_level_high_val; + d->polarity_lo_buf[reg] &= ~t->type_level_high_val; + } break; case IRQ_TYPE_LEVEL_LOW: - d->type_buf[reg] |= t->type_level_low_val; + if (!d->chip->periph_offs) { + d->type_buf[reg] |= t->type_level_low_val; + } else { + d->type_buf[reg] &= ~t->type_level_low_val; + d->polarity_hi_buf[reg] &= ~t->type_level_low_val; + d->polarity_lo_buf[reg] |= t->type_level_low_val; + } break; default: return -EINVAL; @@ -342,12 +416,10 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, struct regmap_irq_sub_irq_map *subreg; int i, ret = 0; - if (!chip->sub_reg_offsets) { - /* Assume linear mapping */ - ret = regmap_read(map, chip->status_base + - (b * map->reg_stride * data->irq_reg_stride), - &data->status_buf[b]); - } else { + if (chip->periph_offs) { + ret = regmap_read(map, chip->status_base + chip->periph_offs[b], + &data->status_buf[b]); + } else if (chip->sub_reg_offsets) { subreg = &chip->sub_reg_offsets[b]; for (i = 0; i < subreg->num_regs; i++) { unsigned int offset = subreg->offset[i]; @@ -357,6 +429,11 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, if (ret) break; } + } else { + /* Assume linear mapping */ + ret = regmap_read(map, chip->status_base + + (b * map->reg_stride * data->irq_reg_stride), + &data->status_buf[b]); } return ret; } @@ -474,10 +551,14 @@ 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]); + if (chip->periph_offs) + reg = chip->status_base + chip->periph_offs[i]; + else + reg = chip->status_base + + (i * map->reg_stride * + data->irq_reg_stride); + + ret = regmap_read(map, reg, &data->status_buf[i]); if (ret != 0) { dev_err(map->dev, @@ -499,8 +580,13 @@ 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); + if (chip->periph_offs) + reg = chip->ack_base + chip->periph_offs[i]; + else + reg = chip->ack_base + + (i * map->reg_stride * + data->irq_reg_stride); + if (chip->ack_invert) ret = regmap_write(map, reg, ~data->status_buf[i]); @@ -662,6 +748,18 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; } + if (chip->periph_offs) { + d->polarity_hi_buf = kcalloc(chip->num_regs, + sizeof(unsigned int), GFP_KERNEL); + if (!d->polarity_hi_buf) + goto err_alloc; + + d->polarity_lo_buf = kcalloc(chip->num_regs, + sizeof(unsigned int), GFP_KERNEL); + if (!d->polarity_lo_buf) + goto err_alloc; + } + d->irq_chip = regmap_irq_chip; d->irq_chip.name = chip->name; d->irq = irq; @@ -700,8 +798,12 @@ 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); + if (chip->periph_offs) + reg = chip->mask_base + chip->periph_offs[i]; + else + reg = chip->mask_base + + (i * map->reg_stride * d->irq_reg_stride); + if (chip->mask_invert) ret = regmap_irq_update_bits(d, reg, d->mask_buf[i], ~d->mask_buf[i]); @@ -725,8 +827,12 @@ 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); + if (chip->periph_offs) + reg = chip->status_base + chip->periph_offs[i]; + else + reg = chip->status_base + + (i * map->reg_stride * d->irq_reg_stride); + 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 +841,13 @@ 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); + if (chip->periph_offs) + reg = chip->ack_base + chip->periph_offs[i]; + else + reg = chip->ack_base + + (i * map->reg_stride * + d->irq_reg_stride); + if (chip->ack_invert) ret = regmap_write(map, reg, ~(d->status_buf[i] & d->mask_buf[i])); @@ -765,8 +876,12 @@ 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); + if (chip->periph_offs) + reg = chip->wake_base + chip->periph_offs[i]; + else + reg = chip->wake_base + + (i * map->reg_stride * + d->irq_reg_stride); if (chip->wake_invert) ret = regmap_irq_update_bits(d, reg, @@ -786,8 +901,12 @@ 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); + if (chip->periph_offs) + reg = chip->type_base + chip->periph_offs[i]; + else + reg = chip->type_base + + (i * map->reg_stride * + d->type_reg_stride); ret = regmap_read(map, reg, &d->type_buf_def[i]); @@ -833,6 +952,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, /* Should really dispose of the domain but... */ err_alloc: kfree(d->type_buf); + kfree(d->polarity_hi_buf); + kfree(d->polarity_lo_buf); kfree(d->type_buf_def); kfree(d->wake_buf); kfree(d->mask_buf_def); @@ -903,6 +1024,8 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d) irq_domain_remove(d->domain); kfree(d->type_buf); + kfree(d->polarity_hi_buf); + kfree(d->polarity_lo_buf); kfree(d->type_buf_def); kfree(d->wake_buf); kfree(d->mask_buf_def); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index e7834d9..6fb1090 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1338,6 +1338,7 @@ 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, ... + * @periph_offs: Array of addresses of peripherals, should be num_reg long. * @num_main_regs: Number of 'main status' irq registers for chips which have * main_status set. * @@ -1350,6 +1351,8 @@ struct regmap_irq_sub_irq_map { * Using zero value is possible with @use_ack bit. * @wake_base: Base address for wake enables. If zero unsupported. * @type_base: Base address for irq type. If zero unsupported. + * @polarity_hi_base: Base address for polarity high when periph_offs is used. + * @polarity_lo_base: Base address for polarity low when periph_offs is used. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. * @mask_invert: Inverted mask register: cleared bits are masked out. @@ -1390,6 +1393,7 @@ struct regmap_irq_chip { unsigned int main_status; unsigned int num_main_status_bits; struct regmap_irq_sub_irq_map *sub_reg_offsets; + unsigned int *periph_offs; int num_main_regs; unsigned int status_base; @@ -1398,6 +1402,8 @@ struct regmap_irq_chip { unsigned int ack_base; unsigned int wake_base; unsigned int type_base; + unsigned int polarity_hi_base; + unsigned int polarity_lo_base; unsigned int irq_reg_stride; bool mask_writeonly:1; bool init_ack_masked:1;
Some MFD chips do not have the register space for their peripherals mapped out with a fixed stride. Add peripheral address offsets to the framework to support such address spaces. In this new scheme, the regmap-irq client registering with the framework shall have to define the *_base registers (e.g. status_base, mask_base, type_base, etc.) as those of the very first peripheral in the chip, and then specify address offsets of each subsequent peripheral so that their corresponding *_base registers may be calculated by the framework. The first element of the periph_offs array must be zero so that the first peripherals' addresses may be accessed. Some MFD chips define two registers in addition to the IRQ type registers: POLARITY_HI and POLARITY_LO, so add support to manage their data as well as write to them. Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> --- drivers/base/regmap/regmap-irq.c | 191 ++++++++++++++++++++++++++++++++------- include/linux/regmap.h | 6 ++ 2 files changed, 163 insertions(+), 34 deletions(-)