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

[Mark Cave-Ayland]

On 16/07/17 16:49, BALATON Zoltan wrote:

> On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
>> On 16/07/17 14:56, BALATON Zoltan wrote:
>>>> 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?
>>
>> For SPARC64 the issue is with pci_mem_base and how it is interpreted in
>> Linux. Before the recent set of patches, the .childaddr for
>> MEMORY_SPACE_32 was generated as APB_MEM_BASE + pci_mem_base, so in
>> effect all we do is miss mapping the first PCI memory section.
>>
>> However in Linux the kernel calculates the address of the host range is
>> by taking the value of an undocumented register in the Simba host bridge
>> (see simba_config_cb() for details) and then adding the offsets on top
>> meaning that you end up with all your addresses out by pci_mem_base.
>>
>> Hence the need to apply pci_mem_base as the starting value for assigning
>> devices to PCI memory space, but not including it in the host ranges
>> itself.
> 
> So does that mean we could theoretically get rid of all the other
> redundant data and put this pci_mem_base offset for SPARC64 in some
> #ifdefed code part (or in the undocumented register to counter the
> offset in Linux) to simplify the current situation where the ranges are
> defined in two different ways?

There was some code for config space registers already #ifdef'd for
SPARC64 but the issue there was that with so many conditionals used for
building up the PCI host bridge ranges that it was very difficult to see
at a glance what the resulting values were.

Bear in mind that PPC also had its own hacks in that it has multiple IO
memory spaces listed for the host bridge, so rather than keep going down
this maze of conditional logic and #ifdef, I decided to explicitly list
the values themselves which solves all the problems in one go.

If you wanted to better document the relationships between the host
ranges properties and the corresponding values in pci_arch_t then you
could use some #defines, but then if you're working down at that level
regardless then I suspect this will be the least of your problems.

> I was also thinking if our problems come from trying to make a single
> driver to manage all the different pci hosts? Are these similar enough
> for this to work or should it be really split into at least three files:
> generic pci stuff, Apple host bus and SPARC host bus? Would that make it
> clearer? There seems to be a lot of #ifdefed code in the pci driver now
> which suggests maybe these should be really separate drivers and only
> keep the generic stuff in pci.c.

I think that's part of it, but the specification is lacking in some
areas. For example I couldn't figure out from the specs I had whether
configuration space should be included in the PCI host bridge ranges -
SPARC64 requires it, whereas PPC panics several (Apple) OSs if they are
included.

Yes you could argue it would be better to split things into separate
files but that's quite far down the priority list at the moment. If
people were constantly adding new architectures to OpenBIOS I could see
the benefit, but I don't see that ever changing.

>>>> 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.
>>
>> I'd say you don't need a deep understanding of PCI to at least rework
>> the pci_arch_t into an array of structs representing each host bridge to
>> see what happens, but as I mentioned above I believe the bulk of the
>> support should now be there.
>>
>> Unfortunately I don't have much in the way of time to look at this now,
>> but I'm happy to point you in the right direction and reply to any
>> questions you might have. At least that's probably an easier option than
>> sitting down trying to read the PCI specification ;)
> 
> Same problem here with time and I don't feel like wanting to debug this
> without understanding how it should work so I pass this for now. If I
> make any progress I'll let you know. (I think I should at least
> understand the mappings and address translations and their
> representation in OpenBIOS to know what am I doing but I don't have that
> knowledge.)

Well I'm sure I'll still be around when you do manage to work on it ;)

But I do highly recommend posting questions and RFC patches to the list,
as there are people on the mailing list other than myself who know a lot
more about PCI than I do.


ATB,

Mark.