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.
> [...]
>
> > diff --git a/configs/geekbox_defconfig b/configs/geekbox_defconfig
> > index 80f91de7a11d..9dc642dc4ad9 100644
> > --- a/configs/geekbox_defconfig
> > +++ b/configs/geekbox_defconfig
> > @@ -22,6 +22,7 @@ CONFIG_REGMAP=y
> > CONFIG_SYSCON=y
> > CONFIG_BOUNCE_BUFFER=y
> > CONFIG_CLK=y
> > +# CONFIG_MMC_DW is not set
> > CONFIG_PINCTRL=y
> > CONFIG_RAM=y
> > CONFIG_DEBUG_UART_SHIFT=2
>
> This one seems to be a mistake, emmc 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).
Same here.
> [...]
>
> > 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?
Thank you and Tom for the quick responses.
ChenYu
