-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sdmmc): Add SDMMC support for P4 + remove BUILTIN LED #10460
feat(sdmmc): Add SDMMC support for P4 + remove BUILTIN LED #10460
Conversation
👋 Hello P-R-O-C-H-Y, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
libraries/SD_MMC/src/SD_MMC.cpp
Outdated
@@ -186,6 +203,19 @@ bool SDMMCFS::begin(const char *mountpoint, bool mode1bit, bool format_if_mount_ | |||
} | |||
_mode1bit = mode1bit; | |||
|
|||
#ifdef SOC_SDMMC_IO_POWER_EXTERNAL | |||
sd_pwr_ctrl_ldo_config_t ldo_config = { | |||
.ldo_chan_id = _pin_power, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you not by any chance confusing GPIO number with LDO channel number here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I get it wrong here.
Should it be always channel 4 for P4 or should I let the user to specify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on the development board. ESP32-P4 EV board powers IO lines of slot 0 using LDO channel 4. Other boards may use other LDO channels or even use an external LDO, or directly power them from VDD.
I think you need to define this in the board-specific header file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved :)
For P4 EV board, SD card should use slot 0. Slot 1 is used for SDIO connection to the C6 module. See espressif/esp-bsp#405 for an example. |
Add missing note about power pin for P4 Co-authored-by: Lucas Saavedra Vaz <32426024+lucasssvaz@users.noreply.github.com>
36083dc
to
21d2b13
Compare
#elif SOC_SDMMC_USE_IOMUX && defined(BOARD_HAS_SDMMC) && defined(CONFIG_IDF_TARGET_ESP32P4) | ||
// ESP32-P4 can use either IOMUX or GPIO matrix | ||
#elif defined(BOARD_HAS_SDMMC) && defined(CONFIG_IDF_TARGET_ESP32P4) | ||
#if defined(BOARD_SDMMC_SLOT) && (BOARD_SDMMC_SLOT == 0) | ||
_pin_clk = SDMMC_SLOT0_IOMUX_PIN_NUM_CLK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually work? IIUC, the current sdmmc driver expects all pins to be set to 0 for IOMUX slots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the pins for Peripheral manager.
Later the config is cleared, as the SDMMC_SLOT_CONFIG_DEFAULT for P4 is setting the pins in the config.
sdmmc_slot_config_t slot_config = SDMMC_SLOT_CONFIG_DEFAULT();
#if defined(CONFIG_IDF_TARGET_ESP32P4) && defined(BOARD_SDMMC_SLOT) && (BOARD_SDMMC_SLOT == 0)
host.slot = SDMMC_HOST_SLOT_0;
// reconfigure slot_config to remove all pins in order to use IO_MUX
slot_config = {
.cd = SDMMC_SLOT_NO_CD,
.wp = SDMMC_SLOT_NO_WP,
.width = 4,
.flags = 0,
};
#else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For P4 it will be always this config:
#define SDMMC_SLOT_CONFIG_DEFAULT() {\
.clk = GPIO_NUM_43, \
.cmd = GPIO_NUM_44, \
.d0 = GPIO_NUM_39, \
.d1 = GPIO_NUM_40, \
.d2 = GPIO_NUM_41, \
.d3 = GPIO_NUM_42, \
.d4 = GPIO_NUM_45, \
.d5 = GPIO_NUM_46, \
.d6 = GPIO_NUM_47, \
.d7 = GPIO_NUM_48, \
.cd = SDMMC_SLOT_NO_CD, \
.wp = SDMMC_SLOT_NO_WP, \
.width = SDMMC_SLOT_WIDTH_DEFAULT, \
.flags = 0, \
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As both IOMUX and GPIO Matrix are supported, this will never be used for P4.
#if SOC_SDMMC_USE_IOMUX && !SOC_SDMMC_USE_GPIO_MATRIX
/**
* Macro defining default configuration of SDMMC host slot
*/
#define SDMMC_SLOT_CONFIG_DEFAULT() {\
.cd = SDMMC_SLOT_NO_CD, \
.wp = SDMMC_SLOT_NO_WP, \
.width = SDMMC_SLOT_WIDTH_DEFAULT, \
.flags = 0, \
}
Description of Change
This PR adds power pin setting for SDMMC, as on P4 the SDMMC IO Power pins is controlled by external power supply.
Also the NEOPIXEL LED and BUILTIN LED got removed, as the P4 Function EV board don't have controllable LED (only power LED)
Tests scenarios
Tested on ESP32-P4 Function EV Board V1.4
Related links
#10278