[PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc()

Hi Jisheng

-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 09:35
To: Gabriele Paoloni
Cc: jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()

Hi Gabriele,

On Thu, 7 Apr 2016 08:20:28 +0000 Gabriele Paoloni wrote:

quoted

Hi Jisheng

Thanks for your reply

quoted

-----Original Message-----
From: Jisheng Zhang [mailto:jszhang at marvell.com]
Sent: 07 April 2016 03:38
To: Gabriele Paoloni; jingoohan1 at gmail.com;

pratyush.anand at gmail.com;

quoted

quoted

bhelgaas at google.com
Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org; linux-

arm-

quoted

quoted

kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup

code

quoted

quoted

to dw_pcie_setup_rc()

Hi Gabriele,

On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:

quoted

Hi, sorry to be late on this

quoted

-----Original Message-----
From: linux-kernel-owner at vger.kernel.org [mailto:linux-kernel-
owner at vger.kernel.org] On Behalf Of Jisheng Zhang
Sent: 16 March 2016 11:41
To: jingoohan1 at gmail.com; pratyush.anand at gmail.com;

bhelgaas at google.com

quoted

quoted

Cc: linux-pci at vger.kernel.org; linux-kernel at vger.kernel.org;

linux-

quoted

quoted

arm-

quoted

quoted

kernel at lists.infradead.org; Jisheng Zhang
Subject: [PATCH v2] PCI: designware: move remaining rc setup

code

quoted

quoted

to

quoted

quoted

dw_pcie_setup_rc()

dw_pcie_setup_rc(), as its name indicates, setups the RC. But

current

quoted

quoted

dw_pcie_host_init() also contains some necessary rc setup code.

Another reason: the host may lost power during suspend to ram,

the

quoted

quoted

RC

quoted

quoted

need to be re-setup after resume. The rc can't be correctly

resumed

quoted

quoted

quoted

quoted

without the rc setup code in dw_pcie_host_init().

So this patch moves the code to dw_pcie_setup_rc() to address

the

quoted

quoted

above

quoted

quoted

two issues. After this patch, each pcie designware driver users

could

quoted

quoted

call dw_pcie_setup_rc() to re-setup rc when resume back.

I think this patch breaks the Hisilicon driver...

Our driver performs linkup setup in UEFI therefore we do not call
dw_pcie_setup_rc(), we only call dw_pcie_host_init().

Thanks for the information. So pcie-hisi rely on UEFI to do

something

quoted

quoted

similar
in dw_pcie_setup_rc(), this comes to a common driver implement
question: should
linux device driver rely on bootloader to configure HW device?

I don't see any issue with this...

quoted

Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc()

in

quoted

quoted

hisi_add_pcie_port()?

I don't think so...that would try to overwrite what is already set by
the bootloader; so it is wrong in principle and maybe it can lead to
undefined behaviours...

make sense! This commit is intend to re-setup the rc when waken from
s2ram (in
s2ram state, the host lost power)

I have no good solution but to introduce one function e.g
dw_pcie_setup_rc_after_linkup(), then move related code from
dw_pcie_host_init
to it, then let my host driver resume hook to call.

Hi Pratyush, Jingoo and Bjorn etc.

any suggestions are appreciated!

What about:

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index a4cccd3..e461f5d 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c

@@ -434,7 +434,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	struct platform_device *pdev = to_platform_device(pp->dev);
 	struct pci_bus *bus, *child;
 	struct resource *cfg_res;
-	u32 val;
 	int i, ret;
 	LIST_HEAD(res);
 	struct resource_entry *win;

@@ -544,25 +543,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (pp->ops->host_init)
 		pp->ops->host_init(pp);
 
-	/*
-	 * If the platform provides ->rd_other_conf, it means the platform
-	 * uses its own address translation component rather than ATU, so
-	 * we should not program the ATU here.
-	 */
-	if (!pp->ops->rd_other_conf)
-		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
-					  PCIE_ATU_TYPE_MEM, pp->mem_base,
-					  pp->mem_bus_addr, pp->mem_size);
-
-	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
-
-	/* program correct class for RC */
-	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
-
-	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
-	val |= PORT_LOGIC_SPEED_CHANGE;
-	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
-
 	pp->root_bus_nr = pp->busn->start;
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		bus = pci_scan_root_bus_msi(pp->dev, pp->root_bus_nr,

@@ -725,6 +705,29 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
+void dw_pcie_setup_own_cfg(struct pcie_port *pp)
+{
+	u32 val;
+	/*
+	 * If the platform provides ->rd_other_conf, it means the platform
+	 * uses its own address translation component rather than ATU, so
+	 * we should not program the ATU here.
+	 */
+	if (!pp->ops->rd_other_conf)
+		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
+					  PCIE_ATU_TYPE_MEM, pp->mem_base,
+					  pp->mem_bus_addr, pp->mem_size);
+
+	dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0);
+
+	/* program correct class for RC */
+	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
+
+	dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val);
+	val |= PORT_LOGIC_SPEED_CHANGE;
+	dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val);
+}
+
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;

@@ -800,6 +803,8 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
 		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
 	dw_pcie_writel_rc(pp, val, PCI_COMMAND);
+
+	dw_pcie_setup_own_cfg(pp);
 }
 
 MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");

diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index f437f9b..caf0f5d 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h

@@ -85,5 +85,6 @@ int dw_pcie_wait_for_link(struct pcie_port *pp);
 int dw_pcie_link_up(struct pcie_port *pp);
 void dw_pcie_setup_rc(struct pcie_port *pp);
 int dw_pcie_host_init(struct pcie_port *pp);
+void dw_pcie_setup_own_cfg(struct pcie_port *pp);
 
 #endif /* _PCIE_DESIGNWARE_H */

diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
index 3e98d4e..8da29b2 100644
--- a/drivers/pci/host/pcie-hisi.c
+++ b/drivers/pci/host/pcie-hisi.c

@@ -164,6 +164,7 @@ static int hisi_add_pcie_port(struct pcie_port *pp,
 		dev_err(&pdev->dev, "failed to initialize host\n");
 		return ret;
 	}
+	dw_pcie_setup_own_cfg(pp);
 
 	return 0;
 }

Thanks,
Jisheng