Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes

[Stanimir Varbanov via iommu <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>]

Hi Marc,

Few comments inline.

On 3/28/19 7:06 PM, Marc Gonzalez wrote:
> Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes.
> 
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 5a1c0961b281..9f979a51f679 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -621,6 +621,84 @@
>  				<GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
>  		};
>  
> +		pcie0: pci@1c00000 {
> +			compatible = "qcom,pcie-msm8996";
> +			reg-names = "parf", "dbi", "elbi", "config";
> +			reg =	<0x01c00000 0x2000>,
> +				<0x1b000000 0xf1d>,
> +				<0x1b000f20 0xa8>,
> +				<0x1b100000 0x100000>;

could you please populate reg property and after that reg-names property.

> +			device_type = "pci";
> +			linux,pci-domain = <0>;
> +			bus-range = <0x00 0xff>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			power-domains = <&gcc PCIE_0_GDSC>;
> +
> +			num-lanes = <1>;
> +			phy-names = "pciephy";
> +			phys = <&pciephy>;
> +
> +			ranges =
> +			/*** downstream I/O ***/

could you make this a standard kernel comment or completely drop it

> +			<0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>,
> +			/*** non-prefetchable memory ***/

ditto

> +			<0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>;
> +
> +			#interrupt-cells = <1>;
> +			interrupt-names = "msi";
> +			interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map =
> +				<0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */

move this to a line upper

> +				<0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +			clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux";
> +			clocks =
> +				<&gcc GCC_PCIE_0_PIPE_CLK>,

move this to a line upper

> +				<&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
> +				<&gcc GCC_PCIE_0_SLV_AXI_CLK>,
> +				<&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> +				<&gcc GCC_PCIE_0_AUX_CLK>;

please swap the order clocks and clock-names

> +
> +			iommu-map = <0x100 &anoc1_smmu 0x1480 1>;

iommu-map-mask? It is optional but I had to ask :)

> +
> +			/* PCIe Fundamental Reset */

this comment is useless :) please drop it

> +			perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		phy@1c06000 {
> +			compatible = "qcom,msm8998-qmp-pcie-phy";
> +			reg = <0x01c06000 0x18c>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			clock-names = "aux", "cfg_ahb", "ref";
> +			clocks =
> +				<&gcc GCC_PCIE_PHY_AUX_CLK>,
> +				<&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> +				<&gcc GCC_PCIE_CLKREF_CLK>;\

please, swap the order of clocks and clock-names, and move first clock a
line upper. Also delete '\' symbol at the end of last line.

> +
> +			reset-names = "phy", "common";
> +			resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>;

resets prop and after that reset-names, please.

> +
> +			vdda-phy-supply = <&vreg_l1a_0p875>;
> +			vdda-pll-supply = <&vreg_l2a_1p2>;
> +
> +			pciephy: lane@1c06800 {
> +				reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>;
> +				#phy-cells = <0>;
> +
> +				clock-names = "pipe0";
> +				clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;

please, swap clocks and clock-names

> +				clock-output-names = "pcie_0_pipe_clk_src";
> +				#clock-cells = <0>;
> +			};
> +		};
> +
>  		tcsr_mutex_regs: syscon@1f40000 {
>  			compatible = "syscon";
>  			reg = <0x1f40000 0x20000>;
> 

-- 
regards,
Stan