Re: [Patch v2 7/9] cpufreq: tegra194: add OPP support and set bandwidth

From: Thierry Reding <treding@nvidia.com>
To: Sumit Gupta <sumitg@nvidia.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	krzysztof.kozlowski@linaro.org, dmitry.osipenko@collabora.com,
	rafael@kernel.org, jonathanh@nvidia.com, robh+dt@kernel.org,
	lpieralisi@kernel.org, linux-kernel@vger.kernel.org,
	linux-tegra@vger.kernel.org, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-pci@vger.kernel.org,
	mmaddireddy@nvidia.com, kw@linux.com, bhelgaas@google.com,
	vidyas@nvidia.com, sanjayc@nvidia.com, ksitaraman@nvidia.com,
	ishah@nvidia.com, bbasu@nvidia.com
Subject: Re: [Patch v2 7/9] cpufreq: tegra194: add OPP support and set bandwidth
Date: Mon, 27 Feb 2023 13:44:21 +0100	[thread overview]
Message-ID: <Y/ylpaJLMOE6zY8C@orome> (raw)
In-Reply-To: <df054fae-5195-1ef8-c72e-e5afe8d901d3@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

On Thu, Feb 23, 2023 at 03:06:26PM +0530, Sumit Gupta wrote:
> 
> 
> On 22/02/23 09:33, Viresh Kumar wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On 20-02-23, 19:35, Sumit Gupta wrote:
> > > +static int tegra_cpufreq_set_bw(struct cpufreq_policy *policy, unsigned long freq_khz)
> > > +{
> > > +     struct dev_pm_opp *opp;
> > > +     struct device *dev;
> > > +     int ret;
> > > +
> > > +     dev = get_cpu_device(policy->cpu);
> > > +     if (!dev)
> > > +             return -ENODEV;
> > > +
> > > +     opp = dev_pm_opp_find_freq_exact(dev, freq_khz * KHZ, true);
> > > +     if (IS_ERR(opp))
> > > +             return PTR_ERR(opp);
> > > +
> > > +     ret = dev_pm_opp_set_opp(dev, opp);
> > > +     dev_pm_opp_put(opp);
> > 
> > What about dev_pm_opp_set_rate() instead ?
> > 
> > > +     return ret;
> > > +}
> > 
> > --
> > viresh
> 
> Tried using it and got below crash. It seems to be coming because we don't
> have clocks property within CPU node for SoC's having BPMP-FW.
> 
>  Unable to handle kernel NULL pointer dereference at virtual address
> 000000000000002e
>  ....
>  Call trace:
>   clk_round_rate+0x38/0xd8
>   dev_pm_opp_set_rate+0xe4/0x1a8
>   tegra194_cpufreq_set_target+0x74/0x88
>   __cpufreq_driver_target+0x154/0x250
>   cpufreq_online+0x7b4/0x9ac

Can you try to find out what exactly is happening here? The clock
framework should be able to deal with NULL clock pointers just fine.
Although, looking at the OPP table code, it seems like we don't use
clk_get_optional(), so opp_table->clk may end up being a pointer-
encoded error. Perhaps we need something like this:

--- >8 ---

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e87567dbe99f..d7baeb6ac697 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1397,6 +1397,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 		 * frequency in opp->rates and also parse the entries in DT.
 		 */
 		opp_table->clk_count = 1;
+		opp_table->clk = NULL;
 
 		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
 		return opp_table;
--- >8 ---

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
