Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

On 22-06-15, 16:43, Lee Jones wrote:

+config ARM_ST_CPUFREQ
+	bool "ST CPUFreq support"

Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?

+	depends on SOC_STIH407

st,opp-list ?

+	if (!opplist) {
+		dev_err(&pdev->dev, "opp-list node missing\n");
+		return -ENODATA;
+	}
+
+	ret = st_cpufreq_get_version(pdev, &minor, &major);
+	if (ret) {
+		dev_err(&pdev->dev, "No OPP match found for this platform\n");
+		return ret;
+	}
+
+	for_each_child_of_node(opplist, opp) {
+		if (ddata->dvfs_tab_count == STI_DVFS_TAB_MAX) {
+			dev_err(&pdev->dev, "Too many DVFS entries found\n");
+			ret = -EOVERFLOW;
+			break;
+		}
+
+		/* Cut version e.g. 2.0 [major.minor] */
+		err = st_cpufreq_check_if_matches(opp, "st,cuts",
+						  (minor << 8) | major);
+		if (err)
+			continue;
+
+		ret = st_cpufreq_check_if_matches(opp, "st,substrate",
+						  ddata->substrate);
+		if (err)
+			continue;
+
+		ret = of_property_read_u32(opp, "st,freq", &dvfs_tab->freq);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't read frequency: %d\n", ret);
+			goto out;
+		}
+		dvfs_tab->freq *= 1000;
+
+		ret = of_property_read_u32_index(opp, "st,avs",
+						 ddata->pcode,
+						 &dvfs_tab->avs);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't read AVS: %d\n", ret);
+			goto out;
+		}
+
+		dvfs_tab++;
+		ddata->dvfs_tab_count++;
+	}
+
+	sort(&ddata->dvfs_tab[0], ddata->dvfs_tab_count,
+	     sizeof(struct st_dvfs_tab), st_cpufreq_cmp, NULL);
+
+out:
+	of_node_put(opplist);
+
+	if (!ddata->dvfs_tab_count) {
+		dev_err(&pdev->dev, "No suitable AVS table found\n");

Why is this an error? I thought in this case you will go ahead with
the normal OPP-table.

+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int sti_cpufreq_voltage_scaling_init(struct platform_device *pdev)
+{
+	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+	struct st_dvfs_tab *dvfs_tab = &ddata->dvfs_tab[0];
+	struct device *cpu_dev;
+	struct dev_pm_opp *opp;
+	unsigned long highest_freq = 0;
+	int ret;
+	int i;
+
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev) {
+		dev_err(&pdev->dev, "Failed to get cpu0 device\n");
+		return -ENODEV;
+	}
+
+	/* Populate OPP table with default non-AVS frequency values */
+	of_init_opp_table(cpu_dev);
+
+	/*
+	 * Disable, but keep default values -- this prevents the framework from
+	 * erroneously re-adding and enabling entries with missing voltage rates
+	 */
+	while (1) {
+		highest_freq++;
+
+		opp = dev_pm_opp_find_freq_ceil(cpu_dev, &highest_freq);
+		if (IS_ERR(opp))
+			break;
+
+		ret = dev_pm_opp_disable(cpu_dev, highest_freq);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to disable freq: %li\n",
+				highest_freq);
+			return ret;
+		}
+	}
+
+	for (i = 0; i < ddata->dvfs_tab_count; i++, dvfs_tab++) {
+		unsigned int f = dvfs_tab->freq * 1000;
+		unsigned int v = dvfs_tab->avs * 1000;
+
+		opp = dev_pm_opp_find_freq_exact(cpu_dev, f, false);
+
+		/* Remove existing voltage-less OPP entry */
+		if (!IS_ERR(opp))
+			dev_pm_opp_remove(cpu_dev, f);
+
+		/* Supply new fully populated OPP entry */
+		ret = dev_pm_opp_add(cpu_dev, f, v);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to add OPP %u\n", f);
+			return ret;
+		}
+	}

So you have added new OPPs here, but cpufreq-dt will try to add old
OPPs. You must be getting lots of warnings ?

+
+	return 0;
+}
+
+static int st_cpufreq_fetch_regmap_field(struct platform_device *pdev,
+					 const struct reg_field *reg_fields,
+					 int pcode_offset, int field)
+{
+	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+	struct regmap_field *regmap_field;
+	struct reg_field reg_field = reg_fields[field];
+	unsigned int value;
+	int ret;
+
+	reg_field.reg = pcode_offset;
+	regmap_field = devm_regmap_field_alloc(&pdev->dev,
+					       ddata->regmap_eng,
+					       reg_field);
+	if (IS_ERR(regmap_field)) {
+		dev_err(&pdev->dev, "Failed to allocate reg field\n");
+		return PTR_ERR(regmap_field);
+	}
+
+	ret = regmap_field_read(regmap_field, &value);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to read %s code\n",
+			field ? "SUBSTRATE" : "PCODE");
+		return ret;
+	}
+
+	return value;
+}
+
+static const struct reg_field sti_stih407_dvfs_regfields[DVFS_MAX_REGFIELDS] = {
+	[PCODE]		= REG_FIELD(0, 16, 19),
+	[SUBSTRATE]	= REG_FIELD(0, 0, 2),
+};
+
+static struct of_device_id sti_cpufreq_of_match[] = {
+	{
+		.compatible = "st,stih407-cpufreq",
+		.data = &sti_stih407_dvfs_regfields,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sti_cpufreq_of_match);
+
+/* Find process code -- calibrated per-SoC */
+static void sti_cpufreq_get_pcode(struct platform_device *pdev)
+{
+	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	const struct reg_field *reg_fields;
+	const struct of_device_id *match;
+	int pcode_offset;
+	int ret;
+
+	ddata->regmap_eng = syscon_regmap_lookup_by_phandle(np, "st,syscfg-eng");
+	if (IS_ERR(ddata->regmap_eng)) {
+		dev_warn(&pdev->dev, "\"st,syscfg-eng\" not supplied\n");
+		goto set_default;
+	}
+
+	ret = of_property_read_u32_index(np, "st,syscfg-eng",
+					 PCODE_INDEX, &pcode_offset);
+	if (ret) {
+		dev_warn(&pdev->dev, "Process code offset is required\n");
+		goto set_default;
+	}
+
+	match = of_match_node(sti_cpufreq_of_match, np);

Are you planning to add more entries to this table? We are here as
probe() is called only after matching for this string.

+	if (!match) {
+		dev_warn(&pdev->dev, "Failed to match device\n");
+		goto set_default;
+	}
+	reg_fields = match->data;
+
+	ddata->pcode = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
+						     pcode_offset,
+						     PCODE);
+	if (ddata->pcode < 0)
+		goto set_default;
+
+	ddata->substrate = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
+							 pcode_offset,
+							 SUBSTRATE);
+	if (ddata->substrate < 0)
+		goto set_default;

Maybe:

	if (ddata->substrate >= 0)
                return;

+
+	return;
+
+set_default:
+	dev_warn(&pdev->dev,
+		 "Setting pcode to highest tolerance/voltage for safety\n");
+	ddata->pcode = 0;
+	ddata->substrate = 0;
+}
+
+static int sti_cpufreq_probe(struct platform_device *pdev)
+{
+	struct st_cpufreq_ddata *ddata;
+	int ret;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ddata);
+
+	sti_cpufreq_get_pcode(pdev);
+
+	ret = st_cpufreq_get_dvfs_info(pdev);
+	if (ret)
+		dev_warn(&pdev->dev, "Not doing voltage scaling [%d]\n", ret);
+	else
+		sti_cpufreq_voltage_scaling_init(pdev);
+
+	platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+
+	return 0;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in