RE: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on Braswell system

[Shaikh, Azhar]

>-----Original Message-----
>From: linux-integrity-owner@xxxxxxxxxxxxxxx [mailto:linux-integrity-
>owner@xxxxxxxxxxxxxxx] On Behalf Of Shaikh, Azhar
>Sent: Monday, December 18, 2017 11:34 AM
>To: Javier Martinez Canillas <javierm@xxxxxxxxxx>; Jason Gunthorpe
><jgg@xxxxxxxx>
>Cc: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>; James Ettle
><james@xxxxxxxxxxxx>; linux-integrity@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; james.l.morris@xxxxxxxxxx
>Subject: RE: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on
>Braswell system
>
>
>
>>-----Original Message-----
>>From: Javier Martinez Canillas [mailto:javierm@xxxxxxxxxx]
>>Sent: Monday, December 18, 2017 11:30 AM
>>To: Jason Gunthorpe <jgg@xxxxxxxx>
>>Cc: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>; James Ettle
>><james@xxxxxxxxxxxx>; linux-integrity@xxxxxxxxxxxxxxx; Shaikh, Azhar
>><azhar.shaikh@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
>>james.l.morris@xxxxxxxxxx
>>Subject: Re: [BISECTED] tpm CLKRUN breaks PS/2 keyboard and touchpad on
>>Braswell system
>>
>>Hello Jason,
>>
>>Thanks a lot for your feedback.
>>
>>On 12/18/2017 06:55 PM, Jason Gunthorpe wrote:
>>> On Mon, Dec 18, 2017 at 01:29:01PM +0100, Javier Martinez Canillas wrote:
>>>> On 12/18/2017 01:22 PM, Javier Martinez Canillas wrote:
>>>>
>>>> [snip]
>>>>
>>>>>
>>>>> James,
>>>>>
>>>>> Can you please test the following (untested) patch on top of the
>>>>> other two mentioned patches to see if it makes a difference for you?
>>>>>
>>>>
>>>> I should had tried to at least compile the patch :)
>>>
>>> I think this is backwards..
>>>
>>> If CLKRUN_EN is on (eg power management is NOT enabled on LPC) then
>>> TPM shouldn't do anything at all.
>>>
>>> If CLKRUN_EN is off, then it should try to turn it on/off to save
>>> power.
>>>
>>
>>My knowledge of LPC is near to non-existent so I please forgive me if
>>I'm wrong, but my understanding is that it's the opposite of what you said.
>>
>>When CLKRUN_EN is SET, power management is ENABLED on the LPC bus
>and
>>the host LCLK clock may be stopped when entering in some low-power
>states.
>>
>>The CLKRUN# signal is then used by peripherals to restart the LCLK
>>clock after resuming from low-power states to be able to transmit again.
>>
>>When CLKRUN_EN is CLEAR, power management is DISABLED on the LPC bus
>>and the host LCLK clock is never stopped even when entering in some
>>low- power states.
>>
>
>Hi Javier,
>
>Yes you are correct with the above understanding of the CLKRUN.
>
>>IIUC, if CLKRUN_EN is enabled, then all the devices attached to the LPC
>>bus have to support the CLKRUN protocol. My guess is that on some
>>Braswell systems LPC power management is enabled but the TPM device
>>doesn't have CLKRUN support.
>>
>
>I think this is what might be happening here.
>
>Since I do not have an end product to test this on, I managed to get one BSW
>Reference Verification Platform(RVP). I flashed the latest Ubuntu (17.10)
>which is on 4.13.13 kernel version and does have the patch.
>

The correct kernel version is 4.13.0-19

>I was able to use the PS/2 mouse and keyboard immediately after bootup and
>also after suspend/resume cycle. The system does have a TPM.(/dev/tpm0
>and /dev/tpmrm0 exist)
>
>

The RVP has a Nuvoton TPM

>>And that was the motivation of commit 5e572cab92f0 ("tpm: Enable CLKRUN
>>protocol for Braswell systems") that introduced the regression.
>>
>>> Perhaps the best work around is to just delete the turning off of
>>> CLKRUN_EN ? Uses more power but keeps the clock running which should
>>> keep both TPM and superio happy.
>>>
>>
>>But that's exactly the goal of the commit 5e572cab92f0, to disable the
>>CLKRUN protocol (probably for what I mentioned before). So doing so
>>will cause issues for those systems again.
>>
>>What the patch I shared with James does is to avoid disabling the
>>CLKRUN_EN if this is already disabled. Which should be a no-op anyways
>>but the problem is that latter it's enabled even when the driver found
>disabled to start with.
>>
>>I still don't like the overall solution since it's too error prone.
>>Touching a global bus configuration in a driver for a single peripheral isn't
>safe to say the least.
>>
>>But regardless of that, the patch I shared has merits on its own since
>>is wrong to keep the bus configuration in a different state that was
>>before the driver was probed. And in fact, James reported that it makes
>>his system to work again.
>>
>>> Jason
>>>
>>
>>Best regards,
>>--
>>Javier Martinez Canillas
>>Software Engineer - Desktop Hardware Enablement Red Hat