Re: [OpenBIOS] Adding new device in the middle of device-tree

16 Jul 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@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@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

    