Re: [PATCH v2 1/7] clk: tegra: Add support for PLLSS

[Peter De Schrijver <pdeschrijver@nvidia.com>]

On Tue, Oct 15, 2013 at 10:20:03PM +0200, Stephen Warren wrote:
> On 10/15/2013 09:14 AM, Peter De Schrijver wrote:
> > Tegra124 introduces a new PLL type, PLLSS. Add support for it.
> 
> > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> 
> 
> > +static int clk_pllss_set_rate(struct clk_hw *hw, unsigned long rate,
> > +				unsigned long parent_rate)
> 
> This function seems pretty generic. Is it possible to share a bit more
> code with any of the other pllXXX_set_rate() functions?
> 
> > +struct clk *tegra_clk_register_pllss(const char *name, const char *parent_name,
> > +				void __iomem *clk_base, unsigned long flags,
> > +				struct tegra_clk_pll_params *pll_params,
> > +				spinlock_t *lock)
> 
> > +	val = pll_readl_base(pll);
> > +	if (val & PLLSS_REF_SRC_SEL_MASK) {
> > +		WARN(1, "Unknown parent selected for %s: %d\n", name,
> > +			(val & PLLSS_REF_SRC_SEL_MASK) >>
> > +			PLLSS_REF_SRC_SEL_SHIFT);
> > +		kfree(pll);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> 
> If there's a field in HW that muxes the clock input between n clocks,
> why does this function assume there's a single parent for this PLL, by
> taking a "const char *parent_name" parameter?
> 
> What happens if the bootloader changed this field in HW; is the kernel
> simply not able to boot?
> 

This logic comes from downstream. I guess it means we're running in an
unvalidated configuration. Do you think we should expose all parents 
anyway? Even if not all configurations have been validated?
(which is quite likely)

Cheers,

Peter.