[U-Boot] [PATCH 05/18] dm: pci: Add support for PCI driver matching
Simon Glass
sjg at chromium.org
Thu Jul 23 01:40:49 CEST 2015
Hi Bin,
On 22 July 2015 at 00:05, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Jul 22, 2015 at 6:00 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 21 July 2015 at 10:12, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Tue, Jul 7, 2015 at 6:47 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> At present all PCI devices must be present in the device tree in order to
>>>> be used. Many or most PCI devices don't require any configuration other than
>>>> that which is done automatically by U-Boot. It is inefficent to add a node
>>>> with nothing but a compatible string in order to get a device working.
>>>>
>>>> Add a mechanism whereby PCI drivers can be declared along with the device
>>>> parameters they support (vendor/device/class). When no suitable driver is
>>>> found in the device tree the list of such devices is consulted to determine
>>>> the correct driver. If this also fails, then a generic driver is used as
>>>> before.
>>>>
>>>> The mechanism used is very similar to that provided by Linux and the header
>>>> file defintions are copied from Linux 4.1.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>> drivers/pci/pci-uclass.c | 129 ++++++++++++++++++++++++++++++++++++++++++-----
>>>> include/pci.h | 79 ++++++++++++++++++++++++++++-
>>>> 2 files changed, 193 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>>> index 5b91fe3..41daa0d 100644
>>>> --- a/drivers/pci/pci-uclass.c
>>>> +++ b/drivers/pci/pci-uclass.c
>>>> @@ -353,6 +353,101 @@ int dm_pci_hose_probe_bus(struct pci_controller *hose, pci_dev_t bdf)
>>>> return sub_bus;
>>>> }
>>>>
>>>> +/**
>>>> + * pci_match_one_device - Tell if a PCI device structure has a matching
>>>> + * PCI device id structure
>>>> + * @id: single PCI device id structure to match
>>>> + * @dev: the PCI device structure to match against
>>>> + *
>>>> + * Returns the matching pci_device_id structure or %NULL if there is no match.
>>>> + */
>>>> +static bool pci_match_one_id(const struct pci_device_id *id,
>>>> + const struct pci_device_id *find)
>>>> +{
>>>> + if ((id->vendor == PCI_ANY_ID || id->vendor == find->vendor) &&
>>>> + (id->device == PCI_ANY_ID || id->device == find->device) &&
>>>> + (id->subvendor == PCI_ANY_ID || id->subvendor == find->subvendor) &&
>>>> + (id->subdevice == PCI_ANY_ID || id->subdevice == find->subdevice) &&
>>>> + !((id->class ^ find->class) & id->class_mask))
>>>> + return true;
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> +/**
>>>> + * pci_find_and_bind_driver() - Find and bind the right PCI driver
>>>> + *
>>>> + * This only looks at certain fields in the descriptor.
>>>> + */
>>>> +static int pci_find_and_bind_driver(struct udevice *parent,
>>>> + struct pci_device_id *find_id, int devfn,
>>>> + struct udevice **devp)
>>>> +{
>>>> + struct pci_driver_entry *start, *entry;
>>>> + const char *drv;
>>>> + int n_ents;
>>>> + int ret;
>>>> + char name[30], *str;
>>>> +
>>>> + *devp = NULL;
>>>> +
>>>> + debug("%s: Searching for driver: vendor=%x, device=%x\n", __func__,
>>>> + find_id->vendor, find_id->device);
>>>> + start = ll_entry_start(struct pci_driver_entry, pci_driver_entry);
>>>> + n_ents = ll_entry_count(struct pci_driver_entry, pci_driver_entry);
>>>> + for (entry = start; entry != start + n_ents; entry++) {
>>>> + const struct pci_device_id *id;
>>>> + struct udevice *dev;
>>>> + const struct driver *drv;
>>>> +
>>>> + for (id = entry->match;
>>>> + id->vendor || id->subvendor || id->class_mask;
>>>> + id++) {
>>>> + if (!pci_match_one_id(id, find_id))
>>>> + continue;
>>>> +
>>>> + drv = entry->driver;
>>>> + /*
>>>> + * We could pass the descriptor to the driver as
>>>> + * platdata (instead of NULL) and allow its bind()
>>>> + * method to return -ENOENT if it doesn't support this
>>>> + * device. That way we could continue the search to
>>>> + * find another driver. For now this doesn't seem
>>>> + * necesssary, so just bind the first match.
>>>> + */
>>>> + ret = device_bind(parent, drv, drv->name, NULL, -1,
>>>> + &dev);
>>>> + if (ret)
>>>> + goto error;
>>>> + debug("%s: Match found: %s\n", __func__, drv->name);
>>>> + dev->driver_data = find_id->driver_data;
>>>> + *devp = dev;
>>>> + return 0;
>>>> + }
>>>> + }
>>>> +
>>>> + /* Bind a generic driver so that the device can be used */
>>>> + sprintf(name, "pci_%x:%x.%x", parent->seq, PCI_DEV(devfn),
>>>> + PCI_FUNC(devfn));
>>>> + str = strdup(name);
>>>> + if (!str)
>>>> + return -ENOMEM;
>>>> + drv = (find_id->class >> 8) == PCI_CLASS_BRIDGE_PCI ? "pci_bridge_drv" :
>>>> + "pci_generic_drv";
>>>> + ret = device_bind_driver(parent, drv, str, devp);
>>>> + if (ret) {
>>>> + debug("%s: Failed to bind generic driver: %d", __func__, ret);
>>>> + return ret;
>>>> + }
>>>> + debug("%s: No match found: bound generic driver instead\n", __func__);
>>>> +
>>>> + return 0;
>>>> +
>>>> +error:
>>>> + debug("%s: No match found: error %d\n", __func__, ret);
>>>> + return ret;
>>>> +}
>>>> +
>>>> int pci_bind_bus_devices(struct udevice *bus)
>>>> {
>>>> ulong vendor, device;
>>>> @@ -387,25 +482,33 @@ int pci_bind_bus_devices(struct udevice *bus)
>>>> bus->seq, bus->name, PCI_DEV(devfn), PCI_FUNC(devfn));
>>>> pci_bus_read_config(bus, devfn, PCI_DEVICE_ID, &device,
>>>> PCI_SIZE_16);
>>>> - pci_bus_read_config(bus, devfn, PCI_CLASS_DEVICE, &class,
>>>> - PCI_SIZE_16);
>>>> + pci_bus_read_config(bus, devfn, PCI_CLASS_REVISION, &class,
>>>> + PCI_SIZE_32);
>>>> + class >>= 8;
>>>>
>>>> /* Find this device in the device tree */
>>>> ret = pci_bus_find_devfn(bus, devfn, &dev);
>>>>
>>>> + /* Search for a driver */
>>>> +
>>>> /* If nothing in the device tree, bind a generic device */
>>>> if (ret == -ENODEV) {
>>>> - char name[30], *str;
>>>> - const char *drv;
>>>> -
>>>> - sprintf(name, "pci_%x:%x.%x", bus->seq,
>>>> - PCI_DEV(devfn), PCI_FUNC(devfn));
>>>> - str = strdup(name);
>>>> - if (!str)
>>>> - return -ENOMEM;
>>>> - drv = class == PCI_CLASS_BRIDGE_PCI ?
>>>> - "pci_bridge_drv" : "pci_generic_drv";
>>>> - ret = device_bind_driver(bus, drv, str, &dev);
>>>> + struct pci_device_id find_id;
>>>> + ulong val;
>>>> +
>>>> + memset(&find_id, '\0', sizeof(find_id));
>>>> + find_id.vendor = vendor;
>>>> + find_id.device = device;
>>>> + find_id.class = class;
>>>> + if ((header_type & 0x7f) == PCI_HEADER_TYPE_NORMAL) {
>>>> + pci_bus_read_config(bus, devfn,
>>>> + PCI_SUBSYSTEM_VENDOR_ID,
>>>> + &val, PCI_SIZE_32);
>>>> + find_id.subvendor = val & 0xffff;
>>>> + find_id.subdevice = val >> 16;
>>>> + }
>>>> + ret = pci_find_and_bind_driver(bus, &find_id, devfn,
>>>> + &dev);
>>>> }
>>>> if (ret)
>>>> return ret;
>>>> diff --git a/include/pci.h b/include/pci.h
>>>> index 3af511b..d21fa8b 100644
>>>> --- a/include/pci.h
>>>> +++ b/include/pci.h
>>>> @@ -468,7 +468,10 @@ typedef int pci_dev_t;
>>>> #define PCI_ANY_ID (~0)
>>>>
>>>> struct pci_device_id {
>>>> - unsigned int vendor, device; /* Vendor and device ID or PCI_ANY_ID */
>>>> + unsigned int vendor, device; /* Vendor and device ID or PCI_ANY_ID */
>>>> + unsigned int subvendor, subdevice; /* Subsystem ID's or PCI_ANY_ID */
>>>> + unsigned int class, class_mask; /* (class,subclass,prog-if) triplet */
>>>> + unsigned long driver_data; /* Data private to the driver */
>>>> };
>>>
>>> Can we create another structure for dm? There are lots of codes which
>>> only defines vendor id and device id like below:
>>>
>>> static struct pci_device_id mmc_supported[] = {
>>> { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_SDIO_0 },
>>> { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_SDIO_1 },
>>> };
>>>
>>> Although compiler does not generate any error or warning, it is confusing.
>>
>> I think the existing structure is for exactly this purpose, - I have
>> just added a few new fields. Why do we want to change it? I also
>> really want to use this name as it matches Linux.
>>
>
> I mean it creates confusion if existing codes are unchanged, which
> makes me think it contains only a vendor id and a device id for each
> structure, but actually it has more members which are omitted. I would
> do:
>
> static struct pci_device_id mmc_supported[] = {
> { .vendor = PCI_VENDOR_ID_INTEL, .device =
> PCI_DEVICE_ID_INTEL_TCF_SDIO_0 },
> { .vendor = PCI_VENDOR_ID_INTEL, .device =
> PCI_DEVICE_ID_INTEL_TCF_SDIO_1 },
>
> And one more comment at the end.
OK I see. Well I can do this as a follow-on patch.
>
>>>
>>>>
>>>> struct pci_controller;
>>>> @@ -1110,7 +1113,79 @@ struct dm_pci_emul_ops {
>>>> int sandbox_pci_get_emul(struct udevice *bus, pci_dev_t find_devfn,
>>>> struct udevice **emulp);
>>>>
>>>> -#endif
>>>> +/**
>>>> + * PCI_DEVICE - macro used to describe a specific pci device
>>>> + * @vend: the 16 bit PCI Vendor ID
>>>> + * @dev: the 16 bit PCI Device ID
>>>> + *
>>>> + * This macro is used to create a struct pci_device_id that matches a
>>>> + * specific device. The subvendor and subdevice fields will be set to
>>>> + * PCI_ANY_ID.
>>>> + */
>>>> +#define PCI_DEVICE(vend, dev) \
>>>> + .vendor = (vend), .device = (dev), \
>>>> + .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
>>>> +
>>>> +/**
>>>> + * PCI_DEVICE_SUB - macro used to describe a specific pci device with subsystem
>>>> + * @vend: the 16 bit PCI Vendor ID
>>>> + * @dev: the 16 bit PCI Device ID
>>>> + * @subvend: the 16 bit PCI Subvendor ID
>>>> + * @subdev: the 16 bit PCI Subdevice ID
>>>> + *
>>>> + * This macro is used to create a struct pci_device_id that matches a
>>>> + * specific device with subsystem information.
>>>> + */
>>>> +#define PCI_DEVICE_SUB(vend, dev, subvend, subdev) \
>>>> + .vendor = (vend), .device = (dev), \
>>>> + .subvendor = (subvend), .subdevice = (subdev)
>>>> +
>>>> +/**
>>>> + * PCI_DEVICE_CLASS - macro used to describe a specific pci device class
>>>> + * @dev_class: the class, subclass, prog-if triple for this device
>>>> + * @dev_class_mask: the class mask for this device
>>>> + *
>>>> + * This macro is used to create a struct pci_device_id that matches a
>>>> + * specific PCI class. The vendor, device, subvendor, and subdevice
>>>> + * fields will be set to PCI_ANY_ID.
>>>> + */
>>>> +#define PCI_DEVICE_CLASS(dev_class, dev_class_mask) \
>>>> + .class = (dev_class), .class_mask = (dev_class_mask), \
>>>> + .vendor = PCI_ANY_ID, .device = PCI_ANY_ID, \
>>>> + .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
>>>> +
>>>> +/**
>>>> + * PCI_VDEVICE - macro used to describe a specific pci device in short form
>>>> + * @vend: the vendor name
>>>> + * @dev: the 16 bit PCI Device ID
>>>> + *
>>>> + * This macro is used to create a struct pci_device_id that matches a
>>>> + * specific PCI device. The subvendor, and subdevice fields will be set
>>>> + * to PCI_ANY_ID. The macro allows the next field to follow as the device
>>>> + * private data.
>>>> + */
>>>> +
>>>> +#define PCI_VDEVICE(vend, dev) \
>>>> + .vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
>>>> + .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0
>>>> +
>>>> +/**
>>>> + * struct pci_driver_entry - Matches a driver to its pci_device_id list
>>>> + * @driver: Driver to use
>>>> + * @match: List of match records for this driver, terminated by {}
>>>> + */
>>>> +struct pci_driver_entry {
>>>> + struct driver *driver;
>>>> + const struct pci_device_id *match;
>>>> +};
>>>> +
>>>> +#define U_BOOT_PCI_DEVICE(__name, __match) \
>>>> + ll_entry_declare(struct pci_driver_entry, __name, pci_driver_entry) = {\
>>>> + .driver = llsym(struct driver, __name, driver), \
>>>> + .match = __match, \
>>>> + }
>>>> +
>>>> +#endif /* CONFIG_DM_PCI */
>>>>
>>>> #endif /* __ASSEMBLY__ */
>>>> #endif /* _PCI_H */
>>>> --
>>>
>>> Do you have plan to address the issue that dm pci cannot be used in
>>> the pre-reloc phase? Like we need pci uart as the U-Boot console.
>>
>
> Do you have plan to address the issue that dm pci cannot be used in
> the pre-reloc phase? Like we need pci uart as the U-Boot console.
I suspect it can if we put it in the device tree. Is that good enough?
Applied to u-boot-dm.
Regards,
Simon
More information about the U-Boot
mailing list