Hi Chen-Yu,
On 4/29/25 7:34 PM, Chen-Yu Tsai wrote:
On Wed, Apr 30, 2025 at 12:30 AM Quentin Schulz
<quentin.sch...@cherry.de> wrote:
Hi Chen-Yu,
On 4/29/25 5:44 PM, Chen-Yu Tsai wrote:
From: Chen-Yu Tsai <w...@csie.org>
The rockchip_dw_mmc driver supports the MMC controller found in Rockchip
SoCs. This controller is used for the SD card on all SoCs and eMMC on
older SoCs. Almost all defconfigs for Rockchip platforms have this
enabled.
Enable it by default for all supported Rockchip SoCs. Disable it
explicitly in defconfigs that previously didn't have it enabled.
I'll let Tom confirm (or not), but I think it'd make sense to make this
patch lighter by not including the defconfig changes that would be
simply done automatically when doing a defconfig sync, e.g. like
bdf41fb7b386bdf60303b7a92431467c12779c86 did? This would be nice because
it would make your patch much easier to apply if they take long to be
applied (e.g. someone else changes the defconfig and now we have
conflicts all over the place). This also would remove unrelated changes
from the diff, specifically the ones for the PX30 boards which are just
noise at this point.
[...]
diff --git a/configs/coolpi-cm5-genbook-rk3588_defconfig
b/configs/coolpi-cm5-genbook-rk3588_defconfig
index 3eb5dc968af6..92676ebb984a 100644
--- a/configs/coolpi-cm5-genbook-rk3588_defconfig
+++ b/configs/coolpi-cm5-genbook-rk3588_defconfig
@@ -65,6 +65,7 @@ CONFIG_MMC_HS400_ES_SUPPORT=y
CONFIG_SPL_MMC_HS400_ES_SUPPORT=y
CONFIG_MMC_HS400_SUPPORT=y
CONFIG_SPL_MMC_HS400_SUPPORT=y
+# CONFIG_MMC_DW is not set
CONFIG_MMC_SDHCI=y
CONFIG_MMC_SDHCI_SDMA=y
CONFIG_MMC_SDHCI_ROCKCHIP=y
Checked that and sdmmc and sio both seems to be disabled in the DTS, so
makes sense to have the driver disabled too.
[...]
diff --git a/configs/evb-rv1108_defconfig b/configs/evb-rv1108_defconfig
index 46b949531017..d23f90a6a606 100644
--- a/configs/evb-rv1108_defconfig
+++ b/configs/evb-rv1108_defconfig
@@ -31,6 +31,7 @@ CONFIG_FASTBOOT_BUF_SIZE=0x08000000
CONFIG_FASTBOOT_FLASH_MMC_DEV=1
CONFIG_ROCKCHIP_GPIO=y
CONFIG_SYS_I2C_ROCKCHIP=y
+# CONFIG_MMC_DW is not set
CONFIG_SPI_FLASH_GIGADEVICE=y
CONFIG_SPI_FLASH_WINBOND=y
CONFIG_SPI_FLASH_MTD=y
This one seems to be a mistake, sdmmc is enabled and is supported by
this driver as far as I know. A separate patch to enable it
(before/after this one, would be nice).
After the patch it would be enabled by default, so nothing actually
has to be done? I can mention in the commit message that this one
and the Geekbox are intentionally enabled by default after the
patch since the DTS have them enabled.
I don't like hidden changes like those, even if documented. Makes
bisecting/reverting harder in my opinion. I would rather have a separate
commit(s) fixing this honestly.
[...]
diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 6740591a6533..f2f0e7dbe601 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -251,6 +251,7 @@ config MMC_DAVINCI
config MMC_DW
bool "Synopsys DesignWare Memory Card Interface"
select BOUNCE_BUFFER
+ default y if ARCH_ROCKCHIP
help
This selects support for the Synopsys DesignWare Mobile Storage IP
block, this provides host support for SD and MMC interfaces, in both
@@ -286,6 +287,7 @@ config MMC_DW_ROCKCHIP
bool "Rockchip SD/MMC controller support"
depends on OF_CONTROL
depends on MMC_DW
+ default y
default y if ARCH_ROCKCHIP
maybe? To avoid boards from other vendors to have to disable it?
Oops, I missed this.
I think
depends on ARCH_ROCKCHIP
default y
would make more sense, and also match the platform-specific options
for the other platforms.
What do you think?
Makes sense to me, to align with MMC_SDHCI_ROCKCHIP. A separate commit
for the "depends on" addition would be welcome though, I believe?
Cheers,
Quentin
