[PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc()
jingoohan1 at gmail.com
Tue Apr 12 22:51:40 PDT 2016
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.
> 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()
> 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
> Many Thanks
> > Bjorn
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the linux-arm-kernel