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

[James Ettle]

The keyboard and touchpad work OK with the patch quoted below and the earlier two applied, i.e. the three patches with signatures:

667dcc75be864ff4c17cf58891853b7393bba3e2
db3248e8a036c39141c8f7e9f1cf5c5ae6815f76
370d45a34dc8914066a995a3a6d6df1953ea9f60

I applied these to a vanilla kernel.org 4.14.7 source using Fedora 4.14.5-300.fc27 .config. Confirmed the tpm modules are loaded.

Tests:
1. Keyboard and touchpad work OK on boot - PASS
2. Still work after suspend/resume - PASS

Let me know if you want any further tests.

Many thanks,
James.


On 18/12/17 12:29, 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 :)
> 
> Updated patch below:
> 
> From 370d45a34dc8914066a995a3a6d6df1953ea9f60 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> Date: Mon, 18 Dec 2017 12:56:28 +0100
> Subject: [PATCH v2] tpm: only attempt to disable the LPC CLKRUN if is already
>  enabled
> 
> Commit 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> added logic in the TPM TIS driver to disable the Low Pin Count CLKRUN
> signal during TPM transactions.
> 
> Unfortunately this breaks other devices that are attached to the LPC bus
> like for example PS/2 mouse and keyboards.
> 
> One flaw with the logic is that it assumes that the CLKRUN is always
> enabled, and so it unconditionally enables it after a TPM transaction.
> 
> But it could be that the CLKRUN signal was already disabled in the LPC
> bus and so after the driver probes, the signal will remain enabled which
> may break other devices transactions since the clocks will be restarted
> by the CLKRUN# signal.
> 
> Fixes: 5e572cab92f0 ("tpm: Enable CLKRUN protocol for Braswell systems")
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index e7bd2e750f69..5f2b1fc2194f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -688,7 +688,8 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value)
>  	struct tpm_tis_data *data = dev_get_drvdata(&chip->dev);
>  	u32 clkrun_val;
>  
> -	if (!IS_ENABLED(CONFIG_X86) || !is_bsw())
> +	if (!IS_ENABLED(CONFIG_X86) || !is_bsw() ||
> +	    !data->ilb_base_addr)
>  		return;
>  
>  	if (value) {
> @@ -746,7 +747,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		      const struct tpm_tis_phy_ops *phy_ops,
>  		      acpi_handle acpi_dev_handle)
>  {
> -	u32 vendor, intfcaps, intmask;
> +	u32 vendor, intfcaps, intmask, clkrun_val;
>  	u8 rid;
>  	int rc, probe;
>  	struct tpm_chip *chip;
> @@ -772,6 +773,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  					ILB_REMAP_SIZE);
>  		if (!priv->ilb_base_addr)
>  			return -ENOMEM;
> +
> +		clkrun_val = ioread32(priv->ilb_base_addr + LPC_CNTRL_OFFSET);
> +		/* Check if CLKRUN# is already not enabled in the LPC bus */
> +		if (!(clkrun_val & LPC_CLKRUN_EN)) {
> +			priv->flags |= TPM_TIS_CLK_ENABLE;
> +			iounmap(priv->ilb_base_addr);
> +			priv->ilb_base_addr = NULL;
> +		}
>  	}
>  
>  	if (chip->ops->clk_enable != NULL)
> @@ -868,7 +877,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	}
>  
>  	rc = tpm_chip_register(chip);
> -	if (rc && is_bsw())
> +	if (rc && is_bsw() && priv->ilb_base_addr)
>  		iounmap(priv->ilb_base_addr);
>  
>  	if (chip->ops->clk_enable != NULL)
>