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

[Chris Johns]

On 17/2/21 4:54 pm, Sebastian Huber wrote:
> On 17/02/2021 03:20, Chris Johns wrote:
> 
>> I have looked into this some more with Joel. Comments below ..
>>
>> On 16/2/21 5:49 pm, Chris Johns wrote:
>>> 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?
>> I have figured this out. We can avoid tags (for now).
> Good, the day will come when we need the tags.
>>
>>> FYI some testing with the zynq in qemu results in the PHY failing to initialise.
>>> It will need a closer look.
>> I am yet to see what the issue was here.
>>
>>>> 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.
>> Given the defined resources I am not sure you fully mix things without using
>> tags. As I will discuss below endian issues with a specific bus can be a reason.
>>
>>>> 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.
>> I am coming the conclusion the current bus.h is not suitable for the PowerPC
>> architecture in general as is. There maybe younger devices with more advanced
>> bus structures optimised for a specific niche however we also need to balance
>> the fact we have decades of proven operation in important systems we need to
>> maintain. Some how we need to find a path all LibBSD PowerPC users are OK with.
> 
> The PowerPC architecture is quite old and well designed. I am pretty sure that
> the Cache-Inhibited and Guarded memory is available in all PowerPC systems we
> support.
> 
>>
>> We need to look at using the `eieio` instruction. I am reluctant to stop using
>> it even if testing shows it is OK. These systems are older PowerPC systems and
>> playing around at the low level after all this time would concern me.
> I didn't found a use of eieio in the FreeBSD bus support. Where have you seen it
> in FreeBSD?
>>
>> The factor that has come to light with my testing is the PCI bus and devices are
>> in LE and the PowerPC is BE. This means a suitability sized volatile pointer to
>> memory does not work. With the VME PowerPC boards I think we are OK with LibBSD
>> as the devices it will interact with will all be PCI and we force all IO to be
>> LE.
> 
> Yes, the endian conversion is an issue. For the NVMe support I hacked it like this:
> 
> https://git.rtems.org/rtems-libbsd/commit/?id=aaeae61bd097db64e9f3c0c8f67de768887197e5
> 
> 
> https://git.rtems.org/rtems-libbsd/commit/?id=cdbae21e4d55c01ce9a3db98443ab315e011e760
> 
> 
> This is definitely not a perfect solution, however, it worked without changing
> the bus space implementation.

I did wonder about this but it does not resolve the `eieio` instruction.

>>>> 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.
>> We need to decide if we let the BSP define a set of calls that can be used. This
>> would mean including `bsp.h` in `bus.h` as I have done. I would rework the
>> implementation to default to a volatile memory pointer if the BSP has not
>> provided a BSP specific replacement.
>>
>> Another alternative is to have a PowerPC specific `bus.h` and then basically do
>> the same thing only with the LE and BE present in that file then somehow
>> figuring out how to select the one needed.
> There are definitely reasons, why the FreeBSD bus space implementation is a bit
> more complicated. For me it would be all right to let BSPs optionally enable the
> full featured bus space implementation ported from FreeBSD. This would be a bit
> of work.

Yes and currently not needed considering the changes I have made in v2 (should
have been v3) of the patch I posted. Those changes also fix a few bugs in some
of the calls.

>> I prefer the first option with the increased build time due to including bsp.h,
>> rtems.h etc.
> I am not concerned about the increased build time. The <bsp.h> and <rtems.h>
> define all sorts of stuff. I would rather use a new BSP provided header file for
> this, e.g. <bsp/bus.h>.

I think this is a good idea and worth sorting out now. It will require a
configure test to enable the include. I am not sure when I will get back to this.

Chris