GPIO checks for PULL_UP/DOWN doesn't work for RTC pins. (IDFGH-14127)

[IhorNehrutsa]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

Functions gpio_ll_get_io_config(), gpio_ll_pullup_is_enabled(), gpio_ll_pulldown_is_enabled() work through the IO_MUX_x_REG

esp-idf/components/hal/esp32/include/hal/gpio_ll.h

Lines 66 to 68 in f420609

	uint32_t iomux_reg_val = REG_READ(GPIO_PIN_MUX_REG[gpio_num]);
	*pu = (iomux_reg_val & FUN_PU_M) >> FUN_PU_S;
	*pd = (iomux_reg_val & FUN_PD_M) >> FUN_PD_S;

esp-idf/components/hal/esp32/include/hal/gpio_ll.h

Lines 108 to 111 in f420609

	static inline bool gpio_ll_pullup_is_enabled(gpio_dev_t *hw, uint32_t gpio_num)
	{
	return REG_GET_BIT(GPIO_PIN_MUX_REG[gpio_num], FUN_PU) ? true : false;
	}

esp-idf/components/hal/esp32/include/hal/gpio_ll.h

Lines 143 to 146 in f420609

	static inline bool gpio_ll_pulldown_is_enabled(gpio_dev_t *hw, uint32_t gpio_num)
	{
	return REG_GET_BIT(GPIO_PIN_MUX_REG[gpio_num], FUN_PD) ? true : false;
	}

But for pins that can operate via RTC, PULL_UP/DOWN is set via the RTC mechanism (see line 86, line 105), not MUX_REG mechanism.

esp-idf/components/esp_driver_gpio/src/gpio.c

Lines 76 to 112 in f420609

	esp_err_t gpio_pullup_en(gpio_num_t gpio_num)
	{
	GPIO_CHECK(GPIO_IS_VALID_GPIO(gpio_num), "GPIO number error", ESP_ERR_INVALID_ARG);
	if (!rtc_gpio_is_valid_gpio(gpio_num) || SOC_GPIO_SUPPORT_RTC_INDEPENDENT) {
	portENTER_CRITICAL(&gpio_context.gpio_spinlock);
	gpio_hal_pullup_en(gpio_context.gpio_hal, gpio_num);
	portEXIT_CRITICAL(&gpio_context.gpio_spinlock);
	} else {
	#if SOC_RTCIO_INPUT_OUTPUT_SUPPORTED
	rtc_gpio_pullup_en(gpio_num);
	#else
	abort(); // This should be eliminated as unreachable, unless a programming error has occurred
	#endif
	}
	return ESP_OK;
	}
	esp_err_t gpio_pullup_dis(gpio_num_t gpio_num)
	{
	GPIO_CHECK(GPIO_IS_VALID_GPIO(gpio_num), "GPIO number error", ESP_ERR_INVALID_ARG);
	if (!rtc_gpio_is_valid_gpio(gpio_num) || SOC_GPIO_SUPPORT_RTC_INDEPENDENT) {
	portENTER_CRITICAL(&gpio_context.gpio_spinlock);
	gpio_hal_pullup_dis(gpio_context.gpio_hal, gpio_num);
	portEXIT_CRITICAL(&gpio_context.gpio_spinlock);
	} else {
	#if SOC_RTCIO_INPUT_OUTPUT_SUPPORTED
	rtc_gpio_pullup_dis(gpio_num);
	#else
	abort(); // This should be eliminated as unreachable, unless a programming error has occurred
	#endif
	}
	return ESP_OK;
	}

See;

esp-idf/components/hal/esp32/include/hal/rtc_io_ll.h

Lines 178 to 183 in f420609

	static inline void rtcio_ll_pullup_enable(int rtcio_num)
	{
	if (rtc_io_desc[rtcio_num].pullup) {
	SET_PERI_REG_MASK(rtc_io_desc[rtcio_num].reg, rtc_io_desc[rtcio_num].pullup);
	}
	}

esp-idf/components/hal/esp32/include/hal/rtc_io_ll.h

Lines 190 to 195 in f420609

	static inline void rtcio_ll_pullup_disable(int rtcio_num)
	{
	if (rtc_io_desc[rtcio_num].pullup) {
	CLEAR_PERI_REG_MASK(rtc_io_desc[rtcio_num].reg, rtc_io_desc[rtcio_num].pullup);
	}
	}

[IhorNehrutsa]

May set SOC_GPIO_SUPPORT_RTC_INDEPENDENT to 1 to resolve the issue.

esp-idf/components/esp_driver_gpio/src/gpio.c

Line 37 in f420609

#define SOC_GPIO_SUPPORT_RTC_INDEPENDENT 0

[safocl]

maybe this version will look and work better:

