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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jisheng

> -----Original Message-----
> From: Jisheng Zhang [mailto:jszhang@xxxxxxxxxxx]
> Sent: 07 April 2016 09:35
> To: Gabriele Paoloni
> Cc: jingoohan1@xxxxxxxxx; pratyush.anand@xxxxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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:
> 
> > Hi Jisheng
> >
> > Thanks for your reply
> >
> > > -----Original Message-----
> > > From: Jisheng Zhang [mailto:jszhang@xxxxxxxxxxx]
> > > Sent: 07 April 2016 03:38
> > > To: Gabriele Paoloni; jingoohan1@xxxxxxxxx;
> pratyush.anand@xxxxxxxxx;
> > > bhelgaas@xxxxxxxxxx
> > > Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> arm-
> > > kernel@xxxxxxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup
> code
> > > to dw_pcie_setup_rc()
> > >
> > > Hi Gabriele,
> > >
> > > On Wed, 6 Apr 2016 14:50:29 +0000 Gabriele Paoloni wrote:
> > >
> > > > Hi, sorry to be late on this
> > > >
> > > > > -----Original Message-----
> > > > > From: linux-kernel-owner@xxxxxxxxxxxxxxx [mailto:linux-kernel-
> > > > > owner@xxxxxxxxxxxxxxx] On Behalf Of Jisheng Zhang
> > > > > Sent: 16 March 2016 11:41
> > > > > To: jingoohan1@xxxxxxxxx; pratyush.anand@xxxxxxxxx;
> > > bhelgaas@xxxxxxxxxx
> > > > > Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-
> > > arm-
> > > > > kernel@xxxxxxxxxxxxxxxxxxx; Jisheng Zhang
> > > > > Subject: [PATCH v2] PCI: designware: move remaining rc setup
> code
> > > to
> > > > > dw_pcie_setup_rc()
> > > > >
> > > > > dw_pcie_setup_rc(), as its name indicates, setups the RC. But
> > > current
> > > > > dw_pcie_host_init() also contains some necessary rc setup code.
> > > > >
> > > > > Another reason: the host may lost power during suspend to ram,
> the
> > > RC
> > > > > need to be re-setup after resume. The rc can't be correctly
> resumed
> > > > > without the rc setup code in dw_pcie_host_init().
> > > > >
> > > > > So this patch moves the code to dw_pcie_setup_rc() to address
> the
> > > above
> > > > > two issues. After this patch, each pcie designware driver users
> > > could
> > > > > 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
> > > 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...
> >
> > >
> > > Is it acceptable that pcie-hisi adds a call to dw_pcie_setup_rc()
> in
> > > 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@xxxxxxxxxxx>");
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux