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

[Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>]

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) 
},
};

static const struct rockchip_grf_info rk3576_sysgrf __initconst = {
	.values = rk3576_defaults,
	.num_values = ARRAY_SIZE(rk3576_defaults),
};

> Such default system-wide settings should not clutter up the pinctrl
> driver please. The grf-"driver" exists for exactly that case.
> 
> That way you also don't need the additional grf-handling here and
> in the dt-binding.
>
> > +	}
> > +
> > 
> >  	if (ctrl->type == RK3588) {
> >  	
> >  		if (bank->bank_num == 0) {
> >  		
> >  			if ((pin >= RK_PB4) && (pin <= RK_PD7)) {
> > 
> > @@ -2038,6 +2081,142 @@ static int rk3568_calc_drv_reg_and_bit(struct
> > rockchip_pin_bank *bank,> 
> >  	return 0;
> >  
> >  }
> > 
> > +#define RK3576_DRV_BITS_PER_PIN		4
> > +#define RK3576_DRV_PINS_PER_REG		4
> > +#define RK3576_DRV_GPIO0_AL_OFFSET	0x10
> > +#define RK3576_DRV_GPIO0_BH_OFFSET	0x2014
> > +#define RK3576_DRV_GPIO1_OFFSET		0x6020
> > +#define RK3576_DRV_GPIO2_OFFSET		0x6040
> > +#define RK3576_DRV_GPIO3_OFFSET		0x6060
> > +#define RK3576_DRV_GPIO4_AL_OFFSET	0x6080
> > +#define RK3576_DRV_GPIO4_CL_OFFSET	0xA090
> > +#define RK3576_DRV_GPIO4_DL_OFFSET	0xB098
> > +
> > +static int rk3576_calc_drv_reg_and_bit(struct rockchip_pin_bank *bank,
> > +					int pin_num, struct 
regmap **regmap,
> > +					int *reg, u8 *bit)
> > +{
> > +	struct rockchip_pinctrl *info = bank->drvdata;
> > +
> > +	*regmap = info->regmap_base;
> > +
> > +	if (bank->bank_num == 0 && pin_num < 12)
> > +		*reg = RK3576_DRV_GPIO0_AL_OFFSET;
> > +	else if (bank->bank_num == 0)
> > +		*reg = RK3576_DRV_GPIO0_BH_OFFSET - 0xc;
> > +	else if (bank->bank_num == 1)
> > +		*reg = RK3576_DRV_GPIO1_OFFSET;
> > +	else if (bank->bank_num == 2)
> > +		*reg = RK3576_DRV_GPIO2_OFFSET;
> > +	else if (bank->bank_num == 3)
> > +		*reg = RK3576_DRV_GPIO3_OFFSET;
> > +	else if (bank->bank_num == 4 && pin_num < 16)
> > +		*reg = RK3576_DRV_GPIO4_AL_OFFSET;
> > +	else if (bank->bank_num == 4 && pin_num < 24)
> > +		*reg = RK3576_DRV_GPIO4_CL_OFFSET - 0x10;
> > +	else if (bank->bank_num == 4)
> > +		*reg = RK3576_DRV_GPIO4_DL_OFFSET - 0x18;
> > +	else
> > +		dev_err(info->dev, "unsupported bank_num %d\n", bank-
>bank_num);
> > +
> > +	*reg += ((pin_num / RK3576_DRV_PINS_PER_REG) * 4);
> > +	*bit = pin_num % RK3576_DRV_PINS_PER_REG;
> > +	*bit *= RK3576_DRV_BITS_PER_PIN;
> > +
> > +	return 0;
> > +}
> > +
> > +#define RK3576_PULL_BITS_PER_PIN	2
> > +#define RK3576_PULL_PINS_PER_REG	8
> > +#define RK3576_PULL_GPIO0_AL_OFFSET	0x20
> > +#define RK3576_PULL_GPIO0_BH_OFFSET	0x2028
> > +#define RK3576_PULL_GPIO1_OFFSET	0x6110
> > +#define RK3576_PULL_GPIO2_OFFSET	0x6120
> > +#define RK3576_PULL_GPIO3_OFFSET	0x6130
> > +#define RK3576_PULL_GPIO4_AL_OFFSET	0x6140
> > +#define RK3576_PULL_GPIO4_CL_OFFSET	0xA148
> > +#define RK3576_PULL_GPIO4_DL_OFFSET	0xB14C
> > +
> > +static int rk3576_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank,
> > +					 int pin_num, struct 
regmap **regmap,
> > +					 int *reg, u8 *bit)
> > +{
> > +	struct rockchip_pinctrl *info = bank->drvdata;
> > +
> > +	*regmap = info->regmap_base;
> > +
> > +	if (bank->bank_num == 0 && pin_num < 12)
> > +		*reg = RK3576_PULL_GPIO0_AL_OFFSET;
> > +	else if (bank->bank_num == 0)
> > +		*reg = RK3576_PULL_GPIO0_BH_OFFSET - 0x4;
> > +	else if (bank->bank_num == 1)
> > +		*reg = RK3576_PULL_GPIO1_OFFSET;
> > +	else if (bank->bank_num == 2)
> > +		*reg = RK3576_PULL_GPIO2_OFFSET;
> > +	else if (bank->bank_num == 3)
> > +		*reg = RK3576_PULL_GPIO3_OFFSET;
> > +	else if (bank->bank_num == 4 && pin_num < 16)
> > +		*reg = RK3576_PULL_GPIO4_AL_OFFSET;
> > +	else if (bank->bank_num == 4 && pin_num < 24)
> > +		*reg = RK3576_PULL_GPIO4_CL_OFFSET - 0x8;
> > +	else if (bank->bank_num == 4)
> > +		*reg = RK3576_PULL_GPIO4_DL_OFFSET - 0xc;
> > +	else
> > +		dev_err(info->dev, "unsupported bank_num %d\n", bank-
>bank_num);
> > +
> > +	*reg += ((pin_num / RK3576_PULL_PINS_PER_REG) * 4);
> > +	*bit = pin_num % RK3576_PULL_PINS_PER_REG;
> > +	*bit *= RK3576_PULL_BITS_PER_PIN;
> > +
> > +	return 0;
> > +}
> > +
> > +#define RK3576_SMT_BITS_PER_PIN		1
> > +#define RK3576_SMT_PINS_PER_REG		8
> > +#define RK3576_SMT_GPIO0_AL_OFFSET	0x30
> > +#define RK3576_SMT_GPIO0_BH_OFFSET	0x2040
> > +#define RK3576_SMT_GPIO1_OFFSET		0x6210
> > +#define RK3576_SMT_GPIO2_OFFSET		0x6220
> > +#define RK3576_SMT_GPIO3_OFFSET		0x6230
> > +#define RK3576_SMT_GPIO4_AL_OFFSET	0x6240
> > +#define RK3576_SMT_GPIO4_CL_OFFSET	0xA248
> > +#define RK3576_SMT_GPIO4_DL_OFFSET	0xB24C
> > +
> > +static int rk3576_calc_schmitt_reg_and_bit(struct rockchip_pin_bank
> > *bank,
> > +					   int pin_num,
> > +					   struct regmap 
**regmap,
> > +					   int *reg, u8 *bit)
> > +{
> > +	struct rockchip_pinctrl *info = bank->drvdata;
> > +
> > +	*regmap = info->regmap_base;
> > +
> > +	if (bank->bank_num == 0 && pin_num < 12)
> > +		*reg = RK3576_SMT_GPIO0_AL_OFFSET;
> > +	else if (bank->bank_num == 0)
> > +		*reg = RK3576_SMT_GPIO0_BH_OFFSET - 0x4;
> > +	else if (bank->bank_num == 1)
> > +		*reg = RK3576_SMT_GPIO1_OFFSET;
> > +	else if (bank->bank_num == 2)
> > +		*reg = RK3576_SMT_GPIO2_OFFSET;
> > +	else if (bank->bank_num == 3)
> > +		*reg = RK3576_SMT_GPIO3_OFFSET;
> > +	else if (bank->bank_num == 4 && pin_num < 16)
> > +		*reg = RK3576_SMT_GPIO4_AL_OFFSET;
> > +	else if (bank->bank_num == 4 && pin_num < 24)
> > +		*reg = RK3576_SMT_GPIO4_CL_OFFSET - 0x8;
> > +	else if (bank->bank_num == 4)
> > +		*reg = RK3576_SMT_GPIO4_DL_OFFSET - 0xc;
> > +	else
> > +		dev_err(info->dev, "unsupported bank_num %d\n", bank-
>bank_num);
> > +
> > +	*reg += ((pin_num / RK3576_SMT_PINS_PER_REG) * 4);
> > +	*bit = pin_num % RK3576_SMT_PINS_PER_REG;
> > +	*bit *= RK3576_SMT_BITS_PER_PIN;
> > +
> > +	return 0;
> > +}
> > +
> > 
> >  #define RK3588_PMU1_IOC_REG		(0x0000)
> >  #define RK3588_PMU2_IOC_REG		(0x4000)
> >  #define RK3588_BUS_IOC_REG		(0x8000)
> > 
> > @@ -2332,6 +2511,10 @@ static int rockchip_set_drive_perpin(struct
> > rockchip_pin_bank *bank,> 
> >  		rmask_bits = RK3568_DRV_BITS_PER_PIN;
> >  		ret = (1 << (strength + 1)) - 1;
> >  		goto config;
> > 
> > +	} else if (ctrl->type == RK3576) {
> > +		rmask_bits = RK3576_DRV_BITS_PER_PIN;
> > +		ret = ((strength & BIT(2)) >> 2) | ((strength & BIT(0)) 
<< 2) |
> > (strength & BIT(1)); +		goto config;
> > 
> >  	}
> >  	
> >  	if (ctrl->type == RV1126) {
> > 
> > @@ -2469,6 +2652,7 @@ static int rockchip_get_pull(struct
> > rockchip_pin_bank *bank, int pin_num)> 
> >  	case RK3368:
> >  	case RK3399:
> > 
> >  	case RK3568:
> > +	case RK3576:
> >  	case RK3588:
> >  		pull_type = bank->pull_type[pin_num / 8];
> >  		data >>= bit;
> > 
> > @@ -2528,6 +2712,7 @@ static int rockchip_set_pull(struct
> > rockchip_pin_bank *bank,> 
> >  	case RK3368:
> >  	case RK3399:
> > 
> >  	case RK3568:
> > +	case RK3576:
> >  	case RK3588:
> >  		pull_type = bank->pull_type[pin_num / 8];
> >  		ret = -EINVAL;
> > 
> > @@ -2793,6 +2978,7 @@ static bool rockchip_pinconf_pull_valid(struct
> > rockchip_pin_ctrl *ctrl,> 
> >  	case RK3368:
> >  	case RK3399:
> > 
> >  	case RK3568:
> > +	case RK3576:
> >  	case RK3588:
> >  		return (pull != PIN_CONFIG_BIAS_PULL_PIN_DEFAULT);
> >  	
> >  	}
> > 
> > @@ -3439,6 +3625,15 @@ static int rockchip_pinctrl_probe(struct
> > platform_device *pdev)> 
> >  		}
> >  	
> >  	}
> > 
> > +	/* try to find the optional reference to the sys_grf syscon */
> > +	node = of_parse_phandle(np, "rockchip,sys-grf", 0);
> > +	if (node) {
> > +		info->regmap_sys_grf = syscon_node_to_regmap(node);
> > +		of_node_put(node);
> > +		if (IS_ERR(info->regmap_sys_grf))
> > +			return PTR_ERR(info->regmap_sys_grf);
> > +	}
> > +
> 
> not needed if the i3c stuff moves into the grf driver.
> 
> >  	/* try to find the optional reference to the pmu syscon */
> >  	node = of_parse_phandle(np, "rockchip,pmu", 0);
> >  	if (node) {
> > 
> > @@ -3949,6 +4144,37 @@ static struct rockchip_pin_ctrl rk3568_pin_ctrl = {
> > 
> >  	.schmitt_calc_reg	= rk3568_calc_schmitt_reg_and_bit,
> >  
> >  };
> > 
> > +#define RK3576_PIN_BANK(ID, LABEL, OFFSET0, OFFSET1, OFFSET2, OFFSET3)	
\
> > +	PIN_BANK_IOMUX_FLAGS_OFFSET_PULL_FLAGS(ID, 32, LABEL,		
\
> > +					       
IOMUX_WIDTH_4BIT,	\
> > +					       
IOMUX_WIDTH_4BIT,	\
> > +					       
IOMUX_WIDTH_4BIT,	\
> > +					       
IOMUX_WIDTH_4BIT,	\
> > +					       OFFSET0, 
OFFSET1,	\
> > +					       OFFSET2, 
OFFSET3,	\
> > +					       
PULL_TYPE_IO_1V8_ONLY,	\
> > +					       
PULL_TYPE_IO_1V8_ONLY,	\
> > +					       
PULL_TYPE_IO_1V8_ONLY,	\
> > +					       
PULL_TYPE_IO_1V8_ONLY)
> > +
> > +static struct rockchip_pin_bank rk3576_pin_banks[] = {
> > +	RK3576_PIN_BANK(0, "gpio0", 0, 0x8, 0x2004, 0x200C),
> > +	RK3576_PIN_BANK(1, "gpio1", 0x4020, 0x4028, 0x4030, 0x4038),
> > +	RK3576_PIN_BANK(2, "gpio2", 0x4040, 0x4048, 0x4050, 0x4058),
> > +	RK3576_PIN_BANK(3, "gpio3", 0x4060, 0x4068, 0x4070, 0x4078),
> > +	RK3576_PIN_BANK(4, "gpio4", 0x4080, 0x4088, 0xA390, 0xB398),
> > +};
> > +
> > +static struct rockchip_pin_ctrl rk3576_pin_ctrl __maybe_unused = {
> > +	.pin_banks		= rk3576_pin_banks,
> > +	.nr_banks		= ARRAY_SIZE(rk3576_pin_banks),
> > +	.label			= "RK3576-GPIO",
> > +	.type			= RK3576,
> > +	.pull_calc_reg		= 
rk3576_calc_pull_reg_and_bit,
> > +	.drv_calc_reg		= rk3576_calc_drv_reg_and_bit,
> > +	.schmitt_calc_reg	= rk3576_calc_schmitt_reg_and_bit,
> > +};
> > +
> > 
> >  static struct rockchip_pin_bank rk3588_pin_banks[] = {
> >  
> >  	RK3588_PIN_BANK_FLAGS(0, 32, "gpio0",
> >  	
> >  			      IOMUX_WIDTH_4BIT, 
PULL_TYPE_IO_1V8_ONLY),
> > 
> > @@ -4005,6 +4231,8 @@ static const struct of_device_id
> > rockchip_pinctrl_dt_match[] = {> 
> >  		.data = &rk3399_pin_ctrl },
> >  	
> >  	{ .compatible = "rockchip,rk3568-pinctrl",
> >  	
> >  		.data = &rk3568_pin_ctrl },
> > 
> > +	{ .compatible = "rockchip,rk3576-pinctrl",
> > +		.data = &rk3576_pin_ctrl },
> > 
> >  	{ .compatible = "rockchip,rk3588-pinctrl",
> >  	
> >  		.data = &rk3588_pin_ctrl },
> >  	
> >  	{},
> > 
> > diff --git a/drivers/pinctrl/pinctrl-rockchip.h
> > b/drivers/pinctrl/pinctrl-rockchip.h index 849266f8b1913..0b2b56014b173
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rockchip.h
> > +++ b/drivers/pinctrl/pinctrl-rockchip.h
> > @@ -197,6 +197,7 @@ enum rockchip_pinctrl_type {
> > 
> >  	RK3368,
> >  	RK3399,
> >  	RK3568,
> > 
> > +	RK3576,
> > 
> >  	RK3588,
> >  
> >  };
> > 
> > @@ -455,6 +456,7 @@ struct rockchip_pinctrl {
> > 
> >  	int				reg_size;
> >  	struct regmap			*regmap_pull;
> >  	struct regmap			*regmap_pmu;
> > 
> > +	struct regmap			*regmap_sys_grf;
> 
> same
> 
> >  	struct device			*dev;
> >  	struct rockchip_pin_ctrl	*ctrl;
> >  	struct pinctrl_desc		pctl;
> 
> The pinctrl changes for the core rk3576 parts look fine.
> 
> Heiko