Cherryview wake up events

Cherryview wake up events

[Johannes Stezenbach]

Hi,

Mika, I've been reading the thread about pinctrl-cherryview
interrupts, but I have some basic questions in understanding
the hardware and the relationship between ACPI and Linux drivers,
so I decided to start a new thread.
https://lkml.kernel.org/g/20160909085832.GK15313@lahna.fi.intel.com

I have one Asus E200HA (Atom x5-Z8300) where the power button
doesn't generate any ACPI events (no SCI), instead it causes
a Thermal Event irq:

 TRM:          3          3          3          4   Thermal event interrupts

[   51.825488] CPU0: Core temperature above threshold, cpu clock throttled (total events = 1)
[   51.826933] CPU1: Core temperature above threshold, cpu clock throttled (total events = 1)
[   51.826965] mce: [Hardware Error]: Machine check events logged
[   51.841180] mce: [Hardware Error]: Machine check events logged

(These events are logged only sometimes, usually a power button
press only increments the TRM count.)

I would like to understand how this is possible, when I boot
with apic=debug I can't see anything claiming vector 0xfa.

The LID causes a gpio irq:
 158:          2          0          0          0  chv-gpio   43 ACPI:Event

However, neither LID nor power button can wake up the
device from "echo freeze >/sys/power/state".  :-(

"grep . /sys/firmware/acpi/interrupts/*" shows only zeros.

I put the DSDT and some other tables at:
https://linuxtv.org/~js/e200ha/

During the last weeks I read what I could about the hardware
and ACPI, and poked at it with acpidbg, devmem, ioport
and in kernel source, but to no avail.

On Thu, Sep 15, 2016 at 06:52:10PM +0300, Mika Westerberg wrote:
> It turns out that for north and southwest communities, they can only
> generate GPIO interrupts for lower 8 interrupts (IntSel value). The upper
> part (8-15) can only generate GPEs (General Purpose Events).

I got the Atom Z8000 series datasheet from
http://www.intel.com/content/www/us/en/processors/atom/atom-technical-resources.html
and tried to find the source for this.  The closest I
could find is the GPIO_ROUT PMC register?
However, the datasheet doesn't tell about the other
interrupts not covered by GPIO_ROUT, if they are fixed
IRQ or SCI or "no effect".

I also don't get the mapping from intsel irq to IO-APIC pin
number.  And also not the mapping between the pin numbers used
on DSDT GpioInt to the pin numbers in pinctrl-cherryview.c.
Could you shed a light on this?  Or point out where I can
find information?

It seems to imply BIOS sets up IntSel.  I'm generally confused
about the responsibility of BIOS vs. drivers making use of the
information from DSDT, e.g. Device (GPO1) has a list of
GpioIo Connections, other devices like PMI2 use GpioInt
from GPO1.  My E200HA has the INT33F5 TI PMIC
Controller, which according to Windows driver strings
seems to be the SND9039.
Does it mean I need a PMIC driver that reads the _CRS and
configures the GPIO?

BTW, the datasheet talks about 4 seconds for power button
override, but it takes 10 seconds.  Maybe it means the
power button is connected to the TI PMIC, not to the
Cherryview SoC?

Another question is about the virtual GPIO device that exists
in hardware and is used by DSDT.  How does that work and
why does pinctrl-cherryview.c exclude it?

Sorry for so many questions, any info is appreciated,
and any suggestion what to try to get the thing to
wake up from freeze.

I was totally unfamiliar with ACPI until now, but I think
the DSDT has some nasty surprise in several _REG methods
that use OEM defined OperatingRegionIds to set some availabilty
flags that are tested in other methods.  So it means if the
Windows drivers aren't loaded, those methods won't do anything,
right? Does anyone have suggestions or even examples how to deal with this?


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Mon, Sep 19, 2016 at 01:21:17PM +0200, Johannes Stezenbach wrote:
> Hi,
> 
> Mika, I've been reading the thread about pinctrl-cherryview
> interrupts, but I have some basic questions in understanding
> the hardware and the relationship between ACPI and Linux drivers,
> so I decided to start a new thread.
> https://lkml.kernel.org/g/20160909085832.GK15313@lahna.fi.intel.com
> 
> I have one Asus E200HA (Atom x5-Z8300) where the power button
> doesn't generate any ACPI events (no SCI), instead it causes
> a Thermal Event irq:
> 
>  TRM:          3          3          3          4   Thermal event interrupts
> 
> [   51.825488] CPU0: Core temperature above threshold, cpu clock throttled (total events = 1)
> [   51.826933] CPU1: Core temperature above threshold, cpu clock throttled (total events = 1)
> [   51.826965] mce: [Hardware Error]: Machine check events logged
> [   51.841180] mce: [Hardware Error]: Machine check events logged
> 
> (These events are logged only sometimes, usually a power button
> press only increments the TRM count.)

Hmm, that's weird.

> I would like to understand how this is possible, when I boot
> with apic=debug I can't see anything claiming vector 0xfa.
> 
> The LID causes a gpio irq:
>  158:          2          0          0          0  chv-gpio   43 ACPI:Event
> 
> However, neither LID nor power button can wake up the
> device from "echo freeze >/sys/power/state".  :-(

The cherryview pinctrl driver does not (yet) support wake up events. It
currently just sets IRQCHIP_SKIP_SET_WAKE for the irqchip.

> "grep . /sys/firmware/acpi/interrupts/*" shows only zeros.
> 
> I put the DSDT and some other tables at:
> https://linuxtv.org/~js/e200ha/
> 
> During the last weeks I read what I could about the hardware
> and ACPI, and poked at it with acpidbg, devmem, ioport
> and in kernel source, but to no avail.
> 
> On Thu, Sep 15, 2016 at 06:52:10PM +0300, Mika Westerberg wrote:
> > It turns out that for north and southwest communities, they can only
> > generate GPIO interrupts for lower 8 interrupts (IntSel value). The upper
> > part (8-15) can only generate GPEs (General Purpose Events).
> 
> I got the Atom Z8000 series datasheet from
> http://www.intel.com/content/www/us/en/processors/atom/atom-technical-resources.html
> and tried to find the source for this.  The closest I
> could find is the GPIO_ROUT PMC register?
> However, the datasheet doesn't tell about the other
> interrupts not covered by GPIO_ROUT, if they are fixed
> IRQ or SCI or "no effect".

Source for this information is coming from an internal documentation for
the SoC. For some reason it is not included in the datasheet.

> I also don't get the mapping from intsel irq to IO-APIC pin
> number.  And also not the mapping between the pin numbers used
> on DSDT GpioInt to the pin numbers in pinctrl-cherryview.c.
> Could you shed a light on this?  Or point out where I can
> find information?

IntSel field is RO and filled in by the BIOS. The mapping from IntSel
and I/O-APIC pins seems also missing from the datasheet but for example
for north community, IntSel 0-7 maps to I/O-APIC pins 51-58 (and OR of
those is 48).

> It seems to imply BIOS sets up IntSel.  I'm generally confused
> about the responsibility of BIOS vs. drivers making use of the
> information from DSDT, e.g. Device (GPO1) has a list of
> GpioIo Connections, other devices like PMI2 use GpioInt
> from GPO1.  My E200HA has the INT33F5 TI PMIC
> Controller, which according to Windows driver strings
> seems to be the SND9039.
> Does it mean I need a PMIC driver that reads the _CRS and
> configures the GPIO?

The GPIOs under GPIO controller are generally used for either ACPI GPIO
events or GPIO Operation Region. Those are used by the AML code to
access the GPIO hardware with the help of OS.

The INT33F5 PMIC from your DSDT table seems to have one GpioInt()
resource which it uses as an interrupt. This is handled already by the
I2C core. In order to use that PMIC you need a driver and I'm not sure
if there is one for that particular part.

> BTW, the datasheet talks about 4 seconds for power button
> override, but it takes 10 seconds.  Maybe it means the
> power button is connected to the TI PMIC, not to the
> Cherryview SoC?

Or it may be handled by the embedded controller itself. The FADT ACPI
table should tell you if it has "fixed power button" or is it using some
other mechanism (like control method power button).

> Another question is about the virtual GPIO device that exists
> in hardware and is used by DSDT.  How does that work and
> why does pinctrl-cherryview.c exclude it?

Because it is "virtual" and does not expose any hardware. The
pinctrl-cherryview deals only with the GPIO block found on
Braswell/Cherryview.

IIRC that virtual GPIO thing was used to fix USB device wakeup or
something like that.

> Sorry for so many questions, any info is appreciated,
> and any suggestion what to try to get the thing to
> wake up from freeze.

I can make you a test patch which adds support for wakes for the pinctrl
driver if you like to test it out. However, that will happen most likely
near end of the week as I have other things right now.

> I was totally unfamiliar with ACPI until now, but I think
> the DSDT has some nasty surprise in several _REG methods
> that use OEM defined OperatingRegionIds to set some availabilty
> flags that are tested in other methods.  So it means if the
> Windows drivers aren't loaded, those methods won't do anything,
> right? Does anyone have suggestions or even examples how to deal with this?

Right, a driver is needed. There are bunch of custom OpRegion handlers
under drivers/acpi/pmic/*.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Mon, Sep 19, 2016 at 02:56:19PM +0300, Mika Westerberg wrote:
> On Mon, Sep 19, 2016 at 01:21:17PM +0200, Johannes Stezenbach wrote:
> > 
> > The LID causes a gpio irq:
> >  158:          2          0          0          0  chv-gpio   43 ACPI:Event
> > 
> > However, neither LID nor power button can wake up the
> > device from "echo freeze >/sys/power/state".  :-(
> 
> The cherryview pinctrl driver does not (yet) support wake up events. It
> currently just sets IRQCHIP_SKIP_SET_WAKE for the irqchip.

OK, but shouldn't the wakeup usually be handled by ACPI?
Clearly I don't understand this.  I mean on the non-ACPI
embedded ARM systems I'm used to I need to enable specific
irqs as wakeup sources, but on ACPI, isn't SCI the implicit
wakeup irq?  Probably I'm just totally confused, so let
me ask another way, below.

> I can make you a test patch which adds support for wakes for the pinctrl
> driver if you like to test it out. However, that will happen most likely
> near end of the week as I have other things right now.

That would be great!


I found in the DSDT:

    Scope (_SB.GPO0)
    {
        Name (EVBF, Buffer (0x03) {})
        CreateByteField (EVBF, Zero, EVST)
        CreateByteField (EVBF, One, ELEN)
        CreateByteField (EVBF, 0x02, ENVT)
        Name (LIDZ, One)
        Method (_E4E, 0, Serialized)  // _Exx: Edge-Triggered GPE
        {
            Name (_T_0, Zero)  // _T_x: Emitted by ASL Compiler
            If (^^PCI0.I2C1.AVBL != One)
            {
                Return (Zero)
            }

            EVBF = ^^PCI0.I2C1.ENID /* \_SB_.PCI0.I2C1.ENID */
...
            _T_0 = ENVT /* \_SB_.GPO0.ENVT */
...
                ElseIf (_T_0 == 0xA9)
                {
                    Notify (PWRB, 0x80) // Status Change
                    Break
                }

and
        Device (GPO0)
        {
...
            Method (_AEI, 0, NotSerialized)  // _AEI: ACPI Event Interrupts
            {
                Name (WBUF, ResourceTemplate ()
                {
                    GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
                        "\\_SB.GPO0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x004E
                        }
                })
                If (OSID == One)
                {
                    Return (WBUF) /* \_SB_.GPO0._AEI.WBUF */
                }
            }

and OSID is a field in 
    OperationRegion (GNVS, SystemMemory, 0x7A158000, 0x0362)
which is inside what /proc/iomem lists as "ACPI no-volatile storage".
OSID is read a lot in the DSDT but never written to.
But calling \_SB.GPO0._AEI in acpidbg returns a buffer of size 25.

Now my question is, is this pin 0x004E the same as this
in /proc/interrupts which fires on LID event?

 158:          2          0          0          0  chv-gpio   43 ACPI:Event



The FADT has
            Control Method Power Button (V1) : 0
            Control Method Sleep Button (V1) : 1

PWRBTN_EN in PM1 is set.  But PWRBTN press causes thermal irq.

No SCI (irq 9) is ever generated, except by writing to the
BIOS_RLS bit in SMI_EN register (IO port 0x430).

GPE block addresses in FADT are 0.  GPE0a_EN register (IO 0x428)
is set to 0x6000 (TCO_EN + PME_B0_EN, but none of the GPIO enables).

Any advice how to continue?


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Mon, Sep 19, 2016 at 10:36:22PM +0200, Johannes Stezenbach wrote:
> On Mon, Sep 19, 2016 at 02:56:19PM +0300, Mika Westerberg wrote:
> > On Mon, Sep 19, 2016 at 01:21:17PM +0200, Johannes Stezenbach wrote:
> > > 
> > > The LID causes a gpio irq:
> > >  158:          2          0          0          0  chv-gpio   43 ACPI:Event
> > > 
> > > However, neither LID nor power button can wake up the
> > > device from "echo freeze >/sys/power/state".  :-(
> > 
> > The cherryview pinctrl driver does not (yet) support wake up events. It
> > currently just sets IRQCHIP_SKIP_SET_WAKE for the irqchip.
> 
> OK, but shouldn't the wakeup usually be handled by ACPI?
> Clearly I don't understand this.  I mean on the non-ACPI
> embedded ARM systems I'm used to I need to enable specific
> irqs as wakeup sources, but on ACPI, isn't SCI the implicit
> wakeup irq?  Probably I'm just totally confused, so let
> me ask another way, below.

It is typically handled by ACPI but it requires some hardware as well.
Looks like your machine does not use SCIs at all but instead events are
triggered using GPIO signals instead of GPEs.

> > I can make you a test patch which adds support for wakes for the pinctrl
> > driver if you like to test it out. However, that will happen most likely
> > near end of the week as I have other things right now.
> 
> That would be great!
> 
> 
> I found in the DSDT:
> 
>     Scope (_SB.GPO0)
>     {
>         Name (EVBF, Buffer (0x03) {})
>         CreateByteField (EVBF, Zero, EVST)
>         CreateByteField (EVBF, One, ELEN)
>         CreateByteField (EVBF, 0x02, ENVT)
>         Name (LIDZ, One)
>         Method (_E4E, 0, Serialized)  // _Exx: Edge-Triggered GPE
>         {
>             Name (_T_0, Zero)  // _T_x: Emitted by ASL Compiler
>             If (^^PCI0.I2C1.AVBL != One)
>             {
>                 Return (Zero)
>             }
> 
>             EVBF = ^^PCI0.I2C1.ENID /* \_SB_.PCI0.I2C1.ENID */
> ...
>             _T_0 = ENVT /* \_SB_.GPO0.ENVT */
> ...
>                 ElseIf (_T_0 == 0xA9)
>                 {
>                     Notify (PWRB, 0x80) // Status Change
>                     Break
>                 }
> 
> and
>         Device (GPO0)
>         {
> ...
>             Method (_AEI, 0, NotSerialized)  // _AEI: ACPI Event Interrupts
>             {
>                 Name (WBUF, ResourceTemplate ()
>                 {
>                     GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>                         "\\_SB.GPO0", 0x00, ResourceConsumer, ,
>                         )
>                         {   // Pin list
>                             0x004E
>                         }
>                 })
>                 If (OSID == One)
>                 {
>                     Return (WBUF) /* \_SB_.GPO0._AEI.WBUF */
>                 }
>             }
> 
> and OSID is a field in 
>     OperationRegion (GNVS, SystemMemory, 0x7A158000, 0x0362)
> which is inside what /proc/iomem lists as "ACPI no-volatile storage".
> OSID is read a lot in the DSDT but never written to.
> But calling \_SB.GPO0._AEI in acpidbg returns a buffer of size 25.
> 
> Now my question is, is this pin 0x004E the same as this
> in /proc/interrupts which fires on LID event?
> 
>  158:          2          0          0          0  chv-gpio   43 ACPI:Event

Yes, it is that one and it triggers \_SB.GPO0._E4E() method to be called
whenever low edge is detected on the GPIO line. This method then handles
many things depending on what the AML code reads from ^^PCI0.I2C1.ENID
notifying the power button device (PWRB) among other things.

I suppose you already have CONFIG_ACPI_I2C_OPREGION=y in your .config?
That allows the AML code to access the I2C bus using the I2C driver.

> The FADT has
>             Control Method Power Button (V1) : 0
>             Control Method Sleep Button (V1) : 1
> 
> PWRBTN_EN in PM1 is set.  But PWRBTN press causes thermal irq.

Yeah, it uses control method power button (PNP0C0C) and ACPI GPIO event
to trigger changes in that.

> No SCI (irq 9) is ever generated, except by writing to the
> BIOS_RLS bit in SMI_EN register (IO port 0x430).
> 
> GPE block addresses in FADT are 0.  GPE0a_EN register (IO 0x428)
> is set to 0x6000 (TCO_EN + PME_B0_EN, but none of the GPIO enables).
> 
> Any advice how to continue?

Please check that you have that CONFIG_ACPI_I2C_OPREGION=y and
CONFIG_MFD_AXP20X=y.

You should see the ACPI:Event interrupt count increasing in
/proc/interrups when you press power button. When that works then we can
start thinking about adding wake support :)

Re: Cherryview wake up events

[Johannes Stezenbach]

On Tue, Sep 20, 2016 at 12:18:40PM +0300, Mika Westerberg wrote:
> On Mon, Sep 19, 2016 at 10:36:22PM +0200, Johannes Stezenbach wrote:
> > Now my question is, is this pin 0x004E the same as this
> > in /proc/interrupts which fires on LID event?
> > 
> >  158:          2          0          0          0  chv-gpio   43 ACPI:Event
> 
> Yes, it is that one and it triggers \_SB.GPO0._E4E() method to be called
> whenever low edge is detected on the GPIO line. This method then handles
> many things depending on what the AML code reads from ^^PCI0.I2C1.ENID
> notifying the power button device (PWRB) among other things.

Thanks for confirmation, but it circles back to the question
how to map the numbers.  Since the document that describes it
is not public, it would be useful if you could add comments
to pinctrl-cherryview.c that describes it.
Or did I just miss something?

> I suppose you already have CONFIG_ACPI_I2C_OPREGION=y in your .config?
> That allows the AML code to access the I2C bus using the I2C driver.
> 
> > The FADT has
> >             Control Method Power Button (V1) : 0
> >             Control Method Sleep Button (V1) : 1
> > 
> > PWRBTN_EN in PM1 is set.  But PWRBTN press causes thermal irq.
> 
> Yeah, it uses control method power button (PNP0C0C) and ACPI GPIO event
> to trigger changes in that.

I'm confused again because I thought "Control Method Power Button (V1) : 0"
means it is a fixed power button, however the DSDT also has

        Device (PWRB)
        {
            Name (_HID, EisaId ("PNP0C0C") /* Power Button Device */)  // _HID: Hardware ID
        }

        Device (SLPB)
        {
            Name (_HID, EisaId ("PNP0C0E") /* Sleep Button Device */)  // _HID: Hardware ID
        }


> > No SCI (irq 9) is ever generated, except by writing to the
> > BIOS_RLS bit in SMI_EN register (IO port 0x430).
> > 
> > GPE block addresses in FADT are 0.  GPE0a_EN register (IO 0x428)
> > is set to 0x6000 (TCO_EN + PME_B0_EN, but none of the GPIO enables).
> > 
> > Any advice how to continue?
> 
> Please check that you have that CONFIG_ACPI_I2C_OPREGION=y and
> CONFIG_MFD_AXP20X=y.
> 
> You should see the ACPI:Event interrupt count increasing in
> /proc/interrups when you press power button. When that works then we can
> start thinking about adding wake support :)

I had CONFIG_ACPI_I2C_OPREGION=y but excluded CONFIG_MFD_AXP20X
based on \_SB.PIC0.I2C7.PMI1._STA returning 0 in acpidbg,

            Device (PMI1)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (_HID, "INT33F4" /* XPOWER PMIC Controller */)  // _HID: Hardware ID
                Name (_CID, "INT33F4" /* XPOWER PMIC Controller */)  // _CID: Compatible ID
                Name (_DDN, "XPOWER PMIC Controller")  // _DDN: DOS Device Name

but \_SB.PIC0.I2C7.PMI1._STA returns 0xf

            Device (PMI2)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (_HID, "INT33F5" /* TI PMIC Controller */)  // _HID: Hardware ID
                Name (_CID, "INT33F5" /* TI PMIC Controller */)  // _CID: Compatible ID
                Name (_DDN, "TI PMIC Controller")  // _DDN: DOS Device Name

So I tried CONFIG_MFD_AXP20X=y anyway, but as expected: no change.

Since TI doesn't even have a product page for the SND9039
(only a few references in TI support forum can be found),
I'm not sure what can be done.  So maybe a better short term
goal would be to get wakeup by LID working.

However, I still wonder why the power button can trigger
a thermal irq, is it related to the PMIC?  I couldn't
find out where the thermal irq is routed.


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Tue, Sep 20, 2016 at 12:14:50PM +0200, Johannes Stezenbach wrote:
> On Tue, Sep 20, 2016 at 12:18:40PM +0300, Mika Westerberg wrote:
> > On Mon, Sep 19, 2016 at 10:36:22PM +0200, Johannes Stezenbach wrote:
> > > Now my question is, is this pin 0x004E the same as this
> > > in /proc/interrupts which fires on LID event?
> > > 
> > >  158:          2          0          0          0  chv-gpio   43 ACPI:Event
> > 
> > Yes, it is that one and it triggers \_SB.GPO0._E4E() method to be called
> > whenever low edge is detected on the GPIO line. This method then handles
> > many things depending on what the AML code reads from ^^PCI0.I2C1.ENID
> > notifying the power button device (PWRB) among other things.
> 
> Thanks for confirmation, but it circles back to the question
> how to map the numbers.  Since the document that describes it
> is not public, it would be useful if you could add comments
> to pinctrl-cherryview.c that describes it.
> Or did I just miss something?

I don't think it is relevant here. The driver uses only wired OR
interrupt which is provided in _CRS of the device.

> > I suppose you already have CONFIG_ACPI_I2C_OPREGION=y in your .config?
> > That allows the AML code to access the I2C bus using the I2C driver.
> > 
> > > The FADT has
> > >             Control Method Power Button (V1) : 0
> > >             Control Method Sleep Button (V1) : 1
> > > 
> > > PWRBTN_EN in PM1 is set.  But PWRBTN press causes thermal irq.
> > 
> > Yeah, it uses control method power button (PNP0C0C) and ACPI GPIO event
> > to trigger changes in that.
> 
> I'm confused again because I thought "Control Method Power Button (V1) : 0"
> means it is a fixed power button, however the DSDT also has

I misread it, sorry about that.

Can you check if you have:

  Hardware Reduced (V5) : 1

in that FADT table?

If yes, it probably does not have the normal Fixed power button but
instead it has something called "Windows button array device" with
_HID/_CID of PNP0C40. Looking at your dsdt.dsl, this looks to be the
case.

That device is driven by soc_button_array.c driver which can be enabled
with CONFIG_KEYBOARD_GPIO=y and CONFIG_INPUT_SOC_BUTTON_ARRAY=y. Can you
check if you have that enabled already?

You should actually see it in /proc/interrupts with names like "power"
and so on.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Tue, Sep 20, 2016 at 01:40:14PM +0300, Mika Westerberg wrote:
> Can you check if you have:
> 
>   Hardware Reduced (V5) : 1
> 
> in that FADT table?

Nope, it is "Hardware Reduced (V5) : 0".  Now the FADT is also at
https://linuxtv.org/~js/e200ha/

> If yes, it probably does not have the normal Fixed power button but
> instead it has something called "Windows button array device" with
> _HID/_CID of PNP0C40. Looking at your dsdt.dsl, this looks to be the
> case.
> 
> That device is driven by soc_button_array.c driver which can be enabled
> with CONFIG_KEYBOARD_GPIO=y and CONFIG_INPUT_SOC_BUTTON_ARRAY=y. Can you
> check if you have that enabled already?
> 
> You should actually see it in /proc/interrupts with names like "power"
> and so on.

I added CONFIG_INPUT_SOC_BUTTON_ARRAY=y, but no joy.
Maybe because the _HID is INTCFD9, only _CID is PNP0C40?
It also has a _DSM with UUID dfbcf3c5-e7a5-44e6-9c1f-29c76f6e059c.

BTW, lsinput already lists two "Power Button" devices,
   phys    : "PNP0C0C/button/input0"
   phys    : "LNXPWRBN/button/input0"

None of them generates events in input-events.


Thanks,
Johannes

Re: Cherryview wake up events

[Johannes Stezenbach]

On Tue, Sep 20, 2016 at 05:59:43PM +0200, Johannes Stezenbach wrote:
> On Tue, Sep 20, 2016 at 01:40:14PM +0300, Mika Westerberg wrote:
> > If yes, it probably does not have the normal Fixed power button but
> > instead it has something called "Windows button array device" with
> > _HID/_CID of PNP0C40. Looking at your dsdt.dsl, this looks to be the
> > case.
> > 
> > That device is driven by soc_button_array.c driver which can be enabled
> > with CONFIG_KEYBOARD_GPIO=y and CONFIG_INPUT_SOC_BUTTON_ARRAY=y. Can you
> > check if you have that enabled already?
> > 
> > You should actually see it in /proc/interrupts with names like "power"
> > and so on.
> 
> I added CONFIG_INPUT_SOC_BUTTON_ARRAY=y, but no joy.
> Maybe because the _HID is INTCFD9, only _CID is PNP0C40?
> It also has a _DSM with UUID dfbcf3c5-e7a5-44e6-9c1f-29c76f6e059c.

Or it is because the PNP0C40 device depends on GpioInt from PMIC
which isn't available...

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (CBUF, ResourceTemplate ()
                {
                    GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullUp, 0x0BB8,
                        "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x0016
                        }
                })
                Return (CBUF) /* \_SB_.TBAD._CRS.CBUF */
            }

Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Tue, Sep 20, 2016 at 11:11:53PM +0200, Johannes Stezenbach wrote:
> On Tue, Sep 20, 2016 at 05:59:43PM +0200, Johannes Stezenbach wrote:
> > On Tue, Sep 20, 2016 at 01:40:14PM +0300, Mika Westerberg wrote:
> > > If yes, it probably does not have the normal Fixed power button but
> > > instead it has something called "Windows button array device" with
> > > _HID/_CID of PNP0C40. Looking at your dsdt.dsl, this looks to be the
> > > case.
> > > 
> > > That device is driven by soc_button_array.c driver which can be enabled
> > > with CONFIG_KEYBOARD_GPIO=y and CONFIG_INPUT_SOC_BUTTON_ARRAY=y. Can you
> > > check if you have that enabled already?
> > > 
> > > You should actually see it in /proc/interrupts with names like "power"
> > > and so on.
> > 
> > I added CONFIG_INPUT_SOC_BUTTON_ARRAY=y, but no joy.
> > Maybe because the _HID is INTCFD9, only _CID is PNP0C40?
> > It also has a _DSM with UUID dfbcf3c5-e7a5-44e6-9c1f-29c76f6e059c.
> 
> Or it is because the PNP0C40 device depends on GpioInt from PMIC
> which isn't available...
> 
>             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>             {
>                 Name (CBUF, ResourceTemplate ()
>                 {
>                     GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullUp, 0x0BB8,
>                         "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
>                         )
>                         {   // Pin list
>                             0x0016
>                         }
>                 })
>                 Return (CBUF) /* \_SB_.TBAD._CRS.CBUF */
>             }

