Message-ID: <EE11001F9E5DDD47B7634E2F8A612F2E1ED4F10E@lhreml503-mbs>
Date:	Wed, 13 Apr 2016 07:57:37 +0000
From:	Gabriele Paoloni <gabriele.paoloni@...wei.com>
To:	Jingoo Han <jingoohan1@...il.com>,
	"'Bjorn Helgaas'" <helgaas@...nel.org>
CC:	"'Jisheng Zhang'" <jszhang@...vell.com>,
	"pratyush.anand@...il.com" <pratyush.anand@...il.com>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v2] PCI: designware: move remaining rc setup code to
 dw_pcie_setup_rc()

Hi Jingoo

> -----Original Message-----
> From: Jingoo Han [mailto:jingoohan1@...il.com]
> Sent: 13 April 2016 06:52
> To: Gabriele Paoloni; 'Bjorn Helgaas'
> Cc: 'Jisheng Zhang'; pratyush.anand@...il.com; bhelgaas@...gle.com;
> linux-pci@...r.kernel.org; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.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@...r.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists