Hi Sebastian, Thank you for the review.
On 16/2/21 5:05 pm, Sebastian Huber wrote: > > On 16/02/2021 03:02, chr...@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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel