| Message ID | 20170616211106.GF11129@bhelgaas-glaptop.roam.corp.google.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi, Bjorn On Sat, Jun 17, 2017 at 5:11 AM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Jun 06, 2017 at 07:19:53PM +0800, Guodong Xu wrote: >> Hi, Arnd >> >> On Tue, Jun 6, 2017 at 5:23 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Sun, Jun 4, 2017 at 2:03 AM, kbuild test robot <lkp@intel.com> wrote: >> >> Hi Xiaowei, >> >> >> >> [auto build test ERROR on pci/next] >> >> [also build test ERROR on v4.12-rc3 next-20170602] >> >> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] >> >> >> >> url: https://github.com/0day-ci/linux/commits/Xiaowei-Song/add-PCIe-driver-for-Kirin-PCIe/20170531-182118 >> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next >> >> config: arm64-allnoconfig (attached as .config) >> >> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 >> >> reproduce: >> >> wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> >> chmod +x ~/bin/make.cross >> >> # save the attached .config to linux build tree >> >> make.cross ARCH=arm64 >> >> >> >> All errors (new ones prefixed by >>): >> >> >> >>>> Error: arch/arm64/boot/dts/hisilicon/hi3660.dtsi:180.24-25 syntax error >> >>>> FATAL ERROR: Unable to parse input tree >> > >> > We keep getting the build errors for patch submissions. Obviously the patch is >> > still broken and can't be merged as-is. What is the plan for merging the series? >> > >> >> This dts patch can be applied to dts series [1]. For upstream review >> purpose, hi3660-hikey960 dts patches, which don't have a related >> driver changes, are sent in [1]. Other patches, which need driver >> changes, like this one, are sent together with driver. >> >> Patchset [1] is now at its v2 review. Rob Herring already gave his ACK >> for some of them in v1. Hopefully I can get more ACK for remaining >> ones, and make them ready for v4.13 merging window. >> >> [1], http://www.spinics.net/lists/devicetree/msg178303.html > > I don't know how you want to deal with the DTS build failure. DTS part of this is also included in a broader Hi3660 dts patchset [1], and was ACK'ed [2] today by HiSilicon SoC maintainer Xu Wei. Hopefully they can land in next merge window. [1] https://www.spinics.net/lists/arm-kernel/msg588232.html [2] https://www.spinics.net/lists/arm-kernel/msg588686.html -Guodong > From a > PCI perspective, I think I could apply patches 1 and 3 pretty easily > by themselves. > > If/when you post these again, please incorporate the following > incremental diff to clean up various whitespace and capitalization > nits (these are spread across several of your patches). > > > diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt > index 68ffa0fbcd73..20357d840af1 100644 > --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt > @@ -24,8 +24,8 @@ Example based on kirin960: > > pcie@f4000000 { > compatible = "hisilicon,kirin-pcie"; > - reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, > - <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>; > + reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, > + <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>; > reg-names = "dbi","apb","phy", "config"; > bus-range = <0x0 0x1>; > #address-cells = <3>; > @@ -46,5 +46,5 @@ Example based on kirin960: > <&crg_ctrl HI3660_ACLK_GATE_PCIE>; > clock-names = "pcie_phy_ref", "pcie_aux", > "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk"; > - reset-gpios = <&gpio11 1 0 >; > + reset-gpios = <&gpio11 1 0>; > }; > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > index e8feb2fb4d53..7bc89baa40ba 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > @@ -159,12 +159,12 @@ > > pcie@f4000000 { > compatible = "hisilicon,kirin960-pcie"; > - reg = <0x0 0xf4000000 0x0 0x1000>, > - <0x0 0xff3fe000 0x0 0x1000>, > + reg = <0x0 0xf4000000 0x0 0x1000>, > + <0x0 0xff3fe000 0x0 0x1000>, > <0x0 0xf3f20000 0x0 0x40000>, > - <0x0 0xF5000000 0x0 0x2000>; > + <0x0 0xf5000000 0x0 0x2000>; > reg-names = "dbi", "apb", "phy", "config"; > - bus-range = <0x0 0x1>; > + bus-range = <0x0 0x1>; > #address-cells = <3>; > #size-cells = <2>; > device_type = "pci"; > @@ -173,7 +173,7 @@ > num-lanes = <1>; > #interrupt-cells = <1>; > interrupt-map-mask = <0xf800 0 0 7>; > - interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>, > + interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>, > <0x0 0 0 2 &gic 0 0 0 283 4>, > <0x0 0 0 3 &gic 0 0 0 284 4>, > <0x0 0 0 4 &gic 0 0 0 285 4>; > @@ -183,8 +183,9 @@ > <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>, > <&crg_ctrl HI3660_ACLK_GATE_PCIE>; > clock-names = "pcie_phy_ref", "pcie_aux", > - "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk"; > - reset-gpios = <&gpio11 1 0 >; > + "pcie_apb_phy", "pcie_apb_sys", > + "pcie_aclk"; > + reset-gpios = <&gpio11 1 0>; > status = "ok"; > }; > }; > diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c > index f63e6548efae..41520dd1d5e5 100644 > --- a/drivers/pci/dwc/pcie-kirin.c > +++ b/drivers/pci/dwc/pcie-kirin.c > @@ -35,8 +35,8 @@ > #define REF_CLK_FREQ 100000000 > > /* PCIe ELBI registers */ > -#define SOC_PCIECTRL_CTRL0_ADDR 0x000 > -#define SOC_PCIECTRL_CTRL1_ADDR 0x004 > +#define SOC_PCIECTRL_CTRL0_ADDR 0x000 > +#define SOC_PCIECTRL_CTRL1_ADDR 0x004 > #define SOC_PCIEPHY_CTRL2_ADDR 0x008 > #define SOC_PCIEPHY_CTRL3_ADDR 0x00c > #define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21) > @@ -48,30 +48,30 @@ > #define PCIE_APB_PHY_STATUS0 0x400 > #define PCIE_LINKUP_ENABLE (0x8020) > #define PCIE_LTSSM_ENABLE_BIT (0x1 << 11) > -#define PIPE_CLK_STABLE (0x1 << 19) > +#define PIPE_CLK_STABLE (0x1 << 19) > #define PIPE_CLK_MAX_TRY_TIMES 10 > -#define PHY_REF_PAD_BIT (0x1 << 8) > +#define PHY_REF_PAD_BIT (0x1 << 8) > #define PHY_PWR_DOWN_BIT (0x1 << 22) > -#define PHY_RST_ACK_BIT (0x1 << 16) > +#define PHY_RST_ACK_BIT (0x1 << 16) > > /* info located in sysctrl */ > #define SCTRL_PCIE_CMOS_OFFSET 0x60 > #define SCTRL_PCIE_CMOS_BIT 0x10 > #define SCTRL_PCIE_ISO_OFFSET 0x44 > #define SCTRL_PCIE_ISO_BIT 0x30 > -#define SCTRL_PCIE_HPCLK_OFFSET 0x190 > +#define SCTRL_PCIE_HPCLK_OFFSET 0x190 > #define SCTRL_PCIE_HPCLK_BIT 0x184000 > #define SCTRL_PCIE_OE_OFFSET 0x14a > -#define PCIE_DEBOUNCE_PARAM 0xF0F400 > +#define PCIE_DEBOUNCE_PARAM 0xf0f400 > #define PCIE_OE_BYPASS (0x3 << 28) > > /* peri_crg ctrl */ > #define CRGCTRL_PCIE_ASSERT_OFFSET 0x88 > -#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 > +#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 > > /* Time for delay */ > -#define REF_2_PERST_MIN 20000 > -#define REF_2_PERST_MAX 25000 > +#define REF_2_PERST_MIN 20000 > +#define REF_2_PERST_MAX 25000 > #define PERST_2_ACCESS_MIN 10000 > #define PERST_2_ACCESS_MAX 12000 > #define LINK_WAIT_MIN 900 > @@ -80,22 +80,22 @@ > #define PIPE_CLK_WAIT_MAX 600 > > struct kirin_pcie { > - struct dw_pcie *pci; > - void __iomem *apb_base; > - void __iomem *phy_base; > - struct regmap *crgctrl; > - struct regmap *sysctrl; > - struct clk *apb_sys_clk; > - struct clk *apb_phy_clk; > - struct clk *phy_ref_clk; > - struct clk *pcie_aclk; > - struct clk *pcie_aux_clk; > - int gpio_id_reset; > + struct dw_pcie *pci; > + void __iomem *apb_base; > + void __iomem *phy_base; > + struct regmap *crgctrl; > + struct regmap *sysctrl; > + struct clk *apb_sys_clk; > + struct clk *apb_phy_clk; > + struct clk *phy_ref_clk; > + struct clk *pcie_aclk; > + struct clk *pcie_aux_clk; > + int gpio_id_reset; > }; > > /* Registers in PCIeCTRL */ > static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie, > - u32 val, u32 reg) > + u32 val, u32 reg) > { > writel(val, kirin_pcie->apb_base + reg); > } > @@ -107,7 +107,7 @@ static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg) > > /* Registers in PCIePHY */ > static inline void kirin_apb_phy_writel(struct kirin_pcie *kirin_pcie, > - u32 val, u32 reg) > + u32 val, u32 reg) > { > writel(val, kirin_pcie->phy_base + reg); > } > @@ -118,7 +118,7 @@ static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg) > } > > static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie, > - struct platform_device *pdev) > + struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > > @@ -146,7 +146,7 @@ static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie, > } > > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > - struct platform_device *pdev) > + struct platform_device *pdev) > { > struct resource *apb; > struct resource *phy; > @@ -184,7 +184,6 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie) > { > u32 reg_val; > - u32 time = PIPE_CLK_MAX_TRY_TIMES; > > reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1); > reg_val &= ~PHY_REF_PAD_BIT; > @@ -459,7 +458,7 @@ static int kirin_pcie_probe(struct platform_device *pdev) > int ret; > > if (!dev->of_node) { > - dev_err(&pdev->dev, "NULL node\n"); > + dev_err(dev, "NULL node\n"); > return -EINVAL; > } >
On Sat, Jun 17, 2017 at 06:31:59AM +0800, Guodong Xu wrote: > Hi, Bjorn > > On Sat, Jun 17, 2017 at 5:11 AM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 06, 2017 at 07:19:53PM +0800, Guodong Xu wrote: > >> Hi, Arnd > >> > >> On Tue, Jun 6, 2017 at 5:23 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Sun, Jun 4, 2017 at 2:03 AM, kbuild test robot <lkp@intel.com> wrote: > >> >> Hi Xiaowei, > >> >> > >> >> [auto build test ERROR on pci/next] > >> >> [also build test ERROR on v4.12-rc3 next-20170602] > >> >> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > >> >> > >> >> url: https://github.com/0day-ci/linux/commits/Xiaowei-Song/add-PCIe-driver-for-Kirin-PCIe/20170531-182118 > >> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next > >> >> config: arm64-allnoconfig (attached as .config) > >> >> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 > >> >> reproduce: > >> >> wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > >> >> chmod +x ~/bin/make.cross > >> >> # save the attached .config to linux build tree > >> >> make.cross ARCH=arm64 > >> >> > >> >> All errors (new ones prefixed by >>): > >> >> > >> >>>> Error: arch/arm64/boot/dts/hisilicon/hi3660.dtsi:180.24-25 syntax error > >> >>>> FATAL ERROR: Unable to parse input tree > >> > > >> > We keep getting the build errors for patch submissions. Obviously the patch is > >> > still broken and can't be merged as-is. What is the plan for merging the series? > >> > > >> > >> This dts patch can be applied to dts series [1]. For upstream review > >> purpose, hi3660-hikey960 dts patches, which don't have a related > >> driver changes, are sent in [1]. Other patches, which need driver > >> changes, like this one, are sent together with driver. > >> > >> Patchset [1] is now at its v2 review. Rob Herring already gave his ACK > >> for some of them in v1. Hopefully I can get more ACK for remaining > >> ones, and make them ready for v4.13 merging window. > >> > >> [1], http://www.spinics.net/lists/devicetree/msg178303.html > > > > I don't know how you want to deal with the DTS build failure. > > DTS part of this is also included in a broader Hi3660 dts patchset [1], and > was ACK'ed [2] today by HiSilicon SoC maintainer Xu Wei. Hopefully > they can land in next merge window. > > [1] https://www.spinics.net/lists/arm-kernel/msg588232.html > [2] https://www.spinics.net/lists/arm-kernel/msg588686.html This sounds good, but doesn't help me make progress. I don't want to apply [PATCH v9 2/4] because it didn't build. I haven't seen an updated series that *does* build. And it probably doesn't make sense for me to apply the arch/arm64 changes anyway because they aren't really in the PCI purview. If you want me to apply something, post patches 1 and 3 by themselves with the trival updates I included. Those are really only PCI and should build without error. > > From a > > PCI perspective, I think I could apply patches 1 and 3 pretty easily > > by themselves. > > > > If/when you post these again, please incorporate the following > > incremental diff to clean up various whitespace and capitalization > > nits (these are spread across several of your patches). > > > > > > diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt > > index 68ffa0fbcd73..20357d840af1 100644 > > --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt > > @@ -24,8 +24,8 @@ Example based on kirin960: > > > > pcie@f4000000 { > > compatible = "hisilicon,kirin-pcie"; > > - reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, > > - <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>; > > + reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, > > + <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>; > > reg-names = "dbi","apb","phy", "config"; > > bus-range = <0x0 0x1>; > > #address-cells = <3>; > > @@ -46,5 +46,5 @@ Example based on kirin960: > > <&crg_ctrl HI3660_ACLK_GATE_PCIE>; > > clock-names = "pcie_phy_ref", "pcie_aux", > > "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk"; > > - reset-gpios = <&gpio11 1 0 >; > > + reset-gpios = <&gpio11 1 0>; > > }; > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > index e8feb2fb4d53..7bc89baa40ba 100644 > > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > @@ -159,12 +159,12 @@ > > > > pcie@f4000000 { > > compatible = "hisilicon,kirin960-pcie"; > > - reg = <0x0 0xf4000000 0x0 0x1000>, > > - <0x0 0xff3fe000 0x0 0x1000>, > > + reg = <0x0 0xf4000000 0x0 0x1000>, > > + <0x0 0xff3fe000 0x0 0x1000>, > > <0x0 0xf3f20000 0x0 0x40000>, > > - <0x0 0xF5000000 0x0 0x2000>; > > + <0x0 0xf5000000 0x0 0x2000>; > > reg-names = "dbi", "apb", "phy", "config"; > > - bus-range = <0x0 0x1>; > > + bus-range = <0x0 0x1>; > > #address-cells = <3>; > > #size-cells = <2>; > > device_type = "pci"; > > @@ -173,7 +173,7 @@ > > num-lanes = <1>; > > #interrupt-cells = <1>; > > interrupt-map-mask = <0xf800 0 0 7>; > > - interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>, > > + interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>, > > <0x0 0 0 2 &gic 0 0 0 283 4>, > > <0x0 0 0 3 &gic 0 0 0 284 4>, > > <0x0 0 0 4 &gic 0 0 0 285 4>; > > @@ -183,8 +183,9 @@ > > <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>, > > <&crg_ctrl HI3660_ACLK_GATE_PCIE>; > > clock-names = "pcie_phy_ref", "pcie_aux", > > - "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk"; > > - reset-gpios = <&gpio11 1 0 >; > > + "pcie_apb_phy", "pcie_apb_sys", > > + "pcie_aclk"; > > + reset-gpios = <&gpio11 1 0>; > > status = "ok"; > > }; > > }; > > diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c > > index f63e6548efae..41520dd1d5e5 100644 > > --- a/drivers/pci/dwc/pcie-kirin.c > > +++ b/drivers/pci/dwc/pcie-kirin.c > > @@ -35,8 +35,8 @@ > > #define REF_CLK_FREQ 100000000 > > > > /* PCIe ELBI registers */ > > -#define SOC_PCIECTRL_CTRL0_ADDR 0x000 > > -#define SOC_PCIECTRL_CTRL1_ADDR 0x004 > > +#define SOC_PCIECTRL_CTRL0_ADDR 0x000 > > +#define SOC_PCIECTRL_CTRL1_ADDR 0x004 > > #define SOC_PCIEPHY_CTRL2_ADDR 0x008 > > #define SOC_PCIEPHY_CTRL3_ADDR 0x00c > > #define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21) > > @@ -48,30 +48,30 @@ > > #define PCIE_APB_PHY_STATUS0 0x400 > > #define PCIE_LINKUP_ENABLE (0x8020) > > #define PCIE_LTSSM_ENABLE_BIT (0x1 << 11) > > -#define PIPE_CLK_STABLE (0x1 << 19) > > +#define PIPE_CLK_STABLE (0x1 << 19) > > #define PIPE_CLK_MAX_TRY_TIMES 10 > > -#define PHY_REF_PAD_BIT (0x1 << 8) > > +#define PHY_REF_PAD_BIT (0x1 << 8) > > #define PHY_PWR_DOWN_BIT (0x1 << 22) > > -#define PHY_RST_ACK_BIT (0x1 << 16) > > +#define PHY_RST_ACK_BIT (0x1 << 16) > > > > /* info located in sysctrl */ > > #define SCTRL_PCIE_CMOS_OFFSET 0x60 > > #define SCTRL_PCIE_CMOS_BIT 0x10 > > #define SCTRL_PCIE_ISO_OFFSET 0x44 > > #define SCTRL_PCIE_ISO_BIT 0x30 > > -#define SCTRL_PCIE_HPCLK_OFFSET 0x190 > > +#define SCTRL_PCIE_HPCLK_OFFSET 0x190 > > #define SCTRL_PCIE_HPCLK_BIT 0x184000 > > #define SCTRL_PCIE_OE_OFFSET 0x14a > > -#define PCIE_DEBOUNCE_PARAM 0xF0F400 > > +#define PCIE_DEBOUNCE_PARAM 0xf0f400 > > #define PCIE_OE_BYPASS (0x3 << 28) > > > > /* peri_crg ctrl */ > > #define CRGCTRL_PCIE_ASSERT_OFFSET 0x88 > > -#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 > > +#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 > > > > /* Time for delay */ > > -#define REF_2_PERST_MIN 20000 > > -#define REF_2_PERST_MAX 25000 > > +#define REF_2_PERST_MIN 20000 > > +#define REF_2_PERST_MAX 25000 > > #define PERST_2_ACCESS_MIN 10000 > > #define PERST_2_ACCESS_MAX 12000 > > #define LINK_WAIT_MIN 900 > > @@ -80,22 +80,22 @@ > > #define PIPE_CLK_WAIT_MAX 600 > > > > struct kirin_pcie { > > - struct dw_pcie *pci; > > - void __iomem *apb_base; > > - void __iomem *phy_base; > > - struct regmap *crgctrl; > > - struct regmap *sysctrl; > > - struct clk *apb_sys_clk; > > - struct clk *apb_phy_clk; > > - struct clk *phy_ref_clk; > > - struct clk *pcie_aclk; > > - struct clk *pcie_aux_clk; > > - int gpio_id_reset; > > + struct dw_pcie *pci; > > + void __iomem *apb_base; > > + void __iomem *phy_base; > > + struct regmap *crgctrl; > > + struct regmap *sysctrl; > > + struct clk *apb_sys_clk; > > + struct clk *apb_phy_clk; > > + struct clk *phy_ref_clk; > > + struct clk *pcie_aclk; > > + struct clk *pcie_aux_clk; > > + int gpio_id_reset; > > }; > > > > /* Registers in PCIeCTRL */ > > static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie, > > - u32 val, u32 reg) > > + u32 val, u32 reg) > > { > > writel(val, kirin_pcie->apb_base + reg); > > } > > @@ -107,7 +107,7 @@ static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg) > > > > /* Registers in PCIePHY */ > > static inline void kirin_apb_phy_writel(struct kirin_pcie *kirin_pcie, > > - u32 val, u32 reg) > > + u32 val, u32 reg) > > { > > writel(val, kirin_pcie->phy_base + reg); > > } > > @@ -118,7 +118,7 @@ static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg) > > } > > > > static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie, > > - struct platform_device *pdev) > > + struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > > > @@ -146,7 +146,7 @@ static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie, > > } > > > > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > > - struct platform_device *pdev) > > + struct platform_device *pdev) > > { > > struct resource *apb; > > struct resource *phy; > > @@ -184,7 +184,6 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > > static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie) > > { > > u32 reg_val; > > - u32 time = PIPE_CLK_MAX_TRY_TIMES; > > > > reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1); > > reg_val &= ~PHY_REF_PAD_BIT; > > @@ -459,7 +458,7 @@ static int kirin_pcie_probe(struct platform_device *pdev) > > int ret; > > > > if (!dev->of_node) { > > - dev_err(&pdev->dev, "NULL node\n"); > > + dev_err(dev, "NULL node\n"); > > return -EINVAL; > > } > >
On Sat, Jun 17, 2017 at 08:33:08AM +0000, songxiaowei wrote: > Hi Bjorn, > > There are serval comments I can not understand, please help me to give more details. Sure. BTW, for some reason your response is all double-spaced, which makes it a little hard to read. Also, it includes HTML and an image, which means the mailing list probably rejected it: http://vger.kernel.org/majordomo-info.html#taboo > diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt > > index 68ffa0fbcd73..20357d840af1 100644 > > --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt > > @@ -24,8 +24,8 @@ Example based on kirin960: > > pcie@f4000000 { > > compatible = "hisilicon,kirin-pcie"; > > - reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, > > - <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>; > > + reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, > > + <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>; > > [songxiaowei] The difference is add one more space between "0x0" and "0x1000" in the first element. > > But, I can't get your mind. It changes from this: reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>; to this: reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>; The extra space makes the elements align vertically. Columns of numbers are conventionally right-aligned, which makes it easier to compare their sizes. This hunk also changed 0xF4000000 to 0xf4000000 in the second line, so hex constants use lower-case consistently. > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > @@ -159,12 +159,12 @@ > > pcie@f4000000 { > > compatible = "hisilicon,kirin960-pcie"; > > - reg = <0x0 0xf4000000 0x0 0x1000>, > > - <0x0 0xff3fe000 0x0 0x1000>, > > + reg = <0x0 0xf4000000 0x0 0x1000>, > > + <0x0 0xff3fe000 0x0 0x1000>, > > [songxiaowei] Can your tell why using two spaces? Reg is listed as <addr_hi addr_lo size_hi size_lo>. Again, just to make the sizes right-aligned so it's easier to compare the sizes. You can ignore this one if you want. There are lots of examples in the tree that don't align these. I just think it looks sloppy when things are almost but not quite aligned. > - <0x0 0xF5000000 0x0 0x2000>; > + <0x0 0xf5000000 0x0 0x2000>; Another case of making hex constants consistently lower-case. > diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c index f63e6548efae..41520dd1d5e5 100644 > > --- a/drivers/pci/dwc/pcie-kirin.c > > +++ b/drivers/pci/dwc/pcie-kirin.c > > @@ -35,8 +35,8 @@ > > #define REF_CLK_FREQ 100000000 > > /* PCIe ELBI registers */ > > -#define SOC_PCIECTRL_CTRL0_ADDR 0x000 > > -#define SOC_PCIECTRL_CTRL1_ADDR 0x004 > > +#define SOC_PCIECTRL_CTRL0_ADDR 0x000 > > +#define SOC_PCIECTRL_CTRL1_ADDR 0x004 > > #define SOC_PCIEPHY_CTRL2_ADDR 0x008 > > #define SOC_PCIEPHY_CTRL3_ADDR 0x00c > ... > [songxiaowei] The space of the name of these macro definition and > > the value are really different from 1 to 3 Tab, but it seem likes as bellow opened by vim. > > [cid:image001.png@01D2E786.14883370] The image shows this: +#define REF_CLK_FREQ 100000000 + +/* PCIe ELBI registers */ +#define SOC_PCIECTRL_CTRL0_ADDR 0x000 +#define SOC_PCIECTRL_CTRL1_ADDR 0x004 +#define SOC_PCIEPHY_CTRL2_ADDR 0x008 +#define SOC_PCIEPHY_CTRL3_ADDR 0x00c So things are nicely aligned in the *patch*. But we don't care about alignment in the patch. What we want is alignment in the *file*, which ends up looking like this with your original patch: #define REF_CLK_FREQ 100000000 /* PCIe ELBI registers */ #define SOC_PCIECTRL_CTRL0_ADDR 0x000 #define SOC_PCIECTRL_CTRL1_ADDR 0x004 #define SOC_PCIEPHY_CTRL2_ADDR 0x008 #define SOC_PCIEPHY_CTRL3_ADDR 0x00c My incremental diff probably makes the patch look ugly, but the resulting file looks nicer. > struct kirin_pcie { > > - struct dw_pcie *pci; > > - void __iomem *apb_base; > ... > + struct dw_pcie *pci; > > + void __iomem *apb_base; > ... > [songxiaowei] it seems the variables list in the same coloumn with vim. Yes, these variables were aligned in your original patch; it's just that they used two or three tabs, when one or two was sufficient. So there's extra separation. Not a big deal. Bjorn
diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt index 68ffa0fbcd73..20357d840af1 100644 --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt @@ -24,8 +24,8 @@ Example based on kirin960: pcie@f4000000 { compatible = "hisilicon,kirin-pcie"; - reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, - <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>; + reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>, + <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xf4000000 0x0 0x2000>; reg-names = "dbi","apb","phy", "config"; bus-range = <0x0 0x1>; #address-cells = <3>; @@ -46,5 +46,5 @@ Example based on kirin960: <&crg_ctrl HI3660_ACLK_GATE_PCIE>; clock-names = "pcie_phy_ref", "pcie_aux", "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk"; - reset-gpios = <&gpio11 1 0 >; + reset-gpios = <&gpio11 1 0>; }; diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi index e8feb2fb4d53..7bc89baa40ba 100644 --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi @@ -159,12 +159,12 @@ pcie@f4000000 { compatible = "hisilicon,kirin960-pcie"; - reg = <0x0 0xf4000000 0x0 0x1000>, - <0x0 0xff3fe000 0x0 0x1000>, + reg = <0x0 0xf4000000 0x0 0x1000>, + <0x0 0xff3fe000 0x0 0x1000>, <0x0 0xf3f20000 0x0 0x40000>, - <0x0 0xF5000000 0x0 0x2000>; + <0x0 0xf5000000 0x0 0x2000>; reg-names = "dbi", "apb", "phy", "config"; - bus-range = <0x0 0x1>; + bus-range = <0x0 0x1>; #address-cells = <3>; #size-cells = <2>; device_type = "pci"; @@ -173,7 +173,7 @@ num-lanes = <1>; #interrupt-cells = <1>; interrupt-map-mask = <0xf800 0 0 7>; - interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>, + interrupt-map = <0x0 0 0 1 &gic 0 0 0 282 4>, <0x0 0 0 2 &gic 0 0 0 283 4>, <0x0 0 0 3 &gic 0 0 0 284 4>, <0x0 0 0 4 &gic 0 0 0 285 4>; @@ -183,8 +183,9 @@ <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>, <&crg_ctrl HI3660_ACLK_GATE_PCIE>; clock-names = "pcie_phy_ref", "pcie_aux", - "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk"; - reset-gpios = <&gpio11 1 0 >; + "pcie_apb_phy", "pcie_apb_sys", + "pcie_aclk"; + reset-gpios = <&gpio11 1 0>; status = "ok"; }; }; diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c index f63e6548efae..41520dd1d5e5 100644 --- a/drivers/pci/dwc/pcie-kirin.c +++ b/drivers/pci/dwc/pcie-kirin.c @@ -35,8 +35,8 @@ #define REF_CLK_FREQ 100000000 /* PCIe ELBI registers */ -#define SOC_PCIECTRL_CTRL0_ADDR 0x000 -#define SOC_PCIECTRL_CTRL1_ADDR 0x004 +#define SOC_PCIECTRL_CTRL0_ADDR 0x000 +#define SOC_PCIECTRL_CTRL1_ADDR 0x004 #define SOC_PCIEPHY_CTRL2_ADDR 0x008 #define SOC_PCIEPHY_CTRL3_ADDR 0x00c #define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21) @@ -48,30 +48,30 @@ #define PCIE_APB_PHY_STATUS0 0x400 #define PCIE_LINKUP_ENABLE (0x8020) #define PCIE_LTSSM_ENABLE_BIT (0x1 << 11) -#define PIPE_CLK_STABLE (0x1 << 19) +#define PIPE_CLK_STABLE (0x1 << 19) #define PIPE_CLK_MAX_TRY_TIMES 10 -#define PHY_REF_PAD_BIT (0x1 << 8) +#define PHY_REF_PAD_BIT (0x1 << 8) #define PHY_PWR_DOWN_BIT (0x1 << 22) -#define PHY_RST_ACK_BIT (0x1 << 16) +#define PHY_RST_ACK_BIT (0x1 << 16) /* info located in sysctrl */ #define SCTRL_PCIE_CMOS_OFFSET 0x60 #define SCTRL_PCIE_CMOS_BIT 0x10 #define SCTRL_PCIE_ISO_OFFSET 0x44 #define SCTRL_PCIE_ISO_BIT 0x30 -#define SCTRL_PCIE_HPCLK_OFFSET 0x190 +#define SCTRL_PCIE_HPCLK_OFFSET 0x190 #define SCTRL_PCIE_HPCLK_BIT 0x184000 #define SCTRL_PCIE_OE_OFFSET 0x14a -#define PCIE_DEBOUNCE_PARAM 0xF0F400 +#define PCIE_DEBOUNCE_PARAM 0xf0f400 #define PCIE_OE_BYPASS (0x3 << 28) /* peri_crg ctrl */ #define CRGCTRL_PCIE_ASSERT_OFFSET 0x88 -#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 +#define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 /* Time for delay */ -#define REF_2_PERST_MIN 20000 -#define REF_2_PERST_MAX 25000 +#define REF_2_PERST_MIN 20000 +#define REF_2_PERST_MAX 25000 #define PERST_2_ACCESS_MIN 10000 #define PERST_2_ACCESS_MAX 12000 #define LINK_WAIT_MIN 900 @@ -80,22 +80,22 @@ #define PIPE_CLK_WAIT_MAX 600 struct kirin_pcie { - struct dw_pcie *pci; - void __iomem *apb_base; - void __iomem *phy_base; - struct regmap *crgctrl; - struct regmap *sysctrl; - struct clk *apb_sys_clk; - struct clk *apb_phy_clk; - struct clk *phy_ref_clk; - struct clk *pcie_aclk; - struct clk *pcie_aux_clk; - int gpio_id_reset; + struct dw_pcie *pci; + void __iomem *apb_base; + void __iomem *phy_base; + struct regmap *crgctrl; + struct regmap *sysctrl; + struct clk *apb_sys_clk; + struct clk *apb_phy_clk; + struct clk *phy_ref_clk; + struct clk *pcie_aclk; + struct clk *pcie_aux_clk; + int gpio_id_reset; }; /* Registers in PCIeCTRL */ static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie, - u32 val, u32 reg) + u32 val, u32 reg) { writel(val, kirin_pcie->apb_base + reg); } @@ -107,7 +107,7 @@ static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg) /* Registers in PCIePHY */ static inline void kirin_apb_phy_writel(struct kirin_pcie *kirin_pcie, - u32 val, u32 reg) + u32 val, u32 reg) { writel(val, kirin_pcie->phy_base + reg); } @@ -118,7 +118,7 @@ static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg) } static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie, - struct platform_device *pdev) + struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -146,7 +146,7 @@ static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie, } static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, - struct platform_device *pdev) + struct platform_device *pdev) { struct resource *apb; struct resource *phy; @@ -184,7 +184,6 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie) { u32 reg_val; - u32 time = PIPE_CLK_MAX_TRY_TIMES; reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1); reg_val &= ~PHY_REF_PAD_BIT; @@ -459,7 +458,7 @@ static int kirin_pcie_probe(struct platform_device *pdev) int ret; if (!dev->of_node) { - dev_err(&pdev->dev, "NULL node\n"); + dev_err(dev, "NULL node\n"); return -EINVAL; }