Re: [PATCH v3 2/2] pinctrl: rockchip: Add rk3576 pinctrl support

Am Montag, 19. August 2024, 17:01:50 CEST schrieb Detlev Casanova:

Hi Heiko,

On Thursday, 15 August 2024 10:05:08 EDT Heiko Stübner wrote:

quoted

Am Donnerstag, 15. August 2024, 00:30:39 CEST schrieb Detlev Casanova:

quoted

From: Steven Liu <redacted>

Add support for the 5 rk3576 GPIO banks.

This also adds support for optionnal support of the sys-grf syscon,

only one "n" in optional

quoted

used for i3c software controlled weak pull-up.

Signed-off-by: Steven Liu <redacted>
[rebase, reword commit message]
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---

 drivers/pinctrl/pinctrl-rockchip.c | 228 +++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-rockchip.h |   2 +
 2 files changed, 230 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c

b/drivers/pinctrl/pinctrl-rockchip.c index 0eacaf10c640f..110ed81d650be
100644

--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c

[...]

quoted

quoted

@@ -1234,6 +1263,20 @@ static int rockchip_set_mux(struct

rockchip_pin_bank *bank, int pin, int mux)> 
 	if (bank->recalced_mask & BIT(pin))
 	
 		rockchip_get_recalced_mux(bank, pin, &reg, &bit, 

&mask);

quoted

quoted

+	if (ctrl->type == RK3576) {
+		if ((bank->bank_num == 0) && (pin >= RK_PB4) && (pin <= 

RK_PB7))

quoted

quoted

+			reg += 0x1FF4; /* 

GPIO0_IOC_GPIO0B_IOMUX_SEL_H */

quoted

0x1ff4 please

quoted

+		/* i3c0 weakpull controlled by software */
+		if (((bank->bank_num == 0) && (pin == RK_PC5) && (mux 

== 0xb)) ||

quoted

quoted

+		    ((bank->bank_num == 1) && (pin == RK_PD1) && (mux 

== 0xa)))

quoted

quoted

+			regmap_update_bits(regmap_sys, 0x4, 

0xc000c0, 0xc000c0);

quoted

quoted

+		/* i3c1 weakpull controlled by software */
+		if (((bank->bank_num == 2) && (pin == RK_PA5) && (mux 

== 0xe)) ||

quoted

quoted

+		    ((bank->bank_num == 2) && (pin == RK_PD6) && (mux 

== 0xc)) ||

quoted

quoted

+		    ((bank->bank_num == 3) && (pin == RK_PD1) && (mux 

== 0xb)))

quoted

quoted

+			regmap_update_bits(regmap_sys, 0x4, 

0x3000300, 0x3000300);

quoted

this setting belongs into drivers/soc/rockchip/grf.c .

You want to decide that the i3c controller has no say over the pull
settings, but instead pinctrl should always be in control.

So If i understand correctly, the GRF driver should contain a rk3576 specific 
entry for default values where i3c0 and i3c1 are activated by default and not 
to be changed later then ?

I didnt realize that in this driver, the bits are only set to one, never 
cleared. So it would make sens to have them set by the GRF driver.

Something like this should do it:

#define RK3576_SYSGRF_SOC_CON1		0x6004

static const struct rockchip_grf_value rk3576_defaults[] __initconst = {
	{ "i3c0 weakpull", RK3576_SYSGRF_SOC_CON1, HIWORD_UPDATE(3, 3, 6) 
},
	{ "i3c1 weakpull", RK3576_SYSGRF_SOC_CON1, HIWORD_UPDATE(3, 3, 8) 

you're actually not configuring the "weak pull" itself, but only the mark
them as software-configured - via pinctrl. In the default setting, the i3c
controller seems to do some voodoo itself, but in the kernel we generally
want to keep control ourself, to not get surprised by the hardware doing
stuff.

So yes, that is exactly what we want.


When you do the grf-"driver" addition, you can also add the sdmmc/jtag thing
which seems to be in TOP_IOC_MISC_CON[1] ... sdmmc_force_jtag,
because we that most of the time causes issues with sd-cards down the
road.


Heiko