Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41553 )
Change subject: device/pci: Don't enabled unassigned resources ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41553/1/src/device/pci_device.c@575 PS1, Line 575: for (res = dev->resource_list; res; res = res->next) : pci_set_resource(dev, res, &command_mask); : /* If there are unassigned resources, we might : have to disable others of the same type. */ : dev->command &= command_mask; :
I am thinking if we should just split bridge and non-bridge resource setting completely. […]
Agreed. We need to be careful about the command bits. Having a helper function get_command_bits(res) can return the right mask to set / clear.
1) Bridge --> a) Always set the mask using get_command_bits(res) 2) Non-bridge --> Get mask using get_command_bits(res), a) Set if resource is valid, b) Clear if resource is invalid and exit.
Hence, the order above is important where bridge resources are considered first and then the non-bridge resources.
With that the 4 cases would be: * No resources => don't set command bit ==> Never sets any mask for the resource type * Any non-bridge resource is invalid => don't set command bit ==> 2b * All resources are valid => set command bit ==> 2a * All non-bridge resources are valid, but a bridge one is not => we can disable bridge resources by programming limit < base, and set command bit ==> 1a + 2a