[6-freebsd-12 PATCH 1/2] rtemsbsd/bus: Add PCI support to the nexus bus

[Chris Johns]

Hi Sebastian,

Thank you for the review.

On 16/2/21 5:05 pm, Sebastian Huber wrote:
> 
> On 16/02/2021 03:02, chrisj at rtems.org wrote:
>> +/*
>> + * Values for the bus space tag, not to be used directly by MI code.
>> + */
>> +#define    BSP_BUS_SPACE_IO    0    /* space is i/o space */
>> +#define    BSP_BUS_SPACE_MEM    1    /* space is mem space */
>> +
>> +/*
>> + * BSP PCI Support
>> + *
>> + * The RTEMS Nexus bus support can optionaly support PCI spaces that
>> + * mapped to BSP speciic address spaces. Add the following define to
>> + * the BSP header file to enable this support:
>> + *
>> + *  #define BSP_HAS_PCI
>> + *
>> + * If enabled a BSP must the following IO region calls:
>> + *
>> + * inb  : read 8 bits
>> + * outb : write 8 bits
>> + * inw  : read 16 bits
>> + * outw : write 16 bits
>> + * inl  : read 32 bits
>> + * outl : write 32 bits
>> + *
>> + * The BSP needs to provide the DRAM address space offset
>> + * PCI_DRAM_OFFSET. This is the base address of the DRAM as seen by a
>> + * PCI master.
> Why is it not possible to account for this offset in the bus_space_handle_t bsh?

How do I do that?

FYI some testing with the zynq in qemu results in the PHY failing to initialise.
It will need a closer look.

> I thought the purpose of the bus_space_tag_t bst was to select different
> instructions to access the memory space.

Instructions or a mix of buses? I am still learning my way around this bus
design but I would have thought tags can handle specific discontinuous buses
mixed into the same linear address space. That is the drivers offsets are all
relative to a specific bus. The PCI reports addresses that are relative to that
buses base address and this is what PCI drivers use.

> The generic file is for systems with memory mapped access only.

The bus.h is per arch so if I specialise it for the PowerPC all BSPs in the
PowerPC arch come across and in the end it becomes the same thing. I cannot see
a way specialise this per BSP and I am not sure it is needed.

>> + *
>> + * i386 BSPs have a special bus.h file and do not use this file.
>> + */
>> +#include <bsp.h>
>> +
>>   /*
>>    * Bus address alignment.
>>    */
>> @@ -144,6 +180,7 @@
>>   /*
>>    * Bus access.
>>    */
>> +#define BUS_SPACE_INVALID_DATA    (~0)
>>   #define BUS_SPACE_UNRESTRICTED    (~0U)
>>     /*
>> @@ -228,29 +265,52 @@ bus_space_barrier(bus_space_tag_t bst __unused,
>> bus_space_handle_t bsh, bus_size
>>    * data is returned.
>>    */
>>   static __inline uint8_t
>> -bus_space_read_1(bus_space_tag_t bst __unused, bus_space_handle_t bsh,
>> bus_size_t ofs)
>> +bus_space_read_1(bus_space_tag_t bst, bus_space_handle_t bsh, bus_size_t ofs)
>>   {
>> +    if (bst == BSP_BUS_SPACE_IO) {
>> +#ifdef BSP_HAS_PCI
>> +        return inb(bsh + ofs);
>> +#else
>> +        return BUS_SPACE_INVALID_DATA;
>> +#endif
>> +    }
> This adds a conditional to all memory mapped generic architectures. From my
> point of view this is a potential code size and performance regression.

As I stated I am not sure it can be specialised for a BSP under the same
architecture. Lets find an agreed solution that works then consider performance.
If we have to find a suitable balance lets see what that is first.

I should also point out this approach is used in the x86 arch and it is used in
some of the most performance intensive applications.

> It also
> includes <bsp.h> and thus <rtems.h> in a file which is included all over the place.

The buf.h header inlines calls which is fine but if we need an offset from a BSP
the BSP header is needed. We only export from a BSP as a single header.

I could not see an easy way to have a BSP indicate it has PCI support in LibBSD.

Chris