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

[Eric Biggers]

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