[PATCH 05/11] rockchip: io-domain: Add support for RK3399

[Quentin Schulz]

Hi Jonas,

On 2/17/24 19:35, Jonas Karlman wrote:
> Port the RK3399 part of the Rockchip IO-domain driver from linux.
> 
> This differs from linux version in that pmu io iodomain bit is enabled
> in the write ops instead of in an init ops as in linux, this way we can
> avoid keeping a full state of all supply that have been configured.
> 
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>

Suggestions below, otherwise I only have one concern:

This actually postpones the moment the IO domain is properly set. 
Before, we knew almost for sure when this was set, as it was pretty 
early in the boot process (misc_init_r), I hope before any device was 
actually probed. Now, because we're using the Device Model for that, we 
may probe this IO domain device after another Device requires it? e.g. 
for Puma, we need the IO domains set for PCIe to work. I assume if we 
were to use PCIe in U-Boot on Puma, we may have this issue (we don't 
right now use PCIe in U-Boot and I don't think we have any plan of doing 
so).

This all reminds me of the attempts we've made in the Linux kernel to 
make sure the IO domain devices are probed before the devices that 
require them to be set up. Maybe we'll need something similar in U-Boot 
as well.

> ---
>   drivers/misc/rockchip-io-domain.c | 79 +++++++++++++++++++++++++++++--
>   1 file changed, 75 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/rockchip-io-domain.c b/drivers/misc/rockchip-io-domain.c
> index 3f6227f993f9..0ffea32ef07f 100644
> --- a/drivers/misc/rockchip-io-domain.c
> +++ b/drivers/misc/rockchip-io-domain.c
> @@ -5,7 +5,6 @@
>    * Ported from linux drivers/soc/rockchip/io-domain.c
>    */
>   
> -#include <common.h>
>   #include <dm.h>
>   #include <dm/device_compat.h>
>   #include <regmap.h>
> @@ -28,6 +27,10 @@
>   #define MAX_VOLTAGE_1_8		1980000
>   #define MAX_VOLTAGE_3_3		3600000
>   
> +#define RK3399_PMUGRF_CON0		0x180
> +#define RK3399_PMUGRF_CON0_VSEL		BIT(8)
> +#define RK3399_PMUGRF_VSEL_SUPPLY_NUM	9
> +
>   #define RK3568_PMU_GRF_IO_VSEL0		0x0140
>   #define RK3568_PMU_GRF_IO_VSEL1		0x0144
>   #define RK3568_PMU_GRF_IO_VSEL2		0x0148
> @@ -35,10 +38,10 @@
>   struct rockchip_iodomain_soc_data {
>   	int grf_offset;
>   	const char *supply_names[MAX_SUPPLIES];
> -	int (*write)(struct regmap *grf, int idx, int uV);
> +	int (*write)(struct regmap *grf, uint offset, int idx, int uV);
>   };
>   
> -static int rk3568_iodomain_write(struct regmap *grf, int idx, int uV)
> +static int rk3568_iodomain_write(struct regmap *grf, uint offset, int idx, int uV)
>   {
>   	u32 is_3v3 = uV > MAX_VOLTAGE_1_8;
>   	u32 val0, val1;
> @@ -78,6 +81,64 @@ static int rk3568_iodomain_write(struct regmap *grf, int idx, int uV)
>   	return 0;
>   }
>   
> +static int rockchip_iodomain_write(struct regmap *grf, uint offset, int idx, int uV)
> +{
> +	u32 val;
> +
> +	/* set value bit */
> +	val = (uV > MAX_VOLTAGE_1_8) ? 0 : 1;
> +	val <<= idx;
> +
> +	/* apply hiword-mask */
> +	val |= (BIT(idx) << 16);
> +
> +	return regmap_write(grf, offset, val);

You could use:

"""
if (uV > MAX_VOLTAGE_1_8) {
     val = RK_CLRBITS(BIT(idx));
} else {
     val = RK_SETBITS(BIT(idx));
}

return regmap_write(grf, offset, val);
"""

by including asm/rockchip/hardware.h.

> +}
> +
> +static int rk3399_pmu_iodomain_write(struct regmap *grf, uint offset, int idx, int uV)
> +{
> +	int ret = rockchip_iodomain_write(grf, offset, idx, uV);
> +
> +	if (!ret && idx == RK3399_PMUGRF_VSEL_SUPPLY_NUM) {
> +		/*
> +		 * set pmu io iodomain to also use this framework
> +		 * instead of a special gpio.
> +		 */
> +		u32 val = RK3399_PMUGRF_CON0_VSEL | (RK3399_PMUGRF_CON0_VSEL << 16);
> +		ret = regmap_write(grf, RK3399_PMUGRF_CON0, val);

Same here, but two options :)

"""
ret = regmap_write(grf, RK3399_PMUGRF_CON0, 
RK_SETBITS(RK3399_PMUGRF_CON0_VSEL));
"""

or
"""
ret = regmap_write(grf, RK3399_PMUGRF_CON0, 
FIELD_PREP(RK3399_PMUGRF_CON0_VSEL, 1));
"""

Maybe the if check on the offset is actually a bit overkill? This 
function can only be called for one IO domain since there's only one IO 
domain in the PMUGRF in RK3399.

Therefore.... Maybe just hardcode everything?

"""
static int rk3399_pmu_iodomain_write(struct regmap *grf, uint offset, 
int idx, int uV)
{
     int ret = rockchip_iodomain_write(grf, offset, 9, uV);
     if (ret)
         return ret;

     return regmap_write(grf, RK3399_PMUGRF_CON0, 
FIELD_PREP(RK3399_PMUGRF_CON0_VSEL, 1));
}

static const struct rockchip_iodomain_soc_data soc_data_rk3399_pmu = {
	.grf_offset = 0x180,
	.supply_names = {
		"pmu1830-supply",	/* PMUIO2_VDD */
	},
	.write = rk3399_pmu_iodomain_write,
};
"""

You can also keep the 8 NULL supplies before to not have to hardcode "9" 
in rk3399_pmu_iodomain_write for example, and still not do the check, 
adding a comment explaining why.

The code looks sound in any case, only issue I have is wrt device probe 
ordering in U-Boot.

Cheers,
Quentin