On Mon, Jan 25, 2021 at 09:18:46AM -0800, Guenter Roeck wrote:

Hi Lukasz,

On Sat, Jan 23, 2021 at 02:42:47AM +0100, Lukasz Majczak wrote:

quoted

There is a missing call to start_tpm_chip before the call to
the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current
approach maight work for tpm2, it fails for tpm1.x - in that case
call to tpm_get_timeouts() or tpm_tis_probe_irq_single() tries to
transmit TPM commands on a disabled chip what what doesn't succeed

s/what what/what/

s/maight/might/

Also, would be nice to have the capatalization of acronyms correct
and consistent. E.g. tpm1.x should be rather written as "TPM 1.x
chips".

It's also incorrect to state that something fails for TPM 1.x chips,
unless you can somehow make a sense that every single TPM 1.x at wild
fails, which probably is not true.

quoted

and in turn causes tpm_tis_core_init() to fail.
Tested on Samsung Chromebook Pro (Caroline).

Anyone can tell me what does Caroline mean anyway?

quoted

Signed-off-by: Lukasz Majczak <redacted>
---
 drivers/char/tpm/tpm_tis_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 92c51c6cfd1b..ff0e5fe46a9d 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c

@@ -1063,12 +1063,16 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
 	init_waitqueue_head(&priv->read_queue);
 	init_waitqueue_head(&priv->int_queue);
 	if (irq != -1) {
+		rc = tpm_chip_start(chip);

Unless I am missing something, the underlying problem seems to be
the calls to tpm1_getcap(). From other code calling this function,
it looks like it may only require tpm_clk_enable() to work.

I don't claim to be bus expert but afaik CLKRUN is used with to power
on/off clock, when using LPC bus.

Also, TPM interface specification speaks about CLKRUN in the section
6.3 "TPM LPC Hardware Protocol".

With that in mind, would it possibly be better to call tpm_clk_enable()
and tpm_clk_disable() around the calls to tpm1_getcap(), ie in
tpm1_get_timeouts() and in tpm_tis_gen_interrupt() ?

This would avoid the unnecessary calls to tpm_chip_start() and
tpm_chip_stop() for tpm2 chips.

Not enough information about hardware and no klog. Cannot make much
conclusions with the information available.

Thanks,
Guenter

Off-topic: I noticed some dumb code in tpm_tis_core.c:

	if (chip->ops->clk_enable != NULL)
		chip->ops->clk_enable(chip, false);

These should be definitely changed as:

       tpm_tis_clkrun_enable(chip, false);

->clk_enable() should be only used in tpm-interface.c.

Not a bug, just really stupid code (and harder to trace).

/Jarkko