Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1384378 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] PCI: designware: move remaining rc setup code to  dw_pcie_setup_rc() Bjorn Helgaas <helgaas@kernel.org> - 2016-04-21 17:50 +0200
    RE: [PATCH v2] PCI: designware: move remaining rc setup code to  dw_pcie_setup_rc() Gabriele Paoloni <gabriele.paoloni@huawei.com> - 2016-04-21 18:00 +0200

#1384378 — 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.

Bjorn

[toc] | [next] | [standalone]

#1384391

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web