On 23-03-13 11:44:37, Eric Biggers wrote:
> 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?
>
Actually, the scm might've not probed yet, so we need to defer.
> 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?
The host controller needs to deal with a not-supported error actually.
We want the ICE instance creation to fail if the driver doesn't support
the HW version.
>
> > 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
