[PATCH 0/3] clk: add duty cycle support

[PATCH 0/3] clk: add duty cycle support

[Jerome Brunet]

This patchset adds the possibility to control the duty cycle ratio of a
clock within the clock framework.

This useful when the duty cycle ratio depends on another parameter
controlled by the clock framework. For example, the duty cycle ratio may
depends on the value of a divider (ratio = N / div).

This is also useful if the related clock is part of large tree. In this
case, using CCF to control the clock tree and the PWM framework for the
duty cycle would be a nightmare for the provider and the consumer.

A clock provider is not required to implement the operation to set and get
the duty cycle. If it does not implement .get_duty_cycle(), the ratio is
assumed to be 50%.

This series also provides pass-through operations ready to be used for
clock which don't resample the clock signal (such as gates and muxes).
This is provided to make things easier for consumers. Clocks are usually
made of several elements, like a composite clocks with a mux, a divider,
and a gate. With the pass-through ops set on the gate, the consumer does
not need to be aware that the duty cycle is actually controlled by the
divider, it can continue to use the clock through the gate, as usual.  The
simple propagation will stop at the first clock which resample the signal
(such as a divider).

Patch 2 and 3 add the pass-through ops to the generic gate and mux ops.  In
this first version, it is unconditional, but maybe we should provide a flag
to let people decide what is best for them ?

The series has been developed to handled the sample clocks provided by
audio clock controller of amlogic's A113 SoC. To support i2s modes, this
clock need to have a 50% duty cycle ratio, while it should be just one
pulse of the parent clock in dsp modes.

PS: Checkpatch complains heavily about the white space in the trace header
file. I believe I have respected the style already used in this
file. Please let me know if it should be done differently.

Jerome Brunet (3):
  clk: add duty cycle support
  clk: gate: add duty cycle passthrough ops
  clk: mux: add duty cycle passthrough ops

 drivers/clk/clk-gate.c       |   2 +
 drivers/clk/clk-mux.c        |   4 +
 drivers/clk/clk.c            | 196 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/clk-provider.h |  17 ++++
 include/linux/clk.h          |  32 +++++++
 include/trace/events/clk.h   |  36 ++++++++
 6 files changed, 282 insertions(+), 5 deletions(-)

-- 
2.14.3

[PATCH 1/3] clk: add duty cycle support

[Jerome Brunet]

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 <jbrunet@baylibre.com>
---
 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 int		duty_num;
+	unsigned int		duty_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_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
@@ -2454,12 +2614,13 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
 	if (!c)
 		return;
 
-	seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu %-3d\n",
+	seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu %5d %6d\n",
 		   level * 3 + 1, "",
 		   30 - level * 3, c->name,
 		   c->enable_count, c->prepare_count, c->protect_count,
 		   clk_core_get_rate(c), clk_core_get_accuracy(c),
-		   clk_core_get_phase(c));
+		   clk_core_get_phase(c),
+		   clk_core_get_scaled_duty_cycle(c, 100000));
 }
 
 static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
@@ -2481,9 +2642,9 @@ static int clk_summary_show(struct seq_file *s, void *data)
 	struct clk_core *c;
 	struct hlist_head **lists = (struct hlist_head **)s->private;
 
-	seq_puts(s, "                                 enable  prepare  protect                               \n");
-	seq_puts(s, "   clock                          count    count    count        rate   accuracy   phase\n");
-	seq_puts(s, "----------------------------------------------------------------------------------------\n");
+	seq_puts(s, "                                 enable  prepare  protect                                duty\n");
+	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle\n");
+	seq_puts(s, "---------------------------------------------------------------------------------------------\n");
 
 	clk_prepare_lock();
 
@@ -2510,6 +2671,8 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
 	seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c));
 	seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c));
 	seq_printf(s, "\"phase\": %d", clk_core_get_phase(c));
