Re: [PATCH v5 3/4] cpufreq: qcom-nvmem: add support for IPQ8064

[Christian Marangi]

On Tue, Oct 10, 2023 at 03:39:54PM +0200, Konrad Dybcio wrote:
> 
> 
> On 9/30/23 12:21, Robert Marko wrote:
> > From: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > 
> > IPQ8064 comes in 3 families:
> > * IPQ8062 up to 1.0GHz
> > * IPQ8064/IPQ8066/IPQ8068 up to 1.4GHz
> > * IPQ8065/IPQ8069 up to 1.7Ghz
> > 
> > So, in order to be able to support one OPP table, add support for
> > IPQ8064 family based of SMEM SoC ID-s and correctly set the version so
> > opp-supported-hw can be correctly used.
> > 
> > Bit are set with the following logic:
> > * IPQ8062 BIT 0
> > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > * IPQ8065/IPQ8069 BIT 2
> > 
> > speed is never fused, only pvs values are fused.
> > 
> > IPQ806x SoC doesn't have pvs_version so we drop and we use the new
> > pattern:
> > opp-microvolt-speed0-pvs<PSV_VALUE>
> > 
> > Example:
> > - for ipq8062 psv2
> >    opp-microvolt-speed0-pvs2 = < 925000 878750 971250>
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > Signed-off-by: Robert Marko <robimarko@xxxxxxxxx>
> > ---
> [...]
> 
> > +{
> > +	int speed = 0, pvs = 0, pvs_ver = 0;
> > +	int msm_id, ret = 0;
> > +	u8 *speedbin;
> > +	size_t len;
> > +
> > +	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> > +
> > +	if (IS_ERR(speedbin))
> The stray newline above this line triggers my OCD :D
> 
> > +		return PTR_ERR(speedbin);
> > +
> > +	if (len != 4) {
> > +		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > +		kfree(speedbin);
> > +		return -ENODEV;
> > +	}
> > +
> > +	get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver, speedbin);
> > +
> > +	ret = qcom_smem_get_soc_id(&msm_id);
> > +	if (ret)
> > +		return ret;
> speedbin leaks here
> 
> you can free it right after the get_krait.. call
> > +
> > +	switch (msm_id) {
> > +	case QCOM_ID_IPQ8062:
> > +		drv->versions = BIT(IPQ8062_VERSION);
> > +		break;
> > +	case QCOM_ID_IPQ8064:
> > +	case QCOM_ID_IPQ8066:
> > +	case QCOM_ID_IPQ8068:
> > +		drv->versions = BIT(IPQ8064_VERSION);
> > +		break;
> > +	case QCOM_ID_IPQ8065:
> > +	case QCOM_ID_IPQ8069:
> > +		drv->versions = BIT(IPQ8065_VERSION);
> > +		break;
> > +	default:
> > +		dev_err(cpu_dev,
> > +			"SoC ID %u is not part of IPQ8064 family, limiting to 1.0GHz!\n",
> > +			msm_id);
> > +		drv->versions = BIT(IPQ8062_VERSION);
> > +		break;
> > +	}
> > +
> > +	/* IPQ8064 speed is never fused. Only pvs values are fused. */
> > +	snprintf(*pvs_name, sizeof("speedXX-pvsXX"), "speed%d-pvs%d",
> > +		 speed, pvs);
> Then drop the format for `speed` and just throw in a zero!
> 
> [...]
> 
> > -	{ .compatible = "qcom,ipq8064", .data = &match_data_krait },
> > +	{ .compatible = "qcom,ipq8064", .data = &match_data_ipq8064 },
> This change demands a Fixes tag, because you're essentially saying "the
> support for this SoC was supposedly there, but it could have never worked
> and was broken all along".
>

Mhhh actually no. We are just changing the opp binding and introducing
hardcoded versions. But the thing worked and actually it's what was used
before this change in openwrt. Also current ipq806x dtsi doesn't have
any opp definition so no regression there. (and also 99% downstream either
use openwrt or use qcom sdk where this implementation is not used at
all)

Given these thing should we still add a fixes tag referencing the commit
that introduced the compatible for qcom,ipq8064? It's quite problematic
as this depends on qcom_smem_get_soc_id().

-- 
	Ansuel