Quoting Sebastian Hesselbarth (2014-05-13 08:11:55)
> On 05/13/2014 02:20 PM, Mark Rutland wrote:
> > On Tue, May 13, 2014 at 12:57:32PM +0100, Gabriel FERNANDEZ wrote:
> >> The patch provides a helper to get flags properties of
> >> a clock node.
> >>
> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxx>
> >> ---
> >> drivers/clk/clk.c | 11 +++++++++++
> >> include/linux/clk-provider.h | 6 ++++++
> >> 2 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index 4d56220..cae8985 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -2528,6 +2528,17 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> >> }
> >> EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
> >>
> >> +unsigned long of_clk_get_flags(struct device_node *np)
> >> +{
> >> + unsigned long flags = 0;
> >> +
> >> + if (of_property_read_bool(np, "set-rate-parent"))
> >> + flags |= CLK_SET_RATE_PARENT;
> >
> > NAK.
> >
> > This is _not_ a hardware property. This flag describes internals of the
> > Linux clock framework, and is thus not suitable for DT.
>
> Mark,
>
> while I agree above property is not a hardware property, it is at least
> some kind of use-case property. If not by DT, we will have to allow some
> way to describe master-slave relationships between clocks in a driver
> independent way.
I agree with Mark.
Generally this stuff belongs in a clock driver. Of course there are the
integration issues you pointed out. More on that below.
As an aside, CLK_SET_RATE_PARENT is a headache, since propagation of the
operation up to the parent clock really should be the default behavior
for .set_rate (and in fact this is the case for the new-ish
.determine_rate op). Some history on those decisions can be found at [1]
and [2].
>
> > You've also failed to document the property.
> >
> > What are you trying to achieve here, and why do you think this is the
> > best way of achieving that?
>
> I cannot tell from the commit msgs, but consider clk-si5351 which is a
> driver for an external programmable clock with N PLLs and M outputs. Now
> connect a video clock consumer and an audio clock consumer to two
> different outputs and those to one PLL (as you want audio clock derived
> from video clock, typical HDMI scenario).
>
> Now, there should be a way to tell the generic driver which outputs are
> allowed to change the PLLs rate and which don't. Otherwise, the clock
> chip would be pretty useless as e.g. your audio clock consumer will
> overwrite the rate the video clock consumer has chosen.
This is really a job for the "coordinated clock rate changes" that are
currently in development. These specify clock sub-tree snapshots of
parent and rate configurations that are predefined. These combinations
can be specified in DT. That helps a lot with clock configurations that
change per board, or for cases where many combinations of parents and
dividers can yield the same output rate, but only a subset of those were
validated by the silicon validation team or had proper timing closure so
we don't want to rely on the "walk up the tree" algorithm.
Regards,
Mike
[1] http://marc.info/?l=linux-kernel&m=136847508109344&w=2
[2] http://marc.info/?l=linux-kernel&m=137477559110865&w=2
>
> BTW, clk-si5351 has vendor-specific properties to specify which output
> clocks are pll-masters for some kernel cycles already.
>
> Sebastian
>
> >> +
> >> + return flags;
> >> +}
> >> +EXPORT_SYMBOL_GPL(of_clk_get_flags);
> >> +
> >> struct clock_provider {
> >> of_clk_init_cb_t clk_init_cb;
> >> struct device_node *np;
> >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> >> index 59e2eb5..650bc10 100644
> >> --- a/include/linux/clk-provider.h
> >> +++ b/include/linux/clk-provider.h
> >> @@ -519,6 +519,7 @@ int of_clk_get_parent_count(struct device_node *np);
> >> const char *of_clk_get_parent_name(struct device_node *np, int index);
> >>
> >> void of_clk_init(const struct of_device_id *matches);
> >> +unsigned long of_clk_get_flags(struct device_node *np);
> >>
> >> #else /* !CONFIG_OF */
> >>
> >> @@ -546,6 +547,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
> >> {
> >> return NULL;
> >> }
> >> +static inline unsigned long of_clk_get_flags(struct device_node *np)
> >> +{
> >> + return NULL;
> >> +}
> >> +
> >> #define of_clk_init(matches) \
> >> { while (0); }
> >> #endif /* CONFIG_OF */
> >> --
> >> 1.9.1
> >>
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
