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

[Kishon Vijay Abraham I <kishon@ti.com>]

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@ti.com>
>> Cc: Tero Kristo <t-kristo@ti.com>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@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..
> 
>>  
>>  Optional properties:
>>   - ctrl-module : phandle of the control module used by PHY driver to power on
>> diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
>> index d43019d..5513aa0 100644
>> --- a/drivers/phy/phy-ti-pipe3.c
>> +++ b/drivers/phy/phy-ti-pipe3.c
>> @@ -293,7 +293,7 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>>  	struct device_node *control_node;
>>  	struct platform_device *control_pdev;
>>  	const struct of_device_id *match;
>> -	struct clk *clk;
>> +	struct clk *clk, *pclk;
>>  
>>  	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>>  	if (!phy) {
>> @@ -302,6 +302,20 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>>  	}
>>  	phy->dev		= &pdev->dev;
>>  
>> +	control_node = of_parse_phandle(node, "ctrl-module", 0);
>> +	if (!control_node) {
>> +		dev_err(&pdev->dev, "Failed to get control device phandle\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	control_pdev = of_find_device_by_node(control_node);
>> +	if (!control_pdev) {
>> +		dev_err(&pdev->dev, "Failed to get control device\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	phy->control_dev = &control_pdev->dev;
>> +
> 
> Why this code was moved move is not part of patch description/subject.

external clock support needs 'control_dev', so had to move the get control
device part before configuring the clocks.
> 
>>  	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>>  		match = of_match_device(of_match_ptr(ti_pipe3_id_table),
>>  					&pdev->dev);
>> @@ -345,19 +359,40 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	if (of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>> -		clk = devm_clk_get(phy->dev, "dpll_ref");
>> -		if (IS_ERR(clk)) {
>> -			dev_err(&pdev->dev, "unable to get dpll ref clk\n");
>> -			return PTR_ERR(clk);
>> +		if (!of_property_read_bool(node, "ti,ext-clk")) {
>> +			clk = devm_clk_get(phy->dev, "dpll_ref");
>> +			if (IS_ERR(clk)) {
>> +				dev_err(&pdev->dev,
>> +					"unable to get dpll ref clk\n");
>> +				return PTR_ERR(clk);
>> +			}
>> +			clk_set_rate(clk, 1500000000);
>> +
>> +			clk = devm_clk_get(phy->dev, "dpll_ref_m2");
>> +			if (IS_ERR(clk)) {
>> +				dev_err(&pdev->dev,
>> +					"unable to get dpll ref m2 clk\n");
>> +				return PTR_ERR(clk);
>> +			}
>> +			clk_set_rate(clk, 100000000);
>> +		} else {
>> +			omap_control_pcie_tx_rx_control(phy->control_dev,
>> +						OMAP_CTRL_PCIE_PHY_RX_ACSPCIE);

                        ^^here

Thanks
Kishon