Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

From: Matti Vaittinen
Date: Wed Jun 06 2018 - 03:35:09 EST

On Tue, Jun 05, 2018 at 10:46:14AM -0500, Rob Herring wrote:
> On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen
> <mazziesaccount@xxxxxxxxx> wrote:
> > On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote:
> >> On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen
> >> <mazziesaccount@xxxxxxxxx> wrote:
> >> > On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> >> >> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> >> >> <mazziesaccount@xxxxxxxxx> wrote:
> >> >> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> >> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> >> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> >> >> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> >> >> >> > > + - interrupts : The interrupt line the device is connected to.
> >> >> >> > > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> >> >> >
> >> >> >> > What sub blocks have interrupts?
> >> >> >>
> >> >> >> The PMIC can generate interrupts from events which cause it to reset.
> >> >> >> Eg, irq from watchdog line change, power button pushes, reset request
> >> >> >> via register interface etc. I don't know any generic handling for these
> >> >> >> interrupts. In "normal" use-case this PMIC is powering the processor
> >> >> >> where driver is running and I do not see reasonable handling because
> >> >> >> power-reset is going to follow the irq.
> >> >> >>
> >> >> >
> >> >> > Oh, but when reading this I understand that the interrupt-controller
> >> >> > property should at least be optional.
> >> >>
> >> >> I don't think it should. The h/w either has an interrupt controller or
> >> >> it doesn't.
> >> >
> >> > I hope this explains why I did this interrupt controller - please tell
> >> > me if this is legitimate use-case and what you think of following:
> >> >
> >> > +Optional properties:
> >> > + - interrupt-controller : Marks the device node as an interrupt controller.
> >> > + BD71837MWV can report different power state change
> >> > + events to other devices. Different events can be seen
> >> > + as separate BD71837 domain interrupts.
> >>
> >> To what other devices?
> >
> > Would it be better if I wrote "other drivers" instead? I think I've seen
> > examples where MFD driver is just providing the interrupts for other
> > drivers - like power-button input driver. Currently I have no such "irq
> > consumer" drivers written. Still I would like to expose these interrupts
> > so that they are ready for using if any platform using PMIC needs them.
>
> No, worse. Interrupt binding describes interrupt connections between a
> controller and devices (which could be sub-blocks in a device), not to
> drivers.

Ok.

> I'm just curious as to what sub-blocks/devices there are. You don't
> have to have a driver (yet) to define the devices.

Right. I should have done this from the start. I just thought everyone
is busy with other things and pushing people to read data sheets might
be considered as lazines. "Go and read from data sheet what my driver
does, I am too lazy to try to explain what I am doing" - type of thing
you know... But as people asked me for more information about HW:

Datasheet:
https://www.rohm.com/datasheet/BD71837MWV/bd71837mwv-e
(Would it be good idea to add this link to comments in MFD driver or to
binding document(s)?) Page 8 contains roughly the same picture I drew
below. Page 69 shows the interrupt registers. And for interested ones,
HW state transitions are described on page 24.

+--------------------------------------------------+
| |
VSYS +-----------------+ +-----------+ |
| | | | |
| +-------+ | | BUCKS 1-4 +-------->
| | | | | | |
I2C IF +-------> H | +--------------------+ DVS +-------->
| | O | | | Support | |
PWRON_B +-------> S | | | +-------->
| | T | | | | |
PMIC_STBY_REQ +-------> | | | +-------->
| | I | | | | |
PMIC_ON_REQ +-------> / | | +-----------+ |
| | F | | |
WDOG_B +-------> | | +-----------+ |
| | | | | +-------->
| | | +--------------------+ BUCKS 5,8 | |
| | | | | +-------->
| | | | +-----------+ |
| | | | |
| | | | +----------+ |
IRQ_OUT <-------+ | | | | |
| | | +---------------------+ LDO1 +-------->
C32K_OUT <-------+ | | | | |
| | | | +----------+ |
| | | | |
| | | | +----------+ |
| | | | | | |
| | | +---------------------+ LDO2 +-------->
| | | | | | |
| | | | +----------+ |
| | | | |
| | | | +----------+ |
| | | | | | |
| | | +---------------------+ LDO7 +-------->
| +-------+ | | | |
| | +----------+ |
| | |
| | +----------+ +-------------------------->
| | | | | |
| +-+ BUCK6 +-+ +----------+ |
| | | | | | | |
| | +----------+ +------> LDO5 +-------->
| | | | | |
| | | +----------+ |
| | | |
| +-------+ | | +----------+ |
| | | | o | | |
XIN +---------+ 32K | | \-----> LDO3 +-------->
| |Crystal| +--------------o | | |
| |Driver | | +---------+ +----------+ |
XOUT +---------+ | | | | |
| | | +--+ BUCK7 +-+-------------------------->
| +-------+ | | | | |
| | +---------+ | +-----------+ |
| | | | | |
| | +------> LDO6 +------->
| | | | | |
| | | +-----------+ |
| | | |
| | | +-----------+ |
| | o | | |
| | \-----> LDO4 +------->
| +--------------o | | |
| +-----------+ |
| |
+--------------------------------------------------+

On the left we see input lines to PMIC. PWRON_B intended to be connected
to power button. PMIC_STBY_REQ and PMIC_ON_REQ lines for controlling HW
state machine PMIC has. And WDOG_B from watch dog.

PMIC has control register for controlling what happens to BUCK/LDO
outputs when input line states change. PMIC reports change in input
lines via the IRQ_OUT line and IRQ status register.

So HW mapping for interrup(s) from PMIC would be:

(HW) event => BD71837 domain IRQ

