Re: [PATCH] pcie: qcom: add support to msm8996 PCIE controller

[Robin Murphy]

On 07/09/16 12:06, Srinivas Kandagatla wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
> 
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks and smmu configuration are only setup after the phy is
> powered on. It also adds ltssm_enable callback as it is very much
> different to other supported SOCs in the driver.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> 
> I tested this patch along with phy driver patch on DB820c based on
> APQ8096 on port A and port B using sata and ethernet controllers.
> 
> Thanks
> srini
> 
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  88 +++++++-
>  drivers/pci/host/pcie-qcom.c                       | 237 ++++++++++++++++++++-
>  2 files changed, 318 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 4059a6f..1ddfcb4 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -92,6 +92,18 @@
>  			- "aux"		Auxiliary (AUX) clock
>  			- "bus_master"	Master AXI clock
>  			- "bus_slave"	Slave AXI clock
> +
> +- clock-names:
> +	Usage: required for msm8996/apq8096
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "aux"		Auxiliary (AUX) clock
> +			- "bus_master"	Master AXI clock
> +			- "bus_slave"	Slave AXI clock
> +			- "pipe"	Pipe Clock driving internal logic.
> +			- "cfg"		Configuration clk.
> +			- "snoc_axi"	SNOC AXI clk
> +			- "cnoc_ahb"	CNOC AHB clk
>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -114,8 +126,15 @@
>  	Definition: Should contain the following entries
>  			- "core" Core reset
>  
> +- iommus:
> +	Usage: required for msm8996/apq8096
> +	Value type: <prop-encoded-array>
> +	Definition: Should point to the respective IOMMU block with master port
> +		    as argument, see Documentation/devicetree/bindings/iommu/
> +		    mediatek,iommu.txt for details.

Qualcomm have cross-licensed the M4U!? :P

In seriousness, though, it seems a bit odd to list "iommus" as a
required property - unless the IOMMU is actually an integral part of the
PCIe root complex being described (in which case it wants its own
binding documenting as well), then the relationship is merely a
circumstance of integration of those particular SoCs.

> +
>  - power-domains:
> -	Usage: required for apq8084
> +	Usage: required for apq8084 and msm8996/apq8096
>  	Value type: <prop-encoded-array>
>  	Definition: A phandle and power domain specifier pair to the
>  		    power domain which is responsible for collapsing
> @@ -137,6 +156,11 @@
>  	Definition: A phandle to the analog power supply for IC which generates
>  		    reference clock
>  
> +- vdda_1p8-supply:
> +	Usage: required for msm8996/apq8096
> +	Value type: <phandle>
> +	Definition: A phandle to the analog power supply for PCIE_1P8
> +
>  - phys:
>  	Usage: required for apq8084
>  	Value type: <phandle>
> @@ -231,3 +255,65 @@
>  		pinctrl-0 = <&pcie0_pins_default>;
>  		pinctrl-names = "default";
>  	};
> +
> +* Example for apq8096:
> +	pcie1: qcom,pcie@00608000 {
> +		compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
> +		power-domains = <&gcc PCIE1_GDSC>;
> +		bus-range = <0x00 0xff>;
> +		num-lanes = <1>;
> +
> +		status  = "disabled";
> +
> +		reg = <0x00608000 0x2000>,
> +		      <0x0d000000 0xf1d>,
> +		      <0x0d000f20 0xa8>,
> +		      <0x0d100000 0x100000>;
> +
> +		reg-names = "parf", "dbi", "elbi","config";
> +
> +		phys = <&pcie_phy 1>;
> +		phy-names = "pciephy";
> +
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
> +			<0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
> +
> +		interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
> +		interrupt-names = "msi";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0x7>;
> +		interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +				<0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
> +		pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
> +
> +
> +		vreg-1.8-supply = <&pm8994_l12>;
> +		vreg-0.9-supply = <&pm8994_l28>;
> +		iommus = <&anoc0_smmu>;

Furthermore, the exact meaning of "iommus" is defined by the relevant
IOMMU's binding itself - at face value here, you're missing the LARB
port number :P - but more generally a bare phandle with no arguments for
something as busy as a root complex smells a bit off.

> +
> +		linux,pci-domain = <1>;
> +
> +		clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> +			<&gcc GCC_PCIE_1_AUX_CLK>,
> +			<&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +			<&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> +			<&gcc GCC_PCIE_1_SLV_AXI_CLK>,
> +			<&gcc GCC_AGGRE0_SNOC_AXI_CLK>,
> +			<&gcc GCC_AGGRE0_CNOC_AHB_CLK>;
> +
> +		clock-names =  "pipe",
> +				"aux",
> +				"cfg",
> +				"bus_master",
> +				"bus_slave",
> +				"snoc_axi",
> +				"cnoc_ahb";
> +
> +	};
> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
> index f2f90c5..7748c11 100644
> --- a/drivers/pci/host/pcie-qcom.c
> +++ b/drivers/pci/host/pcie-qcom.c
[...]
> +static int qcom_pcie_post_init_msm8996(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_msm8996 *res = &pcie->res.msm8996;
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	ret = clk_prepare_enable(res->pipe_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable pipe clock\n");
> +		return ret;
> +	}
> +	/* SMMU specific registers */
> +	writel(0, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_CFG);
> +	writel(0, pcie->parf +	PCIE20_PARF_SID_OFFSET);

Ah, so it's another of these RID->SID lookup-table-type things. That
would be more appropriately described with "iommu-map"[1], particularly
if you want things to actually work as expected[2]. There is of course a
fun circular dependency there in that the DT becomes more of a promise
than an actual description of the hardware state, since the latter
doesn't necessarily exist until the driver programs it here. It might
ultimately be cleaner to have the firmware program this and repaint the
DT accordingly, so that things do look fixed from Linux's perspective.

Robin.

[1]:http://www.mail-archive.com/iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx/msg14165.html
[2]:http://www.mail-archive.com/iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx/msg14162.html