Re: [PATCH 03/17] phy: ti-pipe3: add external clock support for PCIe PHY

[Nishanth Menon]

On Tue, May 27, 2014 at 8:54 PM, Mike Turquette <mturquette@xxxxxxxxxx> 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 <kishon@xxxxxx> 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 <rogerq@xxxxxx> 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 <kishon@xxxxxx> 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 <rnayak@xxxxxx>
>> >>>>>>>>>> Cc: Tero Kristo <t-kristo@xxxxxx>
>> >>>>>>>>>> Cc: Paul Walmsley <paul@xxxxxxxxx>
>> >>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx>
>> >>>>>>>>>> ---
>> >>>>>>>>>>  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 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/