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

[Heiko Stübner <heiko@xxxxxxxxx>]

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:
> > Am Donnerstag, 15. August 2024, 00:30:39 CEST schrieb Detlev Casanova:
> > > From: Steven Liu <steven.liu@xxxxxxxxxxxxxx>
> > > 
> > > 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
> > 
> > > used for i3c software controlled weak pull-up.
> > > 
> > > Signed-off-by: Steven Liu <steven.liu@xxxxxxxxxxxxxx>
> > > [rebase, reword commit message]
> > > Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
> > > ---
> > > 
> > >  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
> [...]
> > > @@ -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);
> > > 
> > > +	if (ctrl->type == RK3576) {
> > > +		if ((bank->bank_num == 0) && (pin >= RK_PB4) && (pin <= 
> RK_PB7))
> > > +			reg += 0x1FF4; /* 
> GPIO0_IOC_GPIO0B_IOMUX_SEL_H */
> > 
> > 0x1ff4 please
> > 
> > > +		/* i3c0 weakpull controlled by software */
> > > +		if (((bank->bank_num == 0) && (pin == RK_PC5) && (mux 
> == 0xb)) ||
> > > +		    ((bank->bank_num == 1) && (pin == RK_PD1) && (mux 
> == 0xa)))
> > > +			regmap_update_bits(regmap_sys, 0x4, 
> 0xc000c0, 0xc000c0);
> > > +		/* i3c1 weakpull controlled by software */
> > > +		if (((bank->bank_num == 2) && (pin == RK_PA5) && (mux 
> == 0xe)) ||
> > > +		    ((bank->bank_num == 2) && (pin == RK_PD6) && (mux 
> == 0xc)) ||
> > > +		    ((bank->bank_num == 3) && (pin == RK_PD1) && (mux 
> == 0xb)))
> > > +			regmap_update_bits(regmap_sys, 0x4, 
> 0x3000300, 0x3000300);
> > 
> > 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