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
