Re: [PATCH] pinctrl/rockchip: re-fix RK3308 pinmux bits

From: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Jianqun Xu <jay.xu@rock-chips.com>
Cc: heiko@sntech.de, linus.walleij@linaro.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH] pinctrl/rockchip: re-fix RK3308 pinmux bits
Date: Fri, 30 Sep 2022 18:23:58 +0200	[thread overview]
Message-ID: <20220930182358.65da06dd@booty> (raw)
In-Reply-To: <20220930102620.1568864-1-jay.xu@rock-chips.com>

Hello Jianqun Xu,

On Fri, 30 Sep 2022 18:26:20 +0800
Jianqun Xu <jay.xu@rock-chips.com> wrote:

> Part of pins from RK3308 SoCs have two registers to do pinmux, one is
> the origin register with 2bits named by gpioxx_sel, and another with
> 3bits and named by gpioxx_sel_plus.

Are the "plus" registers documented anywhere?

The reference manual I have does not mention them.

> The default value is 2bits. But Rockchip downstream pinctrl driver has a
> soc init for RK3308 to switch to the 3bits path. The first patch
> upstream the support for RK3308 pinctrl but drop the soc init codes.
> 
> The commit 1f3e25a06883 ("pinctrl: rockchip: fix RK3308 pinmux bits") try
> to fix back to 2 bits path, but that will makes some iomux not be
> supported.

Sorry about that, of course I could not know because my documentation
does not mention those registers.

> @@ -3014,6 +3014,50 @@ static int __maybe_unused rockchip_pinctrl_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops, rockchip_pinctrl_suspend,
>  			 rockchip_pinctrl_resume);
>  
> +
> +static int rk3308_soc_data_init(struct rockchip_pinctrl *info)
> +{
> +	int ret;
> +
> +	#define RK3308_GRF_SOC_CON13	(0x608)
> +	#define RK3308_GRF_SOC_CON15	(0x610)
> +
> +	/* RK3308_GRF_SOC_CON13 */
> +	#define RK3308_GRF_I2C3_IOFUNC_SRC_CTRL	(BIT(16 + 10) | BIT(10))
> +	#define RK3308_GRF_GPIO2A3_SEL_SRC_CTRL	(BIT(16 + 7)  | BIT(7))
> +	#define RK3308_GRF_GPIO2A2_SEL_SRC_CTRL	(BIT(16 + 3)  | BIT(3))
> +
> +	/* RK3308_GRF_SOC_CON15 */
> +	#define RK3308_GRF_GPIO2C0_SEL_SRC_CTRL	(BIT(16 + 11) | BIT(11))
> +	#define RK3308_GRF_GPIO3B3_SEL_SRC_CTRL	(BIT(16 + 7)  | BIT(7))
> +	#define RK3308_GRF_GPIO3B2_SEL_SRC_CTRL	(BIT(16 + 3)  | BIT(3))
> +
> +	/*
> +	 * Enable the special ctrl of selected sources.
> +	 *
> +	 * Example reference to GRF_SOC_CON13 description:
> +	 *
> +	 * gpio2a2_sel_src_ctrl
> +	 * IOMUX control source selection.
> +	 * 1'b0: use basic GPIO2A_IOMUX[gpio2a2_sel]
> +	 * 1'b1: use gpio2a2_sel_plus instead of GPIO2A_IOMUX[gpio2a2_sel]
> +	 */
> +
> +	ret = regmap_write(info->regmap_base, RK3308_GRF_SOC_CON13,
> +			   RK3308_GRF_I2C3_IOFUNC_SRC_CTRL |
> +			   RK3308_GRF_GPIO2A3_SEL_SRC_CTRL |
> +			   RK3308_GRF_GPIO2A2_SEL_SRC_CTRL);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(info->regmap_base, RK3308_GRF_SOC_CON15,
> +			   RK3308_GRF_GPIO2C0_SEL_SRC_CTRL |
> +			   RK3308_GRF_GPIO3B3_SEL_SRC_CTRL |
> +			   RK3308_GRF_GPIO3B2_SEL_SRC_CTRL);
> +
> +	return ret;
> +}

This is new code, not code that my patch has removed. Is it needed? How
did the driver work before without this code?

I think we need to clarify those registers before applying a fix that
might create other problems.

Best regards.
-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip