[PATCH v8 09/26] memory: tegra30: Support interconnect framework

[Viresh Kumar]

On 11-11-20, 04:14, Dmitry Osipenko wrote:
> +static int tegra_emc_opp_table_init(struct tegra_emc *emc)
> +{
> +	struct opp_table *reg_opp_table = NULL, *clk_opp_table, *hw_opp_table;
> +	u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> +	const char *rname = "core";
> +	int err;
> +
> +	/*
> +	 * Legacy device-trees don't have OPP table and EMC driver isn't
> +	 * useful in this case.
> +	 */
> +	if (!device_property_present(emc->dev, "operating-points-v2")) {

I don't understand why you want to check this ? The below call to
dev_pm_opp_of_add_table() will fail anyway and that should be good for
you.

> +		dev_err(emc->dev,
> +			"OPP table not found, please update your device tree\n");
> +		return -ENODEV;
> +	}
> +
> +	/* voltage scaling is optional */
> +	if (device_property_present(emc->dev, "core-supply")) {
> +		reg_opp_table = dev_pm_opp_set_regulators(emc->dev, &rname, 1);
> +		if (IS_ERR(reg_opp_table))
> +			return dev_err_probe(emc->dev, PTR_ERR(reg_opp_table),
> +					     "failed to set OPP regulator\n");
> +	}
> +
> +	clk_opp_table = dev_pm_opp_set_clkname(emc->dev, NULL);

I think there is still some misunderstanding here. This call with a
NULL connection id is useful only if the DT OPP table is optional for
your platform and you want the same driver to work with and without
the DT OPP table. Clearly in your case you want the OPP table in the
DT to be there and so this call is not required at all.

> +	err = PTR_ERR_OR_ZERO(clk_opp_table);
> +	if (err) {
> +		dev_err(emc->dev, "failed to set OPP clk: %d\n", err);
> +		goto put_reg_table;
> +	}
> +
> +	hw_opp_table = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
> +	err = PTR_ERR_OR_ZERO(hw_opp_table);
> +	if (err) {
> +		dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
> +		goto put_clk_table;
> +	}
> +
> +	err = dev_pm_opp_of_add_table(emc->dev);
> +	if (err) {
> +		dev_err(emc->dev, "failed to add OPP table: %d\n", err);
> +		goto put_hw_table;
> +	}
> +
> +	dev_info(emc->dev, "OPP HW ver. 0x%x, current clock rate %lu MHz\n",
> +		 hw_version, clk_get_rate(emc->clk) / 1000000);
> +
> +	/* first dummy rate-set initializes voltage state */
> +	err = dev_pm_opp_set_rate(emc->dev, clk_get_rate(emc->clk));
> +	if (err) {
> +		dev_err(emc->dev, "failed to initialize OPP clock: %d\n", err);
> +		goto remove_table;
> +	}
> +
> +	return 0;
> +
> +remove_table:
> +	dev_pm_opp_of_remove_table(emc->dev);
> +put_hw_table:
> +	dev_pm_opp_put_supported_hw(hw_opp_table);
> +put_clk_table:
> +	dev_pm_opp_put_clkname(clk_opp_table);
> +put_reg_table:
> +	if (reg_opp_table)
> +		dev_pm_opp_put_regulators(reg_opp_table);
> +
> +	return err;
> +}

-- 
viresh