+	seq_printf(s, "\"duty_cycle\": %u",
+		   clk_core_get_scaled_duty_cycle(c, 100000));
 }
 
 static void clk_dump_subtree(struct seq_file *s, struct clk_core *c, int level)
@@ -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,
+			       &core->duty_num);
+	if (!d)
+		goto err_out;
+
+	d = debugfs_create_u32("clk_duty_den", 0444, core->dentry,
+			       &core->duty_den);
+	if (!d)
+		goto err_out;
+
 	d = debugfs_create_file("clk_flags", 0444, core->dentry, core,
 				&clk_flags_fops);
 	if (!d)
@@ -2926,6 +3099,19 @@ static int __clk_core_init(struct clk_core *core)
 	else
 		core->phase = 0;
 
+	/*
+	 * Set clk's duty cycle.
+	 */
+	if (core->ops->get_duty_cycle) {
+		core->ops->get_duty_cycle(core->hw, &core->duty_num,
+					  &core->duty_den);
+	} else {
+		/* if the operation is not available, assume 50% */
+		core->duty_num = 1;
+		core->duty_den = 2;
+	}
+
+
 	/*
 	 * Set clk's rate.  The preferred method is to use .recalc_rate.  For
 	 * simple clocks and lazy developers the default fallback is to use the
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.
+ *
+ * @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
@@ -217,6 +226,10 @@ struct clk_ops {
 					   unsigned long parent_accuracy);
 	int		(*get_phase)(struct clk_hw *hw);
 	int		(*set_phase)(struct clk_hw *hw, int degrees);
+	int		(*get_duty_cycle)(struct clk_hw *hw, unsigned int *num,
+					  unsigned int *den);
+	int		(*set_duty_cycle)(struct clk_hw *hw, unsigned int num,
+					  unsigned int den);
 	void		(*init)(struct clk_hw *hw);
 	int		(*debug_init)(struct clk_hw *hw, struct dentry *dentry);
 };
@@ -771,6 +784,10 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
+int __clk_set_duty_cycle_passthrough(struct clk_hw *hw, unsigned int num,
+				     unsigned int den);
+int __clk_get_duty_cycle_passthrough(struct clk_hw *hw, unsigned int *num,
+				     unsigned int *den);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 0dbd0885b2c2..ef363fd6218a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -141,6 +141,26 @@ int clk_set_phase(struct clk *clk, int degrees);
  */
 int clk_get_phase(struct clk *clk);
 
