Re: [PATCH v10 5/8] soc/tegra: pmc: Implement dev_get_performance_state() callback

[Dmitry Osipenko]

01.09.2021 10:16, Viresh Kumar пишет:
> On 01-09-21, 09:57, Dmitry Osipenko wrote:
>> 01.09.2021 09:10, Viresh Kumar пишет:
>>> 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.
> 
> That isn't correct, you can call dev_pm_opp_of_add_table() as many
> times as you want. It will just increase the refcount and return the
> next time.
> 
> Yes, you can call the APIs like set-clk-name or supported-hw, since
> they are supposed to be set only once.
> 
>> The current limitation is okay because genpd core doesn't call
>> routine anytime.
> 
> Yeah, but is broken by design. People can make changes to genpd core
> later on to call it as many times and they aren't required to have a
> look at all the users to see who abused the API and who didn't.
> 
>>> 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)
> 
> Thanks for this.
> 
>> {
>> 	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);
> 
> What you end up doing here is pretty much like
> devm_tegra_core_dev_init_opp_table_simple(), right ?

Yes

>> 							opp = dev_pm_opp_get_current(dev);
>> 							dev_pm_opp_of_remove_table(dev);
>> 							dev_pm_opp_put_clkname(opp_table);
> 
> You shouldn't be required to do this at all.
> 
>> 							...
>> 						}
>> 						// opp_table->clk = ERR_PTR(-EINVAL) after the first attachment
>> 					}
>> 				}
>> 			}
>> 		}
>> 	}
>>
>> 	devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
> 
> Can we make the call at the beginning ? before calling
> gr3d_init_power() ? I mean you should only be required to initialize
> the OPP table just once.

This breaks dev_pm_opp_set_supported_hw() which isn't allowed to be
called for a populated OPP table, set/put_hw also isn't refcounted.

> If not, then what about calling
> devm_tegra_core_dev_init_opp_table_simple() from here and from
> tegra_pmc_pd_dev_get_performance_state() as well ?
> 
> And update devm_tegra_core_dev_init_opp_table_simple() to call
> dev_pm_opp_get_opp_table() first and return early if the OPP table is
> already initialized ?
> 

This doesn't work for devm_pm_opp_register_set_opp_helper() that is used
by gr3d_probe() because set_opp_helper() should be invoked only before
table is populated and it's already populated for the case of a
single-domain h/w since domain is attached before driver is probed.

But it's a good idea to use dev_pm_opp_get_opp_table(). I got it to work
by adding dev_pm_opp_get_opp_table() to
tegra_pmc_pd_dev_get_performance_state() and moving
devm_tegra_core_dev_init_opp_table_simple() before gr3d_init_power().

Thank you for the suggestion, I'll change it in v11.