It appears that the latest ChibiOS 21.11.x has a few nice bits we have been missing out on. Namely, SPI slave support.

We should probably be targeting that branch from now when it gets released

TODO:

1. Merge in Chibios-Contrib 21.11.x when it gets dropped
2. Cleanup current HAL LLDs according to latest platform updates
3. Expand the HAL to all the supported modules - We will need to implement SPI, I2C, ADC, WDG drivers
4. Commit a giant PR to upstream Chibios-Contrib
5. ???
6. Profit.

GPIOs are mis-configured during initialization. The 260 is affected by this, but because that shares code with 240B, I expect that 240B is affected as well.

Issue

The GPIO is initialized in os/hal/ports/SN32/LLD/SN32F24xB/GPIO/hal_pal_lld.c by _pal_lld_init() based on the passed in pal_config argument.

During chibios startup _pal_lld_init() is called with pal_default_config.

pal_default_config is initialized in board.c with the different VAL_GPIOx_MODE. The VAL_GPIOx_MODE macros are defined in board.h and are used to select the mode.

const PALConfig pal_default_config = {
  #if SN32_HAS_GPIOA
  {VAL_GPIOA_MODE},
  #endif
  #if SN32_HAS_GPIOB
  {VAL_GPIOB_MODE},
  #endif
  #if SN32_HAS_GPIOC
  {VAL_GPIOC_MODE},
  #endif
  #if SN32_HAS_GPIOD
  {VAL_GPIOD_MODE},
  #endif
};

When we look at the type of PALConfig, we see that the pal_default_config initialization only explicitly initializes the the first field of sn32_gpio_setup_t (data) with VAL_GPIOx_MODE. All other fields are implicitly zero initialized. This is wrong, one would expect the mode field to be initialized with VAL_GPIOx_MODE, not the data field.

typedef struct {
  /** Initial value for DATA register.*/
  uint32_t              data;
  /** Initial value for MODE register.*/
  uint32_t              mode;
  /** Initial value for CFG register.*/
  uint32_t              cfg;
  /** Initial value for IS register.*/
  uint32_t              is;
  /** Initial value for IBS register.*/
  uint32_t              ibs;
  /** Initial value for IEV register.*/
  uint32_t              iev;
  /** Initial value for IE register.*/
  uint32_t              ie;
  /** Initial value for RIS register.*/
  uint32_t              ris;
  /** Initial value for IC register.*/
  uint32_t              ic;
  /** Initial value for BSET register.*/
  uint32_t              bset;
  /** Initial value for BCLR register.*/
  uint32_t              bclr;
} sn32_gpio_setup_t;

typedef struct {
#if SN32_HAS_GPIOA || defined(__DOXYGEN__)
  /** @brief Port A setup data.*/
  sn32_gpio_setup_t    PAData;
#endif
#if SN32_HAS_GPIOB || defined(__DOXYGEN__)
  /** @brief Port B setup data.*/
  sn32_gpio_setup_t    PBData;
#endif
#if SN32_HAS_GPIOC || defined(__DOXYGEN__)
  /** @brief Port C setup data.*/
  sn32_gpio_setup_t    PCData;
#endif
#if SN32_HAS_GPIOD || defined(__DOXYGEN__)
  /** @brief Port D setup data.*/
  sn32_gpio_setup_t    PDData;
#endif
} PALConfig;

Finally when _pal_lld_init() is called during chibios startup, initgpio() is called for each GPIO port. And there the mode configuration is loaded into the data register. And the mode and cfg registers are loaded with zeros.

static void initgpio(SN_GPIO0_Type *gpiop, const sn32_gpio_setup_t *config) {
    gpiop->DATA  = config->data;
    gpiop->MODE  = config->mode;
    gpiop->CFG   = config->cfg;
}

void _pal_lld_init(const PALConfig *config) {
#if SN32_HAS_GPIOA
  initgpio(GPIOA, &config->PAData);
#endif
#if SN32_HAS_GPIOB
  initgpio(GPIOB, &config->PBData);
#endif
#if SN32_HAS_GPIOC
  initgpio(GPIOC, &config->PCData);
#endif
#if SN32_HAS_GPIOD
  initgpio(GPIOD, &config->PDData);
#endif
}

This misconfiguration is visible on the indicator leds on the Keychron C1 white, this how I discovered this bug.

Later when all other drivers (key matrix, indicator leds, etc) are loaded, the misconfiguration is overwritten with a correct configuration by the drivers. And everything is fine again.

Fix

There are multiple ways this can be fixed. One would be to correctly fill the pal_default_config.

An other approach is to switch to new chibios PAL driver model. This can be done with PAL_NEW_INIT. With this enabled the init no longer takes a configuration argument. The PAL driver will do no initial initialization, but lets the other drivers (key matrix, indicator leds, etc) setup the GPIO the way they want it. This way all pal_default_config related stuff can be removed, and can no longer contain bugs.

What is next?

How do we want to proceed with this? Do we want to switch to the PAL_NEW_INIT model?
If so this might also be a good opportunity to unify the GPIO driver for all SN32F2xx chips.