[PATCH rtems-libbsd v3 1/3] rtemsbsd/bus: Add PCI support to the nexus bus
Chris Johns
chrisj at rtems.org
Tue Aug 10 02:07:12 UTC 2021
On 10/8/21 1:43 am, Gedare Bloom wrote:
> On Sun, Aug 8, 2021 at 7:22 PM Chris Johns <chrisj at rtems.org> wrote:
>>
>> - Add PCI IO region support
>>
>> - Add support map buffers to PCI address space
>>
>> - Add BSP conditional IO space support. Some PC implementations
>> have PCI IO space mapped differently to memory space and this needs
>> to be reflected in the busspace.
>>
>> - Include bsp.h to pick per BSP configuration.
>>
>> Closes #4245
>> ---
>> rtemsbsd/include/machine/bus.h | 370 ++++++++++++++++++--------
>> rtemsbsd/rtems/rtems-kernel-bus-dma.c | 5 +-
>> rtemsbsd/rtems/rtems-kernel-nexus.c | 52 +++-
>> 3 files changed, 312 insertions(+), 115 deletions(-)
>>
>> diff --git a/rtemsbsd/include/machine/bus.h b/rtemsbsd/include/machine/bus.h
>> index 2f0e7ad6..999f5d45 100644
>> --- a/rtemsbsd/include/machine/bus.h
>> +++ b/rtemsbsd/include/machine/bus.h
>> @@ -6,9 +6,13 @@
>> * @brief TODO.
>> *
>> * File origin from FreeBSD 'sys/amd64/include/bus.h'.
>> + *
>> + * Conditionally supports PCI IO regions (IO Ports).
>> */
>>
>> /*-
>> + * Copyright (c) 2021 Chris Johns. All rights reserved.
>> + *
>> * Copyright (c) 2009, 2015 embedded brains GmbH. All rights reserved.
>> *
>> * embedded brains GmbH
>> @@ -25,7 +29,7 @@
>> * Redistribution and use in source and binary forms, with or without
>> * modification, are permitted provided that the following conditions
>> * are met:
>> - *
>> + *
>> * 1. Redistributions of source code must retain the above copyright
>> * notice, this list of conditions and the following disclaimer as
>> * the first lines of this file unmodified.
>> @@ -34,7 +38,7 @@
>> * documentation and/or other materials provided with the distribution.
>> * 3. The name of the author may not be used to endorse or promote products
>> * derived from this software without specific prior written permission.
>> - *
>> + *
>> * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
>> * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
>> * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
>> @@ -123,9 +127,46 @@
>> #endif
>>
>> #ifdef __i386__
>> - #error "your include paths are wrong"
>> + #error "x86 has its own bus.h; check your include paths are correct"
>> #endif
>>
>> +#include <bsp.h>
>> +
>> +/*
>> + * BSP PCI Support
>> + *
>> + * The RTEMS Nexus bus support can optionaly support PC PCI spaces that
> optionally
>
>> + * mapped to BSP speciic address spaces. Add the following define to
> specific
>
>> + * the BSP header file to enable this support:
>> + *
>> + * #define BSP_HAS_PC_PCI
>
> Is there an rtems.git patch to add this to some BSP?
Yes that is to come once we have this sorted or it just keeps changing.
>
> I might suggest BSP_HAS_PCI_IO_PORTS
>
I think using PC PCI is better because the PCI originally came from the PC and
it defines the IO space and the memory space so the define means the bus space
will support the IO _and_ the memory spaces. The default is what exists and that
is a flat 1:1 mapping.
>> + *
>> + * If enabled a BSP must the following IO region calls:
> must support?
>
>> + *
>> + * 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.
>> + *
>> + * i386 BSPs have a special bus.h file and do not use this file.
>> + */
>> +
>> +#ifdef BSP_HAS_PC_PCI
>> +
>> +/*
>> + * 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 */
>> +
>> +#endif /* BSP_HAS_PC_PCI */
>> +
>> /*
>> * Bus address alignment.
>> */
>> @@ -144,6 +185,7 @@
>> /*
>> * Bus access.
>> */
>> +#define BUS_SPACE_INVALID_DATA (~0)
> Please use 0U here. I get the undefined behavior (UB) willies when I
> see bit-twiddling on signed integer types.
Sure but I was just keeping the code as close to FB as I could ...
https://github.com/freebsd/freebsd-src/blob/main/sys/x86/include/bus.h#L132
>
>> #define BUS_SPACE_UNRESTRICTED (~0U)
>>
>> /*
>> @@ -222,6 +264,102 @@ bus_space_barrier(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size
>> /* Do nothing */
>> }
>>
>> +/*
>> + * BSP Bus Space Map Support
>> + *
>> + * Provide as C macros in the BSP header (bsp.h) the following:
>
> Not really clear to me under what conditions these should be provided.
> This could be wrapped by #ifndef BSP_HAS_PC_PCI?
The PC PCI is about the type of bus spaces, IO at one location or a special
instruction and memory being cache coherent somewhere else, either at the PCI
config offset or that offset being offset again. The READ_* API is about a
possible need to remap or a special instruction. For example if you have a
single offset flat conherent address space then all you would need to define is
these macros. The MVME2307 has the need for both.
>
>> + *
>> + * RTEMS_BSP_READ_1
>> + * RTEMS_BSP_READ_2
>> + * RTEMS_BSP_READ_4
>> + * RTEMS_BSP_READ_8
>> + * RTEMS_BSP_WRITE_1
>> + * RTEMS_BSP_WRITE_2
>> + * RTEMS_BSP_WRITE_4
>> + * RTEMS_BSP_WRITE_8
>> + */
>> +
>> +static __inline uint8_t
>> +bsp_bus_space_read_1(const uint8_t __volatile *bsp)
>> +{
>> +#if defined(RTEMS_BSP_READ_1)
>> + return RTEMS_BSP_READ_1(bsp);
>> +#else
>> + return (*bsp);
>> +#endif
>> +}
>> +
>> +static __inline uint16_t
>> +bsp_bus_space_read_2(const uint16_t __volatile *bsp)
>> +{
>> +#if defined(RTEMS_BSP_READ_2)
>> + return RTEMS_BSP_READ_2(bsp);
>> +#else
>> + return (*bsp);
>> +#endif
>> +}
>> +
>> +static __inline uint32_t
>> +bsp_bus_space_read_4(const uint32_t __volatile *bsp)
>> +{
>> +#if defined(RTEMS_BSP_READ_4)
>> + return RTEMS_BSP_READ_4(bsp);
>> +#else
>> + return (*bsp);
>> +#endif
>> +}
>> +
>> +static __inline uint64_t
>> +bsp_bus_space_read_8(const uint64_t __volatile *bsp)
>> +{
>> +#if defined(RTEMS_BSP_READ_8)
>> + return RTEMS_BSP_READ_8(bsp);
>> +#else
>> + return (*bsp);
>> +#endif
>> +}
>> +
>> +static __inline void
>> +bsp_bus_space_write_1(uint8_t __volatile *bsp, uint8_t val)
>> +{
>> +#if defined(RTEMS_BSP_WRITE_1)
>> + RTEMS_BSP_WRITE_1(bsp, val);
>> +#else
>> + *bsp = val;
>> +#endif
>> +}
>> +
>> +static __inline void
>> +bsp_bus_space_write_2(uint16_t __volatile *bsp, uint16_t val)
>> +{
>> +#if defined(RTEMS_BSP_WRITE_2)
>> + RTEMS_BSP_WRITE_2(bsp, val);
>> +#else
>> + *bsp = val;
>> +#endif
>> +}
>> +
>> +static __inline void
>> +bsp_bus_space_write_4(uint32_t __volatile *bsp, uint32_t val)
>> +{
>> +#if defined(RTEMS_BSP_WRITE_4)
>> + RTEMS_BSP_WRITE_4(bsp, val);
>> +#else
>> + *bsp = val;
>> +#endif
>> +}
>> +
>> +static __inline void
>> +bsp_bus_space_write_8(uint64_t __volatile *bsp, uint64_t val)
>> +{
>> +#if defined(RTEMS_BSP_WRITE_8)
>> + RTEMS_BSP_WRITE_8(bsp, val);
>> +#else
>> + *bsp = val;
>> +#endif
>> +}
>> +
>> +
>> /*
>> * Read 1 unit of data from bus space described by the tag, handle and ofs
>> * tuple. A unit of data can be 1 byte, 2 bytes, 4 bytes or 8 bytes. The
>> @@ -230,29 +368,48 @@ bus_space_barrier(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size
>> static __inline uint8_t
>> bus_space_read_1(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs)
>
> Remove __unused attribute. Same applies below.
>
Thanks
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + return inb(bsh + ofs);
>> + }
>> +#endif
>> uint8_t __volatile *bsp = (uint8_t __volatile *)(bsh + ofs);
>> - return (*bsp);
>> + return bsp_bus_space_read_1(bsp);
>> }
>>
>> static __inline uint16_t
>> bus_space_read_2(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + return inw(bsh + ofs);
>> + }
>> +#endif
>> uint16_t __volatile *bsp = (uint16_t __volatile *)(bsh + ofs);
>> - return (*bsp);
>> + return bsp_bus_space_read_2(bsp);
>> }
>>
>> static __inline uint32_t
>> bus_space_read_4(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + return inl(bsh + ofs);
>> + }
>> +#endif
>> uint32_t __volatile *bsp = (uint32_t __volatile *)(bsh + ofs);
>> - return (*bsp);
>> + return bsp_bus_space_read_4(bsp);
>> }
>>
>> static __inline uint64_t
>> bus_space_read_8(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO)
>> + return BUS_SPACE_INVALID_DATA;
> add { } to be consistent
Yeap.
>
>> +#endif
>> uint64_t __volatile *bsp = (uint64_t __volatile *)(bsh + ofs);
>> - return (*bsp);
>> + return bsp_bus_space_read_8(bsp);
>> }
>>
>>
>> @@ -265,32 +422,55 @@ static __inline void
>> bus_space_write_1(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs,
>> uint8_t val)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + outb(val, bsh + ofs);
>> + return;
>> + }
>> +#endif
>> uint8_t __volatile *bsp = (uint8_t __volatile *)(bsh + ofs);
>> - *bsp = val;
>> + bsp_bus_space_write_1(bsp, val);
>> }
>>
>> static __inline void
>> bus_space_write_2(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs,
>> uint16_t val)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + outw(val, bsh + ofs);
>> + return;
>> + }
>> +#endif
>> uint16_t __volatile *bsp = (uint16_t __volatile *)(bsh + ofs);
>> - *bsp = val;
>> + bsp_bus_space_write_2(bsp, val);
>> }
>>
>> static __inline void
>> bus_space_write_4(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs,
>> uint32_t val)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + outl(val, bsh + ofs);
>> + return;
>> + }
>> +#endif
>> uint32_t __volatile *bsp = (uint32_t __volatile *)(bsh + ofs);
>> - *bsp = val;
>> + bsp_bus_space_write_4(bsp, val);
>> }
>>
>> static __inline void
>> bus_space_write_8(bus_space_tag_t bst __unused, bus_space_handle_t bsh, bus_size_t ofs,
>> uint64_t val)
>> {
>> +#ifdef BSP_HAS_PC_PCI
>> + if (bst == BSP_BUS_SPACE_IO) {
>> + return;
>> + }
>> +#endif
>> uint64_t __volatile *bsp = (uint64_t __volatile *)(bsh + ofs);
>> - *bsp = val;
>> + bsp_bus_space_write_8(bsp, val);
>> }
>>
>>
>> @@ -305,7 +485,7 @@ bus_space_read_multi_1(bus_space_tag_t bst __unused, bus_space_handle_t bsh,
>> {
>> uint8_t __volatile *bsp = (uint8_t __volatile *)(bsh + ofs);
>> while (count-- > 0) {
>> - *bufp++ = *bsp;
>> + *bufp++ = bsp_bus_space_read_1(bsp);
>
> Can this be called with BSP_HAS_PC_PCI and bst == BSP_BUS_SPACE_IO? If
> so, this looks wrong and you'd need to include the conditionals or
> loop over bus_space_read_1() . If it should not be reached with those
> conditions, can you assert them? Same applies below, and also in the
> region, set, and copy calls.
It could be. I will update this.
Chris
More information about the devel
mailing list