Re: [RFC PATCH 1/5] soc: qcom: Make the Qualcomm UFS/SDCC ICE a dedicated driver

Konrad Dybcio
2023-02-14T13:34:57Z

On 14.02.2023 13:02, Abel Vesa wrote:
> This takes the already existing duplicated support in both ufs-qcom
> and sdhci-msm drivers and makes it a dedicated driver that can be used
> by both mentioned drivers. The reason for this is because, staring with
> SM8550, the ICE IP block is shared between UFS and SDCC, which means we
> need to probe a dedicated device and share it between those two
> consumers. So let's add the ICE dedicated driver as a soc driver.
>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> drivers/soc/qcom/Kconfig | 10 ++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-ice.c | 315 ++++++++++++++++++++++++++++++++++++
> include/soc/qcom/ice.h | 61 +++++++
> 4 files changed, 387 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom-ice.c
> create mode 100644 include/soc/qcom/ice.h
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a8f283086a21..fa76d3ffb4d3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -275,4 +275,14 @@ config QCOM_ICC_BWMON
> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> memory throughput even with lower CPU frequencies.
>
> +config QCOM_INLINE_CRYPTO_ENGINE
> + bool "QCOM UFS & SDCC Inline Crypto Engine driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + depends on SCSI_UFS_CRYPTO || MMC_CRYPTO
> + select QCOM_SCM
> + help
> + Say yes here to support the Qualcomm Inline Crypto Engine driver,
> + providing crypto support to Qualcomm storage drivers like UFS
> + and SDCC.
> +
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 6e88da899f60..30219c164cb0 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -32,3 +32,4 @@ obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> +obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom-ice.o
> diff --git a/drivers/soc/qcom/qcom-ice.c b/drivers/soc/qcom/qcom-ice.c
> new file mode 100644
> index 000000000000..40c9adbc2666
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom-ice.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm ICE (Inline Crypto Engine) support.
> + *
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom_scm.h>
> +
> +#include <soc/qcom/ice.h>
> +
> +#define AES_256_XTS_KEY_SIZE 64
> +
> +/* QCOM ICE registers */
> +
giga nit: other comments don't have a newline between them and
the defines

> +#define QCOM_ICE_REG_CONTROL 0x0000
> +#define QCOM_ICE_REG_RESET 0x0004
> +#define QCOM_ICE_REG_VERSION 0x0008
> +#define QCOM_ICE_REG_FUSE_SETTING 0x0010
> +#define QCOM_ICE_REG_PARAMETERS_1 0x0014
> +#define QCOM_ICE_REG_PARAMETERS_2 0x0018
> +#define QCOM_ICE_REG_PARAMETERS_3 0x001C
> +#define QCOM_ICE_REG_PARAMETERS_4 0x0020
> +#define QCOM_ICE_REG_PARAMETERS_5 0x0024
> +
> +/* QCOM ICE v3.X only */
> +#define QCOM_ICE_GENERAL_ERR_STTS 0x0040
> +#define QCOM_ICE_INVALID_CCFG_ERR_STTS 0x0030
> +#define QCOM_ICE_GENERAL_ERR_MASK 0x0044
> +
> +/* QCOM ICE v2.X only */
> +#define QCOM_ICE_REG_NON_SEC_IRQ_STTS 0x0040
> +#define QCOM_ICE_REG_NON_SEC_IRQ_MASK 0x0044
> +
> +#define QCOM_ICE_REG_NON_SEC_IRQ_CLR 0x0048
> +#define QCOM_ICE_REG_STREAM1_ERROR_SYNDROME1 0x0050
> +#define QCOM_ICE_REG_STREAM1_ERROR_SYNDROME2 0x0054
> +#define QCOM_ICE_REG_STREAM2_ERROR_SYNDROME1 0x0058
> +#define QCOM_ICE_REG_STREAM2_ERROR_SYNDROME2 0x005C
> +#define QCOM_ICE_REG_STREAM1_BIST_ERROR_VEC 0x0060
> +#define QCOM_ICE_REG_STREAM2_BIST_ERROR_VEC 0x0064
> +#define QCOM_ICE_REG_STREAM1_BIST_FINISH_VEC 0x0068
> +#define QCOM_ICE_REG_STREAM2_BIST_FINISH_VEC 0x006C
> +#define QCOM_ICE_REG_BIST_STATUS 0x0070
> +#define QCOM_ICE_REG_BYPASS_STATUS 0x0074
> +#define QCOM_ICE_REG_ADVANCED_CONTROL 0x1000
> +#define QCOM_ICE_REG_ENDIAN_SWAP 0x1004
> +#define QCOM_ICE_REG_TEST_BUS_CONTROL 0x1010
> +#define QCOM_ICE_REG_TEST_BUS_REG 0x1014
> +
> +/* BIST ("built-in self-test"?) status flags */
> +#define QCOM_ICE_BIST_STATUS_MASK 0xF0000000
GENMASK(31, 28)?

