Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Thu, Jun 07, 2018 at 02:12:18PM +0300, Matti Vaittinen wrote: > On Wed, Jun 06, 2018 at 10:16:37AM -0500, Rob Herring wrote: > > On Wed, Jun 6, 2018 at 2:34 AM, Matti Vaittinen > > 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 > > >> 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 > > >> >> 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 > > >> >> >> 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: > > 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 = <90>, <85>, <80>; /* VDD_SOC: > Run-Idle-Suspend */ > bd71837,pmic-buck2-dvs-voltage = <100>, <90>, <0>; /* VDD_ARM: > Run-Idle */ > bd71837,pmic-buck3-dvs-voltage = <100>, <0>, <0>; /* VDD_GPU: Run */ > bd71837,pmic-buck4-dvs-voltage = <100>, <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? I will leave this out for now. Guess this can be added later. > > > > > > 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. > Well, I will only handle the power button irq as you suggested for now. > > > 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 add power button driver (and input subsystem people) tp next patch set. I think I will later also add power/reset driver because PMIC can do reset for the system. Unfortunately the PMIC can't provide power-off. But I leave this out from this series. > > 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. I planned to use SWRESET for power/reset driver - but irq handling is not needed there. > > > 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. I think I found the correct way - I'll send the patch v6 soon. I hope it addresses this issue correctly. Best Regards Matti Vaittinen
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 > 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 > >> 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 > >> >> 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 > >> >> >> 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 = <90>, <85>, <80>; /* VDD_SOC: Run-Idle-Suspend */ bd71837,pmic-buck2-dvs-voltage = <100>, <90>, <0>; /* VDD_ARM: Run-Idle */ bd71837,pmic-buck3-dvs-voltage = <100>, <0>, <0>; /* VDD_GPU: Run */ bd71837,pmic-buck4-dvs-voltage = <100>, <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
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Wed, Jun 6, 2018 at 2:34 AM, Matti Vaittinen 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 >> 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 >> >> 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 >> >> >> 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)?) Yes, it would. > 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 +---> | |+---+| > | | |
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Tue, Jun 05, 2018 at 10:46:14AM -0500, Rob Herring wrote: > On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen > 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 > >> 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 > >> >> 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 || | | | || +> | |
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Mon, Jun 4, 2018 at 6:32 AM, Matti Vaittinen 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 >> 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 >> >> 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. 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. > > I think there are other similar drivers in tree. For example TPS6591x > driver seems to be doing this. (Has MFD driver exposing the interrupts > but no driver handling those). Maybe explanation like this would help: > > "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. > 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. >> > + - #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 connecte
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Fri, Jun 01, 2018 at 12:32:16PM -0500, Rob Herring wrote: > On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen > 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 > >> 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. I think there are other similar drivers in tree. For example TPS6591x driver seems to be doing this. (Has MFD driver exposing the interrupts but no driver handling those). Maybe explanation like this would help: "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) 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. > > > + - #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. > >> My concern is you added it but nothing uses it which tells > >> me your binding is incomplete. I'd rather see complete bindings even > >> if you don't have drivers. > > > > So this makes me wonder if my use-case for interrupt controller is > > valid. I thought making this PMIC as interrupt controller is a nice way > > of hiding the irq register and i2c access from other potential drivers > > using these interrupts. But as I don't know what could be the potential > > user for these irqs, I don't know how to complete binding. This is why I > > also thought of making this optional, so that the potential for using > > the interrupts would be there but it was not required when interrupts > > are not needed. > > The only drivers getting these interrupts wo
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
Quoting Matti Vaittinen (2018-06-01 03:51:05) > On Thu, May 31, 2018 at 07:57:53AM -0700, Stephen Boyd wrote: > > Quoting Rob Herring (2018-05-31 07:07:24) > > > > > > I don't think it should. The h/w either has an interrupt controller or > > > it doesn't. My concern is you added it but nothing uses it which tells > > > me your binding is incomplete. I'd rather see complete bindings even > > > if you don't have drivers. For example, as-is, there's not really any > > > need for the clocks child node. You can just make the parent a clock > > > provider. But we need a complete picture of the h/w to make that > > > determination. > > > > > > > I don't see a reason to have the clk subnode either. > > After some pondering - do you mean I could: > 1. remove clk binfing document and clk node. > 2. add clock-output-names etc to pmic node (and describe them in pmic > node binding document) > 3. use parent DT node in clk driver and do something like: > if (parent->of_node) > ret = of_clk_add_hw_provider(parent->of_node, > of_clk_hw_simple_get, > hw); > 4. remove the clkdev > This sounds ok to me. As Rob said, a more complete picture of the hardware would make this easier.
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Fri, Jun 1, 2018 at 1:25 AM, Matti Vaittinen 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 >> 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? > + - #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. 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. > >> My concern is you added it but nothing uses it which tells >> me your binding is incomplete. I'd rather see complete bindings even >> if you don't have drivers. > > So this makes me wonder if my use-case for interrupt controller is > valid. I thought making this PMIC as interrupt controller is a nice way > of hiding the irq register and i2c access from other potential drivers > using these interrupts. But as I don't know what could be the potential > user for these irqs, I don't know how to complete binding. This is why I > also thought of making this optional, so that the potential for using > the interrupts would be there but it was not required when interrupts > are not needed. The only drivers getting these interrupts would be drivers for this PMIC. Interrupts are handled by the driver owning the h/w that generated the interrupt. I think that is the disconnect here. Take a power button. We don't create a generic power button interrupt and then have some generic power button interrupt handler. We have a handler for specifically for that device and then it generates a power button press event. >> For example, as-is, there's not really any >> need for the clocks child node. You can just make the parent a clock >> provider. > > This sounds correct. I just lack of knowledge on how to handle clocks > in "standard way" using the clock framework and this was a result of > my first attempt. (Funny, I have written clk / synchronization drivers > for work in the past but still I have no idea on how to do this in > "standard way"). > >> But we need a complete picture of the h/w to make that >> determination. > > My attempt is to create generic driver for this PMIC. I would rather not > limit it's use to any particular board/soc. The example binding is based > on my test environment where I simply connected this PMIC break out > board to beagle bone black. (I do also have a test board where i.MX8 and > peripherials are powered by this PMIC but I rather not limit this driver > to such single setup. Besides the linux running on that board is not > 'standard') That's how we design all the PMIC drivers. All the PMIC functions should be exposed thru standard class APIs. Though many PMICs are pretty tightly coupled to particular SoCs either by design or just there's not a lot of boards to create any sort of matrix of combinations. > The PMIC itself just has this 32.768 KHz clock output. Clock can be > enabled/disabled via register interface. I think this is intended to be > use
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Thu, May 31, 2018 at 07:57:53AM -0700, Stephen Boyd wrote: > Quoting Rob Herring (2018-05-31 07:07:24) > > On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen > > wrote: > > > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote: > > >> Hello Rob, > > >> > > >> Thanks for the review! > > >> > > >> 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. My concern is you added it but nothing uses it which tells > > me your binding is incomplete. I'd rather see complete bindings even > > if you don't have drivers. For example, as-is, there's not really any > > need for the clocks child node. You can just make the parent a clock > > provider. But we need a complete picture of the h/w to make that > > determination. > > > > I don't see a reason to have the clk subnode either. After some pondering - do you mean I could: 1. remove clk binfing document and clk node. 2. add clock-output-names etc to pmic node (and describe them in pmic node binding document) 3. use parent DT node in clk driver and do something like: if (parent->of_node) ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get, hw); 4. remove the clkdev I will cook new set of patches with all suggested changes but it may be I don't get it ready for posting untill next week. Br, Matti Vaittinen
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote: > On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen > 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. + - #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. > My concern is you added it but nothing uses it which tells > me your binding is incomplete. I'd rather see complete bindings even > if you don't have drivers. So this makes me wonder if my use-case for interrupt controller is valid. I thought making this PMIC as interrupt controller is a nice way of hiding the irq register and i2c access from other potential drivers using these interrupts. But as I don't know what could be the potential user for these irqs, I don't know how to complete binding. This is why I also thought of making this optional, so that the potential for using the interrupts would be there but it was not required when interrupts are not needed. > For example, as-is, there's not really any > need for the clocks child node. You can just make the parent a clock > provider. This sounds correct. I just lack of knowledge on how to handle clocks in "standard way" using the clock framework and this was a result of my first attempt. (Funny, I have written clk / synchronization drivers for work in the past but still I have no idea on how to do this in "standard way"). > But we need a complete picture of the h/w to make that > determination. My attempt is to create generic driver for this PMIC. I would rather not limit it's use to any particular board/soc. The example binding is based on my test environment where I simply connected this PMIC break out board to beagle bone black. (I do also have a test board where i.MX8 and peripherials are powered by this PMIC but I rather not limit this driver to such single setup. Besides the linux running on that board is not 'standard') The PMIC itself just has this 32.768 KHz clock output. Clock can be enabled/disabled via register interface. I think this is intended to be used for RTC but I thought this driver does not need to care about that. I thought it is just a good idea to provide control via clk subsystem and to not make assumptions on use-cases in this driver. Best Regards Matti Vaittinen
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
Quoting Rob Herring (2018-05-31 07:07:24) > On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen > wrote: > > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote: > >> Hello Rob, > >> > >> Thanks for the review! > >> > >> 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. My concern is you added it but nothing uses it which tells > me your binding is incomplete. I'd rather see complete bindings even > if you don't have drivers. For example, as-is, there's not really any > need for the clocks child node. You can just make the parent a clock > provider. But we need a complete picture of the h/w to make that > determination. > I don't see a reason to have the clk subnode either.
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen wrote: > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote: >> Hello Rob, >> >> Thanks for the review! >> >> 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. My concern is you added it but nothing uses it which tells me your binding is incomplete. I'd rather see complete bindings even if you don't have drivers. For example, as-is, there's not really any need for the clocks child node. You can just make the parent a clock provider. But we need a complete picture of the h/w to make that determination. Rob
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote: > Hello Rob, > > Thanks for the review! > > 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. Br, Matti Vaittinen
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
Hello Rob,
Thanks for the review!
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.
This IRQ might be relevant if use for PMIC is such that it is not
powering the processor where the driver is runninng. Then the controlling
processor can get the notification that chips powered by PMIC are
resetting. But handling for this must be use-case specific, right? So in
short - none of the current sub-devices use the IRQs - they are there
for specific use-cases which some boards may implement. Any suggestions
how to document the available interrupts? (power button line, sw reset
etc). My current assumption has been that one who is interested in using
these irqs should really also see the data-sheet for IRQs. But I admit
that documenting available interrupts here would be helpful. I will just
cook up some explanation and send it as a patch if no suggestions on how
to document those.
Patches 3/6 and 6/6 from the series were already applied to Mark's tree.
So how should I send further patches? Should I still send the whole series
(including already applied patches 3/6 and 6/6) or only the ones I change?
> > +Example:
> > +
> > + pmic: bd71837@4b {
>
> Node names should be generic ideally. So "pmic@4b"
I'll change that.
> > + clk: bd71837-32k-out {
>
> clock-controller {
And I'll change that too.
>
> > + compatible = "rohm,bd71837-clk";
> > + #clock-cells = <0>;
> > + clock-frequency = <32768>;
>
> Can this be anything else?
Not so that I know. Frequency is fixed. Is there a problem with this?
Br,
Matti Vaittinen
Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC MFD.
>
> Signed-off-by: Matti Vaittinen
> ---
> .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 52
> ++
> 1 file changed, 52 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> new file mode 100644
> index ..bbc46d38b162
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> @@ -0,0 +1,52 @@
> +* ROHM BD71837 Power Management Integrated Circuit bindings
> +
> +BD71837MWV is a programmable Power Management IC for powering single-core,
> +dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
> +low BOM cost and compact solution footprint. It integrates 8 Buck
> +egulators and 7 LDO’s to provide all the power rails required by the SoC and
> +the commonly used peripherals.
> +
> +Required properties:
> + - compatible: Should be "rohm,bd71837".
> + - reg : I2C slave address.
> + - interrupt-parent : Phandle to the parent interrupt controller.
> + - interrupts: The interrupt line the device is connected to.
> + - interrupt-controller : Marks the device node as an interrupt
> controller.
What sub blocks have interrupts?
> + - #interrupt-cells : The number of cells to describe an IRQ, should be 1
> or 2.
> + The first cell is the IRQ number.
> + The second cell if present is the flags, encoded as
> trigger
> + masks from ../interrupt-controller/interrupts.txt.
> + - regulators: : List of child nodes that specify the
> regulators
> + Please see ../regulator/rohm,bd71837-regulator.txt
> + - clock:: Please see ../clock/rohm,bd71837-clock.txt
> +
> +Example:
> +
> + pmic: bd71837@4b {
Node names should be generic ideally. So "pmic@4b"
> + compatible = "rohm,bd71837";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x4b>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <29 GPIO_ACTIVE_LOW>;
> + interrupt-names = "irq";
> + #interrupt-cells = <1>;
> + interrupt-controller;
> +
> + regulators {
> + buck1: BUCK1 {
> + regulator-name = "buck1";
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <130>;
> + regulator-boot-on;
> + regulator-ramp-delay = <1250>;
> + };
> + ...
> + };
> + clk: bd71837-32k-out {
clock-controller {
> + compatible = "rohm,bd71837-clk";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
Can this be anything else?
> + clock-output-names = "bd71837-32k-out";
> + };
> + };
> --
> 2.14.3
>
[PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
Document devicetree bindings for ROHM BD71837 PMIC MFD.
Signed-off-by: Matti Vaittinen
---
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 52 ++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
new file mode 100644
index ..bbc46d38b162
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -0,0 +1,52 @@
+* ROHM BD71837 Power Management Integrated Circuit bindings
+
+BD71837MWV is a programmable Power Management IC for powering single-core,
+dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
+low BOM cost and compact solution footprint. It integrates 8 Buck
+egulators and 7 LDO’s to provide all the power rails required by the SoC and
+the commonly used peripherals.
+
+Required properties:
+ - compatible : Should be "rohm,bd71837".
+ - reg : I2C slave address.
+ - interrupt-parent: Phandle to the parent interrupt controller.
+ - interrupts : The interrupt line the device is connected to.
+ - interrupt-controller: Marks the device node as an interrupt
controller.
+ - #interrupt-cells: The number of cells to describe an IRQ, should be 1
or 2.
+ The first cell is the IRQ number.
+ The second cell if present is the flags, encoded as
trigger
+ masks from ../interrupt-controller/interrupts.txt.
+ - regulators: : List of child nodes that specify the regulators
+ Please see ../regulator/rohm,bd71837-regulator.txt
+ - clock: : Please see ../clock/rohm,bd71837-clock.txt
+
+Example:
+
+ pmic: bd71837@4b {
+ compatible = "rohm,bd71837";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x4b>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <29 GPIO_ACTIVE_LOW>;
+ interrupt-names = "irq";
+ #interrupt-cells = <1>;
+ interrupt-controller;
+
+ regulators {
+ buck1: BUCK1 {
+ regulator-name = "buck1";
+ regulator-min-microvolt = <70>;
+ regulator-max-microvolt = <130>;
+ regulator-boot-on;
+ regulator-ramp-delay = <1250>;
+ };
+ ...
+ };
+ clk: bd71837-32k-out {
+ compatible = "rohm,bd71837-clk";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ clock-output-names = "bd71837-32k-out";
+ };
+ };
--
2.14.3
