Message-ID: <20180607111218.GF20078@localhost.localdomain>
Date:   Thu, 7 Jun 2018 14:12:18 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        mikko.mutanen@...rohmeurope.com, heikki.haikola@...rohmeurope.com
Subject: Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM
 BD71837 PMIC

On Wed, Jun 06, 2018 at 10:16:37AM -0500, Rob Herring wrote:
> On Wed, Jun 6, 2018 at 2:34 AM, Matti Vaittinen
> <mazziesaccount@...il.com> wrote:
> > 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@...il.com> 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@...il.com> 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@...il.com> 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:
> > 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)?)
> 
> Yes, it would.

I will add this then. I don't really like adding links like this as urls
tend to change and links become dead - but I guess this is something we
just must live with.

> >                     +--------------------------------------------------+
> >                     |                                                  |
> > VSYS           +-----------------+                    +-----------+    |
> >                     |            |                    |           |    |
> >                     |  +-------+ |                    | BUCKS 1-4 +-------->
> >                     |  |       | |                    |           |    |
> > I2C IF         +------->   H   | +--------------------+  DVS      +-------->
> >                     |  |   O   | |                    |  Support  |    |
> > PWRON_B        +------->   S   | |                    |           +-------->
> >                     |  |   T   | |                    |           |    |
> > PMIC_STBY_REQ  +------->       | |                    |           +-------->

Do you think this ASCII art picture in MFD or regulator driver comments would
be an overkill?

> > 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 it looks like this is just regulators with a few other signals to handle.

Yes. Regulators and the HW state machine which is currently mostly
bypassed by drivers. And while we are at it - is there some standard
device-tree properties for describing the voltages for different PMIC states
(idle, run, standby) so that the driver could configure voltages the
bucks should use when PMIC state is changed. Or do you think I should
use custom ones like:

bd71837,pmic-buck1-dvs-voltage = <900000>, <850000>, <800000>; /* VDD_SOC: Run-Idle-Suspend */
bd71837,pmic-buck2-dvs-voltage = <1000000>, <900000>, <0>; /* VDD_ARM: Run-Idle */
bd71837,pmic-buck3-dvs-voltage = <1000000>, <0>, <0>; /* VDD_GPU: Run */
bd71837,pmic-buck4-dvs-voltage = <1000000>, <0>, <0>; /* VDD_VPU: Run */

I think this is not the only PMIC with configurable voltages for
different states so someone has probably already invented a way to
provide this information - is the DT correct place to search for this?

> 
> > 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
> 
> I'm not really clear how these would have s/w handling. They look like
> handshake signals for the processor to enter and exit standby/suspend.
> H/w designers don't always know what to do with things and may have
> just said lets have an interrupt for every input signal.

The PMIC_ON_REQ can be used to switch the PMIC from 'READY' to 'SNVS'
and from 'SNVS' to 'RUN' states. But same transitions can be configured
to be done by power button presses too.

I think I in some earlier mail said that I can't think how to handle
other but power button interrupts - when PMIC is used to power the
processor controlling PMIC. But I also mentioned another use case
which I can think of - but I am not sure if it is relevant or not. In
this other use case we have a 'slave processor' powered by PMIC doing
some specific tasks - and a 'control processor' which is not powered by
this PMIC - but which may be controlling it (and thus also controls power
for the slave). In such setup all these interrupts mmight become
meaningfull - but handling should be platform specific then. (The
control processor could for example detect slave processor resets or
wake-ups via these interrupts and initiate any platform specific
activities based on this). I have no system design experience so I don't
know if this sounds reasonable or not - but thats why I liked the idea
of providing access to all interrupts via this interrupt-controller.

> I'd think you
> would just want DT properties to configure what action to take (and
> perhaps polarity?).

Currently the driver is not configuring what happens when state of these
signals change. But it could as you say. And it would be nice to get the
configuration from DT. I think the polarity is fixed.


> > PMIC_WDOG_B line level change           => 2
> 
> Ah, this is an input signal, not a watchdog block within the PMIC. I
> think this should just be handled by the core driver. If you need to
> configure what this does, then we can add a property to handle that.

Action taken when WDOG_B is asserted is indeed configurable. PMIC can do
cold reset, warm reset of take no action. But here I again fail to think
how this should be handled - especially because HW default for action is
cold reset. But as I said abowe - it may be there is use case for this
interrupt even though I can't provide generic one. ...and this is why I
liked the idea of having it available via DT... :)

> 
> > PMIC_PWRON_B line level change          => 3
> > PMIC_PWRON_B line/long push detected    => 4
> > PMIC_PWRON_B line/short push detected   => 5
> 
> So you need a power button driver (or maybe not even a separate
> driver) for this, but I don't think that warrants a child node. I
> think some PMIC drivers just generate a power key press (which just
> gets punted to userspace), but some will do an actual power down. Or
> maybe a combination of those based on short/long press.

I agree with you on this. A power button driver could consume these
IRQs. And yes, (input?) event to user-space sounds reasonable. Maybe
also a power-off handler if "system-power-controller" property is given.

Whether this is something crying for own DT node is not my decision. I
just know that having own sub nodes and relying on irq-controller xlate
callbacks eould make driver side really simple.

> 
> I think there's already some DT properties defined to control the
> behavior as well.
> 
> > SWRESET register on PMIC written        => 6
> 
> Probably this can be handled within the core driver. Seems like you'd
> only use this if you have separate entities that write and read this.
> If you don't know, then just ignore it for now.

Yep. My plan was not to implement handling for any of these interrupts
for now. I just wanted to make them easily available for any future use.

> > 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.
> 
> This can still be done but doesn't have to be in DT.

I think this is my weak spot. I don't know how to do this easily without
DT. How do I bring the irq to power-button driver or to WDOG
irq handler (or XXX-driver invented by one who needs these IRQs) without
DT. Options I can think of are:

1. getting the GPIO irq (from MFD parent) and making the IRQ registers
   visible to power button/XXX drivers. Possibly ending up with shared
   IRQ hanlder.
2. Keeping the IRQ register accesses in MFD driver and providing some
   handler callback registration interface from MFD driver to power
   button/XXX drivers
3. Using this interrupt-controller approach.

I dislike option 1 because all drivers using IRQs need to be aware of
IRQ registers.

I dislike option 2 because inventing such IRQ callbacks just feel wrong.
There must be better way - I just don't know it =)

I would like to use option 3 - but I don't know how to do it without DT?
Specifically I don't know how to provide the irq number and interrupt
parent to power-button/XXX drivers if they are not brought from DT. And
why to invent some parallel mechanism for this when DT usage already
provides this. I would be really gratefull for any tips how to do this
correctly if DT is not used.

> > 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?
> 
> I don't think any of this needs to live in DT. All you really need a
> separate driver (and hence irq) for is really just the power button.
> You can just define the interrupts within the kernel.

So I tried to argue that all of the IRQs should be usable and we should
not limit the HW capabilities by design (I like the idea of exposing all
HW functionality because I only rarely know what should be usable better
than the user). Are you sure my dual processor use-case is not
convincing you we should provide these interrupts via DT?

If so - as I already said, I would be gratefull for any suggestions on
how to provide access to irqs without DT. I just can't help feeling that
DT would have been the best way for cleanest and most
understandable/usable end result.

Meanwhile I will prepare a patch where the interrupt handling is
completely ripped from MFD and binding documents as I don't have power
button driver tested (I drafted something though). Maybe such a patch
could be acceptable in order to enable regulator usage (and
compilation). Is that Ok?

Best Regards
    Matti Vaittinen

Powered by blists - more mailing lists