| 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
| ||
|
Date: Tue, 17 Apr 2018 08:28:50 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Stephen Boyd <sboyd@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Russell King <linux@...linux.org.uk>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] clk: add duty cycle support
On Mon, 2018-04-16 at 22:43 -0700, Stephen Boyd wrote:
> Quoting Jerome Brunet (2018-04-16 10:57:41)
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 7af555f0e60c..fff7890ae355 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -68,6 +68,8 @@ struct clk_core {
> > unsigned long max_rate;
> > unsigned long accuracy;
> > int phase;
> > + unsigned int duty_num;
> > + unsigned int duty_den;
>
> Maybe this can be a struct?
>
> struct duty {
> unsigned int num;
> unsigned int den;
> };
>
> Or even more generically, a struct frac?
Sure, I'll look into it
>
> > struct hlist_head children;
> > struct hlist_node child_node;
> > struct hlist_head clks;
> > @@ -2401,6 +2403,164 @@ int clk_get_phase(struct clk *clk)
> > }
> > EXPORT_SYMBOL_GPL(clk_get_phase);
> >
> > +static int clk_core_update_duty_cycle_nolock(struct clk_core *core)
> > +{
> > + int ret;
> > + unsigned int num, den;
> > +
> > + if (!core || !core->ops->get_duty_cycle)
>
> Does !core happen?
With the passthrough mechanism, it does
>
> > + return 0;
> > +
> > + /* Update the duty cycle if the callback is available */
> > + ret = core->ops->get_duty_cycle(core->hw, &num, &den);
> > + if (ret)
> > + return ret;
> > +
> > + /* Don't trust the clock provider too much */
> > + if (den == 0 || (num > den))
>
> Drop useless parenthesis please.
Sure
>
> > + return -EINVAL;
> > +
> > + core->duty_num = num;
> > + core->duty_den = den;
> > +
> > + return 0;
> > +}
> > +
> > +static int clk_core_get_duty_cycle_nolock(struct clk_core *core,
> > + unsigned int *num,
> > + unsigned int *den)
> > +{
> > + int ret;
> > +
> > + if (!core)
> > + return 0;
>
> Shouldn't this return 50%? At least it seems like the "no clk" case
> should be the default 50% duty cycle case.
Can't hurt, It is certainly better than leaving whatever was there.
>
> > +
> > + ret = clk_core_update_duty_cycle_nolock(core);
> > +
> > + if (!ret) {
> > + *num = core->duty_num;
> > + *den = core->duty_den;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int clk_core_set_duty_cycle_nolock(struct clk_core *core,
> > + unsigned int num,
> > + unsigned int den)
> > +{
> > + int ret;
> > +
> > + lockdep_assert_held(&prepare_lock);
> > +
> > + if (!core || !core->ops->set_duty_cycle)
>
> Does the !core case happen?
yep
>
> > + return 0;
> > +
> > + if (clk_core_rate_is_protected(core))
> > + return -EBUSY;
> > +
> > + trace_clk_set_duty_cycle(core, num, den);
> > +
> > + ret = core->ops->set_duty_cycle(core->hw, num, den);
>
> Is the assumption that we aren't going to need to adjust the duty cycle
> of any parent clks when we set the duty cycle on a clk?
I'm not sure I understand what you mean, but I don't make any assumption about
the parent, Did I miss something ?
>
> > + if (ret)
> > + return ret;
> > +
> > + core->duty_num = num;
> > + core->duty_den = den;
> > +
> > + trace_clk_set_duty_cycle_complete(core, num, den);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * clk_set_duty_cycle - adjust the duty cycle ratio of a clock signal
> > + * @clk: clock signal source
> > + * @ratio: duty cycle ratio in milli-percent
>
> This doesn't match anymore.
>
> > + *
> > + * ADD BLURB HERE
>
> Please do! :)
Oops ... sorry about that.
>
> > + */
> > +int clk_set_duty_cycle(struct clk *clk, unsigned int num, unsigned int den)
> > +{
> > + int ret;
> > +
> > + if (!clk)
> > + return 0;
> > +
> > + /* sanity check the ratio */
> > + if (den == 0 || (num > den))
>
> Drop useless parenthesis please.
sure
>
> > + return -EINVAL;
> > +
> > + clk_prepare_lock();
> > +
> > + if (clk->exclusive_count)
> > + clk_core_rate_unprotect(clk->core);
> > +
> > + ret = clk_core_set_duty_cycle_nolock(clk->core, num, den);
> > +
> > + if (clk->exclusive_count)
> > + clk_core_rate_protect(clk->core);
> > +
> > + clk_prepare_unlock();
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_set_duty_cycle);
> > +
> > +static unsigned int clk_core_get_scaled_duty_cycle(struct clk_core *core,
> > + unsigned int scale)
> > +{
> > + int ret;
> > + unsigned int duty;
> > +
> > + clk_prepare_lock();
> > + ret = clk_core_update_duty_cycle_nolock(core);
> > + if (ret)
> > + return 0;
> > +
> > + duty = DIV_ROUND_CLOSEST_ULL((u64)core->duty_num * scale,
> > + core->duty_den);
>
> Just use mult_frac() instead of casting and rounding to closest? I
> haven't looked closely so feel free to correct me if that doesn't make
> sense.
>
> > +
> > + clk_prepare_unlock();
> > +
> > + return duty;
> > +}
> > +
> > +/**
> > + * clk_get_scaled_duty_cycle - return the duty cycle ratio of a clock signal
> > + * @clk: clock signal source
> > + * @scale: scaling factor to be applied to represent the ratio as an integer
> > + *
> > + * Returns the duty cycle ratio of a clock node multiplied by the provided
> > + * scaling factor.
> > + */
> > +unsigned int clk_get_scaled_duty_cycle(struct clk *clk, unsigned int scale)
> > +{
> > + if (!clk)
> > + return 0;
> > +
> > + return clk_core_get_scaled_duty_cycle(clk->core, scale);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_get_scaled_duty_cycle);
> > +
> > +int __clk_set_duty_cycle_passthrough(struct clk_hw *hw, unsigned int num,
> > + unsigned int den)
> > +{
> > + struct clk_core *parent = hw->core->parent;
> > +
> > + return clk_core_set_duty_cycle_nolock(parent, num, den);
>
> Ah I see. So a pass through clk will do the parent traversal itself? And
> more complicated cases could even adjust their phase and then call to
> the parent to do some other adjustment?
Actually, it is more a delegation. Either the clock
* can't do anything (fixed duty)
* or, can the duty cycle
* or, it a "wire" a provides whatever the parent provides
I'm not sure if making something more complicated makes sense. Duty cycle does
not seem to be a function of the parent duty cycle, unlike the rate.
I'm not against the suggestion, but I don't see the use case. Could you give a
hint ?
>
> If we ever want to make the prepare lock more fine grained then we'll
> want the framework to be in control or at least aware of the traversal
> that's going on,
If/when it gets done, I suspect this new lock would have tight relationship with
the core structure, no ? I guess putting the lock where lockdep_assert_held() is
in clk_core_get_duty_cycle_nolock() and clk_core_set_duty_cycle_nolock() would
do the trick.
> so it would be better to design it more like how
> clk_set_rate() works today,
you mean with a recalc(), determine() and set() callback ?
I thought about it, but it seemed a bit too much for a v1 ...
Even with this, as far as I understand, the clock does tree traversal for rate
in the same way, in determine()/round() instead of set().
in clk-divider.c:clk_divider_bestdiv()
- parent_rate = clk_hw_round_rate(parent, rate * i);
Seems to be the same thing/issue.
Did you mean something else ? like core flag (CLK_SET_DUTY_PASSTHROUGH) ?
> even if that is only to direct the traversal
> upwards to the parent until we find the last parent to actually change
> duty cycle on.
> Then we can more easily lock the right clk subtree
> without having to worry about clk providers traversing the tree
> themselves.
>
> > +}
> > +EXPORT_SYMBOL_GPL(__clk_set_duty_cycle_passthrough);
> > +
> > +int __clk_get_duty_cycle_passthrough(struct clk_hw *hw, unsigned int *num,
> > + unsigned int *den)
> > +{
> > + struct clk_core *parent = hw->core->parent;
> > +
> > + return clk_core_get_duty_cycle_nolock(parent, num, den);
> > +}
> > +EXPORT_SYMBOL_GPL(__clk_get_duty_cycle_passthrough);
> > +
> > /**
> > * clk_is_match - check if two clk's point to the same hardware clock
> > * @p: clk compared against q
> > @@ -2638,6 +2801,16 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
> > if (!d)
> > goto err_out;
> >
> > + d = debugfs_create_u32("clk_duty_num", 0444, core->dentry,
>
> Can we spell it out? clk_duty_numerator?
sure
>
> > + &core->duty_num);
> > + if (!d)
> > + goto err_out;
> > +
> > + d = debugfs_create_u32("clk_duty_den", 0444, core->dentry,
>
> And clk_duty_denominator? Or have a two entry file that reads both so
> that there isn't a "debugfs race" where the duty is half updated while
> we read the numerator and denominator changes or something.
I thought it was best keep it simple to parse, with just one value.
I prefer to have this in a single file, I'll do it.
>
> > + &core->duty_den);
> > + if (!d)
> > + goto err_out;
> > +
> > d = debugfs_create_file("clk_flags", 0444, core->dentry, core,
> > &clk_flags_fops);
> > if (!d)
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 1d25e149c1c5..91e0e37e736b 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -168,6 +168,15 @@ struct clk_rate_request {
> > * by the second argument. Valid values for degrees are
> > * 0-359. Return 0 on success, otherwise -EERROR.
> > *
> > + * @get_duty_cycle: Queries the hardware to get the current duty cycle ratio
> > + * of a clock. Returned values denominator cannot be 0 and must be
> > + * superior or egal to the numerator.
>
> equal?
>
> > + *
> > + * @set_duty_cycle: Apply the duty cycle ratio to this clock signal specified by
> > + * the numerator (2nd argurment) and denominator (3rd argument).
> > + * Argument must be a valid ratio (denominator > 0
> > + * and >= numerator) Return 0 on success, otherwise -EERROR.
> > + *
> > * @init: Perform platform-specific initialization magic.
> > * This is not not used by any of the basic clock types.
> > * Please consider other ways of solving initialization problems
Powered by blists - more mailing lists