Most likely this is the reason. I'll try to find if we have an existing
driver for this PMIC somewhere. I guess this is the Dollar Cove which is
successor of Crystal Cove IIRC which is already supported by the
mainline kernel.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Wed, Sep 21, 2016 at 12:06:14PM +0300, Mika Westerberg wrote:
> On Tue, Sep 20, 2016 at 11:11:53PM +0200, Johannes Stezenbach wrote:
> > Or it is because the PNP0C40 device depends on GpioInt from PMIC
> > which isn't available...
> > 
> >             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> >             {
> >                 Name (CBUF, ResourceTemplate ()
> >                 {
> >                     GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullUp, 0x0BB8,
> >                         "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
> >                         )
> >                         {   // Pin list
> >                             0x0016
> >                         }
> >                 })
> >                 Return (CBUF) /* \_SB_.TBAD._CRS.CBUF */
> >             }
> 
> Most likely this is the reason. I'll try to find if we have an existing
> driver for this PMIC somewhere. I guess this is the Dollar Cove which is
> successor of Crystal Cove IIRC which is already supported by the
> mainline kernel.

There is an ADBG ("TI_DCOVE") in PMI2._STA, so Dollar Cove
sounds like a good guess.


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Wed, Sep 21, 2016 at 11:16:35AM +0200, Johannes Stezenbach wrote:
> On Wed, Sep 21, 2016 at 12:06:14PM +0300, Mika Westerberg wrote:
> > On Tue, Sep 20, 2016 at 11:11:53PM +0200, Johannes Stezenbach wrote:
> > > Or it is because the PNP0C40 device depends on GpioInt from PMIC
> > > which isn't available...
> > > 
> > >             Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> > >             {
> > >                 Name (CBUF, ResourceTemplate ()
> > >                 {
> > >                     GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullUp, 0x0BB8,
> > >                         "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
> > >                         )
> > >                         {   // Pin list
> > >                             0x0016
> > >                         }
> > >                 })
> > >                 Return (CBUF) /* \_SB_.TBAD._CRS.CBUF */
> > >             }
> > 
> > Most likely this is the reason. I'll try to find if we have an existing
> > driver for this PMIC somewhere. I guess this is the Dollar Cove which is
> > successor of Crystal Cove IIRC which is already supported by the
> > mainline kernel.
> 
> There is an ADBG ("TI_DCOVE") in PMI2._STA, so Dollar Cove
> sounds like a good guess.

