Could you also put this into linux-ker...@vger.kernel.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.sakki...@linux.intel.com>
> oringal patch:
> 'tpm_crb: implement power tpm crb power management'
>
> Signed-off-by: Tomas Winkler <tomas.wink...@intel.com>
> ---
> 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
------------------------------------------------------------------------------
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
