[PATCH 1/2] mmc: rockchip_dw_mmc: Enable by default for all supported SoCs

[Chen-Yu Tsai]

On Wed, Apr 30, 2025 at 12:30 AM Quentin Schulz
<quentin.schulz at cherry.de> wrote:
>
> Hi Chen-Yu,
>
> On 4/29/25 5:44 PM, Chen-Yu Tsai wrote:
> > From: Chen-Yu Tsai <wens at 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