Re: [PATCH RFC v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Mon, 24 Feb 2014 19:11:18 +0100, Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On 24/02/14 01:48, Mike Turquette wrote:
> > Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> >> > On 20/02/14 15:09, Grant Likely wrote:
> >> > [...]
> >>>>> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > index 7c52c29..d618498 100644
> >>>>> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > @@ -115,3 +115,27 @@ clock signal, and a UART.
> >>>>> > >> >    ("pll" and "pll-switched").
> >>>>> > >> >  * The UART has its baud clock connected the external oscillator and its
> >>>>> > >> >    register clock connected to the PLL clock (the "pll-switched" signal)
> >>>>> > >> > +
> >>>>> > >> > +==Static initial configuration of clock parent and clock frequency==
> >>>>> > >> > +
> >>>>> > >> > +Some platforms require static configuration of (parts of) the clock controller
> >>>>> > >> > +often determined by the board design. Such a configuration can be specified in
> >>>>> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> >>>>> > >> > +properties. The former should contain phandle and clock specifier of the parent
> >>>>> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> >>>>> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
> >>>>> > >> > +pair corresponding to it should be present in the clocks property.
> >>>>> > >> > +
> >>>>> > >> > +    uart@a000 {
> >>>>> > >> > +        compatible = "fsl,imx-uart";
> >>>>> > >> > +        reg = <0xa000 0x1000>;
> >>>>> > >> > +  ...
> >>>>> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> >>>>> > >> > +        clock-names = "baud", "mux";
> >>>>> > >> > +
> >>>>> > >> > +  mux-clk-parent = <&pll 1>;
> >>>>> > >> > +  baud-clk-rate = <460800>;
> >>> > >
> >>> > > This mixes patterns for references to clocks. Plus it requires composing
> >>> > > property names which is a little painful. I'd rather see a list of
> >>> > > tuples to match the existing pattern already in use
> >>> > > 
> >>> > >       clocks = <&clkcon 0>, <&clkcon 3>;
> >>> > >       clock-names = "baud", "mux";
> >>> > >       clock-parents = <0> <&pll 1>;
> >>> > >       clock-rates = <0> <460800>;
> >> > 
> >> > Thank you for the review. This looks much better to me. My bad, I wasn't 
> >> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> >> > to be specifying exact meaning of the 'phandle' property values, except 
> >> > they be unique. Anyway, it seems to be clearly documented within the
> >> > __of_parse_phandle_with_args() function. 
> >> > 
> >> > I'll try this modified binding instead, presumably it would be useful to 
> >> > have a variant of __of_parse_phandle_with_args() function which would 
> >> > accept a context data containing result of previous call within an iteration, 
> >> > similarly as of_property_next_string() is written. So we don't iterate 
> >> > from beginning of the list with each __of_parse_phandle_with_args() call. 
> >> > But it's an optimization issue that could be considered separately I guess.
> >
> > I was always partial to the regulator style of blahblah-supply but
> > Grant's suggestion is much cleaner with respect to the rest of the clock
> > binding.
> 
> I don't have a strong opinion, I'm slightly inclined towards Grant's suggestion,
> which doesn't have a problem of limiting effective clk name to 21 characters.
> 
> Also DT property names with underscores coming from the clock names are a bit
> incorrect and it might be not immediately obvious which part of name is a 
> canonical property's name and which is clock's name. Let's consider property 
> names like:
> 
> mux-clk-parent, divider-clk-rate, sclk_mmc0-clk-parent, sclk_uart_baud0-clk-parent,
> etc.
> 
> > I guess it will be a bit ugly if a very long array is needed with a
> > sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> > only a single clock needs to have its parent or rate specified.
> 
> It was also concerning me, but this inconvenience could be mitigated by 
> reordering content of clocks/clock-names properties so that the clocks for
> which parents and/or rates are being assigned are first on the list.
> Then number of padding zeros is minimized.

If the array really gets that long for a device, then groups of clocks
can also be split off into sub-nodes.

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html