David (CC'd) is working on getting the Dollar Cove PMIC driver
upstreamed to the mainline kernel.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
> On Wed, Sep 21, 2016 at 11:16:35AM +0200, Johannes Stezenbach wrote:
> > There is an ADBG ("TI_DCOVE") in PMI2._STA, so Dollar Cove
> > sounds like a good guess.
> 
> David (CC'd) is working on getting the Dollar Cove PMIC driver
> upstreamed to the mainline kernel.

Excellent news!  Repeating essential info to avoid any
confusion, the PMIC is

            Device (PMI2)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (_HID, "INT33F5" /* TI PMIC Controller */)  // _HID: Hardware ID
                Name (_CID, "INT33F5" /* TI PMIC Controller */)  // _CID: Compatible ID
                Name (_DDN, "TI PMIC Controller")  // _DDN: DOS Device Name

(Because the INT33F4 XPOWER PMIC also has ADBG ("XPWR_DCOVE")
so I'm not sure "Dollar Cove" is a unique name.)


I put the Asus E200HA DSDT at
https://linuxtv.org/~js/e200ha/


Thanks,
Johannes

Re: Cherryview wake up events

[Johannes Stezenbach]

On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
> David (CC'd) is working on getting the Dollar Cove PMIC driver
> upstreamed to the mainline kernel.

May I ask when to expect a patch?  I'm ready if you
have something to test, even if it's not in
shape for mainline yet.

Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Wed, Oct 05, 2016 at 02:46:48PM +0200, Johannes Stezenbach wrote:
> On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
> > David (CC'd) is working on getting the Dollar Cove PMIC driver
> > upstreamed to the mainline kernel.
> 
> May I ask when to expect a patch?  I'm ready if you
> have something to test, even if it's not in
> shape for mainline yet.

It typically takes quite some time to get all the legal stuff done
before the code can be published. And if people are busy with other
things it takes even more time.

So please be patient, it will happen sooner or later ;-)

Re: Cherryview wake up events

[Johannes Stezenbach]

Hi,

On Wed, Oct 05, 2016 at 04:05:11PM +0300, Mika Westerberg wrote:
> On Wed, Oct 05, 2016 at 02:46:48PM +0200, Johannes Stezenbach wrote:
> > On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
> > > David (CC'd) is working on getting the Dollar Cove PMIC driver
> > > upstreamed to the mainline kernel.
> > 
> > May I ask when to expect a patch?  I'm ready if you
> > have something to test, even if it's not in
> > shape for mainline yet.
> 
> It typically takes quite some time to get all the legal stuff done
> before the code can be published. And if people are busy with other
> things it takes even more time.
> 
> So please be patient, it will happen sooner or later ;-)

I don't want to nag, but just so it doesn't drop off
the TODO list due to "lack of interest":  What's the
status?  Will Santa bring the the TI Dollar Cove PMIC driver?

While I'm at it, I also have questions about S0ix support
in Linux which I didn't find answers to by web search.
Does S0ix depend on the PMIC driver?  And will it be
used during run time or only in "sleep" state
(which would mean "echo freeze >/sys/power/state"
since ACPI S3 isn't supported)?
Now all I know is it doesn't seem to be used so far (running 4.9.0-rc7+):

/sys/kernel/debug/pmc_atom/sleep_state:S0IR Residency:  0us
/sys/kernel/debug/pmc_atom/sleep_state:S0I1 Residency:  0us
/sys/kernel/debug/pmc_atom/sleep_state:S0I2 Residency:  0us
/sys/kernel/debug/pmc_atom/sleep_state:S0I3 Residency:  0us
/sys/kernel/debug/pmc_atom/sleep_state:S0   Residency: 160934496us


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Sun, Dec 04, 2016 at 07:52:19PM +0100, Johannes Stezenbach wrote:
> Hi,
> 
> On Wed, Oct 05, 2016 at 04:05:11PM +0300, Mika Westerberg wrote:
> > On Wed, Oct 05, 2016 at 02:46:48PM +0200, Johannes Stezenbach wrote:
> > > On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
> > > > David (CC'd) is working on getting the Dollar Cove PMIC driver
> > > > upstreamed to the mainline kernel.
> > > 
> > > May I ask when to expect a patch?  I'm ready if you
> > > have something to test, even if it's not in
> > > shape for mainline yet.
> > 
> > It typically takes quite some time to get all the legal stuff done
> > before the code can be published. And if people are busy with other
> > things it takes even more time.
> > 
> > So please be patient, it will happen sooner or later ;-)
> 
> I don't want to nag, but just so it doesn't drop off
> the TODO list due to "lack of interest":  What's the
> status?  Will Santa bring the the TI Dollar Cove PMIC driver?

David, do you have any estimate?

> While I'm at it, I also have questions about S0ix support
> in Linux which I didn't find answers to by web search.
> Does S0ix depend on the PMIC driver?  And will it be
> used during run time or only in "sleep" state
> (which would mean "echo freeze >/sys/power/state"
> since ACPI S3 isn't supported)?

AFAIK it is dependent only on components on the SoC. If all goes well
freeze mode will enter S0ix (or it can be entered runtime as well if all
drivers support runtime PM and have moved devices into D3).

> Now all I know is it doesn't seem to be used so far (running 4.9.0-rc7+):
> 
> /sys/kernel/debug/pmc_atom/sleep_state:S0IR Residency:  0us
> /sys/kernel/debug/pmc_atom/sleep_state:S0I1 Residency:  0us
> /sys/kernel/debug/pmc_atom/sleep_state:S0I2 Residency:  0us
> /sys/kernel/debug/pmc_atom/sleep_state:S0I3 Residency:  0us
> /sys/kernel/debug/pmc_atom/sleep_state:S0   Residency: 160934496us

Yeah, seems that the system has not entered S0ix.

Re: Cherryview wake up events

[Johannes Stezenbach]

Hi,

On Mon, Dec 05, 2016 at 01:06:08PM +0200, Mika Westerberg wrote:
> On Sun, Dec 04, 2016 at 07:52:19PM +0100, Johannes Stezenbach wrote:
> > On Wed, Oct 05, 2016 at 04:05:11PM +0300, Mika Westerberg wrote:
> > > On Wed, Oct 05, 2016 at 02:46:48PM +0200, Johannes Stezenbach wrote:
> > > > On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
> > > > > David (CC'd) is working on getting the Dollar Cove PMIC driver
> > > > > upstreamed to the mainline kernel.
> > > > 
> > > > May I ask when to expect a patch?  I'm ready if you
> > > > have something to test, even if it's not in
> > > > shape for mainline yet.
> > > 
> > > It typically takes quite some time to get all the legal stuff done
> > > before the code can be published. And if people are busy with other
> > > things it takes even more time.
> > > 
> > > So please be patient, it will happen sooner or later ;-)
> > 
> > I don't want to nag, but just so it doesn't drop off
> > the TODO list due to "lack of interest":  What's the
> > status?  Will Santa bring the the TI Dollar Cove PMIC driver?
> 
> David, do you have any estimate?


Meanwhile I found out the TI PMIC and power button drivers
has been published as part of the Asus ZenFone Zoom (ZX551ML)
Android kernel code drop (based on linux-3.10.x):

https://www.asus.com/support/Download/39/1/0/26/BXbNqJplzZiLmk6G/32/

Please let me know if there is anything I could do
to help get it mainlined soon.


Thanks,
Johannes

Re: Cherryview wake up events

[Andy Shevchenko]

On Tue, Jan 24, 2017 at 11:41 AM, Johannes Stezenbach <js@sig21.net> wrote:
> Hi,
>
> On Mon, Dec 05, 2016 at 01:06:08PM +0200, Mika Westerberg wrote:
>> On Sun, Dec 04, 2016 at 07:52:19PM +0100, Johannes Stezenbach wrote:
>> > On Wed, Oct 05, 2016 at 04:05:11PM +0300, Mika Westerberg wrote:
>> > > On Wed, Oct 05, 2016 at 02:46:48PM +0200, Johannes Stezenbach wrote:
>> > > > On Fri, Sep 23, 2016 at 11:19:04AM +0300, Mika Westerberg wrote:
>> > > > > David (CC'd) is working on getting the Dollar Cove PMIC driver
>> > > > > upstreamed to the mainline kernel.
>> > > >
>> > > > May I ask when to expect a patch?  I'm ready if you
>> > > > have something to test, even if it's not in
>> > > > shape for mainline yet.
>> > >
>> > > It typically takes quite some time to get all the legal stuff done
>> > > before the code can be published. And if people are busy with other
>> > > things it takes even more time.
>> > >
>> > > So please be patient, it will happen sooner or later ;-)
>> >
>> > I don't want to nag, but just so it doesn't drop off
>> > the TODO list due to "lack of interest":  What's the
>> > status?  Will Santa bring the the TI Dollar Cove PMIC driver?
>>
>> David, do you have any estimate?
>
>
> Meanwhile I found out the TI PMIC and power button drivers
> has been published as part of the Asus ZenFone Zoom (ZX551ML)
> Android kernel code drop (based on linux-3.10.x):
>
> https://www.asus.com/support/Download/39/1/0/26/BXbNqJplzZiLmk6G/32/
>
> Please let me know if there is anything I could do
> to help get it mainlined soon.

AFAIK ASuS Zenfone 2 (Intel based) series uses Intel Moorefield. It
has ShadyCove PMIC.

-- 
With Best Regards,
Andy Shevchenko

Re: Cherryview wake up events

[Johannes Stezenbach]

On Tue, Jan 24, 2017 at 01:14:16PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 24, 2017 at 11:41 AM, Johannes Stezenbach <js@sig21.net> wrote:
> > Meanwhile I found out the TI PMIC and power button drivers
> > has been published as part of the Asus ZenFone Zoom (ZX551ML)
> > Android kernel code drop (based on linux-3.10.x):
> >
> > https://www.asus.com/support/Download/39/1/0/26/BXbNqJplzZiLmk6G/32/
> >
> > Please let me know if there is anything I could do
> > to help get it mainlined soon.
> 
> AFAIK ASuS Zenfone 2 (Intel based) series uses Intel Moorefield. It
> has ShadyCove PMIC.

So Asus released more than they needed.  I confirmed their
source drop contains the TI Dollar Cove driver (dollar_cove_ti.c).
iPreviously I searched for Android devices using CherryView but the
only one I could find is Xioami MiPad 2 and it's released
kernel source doesn't contain the driver.

Anyway, let me know if I can help to get it into mainline soon.


Johannes

Re: Cherryview wake up events

[Andy Shevchenko]

On Tue, Jan 24, 2017 at 3:52 PM, Johannes Stezenbach <js@sig21.net> wrote:
> On Tue, Jan 24, 2017 at 01:14:16PM +0200, Andy Shevchenko wrote:
>> On Tue, Jan 24, 2017 at 11:41 AM, Johannes Stezenbach <js@sig21.net> wrote:
>> > Meanwhile I found out the TI PMIC and power button drivers
>> > has been published as part of the Asus ZenFone Zoom (ZX551ML)
>> > Android kernel code drop (based on linux-3.10.x):
>> >
>> > https://www.asus.com/support/Download/39/1/0/26/BXbNqJplzZiLmk6G/32/
>> >
>> > Please let me know if there is anything I could do
>> > to help get it mainlined soon.
>>
>> AFAIK ASuS Zenfone 2 (Intel based) series uses Intel Moorefield. It
>> has ShadyCove PMIC.
>
> So Asus released more than they needed.  I confirmed their
> source drop contains the TI Dollar Cove driver (dollar_cove_ti.c).

They probably release just almost all One Big Ugly patch from official
BSP, which by some reason, includes all Intel MID SoCs, Baytrail.
I think I know how Dollar Cove related code ended up there. But that
all stuff is a total mess.

> Anyway, let me know if I can help to get it into mainline soon.

-- 
With Best Regards,
Andy Shevchenko

Re: Cherryview wake up events

[Johannes Stezenbach]

On Tue, Jan 24, 2017 at 04:28:29PM +0200, Andy Shevchenko wrote:
> They probably release just almost all One Big Ugly patch from official
> BSP, which by some reason, includes all Intel MID SoCs, Baytrail.
> I think I know how Dollar Cove related code ended up there. But that
> all stuff is a total mess.

I agree.  Probably it was a mistake to bring up this code here.
Let me try to go back two steps:  Could you please let me
know if there is any progress in mainlining the TI Dollar Cove PMIC
and related drivers?  Is there a schedule?
After waiting for four months I'm actually getting impatient
because by now the Cherryview based hardware seems to go out
of production and I fear the mainlining might never happen.

Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Tue, Jan 24, 2017 at 08:23:14PM +0100, Johannes Stezenbach wrote:
> On Tue, Jan 24, 2017 at 04:28:29PM +0200, Andy Shevchenko wrote:
> > They probably release just almost all One Big Ugly patch from official
> > BSP, which by some reason, includes all Intel MID SoCs, Baytrail.
> > I think I know how Dollar Cove related code ended up there. But that
> > all stuff is a total mess.
> 
> I agree.  Probably it was a mistake to bring up this code here.
> Let me try to go back two steps:  Could you please let me
> know if there is any progress in mainlining the TI Dollar Cove PMIC
> and related drivers?  Is there a schedule?

David, can you please answer this!

> After waiting for four months I'm actually getting impatient
> because by now the Cherryview based hardware seems to go out
> of production and I fear the mainlining might never happen.

If we hear nothing from David soon, I think I'll just take it to my
growing todo list. Unless someone else wants to take it.

Re: Cherryview wake up events

[Andy Shevchenko]

On Tue, Jan 24, 2017 at 9:23 PM, Johannes Stezenbach <js@sig21.net> wrote:
> On Tue, Jan 24, 2017 at 04:28:29PM +0200, Andy Shevchenko wrote:
>> They probably release just almost all One Big Ugly patch from official
>> BSP, which by some reason, includes all Intel MID SoCs, Baytrail.
>> I think I know how Dollar Cove related code ended up there. But that
>> all stuff is a total mess.

I'm reading your long thread about the issue.

> but excluded CONFIG_MFD_AXP20X based on \_SB.PIC0.I2C7.PMI1._STA returning 0 in acpidbg,
> but \_SB.PIC0.I2C7.PMI1._STA returns 0xf

Did you mean PMI2 in the second sentence?
Had you tried to add ID to axp20x-i2c.c ?

> Repeating essential info to avoid any confusion, the PMIC is

In code you found there are two DCove drivers :-) One with "ti" suffix.


-- 
With Best Regards,
Andy Shevchenko

Re: Cherryview wake up events

[Johannes Stezenbach]

On Fri, Jan 27, 2017 at 12:56:53AM +0200, Andy Shevchenko wrote:
> 
> I'm reading your long thread about the issue.

Thanks for taking the time!

> > but excluded CONFIG_MFD_AXP20X based on \_SB.PIC0.I2C7.PMI1._STA returning 0 in acpidbg,
> > but \_SB.PIC0.I2C7.PMI1._STA returns 0xf
> 
> Did you mean PMI2 in the second sentence?

Yes, sorry for copy & paste mistake.  I just repated to confirm:
In acpidbg:

- execute \_SB.PCI0.I2C7.PMI1._STA
Evaluating \_SB.PCI0.I2C7.PMI1._STA
Evaluation of \_SB.PCI0.I2C7.PMI1._STA returned object ffffa14a67420000, external buffer length 18
 [Integer] = 0000000000000000

- execute \_SB.PCI0.I2C7.PMI2._STA
Evaluating \_SB.PCI0.I2C7.PMI2._STA
Evaluation of \_SB.PCI0.I2C7.PMI2._STA returned object ffffa14a67420000, external buffer length 18
 [Integer] = 000000000000000F

And the same info is also in sysfs:

# cat /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/808622C1\:06/INT33F4\:00/status
0
# cat /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/808622C1\:06/INT33F5\:00/status
15

The DSDT is still at https://linuxtv.org/~js/e200ha/

> Had you tried to add ID to axp20x-i2c.c ?

Nope, since I have no idea if the axp and TI hardware is similar.

There might be more issues, currently the machine hangs often
during bootup at random points.  I built i915 as a module and
blacklisted it for autoloading so I can read the last message
on the console.  All I can say is that it is more likely to
hang when the loglevel is high, i.e. it almost never succeeds
with "debug" on kernel command line.  Sometimes there are
timeout errors from I2C:
[    4.307189] i2c_designware 808622C1:06: controller timed out
[    5.331709] i2c_designware 808622C1:06: controller timed out

Once it has booted it is running stable.


Thanks,
Johannes

Re: Cherryview wake up events

[Andy Shevchenko]

On Fri, Jan 27, 2017 at 1:38 PM, Johannes Stezenbach <js@sig21.net> wrote:
> On Fri, Jan 27, 2017 at 12:56:53AM +0200, Andy Shevchenko wrote:

> And the same info is also in sysfs:
>
> # cat /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/808622C1\:06/INT33F4\:00/status
> 0
> # cat /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/808622C1\:06/INT33F5\:00/status
> 15
>
> The DSDT is still at https://linuxtv.org/~js/e200ha/
>
>> Had you tried to add ID to axp20x-i2c.c ?
>
> Nope, since I have no idea if the axp and TI hardware is similar.

I think you would give a try.

> There might be more issues, currently the machine hangs often
> during bootup at random points.  I built i915 as a module and
> blacklisted it for autoloading so I can read the last message
> on the console.  All I can say is that it is more likely to
> hang when the loglevel is high, i.e. it almost never succeeds
> with "debug" on kernel command line.  Sometimes there are
> timeout errors from I2C:
> [    4.307189] i2c_designware 808622C1:06: controller timed out
> [    5.331709] i2c_designware 808622C1:06: controller timed out
>
> Once it has booted it is running stable.

This is known: http://www.spinics.net/lists/intel-gfx/msg117738.html

-- 
With Best Regards,
Andy Shevchenko

Re: Cherryview wake up events

[Johannes Stezenbach]

On Fri, Jan 27, 2017 at 03:21:22PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 27, 2017 at 1:38 PM, Johannes Stezenbach <js@sig21.net> wrote:
> > On Fri, Jan 27, 2017 at 12:56:53AM +0200, Andy Shevchenko wrote:
> 
> > And the same info is also in sysfs:
> >
> > # cat /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/808622C1\:06/INT33F4\:00/status
> > 0
> > # cat /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/808622C1\:06/INT33F5\:00/status
> > 15
> >
> > The DSDT is still at https://linuxtv.org/~js/e200ha/
> >
> >> Had you tried to add ID to axp20x-i2c.c ?
> >
> > Nope, since I have no idea if the axp and TI hardware is similar.
> 
> I think you would give a try.

I'll check it.

> > There might be more issues, currently the machine hangs often
> > during bootup at random points.  I built i915 as a module and
> > blacklisted it for autoloading so I can read the last message
> > on the console.  All I can say is that it is more likely to
> > hang when the loglevel is high, i.e. it almost never succeeds
> > with "debug" on kernel command line.  Sometimes there are
> > timeout errors from I2C:
> > [    4.307189] i2c_designware 808622C1:06: controller timed out
> > [    5.331709] i2c_designware 808622C1:06: controller timed out
> >
> > Once it has booted it is running stable.
> 
> This is known: http://www.spinics.net/lists/intel-gfx/msg117738.html

Not sure because it happens with i915 module not loaded
(currently I load it manually after boot completed).
But thanks for the link.

Johannes

Re: Cherryview wake up events

[Johannes Stezenbach]

On Fri, Jan 27, 2017 at 02:30:58PM +0100, Johannes Stezenbach wrote:
> On Fri, Jan 27, 2017 at 03:21:22PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 27, 2017 at 1:38 PM, Johannes Stezenbach <js@sig21.net> wrote:
> > > On Fri, Jan 27, 2017 at 12:56:53AM +0200, Andy Shevchenko wrote:
> > 
> > >> Had you tried to add ID to axp20x-i2c.c ?
> > >
> > > Nope, since I have no idea if the axp and TI hardware is similar.
> > 
> > I think you would give a try.
> 
> I'll check it.

I checked the reference source code, my impression is the
TI Dollar Cove and and AXP288 are completely different hardware.

> > > [    5.331709] i2c_designware 808622C1:06: controller timed out
> > >
> > This is known: http://www.spinics.net/lists/intel-gfx/msg117738.html

Interestingly via this link I found Intel also published
the TI DCove source in a patch series against an unspecified kernel:
https://github.com/01org/ProductionKernelQuilts
specifically
https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/mfd-intel_soc_pmic-add-TI-variant-of-dollar-cove.patch
https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/PWRBTN-add-driver-for-TI-PMIC.patch
and some more (the series is quite messy).

For the Asus E200HA I'm not sure if the charger and coulomb
counter drivers are needed since charging just works and
the battery status is reported via ACPI.  It seems these
drivers are only for tablets without ACPI support, right?


Thanks,
Johannes

Re: Cherryview wake up events

[Andy Shevchenko]

On Mon, Jan 30, 2017 at 10:57 PM, Johannes Stezenbach <js@sig21.net> wrote:
> On Fri, Jan 27, 2017 at 02:30:58PM +0100, Johannes Stezenbach wrote:
>> On Fri, Jan 27, 2017 at 03:21:22PM +0200, Andy Shevchenko wrote:
>> > On Fri, Jan 27, 2017 at 1:38 PM, Johannes Stezenbach <js@sig21.net> wrote:
>> > > On Fri, Jan 27, 2017 at 12:56:53AM +0200, Andy Shevchenko wrote:
>> >
>> > >> Had you tried to add ID to axp20x-i2c.c ?
>> > >
>> > > Nope, since I have no idea if the axp and TI hardware is similar.
>> >
>> > I think you would give a try.
>>
>> I'll check it.
>
> I checked the reference source code, my impression is the
> TI Dollar Cove and and AXP288 are completely different hardware.

Thanks for checking.

Yes, due to not obvious communication to PMIC. I suppose that the IP
core is quite similar in all of them, the difference is just how OS
and other MCUs in SoC communicate with it.

So, basically what it means that I2C direct communication is prohibited here.

>
>> > > [    5.331709] i2c_designware 808622C1:06: controller timed out
>> > >
>> > This is known: http://www.spinics.net/lists/intel-gfx/msg117738.html
>
> Interestingly via this link I found Intel also published
> the TI DCove source in a patch series against an unspecified kernel:
> https://github.com/01org/ProductionKernelQuilts
> specifically
> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/mfd-intel_soc_pmic-add-TI-variant-of-dollar-cove.patch
> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/PWRBTN-add-driver-for-TI-PMIC.patch
> and some more (the series is quite messy).
>
> For the Asus E200HA I'm not sure if the charger and coulomb
> counter drivers are needed since charging just works and
> the battery status is reported via ACPI.  It seems these
> drivers are only for tablets without ACPI support, right?

Have no idea.

What that code reminds me is MID family of devices. So, power button
is (reasonable) easy to get support of in that case.
Look into drivers/platform/x86/intel_mid_powerbtn.c. I recently
updated it to support Basin Cove on Intel Edison.

-- 
With Best Regards,
Andy Shevchenko

Re: Cherryview wake up events

[Johannes Stezenbach]

Hi Andy and Mika,

On Tue, Jan 31, 2017 at 12:05:07AM +0200, Andy Shevchenko wrote:
> On Mon, Jan 30, 2017 at 10:57 PM, Johannes Stezenbach <js@sig21.net> wrote:
> >
> > I checked the reference source code, my impression is the
> > TI Dollar Cove and and AXP288 are completely different hardware.
> 
> Thanks for checking.
> 
> Yes, due to not obvious communication to PMIC. I suppose that the IP
> core is quite similar in all of them, the difference is just how OS
> and other MCUs in SoC communicate with it.
> 
> So, basically what it means that I2C direct communication is prohibited here.

Not sure about that, but I guess this is needed:
https://lists.freedesktop.org/archives/intel-gfx/2017-January/117696.html

> >> > This is known: http://www.spinics.net/lists/intel-gfx/msg117738.html
> >
> > Interestingly via this link I found Intel also published
> > the TI DCove source in a patch series against an unspecified kernel:
> > https://github.com/01org/ProductionKernelQuilts
> > specifically
> > https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/mfd-intel_soc_pmic-add-TI-variant-of-dollar-cove.patch
> > https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/PWRBTN-add-driver-for-TI-PMIC.patch
> > and some more (the series is quite messy).

FWIW, now I came across yet another source for this driver:
https://android.googlesource.com/kernel/x86/+/android-x86-grant-3.10-marshmallow-mr1-wear-release/drivers/external_drivers/drivers/mfd/intel_pmic/
(but seems to be older)

> > For the Asus E200HA I'm not sure if the charger and coulomb
> > counter drivers are needed since charging just works and
> > the battery status is reported via ACPI.  It seems these
> > drivers are only for tablets without ACPI support, right?
> 
> Have no idea.
> 
> What that code reminds me is MID family of devices. So, power button
> is (reasonable) easy to get support of in that case.
> Look into drivers/platform/x86/intel_mid_powerbtn.c. I recently
> updated it to support Basin Cove on Intel Edison.

You seem to suggest I should try and tackle it myself,
which I would do, but for one I don't want to step on
Mika's toes, secondly ISTR you indicated you have newer,
better source than what is available publicly?
If you want me to take it, please let me know which tree
to work against and any other suggestions you have.

Some more questions:
- Powerbutton driver seems simple enough, the only specialty
  of the TI dcove PB driver is the workarond for lost button
  press event after resume.  However, I still don't see how
  the PB would cause thermal event irqs on E200HA and how the
  PMIC driver would change it?
- Wakeup from freeze state (E200HA doesn't support suspend / ACPI S3)
  is only step 1, to make it usable we need S0ix support.
  Any hints about that?

I think the mfd driver would be similar to intel_soc_pmic_crc.c,
the dollar_cove_ti_powerbtn.c I would keep instead of merging
it into intel_mid_powerbtn.c.  I guess what we need is in
drivers/acpi/pmic/ something similar to intel_pmic_crc.c,
the ProductionKernelQuilts has 0001-ACPI-Adding-support-for-TI-pmic-opregion.patch.


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Tue, Jan 31, 2017 at 03:37:40PM +0100, Johannes Stezenbach wrote:
> You seem to suggest I should try and tackle it myself,
> which I would do, but for one I don't want to step on
> Mika's toes

Trust me, you don't step on my toes if you want to handle this yourself :)

But if noone else is going to anything about this, I'll then put it to
my todo list and work on it when I have some spare time. Currently busy
with some other things, though.

Re: Cherryview wake up events

[Johannes Stezenbach]

Hi Mika,

On Tue, Jan 31, 2017 at 03:37:40PM +0100, Johannes Stezenbach wrote:
> - Powerbutton driver seems simple enough, the only specialty
>   of the TI dcove PB driver is the workarond for lost button
>   press event after resume.  However, I still don't see how
>   the PB would cause thermal event irqs on E200HA and how the
>   PMIC driver would change it?

In ProductionKernelQuilts I found
DC-TI-PMIC-disable-power-button-support.patch so I guess it
might not be needed because it's probably handled by ACPI.

> I think the mfd driver would be similar to intel_soc_pmic_crc.c,
> the dollar_cove_ti_powerbtn.c I would keep instead of merging
> it into intel_mid_powerbtn.c.  I guess what we need is in
> drivers/acpi/pmic/ something similar to intel_pmic_crc.c,
> the ProductionKernelQuilts has 0001-ACPI-Adding-support-for-TI-pmic-opregion.patch.

I have preliminary versions of the mfd and opregion driver,
while testing I found the GPIO opregion is not registered:

Excerpt from DSDT:
https://linuxtv.org/~js/e200ha/dsdt.dsl

            Device (PMI2)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (_HID, "INT33F5" /* TI PMIC Controller */)  // _HID: Hardware ID
                Name (_CID, "INT33F5" /* TI PMIC Controller */)  // _CID: Compatible ID
                Name (_DDN, "TI PMIC Controller")  // _DDN: DOS Device Name
                Name (_HRV, 0x03)  // _HRV: Hardware Revision
                Name (_UID, One)  // _UID: Unique ID
                Name (_DEP, Package (0x02)  // _DEP: Dependencies
                {
                    I2C7, 
                    GPO1
                })
                Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
                {
                    Name (SBUF, ResourceTemplate ()
                    {
                        I2cSerialBusV2 (0x005E, ControllerInitiated, 0x000F4240,
                            AddressingMode7Bit, "\\_SB.PCI0.I2C7",
                            0x00, ResourceConsumer, , Exclusive,
                            )
                        GpioInt (Level, ActiveHigh, Shared, PullDefault, 0x0000,
                            "\\_SB.GPO1", 0x00, ResourceConsumer, ,
                            )
                            {   // Pin list
                                0x000F
                            }
                    })
                    Return (SBUF) /* \_SB_.PCI0.I2C7.PMI2._CRS.SBUF */
                }
...
                Name (AVBL, Zero)
                Name (AVBD, Zero)
                Name (AVBG, Zero)
                Method (_REG, 2, NotSerialized)  // _REG: Region Availability
                {
                    If (Arg0 == 0x08)
                    {
                        AVBG = Arg1
                    }

                    If (Arg0 == 0x8D)
                    {
                        AVBL = Arg1
                    }

                    If (Arg0 == 0x8C)
                    {
                        AVBD = Arg1
                    }
                }


acpidbg:
        \_SB.PCI0.I2C7.PMI2.AVBL Integer      ffff8be7b74d97a8 01 = 0000000000000001
        \_SB.PCI0.I2C7.PMI2.AVBD Integer      ffff8be7b74d94d8 01 = 0000000000000001
        \_SB.PCI0.I2C7.PMI2.AVBG Integer      ffff8be7b74d9be0 01 = 0000000000000000

Any idea about it?
devm_gpiochip_add_data() in chv_gpio_probe() indirectly calls acpi_gpiochip_add()
which should use _DEP to figure out to call _REG, right?

Also PMI2 has

                OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0100)
                Field (GPOP, ByteAcc, NoLock, Preserve)
                {
                    Connection (
                        GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
                            "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
                            )
                            {   // Pin list
                                0x0020
                            }
                    ), 
                    GMP0,   1, 
                    ...
                    (repeat for many more pins)

I guess it means it uses chv_gpio pins and can't work
if the GPIO opregion is not registered?


FWIW, with the mfd driver, /proc/interrupts has

 180:          0          0          0          0  chv-gpio    9  TI Dollar Cove

I guess the 9 refers to the 10th pin in north_pins[] which is pin 0x000F, right?
I boot with "dyndbg=file gpiolib* +p" and get

[  +0.012798] acpi INT33F5:00: GPIO: looking up 0 in _CRS
[  +0.000214] intel_soc_pmic_i2c i2c-INT33F5:00: GPIO lookup for consumer intel_soc_pmic
[  +0.000003] intel_soc_pmic_i2c i2c-INT33F5:00: using ACPI for GPIO lookup
[  +0.000005] acpi INT33F5:00: GPIO: looking up intel_soc_pmic-gpios
[  +0.000005] acpi INT33F5:00: GPIO: looking up intel_soc_pmic-gpio
[  +0.000005] acpi INT33F5:00: GPIO: looking up 0 in _CRS
[  +0.000091] cherryview-pinctrl INT33FF:01: request pin 15 (GPIO_SUS0) for INT33FF:01:406

but so far the irq never triggers.


Thanks
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 10:52:00AM +0100, Johannes Stezenbach wrote:
> Hi Mika,
> 
> On Tue, Jan 31, 2017 at 03:37:40PM +0100, Johannes Stezenbach wrote:
> > - Powerbutton driver seems simple enough, the only specialty
> >   of the TI dcove PB driver is the workarond for lost button
> >   press event after resume.  However, I still don't see how
> >   the PB would cause thermal event irqs on E200HA and how the
> >   PMIC driver would change it?
> 
> In ProductionKernelQuilts I found
> DC-TI-PMIC-disable-power-button-support.patch so I guess it
> might not be needed because it's probably handled by ACPI.
> 
> > I think the mfd driver would be similar to intel_soc_pmic_crc.c,
> > the dollar_cove_ti_powerbtn.c I would keep instead of merging
> > it into intel_mid_powerbtn.c.  I guess what we need is in
> > drivers/acpi/pmic/ something similar to intel_pmic_crc.c,
> > the ProductionKernelQuilts has 0001-ACPI-Adding-support-for-TI-pmic-opregion.patch.
> 
> I have preliminary versions of the mfd and opregion driver,
> while testing I found the GPIO opregion is not registered:

Cool, I take it that you started working on that ;-)

> Excerpt from DSDT:
> https://linuxtv.org/~js/e200ha/dsdt.dsl
> 
>             Device (PMI2)
>             {
>                 Name (_ADR, Zero)  // _ADR: Address
>                 Name (_HID, "INT33F5" /* TI PMIC Controller */)  // _HID: Hardware ID
>                 Name (_CID, "INT33F5" /* TI PMIC Controller */)  // _CID: Compatible ID
>                 Name (_DDN, "TI PMIC Controller")  // _DDN: DOS Device Name
>                 Name (_HRV, 0x03)  // _HRV: Hardware Revision
>                 Name (_UID, One)  // _UID: Unique ID
>                 Name (_DEP, Package (0x02)  // _DEP: Dependencies
>                 {
>                     I2C7, 
>                     GPO1
>                 })
>                 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>                 {
>                     Name (SBUF, ResourceTemplate ()
>                     {
>                         I2cSerialBusV2 (0x005E, ControllerInitiated, 0x000F4240,
>                             AddressingMode7Bit, "\\_SB.PCI0.I2C7",
>                             0x00, ResourceConsumer, , Exclusive,
>                             )
>                         GpioInt (Level, ActiveHigh, Shared, PullDefault, 0x0000,
>                             "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>                             )
>                             {   // Pin list
>                                 0x000F
>                             }
>                     })
>                     Return (SBUF) /* \_SB_.PCI0.I2C7.PMI2._CRS.SBUF */
>                 }
> ...
>                 Name (AVBL, Zero)
>                 Name (AVBD, Zero)
>                 Name (AVBG, Zero)
>                 Method (_REG, 2, NotSerialized)  // _REG: Region Availability
>                 {
>                     If (Arg0 == 0x08)
>                     {
>                         AVBG = Arg1
>                     }
> 
>                     If (Arg0 == 0x8D)
>                     {
>                         AVBL = Arg1
>                     }
> 
>                     If (Arg0 == 0x8C)
>                     {
>                         AVBD = Arg1
>                     }
>                 }
> 
> 
> acpidbg:
>         \_SB.PCI0.I2C7.PMI2.AVBL Integer      ffff8be7b74d97a8 01 = 0000000000000001
>         \_SB.PCI0.I2C7.PMI2.AVBD Integer      ffff8be7b74d94d8 01 = 0000000000000001
>         \_SB.PCI0.I2C7.PMI2.AVBG Integer      ffff8be7b74d9be0 01 = 0000000000000000
> 
> Any idea about it?
> devm_gpiochip_add_data() in chv_gpio_probe() indirectly calls acpi_gpiochip_add()
> which should use _DEP to figure out to call _REG, right?

Actually no, we don't support all _DEP in Linux yet. But that's not the
problem though. See below.

> Also PMI2 has
> 
>                 OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0100)
>                 Field (GPOP, ByteAcc, NoLock, Preserve)
>                 {
>                     Connection (
>                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
>                             "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
>                             )
>                             {   // Pin list
>                                 0x0020
>                             }
>                     ), 
>                     GMP0,   1, 
>                     ...
>                     (repeat for many more pins)
> 
> I guess it means it uses chv_gpio pins and can't work
> if the GPIO opregion is not registered?

That is using GPIO pins of the PMI2 device - the PMIC GPIO driver, I
suppose.

So in addition to the PMIC MFD driver, you need to have a GPIO driver
for Dollar Cove (I guess the quilt patch series included that as well?).

If you look under the /sys/bus/acpi/devices/DEVICE, it should include
bunch of physical_nodeX links. Those are the subdevices of the MFD so
when the GPIO driver registers the GPIO core then automatically installs
GPIO OpRegion handler.

> FWIW, with the mfd driver, /proc/interrupts has
> 
>  180:          0          0          0          0  chv-gpio    9  TI Dollar Cove
> 
> I guess the 9 refers to the 10th pin in north_pins[] which is pin 0x000F, right?
> I boot with "dyndbg=file gpiolib* +p" and get
> 
> [  +0.012798] acpi INT33F5:00: GPIO: looking up 0 in _CRS
> [  +0.000214] intel_soc_pmic_i2c i2c-INT33F5:00: GPIO lookup for consumer intel_soc_pmic
> [  +0.000003] intel_soc_pmic_i2c i2c-INT33F5:00: using ACPI for GPIO lookup
> [  +0.000005] acpi INT33F5:00: GPIO: looking up intel_soc_pmic-gpios
> [  +0.000005] acpi INT33F5:00: GPIO: looking up intel_soc_pmic-gpio
> [  +0.000005] acpi INT33F5:00: GPIO: looking up 0 in _CRS
> [  +0.000091] cherryview-pinctrl INT33FF:01: request pin 15 (GPIO_SUS0) for INT33FF:01:406
> 
> but so far the irq never triggers.

Probably because the PMIC does not have anything to report yet. When you
add the DCOVE GPIO driver, and start receiving input events from the
button array, then you should see that interrupt count increasing. If
everything works correctly.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Thu, Feb 02, 2017 at 12:31:22PM +0200, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 10:52:00AM +0100, Johannes Stezenbach wrote:
> >                 OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0100)
> >                 Field (GPOP, ByteAcc, NoLock, Preserve)
> >                 {
> >                     Connection (
> >                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> >                             "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
> >                             )
> >                             {   // Pin list
> >                                 0x0020
> >                             }
> >                     ), 
> >                     GMP0,   1, 
> >                     ...
> >                     (repeat for many more pins)
> > 
> > I guess it means it uses chv_gpio pins and can't work
> > if the GPIO opregion is not registered?
> 
> That is using GPIO pins of the PMI2 device - the PMIC GPIO driver, I
> suppose.
> 
> So in addition to the PMIC MFD driver, you need to have a GPIO driver
> for Dollar Cove (I guess the quilt patch series included that as well?).

Nope, I see it for AX288 but didn't find it for TI DCove.  And in
current Linus' tree axp288_cells[] doesn't include gpio so
I concluded it's not needed... what am I missing?


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 12:12:22PM +0100, Johannes Stezenbach wrote:
> On Thu, Feb 02, 2017 at 12:31:22PM +0200, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 10:52:00AM +0100, Johannes Stezenbach wrote:
> > >                 OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0100)
> > >                 Field (GPOP, ByteAcc, NoLock, Preserve)
> > >                 {
> > >                     Connection (
> > >                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> > >                             "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
> > >                             )
> > >                             {   // Pin list
> > >                                 0x0020
> > >                             }
> > >                     ), 
> > >                     GMP0,   1, 
> > >                     ...
> > >                     (repeat for many more pins)
> > > 
> > > I guess it means it uses chv_gpio pins and can't work
> > > if the GPIO opregion is not registered?
> > 
> > That is using GPIO pins of the PMI2 device - the PMIC GPIO driver, I
> > suppose.
> > 
> > So in addition to the PMIC MFD driver, you need to have a GPIO driver
> > for Dollar Cove (I guess the quilt patch series included that as well?).
> 
> Nope, I see it for AX288 but didn't find it for TI DCove.  And in
> current Linus' tree axp288_cells[] doesn't include gpio so
> I concluded it's not needed... what am I missing?

So reading your DSDT there is that GPIO button array device \_SB.TBAD
which has one GpioInt() referencing \_SB.PCI0.I2C7.PMI2. I suppose that
is the power button GPIO.

In order to use that there needs to be a GPIO driver exposing those
GPIOs to other drivers. So it is definitely needed.

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 01:35:08PM +0200, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 12:12:22PM +0100, Johannes Stezenbach wrote:
> > On Thu, Feb 02, 2017 at 12:31:22PM +0200, Mika Westerberg wrote:
> > > On Thu, Feb 02, 2017 at 10:52:00AM +0100, Johannes Stezenbach wrote:
> > > >                 OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0100)
> > > >                 Field (GPOP, ByteAcc, NoLock, Preserve)
> > > >                 {
> > > >                     Connection (
> > > >                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> > > >                             "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
> > > >                             )
> > > >                             {   // Pin list
> > > >                                 0x0020
> > > >                             }
> > > >                     ), 
> > > >                     GMP0,   1, 
> > > >                     ...
> > > >                     (repeat for many more pins)
> > > > 
> > > > I guess it means it uses chv_gpio pins and can't work
> > > > if the GPIO opregion is not registered?
> > > 
> > > That is using GPIO pins of the PMI2 device - the PMIC GPIO driver, I
> > > suppose.
> > > 
> > > So in addition to the PMIC MFD driver, you need to have a GPIO driver
> > > for Dollar Cove (I guess the quilt patch series included that as well?).
> > 
> > Nope, I see it for AX288 but didn't find it for TI DCove.  And in
> > current Linus' tree axp288_cells[] doesn't include gpio so
> > I concluded it's not needed... what am I missing?
> 
> So reading your DSDT there is that GPIO button array device \_SB.TBAD
> which has one GpioInt() referencing \_SB.PCI0.I2C7.PMI2. I suppose that
> is the power button GPIO.
> 
> In order to use that there needs to be a GPIO driver exposing those
> GPIOs to other drivers. So it is definitely needed.

Actually, looking again the patches you found:

https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/mfd-intel_soc_pmic-add-TI-variant-of-dollar-cove.patch
https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/PWRBTN-add-driver-for-TI-PMIC.patch

Did you try to them both? The latter seems to handle the power button
by talking directly with the PMIC (instead of using a GPIO).

Let's include the original author (Ramakrishna) as well if we could get
some information from him.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Thu, Feb 02, 2017 at 02:16:39PM +0200, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 01:35:08PM +0200, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 12:12:22PM +0100, Johannes Stezenbach wrote:
> > > On Thu, Feb 02, 2017 at 12:31:22PM +0200, Mika Westerberg wrote:
> > > > On Thu, Feb 02, 2017 at 10:52:00AM +0100, Johannes Stezenbach wrote:
> > > > >                 OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0100)
> > > > >                 Field (GPOP, ByteAcc, NoLock, Preserve)
> > > > >                 {
> > > > >                     Connection (
> > > > >                         GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> > > > >                             "\\_SB.PCI0.I2C7.PMI2", 0x00, ResourceConsumer, ,
> > > > >                             )
> > > > >                             {   // Pin list
> > > > >                                 0x0020
> > > > >                             }
> > > > >                     ), 
> > > > >                     GMP0,   1, 
> > > > >                     ...
> > > > >                     (repeat for many more pins)
> > > > > 
> > > > > I guess it means it uses chv_gpio pins and can't work
> > > > > if the GPIO opregion is not registered?
> > > > 
> > > > That is using GPIO pins of the PMI2 device - the PMIC GPIO driver, I
> > > > suppose.
> > > > 
> > > > So in addition to the PMIC MFD driver, you need to have a GPIO driver
> > > > for Dollar Cove (I guess the quilt patch series included that as well?).
> > > 
> > > Nope, I see it for AX288 but didn't find it for TI DCove.  And in
> > > current Linus' tree axp288_cells[] doesn't include gpio so
> > > I concluded it's not needed... what am I missing?
> > 
> > So reading your DSDT there is that GPIO button array device \_SB.TBAD
> > which has one GpioInt() referencing \_SB.PCI0.I2C7.PMI2. I suppose that
> > is the power button GPIO.
> > 
> > In order to use that there needs to be a GPIO driver exposing those
> > GPIOs to other drivers. So it is definitely needed.
> 
> Actually, looking again the patches you found:
> 
> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/mfd-intel_soc_pmic-add-TI-variant-of-dollar-cove.patch
> https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/PWRBTN-add-driver-for-TI-PMIC.patch
> 
> Did you try to them both? The latter seems to handle the power button
> by talking directly with the PMIC (instead of using a GPIO).

Nope, as I've written earlier:
> In ProductionKernelQuilts I found
> DC-TI-PMIC-disable-power-button-support.patch so I guess it
> might not be needed because it's probably handled by ACPI.

[  +0.000338] input: Power Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input0
[  +0.000127] ACPI: Power Button [PWRB]
...
[  +0.000248] input: Power Button as /devices/LNXSYSTM:00/LNXPWRBN:00/input/input3
[  +0.000116] ACPI: Power Button [PWRF]

And I also have:

[  +0.000004] soc_button_array INTCFD9:00: GPIO lookup for consumer soc_button_array
[  +0.000002] soc_button_array INTCFD9:00: using ACPI for GPIO lookup
[  +0.000003] acpi INTCFD9:00: GPIO: looking up soc_button_array-gpios
[  +0.000004] acpi INTCFD9:00: GPIO: looking up soc_button_array-gpio
[  +0.000003] acpi INTCFD9:00: GPIO: looking up 0 in _CRS
[  +0.000610] soc_button_array INTCFD9:00: lookup for GPIO soc_button_array failed
(repeats for 5 buttons, one of them should succeed)

> Let's include the original author (Ramakrishna) as well if we could get
> some information from him.

Looking at 0002-GPIO-Adding-AXP288-PMIC-GPIO-driver.patch from ProductionKernelQuilts,
it doesn't seem hard to do the same for the TI PMIC, but it needs information
from the PMIC datasheet for irq and gpio control registers.
Hopefully you have a patch or at least could provide the information.


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 02:52:57PM +0100, Johannes Stezenbach wrote:
> Nope, as I've written earlier:
> > In ProductionKernelQuilts I found
> > DC-TI-PMIC-disable-power-button-support.patch so I guess it
> > might not be needed because it's probably handled by ACPI.
> 
> [  +0.000338] input: Power Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input0
> [  +0.000127] ACPI: Power Button [PWRB]
> ...
> [  +0.000248] input: Power Button as /devices/LNXSYSTM:00/LNXPWRBN:00/input/input3
> [  +0.000116] ACPI: Power Button [PWRF]

Yeah, but shouldn't hurt to try it out :)

> And I also have:
> 
> [  +0.000004] soc_button_array INTCFD9:00: GPIO lookup for consumer soc_button_array
> [  +0.000002] soc_button_array INTCFD9:00: using ACPI for GPIO lookup
> [  +0.000003] acpi INTCFD9:00: GPIO: looking up soc_button_array-gpios
> [  +0.000004] acpi INTCFD9:00: GPIO: looking up soc_button_array-gpio
> [  +0.000003] acpi INTCFD9:00: GPIO: looking up 0 in _CRS
> [  +0.000610] soc_button_array INTCFD9:00: lookup for GPIO soc_button_array failed
> (repeats for 5 buttons, one of them should succeed)

It fails because of the missing GPIO driver.

> > Let's include the original author (Ramakrishna) as well if we could get
> > some information from him.
> 
> Looking at 0002-GPIO-Adding-AXP288-PMIC-GPIO-driver.patch from ProductionKernelQuilts,
> it doesn't seem hard to do the same for the TI PMIC, but it needs information
> from the PMIC datasheet for irq and gpio control registers.
> Hopefully you have a patch or at least could provide the information.

That patch looks like a GPIO driver for DCOVE. Did you try it already?

Re: Cherryview wake up events

[Johannes Stezenbach]

On Thu, Feb 02, 2017 at 04:26:18PM +0200, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 02:52:57PM +0100, Johannes Stezenbach wrote:
> > Looking at 0002-GPIO-Adding-AXP288-PMIC-GPIO-driver.patch from ProductionKernelQuilts,
> > it doesn't seem hard to do the same for the TI PMIC, but it needs information
> > from the PMIC datasheet for irq and gpio control registers.
> > Hopefully you have a patch or at least could provide the information.
> 
> That patch looks like a GPIO driver for DCOVE. Did you try it already?

Hell, no.  Without datasheets I can't compare if registers
are compatible between AXP288 and TI DDOVE (SND9039).
Couldn't it damager the hardware if I mess up charger
and voltage regulator related registers?

And current Linus' tree doesn't have the AXP288 GPIO,
and ProductionKernelQuilts doesn't use AXP288 GPIO
for TI DCOVE.

Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 03:31:45PM +0100, Johannes Stezenbach wrote:
> On Thu, Feb 02, 2017 at 04:26:18PM +0200, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 02:52:57PM +0100, Johannes Stezenbach wrote:
> > > Looking at 0002-GPIO-Adding-AXP288-PMIC-GPIO-driver.patch from ProductionKernelQuilts,
> > > it doesn't seem hard to do the same for the TI PMIC, but it needs information
> > > from the PMIC datasheet for irq and gpio control registers.
> > > Hopefully you have a patch or at least could provide the information.
> > 
> > That patch looks like a GPIO driver for DCOVE. Did you try it already?
> 
> Hell, no.  Without datasheets I can't compare if registers
> are compatible between AXP288 and TI DDOVE (SND9039).
> Couldn't it damager the hardware if I mess up charger
> and voltage regulator related registers?

It might yes :)

