| Message ID | EE11001F9E5DDD47B7634E2F8A612F2E1ED44208@lhreml503-mbs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 07, 2016 at 10:06:45AM +0000, Gabriele Paoloni wrote: > Hi Jisheng > > > -----Original Message----- > > From: Jisheng Zhang [mailto:jszhang@marvell.com] > > Sent: 07 April 2016 09:35 > > To: Gabriele Paoloni > > Cc: jingoohan1@gmail.com; pratyush.anand@gmail.com; > > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-arm-kernel@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: > > > > > Hi Jisheng > > > > > > Thanks for your reply > > > > > > > -----Original Message----- > > > > From: Jisheng Zhang [mailto:jszhang@marvell.com] > > > > Sent: 07 April 2016 03:38 > > > > To: Gabriele Paoloni; jingoohan1@gmail.com; > > pratyush.anand@gmail.com; > > > > bhelgaas@google.com > > > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > > arm- > > > > kernel@lists.infradead.org > > > > 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@vger.kernel.org [mailto:linux-kernel- > > > > > > owner@vger.kernel.org] On Behalf Of Jisheng Zhang > > > > > > Sent: 16 March 2016 11:41 > > > > > > To: jingoohan1@gmail.com; pratyush.anand@gmail.com; > > > > bhelgaas@google.com > > > > > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux- > > > > arm- > > > > > > kernel@lists.infradead.org; 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@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; > } What's the hisi plan for resuming after suspend-to-RAM? How does the RC get reprogrammed after it loses all its state? What would break if hisi did call dw_pcie_setup_rc()? I know you said it would overwrite what the bootloader already did, which is true. But hisi does call dw_pcie_host_init(), so it reads pp->mem (which determines pp->mem_base) and pp->lanes from the DT. Other drivers then call dw_pcie_setup_rc() which programs the RC based on pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC to match the DT, while the other drivers read the DT and program the RC to match. The latter seems more robust because it enforces the consistency rather than relying on it. Bjorn
Hi Bjorn Many thanks for your reply > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 07 April 2016 15:06 > To: Gabriele Paoloni > Cc: Jisheng Zhang; jingoohan1@gmail.com; pratyush.anand@gmail.com; > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > On Thu, Apr 07, 2016 at 10:06:45AM +0000, Gabriele Paoloni wrote: > > Hi Jisheng > > > > > -----Original Message----- > > > From: Jisheng Zhang [mailto:jszhang@marvell.com] > > > Sent: 07 April 2016 09:35 > > > To: Gabriele Paoloni > > > Cc: jingoohan1@gmail.com; pratyush.anand@gmail.com; > > > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > > > kernel@vger.kernel.org; linux-arm-kernel@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: > > > > > > > Hi Jisheng > > > > > > > > Thanks for your reply > > > > > > > > > -----Original Message----- > > > > > From: Jisheng Zhang [mailto:jszhang@marvell.com] > > > > > Sent: 07 April 2016 03:38 > > > > > To: Gabriele Paoloni; jingoohan1@gmail.com; > > > pratyush.anand@gmail.com; > > > > > bhelgaas@google.com > > > > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > linux- > > > arm- > > > > > kernel@lists.infradead.org > > > > > 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@vger.kernel.org [mailto:linux- > kernel- > > > > > > > owner@vger.kernel.org] On Behalf Of Jisheng Zhang > > > > > > > Sent: 16 March 2016 11:41 > > > > > > > To: jingoohan1@gmail.com; pratyush.anand@gmail.com; > > > > > bhelgaas@google.com > > > > > > > Cc: linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; > > > linux- > > > > > arm- > > > > > > > kernel@lists.infradead.org; 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@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; > > } > > What's the hisi plan for resuming after suspend-to-RAM? How does the > RC get reprogrammed after it loses all its state? PM is not part of the driver yet. This is planned for near future release so haven't made such considerations yet > > What would break if hisi did call dw_pcie_setup_rc()? I know you said > it would overwrite what the bootloader already did, which is true. I am try to figure this out now with our HW team. > > But hisi does call dw_pcie_host_init(), so it reads pp->mem (which > determines pp->mem_base) and pp->lanes from the DT. Other drivers > then call dw_pcie_setup_rc() which programs the RC based on > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC to > match the DT, while the other drivers read the DT and program the RC > to match. The latter seems more robust because it enforces the > consistency rather than relying on it. Yes I agree with you, however we have preferred to move RC config to BIOS to have a single driver to support multiple versions of the same SoC. The patch I proposed above does the same job as the original patch proposed by Jisheng and also allows hisi driver to call the moved code. Do you see anything wrong with it? Thanks Gab > > Bjorn
On Fri, Apr 08, 2016 at 01:33:28PM +0000, Gabriele Paoloni wrote: > Hi Bjorn > > Many thanks for your reply > > > -----Original Message----- > > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > > Sent: 07 April 2016 15:06 > > To: Gabriele Paoloni > > Cc: Jisheng Zhang; jingoohan1@gmail.com; pratyush.anand@gmail.com; > > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > > to dw_pcie_setup_rc() > > > > On Thu, Apr 07, 2016 at 10:06:45AM +0000, Gabriele Paoloni wrote: > > > Hi Jisheng > > > > > > > -----Original Message----- > > > > From: Jisheng Zhang [mailto:jszhang@marvell.com] > > > > Sent: 07 April 2016 09:35 > > > > To: Gabriele Paoloni > > > > Cc: jingoohan1@gmail.com; pratyush.anand@gmail.com; > > > > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > > > > kernel@vger.kernel.org; linux-arm-kernel@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: > > > > > > > > > Hi Jisheng > > > > > > > > > > Thanks for your reply > > > > > > > > > > > -----Original Message----- > > > > > > From: Jisheng Zhang [mailto:jszhang@marvell.com] > > > > > > Sent: 07 April 2016 03:38 > > > > > > To: Gabriele Paoloni; jingoohan1@gmail.com; > > > > pratyush.anand@gmail.com; > > > > > > bhelgaas@google.com > > > > > > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux- > > > > arm- > > > > > > kernel@lists.infradead.org > > > > > > 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@vger.kernel.org [mailto:linux- > > kernel- > > > > > > > > owner@vger.kernel.org] On Behalf Of Jisheng Zhang > > > > > > > > Sent: 16 March 2016 11:41 > > > > > > > > To: jingoohan1@gmail.com; pratyush.anand@gmail.com; > > > > > > bhelgaas@google.com > > > > > > > > Cc: linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org; > > > > linux- > > > > > > arm- > > > > > > > > kernel@lists.infradead.org; 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@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; > > > } > > > > What's the hisi plan for resuming after suspend-to-RAM? How does the > > RC get reprogrammed after it loses all its state? > > PM is not part of the driver yet. This is planned for near > future release so haven't made such considerations yet > > > > What would break if hisi did call dw_pcie_setup_rc()? I know you said > > it would overwrite what the bootloader already did, which is true. > > I am try to figure this out now with our HW team. > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem (which > > determines pp->mem_base) and pp->lanes from the DT. Other drivers > > then call dw_pcie_setup_rc() which programs the RC based on > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC to > > match the DT, while the other drivers read the DT and program the RC > > to match. The latter seems more robust because it enforces the > > consistency rather than relying on it. > > Yes I agree with you, however we have preferred to move RC config to > BIOS to have a single driver to support multiple versions of the > same SoC. I think there are two reasonable approaches: 1) A single generic driver that doesn't have any knowledge about the chipset registers; it uses run-time firmware interfaces to manage the bridge. The ACPI pci_root.c driver is the best example so far and works very well. It supports basically all x86 and ia64 chipsets and requires no kernel work for new ones. 2) Native drivers specific to each chipset. These may get configuration information from DT, but they do their own register-level programming of the device without run-time help from firmware. I think hisi is a native driver because it uses hip05/hip06 registers to check link state and perform config operations. And apparently you rely on the ATU, BAR, class, and link width programming currently done in dw_pcie_host_init(). But you want to rely on pre-boot firmware to set up the link. That doesn't make sense to me -- if the driver wants to twiddle the registers, it should know how to do it all. I don't see how you can reasonably manage this half-way approach. > The patch I proposed above does the same job as the original patch > proposed by Jisheng and also allows hisi driver to call the moved > code. > > Do you see anything wrong with it? Only that it makes the structure more complicated and we haven't identified a corresponding benefit yet. Bjorn
Hi Bjorn [...] > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How does > the > > > RC get reprogrammed after it loses all its state? > > > > PM is not part of the driver yet. This is planned for near > > future release so haven't made such considerations yet > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know you > said > > > it would overwrite what the bootloader already did, which is true. > > > > I am try to figure this out now with our HW team. > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem (which > > > determines pp->mem_base) and pp->lanes from the DT. Other drivers > > > then call dw_pcie_setup_rc() which programs the RC based on > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC > to > > > match the DT, while the other drivers read the DT and program the > RC > > > to match. The latter seems more robust because it enforces the > > > consistency rather than relying on it. > > > > Yes I agree with you, however we have preferred to move RC config to > > BIOS to have a single driver to support multiple versions of the > > same SoC. > > I think there are two reasonable approaches: > > 1) A single generic driver that doesn't have any knowledge about the > chipset registers; it uses run-time firmware interfaces to manage > the bridge. The ACPI pci_root.c driver is the best example so far > and works very well. It supports basically all x86 and ia64 > chipsets and requires no kernel work for new ones. > > 2) Native drivers specific to each chipset. These may get > configuration information from DT, but they do their own > register-level programming of the device without run-time help from > firmware. > > I think hisi is a native driver because it uses hip05/hip06 registers > to check link state and perform config operations. And apparently you > rely on the ATU, BAR, class, and link width programming currently done > in dw_pcie_host_init(). But you want to rely on pre-boot firmware to > set up the link. That doesn't make sense to me -- if the driver wants > to twiddle the registers, it should know how to do it all. I don't > see how you can reasonably manage this half-way approach. > > > The patch I proposed above does the same job as the original patch > > proposed by Jisheng and also allows hisi driver to call the moved > > code. > > > > Do you see anything wrong with it? > > Only that it makes the structure more complicated and we haven't > identified a corresponding benefit yet. Finally I have checked that assigning .host_init function pointer in our driver to call dw_pcie_setup_rc() will not affect the values already set by BIOS. Also I agree with you that a hybrid approach is not ideal. So I will update the driver to call dw_pcie_setup_rc() from .host_init and ask the BIOS team to update the firmware for next releases (the driver will be backward compatible anyway). Also during my investigation I have noticed that in dw_pcie_setup_rc() http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L762 we use pp->mem_base rather than pp->mem_bus_addr to setup memory base and memory limit in the Type1 header...I think this is wrong right? Also I do not see why this code is needed at all since we overwrite this register when we call pci_bus_assign_resources(bus) that will end up in calling pci_setup_bridge() and then pci_setup_bridge_mmio()...? Many Thanks Gab > > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote: > > Hi Bjorn > > [...] > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How does > > the > > > > RC get reprogrammed after it loses all its state? > > > > > > PM is not part of the driver yet. This is planned for near > > > future release so haven't made such considerations yet > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know you > > said > > > > it would overwrite what the bootloader already did, which is true. > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem (which > > > > determines pp->mem_base) and pp->lanes from the DT. Other drivers > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC > > to > > > > match the DT, while the other drivers read the DT and program the > > RC > > > > to match. The latter seems more robust because it enforces the > > > > consistency rather than relying on it. > > > > > > Yes I agree with you, however we have preferred to move RC config to > > > BIOS to have a single driver to support multiple versions of the > > > same SoC. > > > > I think there are two reasonable approaches: > > > > 1) A single generic driver that doesn't have any knowledge about the > > chipset registers; it uses run-time firmware interfaces to manage > > the bridge. The ACPI pci_root.c driver is the best example so far > > and works very well. It supports basically all x86 and ia64 > > chipsets and requires no kernel work for new ones. > > > > 2) Native drivers specific to each chipset. These may get > > configuration information from DT, but they do their own > > register-level programming of the device without run-time help from > > firmware. > > > > I think hisi is a native driver because it uses hip05/hip06 registers > > to check link state and perform config operations. And apparently you > > rely on the ATU, BAR, class, and link width programming currently done > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware to > > set up the link. That doesn't make sense to me -- if the driver wants > > to twiddle the registers, it should know how to do it all. I don't > > see how you can reasonably manage this half-way approach. > > > > > The patch I proposed above does the same job as the original patch > > > proposed by Jisheng and also allows hisi driver to call the moved > > > code. > > > > > > Do you see anything wrong with it? > > > > Only that it makes the structure more complicated and we haven't > > identified a corresponding benefit yet. > > Finally I have checked that assigning .host_init function pointer > in our driver to call dw_pcie_setup_rc() will not affect the values > already set by BIOS. > > Also I agree with you that a hybrid approach is not ideal. I also agree with Bjorn's opinion. As far as I know, two approaches are reasonable. In the case of using UEFI, how about using 'pci-host-generic.c'? You may consult with Linaro guys for this issue. Good luck. Best regards, Jingoo Han > > So I will update the driver to call dw_pcie_setup_rc() from > .host_init and ask the BIOS team to update the firmware for next > releases (the driver will be backward compatible anyway). > > Also during my investigation I have noticed that in dw_pcie_setup_rc() > http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L762 > > we use pp->mem_base rather than pp->mem_bus_addr to setup > memory base and memory limit in the Type1 header...I think this > is wrong right? > Also I do not see why this code is needed at all since we overwrite > this register when we call pci_bus_assign_resources(bus) that > will end up in calling pci_setup_bridge() and then > pci_setup_bridge_mmio()...? > > Many Thanks > > Gab > > > > > Bjorn > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jingoo > -----Original Message----- > From: Jingoo Han [mailto:jingoohan1@gmail.com] > Sent: 13 April 2016 06:52 > To: Gabriele Paoloni; 'Bjorn Helgaas' > Cc: 'Jisheng Zhang'; pratyush.anand@gmail.com; bhelgaas@google.com; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; 'Jingoo Han' > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote: > > > > Hi Bjorn > > > > [...] > > > > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How > does > > > the > > > > > RC get reprogrammed after it loses all its state? > > > > > > > > PM is not part of the driver yet. This is planned for near > > > > future release so haven't made such considerations yet > > > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know > you > > > said > > > > > it would overwrite what the bootloader already did, which is > true. > > > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem > (which > > > > > determines pp->mem_base) and pp->lanes from the DT. Other > drivers > > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed > the RC > > > to > > > > > match the DT, while the other drivers read the DT and program > the > > > RC > > > > > to match. The latter seems more robust because it enforces the > > > > > consistency rather than relying on it. > > > > > > > > Yes I agree with you, however we have preferred to move RC config > to > > > > BIOS to have a single driver to support multiple versions of the > > > > same SoC. > > > > > > I think there are two reasonable approaches: > > > > > > 1) A single generic driver that doesn't have any knowledge about > the > > > chipset registers; it uses run-time firmware interfaces to manage > > > the bridge. The ACPI pci_root.c driver is the best example so > far > > > and works very well. It supports basically all x86 and ia64 > > > chipsets and requires no kernel work for new ones. > > > > > > 2) Native drivers specific to each chipset. These may get > > > configuration information from DT, but they do their own > > > register-level programming of the device without run-time help > from > > > firmware. > > > > > > I think hisi is a native driver because it uses hip05/hip06 > registers > > > to check link state and perform config operations. And apparently > you > > > rely on the ATU, BAR, class, and link width programming currently > done > > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware > to > > > set up the link. That doesn't make sense to me -- if the driver > wants > > > to twiddle the registers, it should know how to do it all. I don't > > > see how you can reasonably manage this half-way approach. > > > > > > > The patch I proposed above does the same job as the original > patch > > > > proposed by Jisheng and also allows hisi driver to call the moved > > > > code. > > > > > > > > Do you see anything wrong with it? > > > > > > Only that it makes the structure more complicated and we haven't > > > identified a corresponding benefit yet. > > > > Finally I have checked that assigning .host_init function pointer > > in our driver to call dw_pcie_setup_rc() will not affect the values > > already set by BIOS. > > > > Also I agree with you that a hybrid approach is not ideal. > > I also agree with Bjorn's opinion. > As far as I know, two approaches are reasonable. > > In the case of using UEFI, how about using 'pci-host-generic.c'? > You may consult with Linaro guys for this issue. Many thanks for your suggestion, I'll take it into account for next releases > Good luck. > > Best regards, > Jingoo Han > > > > > So I will update the driver to call dw_pcie_setup_rc() from > > .host_init and ask the BIOS team to update the firmware for next > > releases (the driver will be backward compatible anyway). > > > > Also during my investigation I have noticed that in > dw_pcie_setup_rc() > > http://lxr.free-electrons.com/source/drivers/pci/host/pcie- > designware.c#L762 > > > > we use pp->mem_base rather than pp->mem_bus_addr to setup > > memory base and memory limit in the Type1 header...I think this > > is wrong right? > > Also I do not see why this code is needed at all since we overwrite > > this register when we call pci_bus_assign_resources(bus) that > > will end up in calling pci_setup_bridge() and then > > pci_setup_bridge_mmio()...? Do you have any comment on this issue above? > > > > Many Thanks > > > > Gab > > > > > > > > Bjorn > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux- > pci" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, April 13, 2016 4:58 PM, Gabriele Paoloni wrote: > > Hi Jingoo > > On 13 April 2016 06:52, Jingoo Han wrote: > > On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote: > > > > > > Hi Bjorn > > > > > > [...] > > > > > > > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How > > does > > > > the > > > > > > RC get reprogrammed after it loses all its state? > > > > > > > > > > PM is not part of the driver yet. This is planned for near > > > > > future release so haven't made such considerations yet > > > > > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know > > you > > > > said > > > > > > it would overwrite what the bootloader already did, which is > > true. > > > > > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem > > (which > > > > > > determines pp->mem_base) and pp->lanes from the DT. Other > > drivers > > > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed > > the RC > > > > to > > > > > > match the DT, while the other drivers read the DT and program > > the > > > > RC > > > > > > to match. The latter seems more robust because it enforces the > > > > > > consistency rather than relying on it. > > > > > > > > > > Yes I agree with you, however we have preferred to move RC config > > to > > > > > BIOS to have a single driver to support multiple versions of the > > > > > same SoC. > > > > > > > > I think there are two reasonable approaches: > > > > > > > > 1) A single generic driver that doesn't have any knowledge about > > the > > > > chipset registers; it uses run-time firmware interfaces to manage > > > > the bridge. The ACPI pci_root.c driver is the best example so > > far > > > > and works very well. It supports basically all x86 and ia64 > > > > chipsets and requires no kernel work for new ones. > > > > > > > > 2) Native drivers specific to each chipset. These may get > > > > configuration information from DT, but they do their own > > > > register-level programming of the device without run-time help > > from > > > > firmware. > > > > > > > > I think hisi is a native driver because it uses hip05/hip06 > > registers > > > > to check link state and perform config operations. And apparently > > you > > > > rely on the ATU, BAR, class, and link width programming currently > > done > > > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware > > to > > > > set up the link. That doesn't make sense to me -- if the driver > > wants > > > > to twiddle the registers, it should know how to do it all. I don't > > > > see how you can reasonably manage this half-way approach. > > > > > > > > > The patch I proposed above does the same job as the original > > patch > > > > > proposed by Jisheng and also allows hisi driver to call the moved > > > > > code. > > > > > > > > > > Do you see anything wrong with it? > > > > > > > > Only that it makes the structure more complicated and we haven't > > > > identified a corresponding benefit yet. > > > > > > Finally I have checked that assigning .host_init function pointer > > > in our driver to call dw_pcie_setup_rc() will not affect the values > > > already set by BIOS. > > > > > > Also I agree with you that a hybrid approach is not ideal. > > > > I also agree with Bjorn's opinion. > > As far as I know, two approaches are reasonable. > > > > In the case of using UEFI, how about using 'pci-host-generic.c'? > > You may consult with Linaro guys for this issue. > > Many thanks for your suggestion, I'll take it into account for next > releases > > > Good luck. > > > > Best regards, > > Jingoo Han > > > > > > > > So I will update the driver to call dw_pcie_setup_rc() from > > > .host_init and ask the BIOS team to update the firmware for next > > > releases (the driver will be backward compatible anyway). > > > > > > Also during my investigation I have noticed that in > > dw_pcie_setup_rc() > > > http://lxr.free-electrons.com/source/drivers/pci/host/pcie- > > designware.c#L762 > > > > > > we use pp->mem_base rather than pp->mem_bus_addr to setup > > > memory base and memory limit in the Type1 header...I think this > > > is wrong right? > > > Also I do not see why this code is needed at all since we overwrite > > > this register when we call pci_bus_assign_resources(bus) that > > > will end up in calling pci_setup_bridge() and then > > > pci_setup_bridge_mmio()...? > > Do you have any comment on this issue above? Sorry, I am not sure. However, there are some redundant codes like this. At that time, I was not able to decide to remove these codes. Maybe, Pratyush Anand or other guys would give opinions about this. Best regards, Jingoo Han > > > > > > > Many Thanks > > > > > > Gab > > > > > > > > > > > Bjorn > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux- > > pci" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Gabriele, On Thu, Apr 14, 2016 at 5:22 PM, Jingoo Han <jingoohan1@gmail.com> wrote: > On Wednesday, April 13, 2016 4:58 PM, Gabriele Paoloni wrote: >> >> Hi Jingoo >> >> On 13 April 2016 06:52, Jingoo Han wrote: >> > On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote: [...] >> > > So I will update the driver to call dw_pcie_setup_rc() from >> > > .host_init and ask the BIOS team to update the firmware for next >> > > releases (the driver will be backward compatible anyway). >> > > >> > > Also during my investigation I have noticed that in >> > dw_pcie_setup_rc() >> > > http://lxr.free-electrons.com/source/drivers/pci/host/pcie- >> > designware.c#L762 >> > > >> > > we use pp->mem_base rather than pp->mem_bus_addr to setup >> > > memory base and memory limit in the Type1 header...I think this >> > > is wrong right? Yes. RC's "memory base" and "memory limit" should be governed by PCI addresses and not CPU addresses. So, it should use pp->mem_bus_addr. >> > > Also I do not see why this code is needed at all since we overwrite >> > > this register when we call pci_bus_assign_resources(bus) that >> > > will end up in calling pci_setup_bridge() and then >> > > pci_setup_bridge_mmio()...? >> >> Do you have any comment on this issue above? Probably thats why things are working. Thanks for finding it. I think, /* setup memory base, memory limit */ hunk can be removed from dw_pcie_setup_rc. ~Pratyush
Hi Pratyush thanks for you reply > -----Original Message----- > From: Pratyush Anand [mailto:pratyush.anand@gmail.com] > Sent: 14 April 2016 14:08 > To: Jingoo Han; Gabriele Paoloni > Cc: Bjorn Helgaas; Jisheng Zhang; Bjorn Helgaas; linux- > pci@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > Hi Gabriele, > > On Thu, Apr 14, 2016 at 5:22 PM, Jingoo Han <jingoohan1@gmail.com> > wrote: > > On Wednesday, April 13, 2016 4:58 PM, Gabriele Paoloni wrote: > >> > >> Hi Jingoo > >> > >> On 13 April 2016 06:52, Jingoo Han wrote: > >> > On Tuesday, April 12, 2016 6:44 PM, Gabriele Paoloni wrote: > > [...] > > >> > > So I will update the driver to call dw_pcie_setup_rc() from > >> > > .host_init and ask the BIOS team to update the firmware for next > >> > > releases (the driver will be backward compatible anyway). > >> > > > >> > > Also during my investigation I have noticed that in > >> > dw_pcie_setup_rc() > >> > > http://lxr.free-electrons.com/source/drivers/pci/host/pcie- > >> > designware.c#L762 > >> > > > >> > > we use pp->mem_base rather than pp->mem_bus_addr to setup > >> > > memory base and memory limit in the Type1 header...I think this > >> > > is wrong right? > > Yes. RC's "memory base" and "memory limit" should be governed by PCI > addresses and not CPU addresses. So, it should use pp->mem_bus_addr. > > >> > > Also I do not see why this code is needed at all since we > overwrite > >> > > this register when we call pci_bus_assign_resources(bus) that > >> > > will end up in calling pci_setup_bridge() and then > >> > > pci_setup_bridge_mmio()...? > >> > >> Do you have any comment on this issue above? > > Probably thats why things are working. > Thanks for finding it. I think, /* setup memory base, memory limit */ > hunk can be removed from dw_pcie_setup_rc. Great, I'll send out a patch to remove this Thanks Gab > > ~Pratyush
On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote: > Hi Bjorn > > [...] > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How does > > the > > > > RC get reprogrammed after it loses all its state? > > > > > > PM is not part of the driver yet. This is planned for near > > > future release so haven't made such considerations yet > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know you > > said > > > > it would overwrite what the bootloader already did, which is true. > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem (which > > > > determines pp->mem_base) and pp->lanes from the DT. Other drivers > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed the RC > > to > > > > match the DT, while the other drivers read the DT and program the > > RC > > > > to match. The latter seems more robust because it enforces the > > > > consistency rather than relying on it. > > > > > > Yes I agree with you, however we have preferred to move RC config to > > > BIOS to have a single driver to support multiple versions of the > > > same SoC. > > > > I think there are two reasonable approaches: > > > > 1) A single generic driver that doesn't have any knowledge about the > > chipset registers; it uses run-time firmware interfaces to manage > > the bridge. The ACPI pci_root.c driver is the best example so far > > and works very well. It supports basically all x86 and ia64 > > chipsets and requires no kernel work for new ones. > > > > 2) Native drivers specific to each chipset. These may get > > configuration information from DT, but they do their own > > register-level programming of the device without run-time help from > > firmware. > > > > I think hisi is a native driver because it uses hip05/hip06 registers > > to check link state and perform config operations. And apparently you > > rely on the ATU, BAR, class, and link width programming currently done > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware to > > set up the link. That doesn't make sense to me -- if the driver wants > > to twiddle the registers, it should know how to do it all. I don't > > see how you can reasonably manage this half-way approach. > > > > > The patch I proposed above does the same job as the original patch > > > proposed by Jisheng and also allows hisi driver to call the moved > > > code. > > > > > > Do you see anything wrong with it? > > > > Only that it makes the structure more complicated and we haven't > > identified a corresponding benefit yet. > > Finally I have checked that assigning .host_init function pointer > in our driver to call dw_pcie_setup_rc() will not affect the values > already set by BIOS. > > Also I agree with you that a hybrid approach is not ideal. > > So I will update the driver to call dw_pcie_setup_rc() from > .host_init and ask the BIOS team to update the firmware for next > releases (the driver will be backward compatible anyway). Am I right in assuming that the patch currently in my tree: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-designware&id=1488aefa37a4033080942c860294d13c613ec829 will work for you? I'm going to assume so unless I hear otherwise. Bjorn
Hi Bjorn > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 21 April 2016 16:49 > To: Gabriele Paoloni > Cc: Jisheng Zhang; jingoohan1@gmail.com; pratyush.anand@gmail.com; > bhelgaas@google.com; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code > to dw_pcie_setup_rc() > > On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote: > > Hi Bjorn > > > > [...] > > > > > > > > > > > > What's the hisi plan for resuming after suspend-to-RAM? How > does > > > the > > > > > RC get reprogrammed after it loses all its state? > > > > > > > > PM is not part of the driver yet. This is planned for near > > > > future release so haven't made such considerations yet > > > > > > > > > > What would break if hisi did call dw_pcie_setup_rc()? I know > you > > > said > > > > > it would overwrite what the bootloader already did, which is > true. > > > > > > > > I am try to figure this out now with our HW team. > > > > > > > > > > > > > > But hisi does call dw_pcie_host_init(), so it reads pp->mem > (which > > > > > determines pp->mem_base) and pp->lanes from the DT. Other > drivers > > > > > then call dw_pcie_setup_rc() which programs the RC based on > > > > > pp->mem_base and pp->lanes. So hisi assumes UEFI programmed > the RC > > > to > > > > > match the DT, while the other drivers read the DT and program > the > > > RC > > > > > to match. The latter seems more robust because it enforces the > > > > > consistency rather than relying on it. > > > > > > > > Yes I agree with you, however we have preferred to move RC config > to > > > > BIOS to have a single driver to support multiple versions of the > > > > same SoC. > > > > > > I think there are two reasonable approaches: > > > > > > 1) A single generic driver that doesn't have any knowledge about > the > > > chipset registers; it uses run-time firmware interfaces to manage > > > the bridge. The ACPI pci_root.c driver is the best example so > far > > > and works very well. It supports basically all x86 and ia64 > > > chipsets and requires no kernel work for new ones. > > > > > > 2) Native drivers specific to each chipset. These may get > > > configuration information from DT, but they do their own > > > register-level programming of the device without run-time help > from > > > firmware. > > > > > > I think hisi is a native driver because it uses hip05/hip06 > registers > > > to check link state and perform config operations. And apparently > you > > > rely on the ATU, BAR, class, and link width programming currently > done > > > in dw_pcie_host_init(). But you want to rely on pre-boot firmware > to > > > set up the link. That doesn't make sense to me -- if the driver > wants > > > to twiddle the registers, it should know how to do it all. I don't > > > see how you can reasonably manage this half-way approach. > > > > > > > The patch I proposed above does the same job as the original > patch > > > > proposed by Jisheng and also allows hisi driver to call the moved > > > > code. > > > > > > > > Do you see anything wrong with it? > > > > > > Only that it makes the structure more complicated and we haven't > > > identified a corresponding benefit yet. > > > > Finally I have checked that assigning .host_init function pointer > > in our driver to call dw_pcie_setup_rc() will not affect the values > > already set by BIOS. > > > > Also I agree with you that a hybrid approach is not ideal. > > > > So I will update the driver to call dw_pcie_setup_rc() from > > .host_init and ask the BIOS team to update the firmware for next > > releases (the driver will be backward compatible anyway). > > Am I right in assuming that the patch currently in my tree: > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h= > pci/host-designware&id=1488aefa37a4033080942c860294d13c613ec829 > > will work for you? I'm going to assume so unless I hear otherwise. Yes you are right. I thought it was clear by the last conclusion. Sorry if it was not explicit. Many Thanks and Regards Gab > > Bjorn
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; }