[U-Boot-Users] Please explain this code in drivers/pci.c

[U-Boot-Users] Please explain this code in drivers/pci.c

[Donald White]

Group,

Please help me to understand the following code.  It occurs in drivers/pci.c in the 
function pci_hose_config_device().  I have ported U-Boot 0.3.0 and HHL 2.0 to our 
custom MPC8265A board.  It has one device on the PCI bus.  The driver develop was 
wanting a specific memory space allocation for the device.  So I created a PCI 
configuration table with the memory base of 0x90000000.  The card wants 4 regions 0f 
0x8000 bytes in size.  The driver writer thinks this should be 0x90000000, 
0x90008000, 0x90010000 and 0x90018000.  This brings us to the code snippet:

	bar_size = ~(bar_response & PCI_BASE_ADDRESS_MEM_MASK) + 1;
	bar_value = mem;

	mem = ((mem - 1) | (bar_size - 1)) + 1;

The value of bar_response is 0xffff8008 which looks good.  This gives bar size of 
0x8000 which also looks good.  The value of mem is initially 0x90000000.  So,
(mem -  1) = 0x8fffffff and (bar_size - 1) = 0x7fff).  When these are or'd one gets 
0x8fffffff and adding 1 gives 0x90000000.  This result is that the card is given 4 
address bases all at 0x90000000.

Thanks for any help,

Don

[U-Boot-Users] Please explain this code in drivers/pci.c

[Rune Torgersen]

>Subject: [U-Boot-Users] Please explain this code in drivers/pci.c

>The value of bar_response is 0xffff8008 which looks good.  This gives bar
size of 
>0x8000 which also looks good.  The value of mem is initially 0x90000000.
So,
>(mem -  1) = 0x8fffffff and (bar_size - 1) = 0x7fff).  When these are or'd
one gets 
>0x8fffffff and adding 1 gives 0x90000000.  This result is that the card is
given 4 
>address bases all at 0x90000000.


If the card wants 4 different memory regiuons, I assume it has 4 BAR
registers? Each BAR must have the correct base address, so you have to add
the size of the last region to the memory pointer before writing the
address to the next BAR

[U-Boot-Users] Please explain this code in drivers/pci.c

[Donald White]

Rune Torgersen wrote:
> 
>>Subject: [U-Boot-Users] Please explain this code in drivers/pci.c
> 
> 
>>The value of bar_response is 0xffff8008 which looks good.  This gives bar
> 
> size of 
> 
>>0x8000 which also looks good.  The value of mem is initially 0x90000000.
> 
> So,
> 
>>(mem -  1) = 0x8fffffff and (bar_size - 1) = 0x7fff).  When these are or'd
> 
> one gets 
> 
>>0x8fffffff and adding 1 gives 0x90000000.  This result is that the card is
> 
> given 4 
> 
>>address bases all at 0x90000000.
> 
> 
> 
> If the card wants 4 different memory regiuons, I assume it has 4 BAR
> registers? Each BAR must have the correct base address, so you have to add
> the size of the last region to the memory pointer before writing the
> address to the next BAR
> 
> 

I am not sure if you are trying to clarify the question or provide an answer.  My 
complaint is that the code does not do what you say.  I believe the code should 
provide four unique values for the four BARs, but it does not.  Each gets the same 
value because the ORing logic does not succeed in doing a new base = old base + size.

The logic envoked by CONFIG_PCI_PNP gets the calculation right.  If someone is 
familiar with the code and can confirm that it is not behaving as intended, then I 
will do a patch.  The ORing logic looks to me to be copied from the Linux kernel 
where the allocation starts at the top of the address range and proceeds downward, 
but maybe it is an attempt to force alignment.  I don't know.

Don

[U-Boot-Users] Please explain this code in drivers/pci.c

[Donald White]

Udi,

