[alsa-devel] [PATCH v5 1/6] ASoC: Intel: Skylake: Add ssp clock driver

[Stephen Boyd]

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
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
the rate would be if we were to use some rate configuration?

> > > 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.

> > 
> > > 
> > > > +
> > > > +	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