From: Liviu Dudau <Liviu.Dudau@arm.com>
To: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Mark Rutland <Mark.Rutland@arm.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"jason@lakedaemon.net" <jason@lakedaemon.net>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
Marc Zyngier <Marc.Zyngier@arm.com>,
Will Deacon <Will.Deacon@arm.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
Catalin Marinas <Catalin.Marinas@arm.com>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"tglx@linutronix.de" <tglx@linutronix.de>
Subject: Re: [RFC 2/4] PCI: generic: Add support for ARM64 and MSI(x)
Date: Tue, 30 Sep 2014 18:48:21 +0100 [thread overview]
Message-ID: <20140930174821.GX841@e106497-lin.cambridge.arm.com> (raw)
In-Reply-To: <20140930173540.GB16583@e102568-lin.cambridge.arm.com>
On Tue, Sep 30, 2014 at 06:35:40PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Sep 30, 2014 at 05:42:56PM +0100, Liviu Dudau wrote:
> > On Tue, Sep 30, 2014 at 05:12:41PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Sep 30, 2014 at 01:31:44PM +0100, Arnd Bergmann wrote:
> > > > On Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote:
> > > > > > > static int gen_pci_probe(struct platform_device *pdev)
> > > > > > > {
> > > > > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > > > > struct device *dev = &pdev->dev;
> > > > > > > struct device_node *np = dev->of_node;
> > > > > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > > > > +#ifndef CONFIG_ARM64
> > > > > > > struct hw_pci hw = {
> > > > > > > .nr_controllers = 1,
> > > > > > > .private_data = (void **)&pci,
> > > > > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > > > > .map_irq = of_irq_parse_and_map_pci,
> > > > > > > .ops = &gen_pci_ops,
> > > > > > > };
> > > > > > > +#endif
> > > > > > >
> > > > > >
> > > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
> > > > > > is an arm32 specific data structure.
> > > > >
> > > > > I do not think we need hw struct at all, see below, we can write code so
> > > > > that we do not rely on ARM32 PCI bios, I will have a stab at that and
> > > > > post the resulting code.
> > > >
> > > > That would of course be best. I think it needs some rework of the
> > > > arm32 PCI code though, or you'd still have to create pci_sys_data
> > > > manually, and that is currently allocated by pcibios_init_hw.
> >
> > I don't see why we need to involve the arm32 code here at all. A host bridge can
> > be fully functional with the generic code without having to use any of the
> > arm32 code (unless I'm missing something here).
>
> Ok so I can remove the pci_common_init() call, use the common PCI API and
> everything will work as expected, even if there is a list of functions (see
> below) that *require* pci_sys_data to exist (and that's allocated in arm32
> pcibios code in pcibios_init_hw(), called from pci_common_init()) ?
See bellow my comments on those functions.
>
> I like the idea but I think that's optimistic, or at least we did not
> trigger the code paths that can cause issues.
>
> > > Right, as far as I can see, creating a pci_sys_data struct
> > > that's all we would need. "Problem" is that it does not exist on ARM64
> > > so to avoid ifdeffery we have to declare a struct with the same
> > > fields (ie only pci_sys_data.private_data is used by this driver -
> > > apart from arm32 specific functions usage) that is passed to the PCI layer
> > > and stored in the bus.sysdata, but that's extremely ugly (and we won't
> > > need this when the arm32 conversion is completed).
> > >
> > > > > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start,
> > > > > > > + &gen_pci_ops, pci, &pci->resources)) {
> > > > > > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n");
> > > > > > > + return -ENODEV;
> > > > > > > + }
> > > > > > > +#else
> > > > > > > pci_common_init_dev(dev, &hw);
> > > > > > > +#endif /* CONFIG_ARM64 */
> > > > > > >
> > > > > >
> > > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
> > > > > > the generic case after it, outside of the #ifdef.
> > > > >
> > > > > I went through the code quickly but I think we can (and should) remove
> > > > > this quite ugly ifdeffery altogether. Most of the functionality in
> > > > > pci_common_init_dev() can be implemented through the common PCI API (and this
> > > > > would make this driver arch agnostic as it should be), I will go through ARM32
> > > > > PCI bios code to check what is executed in detail in pci_common_init_dev() and
> > > > > make sure that we follow those initialization steps in the resulting probe code
> > > > > for this PCI generic host controller driver.
> > > >
> > > > These are the functions I found that refer to pci_sys_data on arm32:
> > > >
> > > > pcibios_add_bus
> > > > pcibios_remove_bus
These are only needed if you want to do per HB processing of the bus
> > > > pcibios_align_resource
mvebu is the only user of this function.
> > > > pci_mmap_page_range
This is only needed when mapping a PCI resource to userspace. Is that your case here?
> > > > pci_domain_nr
> > > > pci_proc_domain
We have equivalent functionality in the generic patches for those.
Best regards,
Liviu
> > > >
> > > > This is not as bad as I had feared, but we still have to ensure that
> > > > any caller of these functions will work with both the generic PCI support
> > > > and the arm32 specific drivers that today use hw_pci.
> > > >
> > > > My idea for dealing with this was to convert all host drivers in
> > > > drivers/pci/host to the generic PCI code and never build the arm32
> > > > bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that
> > > > requires either doing them all at once or coming up with a migration
> > > > strategy so we don't break things in the process.
> > >
> > > That makes sense. Related to the migration strategy, thoughts
> > > appreciated. Declaring a static pci_sys_data (with some ifdef around it)
> > > seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM
> > > is rather horrible too, but we can probably live with that.
> > >
> > > I do not see anything else as possible solution at the moment unless
> > > we go the whole nine yards and do what you suggest above, might take a
> > > little while though.
> > >
> > > Probably leaving pci_common_init() call (and related hw_pci struct, and
> > > related ifdeffery to differentiate between different sysdata layouts for ARM
> > > and ARM64) is the fastest path but I still think it is not nice at all.
> >
> > Rob Herring found the conversion of mach-integrator/pci_v3.c to the generic
> > framework quite painless. We might have to go through a lot of testing, but I don't
> > see the process to be too horrendous.
>
> See my comments above, I have not said that the conversion is complicated,
> what I am saying is that I am not sure we can get rid of pcibios code calls
> yet, as Arnd pointed out.
>
> Lorenzo
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
next prev parent reply other threads:[~2014-09-30 17:48 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-28 20:53 [RFC 0/4] Add PCI/MSI(x) support for AMD Seattle Platform suravee.suthikulpanit
2014-09-28 20:53 ` [RFC 1/4] arm64: amd-seattle: Adding device tree for AMD Seattle platform suravee.suthikulpanit
2014-10-10 13:45 ` Mark Rutland
2014-10-24 12:08 ` Suravee Suthikulpanit
2014-09-28 20:53 ` [RFC 2/4] PCI: generic: Add support for ARM64 and MSI(x) suravee.suthikulpanit
2014-09-29 14:36 ` Arnd Bergmann
2014-09-30 12:03 ` Lorenzo Pieralisi
2014-09-30 12:31 ` Arnd Bergmann
2014-09-30 16:12 ` Lorenzo Pieralisi
2014-09-30 16:42 ` Liviu Dudau
2014-09-30 17:35 ` Lorenzo Pieralisi
2014-09-30 17:48 ` Liviu Dudau [this message]
2014-09-30 18:54 ` Arnd Bergmann
2014-09-30 20:01 ` Arnd Bergmann
2014-10-01 8:46 ` Liviu Dudau
2014-10-01 9:38 ` Arnd Bergmann
2014-10-07 12:06 ` Lorenzo Pieralisi
2014-10-07 13:52 ` Arnd Bergmann
2014-10-07 14:47 ` Lorenzo Pieralisi
2014-10-07 21:39 ` Arnd Bergmann
2014-10-08 10:19 ` Lorenzo Pieralisi
2014-10-08 14:47 ` Arnd Bergmann
2014-10-09 9:04 ` Lorenzo Pieralisi
2014-10-09 10:51 ` Arnd Bergmann
2014-10-10 13:58 ` Lorenzo Pieralisi
2014-10-10 18:31 ` Arnd Bergmann
2014-10-13 9:36 ` Lorenzo Pieralisi
2014-10-22 15:59 ` Lorenzo Pieralisi
2014-10-22 16:49 ` Bjorn Helgaas
2014-10-22 20:52 ` Arnd Bergmann
2014-10-23 9:13 ` Liviu Dudau
2014-10-23 11:27 ` Lorenzo Pieralisi
2014-10-23 16:52 ` Jason Gunthorpe
2014-10-27 16:10 ` Lorenzo Pieralisi
2014-10-23 13:33 ` Arnd Bergmann
2014-10-24 10:04 ` Liviu Dudau
2014-11-05 23:40 ` Bjorn Helgaas
2014-11-06 0:06 ` Arnd Bergmann
2014-12-29 19:32 ` Suravee Suthikulpanit
2015-01-02 11:55 ` Lorenzo Pieralisi
2015-01-02 18:18 ` Suravee Suthikulanit
2015-01-02 21:09 ` Arnd Bergmann
2015-01-05 14:48 ` Lorenzo Pieralisi
2014-11-05 23:39 ` Bjorn Helgaas
2014-11-06 0:05 ` Arnd Bergmann
2014-11-06 9:52 ` Lorenzo Pieralisi
2014-09-29 19:19 ` Sunil Kovvuri
2014-09-28 20:53 ` [RFC 3/4] arm64: Do not call enable PCI resources when specify PCI_PROBE_ONLY suravee.suthikulpanit
2014-09-29 14:38 ` Arnd Bergmann
2014-09-29 18:17 ` Bjorn Helgaas
2015-06-23 22:34 ` Benjamin Herrenschmidt
2015-06-23 23:05 ` Russell King - ARM Linux
2015-06-23 22:32 ` Benjamin Herrenschmidt
2014-09-28 20:53 ` [RFC 4/4] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X) suravee.suthikulpanit
2014-09-28 21:35 ` Suravee Suthikulpanit
2014-09-29 14:23 ` Thomas Gleixner
2014-09-29 14:42 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140930174821.GX841@e106497-lin.cambridge.arm.com \
--to=liviu.dudau@arm.com \
--cc=Catalin.Marinas@arm.com \
--cc=Lorenzo.Pieralisi@arm.com \
--cc=Marc.Zyngier@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=Will.Deacon@arm.com \
--cc=arnd@arndb.de \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=jason@lakedaemon.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=suravee.suthikulpanit@amd.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).