+/**
+ * clk_set_duty_cycle - adjust the duty cycle ratio of a clock signal
+ * @clk: clock signal source
+ * @num: numerator of the duty cycle ratio to be applied
+ * @den: denominator of the duty cycle ratio to be applied
+ *
+ * Adjust the duty cycle of a clock signal by the specified ratio. Returns 0 on
+ * success, -EERROR otherwise.
+ */
+int clk_set_duty_cycle(struct clk *clk, unsigned int num, unsigned int den);
+
+/**
+ * clk_get_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 multiplied by the scale provided
+ */
+unsigned int clk_get_scaled_duty_cycle(struct clk *clk, unsigned int scale);
+
 /**
  * clk_is_match - check if two clk's point to the same hardware clock
  * @p: clk compared against q
@@ -183,6 +203,18 @@ static inline long clk_get_phase(struct clk *clk)
 	return -ENOTSUPP;
 }
 
+static inline int clk_set_duty_cycle(struct clk *clk, unsigned int num,
+				     unsigned int den)
+{
+	return -ENOTSUPP;
+}
+
+static inline unsigned int clk_get_scaled_duty_cycle(struct clk *clk,
+						     unsigned int scale)
+{
+	return 0;
+}
+
 static inline bool clk_is_match(const struct clk *p, const struct clk *q)
 {
 	return p == q;
diff --git a/include/trace/events/clk.h b/include/trace/events/clk.h
index 2cd449328aee..9625a19cc159 100644
--- a/include/trace/events/clk.h
+++ b/include/trace/events/clk.h
@@ -192,6 +192,42 @@ DEFINE_EVENT(clk_phase, clk_set_phase_complete,
 	TP_ARGS(core, phase)
 );
 
+DECLARE_EVENT_CLASS(clk_duty_cycle,
+
+	TP_PROTO(struct clk_core *core, unsigned int num, unsigned int den),
+
+		    TP_ARGS(core, num, den),
+
+	TP_STRUCT__entry(
+		__string(        name,           core->name              )
+		__field( unsigned int,           num                     )
+		__field( unsigned int,           den                     )
+	),
+
+	TP_fast_assign(
+		__assign_str(name, core->name);
+		__entry->num = num;
+		__entry->den = den;
+	),
+
+	TP_printk("%s %u/%u", __get_str(name), (unsigned int)__entry->num,
+		  (unsigned int)__entry->den)
+);
+
+DEFINE_EVENT(clk_duty_cycle, clk_set_duty_cycle,
+
+	TP_PROTO(struct clk_core *core, unsigned int num, unsigned int den),
+
+	TP_ARGS(core, num, den)
+);
+
+DEFINE_EVENT(clk_duty_cycle, clk_set_duty_cycle_complete,
+
+	TP_PROTO(struct clk_core *core, unsigned int num, unsigned int den),
+
+	TP_ARGS(core, num, den)
+);
+
 #endif /* _TRACE_CLK_H */
 
 /* This part must be outside protection */
-- 
2.14.3

[PATCH 2/3] clk: gate: add duty cycle passthrough ops

[Jerome Brunet]

A clock gate does not resample the clock signal, it give the same
signal as the parent if enabled, so it can use the duty cycle
passthrough operations

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk-gate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index dd82485e09a1..eb6dcebfcd5c 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -107,6 +107,8 @@ const struct clk_ops clk_gate_ops = {
 	.enable = clk_gate_enable,
 	.disable = clk_gate_disable,
 	.is_enabled = clk_gate_is_enabled,
+	.set_duty_cycle = __clk_set_duty_cycle_passthrough,
+	.get_duty_cycle = __clk_get_duty_cycle_passthrough,
 };
 EXPORT_SYMBOL_GPL(clk_gate_ops);
 
-- 
2.14.3

[PATCH 3/3] clk: mux: add duty cycle passthrough ops

[Jerome Brunet]

A clock mux does not resample the clock signal, it give the same
signal as the selected parent, so it can use the duty cycle
passthrough operations.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk-mux.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 1628b93655ed..f7c597779928 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -124,11 +124,15 @@ const struct clk_ops clk_mux_ops = {
 	.get_parent = clk_mux_get_parent,
 	.set_parent = clk_mux_set_parent,
 	.determine_rate = clk_mux_determine_rate,
+	.set_duty_cycle = __clk_set_duty_cycle_passthrough,
+	.get_duty_cycle = __clk_get_duty_cycle_passthrough,
 };
 EXPORT_SYMBOL_GPL(clk_mux_ops);
 
 const struct clk_ops clk_mux_ro_ops = {
 	.get_parent = clk_mux_get_parent,
+	.set_duty_cycle = __clk_set_duty_cycle_passthrough,
+	.get_duty_cycle = __clk_get_duty_cycle_passthrough,
 };
 EXPORT_SYMBOL_GPL(clk_mux_ro_ops);
 
-- 
2.14.3

Re: [PATCH 1/3] clk: add duty cycle support

[Stephen Boyd]

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?

>         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
> + * 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?

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, so it would be better to design it more like how
clk_set_rate() works today, 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?

> +                              &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.

> +                              &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

Re: [PATCH 1/3] clk: add duty cycle support

[Stephen Boyd]

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?

>         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 =3D core->ops->get_duty_cycle(core->hw, &num, &den);
> +       if (ret)
> +               return ret;
> +
> +       /* Don't trust the clock provider too much */
> +       if (den =3D=3D 0 || (num > den))

Drop useless parenthesis please.

> +               return -EINVAL;
> +
> +       core->duty_num =3D num;
> +       core->duty_den =3D 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 =3D clk_core_update_duty_cycle_nolock(core);
> +
> +       if (!ret) {
> +               *num =3D core->duty_num;
> +               *den =3D 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 =3D 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 =3D num;
> +       core->duty_den =3D 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 d=
en)
> +{
> +       int ret;
> +
> +       if (!clk)
> +               return 0;
> +
> +       /* sanity check the ratio */
> +       if (den =3D=3D 0 || (num > den))

Drop useless parenthesis please.

> +               return -EINVAL;
> +
> +       clk_prepare_lock();
> +
> +       if (clk->exclusive_count)
> +               clk_core_rate_unprotect(clk->core);
> +
> +       ret =3D 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 =3D clk_core_update_duty_cycle_nolock(core);
> +       if (ret)
> +               return 0;
> +
> +       duty =3D 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 si=
gnal
> + * @clk: clock signal source
> + * @scale: scaling factor to be applied to represent the ratio as an int=
eger
> + *
> + * Returns the duty cycle ratio of a clock node multiplied by the provid=
ed
> + * scaling factor.
> + */
> +unsigned int clk_get_scaled_duty_cycle(struct clk *clk, unsigned int sca=
le)
> +{
> +       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 =3D 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?

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, so it would be better to design it more like how
clk_set_rate() works today, 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 *nu=
m,
> +                                    unsigned int *den)
> +{
> +       struct clk_core *parent =3D 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 *c=
ore, struct dentry *pdentry)
>         if (!d)
>                 goto err_out;
>  =

> +       d =3D debugfs_create_u32("clk_duty_num", 0444, core->dentry,

Can we spell it out? clk_duty_numerator?

> +                              &core->duty_num);
> +       if (!d)
> +               goto err_out;
> +
> +       d =3D 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.

> +                              &core->duty_den);
> +       if (!d)
> +               goto err_out;
> +
>         d =3D 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 r=
atio
> + *              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 spec=
ified by
> + *              the numerator (2nd argurment) and denominator (3rd  argu=
ment).
> + *              Argument must be a valid ratio (denominator > 0
> + *              and >=3D numerator) Return 0 on success, otherwise -EERR=
OR.
> + *
>   * @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 prob=
lems

Re: [PATCH 0/3] clk: add duty cycle support

[Stephen Boyd]

Quoting Jerome Brunet (2018-04-16 10:57:40)
> This patchset adds the possibility to control the duty cycle ratio of a
> clock within the clock framework.
> 
> This useful when the duty cycle ratio depends on another parameter
> controlled by the clock framework. For example, the duty cycle ratio may
> depends on the value of a divider (ratio = N / div).
> 
> This is also useful if the related clock is part of large tree. In this
> case, using CCF to control the clock tree and the PWM framework for the
> duty cycle would be a nightmare for the provider and the consumer.

Can you please Cc the PWM maintainer on this patch series?

> 
> A clock provider is not required to implement the operation to set and get
> the duty cycle. If it does not implement .get_duty_cycle(), the ratio is
> assumed to be 50%.
> 
> This series also provides pass-through operations ready to be used for
> clock which don't resample the clock signal (such as gates and muxes).
> This is provided to make things easier for consumers. Clocks are usually
> made of several elements, like a composite clocks with a mux, a divider,
> and a gate. With the pass-through ops set on the gate, the consumer does
> not need to be aware that the duty cycle is actually controlled by the
> divider, it can continue to use the clock through the gate, as usual.  The
> simple propagation will stop at the first clock which resample the signal
> (such as a divider).
> 
> Patch 2 and 3 add the pass-through ops to the generic gate and mux ops.  In
> this first version, it is unconditional, but maybe we should provide a flag
> to let people decide what is best for them ?
> 
> The series has been developed to handled the sample clocks provided by
> audio clock controller of amlogic's A113 SoC. To support i2s modes, this
> clock need to have a 50% duty cycle ratio, while it should be just one
> pulse of the parent clock in dsp modes.

