| Message ID | 20230327134734.3256974-1-abel.vesa@linaro.org |
|---|---|
| Headers | show |
| Series | Add dedicated Qcom ICE driver | expand |
On 27/03/2023 15:47, Abel Vesa wrote: > Starting with SM8550, the ICE will have its own devicetree node > so add the qcom,ice property to reference it. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > > The v3 (RFC) is here: > https://lore.kernel.org/all/20230313115202.3960700-4-abel.vesa@linaro.org/ > > Changes since v3: > * dropped the "and drop core clock" part from subject line > > Changes since v2: > * dropped all changes except the qcom,ice property > > Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > index c5a06c048389..7384300c421d 100644 > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml > @@ -70,6 +70,10 @@ properties: > power-domains: > maxItems: 1 > > + qcom,ice: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to the Inline Crypto Engine node Didn't we discuss to disallow the ICE IO space if this is provided? Same for previous patch actually... Best regards, Krzysztof
On Mon, Mar 27, 2023 at 04:47:29PM +0300, Abel Vesa wrote: > Starting with SM8550, the ICE will have its own devicetree node > so add the qcom,ice property to reference it. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > > The v3 (RFC) is here: > https://lore.kernel.org/all/20230313115202.3960700-3-abel.vesa@linaro.org/ > > Changes since v3: > * dropped the "and drop core clock" part from subject line > > Changes since v2: > * dropped all changes except the qcom,ice property > > Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > index 64df6919abaf..0ad14d5b722e 100644 > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml > @@ -120,6 +120,10 @@ properties: > $ref: /schemas/types.yaml#/definitions/uint32 > description: platform specific settings for DLL_CONFIG reg. > > + qcom,ice: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to the Inline Crypto Engine node > + It would be helpful if the description was more detailed and explained that this is a replacement for the directly specified reg and clock. Otherwise, looks good to me. Reviewed-by: Eric Biggers <ebiggers@google.com> - Eric
Hi Abel, On Mon, Mar 27, 2023 at 04:47:32PM +0300, Abel Vesa wrote: > Now that there is a new dedicated ICE driver, drop the ufs-qcom-ice and > use the new ICE api provided by the Qualcomm soc driver ice. The platforms > that already have ICE support will use the API as library since there will > not be a devicetree node, but instead they have reg range. In this case, > the of_qcom_ice_get will return an ICE instance created for the consumer's > device. But if there are platforms that do not have ice reg in the > consumer devicetree node and instead provide a dedicated ICE devicetree > node, the of_qcom_ice_get will look up the device based on qcom,ice > property and will get the ICE instance registered by the probe function > of the ice driver. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> I am still worried about the ICE clock. Are you sure it is being managed correctly? With your patch, the ICE clock gets enabled in ufs_qcom_ice_resume and disabled in ufs_qcom_ice_suspend, which hopefully pair up. But it also gets enabled in ufs_qcom_ice_enable which isn't paired with anything. Also, this all happens at a different time from the existing UFS clocks being enabled/disabled. I wonder if the ICE clock should be enabled/disabled in ufs_qcom_setup_clocks() instead of what you are doing currently? > +static int ufs_qcom_ice_init(struct ufs_qcom_host *host) > +{ > + struct ufs_hba *hba = host->hba; > + struct device *dev = hba->dev; > + > + host->ice = of_qcom_ice_get(dev); > + if (host->ice == ERR_PTR(-EOPNOTSUPP)) { > + dev_warn(dev, "Disabling inline encryption support\n"); > + hba->caps &= ~UFSHCD_CAP_CRYPTO; > + host->ice = NULL; > + } > + > + if (IS_ERR(host->ice)) > + return PTR_ERR(host->ice); > + > + return 0; > +} This is still sometimes leaving UFSHCD_CAP_CRYPTO set in cases where ICE is unsupported. Moving the *setting* of UFSHCD_CAP_CRYPTO into here would fix that. It is also hard to understand how the -EOPNOTSUPP case differs from the NULL case. Can you add a comment? Or just consider keeping the original behavior, which did not distinguish between these cases (as long as MASK_CRYPTO_SUPPORT was set in REG_CONTROLLER_CAPABILITIES, which was checked first). - Eric
On 23-03-27 11:19:34, Eric Biggers wrote: > Hi Abel, > > On Mon, Mar 27, 2023 at 04:47:32PM +0300, Abel Vesa wrote: > > Now that there is a new dedicated ICE driver, drop the ufs-qcom-ice and > > use the new ICE api provided by the Qualcomm soc driver ice. The platforms > > that already have ICE support will use the API as library since there will > > not be a devicetree node, but instead they have reg range. In this case, > > the of_qcom_ice_get will return an ICE instance created for the consumer's > > device. But if there are platforms that do not have ice reg in the > > consumer devicetree node and instead provide a dedicated ICE devicetree > > node, the of_qcom_ice_get will look up the device based on qcom,ice > > property and will get the ICE instance registered by the probe function > > of the ice driver. > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > I am still worried about the ICE clock. Are you sure it is being managed > correctly? With your patch, the ICE clock gets enabled in ufs_qcom_ice_resume > and disabled in ufs_qcom_ice_suspend, which hopefully pair up. But it also gets > enabled in ufs_qcom_ice_enable which isn't paired with anything. Also, this all > happens at a different time from the existing UFS clocks being enabled/disabled. Right, I messed this up since the last version. Sorry about that. What I need to do is to drop the enabling of the clock from qcom_ice_enable and only do it from qcom_ice_resume. As for disabling it, it remains as is, that is, in qcom_ice_disable. Then, I need to enable the clock right before checking the supported version. I'll do that with devm_clk_get_enabled (also optional for the legacy once as I explained in the reply to the 6th patch). > > I wonder if the ICE clock should be enabled/disabled in ufs_qcom_setup_clocks() > instead of what you are doing currently? > > > +static int ufs_qcom_ice_init(struct ufs_qcom_host *host) > > +{ > > + struct ufs_hba *hba = host->hba; > > + struct device *dev = hba->dev; > > + > > + host->ice = of_qcom_ice_get(dev); > > + if (host->ice == ERR_PTR(-EOPNOTSUPP)) { > > + dev_warn(dev, "Disabling inline encryption support\n"); > > + hba->caps &= ~UFSHCD_CAP_CRYPTO; > > + host->ice = NULL; > > + } > > + > > + if (IS_ERR(host->ice)) > > + return PTR_ERR(host->ice); > > + > > + return 0; > > +} > > This is still sometimes leaving UFSHCD_CAP_CRYPTO set in cases where ICE is > unsupported. > > Moving the *setting* of UFSHCD_CAP_CRYPTO into here would fix that. > I'll do exactly that. Thanks. > It is also hard to understand how the -EOPNOTSUPP case differs from the NULL > case. Can you add a comment? Or just consider keeping the original behavior, > which did not distinguish between these cases (as long as MASK_CRYPTO_SUPPORT > was set in REG_CONTROLLER_CAPABILITIES, which was checked first). I believe it makes more sense to return -EOPNOTSUPP when the driver doesn't support a specific version of the HW. If you do not agree, I'll make it return NULL then. > > - Eric