From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C917C76195 for ; Fri, 24 Mar 2023 20:08:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231869AbjCXUI0 (ORCPT ); Fri, 24 Mar 2023 16:08:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47906 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231623AbjCXUIZ (ORCPT ); Fri, 24 Mar 2023 16:08:25 -0400 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4EC141421B for ; Fri, 24 Mar 2023 13:08:23 -0700 (PDT) Received: by mail-ed1-x52a.google.com with SMTP id w9so12358479edc.3 for ; Fri, 24 Mar 2023 13:08:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1679688502; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=/nXQwSluZLeTH688ZepgBtPa+FZFYWScgxaIXsWUlLM=; b=zOa1jp5Kvn7C2FtxHhAfS1DGISKaM696BDoCyWCS/u3zYWs4ZnRxOYsqzLNtOP0mxh kHlqSsolgSqOIXOAhjPuQGkdlmKSS4G9C18z9FzEQx3fNzQJc06SPOcB8H3EMif96tNi Fs2b1Nb1viDFUrZYhqv0ky6pvdmhbcBpz0K46Dp2ichIslDrpzCmouZqr9pDEv00Z95m Ejdgv7c4559vpsfHTPzE12co69RTynBbriIfXneNQCgz/dyeHlyxLNqIi6F6VTbbD2vr zG3Yl612KB/7W1DCHdOQaSAJIbDfVup50nwFv2p5Ap6C7U9lMXP+kbdqzMvnlq3vBn+h IvNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679688502; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=/nXQwSluZLeTH688ZepgBtPa+FZFYWScgxaIXsWUlLM=; b=7XnCnQGnKYs+HjO5xilOl5h9ND2jnpEUF4s2S77iXX0E5u/r2LOIAs74mq3TyB23RQ 6n/+lX+yyUcvebPdQCQacSXRAquZ6SSn4H19/bzofifStXE5DPxL2ugK2jL6NxiB0asD VMuf8HeNFeoEME8K0b1tTMh7wPdWDfY12830YLpdS6o3tJeRAj62OzKyR+sr3ljFRe8V Ng6sRYobHHoVMCdVZUvMpcXwTshj9QvNelJ4HfRsnybaIJPjtrCLXBsq4s5SB8P15B70 5lzld8TFZ7Y16iVZQXYnWNbYujr34wwekjcafCI1bbRSyOl9v6wSDIv/Ornlhutzzo/6 TjhA== X-Gm-Message-State: AAQBX9cnqE39XtF8cg2O+J1kN3TWdH9zf/Uz5YXyl8giptFZZFESua63 /pSGqsBmGQrB2RYaDwRW09HAMyCkMLPoZAPL7CE= X-Google-Smtp-Source: AKy350bsPiDq70VpGF5giZ4OEl4HSHYv8y8CbagTWXFgUPS4Fk/C88VwfsR35Monb/9vE1j+xAiGFQ== X-Received: by 2002:a17:906:3611:b0:930:d30a:6c20 with SMTP id q17-20020a170906361100b00930d30a6c20mr3928509ejb.17.1679688501764; Fri, 24 Mar 2023 13:08:21 -0700 (PDT) Received: from linaro.org ([94.52.112.99]) by smtp.gmail.com with ESMTPSA id a20-20020a17090680d400b008def483cf79sm10594355ejx.168.2023.03.24.13.08.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Mar 2023 13:08:21 -0700 (PDT) Date: Fri, 24 Mar 2023 22:08:19 +0200 From: Abel Vesa To: Eric Biggers Cc: Ulf Hansson , Rob Herring , Krzysztof Kozlowski , Andy Gross , Bjorn Andersson , Konrad Dybcio , Manivannan Sadhasivam , Alim Akhtar , Avri Altman , Bart Van Assche , Adrian Hunter , "James E . J . Bottomley" , "Martin K . Petersen" , Herbert Xu , "David S . Miller" , linux-mmc@vger.kernel.org, devicetree@vger.kernel.org, Linux Kernel Mailing List , 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: 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: Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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 > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#include > > + > > +#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 > > 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