linux-kernel - Re: [PATCH v3 01/11] clk: davinci

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: <50912BCE.5000401@ti.com>
Date:	Wed, 31 Oct 2012 09:46:54 -0400
From:	Murali Karicheri <m-karicheri2@...com>
To:	Sekhar Nori <nsekhar@...com>
CC:	<mturquette@...aro.org>, <arnd@...db.de>,
	<akpm@...ux-foundation.org>, <shawn.guo@...aro.org>,
	<rob.herring@...xeda.com>, <linus.walleij@...aro.org>,
	<viresh.linux@...il.com>, <linux-kernel@...r.kernel.org>,
	<khilman@...com>, <linux@....linux.org.uk>, <sshtylyov@...sta.com>,
	<davinci-linux-open-source@...ux.davincidsp.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-keystone@...t.ti.com>
Subject: Re: [PATCH v3 01/11] clk: davinci - add main PLL clock driver

On 10/31/2012 08:29 AM, Sekhar Nori wrote:
> Hi Murali,
>
> On 10/25/2012 9:41 PM, Murali Karicheri wrote:
>> This is the driver for the main PLL clock hardware found on DM SoCs.
>> This driver borrowed code from arch/arm/mach-davinci/clock.c and
>> implemented the driver as per common clock provider API. The main PLL
>> hardware typically has a multiplier, a pre-divider and a post-divider.
>> Some of the SoCs has the divider fixed meaning they can not be
>> configured through a register. HAS_PREDIV and HAS_POSTDIV flags are used
>> to tell the driver if a hardware has these dividers present or not.
>> Driver is configured through the struct clk_pll_data that has the
>> SoC specific clock data.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@...com>
>> ---
>> diff --git a/drivers/clk/davinci/clk-pll.c b/drivers/clk/davinci/clk-pll.c
>> +static unsigned long clk_pllclk_recalc(struct clk_hw *hw,
>> +					unsigned long parent_rate)
>> +{
>> +	struct clk_pll *pll = to_clk_pll(hw);
>> +	struct clk_pll_data *pll_data = pll->pll_data;
>> +	u32 mult = 1, prediv = 1, postdiv = 1;
> No need to initialize mult here. I gave this comment last time around as
> well.
Yes. I missed it. I will fix it in the next revision.
>> +	unsigned long rate = parent_rate;
>> +
>> +	mult = readl(pll_data->reg_pllm);
>> +
>> +	/*
>> +	 * if fixed_multiplier is non zero, multiply pllm value by this
>> +	 * value.
>> +	 */
>> +	if (pll_data->fixed_multiplier)
>> +		mult =  pll_data->fixed_multiplier *
>> +				(mult & pll_data->pllm_mask);
>> +	else
>> +		mult = (mult & pll_data->pllm_mask) + 1;
> Hmm, this is interpreting the 'mult' register field differently in both
> cases. In one case it is 'actual multiplier - 1' and in other case it is
> the 'actual multiplier' itself. Can we be sure that the mult register
> definition will change whenever there is a fixed multiplier in the PLL
> block? I don't think any of the existing DaVinci devices have a fixed
> multiplier. Is this on keystone?
Read section 6.4.3 (PLL Mode) in DM365 documentation (SPRUFG5a.pdf) that 
states PLL multiplies the clock by 2x the value in the PLLM. In the old 
code this is handled by a if cpu_is_* macro that we can't use in the 
driver. So this is represented by a fixed_multiplier that can be set to 
2 for DM365 and zero on other SoCs.
>> +struct clk *clk_register_davinci_pll(struct device *dev, const char *name,
>> +			const char *parent_name,
>> +			struct clk_pll_data *pll_data)
>> +{
>> +	struct clk_init_data init;
>> +	struct clk_pll *pll;
>> +	struct clk *clk;
>> +
>> +	if (!pll_data)
>> +		return ERR_PTR(-ENODEV);
> -EINVAL? Clearly you are treating NULL value as an invalid argument here.
Ok. Will fix to ERR_PTR(-EINVAL).
>
> Thanks,
> Sekhar
>

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