[PATCH] mmc: renesas_sdhi: add regulator dependency

[PATCH] mmc: renesas_sdhi: add regulator dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The driver started using the regulator subsystem and fails to build without
that now:

ERROR: modpost: "rdev_get_drvdata" [drivers/mmc/host/renesas_sdhi_core.ko] undefined!
ERROR: modpost: "devm_regulator_register" [drivers/mmc/host/renesas_sdhi_core.ko] undefined!

Add a Kconfig dependency for this.

The 'select RESET_CONTROLLER' needs to either go away or get changed to a dependency
in order to avoid Kconfig dependency loops here. The 'select' is otherwise used by
drivers that register a reset controller, but normally not by those that just use one.

Fixes: fae80a99dc03 ("mmc: renesas_sdhi: Add support for RZ/G3E SoC")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mmc/host/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index d7c90a9630e9..c0f03367d765 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -692,9 +692,9 @@ config MMC_TMIO_CORE
 
 config MMC_SDHI
 	tristate "Renesas SDHI SD/SDIO controller support"
-	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
+	depends on REGULATOR
 	select MMC_TMIO_CORE
-	select RESET_CONTROLLER if ARCH_RENESAS
 	help
 	  This provides support for the SDHI SD/SDIO controller found in
 	  Renesas SuperH, ARM and ARM64 based SoCs
-- 
2.39.5

Re: [PATCH] mmc: renesas_sdhi: add regulator dependency

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:

>  config MMC_SDHI
>  	tristate "Renesas SDHI SD/SDIO controller support"
> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
> +	depends on REGULATOR

Hmm, this is too strict IMO. SuperH does not need REGULATOR. But adding
REGULATOR to ARCH_RENESAS like RESET_CONTROLLER will still fail for
COMPILE_TEST, right?

Maybe we need to maintain the dependency in Kconfig after all:

	depends on REGULATOR if ARCH_R8A<whichever needs it>
?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] mmc: renesas_sdhi: add regulator dependency

[Arnd Bergmann]

On Sat, Mar 29, 2025, at 10:46, Wolfram Sang wrote:
> On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:
>
>>  config MMC_SDHI
>>  	tristate "Renesas SDHI SD/SDIO controller support"
>> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
>> +	depends on REGULATOR
>
> Hmm, this is too strict IMO. SuperH does not need REGULATOR.

I haven't tried building on sh, but I don't see why it wouldn't
need the regulator dependency. The code that calls it is

        rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator");
        if (!of_device_is_available(rcfg.of_node)) {
                of_node_put(rcfg.of_node);
                rcfg.of_node = NULL;
        }

        if (rcfg.of_node) {
                rcfg.driver_data = priv->host;
                rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg);
        ...

which sounds like regulators are always needed when
of_get_child_by_name() may return a non-NULL pointer, i.e.
when CONFIG_OF is enabled.

If this is correct, maybe this is the best variant:

config MMC_SDHI
  	tristate "Renesas SDHI SD/SDIO controller support"
	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
        depends on (REGULATOR && RESET_CONTROLLER) || !OF

CONFIG_ARCH_RENESAS is only set when CONFIG_OF is also set,
so both subsystem dependencies are covered by that, while
SUPERH doesn't currently enable OF, but will need the
regulator and reset controller if that patch is ever merged.

      Arnd

Re: [PATCH] mmc: renesas_sdhi: add regulator dependency

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

On Sat, Mar 29, 2025 at 11:46:10AM +0100, Arnd Bergmann wrote:
> On Sat, Mar 29, 2025, at 10:46, Wolfram Sang wrote:
> > On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:
> >
> >>  config MMC_SDHI
> >>  	tristate "Renesas SDHI SD/SDIO controller support"
> >> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> >> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
> >> +	depends on REGULATOR
> >
> > Hmm, this is too strict IMO. SuperH does not need REGULATOR.
> 
> I haven't tried building on sh, but I don't see why it wouldn't
> need the regulator dependency. The code that calls it is
> 
>         rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator");
>         if (!of_device_is_available(rcfg.of_node)) {
>                 of_node_put(rcfg.of_node);
>                 rcfg.of_node = NULL;
>         }
> 
>         if (rcfg.of_node) {
>                 rcfg.driver_data = priv->host;
>                 rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg);
>         ...
> 
> which sounds like regulators are always needed when
> of_get_child_by_name() may return a non-NULL pointer, i.e.
> when CONFIG_OF is enabled.

rcfg.of_node will be NULLed when the regulator device is not available.
Which is only true for a few SoC variants. Most other SoCs use
'vmmc-supply' insteand of 'vqmmc-regulator'.

> If this is correct, maybe this is the best variant:
> 
> config MMC_SDHI
>   	tristate "Renesas SDHI SD/SDIO controller support"
> 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>         depends on (REGULATOR && RESET_CONTROLLER) || !OF
> 
> CONFIG_ARCH_RENESAS is only set when CONFIG_OF is also set,
> so both subsystem dependencies are covered by that, while
> SUPERH doesn't currently enable OF, but will need the
> regulator and reset controller if that patch is ever merged.

Even given the above, I like this approach and think it still works.
But Biju is the SDHI regulator expert here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] mmc: renesas_sdhi: add regulator dependency

[Biju Das]

Hi All,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: 29 March 2025 11:47
> Subject: Re: [PATCH] mmc: renesas_sdhi: add regulator dependency
> 
> On Sat, Mar 29, 2025 at 11:46:10AM +0100, Arnd Bergmann wrote:
> > On Sat, Mar 29, 2025, at 10:46, Wolfram Sang wrote:
> > > On Sat, Mar 29, 2025 at 09:20:52AM +0100, Arnd Bergmann wrote:
> > >
> > >>  config MMC_SDHI
> > >>  	tristate "Renesas SDHI SD/SDIO controller support"
> > >> -	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> > >> +	depends on SUPERH || (ARCH_RENESAS && RESET_CONTROLLER) || COMPILE_TEST
> > >> +	depends on REGULATOR
> > >
> > > Hmm, this is too strict IMO. SuperH does not need REGULATOR.
> >
> > I haven't tried building on sh, but I don't see why it wouldn't need
> > the regulator dependency. The code that calls it is
> >
> >         rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator");
> >         if (!of_device_is_available(rcfg.of_node)) {
> >                 of_node_put(rcfg.of_node);
> >                 rcfg.of_node = NULL;
> >         }
> >
> >         if (rcfg.of_node) {
> >                 rcfg.driver_data = priv->host;
> >                 rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg);
> >         ...
> >
> > which sounds like regulators are always needed when
> > of_get_child_by_name() may return a non-NULL pointer, i.e.
> > when CONFIG_OF is enabled.
> 
> rcfg.of_node will be NULLed when the regulator device is not available.
> Which is only true for a few SoC variants. Most other SoCs use 'vmmc-supply' instean
d of 'vqmmc-
> regulator'.

Yes, Only few SoCs has internal regulator support and is based on
of_device_is_available().


> 
> > If this is correct, maybe this is the best variant:
> >
> > config MMC_SDHI
> >   	tristate "Renesas SDHI SD/SDIO controller support"
> > 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> >         depends on (REGULATOR && RESET_CONTROLLER) || !OF
> >
> > CONFIG_ARCH_RENESAS is only set when CONFIG_OF is also set, so both
> > subsystem dependencies are covered by that, while SUPERH doesn't
> > currently enable OF, but will need the regulator and reset controller
> > if that patch is ever merged.
> 
> Even given the above, I like this approach and think it still works.
> But Biju is the SDHI regulator expert here.

Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju