Change of current device table approach

[gamelaster]

Let's take this code:

void main() {
    struct bflb_device_s * dev = bflb_device_get_by_name("gpio");
    uint32_t reg_base = dev->reg_base;
    printf("GPIO reg base: %08X\n", reg_base);

    struct bflb_device_s * dev2 = bflb_device_get_by_name("sdio2");
    printf("SDIO base: %08X\n", dev2->reg_base);
}

Current approach

Current approach implements bflb_device_get_by_name like this (link to code):

struct bflb_device_s *bflb_device_get_by_name(const char *name)
{
    for (uint8_t i = 0; i < sizeof(bl602_device_table) / sizeof(bl602_device_table[0]); i++) {
        if (strcmp(bl602_device_table[i].name, name) == 0) {
            return &bl602_device_table[i];
        }
    }
    return NULL;
}

Personally, I generally don't like this approach, to have function to get base address and configuration for the IP Core, but that's not the matter. The matter is, that this code iterates the structure and does strcmp on every item, so gcc cannot optimize it at compile time even when maximum optimization option -O3 is enabled, thus this function is really waste of cycles, as it can be all computed in compilation stage. We can verify it, by putting the code into godbolt. (65 lines of visible assembly code for this approach)

Solution 1.

This solution breaks compatibility

Since function is absolutely not needed for this, we can rather have pre-defined global struct variables. Thanks to this, if LHAL or user will use IP core, which is not available for used chip, it will show error during compile time, what is good and eliminates a lot of bugs. So, it would look like this:

struct bflb_device_s  dev_gpio = { 
   // ....
};

void main() {
    struct bflb_device_s * dev = &dev_gpio;
    uint32_t reg_base = dev->reg_base;
    printf("GPIO reg base: %08X\n", reg_base);

    struct bflb_device_s * dev2 = &dev_sdio2;
    printf("SDIO base: %08X\n", dev2->reg_base);
}

(Note, this is just example, dev_gpio can also be pointer to the original device table struct)

After putting this in godbolt, now it is only 17 lines of assembly, to do the absolutely same.

Solution 2.

This solution breaks compatibility (but it is better than first solution)

Sadly, due to limitations of macros, we cannot destringify arguments into macro, thus, we cannot make backwards compatibility. The most closest we can get is as follows:

#define bflb_device_get_by_name(name) (&dev_##name)

void main() {
    struct bflb_device_s * dev = bflb_device_get_by_name(gpio);
    uint32_t reg_base = dev->reg_base;
    printf("GPIO reg base: %08X\n", reg_base);

    struct bflb_device_s * dev2 = bflb_device_get_by_name(sdio2);
    printf("SDIO base: %08X\n", dev2->reg_base);
}

Basically, only change is to remove quotes in arguments, but still, compiler will complain. The optimization is same good, as first solution: godbolt.

Summary

So far, I wasn't able to find a way, where we could keep backwards compatibility, but also keep the same optimization as in solution 1. Thus, there is not easy solution, but I think breaking compatibility is worth of performance improvement this will do, as developers, whose don't know how bflb_device_get_by_name works, will use it inappropriately, thus causing unnecessary overhead.

[gamelaster]

Solution 3.

This solution does not break compatibility

The bflb_device_get_by_name will be kept as is, but there will be in every device_table.h pointer to specific area on device_table. Example:

struct bflb_device_s* dev_gpio = &bl602_device_table[3];

void main() {
    struct bflb_device_s* dev = dev_gpio; // new method
    uint32_t reg_base = dev->reg_base;
    printf("GPIO reg base: %08X\n", reg_base);

    struct bflb_device_s * dev2 = bflb_device_get_by_name("sdio2"); // old method still works
    printf("SDIO base: %08X\n", dev2->reg_base);
}

Solution 4.

This solution does not break compatibility

The bflb_device_get_by_name will be kept as is, but there will be in every device_table.h struct with every supported device, as pointer to original device_table array. Example:

struct {
    struct bflb_device_s* gpio;
} device_table = {
    .gpio = &bl602_device_table[3];
};

void main() {
    struct bflb_device_s* dev = device_table.gpio; // new method
    uint32_t reg_base = dev->reg_base;
    printf("GPIO reg base: %08X\n", reg_base);

    struct bflb_device_s * dev2 = bflb_device_get_by_name("sdio2"); // old method still works
    printf("SDIO base: %08X\n", dev2->reg_base);
}

[sakumisu]

Let users know device is on which array index is not a better way. And the code has become more.

[gamelaster]

User doesn't need to know that. The device_table struct is defined in for example bl602_device_table.h, so user will only directly use device_table.gpio. Yes, it is more code, but that's sadly necessary, to have both backwards compatibility, but also new faster way of having device table feature.