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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131016074829.GD5643@tbergstrom-lnx.Nvidia.com>
Date:	Wed, 16 Oct 2013 10:48:29 +0300
From:	Peter De Schrijver <pdeschrijver@...dia.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Prashant Gaikwad <pgaikwad@...dia.com>,
	Mike Turquette <mturquette@...aro.org>,
	Thierry Reding <thierry.reding@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v2 1/7] clk: tegra: Add support for PLLSS

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/