On Fri, Nov 07, 2014 at 04:07:30PM +0000, Rob Herring wrote:
> On Thu, Nov 6, 2014 at 9:32 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@xxxxxxx> wrote:
> > Most if not all ARM PCI host controller device drivers either ignore the
> > domain field in the pci_sys_data structure or just increment it every
> > time a host controller is probed, using it as a domain counter.
> >
> > Therefore, instead of relying on pci_sys_data to stash the domain number
> > in a standard location, ARM pcibios code can be moved to the newly
> > introduced generic PCI domains code, implemented in commits:
> >
> > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> >
> > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > ("PCI: Add generic domain handling")
> >
> > In order to assign a domain number dynamically, the ARM pcibios defines
> > the function, called by core PCI code:
> >
> > void pci_bus_assign_domain_nr(...)
> >
> > that relies on a DT property to define the domain number or falls back to
> > a counter; its usage replaces the current domain assignment code in PCI
> > host controllers present in the kernel.
>
> I realize you are just copying the arm64 version, but as we've agreed
> the DT domain handling should be common and what the error cases are,
> I've got some comments on the current implementation.
So this means that either I patch arm64 code with your implementation
below and I duplicate the code for arm too, or I have to factor out an
implementation based on your code below and add it somewhere in common
kernel code (drivers/pci/pci.c ? drivers/pci/of.c ?), where, it has to be
seen.
>
> [...]
>
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +static bool dt_domain_found;
>
> This can be moved into the function.
Yes, I noticed while putting the patch together, I was more concerned
about the removal of pci_sys_data.domain than the code parsing the domain
value, which I considered agreed upon.
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > + int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > + if (domain >= 0) {
> > + dt_domain_found = true;
> > + } else if (dt_domain_found == true) {
> > + dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > + parent->of_node->full_name);
> > + return;
>
> No error other than a printk? Do we want to set domain_nr to an
> illegal value or something?
See above.
> > + } else {
> > + domain = pci_get_new_domain_nr();
> > + }
> > +
> > + bus->domain_nr = domain;
> > +}
>
> This doesn't handle the case where the 1st node(s) probed does not
> have a domain prop and a later one does. I think something like this
> should work:
Well, a boolean won't do, your code below looks ok, I can add a trivial enum
to make use_dt_domains usage clearer (but I think a comment to your code will
do):
enum {
PCI_DOMAIN_INVALID,
PCI_DOMAIN_DT,
PCI_DOMAIN_DEFAULT
};
> static int use_dt_domains = -1;
> int domain = of_get_pci_domain_nr(parent->of_node);
>
> if (domain >= 0 && use_dt_domains) {
> use_dt_domains = 1;
> } else if (domain < 0 && use_dt_domain != 1) {
> use_dt_domains = 0;
> domain = pci_get_new_domain_nr();
> } else {
> dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\"
> property in DT\n",
> parent->of_node->full_name);
> domain = -1; // Not sure if -1 would be illegal or not????
I have to check, that's a valid point, we can't certainly leave it as it is.
Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
