| Message ID | 186f7ca2-84e2-a37e-79e6-f1fec8e25374@free.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCIe and AR8151 on APQ8098/MSM8998 | expand |
On 28/03/2019 18:06, Marc Gonzalez wrote: > Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes. I suppose I should add a reference to the downstream DTS: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998.dtsi?h=LE.UM.1.3.r3.25#n2537 > 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"; Should probably be "qcom,pcie-msm8998"; in case we need to fudge something in 8998 and not in 8996. Apart from this minor issue, I think it's good to go for 5.2 (I'll respin without the SMMU patch) I just need Kishon to take the PHY series, and Stephen to take the clock hack, and we're rolling. Oh and it would be useful to have someone review the PCIE20_PARF_BDF_TRANSLATE_N patch. Regards.
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@free.fr> > --- > 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>; >
On 10/04/2019 17:32, Stanimir Varbanov wrote: > Few comments inline. I'll send v3. Changes: - Move all X-names props *after* corresponding X(s) prop - Drop comments >> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; > > iommu-map-mask? It is optional but I had to ask :) The only RID in the system is 0x100. # lspci 00:00.0 PCI bridge: Qualcomm Device 0105 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) Since we just want to map 0x100, we don't need an iommu-map-mask. >> + /* PCIe Fundamental Reset */ > > this comment is useless :) please drop it IMO, "perst" is a poor name. Can you guess what it stands for? Regards.
Hi Marc, On 4/11/19 11:44 AM, Marc Gonzalez wrote: > On 10/04/2019 17:32, Stanimir Varbanov wrote: > >> Few comments inline. > > I'll send v3. > > Changes: > - Move all X-names props *after* corresponding X(s) prop > - Drop comments > > >>> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; >> >> iommu-map-mask? It is optional but I had to ask :) > > The only RID in the system is 0x100. > > # lspci > 00:00.0 PCI bridge: Qualcomm Device 0105 > 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) > > Since we just want to map 0x100, we don't need an iommu-map-mask. Do you see warnings during boot about missing property? > > >>> + /* PCIe Fundamental Reset */ >> >> this comment is useless :) please drop it > > IMO, "perst" is a poor name. Can you guess what it stands for? The name is got from PCIE base specification. See 6.6.1. Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0"
On 11/04/2019 11:09, Stanimir Varbanov wrote: > On 4/11/19 11:44 AM, Marc Gonzalez wrote: > >> Since we just want to map 0x100, we don't need an iommu-map-mask. > > Do you see warnings during boot about missing property? Absent iommu-map-mask property is expected. No warning: https://elixir.bootlin.com/linux/latest/source/drivers/of/base.c#L2245 /* The default is to select all bits. */ map_mask = 0xffffffff; /* * Can be overridden by "{iommu,msi}-map-mask" property. * If of_property_read_u32() fails, the default is used. */ if (map_mask_name) of_property_read_u32(np, map_mask_name, &map_mask); >>>> + /* PCIe Fundamental Reset */ >>> >>> this comment is useless :) please drop it >> >> IMO, "perst" is a poor name. Can you guess what it stands for? > > The name is got from PCIE base specification. See 6.6.1. > Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0" Indeed... Well, "perst" still sucks :-p Regards.
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>; + 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 ***/ + <0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>, + /*** non-prefetchable memory ***/ + <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 */ + <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>, + <&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>; + + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; + + /* PCIe Fundamental Reset */ + 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>; + + reset-names = "phy", "common"; + resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>; + + 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>; + clock-output-names = "pcie_0_pipe_clk_src"; + #clock-cells = <0>; + }; + }; + tcsr_mutex_regs: syscon@1f40000 { compatible = "syscon"; reg = <0x1f40000 0x20000>;
Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)