Hi Lucas: Thanks for your review comments. > -----Original Message----- > From: Lucas Stach [mailto:l.stach@xxxxxxxxxxxxxx] > Sent: Monday, September 29, 2014 6:13 PM > To: Zhu Richard-R65037 > Cc: linux-pci-owner@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Guo Shawn- > R65073; festevam@xxxxxxxxx; tharvey@xxxxxxxxxxxxx > Subject: Re: [PATCH v3 3/9] PCI: imx6: update dts and binding for imx6sx pcie > > Am Montag, den 29.09.2014, 13:03 +0800 schrieb Richard Zhu: > > - imx6sx pcie phy has its own power regulator. Add the pcie phy power > > suppy into im6sx pcie dts and binding. > > - in order to align with imx6qdl's pcie dts, re-format imx6sx pcie > > dts. > > - in order to align with imx6qdl pcie dts format and keep clean of > > imx6 pcie driver, keep the pcie phy clock in imx6sx pcie dts, although > > it's the parent clk of the pcie bus clock now, and would be enabled > > automatically when pcie bus clock is enabled. secondly, it's possible > > that the external osc maybe used as source of the pcie_bus clk in > > board design in future. > > - disp_axi clock is required by pcie inbound axi port. > > Add one more clock for imx6sx pcie. > > > > Signed-off-by: Richard Zhu <r65037@xxxxxxxxxxxxx> > > --- > > .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 +++- > > arch/arm/boot/dts/imx6sx.dtsi | 30 ++++++++++++------- > --- > > 2 files changed, 21 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > index 9455fd0..981e41d 100644 > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > @@ -4,7 +4,7 @@ This PCIe host controller is based on the Synopsis > > Designware PCIe IP and thus inherits all the common properties defined in > designware-pcie.txt. > > > > Required properties: > > -- compatible: "fsl,imx6q-pcie" > > +- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie" > > - reg: base addresse and length of the pcie controller > > - interrupts: A list of interrupt outputs of the controller. Must contain > an > > entry for each entry in the interrupt-names property. > > @@ -13,6 +13,9 @@ Required properties: > > - clock-names: Must include the following additional entries: > > - "pcie_phy" > > > > +Power supplies for imx6sx: > > +- pcie_phy-supply: regulator used by imx6sx pcie phy. > > + > To align with the previous practice of naming regulator supplies please don't > use underscores in the name. Also you aren't documenting the additional clock > (which also needs a descriptive name, NOT "disp_axi"). > I would recommend the documentation change to look like this: > > Additional required properties for imx6sx-pcie: > - clock names: Must include the following additional entries: > - "pcie_inbound_axi" > - power supplies: > - pcie-phy-supply: regulator used to power the PCIe PHY > > Please update the DTS and driver accordingly. > [Richard] Accepted. pcie_inbound_axi is ok. > > Example: > > > > pcie@0x01000000 { > > diff --git a/arch/arm/boot/dts/imx6sx.dtsi > > b/arch/arm/boot/dts/imx6sx.dtsi index f4b9da6..b4ca94b 100644 > > --- a/arch/arm/boot/dts/imx6sx.dtsi > > +++ b/arch/arm/boot/dts/imx6sx.dtsi > > @@ -599,9 +599,9 @@ > > anatop-max-voltage = <1450000>; > > }; > > > > - reg_pcie: regulator-vddpcie@140 { > > + reg_pcie_phy: regulator-vddpcie_phy@140 { > > compatible = "fsl,anatop-regulator"; > > - regulator-name = "vddpcie"; > > + regulator-name = "vddpcie_phy"; > > regulator-min-microvolt = <725000>; > > regulator-max-microvolt = <1450000>; > > anatop-reg-offset = <0x140>; > > @@ -1188,20 +1188,24 @@ > > #address-cells = <3>; > > #size-cells = <2>; > > device_type = "pci"; > > - /* configuration space */ > > - ranges = <0x00000800 0 0x08f00000 0x08f00000 0 0x00080000 > > - /* downstream I/O */ > > - 0x81000000 0 0 0x08f80000 0 0x00010000 > > - /* non-prefetchable memory */ > > - 0x82000000 0 0x08000000 0x08000000 0 0x00f00000>; > > + ranges = <0x00000800 0 0x08f00000 0x08f00000 0 0x00080000 /* > configuration space */ > > + 0x81000000 0 0 0x08f80000 0 0x00010000 /* > downstream I/O */ > > + 0x82000000 0 0x08000000 0x08000000 0 0x00f00000>; /* > > +non-prefetchable memory */ > > num-lanes = <1>; > > - interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>; > > - clocks = <&clks IMX6SX_CLK_PCIE_REF_125M>, > > - <&clks IMX6SX_CLK_PCIE_AXI>, > > + interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "msi"; > > + #interrupt-cells = <1>; > > + interrupt-map-mask = <0 0 0 0x7>; > > + interrupt-map = <0 0 0 1 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, > > + <0 0 0 2 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>, > > + <0 0 0 3 &intc GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>, > > + <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clks IMX6SX_CLK_PCIE_AXI>, > > <&clks IMX6SX_CLK_LVDS1_OUT>, > > + <&clks IMX6SX_CLK_PCIE_REF_125M>, > > <&clks IMX6SX_CLK_DISPLAY_AXI>; > > - clock-names = "pcie_ref_125m", "pcie_axi", > > - "lvds_gate", "display_axi"; > > + clock-names = "pcie", "pcie_bus", "pcie_phy", "disp_axi"; > > + pcie_phy-supply = <®_pcie_phy>; > > status = "disabled"; > > }; > > }; > > You are still missing the "config" reg space. Please look at the designware- > pcie.txt base binding, it's a required property now. Also please be more > careful to address my review comments. > [Richard]Sorry, mis-understand your previous comment. Catch your point now, would add the new "config" reg space later. > Regards, > Lucas > -- > Pengutronix e.K. | Lucas Stach | > Industrial Linux Solutions | http://www.pengutronix.de/ | Best Regards Richard Zhu ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
