linux-kernel - Re: [PATCH v5 10/44] clk: davinci: New driver for davinci PSC clocks

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: <83f3d207-9645-cbdf-d6cf-b6e6a8458abe@lechnology.com>
Date:   Tue, 16 Jan 2018 10:51:45 -0600
From:   David Lechner <david@...hnology.com>
To:     Sekhar Nori <nsekhar@...com>, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Kevin Hilman <khilman@...nel.org>,
        Adam Ford <aford173@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 10/44] clk: davinci: New driver for davinci PSC clocks

On 01/16/2018 05:03 AM, Sekhar Nori wrote:
> On Monday 08 January 2018 07:47 AM, David Lechner wrote:
>> This adds a new driver for mach-davinci PSC clocks. This is porting the
>> code from arch/arm/mach-davinci/psc.c to the common clock framework and
>> is converting it to use regmap to simplify the code. Additionally, it adds
>> device tree support for these clocks.
>>
>> Note: although there are similar clocks for TI Keystone we are not able
>> to share the code for a few reasons. The keystone clocks are device tree
>> only and use legacy one-node-per-clock bindings. Also the keystone driver
>> makes the assumption that there is only one PSC per SoC and uses global
>> variables, but here we have two controllers per SoC.
>>
>> Signed-off-by: David Lechner <david@...hnology.com>
>> ---

>> +static void psc_config(struct davinci_psc_clk *psc,
>> +		       enum davinci_psc_state next_state)
>> +{
>> +	u32 epcpr, pdstat, mdstat, mdctl, ptstat;
>> +
>> +	mdctl = next_state;
>> +	if (psc->flags & LPSC_FORCE)
>> +		mdctl |= MDCTL_FORCE;
>> +	regmap_write_bits(psc->regmap, MDCTL(psc->lpsc), MDSTAT_STATE_MASK,
>> +			  mdctl);
> 
> Wont this ignore the MDCTL_FORCE bit since MDSTAT_STATE_MASK does not
> cover that?
> 
>> +
>> +	regmap_read(psc->regmap, PDSTAT(psc->pd), &pdstat);
>> +	if ((pdstat & PDSTAT_STATE_MASK) == 0) {
>> +		regmap_write_bits(psc->regmap, PDSTAT(psc->pd),
>> +				  PDSTAT_STATE_MASK, PDCTL_NEXT);
> 
> Shouldn't this be a write to PDCTL register?
> 

Looks like I have some mistakes here. Thank you.

...

>> +static struct clk *davinci_psc_clk_register(const char *name,
>> +					    const char *parent_name,
>> +					    struct regmap *regmap,
>> +					    u32 lpsc, u32 pd, u32 flags)
>> +{
>> +	struct clk_init_data init;
>> +	struct davinci_psc_clk *psc;
>> +	struct clk *clk;
>> +
>> +	psc = kzalloc(sizeof(*psc), GFP_KERNEL);
>> +	if (!psc)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	init.name = name;
>> +	init.ops = &davinci_psc_clk_ops;
>> +	init.parent_names = (parent_name ? &parent_name : NULL);
>> +	init.num_parents = (parent_name ? 1 : 0);
>> +	init.flags = CLK_SET_RATE_PARENT;
> 
> Is this needed since PSC does not cause any rate change?

Yes, because one of the PSCs is the ARM clock and for cpufreq, we
need to propagate the rate change up the chain to SYSCLK6.