On Mon, Dec 18, 2017 at 11:10:40AM -0800, Stephen Boyd wrote:
On 12/18, Subhransu S. Prusty wrote:
On Mon, Dec 18, 2017 at 09:27:16AM +0530, Subhransu S. Prusty wrote:
On Wed, Dec 13, 2017 at 02:30:32PM -0800, Stephen Boyd wrote:
On 12/11, Sriram Periyasamy wrote:
- if (!rate)
return -EINVAL;- if (__clk_is_enabled(hw->clk) && (clkdev->rate != rate))
Any chance you can directly read the hardware instead of going through the framework to find out if the clk is enabled? Seems
No. This involves sending an IPC to DSP to enable clock and interpreting the return error code. I would like to avoid doing this here in set_rate.
Ok. So we're checking to see if software has already enabled the clk and then checking to see if the rate the consumer is requesting is the same as the rate it previously requested? I'm
The second check is not required, will remove it.
still confused what's going on here. Does skl_fill_clk_ipc() change the rate of the clk? Is there any way to ask the DSP what
skl_fill_clk_ipc() prepares the IPC message based on the rate request by the consumer. This IPC will be sent to DSP during a call to clock enable.
the rate would be if we were to use some rate configuration?
No. If the clock is already running, reconfiguration is not allowed. So the above check is invalid.
circular to do it this way.
return -EBUSY;- rcfg = skl_get_rate_cfg(clkdev->pdata->ssp_clks[clkdev->id].rate_cfg,
rate);- if (!rcfg)
return -EINVAL;- clk_type = skl_get_clk_type(clkdev->id);
- if (clk_type < 0)
return clk_type;- skl_fill_clk_ipc(rcfg, clk_type);
- clkdev->rate = rate;
- return 0;
+}
+static unsigned long skl_clk_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)+{
- struct skl_clk *clkdev = to_skl_clk(hw);
- struct skl_clk_rate_cfg_table *rcfg;
- int clk_type;
- if (!clkdev)
return 0;- if (clkdev->rate)
return clkdev->rate;Why is the rate being cached? We should always be able to calculate the rate based on parent_rate that gets passed to this function?
Will check and get back.
If I understand correctly, you refer to deriving the rate from parent_rate using ratios. But since only the DSP is aware of the ratios and not the driver, the driver can't derive the rate from the parent_rate and thus cached.
I was thinking the code would do what's below all the time.
I think we interpreted incorrectly. As recalc_rate is meant to be used only when parent_rate changes, so this can be removed as the set_parent is not supported for this driver. Please let me know if I understand correctly.
Regards, Subhransu
- rcfg = skl_get_rate_cfg(clkdev->pdata->ssp_clks[clkdev->id].rate_cfg,
parent_rate);- if (!rcfg)
return 0;- clk_type = skl_get_clk_type(clkdev->id);
- if (clk_type < 0)
return 0;- skl_fill_clk_ipc(rcfg, clk_type);
- clkdev->rate = rcfg->rate;
- return clkdev->rate;
+}
I guess that means doing an IPC to the DSP to figure out the ratio and how that relates to the parent rate? recalc_rate() can be called many times with different things when the framework is speculating on the tree. We don't want clk providers to rely on the order of this op being called with respect to clk_set_rate().
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
--