> And current Linus' tree doesn't have the AXP288 GPIO,
> and ProductionKernelQuilts doesn't use AXP288 GPIO
> for TI DCOVE.

OK, I guess it is easier if I just order one of those machines here and
figure out how to get the PMIC driver working.

Is the model Asus E200HA? Something like this (sorry the information is
in Finnish but the machine should look the same):

https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone

Also can you remind me what exactly is not working so we can prioritize?

Thanks.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Thu, Feb 02, 2017 at 05:02:12PM +0200, Mika Westerberg wrote:
> OK, I guess it is easier if I just order one of those machines here and
> figure out how to get the PMIC driver working.

Oh, I assumed the bottleneck is developer time, not lack of hardware...

> Is the model Asus E200HA? Something like this (sorry the information is
> in Finnish but the machine should look the same):
> 
> https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone

Looks right, mine is E200HA-FD0004TS but I think that just means dark blue color.

> Also can you remind me what exactly is not working so we can prioritize?

There are reports hibernate isn't working, presumably because
the storage is 32MB eMMC.  I've never tried.  It doesn't
support ACPI S3 (suspend-to-RAM).  So currently one has
to boot+shutdown everytime (or keep it running).

1. There seems to be no way to wake it up after "echo freeze >/sys/power/state".
   That is the reason for wanting the power button to wake it up.
   Whether the PB creates an input event is secondary.
   (the LID also doesn't wake it up, but it creates an input event)
2. I've no idea what would be the power consumption in freeze state,
   so I guess support for the S0ix states is needed
3. It randomly hangs at boot, often with a message related to i2c timeout.
   I tried Hans de Goede's patches but it didn't work for me
   (question is if the semphore address is the same for
   AXP288 and TI DCove; the DSDT has the _SEM method so the
   semphore is needed).

Everything else is secondary.  E.g. there is an ADC driver used for thermal
used by the opregion driver, I didn't port it and just implemented
like for AXP288 by simple register reads (which might not work for TI).
We could fix this later.


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 04:42:43PM +0100, Johannes Stezenbach wrote:
> On Thu, Feb 02, 2017 at 05:02:12PM +0200, Mika Westerberg wrote:
> > OK, I guess it is easier if I just order one of those machines here and
> > figure out how to get the PMIC driver working.
> 
> Oh, I assumed the bottleneck is developer time, not lack of hardware...

Well, both actually ;)

