On 2015/5/20 20:50, Jingoo Han wrote: > On Wed, 20 May 2015 14:21:40 +0800, Zhou Wang wrote: >> >> This patch adds PCIe host support for Hisilicon Soc Hip05. >> >> Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx> > > Hi Zhou Wang, > > I added some minor comments. > Hi Jingoo, >> --- >> drivers/pci/host/Kconfig | 5 + >> drivers/pci/host/Makefile | 1 + >> drivers/pci/host/pcie-hisi.c | 252 +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 258 insertions(+) >> create mode 100644 drivers/pci/host/pcie-hisi.c >> >> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig >> index 1dfb567..486d822 100644 >> --- a/drivers/pci/host/Kconfig >> +++ b/drivers/pci/host/Kconfig >> @@ -125,4 +125,9 @@ config PCIE_IPROC_PLATFORM >> Say Y here if you want to use the Broadcom iProc PCIe controller >> through the generic platform bus interface >> >> +config PCI_HISI >> + depends on OF && ARM64 >> + bool "Hisilicon Soc HIP05 PCIe controller" >> + select PCIE_DW >> + >> endmenu >> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile >> index f733b4e..562142e 100644 >> --- a/drivers/pci/host/Makefile >> +++ b/drivers/pci/host/Makefile >> @@ -15,3 +15,4 @@ obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o >> obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o >> obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o >> obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o >> +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o >> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c >> new file mode 100644 >> index 0000000..3f8cb9a >> --- /dev/null >> +++ b/drivers/pci/host/pcie-hisi.c >> @@ -0,0 +1,252 @@ >> +/* >> + * PCIe host controller driver for Hisilicon Hip05 SoCs >> + * >> + * Copyright (C) 2014 Hisilicon Co., Ltd. http://www.hisilicon.com > > s/2014/2015 > Thanks, forgot to change this. >> + * >> + * Author: Zhou Wang <wangzhou1@xxxxxxxxxxxxx> >> + * Dacai Zhu <zhudacai@xxxxxxxxxxxxx> > > [.....] > >> +#define PCIE_SUBCTRL_MODE_REG (0x2800) >> +#define PCIE_SUBCTRL_SYS_STATE4_REG (0x6818) >> +#define PCIE_SLV_DBI_MODE (0x0) >> +#define PCIE_SLV_SYSCTRL_MODE (0x1) >> +#define PCIE_SLV_CONTENT_MODE (0x2) >> +#define PCIE_LTSSM_LINKUP_STATE (0x11) >> +#define PCIE_LTSSM_STATE_MASK (0x3F) >> +#define PCIE_MSI_CONTEXT_VALUE (0x1011000) >> +#define PCIE_MSI_TRANS_ENABLE (0x1ff0) >> +#define PCIE_MSI_LOW_ADDRESS (0x1b4) >> +#define PCIE_MSI_HIGH_ADDRESS (0x1c4) > > Please remove unnecessary braces as below. > Ok, will do as below. > +#define PCIE_SUBCTRL_MODE_REG 0x2800 > +#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818 > ..... > > [.....] > >> +/* Configure vmid/asid table in PCIe host */ >> +static void hisi_pcie_config_context(struct hisi_pcie *pcie) >> +{ >> + int i; >> + >> + hisi_pcie_change_apb_mode(pcie, PCIE_SLV_CONTENT_MODE); >> + >> + for (i = 0; i < 0x400; i++) >> + hisi_pcie_apb_writel(pcie, 0x0, i * 4); >> + >> + for (i = 0x400; i < 0x800; i++) >> + hisi_pcie_apb_writel(pcie, 0x0, i * 4); > > How about the following? > > + for (i = 0; i < 0x800; i++) > + hisi_pcie_apb_writel(pcie, 0x0, i * 4); > This is to configure init value of vmid and asid of each pcie device. >> + >> + hisi_pcie_change_apb_mode(pcie, PCIE_SLV_SYSCTRL_MODE); >> + >> + hisi_pcie_apb_writel(pcie, 0xb7010040, PCIE_MSI_LOW_ADDRESS); >> + hisi_pcie_apb_writel(pcie, 0x0, PCIE_MSI_HIGH_ADDRESS); >> + hisi_pcie_apb_writel(pcie, PCIE_MSI_CONTEXT_VALUE, 0x10); >> + hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, 0x1c8); > > Please don't use hardcoded numbers as possible. > OK, will use macro to do this. >> + >> + hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE); >> +} > [.....] > > Best regards, > Jingoo Han > Many thanks for your review and Best regards, Zhou Wang > > > . > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
