[OpenBIOS] [PATCH] pci: fix BAR setup

[Artyom Tarasenko]

On Thu, Apr 19, 2012 at 10:04 PM, Blue Swirl <blauwirbel at gmail.com> wrote:
> On Thu, Apr 19, 2012 at 19:06, Artyom Tarasenko <atar4qemu at gmail.com> wrote:
>> On Sun, Mar 11, 2012 at 11:59 AM, Blue Swirl <blauwirbel at gmail.com> wrote:
>>> A change in QEMU on how PCI bridges are setup revealed
>>> a bug in OpenBIOS PCI setup. On Sparc64, the BARs just
>>> happened to get somewhat correct values by accident before
>>> the commit but not after the change.
>>>
>>> Don't use arch->io_base for PCI I/O port BARs.
>>>
>>> Fix Sparc64 PCI memory base.
>>
>> Could it be that the ranges property needs to be adjusted to look as
>> it did before the patch?
>>
>> This patch breaks HelenOS-0.4.3/sparc boot. But if I modify the ranges
>> property to the old value, HelenOS boots.
>>
>> 0 > cd /pci at 1fe,0  ok
>> 0 > .properties
>> [...]
>> ranges                    -- 54 : 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 01 fe 01 00 00 00 00 00 00 00 02 00 00 00 01 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 01 fe 02 00 00 00 00 00 00 00 00 01 00 00 02 00
>> 00 00 00 00 00 00 00 10 00 00 00 00 01 ff 00 00 00 00 00 00 00 00 10
>> 00 00 00
>>
>> Not sure about the best formatting. If I reformat it like this:
>>
>> 00000000 00000000 00000000 000001fe 01000000 00000000 02000000
>> 01000000 00000000 00000000 000001fe 02000000 00000000 00010000
>> 02000000 00000000 00100000 000001ff 00000000 00000000 10000000
>>                               ^^^^ this one (the third) used to be 0
>>
>
> On an Ultra-5 this should be
> 00000000.00000000.00000000.000001fe.01000000.00000000.01000000.
> 01000000.00000000.00000000.000001fe.02000000.00000000.01000000.
> 02000000.00000000.00000000.000001ff.00000000.00000001.00000000.
> 03000000.00000000.00000000.000001ff.00000000.00000001.00000000
>
> http://git.kernel.org/?p=linux/kernel/git/davem/prtconfs.git;a=blob;f=ultra5;h=e0300cecd79cdb4ef435272a4bc2c7f758fa4dbd;hb=HEAD#l164
>
> I guess the problem is that the properties are added by finding the
> host bridge in the PCI bus (which btw isn't so nice), so we use the
> pci_mem_base. Using pci_mem_base = 0 would probably fix the BAR, but
> then VGA would get in the way. Probably something more complex is
> needed.

Hmm. Complicated. What is actually the meaning of this property? Don't
we need the values ourselves?
If these are just BARs, can't we pass the configuration from qemu?

>>> Signed-off-by: Blue Swirl <blauwirbel at gmail.com>
>>> ---
>>>
>>> I removed the bridge setup code, it's not really valid for Sparc64 APB
>>> bridges anyway and currently no devices exist behind the bridges (but
>>> that's where they really should be).
>>>
>>> Tested Sparc64 and PPC.
>>>
>>> ---
>>>  arch/sparc64/openbios.c |    2 +-
>>>  drivers/pci.c           |   11 +++++++++--
>>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/sparc64/openbios.c b/arch/sparc64/openbios.c
>>> index ac709fe..ab32a8f 100644
>>> --- a/arch/sparc64/openbios.c
>>> +++ b/arch/sparc64/openbios.c
>>> @@ -64,7 +64,7 @@ static const struct hwdef hwdefs[] = {
>>>             .cfg_base = APB_SPECIAL_BASE,
>>>             .cfg_len = 0x2000000,
>>>             .host_mem_base = APB_MEM_BASE,
>>> -            .pci_mem_base = 0,
>>> +            .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */
>>>             .mem_len = 0x10000000,
>>>             .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space
>>>             .io_len = 0x10000,
>>> diff --git a/drivers/pci.c b/drivers/pci.c
>>> index f8c6414..e270a73 100644
>>> --- a/drivers/pci.c
>>> +++ b/drivers/pci.c
>>> @@ -931,6 +931,7 @@ static void ob_pci_configure_bar(pci_addr addr,
>>> pci_config_t *config,
>>>
>>>         if ((*p_omask & 0x0000000f) == 0x4) {
>>>                 /* 64 bits memory mapping */
>>> +                PCI_DPRINTF("Skipping 64 bit BARs for %s\n", config->path);
>>>                 return;
>>>         }
>>>
>>> @@ -966,11 +967,17 @@ static void ob_pci_configure_bar(pci_addr addr,
>>> pci_config_t *config,
>>>                 size = min_align;
>>>         reloc = (reloc + size -1) & ~(size - 1);
>>>         if (*io_base == base) {
>>> +                PCI_DPRINTF("changing io_base from 0x%lx to 0x%x\n",
>>> +                            *io_base, reloc + size);
>>>                 *io_base = reloc + size;
>>> -                reloc -= arch->io_base;
>>>         } else {
>>> +                PCI_DPRINTF("changing mem_base from 0x%lx to 0x%x\n",
>>> +                            *mem_base, reloc + size);
>>>                 *mem_base = reloc + size;
>>>         }
>>> +        PCI_DPRINTF("Configuring BARs for %s: reloc 0x%x omask 0x%x "
>>> +                    "io_base 0x%lx mem_base 0x%lx size 0x%x\n",
>>> +                    config->path, reloc, *p_omask, *io_base, *mem_base, size);
>>>         pci_config_write32(addr, config_addr, reloc | *p_omask);
>>>         config->assigned[reg] = reloc | *p_omask;
>>>  }
>>> @@ -1260,7 +1267,7 @@ int ob_pci_init(void)
>>>     mem_base = arch->pci_mem_base;
>>>     /* I/O ports under 0x400 are used by devices mapped at fixed
>>>        location. */
>>> -    io_base = arch->io_base + 0x400;
>>> +    io_base = 0x400;
>>>
>>>     bus = 0;
>>>
>>> --
>>> 1.7.9
>>>
>>> --
>>> OpenBIOS                 http://openbios.org/
>>> Mailinglist:  http://lists.openbios.org/mailman/listinfo
>>> Free your System - May the Forth be with you
>>
>>
>>
>> --
>> Regards,
>> Artyom Tarasenko
>>
>> solaris/sparc under qemu blog: http://tyom.blogspot.com/search/label/qemu
>>
>> --
>> OpenBIOS                 http://openbios.org/
>> Mailinglist:  http://lists.openbios.org/mailman/listinfo
>> Free your System - May the Forth be with you
>
> --
> OpenBIOS                 http://openbios.org/
> Mailinglist:  http://lists.openbios.org/mailman/listinfo
> Free your System - May the Forth be with you



-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/search/label/qemu