Cool. I've heard rumblings from some other SoC manufacturers that they
want duty cycle control via the clk framework but nothing came of it, so
thanks for picking this up.

> 
> PS: Checkpatch complains heavily about the white space in the trace header
> file. I believe I have respected the style already used in this
> file. Please let me know if it should be done differently.

The trace file looks fine. Ignore checkpatch.

Re: [PATCH 0/3] clk: add duty cycle support

[Stephen Boyd]

Quoting Jerome Brunet (2018-04-16 10:57:40)
> This patchset adds the possibility to control the duty cycle ratio of a
> clock within the clock framework.
> =

> This useful when the duty cycle ratio depends on another parameter
> controlled by the clock framework. For example, the duty cycle ratio may
> depends on the value of a divider (ratio =3D N / div).
> =

> This is also useful if the related clock is part of large tree. In this
> case, using CCF to control the clock tree and the PWM framework for the
> duty cycle would be a nightmare for the provider and the consumer.

Can you please Cc the PWM maintainer on this patch series?

> =

> A clock provider is not required to implement the operation to set and get
> the duty cycle. If it does not implement .get_duty_cycle(), the ratio is
> assumed to be 50%.
> =

> This series also provides pass-through operations ready to be used for
> clock which don't resample the clock signal (such as gates and muxes).
> This is provided to make things easier for consumers. Clocks are usually
> made of several elements, like a composite clocks with a mux, a divider,
> and a gate. With the pass-through ops set on the gate, the consumer does
> not need to be aware that the duty cycle is actually controlled by the
> divider, it can continue to use the clock through the gate, as usual.  The
> simple propagation will stop at the first clock which resample the signal
> (such as a divider).
> =

> Patch 2 and 3 add the pass-through ops to the generic gate and mux ops.  =
In
> this first version, it is unconditional, but maybe we should provide a fl=
ag
> to let people decide what is best for them ?
> =

> The series has been developed to handled the sample clocks provided by
> audio clock controller of amlogic's A113 SoC. To support i2s modes, this
> clock need to have a 50% duty cycle ratio, while it should be just one
> pulse of the parent clock in dsp modes.

Cool. I've heard rumblings from some other SoC manufacturers that they
want duty cycle control via the clk framework but nothing came of it, so
thanks for picking this up.

> =

> PS: Checkpatch complains heavily about the white space in the trace header
> file. I believe I have respected the style already used in this
> file. Please let me know if it should be done differently.

The trace file looks fine. Ignore checkpatch.

Re: [PATCH 1/3] clk: add duty cycle support

[Jerome Brunet]

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

Re: [PATCH 1/3] clk: add duty cycle support

[Jerome Brunet]

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

Re: [PATCH 0/3] clk: add duty cycle support

[Jerome Brunet]

On Mon, 2018-04-16 at 22:49 -0700, Stephen Boyd wrote:
> Quoting Jerome Brunet (2018-04-16 10:57:40)
> > This patchset adds the possibility to control the duty cycle ratio of a
> > clock within the clock framework.
> > 
> > This useful when the duty cycle ratio depends on another parameter
> > controlled by the clock framework. For example, the duty cycle ratio may
> > depends on the value of a divider (ratio = N / div).
> > 
> > This is also useful if the related clock is part of large tree. In this
> > case, using CCF to control the clock tree and the PWM framework for the
> > duty cycle would be a nightmare for the provider and the consumer.
> 
> Can you please Cc the PWM maintainer on this patch series?
Sure. I'll see him of the v2.