I am convinced now it is incorrect.  A minimal fix is to make the code look like this:

     /* Check the BAR type and set our address mask */
     if (bar_response & PCI_BASE_ADDRESS_SPACE)
     {
         bar_size = ~(bar_response & PCI_BASE_ADDRESS_IO_MASK) + 1;

         /* round up region base address to a multiple of size */
         io = ((io - 1) | (bar_size - 1)) + 1;
         bar_value = io;
         /* compute new base address for region */
         io = io + bar_size;
     }
     else
     {
         if ( (bar_response & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
              PCI_BASE_ADDRESS_MEM_TYPE_64)
             found_mem64 = 1;

         bar_size = ~(bar_response & PCI_BASE_ADDRESS_MEM_MASK) + 1;
         /* round up region base address to multiple of size */
         mem = ((mem - 1) | (bar_size - 1)) + 1;
         bar_value = mem;
         /* compute new region base address */
         mem = mem + bar_size;
     }

The ORing just rounds up the current base for the region to be a multiple of the 
size.  The ORing was in the wrong place and there was no increment of the region 
base to beyond the allocated area.  It worked with cards with single BARs and 
suitable region bases.

This is not optimal, but it is useable for simple cases.

Thanks for the comments,

Don


Udi Finkelstein wrote:
> I've looked at the code you posted, and it looks indeed incorrect. (I 
> didn't look into the entire file in u-boot, so take this with a grain of 
> salt).
> However, and optimal PCI alolcator must sort the BAR size in decreasing 
> orders if it wants to pack the BARs into an area as small as possible.
> If you have BAR0 size 4K and BAR1 size 256M and BAR2 size 4K, then 
> allocating BAR0 from (for example) 0xe0000000 would yield BAR0 range of 
> 0xe0000000-0xe0000fff, BAR1 would be mapped to 0xf0000000 - 0xffffffff, 
> and BAR2 would not fit!
> An optimal allocator must start from the biggest BARs first:
> 
> BAR1 would fit at 0xe0000000-0xefffffff
> BAR2 would fit at 0xf0000000-0xf0ffffff
> BAR0 would fit at 0xf1000000-0xf1000fff
> 
> Udi
> 
> Donald White wrote:
> 
>>
>> Rune Torgersen wrote:
>>
>>>
>>>> Subject: [U-Boot-Users] Please explain this code in drivers/pci.c
>>>
>>>
>>>
>>>
>>>> The value of bar_response is 0xffff8008 which looks good.  This 
>>>> gives bar
>>>
>>>
>>>
>>> size of
>>>
>>>> 0x8000 which also looks good.  The value of mem is initially 
>>>> 0x90000000.
>>>
>>>
>>>
>>> So,
>>>
>>>> (mem -  1) = 0x8fffffff and (bar_size - 1) = 0x7fff).  When these 
>>>> are or'd
>>>
>>>
>>>
>>> one gets
>>>
>>>> 0x8fffffff and adding 1 gives 0x90000000.  This result is that the 
>>>> card is
>>>
>>>
>>>
>>> given 4
>>>
>>>> address bases all at 0x90000000.
>>>
>>>
>>>
>>>
>>>
>>> If the card wants 4 different memory regiuons, I assume it has 4 BAR
>>> registers? Each BAR must have the correct base address, so you have 
>>> to add
>>> the size of the last region to the memory pointer before writing the
>>> address to the next BAR
>>>
>>>
>>
>> I am not sure if you are trying to clarify the question or provide an 
>> answer.  My complaint is that the code does not do what you say.  I 
>> believe the code should provide four unique values for the four BARs, 
>> but it does not.  Each gets the same value because the ORing logic 
>> does not succeed in doing a new base = old base + size.
>>
>> The logic envoked by CONFIG_PCI_PNP gets the calculation right.  If 
>> someone is familiar with the code and can confirm that it is not 
>> behaving as intended, then I will do a patch.  The ORing logic looks 
>> to me to be copied from the Linux kernel where the allocation starts 
>> at the top of the address range and proceeds downward, but maybe it is 
>> an attempt to force alignment.  I don't know.
>>
>> Don
>>
>>
>>
>> -------------------------------------------------------
>> This SF.net email is sponsored by: ObjectStore.
>> If flattening out C++ or Java code to make your application fit in a
>> relational database is painful, don't do it! Check out ObjectStore.
>> Now part of Progress Software. http://www.objectstore.net/sourceforge
>> _______________________________________________
>> U-Boot-Users mailing list
>> U-Boot-Users at lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/u-boot-users
>>
>