Hi Jonas, Thanks for the quick initial review.
On Jan 24, 2025 at 14:25:25, Jonas Karlman <[email protected]> wrote: > Hi Justin, > > On 2025-01-24 22:19, Justin Klaassen wrote: > > The NanoPi R4S supports UHS-I (up to SDR104) SD cards, however using any > > of these 1.8v modes results in a boot failure in SPL upon soft reboot. > > > On the R4S the SD card power cannot be cycled, so upon reboot the SD > > card continues to operate at 1.8v but the GPIO is configured by default > > at the 3.3v level. > > > Sounds like similar/same issue [1] that I faced on Asus Tinker Board a > few years ago :-) > > [1] https://patchwork.kernel.org/patch/10817217/ > > > This change enables the RK8XX regulators and Rockchip IO-domain drivers > > in SPL, which reads the voltage level of the "vcc_sdio" regulator and > > configures the GPIO for the appropriate level on boot. This allows the > > MMC subsystem to operate successfully at the 1.8v level in SPL. > > > Signed-off-by: Justin Klaassen <[email protected]> > > --- > > > arch/arm/dts/rk3399-nanopi4-u-boot.dtsi | 9 +++++++++ > > configs/nanopi-r4s-rk3399_defconfig | 7 +++++++ > > drivers/misc/Kconfig | 8 ++++++++ > > drivers/misc/rockchip-io-domain.c | 7 ++++++- > > drivers/power/regulator/Kconfig | 9 +++++++++ > > drivers/power/regulator/rk8xx.c | 2 +- > > 6 files changed, 40 insertions(+), 2 deletions(-) > > > diff --git a/arch/arm/dts/rk3399-nanopi4-u-boot.dtsi > b/arch/arm/dts/rk3399-nanopi4-u-boot.dtsi > > index 75736124996..146b62c2e7a 100644 > > --- a/arch/arm/dts/rk3399-nanopi4-u-boot.dtsi > > +++ b/arch/arm/dts/rk3399-nanopi4-u-boot.dtsi > > @@ -23,4 +23,13 @@ > > > > &vcc_sdio { > > regulator-init-microvolt = <3000000>; > > + bootph-pre-ram; > > +}; > > + > > +&io_domains { > > + bootph-pre-ram; > > +}; > > + > > +&i2c0_xfer { > > > Please sort nodes in alphabetical order based on label. > > These should probably be added to rk3399-nanopi-r4s-u-boot.dtsi since > that is the only defconfig you enable relevant Kconfig options for. > I believe this same issue and fix applies to at least all the rk3399 boards using rk3399-nanopi4-u-boot.dtsi: nanopc-t4-rk3399 nanopi-m4-2gb-rk3399 nanopi-m4-rk3399 nanopi-m4b-rk3399 nanopi-neo4-rk3399 nanopi-r4s-rk3399 I only have a R4S to test with, but I did verify that all of those boards have the same schematic and pin configuration for the sdmmc slot. Given this information do you prefer I keep this change scoped to the R4S for now or should I add the defconfig changes for those boards as well? > + bootph-pre-ram; > > }; > > diff --git a/configs/nanopi-r4s-rk3399_defconfig > b/configs/nanopi-r4s-rk3399_defconfig > > index a6dafe3d9eb..905de05026f 100644 > > --- a/configs/nanopi-r4s-rk3399_defconfig > > +++ b/configs/nanopi-r4s-rk3399_defconfig > > @@ -8,6 +8,7 @@ CONFIG_ENV_OFFSET=0x3F8000 > > CONFIG_DEFAULT_DEVICE_TREE="rockchip/rk3399-nanopi-r4s" > > CONFIG_DM_RESET=y > > CONFIG_ROCKCHIP_RK3399=y > > +CONFIG_SPL_DRIVERS_MISC=y > > CONFIG_TARGET_EVB_RK3399=y > > CONFIG_SYS_LOAD_ADDR=0x800800 > > CONFIG_DEBUG_UART_BASE=0xFF1A0000 > > @@ -18,6 +19,8 @@ CONFIG_DISPLAY_BOARDINFO_LATE=y > > CONFIG_SPL_MAX_SIZE=0x40000 > > CONFIG_SPL_PAD_TO=0x7f8000 > > # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set > > +CONFIG_SPL_I2C=y > > +CONFIG_SPL_POWER=y > > CONFIG_SPL_ATF_NO_PLATFORM_PARAM=y > > CONFIG_TPL=y > > CONFIG_CMD_BOOTZ=y > > @@ -33,6 +36,7 @@ CONFIG_SYS_RELOC_GD_ENV_ADDR=y > > CONFIG_ROCKCHIP_GPIO=y > > CONFIG_SYS_I2C_ROCKCHIP=y > > CONFIG_ROCKCHIP_IODOMAIN=y > > +CONFIG_SPL_ROCKCHIP_IODOMAIN=y > > CONFIG_MMC_DW=y > > CONFIG_MMC_DW_ROCKCHIP=y > > CONFIG_MMC_SDHCI=y > > @@ -45,8 +49,11 @@ CONFIG_GMAC_ROCKCHIP=y > > CONFIG_PHY_ROCKCHIP_INNO_USB2=y > > CONFIG_PHY_ROCKCHIP_TYPEC=y > > CONFIG_PMIC_RK8XX=y > > +CONFIG_SPL_PMIC_RK8XX=y > > +CONFIG_SPL_DM_REGULATOR=y > > CONFIG_SPL_DM_REGULATOR_FIXED=y > > CONFIG_REGULATOR_RK8XX=y > > +CONFIG_SPL_REGULATOR_RK8XX=y > > CONFIG_PWM_ROCKCHIP=y > > CONFIG_RAM_ROCKCHIP_LPDDR4=y > > CONFIG_BAUDRATE=1500000 > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index da84b35e804..ad935b81ae1 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -110,6 +110,14 @@ config ROCKCHIP_IODOMAIN > > for the IO-domain setting of the SoC to match the voltage supplied > > by the regulators. > > > > +config SPL_ROCKCHIP_IODOMAIN > > + bool "Rockchip IO-domain driver support in SPL" > > + depends on SPL_MISC && ARCH_ROCKCHIP > > + help > > + Enable support for IO-domains in Rockchip SoCs in SPL. It is necessary > > + for the IO-domain setting of the SoC to match the voltage supplied > > + by the regulators. > > + > > config SIFIVE_OTP > > bool "SiFive eMemory OTP driver" > > depends on MISC > > diff --git a/drivers/misc/rockchip-io-domain.c > b/drivers/misc/rockchip-io-domain.c > > index 025b6049a9f..d24d0781b08 100644 > > --- a/drivers/misc/rockchip-io-domain.c > > +++ b/drivers/misc/rockchip-io-domain.c > > @@ -5,6 +5,8 @@ > > * Ported from linux drivers/soc/rockchip/io-domain.c > > */ > > > > +#define LOG_CATEGORY UCLASS_NOP > > + > > #include <dm.h> > > #include <dm/device_compat.h> > > #include <regmap.h> > > @@ -344,8 +346,10 @@ static int rockchip_iodomain_probe(struct udevice > *dev) > > continue; > > > > ret = device_get_supply_regulator(dev, supply_name, ®); > > - if (ret) > > + if (ret) { > > + log_debug("%s: Missing regulator for %s\n", __func__, supply_name); > > > log_debug() is already able to log function name with LOG=y and > LOGF_FUNC=y, please drop __func__. > > continue; > > + } > > > > ret = regulator_autoset(reg); > > if (ret && ret != -EALREADY && ret != -EMEDIUMTYPE && > > @@ -353,6 +357,7 @@ static int rockchip_iodomain_probe(struct udevice *dev) > > continue; > > > > uV = regulator_get_value(reg); > > + log_debug("%s: %s = %dV\n", __func__, supply_name, uV); > > > Same here. > > Also for consistency please use a verbose or minimalist log message, > do not mix. This has a verbose 'Missing regulator' above and minimalist > message here. > > if (uV <= 0) > > continue; > > > > diff --git a/drivers/power/regulator/Kconfig > b/drivers/power/regulator/Kconfig > > index 958f337c7e7..d873ca06a47 100644 > > --- a/drivers/power/regulator/Kconfig > > +++ b/drivers/power/regulator/Kconfig > > @@ -241,6 +241,15 @@ config REGULATOR_RK8XX > > by the PMIC device. This driver is controlled by a device tree node > > which includes voltage limits. > > > > +config SPL_REGULATOR_RK8XX > > + bool "Enable driver for RK8XX regulators in SPL" > > + depends on SPL_DM_REGULATOR && SPL_PMIC_RK8XX > > + ---help--- > > > Use help keyword without '---' and indent the help text. > > + Enable support for the regulator functions of the RK8XX PMIC in SPL. The > > + driver implements get/set api for the various BUCKS and LDOs supported > > + by the PMIC device. This driver is controlled by a device tree node > > + which includes voltage limits. > > + > > config DM_REGULATOR_S2MPS11 > > bool "Enable driver for S2MPS11 regulator" > > depends on DM_REGULATOR && PMIC_S2MPS11 > > diff --git a/drivers/power/regulator/rk8xx.c > b/drivers/power/regulator/rk8xx.c > > index 368675ebb9f..35615e96b1c 100644 > > --- a/drivers/power/regulator/rk8xx.c > > +++ b/drivers/power/regulator/rk8xx.c > > @@ -16,7 +16,7 @@ > > #include <power/pmic.h> > > #include <power/regulator.h> > > > > -#ifndef CONFIG_XPL_BUILD > > +#if CONFIG_IS_ENABLED(REGULATOR_RK8XX) > > > Please drop the whole ENABLE_DRIVER instead and use > CONFIG_IS_ENABLED in its place. > > #define ENABLE_DRIVER > > #endif > > > > > This should also be split up into multiple patches, possible: > - io-domain debug logging > - io-domain SPL Kconfig symbol > - rk8xx.c + related Kconfig can possible go together > - nanopi-r4s-u-boot.dtsi + defconfig can possible go together > > Regards, > Jonas > > Best, Justin
