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.
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.
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
