RE: [PATCH v3] PCI: imx6: Add PHY reference clock source support

[Richard Zhu]

-----Original Message-----
From: Ilya Ledvich [mailto:ilya@xxxxxxxxxxxxxx] 
Sent: 2018å1æ10æ 21:44
To: Lucas Stach <l.stach@xxxxxxxxxxxxxx>; Richard Zhu <hongxing.zhu@xxxxxxx>
Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v3] PCI: imx6: Add PHY reference clock source support

Hi Lucas,

On 01/08/2018 12:43 PM, Lucas Stach wrote:
> Am Donnerstag, den 04.01.2018, 15:52 +0200 schrieb Ilya Ledvich:
>> i.MX7D variant of the IP can use either Crystal Oscillator input or 
>> internal clock input as a Reference Clock input for PCIe PHY.
>> Add support for an optional property 'fsl,pcie-phy-refclk-internal'.
>> If present then an internal clock input is used as PCIe PHY reference 
>> clock source. By default an external oscillator input is still used.
>>
>> Verified on Compulab SBC-iMX7 Single Board Computer.
> 
> Sorry to get in late here, but I would rather have the external clock 
> input modeled as a real clock and only use the internal clock if that 
> isn't present.
> 

I tried to follow the logic described in the iMX7 TRM, where external oscillator is a default option. Additionally, the external clock input model you've suggested, requires additional changes in the iMX7 SabreSD board (and probably other boards which use an external input too) devicetree files.
[Richard] Maybe Lucas want to have the internal PLL clock mode as the iMX PCIE default reference clock. And mark the external OSC clock input mode explicitly, and documented in the DT binding document.

> Are you even sure that the i.MX7 clock you mention isn't the already 
> documented "pcie_bus" clock? This one is also allowed to be sourced 
> externally on the i.MX6.

To the best of my understanding it's not the pcie_bus clock, but I'm absolutely sure. Could anybody from the BSP team guys elaborate on this issue? Thanks a lot!
Best regards,
[Richard] iMX PCIE is integrated on the AXI bus in the SOC internally.
The "pcie_busâ clock marked as the clock provided by the RC, and used by the EP in the iMX6 platforms.
On iMX6 platforms, the internal PLL is output from LVDS1 N/P pad, and used by EP in the HW reference designs.
                        clocks = <&clks IMX6QDL_CLK_PCIE_AXI>,
                                 <&clks IMX6QDL_CLK_LVDS1_GATE>,
                                 <&clks IMX6QDL_CLK_PCIE_REF_125M>;
                        clock-names = "pcie", "pcie_bus", "pcie_phy";

Ilya.

>> ---
>> changes since V2:
>> 	add a vendor prefix 'fsl' to a new property
>>
>>  ÂDocumentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 +++++
>>  Âdrivers/pci/dwc/pci-imx6.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 8
>> +++++++-
>>  Â2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> index 7b1e48b..1591a6a 100644
>> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> @@ -50,6 +50,11 @@ Additional required properties for imx7d-pcie:
>>  Â	ÂÂÂÂÂÂÂ- "pciephy"
>>  Â	ÂÂÂÂÂÂÂ- "apps"
>>   
>> +Additional optional properties for imx7d-pcie:
>> +- fsl,pcie-phy-refclk-internal: If present then an internal PLL
>> input is used
>> +ÂÂas PCIe PHY reference clock source. By default an external
>> oscillator input
>> +ÂÂis used.
>> +
>>  ÂExample:
>>   
>>  Â	pcie@0x01000000 {
>> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c 
>> index b734835..36812d3 100644
>> --- a/drivers/pci/dwc/pci-imx6.c
>> +++ b/drivers/pci/dwc/pci-imx6.c
>> @@ -61,6 +61,7 @@ struct imx6_pcie {
>>  Â	u32			tx_swing_low;
>>  Â	int			link_gen;
>>  Â	struct regulator	*vpcie;
>> +	bool			pciephy_refclk_sel;
>>  Â};
>>   
>>  Â/* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ 
>> @@ -474,7 +475,9 @@ static void imx6_pcie_init_phy(struct imx6_pcie
>> *imx6_pcie)
>>  Â	switch (imx6_pcie->variant) {
>>  Â	case IMX7D:
>>  Â		regmap_update_bits(imx6_pcie->iomuxc_gpr,
>> IOMUXC_GPR12,
>> -				ÂÂÂIMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
>> 0);
>> +				ÂÂÂIMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
>> +				ÂÂÂimx6_pcie->pciephy_refclk_sel ?
>> +				ÂÂÂIMX7D_GPR12_PCIE_PHY_REFCLK_SEL :
>> 0);
>>  Â		break;
>>  Â	case IMX6SX:
>>  Â		regmap_update_bits(imx6_pcie->iomuxc_gpr,
>> IOMUXC_GPR12,
>> @@ -840,6 +843,9 @@ static int imx6_pcie_probe(struct platform_device
>> *pdev)
>>  Â		imx6_pcie->vpcie = NULL;
>>  Â	}
>>   
>> +	imx6_pcie->pciephy_refclk_sel =
>> +		of_property_read_bool(node, "fsl,pcie-phy-refclk-
>> internal");
>> +
>>  Â	platform_set_drvdata(pdev, imx6_pcie);
>>   
>>  Â	ret = imx6_add_pcie_port(imx6_pcie, pdev);