Date:   Mon, 13 Mar 2023 11:44:37 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Abel Vesa <abel.vesa@linaro.org>
Cc:     Ulf Hansson <ulf.hansson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S . Miller" <davem@davemloft.net>,
        linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org, linux-crypto@vger.kernel.org,
        linux-scsi@vger.kernel.org
Subject: Re: [RFC PATCH v3 4/7] soc: qcom: Make the Qualcomm UFS/SDCC ICE a
 dedicated driver
Message-ID: <ZA9vFcjLMoifqcsE@sol.localdomain>
References: <20230313115202.3960700-1-abel.vesa@linaro.org>
 <20230313115202.3960700-5-abel.vesa@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230313115202.3960700-5-abel.vesa@linaro.org>
Precedence: bulk

On Mon, Mar 13, 2023 at 01:51:59PM +0200, Abel Vesa wrote:
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> new file mode 100644
> index 000000000000..d664dd598791
> --- /dev/null
> +++ b/drivers/soc/qcom/ice.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm ICE (Inline Crypto Engine) support.
> + *
> + * Copyright (c) 2013-2019, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2019, Google LLC
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +#include <linux/of_platform.h>
> +
> +#include <linux/firmware/qcom/qcom_scm.h>
> +
> +#include <soc/qcom/ice.h>
> +
> +#define AES_256_XTS_KEY_SIZE			64
> +
> +/* QCOM ICE registers */
> +#define QCOM_ICE_REG_VERSION			0x0008
> +#define QCOM_ICE_REG_FUSE_SETTING		0x0010
> +
> +/* QCOM ICE v2.X only */
> +
> +#define QCOM_ICE_REG_BIST_STATUS		0x0070
> +#define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000

The "/* QCOM ICE v2.X only */" comment should be removed, as it's misleading.
This driver only supports v3.  I think this comment also originally described
registers that have now been removed from the file.

> +/* BIST ("built-in self-test"?) status flags */
> +#define QCOM_ICE_BIST_STATUS_MASK		GENMASK(31, 28)

I think we're confident enough in what "BIST" stands for now that the question
mark can be removed.

> +/* Only one ICE instance is currently supported by HW */
> +static bool qcom_ice_check_supported(struct qcom_ice *ice)

I don't see how the comment relates to the function it documents.

> +static int __qcom_ice_enable(struct qcom_ice *ice, bool enable)
> +{
> +	struct device *dev = ice->dev;
> +	int err;
> +
> +	err = clk_prepare_enable(ice->core_clk);
> +	if (err) {
> +		dev_err(dev, "failed to enable core clock (%d)\n",
> +			err);
> +		return err;
> +	}
> +
> +	if (enable) {
> +		qcom_ice_low_power_mode_enable(ice);
> +		qcom_ice_optimization_enable(ice);
> +	}
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err) {
> +		dev_err(dev, "BIST status error (%d)\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}

The 'enable' parameter is confusing.  Maybe call it 'enable_optimizations'?

> +
> +int qcom_ice_program_key(struct qcom_ice *ice, u8 crypto_cap_idx,
> +			 u8 algorithm_id, u8 key_size,
> +			 const u8 crypto_key[], u8 data_unit_size,
> +			 int slot)
> +{
> +	struct device *dev;
> +	union {
> +		u8 bytes[AES_256_XTS_KEY_SIZE];
> +		u32 words[AES_256_XTS_KEY_SIZE / sizeof(u32)];
> +	} key;
> +	int i;
> +	int err;
> +
> +	dev = ice->dev;

Nit: declare and initialize 'dev' on the same line.

> +static struct qcom_ice *qcom_ice_create(struct platform_device *pdev, void __iomem *base)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct qcom_ice *engine;
> +
> +	if (!qcom_scm_is_available())
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!qcom_scm_ice_available()) {
> +		dev_warn(dev, "ICE SCM interface not found\n");
> +		return NULL;
> +	}
> +
> +	engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
> +	if (!engine)
> +		return ERR_PTR(-ENOMEM);
> +
> +	engine->dev = &pdev->dev;
> +	engine->np = np;
> +	engine->base = base;
> +
> +	engine->core_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(engine->core_clk))
> +		return ERR_CAST(engine->core_clk);
> +
> +	if (!qcom_ice_check_supported(engine))
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	dev_info(dev, "Registered Qualcomm Inline Crypto Engine\n");
> +
> +	return engine;

Shouldn't the !qcom_scm_is_available() and !qcom_ice_check_supported() cases
have the same return value?  Both mean not supported, right?

And shouldn't it be NULL, not ERR_PTR(-EOPNOTSUPP), so that the caller doesn't
fail to probe the host controller just because ICE is not supported?

> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> new file mode 100644
> index 000000000000..d4644c9f1bcd
> --- /dev/null
> +++ b/include/soc/qcom/ice.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#ifndef __QCOM_ICE_H__
> +#define __QCOM_ICE_H__
> +
> +#include <linux/err.h>

<linux/types.h> would be more appropriate here, I think.

> +
> +#if IS_ENABLED(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)

This #if does not appear to be necessary.

> +int qcom_ice_enable(struct qcom_ice *ice);
> +int qcom_ice_resume(struct qcom_ice *ice);
> +int qcom_ice_suspend(struct qcom_ice *ice);
> +struct qcom_ice *of_qcom_ice_get(struct device *dev);
> +int qcom_ice_program_key(struct qcom_ice *ice, u8 crypto_cap_idx,
> +			 u8 algorithm_id, u8 key_size,
> +			 const u8 crypto_key[], u8 data_unit_size,
> +			 int slot);

The crypto_cap_idx parameter is unused and should be removed.

> +int qcom_ice_evict_key(struct qcom_ice *ice, int slot);

Nit: these declarations are in a slightly different order from the definitions
in the .c file.

- Eric