[OpenBIOS] Adding new device in the middle of device-tree
BALATON Zoltan
balaton at eik.bme.hu
Mon Jul 17 01:04:23 CEST 2017
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
> On 16/07/17 21:56, BALATON Zoltan wrote:
>>>> So does that mean we could theoretically get rid of all the other
>>>> redundant data and put this pci_mem_base offset for SPARC64 in some
>>>> #ifdefed code part (or in the undocumented register to counter the
>>>> offset in Linux) to simplify the current situation where the ranges are
>>>> defined in two different ways?
>>>
>>> There was some code for config space registers already #ifdef'd for
>>> SPARC64 but the issue there was that with so many conditionals used for
>>> building up the PCI host bridge ranges that it was very difficult to see
>>> at a glance what the resulting values were.
>>>
>>> Bear in mind that PPC also had its own hacks in that it has multiple IO
>>> memory spaces listed for the host bridge, so rather than keep going down
>>> this maze of conditional logic and #ifdef, I decided to explicitly list
>>> the values themselves which solves all the problems in one go.
>>>
>>> If you wanted to better document the relationships between the host
>>> ranges properties and the corresponding values in pci_arch_t then you
>>> could use some #defines, but then if you're working down at that level
>>> regardless then I suspect this will be the least of your problems.
>>
>> No I don't mean defines but something like this:
>>
>> From: BALATON Zoltan <balaton at eik.bme.hu>
>> Date: Sun, 16 Jul 2017 22:41:38 +0200
>> Subject: [PATCH] RFC patch to remove the now redundant io_base and io_len from
>> pci_arch_t that are present in newly added pci ranges. This was only compile
>> tested on PPC and not sure what to do with other redundandant addresses and
>> lengths but similar handling might be possible for those too. Feel free to do
>> anything with this patch.
>>
>> Signed-off-by: BALATON Zoltan <balaton at eik.bme.hu>
>> ---
>> arch/ppc/qemu/init.c | 12 ++----------
>> drivers/pci.c | 39 ++++++++++++++++++++++++++++++---------
>> include/drivers/pci.h | 19 ++++++++++---------
>> 3 files changed, 42 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c
>> index b0c0cfc..a4746c2 100644
>> --- a/arch/ppc/qemu/init.c
>> +++ b/arch/ppc/qemu/init.c
>> @@ -104,8 +104,6 @@ static const pci_arch_t known_arch[] = {
>> .host_pci_base = 0xc0000000,
>> .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */
>> .mem_len = 0x10000000,
>> - .io_base = 0x80000000,
>> - .io_len = 0x00010000,
>> .host_ranges = {
>> { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0x80000000, .len = 0x00010000 },
>> { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xc0100000, .len = 0x10000000 },
>> @@ -124,8 +122,6 @@ static const pci_arch_t known_arch[] = {
>> .host_pci_base = 0x0,
>> .pci_mem_base = 0x80000000,
>> .mem_len = 0x10000000,
>> - .io_base = 0xf2000000,
>> - .io_len = 0x00800000,
>> .host_ranges = {
>> { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 },
>> { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },
>> @@ -144,8 +140,6 @@ static const pci_arch_t known_arch[] = {
>> .host_pci_base = 0x0,
>> .pci_mem_base = 0x80000000,
>> .mem_len = 0x10000000,
>> - .io_base = 0xf2000000,
>> - .io_len = 0x00800000,
>> .host_ranges = {
>> { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 },
>> { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },
>> @@ -164,8 +158,6 @@ static const pci_arch_t known_arch[] = {
>> .host_pci_base = 0x0,
>> .pci_mem_base = 0x80000000,
>> .mem_len = 0x10000000,
>> - .io_base = 0xfe000000,
>> - .io_len = 0x00800000,
>> .host_ranges = {
>> { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xfe000000, .len = 0x00800000 },
>> { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xfd000000, .len = 0x01000000 },
>> @@ -198,8 +190,8 @@ entry(void)
>> arch = &known_arch[machine_id];
>> }
>> }
>> -
>> - isa_io_base = arch->io_base;
>> + const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
>> + isa_io_base = range->childaddr;
>>
>> #ifdef CONFIG_DEBUG_CONSOLE
>> if (is_apple()) {
>> diff --git a/drivers/pci.c b/drivers/pci.c
>> index f6d9437..2f20dcc 100644
>> --- a/drivers/pci.c
>> +++ b/drivers/pci.c
>> @@ -140,14 +140,21 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
>>
>> static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba)
>> {
>> - if (space == IO_SPACE) {
>> - return arch->io_base + (unsigned long)ba;
>> - } else if (space == MEMORY_SPACE_32) {
>> - return arch->host_pci_base + (unsigned long)ba;
>> - } else {
>> + unsigned long host_addr = (unsigned long)ba;
>> + const pci_range_t *range = ob_pci_get_range(space, arch->host_ranges);
>> +
>> + switch (space) {
>> + case IO_SPACE:
>> + host_addr += range->childaddr;
>> + break;
>> + case MEMORY_SPACE_32:
>> + host_addr += arch->host_pci_base;
>> + break;
>> + default:
>> /* Return unaltered to aid debugging property values */
>> - return (unsigned long)ba;
>> + break;
>> }
>> + return host_addr;
>> }
>>
>> static void
>> @@ -491,6 +498,17 @@ static void pci_host_set_ranges(const pci_config_t *config)
>> set_property(dev, "ranges", (char *)props, ncells * sizeof(props[0]));
>> }
>>
>> +const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[])
>> +{
>> + const pci_range_t *range = NULL;
>> + int i;
>> + for (i = 0; i < 4 && ranges[i].type != SPACE_END; i++) {
>> + if (ranges[i].type == type)
>> + range = &ranges[i];
>> + }
>> + return range;
>> +}
>> +
>> int host_config_cb(const pci_config_t *config)
>> {
>> //XXX this overrides "reg" property
>> @@ -988,11 +1006,12 @@ int ebus_config_cb(const pci_config_t *config)
>>
>> int i82378_config_cb(const pci_config_t *config)
>> {
>> + const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
>> #ifdef CONFIG_DRIVER_PC_SERIAL
>> - ob_pc_serial_init(config->path, "serial", arch->io_base, 0x3f8ULL, 0);
>> + ob_pc_serial_init(config->path, "serial", range->childaddr, 0x3f8ULL, 0);
>> #endif
>> #ifdef CONFIG_DRIVER_PC_KBD
>> - ob_pc_kbd_init(config->path, "8042", NULL, arch->io_base, 0x60ULL, 0, 0);
>> + ob_pc_kbd_init(config->path, "8042", NULL, range->childaddr, 0x60ULL, 0, 0);
>> #endif
>> #ifdef CONFIG_DRIVER_IDE
>> ob_ide_init(config->path, 0x1f0, 0x3f6, 0x170, 0x376);
>> @@ -1649,12 +1668,14 @@ static void ob_pci_set_available(phandle_t host, unsigned long mem_base, unsigne
>> /* Create an available property for both memory and IO space */
>> uint32_t props[10];
>> int ncells;
>> + const pci_range_t *range;
>>
>> ncells = 0;
>> ncells += pci_encode_phys_addr(props + ncells, 0, MEMORY_SPACE_32, 0, 0, mem_base);
>> ncells += pci_encode_size(props + ncells, arch->mem_len - mem_base);
>> ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE, 0, 0, io_base);
>> - ncells += pci_encode_size(props + ncells, arch->io_len - io_base);
>> + range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
>> + ncells += pci_encode_size(props + ncells, range->len - io_base);
>>
>> set_property(host, "available", (char *)props, ncells * sizeof(props[0]));
>> }
>> diff --git a/include/drivers/pci.h b/include/drivers/pci.h
>> index e9f20a1..e267db2 100644
>> --- a/include/drivers/pci.h
>> +++ b/include/drivers/pci.h
>> @@ -1,19 +1,20 @@
>> #ifndef _H_PCI
>> #define _H_PCI
>>
>> -enum {
>> - CONFIGURATION_SPACE = 0,
>> - IO_SPACE = 1,
>> - MEMORY_SPACE_32 = 2,
>> - MEMORY_SPACE_64 = 3,
>> -};
>> +typedef enum {
>> + SPACE_END = 0,
>> + CONFIGURATION_SPACE,
>> + IO_SPACE,
>> + MEMORY_SPACE_32,
>> + MEMORY_SPACE_64
>> +} pci_range_type;
>
> The space identifiers need to be identical to the current ones since
> these are well-known values used by client OSs to enumerate the space
> types from the device tree.
Are these PCI bar numbers? Maybe the ranges should be indexed by these
then so we don't need the loop to find the right range but we can just get
host_ranges[IO_SPACE] and so on. That may be more efficient than always
looping through this array.
>> typedef uint32_t pci_addr;
>>
>> typedef struct pci_range_t pci_range_t;
>>
>> struct pci_range_t {
>> - unsigned int type;
>> + pci_range_type type;
>> unsigned long childaddr;
>> unsigned long parentaddr;
>> unsigned long len;
>> @@ -32,14 +33,14 @@ struct pci_arch_t {
>> unsigned long host_pci_base; /* offset of PCI memory space within host memory space */
>> unsigned long pci_mem_base; /* in PCI memory space */
>> unsigned long mem_len;
>> - unsigned long io_base;
>> - unsigned long io_len;
>> pci_range_t host_ranges[4];
>> uint8_t irqs[4];
>> };
>>
>> extern const pci_arch_t *arch;
>>
>> +const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]);
>> +
>> /* Device tree offsets */
>>
>> #define PCI_INT_MAP_PCI0 0
>
> I can see what you're trying to do here, however for me the part I like
> least about this is that it introduces asymmetry between the PCI IO and
> MMIO memory spaces. Effectively what you're doing is saying that for PCI
> IO memory you need to extract the information from the range whilst for
> PCI MMIO memory you need to use the standard struct values, which for me
> seems something that's easy to trip up developers.
No, I meant it should be done also for MMIO (and maybe config regions as
well if possible) but I haven't done that. I've only sent this patch to
explain what I meant, this is not finished. I'll leave that to someone
interested. My comment was that we should have either the ranges or the
struct values but not both as that's redundant. It should be possible to
compute the ranges from the values but you've found that was not working
and it's clearer to start from ranges. That's fine but then we should not
need the struct values and get those from the ranges or if we can't do
that then there may a bug somewhere as these should be identical because
they represent the same thing, don't they?
> If you could find a way to make the changes consistent between both MMIO
> and IO memory I think the patch will improve things, although you also
> need to handle the case for ARCH_HEATHROW where you have multiple 32-bit
> MMIO ranges and for SPARC64 whereby the MMIO child address is not the
> same as pci_mem_base.
These are the cases that I said it needs understanding of pci structure of
each arch in which I'm not really interested and have no time for now. But
why does Heathrow have two mmio ranges? I've found this log:
https://gist.github.com/tomari/3186021
This mentions that the one at fd000000 is some kind of ISA memory hole so
this is probably a special case that could be added by an #ifdefed hack in
pci.c and not need to be present in the pci_arch_t? For the SPARC64 case I
only know what you've said but if I got that correctly the problem is that
while mapping would be the same but if you do that Linux adds an offset
from a special register and this makes the values off. So can't you patch
this by controlling the special register in QEMU and return the right
offset (or the negative of the value by which the mapping is off to
counter this problem)? Then SPARC64 should not need special handling but
I'm likely missing something here.
> Note that if you experiment further with this you can also see that
> .parentaddr is also equivalent to host_pci_base which can help simplify
> things further.
And this is where understanding of these pci mappings and representations
would help. As I don't have this understanding I don't think I can do it
easily. I don't know for example what the cfg_* values are but I think
they might be offsets into a configuration space, so at least some of
those could also be removed (with that a config space may need to be
defined for Apple machines too but an #ifdef in pci.c to not add ranges
for that in the device tree).
So all this is just brain storming and don't wait for me to provide a
better patch. I'm just trying to get my idea across, I don't intend to
work on this in the near future.
Regards,
BALATON Zoltan
More information about the OpenBIOS
mailing list