Hi,
On Thursday 15 May 2014 06:03 PM, Nishanth Menon wrote:
> 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"
Ok, will remove "ti,ext-clk".
Thanks
Kishon
>
> 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?
>
--
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
