Date:   Wed, 1 Sep 2021 09:57:36 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH v10 5/8] soc/tegra: pmc: Implement
 dev_get_performance_state() callback

01.09.2021 09:10, Viresh Kumar пишет:
> On 31-08-21, 16:54, Dmitry Osipenko wrote:
>> +static int
>> +tegra_pmc_pd_dev_get_performance_state(struct generic_pm_domain *genpd,
>> +				       struct device *dev)
>> +{
>> +	struct opp_table *hw_opp_table, *clk_opp_table;
>> +	struct dev_pm_opp *opp;
>> +	u32 hw_version;
>> +	int ret;
>> +
>> +	/*
>> +	 * The EMC devices are a special case because we have a protection
>> +	 * from non-EMC drivers getting clock handle before EMC driver is
>> +	 * fully initialized.  The goal of the protection is to prevent
>> +	 * devfreq driver from getting failures if it will try to change
>> +	 * EMC clock rate until clock is fully initialized.  The EMC drivers
>> +	 * will initialize the performance state by themselves.
>> +	 *
>> +	 * Display controller also is a special case because only controller
>> +	 * driver could get the clock rate based on configuration of internal
>> +	 * divider.
>> +	 *
>> +	 * Clock driver uses its own state syncing.
>> +	 */
>> +	if (of_device_compatible_match(dev->of_node, tegra_pd_no_perf_compats))
>> +		return 0;
>> +
>> +	if (of_machine_is_compatible("nvidia,tegra20"))
>> +		hw_version = BIT(tegra_sku_info.soc_process_id);
>> +	else
>> +		hw_version = BIT(tegra_sku_info.soc_speedo_id);
>> +
>> +	hw_opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
>> +	if (IS_ERR(hw_opp_table)) {
>> +		dev_err(dev, "failed to set OPP supported HW: %pe\n",
>> +			hw_opp_table);
>> +		return PTR_ERR(hw_opp_table);
>> +	}
>> +
>> +	/*
>> +	 * Older device-trees don't have OPPs, meanwhile drivers use
>> +	 * dev_pm_opp_set_rate() and it requires OPP table to be set
>> +	 * up using dev_pm_opp_set_clkname().
>> +	 *
>> +	 * The devm_tegra_core_dev_init_opp_table() is a common helper
>> +	 * that sets up OPP table for Tegra drivers and it sets the clk
>> +	 * for compatibility with older device-tress.  GR3D driver uses that
>> +	 * helper, it also uses devm_pm_opp_attach_genpd() to manually attach
>> +	 * power domains, which now holds the reference to OPP table that
>> +	 * already has clk set up implicitly by OPP core for a newly created
>> +	 * table using dev_pm_opp_of_add_table() invoked below.
>> +	 *
>> +	 * Hence we need to explicitly set/put the clk, otherwise
>> +	 * further dev_pm_opp_set_clkname() will fail with -EBUSY.
>> +	 */
>> +	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
>> +	if (IS_ERR(clk_opp_table)) {
>> +		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
>> +		ret = PTR_ERR(clk_opp_table);
>> +		goto put_hw;
>> +	}
>> +
>> +	ret = dev_pm_opp_of_add_table(dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to add OPP table: %d\n", ret);
>> +		goto put_clk;
>> +	}
>> +
>> +	opp = dev_pm_opp_get_current(dev);
>> +	if (IS_ERR(opp)) {
>> +		dev_err(dev, "failed to get current OPP: %pe\n", opp);
>> +		ret = PTR_ERR(opp);
>> +	} else {
>> +		ret = dev_pm_opp_get_required_pstate(opp, 0);
>> +		dev_pm_opp_put(opp);
>> +	}
>> +
>> +	dev_pm_opp_of_remove_table(dev);
>> +put_clk:
>> +	dev_pm_opp_put_clkname(clk_opp_table);
>> +put_hw:
>> +	dev_pm_opp_put_supported_hw(hw_opp_table);
> 
> So you create the OPP table and just after that you remove it ? Just
> to get the current OPP and pstate ? This doesn't look okay.
> 
> Moreover, this routine must be implemented with the expectation that
> the genpd core can call it anytime it wants, not just at the
> beginning. And so if the table is already setup by someone else then,
> then this all will just fail.

This is not doable using the current OPP API, it doesn't allow to re-use initialized OPP table. The current limitation is okay because genpd core doesn't call routine anytime.

> I did have a look at the comment you added above regarding
> devm_tegra_core_dev_init_opp_table(), but I am not sure of the series
> of events right now. For me, the OPP table should just be added once
> and for ever. You shouldn't remove and add it again. That is bound to
> break somewhere later.

I see that the comment about devm_tegra_core_dev_init_opp_table() is outdated now, will update it. There was a refcounting bug in v9 where I accidentally used devm_pm_opp_of_add_table instead of dev_, still it fails similarly, but in a different place now. 

> Can you give the sequence in which the whole thing works out with
> respect to the OPP core? who calls
> devm_tegra_core_dev_init_opp_table() and when, and when exactly will
> this routine get called, etc ?
> 

gr3d_probe(struct platform_device *pdev)
{
	gr3d_init_power(dev)
	{
		static const char * const opp_genpd_names[] = { "3d0", "3d1", NULL };

		devm_pm_opp_attach_genpd(dev, opp_genpd_names)
		{
			dev_pm_opp_attach_genpd(dev, names, virt_devs)
			{
				// takes and holds table reference
				opp_table = _add_opp_table(dev, false);

				while (*name) {
					// first attachment succeeds, 
					// second fails with "tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY"
					dev_pm_domain_attach_by_name(dev, *name)
					{
						tegra_pmc_pd_dev_get_performance_state(dev)
						{
							dev_pm_opp_set_clkname(dev, NULL);
							dev_pm_opp_of_add_table(dev);
							opp = dev_pm_opp_get_current(dev);
							dev_pm_opp_of_remove_table(dev);
							dev_pm_opp_put_clkname(opp_table);
							...
						}
						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
					}
				}
			}
		}
	}

	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);

	return 0;
}


