[OpenBIOS] Adding new device in the middle of device-tree

[BALATON Zoltan]

On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
> On 15/07/17 21:27, BALATON Zoltan wrote:
>> I've noticed some possible redundancy though in the pci arch definition:
>>
>> [ARCH_MAC99] = {
>>     .name = "MAC99",
>>     .vendor_id = PCI_VENDOR_ID_APPLE,
>>     .device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI,
>>     .cfg_addr = 0xf2800000,
>>     .cfg_data = 0xf2c00000,
>>     .cfg_base = 0xf2000000,
>>     .cfg_len = 0x02000000,
>>     .host_pci_base = 0x0,
>>     .pci_mem_base = 0x80000000,
>>     .mem_len = 0x10000000,
>>     .io_base = 0xf2000000,
>>     .io_len = 0x00800000,
>>     .host_ranges = {
>>         { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000,
>> .len = 0x00800000 },
>>         { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr
>> = 0x80000000, .len = 0x10000000 },
>>         { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 }
>>      },
>>     .irqs = { 0x1b, 0x1c, 0x1d, 0x1e }
>> },
>>
>> Aren't pci_mem_base/mem_len and io_base/io_len the same as the IO and
>> MEMORY ranges in host_ranges (and the cfg_* values offsets into the IO
>> space)? Do we need these separately or could it be simplified?
>
> They are for PPC, but unfortunately not for SPARC64. That's why they
> were split out as separate values in the last set of patches because the
> existing logic couldn't handle all cases of multiple address spaces with
> different offsets :(

I don't see why SPARC64 is not also redundant. Here's its pci arch struct:

.pci = {
     .name = "SUNW,sabre",
     .vendor_id = PCI_VENDOR_ID_SUN,
     .device_id = PCI_DEVICE_ID_SUN_SABRE,
     .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space
     .cfg_data = APB_MEM_BASE,                    // PCI bus memory space
     .cfg_base = APB_SPECIAL_BASE,
     .cfg_len = 0x1000000,
     .host_pci_base = APB_MEM_BASE,
     .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */
     .mem_len = 0xf0000000,
     .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space
     .io_len = 0x1000000,
     .host_ranges = {
         { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 },
         { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 },
         { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 },
         { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 }
     },
     .irqs = { 0, 1, 2, 3 },
},

The addresses and lengths here are also mostly the same as in host_ranges. 
Where does the difference come from and could we get rid of the individual 
*_base, *_addr and *_len fields and define everything in the host_ranges 
structure? (Maybe we need some additional offsets in the config space but 
that's all we should need I think.)

It looks like that now the host_ranges are used to create the ranges 
property while the other addresses and lengths are used to create the 
mapping but these two should match (because the ranges property is 
supposed to describe these mappings) so one should be computable from the 
other otherwise we may have a bug (or I'm missing something which is also 
likely as I know nothing about this). Therefore I think it should be 
possible to get rid of one of these. What am I missing?

> As a starting point, why not make the above struct an array of host
> bridges, e.g.
>
>
> [ARCH_MAC99] =
>  [
>   {
>   }, {
>      ....
>   }
>  ]
> },
>
>
> Then remove the "break" from ob_pci_init() and see how you get on.
> You'll need to figure out what to do with the configuration space,
> however with the last set of fix-ups I really don't think there is much
> to do.

Other than understanding a lot of low level pci stuff, how it all worked 
in OpenBIOS before and how it works now after your changes and also the 
pci structure of all architectures... I could not get this in the short 
time I've spent looking at this so I gave up because to me it seems like a 
lot to do at the moment.

Regards,
BALATON Zoltan