> 
> > 
> > A clock provider is not required to implement the operation to set and get
> > the duty cycle. If it does not implement .get_duty_cycle(), the ratio is
> > assumed to be 50%.
> > 
> > This series also provides pass-through operations ready to be used for
> > clock which don't resample the clock signal (such as gates and muxes).
> > This is provided to make things easier for consumers. Clocks are usually
> > made of several elements, like a composite clocks with a mux, a divider,
> > and a gate. With the pass-through ops set on the gate, the consumer does
> > not need to be aware that the duty cycle is actually controlled by the
> > divider, it can continue to use the clock through the gate, as usual.  The
> > simple propagation will stop at the first clock which resample the signal
> > (such as a divider).
> > 
> > Patch 2 and 3 add the pass-through ops to the generic gate and mux ops.  In
> > this first version, it is unconditional, but maybe we should provide a flag
> > to let people decide what is best for them ?
> > 
> > The series has been developed to handled the sample clocks provided by
> > audio clock controller of amlogic's A113 SoC. To support i2s modes, this
> > clock need to have a 50% duty cycle ratio, while it should be just one
> > pulse of the parent clock in dsp modes.
> 
> Cool. I've heard rumblings from some other SoC manufacturers that they
> want duty cycle control via the clk framework but nothing came of it, so
> thanks for picking this up.
> 
> > 
> > PS: Checkpatch complains heavily about the white space in the trace header
> > file. I believe I have respected the style already used in this
> > file. Please let me know if it should be done differently.
> 
> The trace file looks fine. Ignore checkpatch.

Re: [PATCH 0/3] clk: add duty cycle support

[Jerome Brunet]

On Mon, 2018-04-16 at 22:49 -0700, Stephen Boyd wrote:
> Quoting Jerome Brunet (2018-04-16 10:57:40)
> > This patchset adds the possibility to control the duty cycle ratio of a
> > clock within the clock framework.
> > 
> > This useful when the duty cycle ratio depends on another parameter
> > controlled by the clock framework. For example, the duty cycle ratio may
> > depends on the value of a divider (ratio = N / div).
> > 
> > This is also useful if the related clock is part of large tree. In this
> > case, using CCF to control the clock tree and the PWM framework for the
> > duty cycle would be a nightmare for the provider and the consumer.
> 
> Can you please Cc the PWM maintainer on this patch series?
Sure. I'll see him of the v2.

> 
> > 
> > A clock provider is not required to implement the operation to set and get
> > the duty cycle. If it does not implement .get_duty_cycle(), the ratio is
> > assumed to be 50%.
> > 
> > This series also provides pass-through operations ready to be used for
> > clock which don't resample the clock signal (such as gates and muxes).
> > This is provided to make things easier for consumers. Clocks are usually
> > made of several elements, like a composite clocks with a mux, a divider,
> > and a gate. With the pass-through ops set on the gate, the consumer does
> > not need to be aware that the duty cycle is actually controlled by the
> > divider, it can continue to use the clock through the gate, as usual.  The
> > simple propagation will stop at the first clock which resample the signal
> > (such as a divider).
> > 
> > Patch 2 and 3 add the pass-through ops to the generic gate and mux ops.  In
> > this first version, it is unconditional, but maybe we should provide a flag
> > to let people decide what is best for them ?
> > 
> > The series has been developed to handled the sample clocks provided by
> > audio clock controller of amlogic's A113 SoC. To support i2s modes, this
> > clock need to have a 50% duty cycle ratio, while it should be just one
> > pulse of the parent clock in dsp modes.
> 
> Cool. I've heard rumblings from some other SoC manufacturers that they
> want duty cycle control via the clk framework but nothing came of it, so
> thanks for picking this up.
> 
> > 
> > PS: Checkpatch complains heavily about the white space in the trace header
> > file. I believe I have respected the style already used in this
> > file. Please let me know if it should be done differently.
> 
> The trace file looks fine. Ignore checkpatch.