WARNING: CPU: 3 PID: 7 at drivers/opp/core.c:2151 dev_pm_opp_set_clkname+0x97/0xb8
Modules linked in:
CPU: 3 PID: 7 Comm: kworker/u8:0 Tainted: G        W         5.14.0-next-20210831-00177-g6850c69f8c92-dirty #9371
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
[<c010cc91>] (unwind_backtrace) from [<c0108d35>] (show_stack+0x11/0x14)
[<c0108d35>] (show_stack) from [<c0a6e25d>] (dump_stack_lvl+0x2b/0x34)
[<c0a6e25d>] (dump_stack_lvl) from [<c011fc7f>] (__warn+0xbb/0x100)
[<c011fc7f>] (__warn) from [<c0a6b783>] (warn_slowpath_fmt+0x4b/0x80)
[<c0a6b783>] (warn_slowpath_fmt) from [<c0742613>] (dev_pm_opp_set_clkname+0x97/0xb8)
[<c0742613>] (dev_pm_opp_set_clkname) from [<c0509815>] (tegra_pmc_pd_dev_get_performance_state+0x85/0x120)
[<c0509815>] (tegra_pmc_pd_dev_get_performance_state) from [<c05cd3db>] (__genpd_dev_pm_attach+0xe7/0x218)
[<c05cd3db>] (__genpd_dev_pm_attach) from [<c05cd5d3>] (genpd_dev_pm_attach_by_id+0x8b/0xec)
[<c05cd5d3>] (genpd_dev_pm_attach_by_id) from [<c074287f>] (dev_pm_opp_attach_genpd+0x97/0x11c)
[<c074287f>] (dev_pm_opp_attach_genpd) from [<c0742913>] (devm_pm_opp_attach_genpd+0xf/0x6c)
[<c0742913>] (devm_pm_opp_attach_genpd) from [<c05ac7a5>] (gr3d_probe+0x245/0x348)
[<c05ac7a5>] (gr3d_probe) from [<c05bc003>] (platform_probe+0x43/0x84)
[<c05bc003>] (platform_probe) from [<c05ba569>] (really_probe.part.0+0x69/0x200)
[<c05ba569>] (really_probe.part.0) from [<c05ba773>] (__driver_probe_device+0x73/0xd4)
[<c05ba773>] (__driver_probe_device) from [<c05ba809>] (driver_probe_device+0x35/0xd0)
[<c05ba809>] (driver_probe_device) from [<c05bac11>] (__device_attach_driver+0x75/0x98)
[<c05bac11>] (__device_attach_driver) from [<c05b9005>] (bus_for_each_drv+0x51/0x7c)
[<c05b9005>] (bus_for_each_drv) from [<c05ba9f7>] (__device_attach+0x8b/0x104)
[<c05ba9f7>] (__device_attach) from [<c05b9b1b>] (bus_probe_device+0x5b/0x60)
[<c05b9b1b>] (bus_probe_device) from [<c05b7707>] (device_add+0x293/0x65c)
[<c05b7707>] (device_add) from [<c07798b7>] (of_platform_device_create_pdata+0x63/0x88)
[<c07798b7>] (of_platform_device_create_pdata) from [<c07799e5>] (of_platform_bus_create+0xfd/0x26c)
[<c07799e5>] (of_platform_bus_create) from [<c0779c2d>] (of_platform_populate+0x45/0x84)
[<c0779c2d>] (of_platform_populate) from [<c0779cc5>] (devm_of_platform_populate+0x41/0x6c)
[<c0779cc5>] (devm_of_platform_populate) from [<c054aa65>] (host1x_probe+0x1e9/0x2c8)
[<c054aa65>] (host1x_probe) from [<c05bc003>] (platform_probe+0x43/0x84)
[<c05bc003>] (platform_probe) from [<c05ba569>] (really_probe.part.0+0x69/0x200)
[<c05ba569>] (really_probe.part.0) from [<c05ba773>] (__driver_probe_device+0x73/0xd4)
[<c05ba773>] (__driver_probe_device) from [<c05ba809>] (driver_probe_device+0x35/0xd0)
[<c05ba809>] (driver_probe_device) from [<c05bac11>] (__device_attach_driver+0x75/0x98)
[<c05bac11>] (__device_attach_driver) from [<c05b9005>] (bus_for_each_drv+0x51/0x7c)
[<c05b9005>] (bus_for_each_drv) from [<c05ba9f7>] (__device_attach+0x8b/0x104)
[<c05ba9f7>] (__device_attach) from [<c05b9b1b>] (bus_probe_device+0x5b/0x60)
[<c05b9b1b>] (bus_probe_device) from [<c05b9dfb>] (deferred_probe_work_func+0x57/0x78)
[<c05b9dfb>] (deferred_probe_work_func) from [<c013701f>] (process_one_work+0x147/0x3f8)
[<c013701f>] (process_one_work) from [<c0137805>] (worker_thread+0x21d/0x3f4)
[<c0137805>] (worker_thread) from [<c013c1bb>] (kthread+0x123/0x140)
[<c013c1bb>] (kthread) from [<c0100135>] (ret_from_fork+0x11/0x1c)
Exception stack(0xc1567fb0 to 0xc1567ff8)
---[ end trace f68728a0d3053b54 ]---
tegra-gr3d 54180000.gr3d: failed to set OPP clk: -EBUSY
genpd genpd:1:54180000.gr3d: failed to get performance state of 54180000.gr3d for power-domain 3d1: -16
tegra-gr3d 54180000.gr3d: Couldn't attach to pm_domain: -16

Powered by blists - more mailing lists