GPIO mode mappings are wrong. Never get remapped to the SPL definitions. Needs rework

[bmourit]

Looking through the GPIO mapping and functions, it appears we have tried to abstract the functions away from the hardware registers, but haven't properly mapped back to them. Rather, we are calling the SPL gpio_init function from pin_function, but passing it variables that are still mapped to the abstraction when it is expecting the actual hardware defined bits. It seems that setting/changing GPIO modes won't be working correctly as is.

Since we don't have a HAL library that we are working with here, we really shouldn't abstract away much at all. There are two different GPIO mode APIs used in the SPL (depending on the specific chip) that need to be sorted, but we can either use the SPL header definitions directly here, or alternatively use switch methods in pin_function (and any place else) to remap these bits back to the proper SPL register definitions. This will have to happen anywhere we call SPL functions, so if there are too many cases - we might want to find a better way??

[bmourit]

For example: In pin_function

uint32_t mode   = GD_PIN_MODE_GET(function);
    uint32_t remap  = GD_PIN_REMAP_GET(function);
    uint32_t speed  = GD_PIN_SPEED_GET(function);
    uint32_t port   = GD_PORT_GET(pin);
    uint32_t gd_pin = 1 << GD_PIN_GET(pin);

    uint32_t gpio = gpio_clock_enable(port);

#if defined(GD32F30x) || defined(GD32F10x)|| defined(GD32E50X)
    gpio_init(gpio, GD_GPIO_MODE[mode], GD_GPIO_SPEED[speed], gd_pin);

for mode from GD_PIN_MODE_GET we get our bit packed gpio info and extract the mode, and then GD_GPIO_MODE turns that into one of the following

const int GD_GPIO_MODE[] = {
    GPIO_MODE_INPUT,             /* 0 */
    GPIO_MODE_OUTPUT,            /* 1 */
    GPIO_MODE_AF,                /* 2 */
    GPIO_MODE_ANALOG,            /* 3 */
};

Which is a const int. But gpio_init expects this to be one of the following:

/* GPIO mode definitions */
#define GPIO_MODE_AIN                    ((uint8_t)0x00U)          /*!< analog input mode */
#define GPIO_MODE_IN_FLOATING            ((uint8_t)0x04U)         /*!< floating input mode */
#define GPIO_MODE_OUT_PP                 ((uint8_t)0x10U)          /*!< GPIO output with push-pull */
#define GPIO_MODE_OUT_OD                 ((uint8_t)0x14U)          /*!< GPIO output with open-drain */
#define GPIO_MODE_AF_PP                  ((uint8_t)0x18U)          /*!< AFIO output with push-pull */
#define GPIO_MODE_AF_OD                  ((uint8_t)0x1CU)          /*!< AFIO output with open-drain */
#define GPIO_MODE_IPD                    ((uint8_t)0x28U)         /*!< pull-down input mode */
#define GPIO_MODE_IPU                    ((uint8_t)0x48U)         /*!< pull-up input mode */

For one API, or:

#define CTL_CLTR(regval)           (BITS(0,1) & ((uint32_t)(regval) << 0))
#define GPIO_MODE_INPUT            CTL_CLTR(0)           /*!< input mode */
#define GPIO_MODE_OUTPUT           CTL_CLTR(1)           /*!< output mode */
#define GPIO_MODE_AF               CTL_CLTR(2)           /*!< alternate function mode */
#define GPIO_MODE_ANALOG           CTL_CLTR(3)           /*!< analog mode */

For the other API.

But we never map our int to one of these, and instead we are passing the plain int as the mode.

Same thing for speed. We should just use the SPL defines here directly to remap these back, similar to how it was done in gpio_clock_enable, no?

[maxgerhardt]

Agreeing very much. This legacy thing that was carried over from the initial core needs to be reworked. I'm a bit skeptical though that it can be done completely without a PeripheralPins.h/.c, after all, the different chips all have different available pins, but using the pure GD32 SPL would expose them all equally, even ones not existing in the package.

[bmourit]

Yes. I could hack something together, but I'm looking for some advice on the best/cleanest/preferred way for it to be done. I'm not sure the answer. While I'm at it, if there is consistency with the chip lines (which is doubtful - but maybe), I may redo the defines to avoid directly ifdef-ing individual chips and do something along the lines of an API V1.0/API V2.0 define instead. It would clean up the code a little, but is a relatively minor change. If it is unwanted I won't though.

To be upfront, my main motivation is to ultimately get this to work with Marlin Firmware. Since Creality quietly began shipping motherboards with GD32 mcus, users are left with a less optimized firmware for this chip. (ie. Slower clocks, unsupported mcu features, etc). I would love to be able to use native chip features, since there appears to be an ever-growing list of these chips showing up in 3d printer motherboards. These chips have their upsides and downsides compared to STM32, but so far we haven't been able to tap into any of the upsides.