Attention is currently required from: Nico Huber, Tim Wawrzynczak.
21 comments:
Commit Message:
Patch Set #26, Line 25: meet the FSPs rules
nit: I think most(all but 1?) rules are what the BWG requires.
File src/soc/intel/common/block/include/intelblocks/irq.h:
Patch Set #26, Line 14: irq_constraint_type
Can a single device have multiple constraints? e.g. FIXED_INT_PIN and IDENTITY? or FIXED_INT_PIN and UNIQUE_IRQ?
Patch Set #26, Line 26: IDENTITY
Haven't read rest of the CL yet, but it is not very clear from the comment above how `IDENTITY` is different than PIRQ?
Patch Set #26, Line 30: pci_devfn_t
Shouldn't this be `unsigned int`?
Patch Set #26, Line 32: pci_pin
Is this set only in case of `FIXED_INT_PIN`?
Patch Set #26, Line 40: pci_devfn_t
`unsigned int`?
File src/soc/intel/common/block/irq/irq.c:
Patch Set #26, Line 13: MIN_IRQ
Should this be `MIN_SHARED_IRQ` so that it is consistent with other _SHARED_IRQ macros below?
#define TOTAL_IRQ (MAX_IRQ - MIN_IRQ + 1)
#define MAX_IRQ_ENTRIES 120
These macros are unused. Are these required?
Patch Set #26, Line 21: MAX_PINS
Is this the same as PCI_INT_MAX?
Shouldn't this be ==?
Instead of determining the pin state here and in `find_shareable_pin`, do you think if it would make sense to have another structure tracking the pin state:
```
struct pin_info {
enum pin_state {
FREE_PIN,
SHARED_PIN,
UNIQUE_IRQ_PIN,
} pin_state;
int usage_count;
bool identity_map;
int irq;
} pin_info[MAX_PINS];
```
It is basically just combining the information you are already tracking/using in different places:
It helps with a few things:
1. Don't need to calculate pin state (used/free) or usage count every time the function is called.
2. Don't need to track pin_irq_map and MAX_SHARED_IRQ to determine if a pin is being used for unique irq. Makes couple of checks in the `find_shareable_pin()` cleaner.
3. With #2 addressed, I think we can have a clean split of pin allocation followed by irq allocation (probably makes it easier to read?):
```
assign_slot()
{
assign_pins(); // Assigns pin to each valid function. Generates pin_info and fn_pin_map
assign_irqs(); // Assigns IRQs to each pin using information from pin_info
add_slot_entries(); // Adds entry for each slot function using fn_pin_map and pin_info
}
```
where
```
assign_pins()
{
assign_fixed_pins();
assign_unique_irq_pins();
assign_shareable_pins();
}
```
I think these functions can also make use of some helper function to set pin_info[] which does the required sanity check.
Patch Set #26, Line 136: unsigned int share_count[TOTAL_SHARED_IRQ] = {0};
Should this be a global array to avoid having to calculate the share_count every time? IIUC, one of the reasons why you are recalculating the share_count every time is because we haven't yet added entries for the current slot. But, if we don't rely on `entries[]` to determine this and instead add a step in `assign_irqs()` to `increment_shared_count()` then share_count will always have the up-to-date information.
```
assign_irqs()
{
// If unique, assign unique irq.
// If shared:
// 1. get least used
// 2. call increment_shared_count()
}
```
Patch Set #26, Line 175: fn_pin_map
I am trying to understand what state `fn_pin_map[]` would be in. By default, you set all functions 0 - 7 as `EMPTY_FN` (0xff) which I think means that the function is not present in the slot.
And then in the loop below, if `constraints[i].type` is `UNKNOWN`, then the function is left as `EMPTY_FN`, else it is set to `PCI_INT_NONE`. So, IIUC, what you want after this function is:
But, the loop below assumes that all functions exist in the order 0 - 7(in the constraints list) and there are no holes in between. I think that assumption is not really clear at the API level. It might be better to check for the function # in the loop and set `fn_pin_map[i]` accordingly.
```
for (i = 0; i < MAX_FNS; i++) {
func = PCI_FUNC(constraints[i].devfn);
if (constraints[i].type != UNKNOWN)
fn_pin_map[func] = PCI_INT_NONE;
...
}
```
BTW, is there any advantage of using `EMPTY_FN`? I don't really see it getting used anywhere outside of this function. Can we simply initialize `fn_pin_map` to `PCI_INT_NONE`? Ideally, there should not be any slots that are all empty in the constraint table when we are in this function. You can use the slot count(as I mentioned on line 342) to ensure that.
Patch Set #26, Line 198: constraints[i].type == FIXED_INT_PIN || constraints[i].type == IDENTITY
I wonder if the constraints type should be a bit-field so that multiple can be set e.g. `FIXED_INT_PIN` and `IDENTITY`. `FIXED_INT_PIN` indicates that a fixed pin is allocated for the slot-function. `IDENTITY` indicates that the pin is mapped 1:1 to PIRQ #. Then, we don't need to make the assumption that IDENTITY implies fixed pin allocation.
Also, do we need to handle cases like a `FIXED_INT_PIN` also requires a `UNIQUE_IRQ`?
This needs some more thought because I understand that having multiple bits set at the same time also requires more paths to handle in `assign_pins()`. Probably, will require bits in the same order as we will process them and use something like `fls()` to properly handle it.
Patch Set #26, Line 200: return false;
printk(BIOS_ERR, ""); to indicate that the SoC constraint is incorrect.
Same comment as above. This will have to be PCI_FUNC(...)
Patch Set #26, Line 208: (pin - PCI_INT_A)
PIN2IDX?
Patch Set #26, Line 214: fn_pin_map[0] = PCI_INT_A;
Should there be a check to ensure that fn_pin_map[0] is not already assigned to something other than PCI_INT_A?
Patch Set #26, Line 226: if (constraints[i].type == UNIQUE_IRQ) {
If you invert this check, additional tab on rest of the lines can be avoided.
```
if (constraints[i].type != UNIQUE_IRQ)
continue;
```
Patch Set #26, Line 337: entry_count
Should `entry_count` be initialized to -1 by default? If there is a failure in `assign_slot()`, `entry_count` can be set to 0 and we don't have to go through the assignment functions again since it is anyways going to fail.
Patch Set #26, Line 342: soc_irq_constraints
Since `assign_pci_irqs()` is called by SoC code, can we accept this list as an input parameter rather than making a callback into SoC for getting the IRQ constraints? Also, I think you will need a `count` parameter to indicate the number of slots that the SoC has provided in the constraints list.
To view, visit change 49408. To unsubscribe, or for help writing mail filters, visit settings.