Re: [PATCH v1 3/3] pinctrl: rockchip: add rk3506 rmio support

["Heiko Stübner" <heiko@sntech.de>]

Am Dienstag, 4. November 2025, 03:12:23 Mitteleuropäische Normalzeit schrieb Ye Zhang:
> Support rockchip matrix io
> 
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 75 ++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-rockchip.h |  1 +
>  2 files changed, 76 insertions(+)

Here I disagree though.

The RMIO controller is a completely separate thing and from what I understand
from the documentation

- you set the pinmux to go to the rmio controller, and then that controller
  selects the function for this pin.

For example pinmux values for gpio0a7_sel are
- 0: GPIO0_A7
- 1: F SAI0_SDI3
- 2: SPI1_CSN1
- 7: RM_IO7

With 7 being the route to the matrix-io controller.


So lumping this into the main pinctrl feels definitly wrong, as then you
create a number of "virtual" pinmuxes where they don't belong.

So instead of trying to bolt this onto the main pinctrl, I'd like things
to be separate ... for the main iomux you route the pin to the rmio
controller and then have a separate configuration for the rmio marix.

bus2: bus2 {
	rockchip,pins = <0 RK_PA0 7 &pcfg_pull_none_drv_8ma>,
		<0 RK_PA1 7 &pcfg_pull_none_drv_8ma>,
		<0 RK_PA2 7 &pcfg_pull_none_drv_8ma>,
		<0 RK_PA3 7 &pcfg_pull_none_drv_8ma>,

	rockchip,rmio-pins {
		/* some way to sanely describe the rmio-config */
		pins = "GPIO0_A0", "GPIO0_A1";
		functions = "i2c0-scl", "i2c0-sda";
	};
};

This is especially true, as each pin in the rmio-controller can have each
function. So gpio0-a0 can be uart1-tx, uart1-rx etc etc ... 98 different
functions according to the documentation.


Heiko

> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index e44ef262beec..89ff8d8c7fcc 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -1258,6 +1258,74 @@ static int rockchip_verify_mux(struct rockchip_pin_bank *bank,
>  	return 0;
>  }
>  
> +static int rockchip_set_rmio(struct rockchip_pin_bank *bank, int pin, int *mux)
> +{
> +	struct rockchip_pinctrl *info = bank->drvdata;
> +	struct rockchip_pin_ctrl *ctrl = info->ctrl;
> +	struct regmap *regmap;
> +	int reg, function;
> +	u32 data, rmask;
> +	int ret = 0;
> +	int iomux_num = (pin / 8);
> +	u32 iomux_max, mux_type;
> +
> +	mux_type = bank->iomux[iomux_num].type;
> +	if (mux_type & IOMUX_WIDTH_4BIT)
> +		iomux_max = (1 << 4) - 1;
> +	else if (mux_type & IOMUX_WIDTH_3BIT)
> +		iomux_max = (1 << 3) - 1;
> +	else
> +		iomux_max = (1 << 2) - 1;
> +
> +	if (*mux > iomux_max)
> +		function = *mux - iomux_max;
> +	else
> +		return 0;
> +
> +	switch (ctrl->type) {
> +	case RK3506:
> +		regmap = info->regmap_rmio;
> +		if (bank->bank_num == 0) {
> +			if (pin < 24)
> +				reg = 0x80 + 0x4 * pin;
> +			else
> +				ret = -EINVAL;
> +		} else if (bank->bank_num == 1) {
> +			if (pin >= 9 && pin <= 11)
> +				reg = 0xbc + 0x4 * pin;
> +			else if (pin >= 18 && pin <= 19)
> +				reg = 0xa4 + 0x4 * pin;
> +			else if (pin >= 25 && pin <= 27)
> +				reg = 0x90 + 0x4 * pin;
> +			else
> +				ret = -EINVAL;
> +		} else {
> +			ret = -EINVAL;
> +		}
> +
> +		if (ret) {
> +			dev_err(info->dev,
> +				"rmio unsupported bank_num %d function %d\n",
> +				bank->bank_num, function);
> +
> +			return -EINVAL;
> +		}
> +
> +		rmask = 0x7f007f;
> +		data = 0x7f0000 | function;
> +		*mux = 7;
> +		ret = regmap_update_bits(regmap, reg, rmask, data);
> +		if (ret)
> +			return ret;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Set a new mux function for a pin.
>   *
> @@ -1291,6 +1359,10 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
>  
>  	dev_dbg(dev, "setting mux of GPIO%d-%d to %d\n", bank->bank_num, pin, mux);
>  
> +	ret = rockchip_set_rmio(bank, pin, &mux);
> +	if (ret)
> +		return ret;
> +
>  	if (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU)
>  		regmap = info->regmap_pmu;
>  	else if (bank->iomux[iomux_num].type & IOMUX_L_SOURCE_PMU)
> @@ -4247,6 +4319,9 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
>  	/* try to find the optional reference to the ioc1 syscon */
>  	info->regmap_ioc1 = syscon_regmap_lookup_by_phandle_optional(np, "rockchip,ioc1");
>  
> +	/* try to find the optional reference to the rmio syscon */
> +	info->regmap_rmio = syscon_regmap_lookup_by_phandle_optional(np, "rockchip,rmio");
> +
>  	ret = rockchip_pinctrl_register(pdev, info);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> index 4f4aff42a80a..6d79ccf73b71 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.h
> +++ b/drivers/pinctrl/pinctrl-rockchip.h
> @@ -462,6 +462,7 @@ struct rockchip_pinctrl {
>  	struct regmap			*regmap_pull;
>  	struct regmap			*regmap_pmu;
>  	struct regmap			*regmap_ioc1;
> +	struct regmap			*regmap_rmio;
>  	struct device			*dev;
>  	struct rockchip_pin_ctrl	*ctrl;
>  	struct pinctrl_desc		pctl;
>