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

Wed Feb 17 05:54:29 UTC 2021

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.

>
>>> 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.
>
> 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>.
>
> Chris

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/