> > Is the model Asus E200HA? Something like this (sorry the information is
> > in Finnish but the machine should look the same):
> > 
> > https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone
> 
> Looks right, mine is E200HA-FD0004TS but I think that just means dark blue color.

OK, we have one other Cherrytrail machine here which may have the same
PMIC. We'll check that first and if it does not have the same, I'll
order the above machine.

> > Also can you remind me what exactly is not working so we can prioritize?
> 
> There are reports hibernate isn't working, presumably because
> the storage is 32MB eMMC.  I've never tried.  It doesn't
> support ACPI S3 (suspend-to-RAM).  So currently one has
> to boot+shutdown everytime (or keep it running).
> 
> 1. There seems to be no way to wake it up after "echo freeze >/sys/power/state".
>    That is the reason for wanting the power button to wake it up.
>    Whether the PB creates an input event is secondary.
>    (the LID also doesn't wake it up, but it creates an input event)
> 2. I've no idea what would be the power consumption in freeze state,
>    so I guess support for the S0ix states is needed
> 3. It randomly hangs at boot, often with a message related to i2c timeout.
>    I tried Hans de Goede's patches but it didn't work for me
>    (question is if the semphore address is the same for
>    AXP288 and TI DCove; the DSDT has the _SEM method so the
>    semphore is needed).
> 
> Everything else is secondary.  E.g. there is an ADC driver used for thermal
> used by the opregion driver, I didn't port it and just implemented
> like for AXP288 by simple register reads (which might not work for TI).
> We could fix this later.

OK, thanks.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Thu, Feb 02, 2017 at 05:58:26PM +0200, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 04:42:43PM +0100, Johannes Stezenbach wrote:
> > On Thu, Feb 02, 2017 at 05:02:12PM +0200, Mika Westerberg wrote:
> > > Is the model Asus E200HA? Something like this (sorry the information is
> > > in Finnish but the machine should look the same):
> > > 
> > > https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone
> > 
> > Looks right, mine is E200HA-FD0004TS but I think that just means dark blue color.
> 
> OK, we have one other Cherrytrail machine here which may have the same
> PMIC. We'll check that first and if it does not have the same, I'll
> order the above machine.

In case it is useful to know, I installed Debian stretch following this:
https://wiki.debian.org/InstallingDebianOn/Asus/E200HA

I built my kernel using a relatively minimal kernel config,
let me know if you want it.  I could also post the
two patches which port the mfd and opregion drivers, but
they are straight forward copies of intel_soc_pmic_crc.c
and intel_pmic_crc.c from 4.10.0-rc6+ with code copy&pasted
from the ProductionKernelQuilts patches and s/crc/dc_ti/ etc.,
except I scamped the thermal handler to skip the ADC driver port for now.


Maybe I should've used intel_pmic_xpower.c instead of
intel_pmic_crc.c, since as I write this I see there
is a no-op intel_xpower_pmic_gpio_handler() registered.
This is the trick that fixes this:
        \_SB.PCI0.I2C7.PMI2.AVBG Integer      ffff8be7b74d9be0 01 = 0000000000000001

But now it generates ACPI errors about thermal zone
and "acpi -V" usually hangs it up.

[    5.500927] ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion] (20160930/evregion-300)
[    5.503842] No Local Variables are initialized for method [TMPR]
[    5.506703] No Arguments are initialized for method [TMPR]
[    5.509557] ACPI Error: Method parse/execution failed [\_SB.ATKD.TMPR] (Node ffff8a7d374e87f8), AE_ERROR (20160930/ps
parse-543)
[    5.512481] ACPI Error: Method parse/execution failed [\_SB.ATKD.WMNB] (Node ffff8a7d374e7ed8), AE_ERROR (20160930/ps
parse-543)
[    6.545403] i2c_designware 808622C1:06: controller timed out
[    6.550763] ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion] (20160930/evregion-300)
[    6.555783] No Local Variables are initialized for method [TMPR]
[    6.558769] No Arguments are initialized for method [TMPR]
[    6.561571] ACPI Error: Method parse/execution failed [\_SB.ATKD.TMPR] (Node ffff8a7d374e87f8), AE_ERROR (20160930/ps
parse-543)
[    6.564487] ACPI Error: Method parse/execution failed [\_SB.ATKD.WMNB] (Node ffff8a7d374e7ed8), AE_ERROR (20160930/ps
parse-543)

