On Thu, Mar 15, 2012 at 04:29:30PM +1300, Alexey Korolev wrote:
> On 14/03/12 13:48, Kevin O'Connor wrote:
> > On Tue, Mar 13, 2012 at 05:45:19PM +1300, Alexey Korolev wrote:
> >> Added pci_region_entry structure and list operations to pciinit.c
> >> List is filled with entries during pci_check_devices.
> >> List is used just for printing space allocation if we were using lists.
> >> Next step will resource allocation using mapping functions.
[...]
> > - struct {
> > - u32 addr;
> > - u32 size;
> > - int is64;
> > - } bars[PCI_NUM_REGIONS];
[...]
> Yes I see what you mean here.
Thanks - I do find this patch much easier to understand. I do have
some comments on the patch (see below).
> > The code is being changed -
> > it's not new code being added and old code being deleted - the patches
> > need to reflect that.
> Because of structural changes it is not possible to completely avoid
> this scenario where new code is added and old deleted. In this
> patch series I tried my best to make migration as obvious and safe
> as possible. So the existing approach (with your suggestions) for
> pciinit.c redesign is this:
> 1. Introduce list operations
> 2. Introduce pci_region_entry structure and add code which fills this new
> structure.
> We don't modify resource addresses calculations, but we use pci_region_entry
> data to do resource assignment.
> 3. Modify resource addresses calculations to be based on linked lists of
> region entries.
> 4. Remove code which fills the arrays, remove use of arrays for mapping.
> (note 3&4 could be combined altogether but it will be harder to read then)
On patch 3/4, neither patch should introduce code that isn't actually
used nor leave code that isn't used still in. So, for example, if the
arrays are still used after patch 3 then it's okay to leave them to
patch 4, but if they are no longer used at all they should be removed
within patch 3.
> Could you please have a look at the other parts in this series and
> let me know if you are happy about this approach, so I won't have to
> redo patchwork too many times?
patch 1/6 - I'd prefer to have a list.h with struct
hlist_head/hlist_node and container_of before extending to other
parts of seabios. That said, I'm okay with what you have for
pciinit - we can introduce list.h afterwards.
patch 3/4 - was confusing to me as it added new code in one patch and
removed the replaced code in the second patch.
patch 5 - looked okay to me.
Thanks for looking at this - I know it's time consuming. Given the
churn in this area I want to make sure there is a good understanding
before any big changes.
comments on the patch:
[...]
> +struct pci_region_entry *
> +pci_region_create_entry(struct pci_bus *bus, struct pci_device *dev,
> + u32 size, int type, int is64bit)
> +{
> + struct pci_region_entry *entry= malloc_tmp(sizeof(*entry));
> + if (!entry) {
> + warn_noalloc();
> + return NULL;
Minor - whitespace.
[...]
> +static int pci_bios_check_devices(struct pci_bus *busses)
> {
> dprintf(1, "PCI: check devices\n");
> + struct pci_region_entry *entry;
>
> // Calculate resources needed for regular (non-bus) devices.
> struct pci_device *pci;
> @@ -378,19 +419,27 @@ static void pci_bios_check_devices(struct pci_bus
> *busses)
> struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)];
> int i;
> for (i = 0; i < PCI_NUM_REGIONS; i++) {
> - u32 val, size;
> + u32 val, size, min_size;
> + int type, is64bit;
Minor - I prefer to use C99 inline variable declarations though it
isn't a requirement.
> + min_size = (type == PCI_REGION_TYPE_IO) ?
> (1<<PCI_IO_INDEX_SHIFT):
> + (1<<PCI_MEM_INDEX_SHIFT);
> + size = (size > min_size) ? size : min_size;
My only real comment: Why the min_size change? Is that a fix of some
sort or is it related to the move to lists?
[...]
>
> -
> /****************************************************************
> * Main setup code
Minor - whitespace change.
-Kevin