struct get_io_config_info {
    bool *pu;
    bool *pd;
    bool *ie;
    bool *oe;
    bool *od;
    uint32_t *drv;
    uint32_t *fun_sel;
    uint32_t *sig_out;
    bool *slp_sel;
};

static inline get_io_config_info gpio_ll_get_io_config(gpio_dev_t *hw, uint32_t gpio_num)
{
    uint32_t bit_shift = (gpio_num < 32) ? gpio_num : (gpio_num - 32);
    uint32_t bit_mask = 1 << bit_shift;
    uint32_t iomux_reg_val = REG_READ(GPIO_PIN_MUX_REG[gpio_num]);

    get_io_config_info ret;
    get_io_config_info.pu = (iomux_reg_val & FUN_PU_M) >> FUN_PU_S;
    get_io_config_info.pd = (iomux_reg_val & FUN_PD_M) >> FUN_PD_S;
    get_io_config_info.ie = (iomux_reg_val & FUN_IE_M) >> FUN_IE_S;
    get_io_config_info.oe = (((gpio_num < 32) ? hw->enable : hw->enable1.val) & bit_mask) >> bit_shift;
    get_io_config_info.od = hw->pin[gpio_num].pad_driver;
    get_io_config_info.drv = (iomux_reg_val & FUN_DRV_M) >> FUN_DRV_S;
    get_io_config_info.fun_sel = (iomux_reg_val & MCU_SEL_M) >> MCU_SEL_S;
    get_io_config_info.sig_out = hw->func_out_sel_cfg[gpio_num].func_sel;
    get_io_config_info.slp_sel = (iomux_reg_val & SLP_SEL_M) >> SLP_SEL_S;

    return ret;
}

or

    return { 
        .pu = (iomux_reg_val & FUN_PU_M) >> FUN_PU_S,
        .pd = (iomux_reg_val & FUN_PD_M) >> FUN_PD_S,
        .ie = (iomux_reg_val & FUN_IE_M) >> FUN_IE_S,
        .oe = (((gpio_num < 32) ? hw->enable : hw->enable1.val) & bit_mask) >> bit_shift,
        .od = hw->pin[gpio_num].pad_driver,
        .drv = (iomux_reg_val & FUN_DRV_M) >> FUN_DRV_S,
        .fun_sel = (iomux_reg_val & MCU_SEL_M) >> MCU_SEL_S,
        .sig_out = hw->func_out_sel_cfg[gpio_num].func_sel,
        .slp_sel = (iomux_reg_val & SLP_SEL_M) >> SLP_SEL_S
        };

?

This gives some advantages -- at least you won't have to check for nullptr pointers.

[IhorNehrutsa]

@safocl @songruo
The main problem is that when we get the PULLUP/PULLDOWN status, we check the IO_MUX_x_REG in the GPIO functions

 *pu = (iomux_reg_val & FUN_PU_M) >> FUN_PU_S; 
 *pd = (iomux_reg_val & FUN_PD_M) >> FUN_PD_S; 

but to set PULLUP/PULLDOWN we use RTC function

rtc_gpio_pullup_en(gpio_num); 
rtc_gpio_pullup_dis(gpio_num); 

not the GPIO function

gpio_hal_pullup_en(gpio_context.gpio_hal, gpio_num); 
gpio_hal_pullup_dis(gpio_context.gpio_hal, gpio_num); 

Fast solution in #14931 (comment)

[IhorNehrutsa]

The bug remains present and not solved.
Closing the issue is premature and unjustified.

[IhorNehrutsa]

#14923 doesn't solve this issue/bug.

[IhorNehrutsa]

Do I must to reopen this issue?

[songruo]

@IhorNehrutsa The fix is here:

esp-idf/components/esp_driver_gpio/src/gpio.c

Lines 1026 to 1039 in c749ec6

	#if !SOC_GPIO_SUPPORT_RTC_INDEPENDENT && SOC_RTCIO_PIN_COUNT > 0
	if (rtc_gpio_is_valid_gpio(gpio_num)) {
	int rtcio_num = rtc_io_number_get(gpio_num);
	if (pu) {
	*pu = rtcio_hal_is_pullup_enabled(rtcio_num);
	}
	if (pd) {
	*pd = rtcio_hal_is_pulldown_enabled(rtcio_num);
	}
	if (drv) {
	*drv = rtcio_hal_get_drive_capability(rtcio_num);
	}
	}
	#endif

Are you sure this doesn't fix the issue?

[safocl]

@IhorNehrutsa
I'm talking about the general case with this function, the form to which it was brought looks with a lot of overhead in my opinion... (many pointers to simple data in function arguments that need to be checked for nullptr before use to avoid UB)
but you can avoid such things by simply outputting the data in return

[IhorNehrutsa]

@songruo
Does c749ec6 apply to other releases?

[songruo]

Will backport to previous releases.