(I knew my thermal opregion code was preliminary but I didn't expect it to error.)


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 02, 2017 at 06:32:33PM +0100, Johannes Stezenbach wrote:
> On Thu, Feb 02, 2017 at 05:58:26PM +0200, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 04:42:43PM +0100, Johannes Stezenbach wrote:
> > > On Thu, Feb 02, 2017 at 05:02:12PM +0200, Mika Westerberg wrote:
> > > > Is the model Asus E200HA? Something like this (sorry the information is
> > > > in Finnish but the machine should look the same):
> > > > 
> > > > https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone
> > > 
> > > Looks right, mine is E200HA-FD0004TS but I think that just means dark blue color.
> > 
> > OK, we have one other Cherrytrail machine here which may have the same
> > PMIC. We'll check that first and if it does not have the same, I'll
> > order the above machine.
> 
> In case it is useful to know, I installed Debian stretch following this:
> https://wiki.debian.org/InstallingDebianOn/Asus/E200HA
> 
> I built my kernel using a relatively minimal kernel config,
> let me know if you want it.  I could also post the
> two patches which port the mfd and opregion drivers, but
> they are straight forward copies of intel_soc_pmic_crc.c
> and intel_pmic_crc.c from 4.10.0-rc6+ with code copy&pasted
> from the ProductionKernelQuilts patches and s/crc/dc_ti/ etc.,
> except I scamped the thermal handler to skip the ADC driver port for now.

Thanks!

Just for book keeping purposes, can you file a kernel.org bugzilla bug
about this and add all the necessary information, and your patches
there? You can assign the bug directly to me.

Re: Cherryview wake up events

[Johannes Stezenbach]

On Fri, Feb 03, 2017 at 12:00:00PM +0200, Mika Westerberg wrote:
> Just for book keeping purposes, can you file a kernel.org bugzilla bug
> about this and add all the necessary information, and your patches
> there? You can assign the bug directly to me.

I filed it but cannot assign it, added you to CC.
https://bugzilla.kernel.org/show_bug.cgi?id=193891

Thanks,
Johannes

Re: Cherryview wake up events

[Johannes Stezenbach]

On Thu, Feb 02, 2017 at 05:58:26PM +0200, Mika Westerberg wrote:
> On Thu, Feb 02, 2017 at 04:42:43PM +0100, Johannes Stezenbach wrote:
> > On Thu, Feb 02, 2017 at 05:02:12PM +0200, Mika Westerberg wrote:
> > > Is the model Asus E200HA? Something like this (sorry the information is
> > > in Finnish but the machine should look the same):
> > > 
> > > https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone
> > 
> > Looks right, mine is E200HA-FD0004TS but I think that just means dark blue color.
> 
> OK, we have one other Cherrytrail machine here which may have the same
> PMIC. We'll check that first and if it does not have the same, I'll
> order the above machine.

Probably it's too early to ask, but did you go for the E200HA
or what device are you going to use?  And did you start poking
at it, or what timeframe can we expect some patches to test?

BTW, just to clarify about the test patches I added in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=193891
You can use them but I also don't mind if they go to the
garbage can, they were just quickly cobbled together
for testing.


Thanks,
Johannes

Re: Cherryview wake up events

[Mika Westerberg]

On Thu, Feb 09, 2017 at 10:24:26AM +0100, Johannes Stezenbach wrote:
> On Thu, Feb 02, 2017 at 05:58:26PM +0200, Mika Westerberg wrote:
> > On Thu, Feb 02, 2017 at 04:42:43PM +0100, Johannes Stezenbach wrote:
> > > On Thu, Feb 02, 2017 at 05:02:12PM +0200, Mika Westerberg wrote:
> > > > Is the model Asus E200HA? Something like this (sorry the information is
> > > > in Finnish but the machine should look the same):
> > > > 
> > > > https://www.karkkainen.com/verkkokauppa/asus-e200ha-fd0005ts-11-6--hd-kannettava-tietokone
> > > 
> > > Looks right, mine is E200HA-FD0004TS but I think that just means dark blue color.
> > 
> > OK, we have one other Cherrytrail machine here which may have the same
> > PMIC. We'll check that first and if it does not have the same, I'll
> > order the above machine.
> 
> Probably it's too early to ask, but did you go for the E200HA
> or what device are you going to use?  And did you start poking
> at it, or what timeframe can we expect some patches to test?

I ordered E200HA from the above link. It should arrive soon so I have
not yet started working on it. My colleaque (Erik Veijola, CC'd) is
actually doing the real work - I'll help him when there is need.

About the time frame, once the machine arrives we'll start working and
try to get the patches ready as soon as possible (but remember we have
other things to do as well).

Let's use the bugzilla entry to track progress.

> BTW, just to clarify about the test patches I added in bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=193891
> You can use them but I also don't mind if they go to the
> garbage can, they were just quickly cobbled together
> for testing.

OK, thanks.