mbox series

[v2,0/3] Add support for Qualcomm MFD PMIC register layout

Message ID cover.1603402280.git.gurus@codeaurora.org
Headers show
Series Add support for Qualcomm MFD PMIC register layout | expand


Guru Das Srinagesh Oct. 22, 2020, 9:35 p.m. UTC
Changes from v1:
- Fixed YAML errors in dt binding document.

This is a follow-up as promised [1] to the earlier attempts [2] [3] to upstream
the driver that has been hitherto used to handle IRQs for Qualcomm's PMICs that
have multiple on-board peripherals when they are interfaced over the I2C

This series is a rewrite of that driver while making use of the regmap-irq
framework, which needs some modifications to handle the register layout of
Qualcomm's PMICs. This is an RFC because I would like to get feedback on my
general approach before submitting as a patch per se.

Upon inspection of the regmap-irq framework, it was observed that the
downstream driver was essentially replicating the framework's IRQ handling
logic (such as adding an IRQ domain, and the interrupt handler thread that
reads sub-irqs from a main status register). It was also observed that the
framework could not be used as-is because:
- Qualcomm's PMIC peripheral register layout does not follow a fixed
  irq_reg_stride, and
- The "IRQ TYPE" configuration register takes one bit per interrupt, which when
  set configures that interrupt as Edge triggered, and when cleared sets it to
  Level triggered.
- There are two IRQ configuration registers in addition to "IRQ TYPE" that
  further configure the IRQ type as triggered by rising-edge/level high or
  alternatively, falling-edge/level low that have no support in the regmap-irq
  framework currently.

This patch series has been tested on an internal platform using PM8008 as a
test MFD PMIC chip. PM8008 is a PMIC that contains 7 LDOs, 2 GPIOs, temperature
monitoring, and can be interfaced over I2C.

Both the framework modifications as well as the chip driver
have been submitted here for review. Some details about the specific
differences between the framework and QCOM PMICs' register layout are provided
below using PM8008 as an example.

[PM8008 peripheral register layout]

Of all the peripherals in PM8008, only a few need IRQ support. They are laid
out at the following base addresses (only four are added at the moment for

	0x0900, 0x2400, 0xC000, 0xC100

Each peripheral is allocated a uniform size of 0x100 bytes and its IRQs are
configured through a set of registers that are located at fixed offsets from
the above base addresses, uniformly:

	Register name	       Addr	regmap-irq equivalent	Comment
	INT_RT_STS_OFFSET      0x10	(no equivalent)		See #1 below
	INT_SET_TYPE_OFFSET    0x11	type_base 		See #2 below
	INT_POL_HIGH_OFFSET    0x12	(no equivalent)		See #3 below
	INT_POL_LOW_OFFSET     0x13	(no equivalent)		See #3 below
	INT_EN_SET_OFFSET      0x15	unmask_base		See #4 below
	INT_EN_CLR_OFFSET      0x16	mask_base		See #4 below
	INT_LATCHED_STS_OFFSET 0x18	status_base

Comments (all registers are one bit per interrupt):
1. INT_RT_STS_OFFSET is not used by the regmap-irq, so it may be ignored.
2. INT_SET_TYPE_OFFSET: 1 for edge trigger, 0 for level trigger.
3. Support needs to be added for writing to INT_POL_HIGH_OFFSET and
   INT_POL_LOW_OFFSET correctly in the framework. Set to 1 or 0 to enable or
   disable rising-edge/level high or falling-edge/level low.
4. Even though INT_EN_SET_OFFSET and INT_EN_CLR_OFFSET map to unmask_base and
   mask_base in the regmap-irq framework conceptually, they are swapped in the
   chip driver because `unmask_offset` in the framework expects unmask_base to
   be larger than mask_base numerically. This has to be kept in mind while
   reviewing the "mfd: Add PM8008 driver" patch below.

[Summary of framework changes]

The main thrust of the changes is to introduce an array of peripheral offset
values, which are to be added to the *_base addresses in order to arrive at the
correct register addresses per peripheral. In order to get at the first
peripheral's addresses, the first element of this array must be zero.

Since there are two new registers (INT_POL_HIGH_OFFSET and INT_POL_LOW_OFFSET),
add support for storing the per-peripheral values and also writing to them.
These will be used only if peripheral offsets are specified.

[1] https://lore.kernel.org/lkml/20200519185757.GA13992@codeaurora.org/
[2] https://lore.kernel.org/lkml/cover.1588037638.git.gurus@codeaurora.org/
[3] https://lore.kernel.org/lkml/cover.1588115326.git.gurus@codeaurora.org/

Guru Das Srinagesh (3):
  regmap-irq: Add support for peripheral offsets
  dt-bindings: mfd: Add QCOM PM8008 MFD bindings
  mfd: Add PM8008 driver

 .../bindings/mfd/qcom,pm8008-irqchip.yaml          | 102 +++++++++++
 drivers/base/regmap/regmap-irq.c                   | 191 ++++++++++++++++----
 drivers/mfd/Kconfig                                |  14 ++
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/qcom-pm8008.c                          | 197 +++++++++++++++++++++
 include/linux/regmap.h                             |   6 +
 6 files changed, 477 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008-irqchip.yaml
 create mode 100644 drivers/mfd/qcom-pm8008.c


Mark Brown Nov. 12, 2020, 7:33 p.m. UTC | #1
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.