PMIC_STBY_REQ line level change => 0
PMIC_ON_REQ line level change => 1
PMIC_WDOG_B line level change => 2
PMIC_PWRON_B line level change => 3
PMIC_PWRON_B line/long push detected => 4
PMIC_PWRON_B line/short push detected => 5
SWRESET register on PMIC written => 6

> > "The BD71837 driver only provides the infrastructure for the IRQs. The
> > users can write his own driver to convert the IRQ into the event they
> > wish. The IRQ can be used with the standard
> > request_irq/enable_irq/disable_irq API inside the kernel." (I found this
> > text from NXP forums and ruthlessly copied and modified it over here)
>
> That's all OS details that have nothing to do with the binding. The
> binding describes the h/w.

Right. I'll drop it.

>
> > If this is not feasible, then I will remove the irq handling from MFD
> > (or leave code there but remove the binding information?) as I don't
> > know what the irq handles should do in generic case.
>
> I don't understand what you mean by generic. An IRQ has to be wired to
> something. The only generic IRQs are GPIOs.

By generic case I mean for example when PMIC_WDOG_B line changes. In
example use-case I have seen, this would be cutting the power from
processor. But this is not necessarily the case. This can be configured
from PMIC register so that action can be warm or cold reset, or no
action. Finally, I'd rather not expect that the BUCKs are supplying
power to processor which is controlling the PMIC. Thus I do not know how
to do generic _handler_ for these interrupts.

So from PMIC HW point of view I know that the interrupt is tied to
PMIC_WDOG_B line change. And this can be described in binding document.
(I tried doing this to v5 patch). Still from system/SW point of view I
don't know what action should be taken (or by which driver) when such
change happens. Hence I liked the idea of hiding the irq register
details in MFD driver by declaring it as interrupt controller - and
leaving the interrupts to be used by what ever drivers need the change
information is system the PMIC is used.

> >> > + - #interrupt-cells : The number of cells to describe an IRQ should be 1.
> >> > + The first cell is the IRQ number.
> >> > + masks from ../interrupt-controller/interrupts.txt.
> >
> > Sorry this "masks from ../interrupt-controller/interrupts.txt." was
> > accidentally pasted here. I should have deleted it.
> >
> >> I'm still not clear. Generally in a PMIC, you'd define an interrupt
> >> controller when there's a common set of registers to manage sub-block
> >> interrupts (typical mask/unmask, ack regs) and the subblocks
> >> themselves have control of masking/unmasking interrupts. If there's
> >> not a need to have these 2 levels of interrupt handling, then you
> >> don't really need to define an interrupt controller.
> >
> > And to clarify - the PMIC can generate irq via one irq line. This is
> > typical ACTIVE_LOW irq with 8 bit "write 1 to clear" status register and
> > 8 bit mask register. The role of interrupt-controller code here is just
> > to allow these 8 irq reasons to be seen as individual BD71837 domain
> > interrupts. I just don't have the driver(s) for handling these
> > interrupts.
>
> If what I'm asking for above is still not clear, what are the 8 bits
> defined as or what are those 8 lines connected to?

I am sorry - there were only 7 bits. One bit was unused. I hope my
explanation abowe did clarify this.

> > Or should I just
> > somehow state that irq X in BD71837 is a "power button short push"
> > event and power button driver should be the consumer for it?
>
> Yes, at least, but who is the consumer is an OS detail that is not
> relevant to the binding. Ideally, you would describe the node with the
> interrupts property for "irq X".

I think I need to try changing my mind set. I tend to think the DT nodes
are for drivers so that drivers can get the information they need. An as
I don't know what kind of driver would be handling the irq, I don't know
what kind of DT node would be good for it. Hence I would rather leave
constructing the node who consumes the IRQ to someone who knows what
they want to do with this IRQ information.

>
> > Rest of the interrupts are not so obvious. I have no idea how I should
> > handle rest of the interrupts. Those are interrupts which cause the PMIC
> > to reset and cut the powers from most of the regulators too. I can
> > easily think setup where one processor is controlling PMIC which powers
> > for the other processor. And getting IRQ if for example watchdog reset
> > the other processor would probably be very usefull. But doing any
> > 'de-facto' handler for this is hard. Only generally usefull thing would
> > be notifying the user-space but I don't think I should invent any new
> > kernelspace - userspace interfaces for this. I believe that when such
> > are needed those should be implemented by ones knowing the platform.
>
> Don't think about the OS or driver details. Think about sub-blocks of
> the hardware and the connections between them (like irqs) and to board
> that need to be described in DT.
>
> If you can't describe that, then you just probably shouldn't have
> sub-nodes in DT (ever).

This is why I did not add any "irq consumer" nodes in example DT. But I
believe someone can think of a board/setup where such are needed. Thus I
liked the idea of providing the interrupt-controller.

> >
> > So please bear with me but do you mean I should
> > a) document what conditions generate which IRQ
> > or
> > b) should I tell what kind of driver is needed for handling the IRQs
> > or
> > c) should I first write code using IRQs before addinf MFD binding?
>
> A.


So what do you think of this:

+Optional properties:
+ - interrupt-controller : Marks the device node as an interrupt controller.
+ BD71837MWV can report input line state change and SW
+ reset events via interrupts. Different events can be seen
+ as separate BD71837 domain interrupts.
+ - #interrupt-cells : The number of cells to describe an IRQ should be 1.
+ The value in cell is the IRQ number.
+ Meaningfull numbers are:
+ 0 => PMIC_STBY_REQ level change
+ 1 => PMIC_ON_REQ level change
+ 2 => WDOG_B level change
+ 3 => Power Button level change
+ 4 => Power Button Long Push
+ 5 => Power Button Short Push
+ 6 => SWRESET register is written 1

Would this be getting closer to what is needed from binding document?

Oh, and thanks for the patience =)

Br,
Matti Vaittinen