PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05
Quoted 4 lines · Zhou Wang
This patch adds PCIe host support for Hisilicon Soc Hip05.
Signed-off-by Zhou Wang <[email protected]>
Hi Zhou Wang,
I added some minor comments.
Quoted 36 lines
--- 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.cdrivers/pci/host/Kconfig +5
-0@@ -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 + endmenudrivers/pci/host/Makefile +1
-0@@ -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.odrivers/pci/host/pcie-hisi.c new file +0
-0
drivers/pci/host/pcie-hisi.c line 4
Quoted 4 lines
drivers/pci/host/pcie-hisi.c new file +4
-0@@ -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
Quoted 3 lines
drivers/pci/host/pcie-hisi.c new file +3
-0+* +* Author: Zhou Wang <[email protected]> +* Dacai Zhu <[email protected]>
[.....]
drivers/pci/host/pcie-hisi.c
Quoted 11 lines
drivers/pci/host/pcie-hisi.c new file +11
-0+#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.
+#define PCIE_SUBCTRL_MODE_REG 0x2800
+#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818
.....
[.....]
drivers/pci/host/pcie-hisi.c
Quoted 12 lines
drivers/pci/host/pcie-hisi.c new file +12
-0+/* 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);
drivers/pci/host/pcie-hisi.c
Quoted 7 lines
drivers/pci/host/pcie-hisi.c new file +7
-0+ +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.
drivers/pci/host/pcie-hisi.c
Quoted 3 lines
drivers/pci/host/pcie-hisi.c new file +3
-0+ +hisi_pcie_change_apb_mode (pcie,PCIE_SLV_DBI_MODE ); +}
[.....]
Best regards,
Jingoo Han
RFC PATCH 2/3 PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05
Quoted 10 lines · Jingoo Han
Quoted 4 lines · Zhou Wang
This patch adds PCIe host support for Hisilicon Soc Hip05.
Signed-off-by Zhou Wang <[email protected]>Hi Zhou Wang,
I added some minor comments.
Hi Jingoo,
Quoted 34 lines
Quoted 34 lines
--- 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.cdrivers/pci/host/Kconfig +5
-0@@ -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 + endmenudrivers/pci/host/Makefile +1
-0@@ -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.odrivers/pci/host/pcie-hisi.c new file +0
-1--- /dev/null
Quoted 9 lines
drivers/pci/host/pcie-hisi.c line 4
Quoted 6 lines
+++
b/drivers/pci/host/pcie-hisi.cdrivers/pci/host/pcie-hisi.c new file +4
-0@@ -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.
Quoted 6 lines
Quoted 3 lines
drivers/pci/host/pcie-hisi.c new file +3
-0+* +* Author: Zhou Wang <[email protected]> +* Dacai Zhu <[email protected]> [.....]
Quoted 14 lines
drivers/pci/host/pcie-hisi.c
Quoted 11 lines
drivers/pci/host/pcie-hisi.c new file +11
-0+#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.
Quoted 22 lines
(quoted diff) +2
-0+#define PCIE_SUBCTRL_MODE_REG 0x2800 +#define PCIE_SUBCTRL_SYS_STATE4_REG 0x6818drivers/pci/host/pcie-hisi.c
Quoted 13 lines
drivers/pci/host/pcie-hisi.c new file +12
-0+/* 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?
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 10 lines
drivers/pci/host/pcie-hisi.c
Quoted 7 lines
drivers/pci/host/pcie-hisi.c new file +7
-0+ +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.
Quoted 8 lines
drivers/pci/host/pcie-hisi.c
Quoted 3 lines
drivers/pci/host/pcie-hisi.c new file +3
-0+ +hisi_pcie_change_apb_mode (pcie,PCIE_SLV_DBI_MODE ); +}[.....]
Best regards,
Jingoo Han
Many thanks for your review and Best regards,
Zhou Wang
Quoted 4 lines
.
Bjorn Helgaas
maintainerRFC PATCH 2/3 PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05
Quoted 18 lines · Zhou Wang
Quoted 17 lines · Jingoo Han
Review
Quoted 12 lines · Zhou Wang
(quoted diff) +12
-0+/* 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?
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ 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.
But the combined loop looks functionally equivalent, i.e., I don't think the device can tell the difference. Personally I wouldn't mind if you kept the two loops separate, but if you do, each one needs a comment immediately before it so the reader has a clue that they are doing semantically different things. Otherwise it just looks like a mistake and the reader wonders why they are split.
Bjorn
RFC PATCH 2/3 PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05
Quoted 25 lines · Bjorn Helgaas
Quoted 18 lines · Zhou Wang
Quoted 17 lines · Jingoo Han
Review
Quoted 12 lines · Zhou Wang
(quoted diff) +12
-0+/* 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?
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ 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.
Quoted 8 lines
But the combined loop looks functionally equivalent, i.e., I don't think the device can tell the difference. Personally I wouldn't mind if you kept the two loops separate, but if you do, each one needs a comment immediately before it so the reader has a clue that they are doing semantically different things. Otherwise it just looks like a mistake and the reader wonders why they are split.
Bjorn
Got it. There is a vmid table and a asid table in host controll registers. They are just adjoining as above. I will combine above loops with more comments together.
Thanks and Regards,
Zhou
Quoted 3 lines
.