Hi Bjorn,
[Adding rmk]
On Wed, Feb 12, 2014 at 01:06:50AM +0000, Bjorn Helgaas wrote:
> On Tue, Feb 04, 2014 at 04:53:02PM +0000, Will Deacon wrote:
> > This patch moves bios32 over to using the generic code for enabling PCI
> > resources. All that's left to take care of is the case of PCI bridges,
> > which need to be enabled for both IO and MEMORY, regardless of the
> > resource types.
> >
> > A side-effect of this change is that we no longer explicitly enable
> > devices when running in PCI_PROBE_ONLY mode. This stays closer to the
> > meaning of the option and prevents us from trying to enable devices
> > without any assigned resources (the core code refuses to enable
> > resources without parents).
>
> Heh, I've been looking at this, too :) I have a similar patch for ARM and
> other architectures with their own versions of pcibios_enable_device().
>
> Several of them (arm m68k tile tegra unicore32) have this special code that
> enables IO and MEMORY for bridges unconditionally. But from a PCI
> perspective, I don't know why we should do this unconditionally. If a
> bridge doesn't have an enabled MEM window or MEM BAR, I don't think we
> should have to enable PCI_COMMAND_MEMORY for it.
>
> The architectures that do this only check for valid MEM BARs, i.e., they
> only look at resources 0-5, and they don't look at the window resources.
Ok, so they would miss the bridge resources, which would explain the
additional step to enable both IO and MEM unconditionally.
> The architectures that don't enable IO and MEMORY for bridges
> unconditionally check *all* the resources up to PCI_NUM_RESOURCES, which
> includes the BARs, bridge windows, and any IOV resources.
>
> The generic pci_enable_resources() does check all the resources, so I
> *think* it should be safe for all architectures to use that and just drop
> the check for bridges. What do you think?
Right, your explanation certainly makes sense to me: if
pci_enable_resources() enables bridge resources, then there's no reason for
the extra logic in the caller.
The problem is, I don't have a platform to test our theory. I've added
Russell, since he might have a relevant ARM platform where we could test our
change.
Will
> > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> > index 317da88ae65b..9f3c76638689 100644
> > --- a/arch/arm/kernel/bios32.c
> > +++ b/arch/arm/kernel/bios32.c
> > @@ -608,40 +608,25 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> > */
> > int pcibios_enable_device(struct pci_dev *dev, int mask)
> > {
> > - u16 cmd, old_cmd;
> > - int idx;
> > - struct resource *r;
> > + int err;
> > + u16 cmd;
> >
> > - pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > - old_cmd = cmd;
> > - for (idx = 0; idx < 6; idx++) {
> > - /* Only set up the requested stuff */
> > - if (!(mask & (1 << idx)))
> > - continue;
> > + if (pci_has_flag(PCI_PROBE_ONLY))
> > + return 0;
> >
> > - r = dev->resource + idx;
> > - if (!r->start && r->end) {
> > - printk(KERN_ERR "PCI: Device %s not available because"
> > - " of resource collisions\n", pci_name(dev));
> > - return -EINVAL;
> > - }
> > - if (r->flags & IORESOURCE_IO)
> > - cmd |= PCI_COMMAND_IO;
> > - if (r->flags & IORESOURCE_MEM)
> > - cmd |= PCI_COMMAND_MEMORY;
> > - }
> > + err = pci_enable_resources(dev, mask);
> > + if (err)
> > + return err;
> >
> > /*
> > * Bridges (eg, cardbus bridges) need to be fully enabled
> > */
> > - if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> > + if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) {
> > + pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
> > -
> > - if (cmd != old_cmd) {
> > - printk("PCI: enabling device %s (%04x -> %04x)\n",
> > - pci_name(dev), old_cmd, cmd);
> > pci_write_config_word(dev, PCI_COMMAND, cmd);
> > }
> > +
> > return 0;
> > }
> >
> > --
> > 1.8.2.2
> >
>
--
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
