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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Feb 2023 23:42:50 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>
Cc:     Abel Vesa <abel.vesa@...aro.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "James E . J . Bottomley" <jejb@...ux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-mmc@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [RFC PATCH 1/5] soc: qcom: Make the Qualcomm UFS/SDCC ICE a
 dedicated driver

On Tue, Feb 14, 2023 at 02:34:47PM +0100, Konrad Dybcio wrote:
> > +#define QCOM_ICE_BIST_STATUS_MASK		0xF0000000
> GENMASK(31, 28)?

I personally think the plain number is much easier to read...

> btw, most of these defines seem unused?

Yes, the unused definitions can be dropped if people prefer.  I only included
them in the original version because this hardware has no public documentation,
so maybe it's helpful to see what registers and fields are available.

I suppose that downstream code could always be dug up if needed, though.  Or
maybe someday there will actually be documentation?

> > +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?

Similarly, plain bit operations are much more universally understood...

> > +	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);

I'm not sure either!  I've never had access to any documentation for this
hardware, so the above logic is just taken from downstream code.  I kept that
comment because it was the only available explanation for the value OR'd.

Since it doesn't seem to be useful, I'm fine with just removing it.  (But please
keep the "Enable low power mode sequence" part, as that's useful.)

My guess is that it is actually just describing the bits backwards, so [3]-E
corresponds to the three bits that are set.

> > +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.

I don't know what the 0xD807100 value means, sorry :-(  This is just the value
that works to enable the "optimizations", and which the downstream code was
using.  If anyone has access to the ICE hardware documentation (if there even
*is* documentation), they might be able to say.

- Eric