Hi Simon, On Wed, Jul 22, 2015 at 6:00 AM, Simon Glass <s...@chromium.org> wrote: > Hi Bin, > > On 21 July 2015 at 10:12, Bin Meng <bmeng...@gmail.com> wrote: >> Hi Simon, >> >> On Tue, Jul 7, 2015 at 6:47 AM, Simon Glass <s...@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 <s...@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. >> >>> >>> 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. Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot