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 intduty_num;
> > + unsigned intduty_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(cor
Re: [PATCH 1/3] clk: add duty cycle support
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 intduty_num;
> + unsigned intduty_den;
Maybe this can be a struct?
struct duty {
unsigned int num;
unsigned int den;
};
Or even more generically, a struct frac?
> 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?
> + 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.
> + 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.
> +
> + 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?
> + 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?
> + 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! :)
> + */
> +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.
> + 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
[PATCH 1/3] clk: add duty cycle support
Add the possibility to apply and query the clock signal duty cycle ratio.
This is useful when the duty cycle of the clock signal depends on some
other parameters controlled by the clock framework.
Signed-off-by: Jerome Brunet
---
drivers/clk/clk.c| 196 +--
include/linux/clk-provider.h | 17
include/linux/clk.h | 32 +++
include/trace/events/clk.h | 36
4 files changed, 276 insertions(+), 5 deletions(-)
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 intduty_num;
+ unsigned intduty_den;
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)
+ 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))
+ 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;
+
+ 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)
+ 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);
+ 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
+ *
+ * ADD BLURB HERE
+ */
+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))
+ 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);
+
+ 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);
+}
+EXPORT_SYMBOL_GPL(__clk_set_duty_cycle_passthro
