On Tue, May 27, 2014 at 8:54 PM, Mike Turquette <mturque...@linaro.org> wrote:
> Quoting Nishanth Menon (2014-05-15 05:33:13)
>> On 05/15/2014 07:18 AM, Kishon Vijay Abraham I wrote:
>> > Hi,
>> >
>> > On Thursday 15 May 2014 05:42 PM, Nishanth Menon wrote:
>> >> On Thu, May 15, 2014 at 6:59 AM, Kishon Vijay Abraham I <kis...@ti.com>
>> >> wrote:
>> >>> Hi Nishant,
>> >>>
>> >>> On Thursday 15 May 2014 05:16 PM, Nishanth Menon wrote:
>> >>>> On Thu, May 15, 2014 at 4:25 AM, Roger Quadros <rog...@ti.com> wrote:
>> >>>>> On 05/15/2014 12:15 PM, Kishon Vijay Abraham I wrote:
>> >>>>>> Hi Nishanth,
>> >>>>>>
>> >>>>>> On Wednesday 14 May 2014 09:04 PM, Nishanth Menon wrote:
>> >>>>>>> On Wed, May 14, 2014 at 10:19 AM, Kishon Vijay Abraham I
>> >>>>>>> <kis...@ti.com> wrote:
>> >>>>>>>> Hi Roger,
>> >>>>>>>>
>> >>>>>>>> On Wednesday 14 May 2014 06:46 PM, Roger Quadros wrote:
>> >>>>>>>>> Hi Kishon,
>> >>>>>>>>>
>> >>>>>>>>> On 05/06/2014 04:33 PM, Kishon Vijay Abraham I wrote:
>> >>>>>>>>>> APLL used by PCIE phy can either use external clock as input or
>> >>>>>>>>>> the clock
>> >>>>>>>>>> from DPLL. Added support for the APLL to use external clock as
>> >>>>>>>>>> input here.
>> >>>>>>>>>>
>> >>>>>>>>>> Cc: Rajendra Nayak <rna...@ti.com>
>> >>>>>>>>>> Cc: Tero Kristo <t-kri...@ti.com>
>> >>>>>>>>>> Cc: Paul Walmsley <p...@pwsan.com>
>> >>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kis...@ti.com>
>> >>>>>>>>>> ---
>> >>>>>>>>>> Documentation/devicetree/bindings/phy/ti-phy.txt | 4 ++
>> >>>>>>>>>> drivers/phy/phy-ti-pipe3.c | 75
>> >>>>>>>>>> ++++++++++++++--------
>> >>>>>>>>>> 2 files changed, 52 insertions(+), 27 deletions(-)
>> >>>>>>>>>>
>> >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> index bc9afb5..d50f8ee 100644
>> >>>>>>>>>> --- a/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/ti-phy.txt
>> >>>>>>>>>> @@ -76,6 +76,10 @@ Required properties:
>> >>>>>>>>>> * "dpll_ref_m2" - external dpll ref clk
>> >>>>>>>>>> * "phy-div" - divider for apll
>> >>>>>>>>>> * "div-clk" - apll clock
>> >>>>>>>>>> + * "apll_mux" - mux for pcie apll
>> >>>>>>>>>> + * "refclk_ext" - external reference clock for pcie apll
>> >>>>>>>>>> + - ti,ext-clk: To specifiy if PCIE apll should use external
>> >>>>>>>>>> clock. Applicable
>> >>>>>>>>>> + only to PCIE PHY.
>> >>>>>>>>>
>> >>>>>>>>> Instead of specifying both clock sources "dpll_ref_clock",
>> >>>>>>>>> "refclk_ext" and then specifying a 3rd control option "ti,ext-clk"
>> >>>>>>>>> to select one of the 2 sources, why can't the DT just supply one
>> >>>>>>>>> clock source, i.e. the one that is being used in the board
>> >>>>>>>>> instance? The driver should then just configure the clock rate
>> >>>>>>>>> that is needed at that node. Shouldn't the clock framework
>> >>>>>>>>> automatically take care of muxing and parent rates?
>> >>>>>>>>
>> >>>>>>>> Want the dt to have all the clocks used by the controller.
>> >>>>>>>> "ti,ext-clk" should
>> >>>>>>>> go in the board dt file (suggested by Nishanth).
>> >>>>>>>> The point is at some point later if some one wants to change the
>> >>>>>>>> clock source,
>> >>>>>>>> it should be a simple enabling "ti,ext-clk" flag instead of finding
>> >>>>>>>> the clock
>> >>>>>>>> phandle etc..
>> >>>>>>>
>> >>>>>>> Wonder if that is implicit by the presence of "refclk_ext" in the
>> >>>>>>> clocks provided?
>> >>>>>>
>> >>>>>> IMO the presence of "refclk_ext" is useless unless the board
>> >>>>>> indicates it
>> >>>>>> provides the clock source.
>> >>>>>>
>> >>>>>> refclk_ext holds phandle for *fixed-clock*, so irrespective of
>> >>>>>> whether the
>> >>>>>> board provides a clock or not, it can have that handle for
>> >>>>>> configuring in PRCM.
>> >>>>>> However if the board does not provide the clock source, configuring
>> >>>>>> refclk_ext
>> >>>>>> in PRCM is useless.
>> >>>>>
>> >>>>> I think what Nishant meant is that if "refclk_ext" is provided it
>> >>>>> means that the driver
>> >>>>> should use that over "dpll_ref_clock" so no need of a separate
>> >>>>> "ti,ext-clk" flag.
>> >>>>
>> >>>> yes, thank you for clarifying - it does indeed redundant to have
>> >>>> "ti,ext-clk". and apologies on being a little obscure in the comment.
>> >>>
>> >>> Irrespective of whether external reference clock is used or not, all DRA7
>> >>> (apll) has an input for external reference clock (and also a PRCM
>> >>> register for
>> >>> programming it) and it has to be specified in dt no?
>> >>
>> >> Why is that a binding for ti-phy? that is a problem for the APLL clock
>> >> driver (selecting it's own source). PHY properties should describe
>> >> itself -> let the bindings of the APLL describe itself. please dont
>> >> mix the two up.
>> >
>> > The apll clock node is like this
>> >
>> > apll_pcie_in_clk_mux: apll_pcie_in_clk_mux@4ae06118 {
>> > compatible = "mux-clock";
>> > clocks = <&dpll_pcie_ref_m2ldo_ck>, <&pciesref_acs_clk_ck>;
>> > #clock-cells = <0>;
>> > reg = <0x4a00821c 0x4>;
>> > bit-mask = <0x80>;
>> > };
>> >
>> > The external reference clock is denoted by *pciesref_acs_clk_ck*.
>> >
>> > refclk_ext holds the phandle to *pciesref_acs_clk_ck* and is used in
>> > "clk_set_parent" to set the parent of apll mux.
>>
>> So, How about this: if refclk_ext is not defined, dont do setparent,
>> if it is defined, do setparent.
>> in short:
>> Optional Properties:
>> * "refclk_ext" - external reference clock for pcie apll - if defined,
>> used as the parent to "apll_mux"
>>
>> That said, my problem in general with this approach(which many folks
>> have taken of describing parent of clock X in hardware block binding
>> for Y) is the following:
>>
>> The binding now has dependency on clock tree hierarchy. What if
>> towmorrow, we have a tree where refclk_ext parent of muxZ parent of
>> apll_mux? the binding breaks down. Lets not forget that we consider DT
>> as an ABI - we dont want to change the binding in the future.
>>
>> I had always preferred describing parent-child relationship of clocks
>> by the approach Tero posted: https://patchwork.kernel.org/patch/3871551/
>>
>> Maybe Mike and Tero could help here?
>
> Hi all,
>
> I didn't read all of this context, but it seems like
> https://lkml.org/lkml/2014/5/19/539
>
> might be a good fit here. Assuming that the PIPE3 PHY consumes the APLL
> clock, then it reasonable for the dts phy node to specify
> assigned-clock-parent.
Yes indeed. Seems like the need to describe some sort of parent
description in dt makes sense for other users as well. I wonder if we
will see some alignment soonish enough for us to start using them.
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