btw, most of these defines seem unused?

> +
> +#define QCOM_ICE_FUSE_SETTING_MASK 0x1
> +#define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK 0x2
> +#define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK 0x4
> +
> +#define qcom_ice_writel(engine, val, reg) \
> + writel((val), (engine)->base + (reg))
> +
> +#define qcom_ice_readl(engine, reg) \
> + readl((engine)->base + (reg))
> +
> +/* Only one ICE instance is supported currently by HW */
s/supported currently/currently supported if you meant
"on current SoCs / this driver revision, only one ICE
instance can be running at the same time but that may
change"

or

s/currently/concurrently/ if you meant "only one ICE
instance can be running simultaneously and that won't
change"

> +static struct qcom_ice *engine;
> +
> +static bool qcom_ice_check_supported(struct qcom_ice *ice)
> +{
> + u32 regval = qcom_ice_readl(ice, QCOM_ICE_REG_VERSION);
> + struct device *dev = ice->dev;
> + int major = regval >> 24;
> + int minor = (regval >> 16) & 0xFF;
> + int step = regval & 0xFFFF;
FIELD_GET?

> +
> + ice->supported = true;
> +
> + /* For now this driver only supports ICE version 3. */
> + if (major != 3) {
> + dev_warn(dev, "Unsupported ICE version: v%d.%d.%d\n",
> + major, minor, step);
> + ice->supported = false;
> + goto out;
> + }
> +
> + dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
> + major, minor, step);
> +
> + /* If fuses are blown, ICE might not work in the standard way. */
> + regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING);
> + if (regval & (QCOM_ICE_FUSE_SETTING_MASK |
> + QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK |
> + QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK)) {
> + dev_warn(dev, "Fuses are blown; ICE is unusable!\n");
> + ice->supported = false;
> + goto out;
> + }
> +out:
> + return ice->supported;
> +}
> +
> +void qcom_ice_low_power_mode_enable(struct qcom_ice *ice)
> +{
> + u32 regval;
> +
> + if (!ice)
> + return;
> +
> + regval = qcom_ice_readl(ice, QCOM_ICE_REG_ADVANCED_CONTROL);
> + /*
> + * Enable low power mode sequence
> + * [0]-0, [1]-0, [2]-0, [3]-E, [4]-0, [5]-0, [6]-0, [7]-0
Pardon my ignorance, but I have no idea how this comment corresponds
to the value OR'd..

> + */
> + regval |= 0x7000;
> + qcom_ice_writel(ice, regval, QCOM_ICE_REG_ADVANCED_CONTROL);
> +}
> +
> +static void qcom_ice_optimization_enable(struct qcom_ice *ice)
> +{
> + u32 regval;
> +
> + if (!ice)
> + return;
> +
> + /* ICE Optimizations Enable Sequence */
> + regval = qcom_ice_readl(ice, QCOM_ICE_REG_ADVANCED_CONTROL);
> + regval |= 0xD807100;
Please use lowercase hex, or de-magic-ify this if you have the means to.

> + /* ICE HPG requires delay before writing */
> + udelay(5);
> + qcom_ice_writel(ice, regval, QCOM_ICE_REG_ADVANCED_CONTROL);
> + udelay(5);
> +}
> +
> +/* Poll until all BIST bits are reset */
> +static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
> +{
> + int count;
> + u32 reg;
> +
> + if (!ice)
> + return 0;
> +
> + for (count = 0; count < 100; count++) {
> + reg = qcom_ice_readl(ice, QCOM_ICE_REG_BIST_STATUS);
> + if (!(reg & QCOM_ICE_BIST_STATUS_MASK))
> + break;
> + udelay(50);
> + }
> +
> + if (reg)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +int qcom_ice_resume(struct qcom_ice *ice)
> +{
> + struct device *dev = ice->dev;
> + int err;
> +
> + if (!ice)
> + return 0;
> +
> + err = qcom_ice_wait_bist_status(ice);
> + if (err) {
> + dev_err(dev, "BIST status error (%d)\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_resume);
> +
> +int qcom_ice_enable(struct qcom_ice *ice)
> +{
> + if (!ice)
> + return 0;
> +
> + qcom_ice_low_power_mode_enable(ice);
> + qcom_ice_optimization_enable(ice);
> +
> + return qcom_ice_resume(ice);
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_enable);
> +
> +/*
> + * Program a key into a QC ICE keyslot, or evict a keyslot. QC ICE requires
double space before "QC ICE requires"

> + * vendor-specific SCM calls for this; it doesn't support the standard way.
> + */
> +int qcom_ice_program_key(struct qcom_ice *ice, bool config_enable,
> + u8 crypto_cap_idx, u8 algorithm_id,
> + u8 key_size, const u8 crypto_key[],
> + u8 data_unit_size, int slot)
> +{
> + struct device *dev = ice->dev;
> + union {
> + u8 bytes[AES_256_XTS_KEY_SIZE];
> + u32 words[AES_256_XTS_KEY_SIZE / sizeof(u32)];
> + } key;
> + int i;
> + int err;
> +
> + if (!config_enable)
> + return qcom_scm_ice_invalidate_key(slot);
> +
> + /* Only AES-256-XTS has been tested so far. */
> + if (algorithm_id != QCOM_ICE_CRYPTO_ALG_AES_XTS ||
> + key_size != QCOM_ICE_CRYPTO_KEY_SIZE_256) {
> + dev_err_ratelimited(dev,
> + "Unhandled crypto capability; algorithm_id=%d, key_size=%d\n",
> + algorithm_id, key_size);
> + return -EINVAL;
> + }
> +
> + memcpy(key.bytes, crypto_key, AES_256_XTS_KEY_SIZE);
> +
> + /*
> + * The SCM call byte-swaps the 32-bit words of the key. So we have to
> + * do the same, in order for the final key be correct.
> + */
> + for (i = 0; i < ARRAY_SIZE(key.words); i++)
> + __cpu_to_be32s(&key.words[i]);
> +
> + err = qcom_scm_ice_set_key(slot, key.bytes, AES_256_XTS_KEY_SIZE,
> + QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> + data_unit_size);
> +
> + memzero_explicit(&key, sizeof(key));
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_program_key);
> +
> +struct qcom_ice *of_qcom_ice_get(struct device *dev)
> +{
> + struct qcom_ice *ice = ERR_PTR(-EPROBE_DEFER);
> + struct device_node *np, *node;
> +
> + if (!dev || !dev->of_node)
> + return ERR_PTR(-ENODEV);
> +
> + np = dev->of_node;
> +
> + node = of_parse_phandle(np, "qcom,ice", 0);
> +
> + if (engine && engine->supported)
> + if (node == engine->np)
> + ice = engine;
> +
> + of_node_put(node);
> +
> + return ice;
> +}
> +EXPORT_SYMBOL_GPL(of_qcom_ice_get);
> +
> +int qcom_ice_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> +
> + if (!qcom_scm_ice_available()) {
> + dev_warn(dev, "ICE SCM interface not found\n");
> + return 0;
> + }
> +
> + engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
> + if (!engine)
> + return -ENOMEM;
> +
> + engine->dev = dev;
> + engine->np = np;
> +
> + engine->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(engine->base))
> + return PTR_ERR(engine->base);
> +
> + engine->core_clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(engine->core_clk))
> + return dev_err_probe(dev, PTR_ERR(engine->core_clk),
> + "failed to get core clk\n");
> +
> + if (!qcom_ice_check_supported(engine))
> + return -EOPNOTSUPP;
> +
> + dev_set_drvdata(dev, engine);
> +
> + dev_info(dev, "Registered Qualcomm Inline Crypto Engine\n");
Not sure if we want two lines informing us about the presence of ICE
(the other one prints the hw version).. dev_dbg, perhaps?

> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_ice_of_match_table[] = {
> + {
> + .compatible = "qcom,inline-crypto-engine",
> + },
I think the common way to go about this is to keep the OF match table
entries in a single line, unless you're adding a long-named match_data
variable, so:

{ .compatible = "qcom,inline-crypto-engine" },

> + { },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
> +
> +static struct platform_driver qcom_ice_driver = {
> + .probe = qcom_ice_probe,
> + .driver = {> + .name = "qcom-ice",
> + .of_match_table = qcom_ice_of_match_table,
> + },
Please use either spaces or tabs, consistently, near the
assignments.

Konrad
> +};
> +
> +module_platform_driver(qcom_ice_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Inline Crypto Engine driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> new file mode 100644
> index 000000000000..3ee8add7aeb9
> --- /dev/null
> +++ b/include/soc/qcom/ice.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2023, Linaro Limited
> + */
> +
> +#ifndef __QCOM_ICE_H__
> +#define __QCOM_ICE_H__
> +
> +#include <linux/err.h>
> +
> +struct qcom_ice {
> + struct device *dev;
> + struct device_node *np;
> + void __iomem *base;
> +
> + struct clk *core_clk;
> +
> + bool supported;
> +};
> +
> +enum qcom_ice_crypto_key_size {
> + QCOM_ICE_CRYPTO_KEY_SIZE_INVALID = 0x0,
> + QCOM_ICE_CRYPTO_KEY_SIZE_128 = 0x1,
> + QCOM_ICE_CRYPTO_KEY_SIZE_192 = 0x2,
> + QCOM_ICE_CRYPTO_KEY_SIZE_256 = 0x3,
> + QCOM_ICE_CRYPTO_KEY_SIZE_512 = 0x4,
> +};
> +
> +enum qcom_ice_crypto_alg {
> + QCOM_ICE_CRYPTO_ALG_AES_XTS = 0x0,
> + QCOM_ICE_CRYPTO_ALG_BITLOCKER_AES_CBC = 0x1,
> + QCOM_ICE_CRYPTO_ALG_AES_ECB = 0x2,
> + QCOM_ICE_CRYPTO_ALG_ESSIV_AES_CBC = 0x3,
> +};
> +
> +#if IS_ENABLED(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)
> +int qcom_ice_enable(struct qcom_ice *ice);
> +int qcom_ice_resume(struct qcom_ice *ice);
> +struct qcom_ice *of_qcom_ice_get(struct device *dev);
> +int qcom_ice_program_key(struct qcom_ice *ice, bool config_enable,
> + u8 crypto_cap_idx, u8 algorithm_id, u8 key_size,
> + const u8 crypto_key[], u8 data_unit_size,
> + int slot);
> +#else
> +static int qcom_ice_enable(struct qcom_ice *ice) { return 0; }
> +static int qcom_ice_resume(struct qcom_ice *ice) { return 0; }
> +
> +static struct qcom_ice *of_qcom_ice_get(struct device *dev)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +int qcom_ice_program_key(struct qcom_ice *ice, bool config_enable,
> + u8 crypto_cap_idx, u8 algorithm_id, u8 key_size,
> + const u8 crypto_key[], u8 data_unit_size,
> + int slot)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_QCOM_INLINE_CRYPTO_ENGINE */
> +#endif /* __QCOM_ICE_H__ */