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

[Quentin Schulz]

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.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.
> 

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