linux-kernel - Re: [PATCH 2/4] clk: Implement clk_set

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>] [day] [month] [year] [list]

Date:	Thu, 26 May 2011 08:54:29 +0200
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Jeremy Kerr <jeremy.kerr@...onical.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-sh@...r.kernel.org
Subject: Re: [PATCH 2/4] clk: Implement clk_set_rate

[put the lists back on cc]

On Thu, May 26, 2011 at 09:37:47AM +0800, Jeremy Kerr wrote:
> Hi Sascha,
> 
> > > +
> > > +propagate:
> > > +	ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* ops->set_rate may require the parent's rate to change (to
> > > +	 * parent_rate), we need to propagate the set_rate call to the
> > > +	 * parent.
> > > +	 */
> > > +	if (ret == CLK_SET_RATE_PROPAGATE) {
> > > +		new_rate = parent_rate;
> > > +		clk = clk->parent;
> > > +		goto propagate;
> > > +	}
> > 
> > I'm unsure about this one. Every clock should have the ability to stop
> > or continue the rate propagation to the parent. This suggests to leave
> > the decision whether or not to propagate to the core and not to the
> > individual clocks.
> 
> Maybe I'm not understanding you correctly, but that's exactly what this
> code does. The decision to propagate is left up to the
> implementation-specific set_rate callback - if it returns
> CLK_SET_RATE_PROPAGATE (and populate the parent_rate argument with the
> requested parent rate), then we propagate the rate change to the parent.

I understood how the code is meant to work. It's just that IMO the place
where the propagation flag is stored is the wrong one, given that it's a
flag that all clocks (can) have.

> 
> > Right now each mux/div/gate needs an individual propagate flag. By
> > adding the flag to the core the building block implementations could be
> > simpler and the knowledge about propagatability might become handy for
> > the core later.
> 
> We could do this with a flag too, yes. But then there's no way of
> altering the rate (which we need to do with a divider) as we propagate
> it upwards. The current set_rate code lets us do that.

Hm, the core could pass a NULL pointer as the third argument to
set_rate to indicate that the parent rate is not allowed to change.
Then we could initialize &parent_rate to zero before calling set_rate.
If the set_rate function does not change it, we don't have to propagate,
otherwise yes. Or instead we could just use the CLK_SET_RATE_PROPAGATE
return value like we do in the current version.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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/