Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

Could you also put this into linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org?

On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
> SW to indicate that the device can enter or should exit the idle state.
> 
> The legacy ACPI-start (SMI + DMA) based devices do not support these
> bits and the idle state management is not exposed to the host SW.
> Thus, this functionality only is enabled only for a CRB start (MMIO)
> based devices.
> 
> Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> oringal patch:
> 'tpm_crb: implement power tpm crb power management'
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> V2: do not export the functions via tpm ops

I'm not sure about this. Even if callbacks are there tpm_crb and other
device drivers can use runtime PM internally (if they want).

>  drivers/char/tpm/tpm_crb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 6e9d1bca712f..49023ac3dea1 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -83,6 +83,68 @@ struct crb_priv {
>  	u32 cmd_size;
>  };
>  
> +/**
> + * crb_go_idle - write crb_ctrl_req_go_idle to tpm_crb_ctrl_req
> + *    the device should respond within timeout_c by clearing the bit.
> + *    anyhow, we do not wait here as a consequent cmd_ready request
> + *    will be handled correctly even if idle was not completed.

Why the function descriptions have different formatting than elsewhere
in the subsystem?

> + *
> + * @dev:  crb device

'pdev' would be a better name to differentiate from character device.  I
know that in crb_acpi_add the name of the local is dev but it was just a
bad choice by me.

Also documentation could be:

@pdev:	CRB platform device

> + * @priv: crb private data

@priv:	CRB private data

> + * return:  0 always

According to

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

it should be 'Return:'.

Why this isn't a void function?

> + */
> +static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
> +{
> +	if (priv->flags & crb_fl_acpi_start)
> +		return 0;
> +
> +	iowrite32(crb_ctrl_req_go_idle, &priv->cca->req);
> +	/* we don't really care when this settles */
> +
> +	return 0;
> +}
> +
> +/**
> + * crb_cmd_ready - write crb_ctrl_req_cmd_ready to tpm_crb_ctrl_req
> + *      and poll till the device acknowledge it by clearing the bit.
> + *      the device should respond within timeout_c.
> + *
> + *      the function does nothing for devices with acpi-start method
> + *
> + * @dev:  crb device
> + * @priv: crb private data
> + *
> + * return:  0 on success -etime on timeout;

Same stuff about the documentation as for the previous function.

Also, I don't like the naming. I would rather have the names I did for
[1]. There I have 'go_to_idle' and 'go_to_ready', which are much more
obvious. I'm can live also with go_ready and go_idle if you prefer that.

> + */
> +static int __maybe_unused crb_cmd_ready(struct device *dev,
> +					struct crb_priv *priv)
> +{
> +	ktime_t stop, start;
> +
> +	if (priv->flags & crb_fl_acpi_start)
> +		return 0;
> +
> +	iowrite32(crb_ctrl_req_cmd_ready, &priv->cca->req);
> +
> +	start = ktime_get();
> +	stop = ktime_add(start, ms_to_ktime(tpm2_timeout_c));
> +	do {
> +		if (!(ioread32(&priv->cca->req) & crb_ctrl_req_cmd_ready)) {
> +			dev_dbg(dev, "cmdready in %lld usecs\n",
> +				ktime_to_us(ktime_sub(ktime_get(), start)));
> +			return 0;
> +		}
> +		usleep_range(50, 100);
> +	} while (ktime_before(ktime_get(), stop));
> +
> +	if (ioread32(&priv->cca->req) & crb_ctrl_req_cmd_ready) {
> +		dev_warn(dev, "cmdready timed out\n");
> +		return -etime;
> +	}
> +
> +	return 0;
> +}
> +

Please use wait_for_tpm_stat(). Please argument in th commit message
if you don't. So far the arguments haven't made sense to me.

I think the whole status thing should be redesigned to have common
synthetized status code shared by all TPM drivers but the way I use it
in [1] works. It's bit ugly but I rather have that than duplicate code.

>  static simple_dev_pm_ops(crb_pm, tpm_pm_suspend, tpm_pm_resume);
>  
>  static u8 crb_status(struct tpm_chip *chip)
> -- 
> 2.7.4
> 

[1] http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/7a1172b5b3cb38083ae931309db216db3c528efe

/Jarkko

------------------------------------------------------------------------------