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

linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed

* Re: [PATCH v3 2/2] pinctrl: rockchip: Add rk3576 pinctrl support
       [not found] ` <20240814223217.3498-3-detlev.casanova@collabora.com>
@ 2024-08-15 14:05   ` Heiko Stübner
       [not found]     ` <3025828.e9J7NaK4W3@trenzalore>
  0 siblings, 1 reply; 3+ messages in thread
From: Heiko Stübner @ 2024-08-15 14:05 UTC (permalink / raw)
  To: linux-kernel, Detlev Casanova
  Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-gpio, devicetree, linux-arm-kernel, linux-rockchip, kernel,
	Steven Liu, Detlev Casanova

Am Donnerstag, 15. August 2024, 00:30:39 CEST schrieb Detlev Casanova:
> From: Steven Liu <steven.liu@rock-chips.com>
> 
> 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@rock-chips.com>
> [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
> @@ -84,6 +84,27 @@
>  		},							\
>  	}
>  
> +#define PIN_BANK_IOMUX_FLAGS_OFFSET_PULL_FLAGS(id, pins, label, iom0,	\
> +					       iom1, iom2, iom3,	\
> +					       offset0, offset1,	\
> +					       offset2, offset3, pull0,	\
> +					       pull1, pull2, pull3)	\
> +	{								\
> +		.bank_num	= id,					\
> +		.nr_pins	= pins,					\
> +		.name		= label,				\
> +		.iomux		= {					\
> +			{ .type = iom0, .offset = offset0 },		\
> +			{ .type = iom1, .offset = offset1 },		\
> +			{ .type = iom2, .offset = offset2 },		\
> +			{ .type = iom3, .offset = offset3 },		\
> +		},							\
> +		.pull_type[0] = pull0,					\
> +		.pull_type[1] = pull1,					\
> +		.pull_type[2] = pull2,					\
> +		.pull_type[3] = pull3,					\
> +	}
> +
>  #define PIN_BANK_DRV_FLAGS(id, pins, label, type0, type1, type2, type3) \
>  	{								\
>  		.bank_num	= id,					\
> @@ -1120,6 +1141,11 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin)
>  	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 in lower case please


> +	}
> +
>  	if (ctrl->type == RK3588) {
>  		if (bank->bank_num == 0) {
>  			if ((pin >= RK_PB4) && (pin <= RK_PD7)) {
> @@ -1193,6 +1219,7 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
>  	struct device *dev = info->dev;
>  	int iomux_num = (pin / 8);
>  	struct regmap *regmap;
> +	struct regmap *regmap_sys;
>  	int reg, ret, mask, mux_type;
>  	u8 bit;
>  	u32 data, rmask, route_location, route_reg, route_val;
> @@ -1213,6 +1240,8 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
>  	else
>  		regmap = info->regmap_base;
>  
> +	regmap_sys = info->regmap_sys_grf;
> +
>  	/* get basic quadrupel of mux registers and the correct reg inside */
>  	mux_type = bank->iomux[iomux_num].type;
>  	reg = bank->iomux[iomux_num].offset;
> @@ -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.

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




^ permalink raw reply	[flat|nested] 3+ messages in thread

[parent not found: <3025828.e9J7NaK4W3@trenzalore>]

* Re: [PATCH v3 2/2] pinctrl: rockchip: Add rk3576 pinctrl support
       [not found]     ` <3025828.e9J7NaK4W3@trenzalore>
@ 2024-08-19 15:32       ` Heiko Stübner
  0 siblings, 0 replies; 3+ messages in thread
From: Heiko Stübner @ 2024-08-19 15:32 UTC (permalink / raw)
  To: linux-kernel, Detlev Casanova
  Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-gpio, devicetree, linux-arm-kernel, linux-rockchip, kernel,
	Steven Liu

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@rock-chips.com>
> > > 
> > > 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@rock-chips.com>
> > > [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
> [...]
> > > @@ -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




^ permalink raw reply	[flat|nested] 3+ messages in thread

[parent not found: <20240814223217.3498-2-detlev.casanova@collabora.com>]

* Re: [PATCH v3 1/2] dt-bindings: pinctrl: Add rk3576 pinctrl bindings
       [not found] ` <20240814223217.3498-2-detlev.casanova@collabora.com>
@ 2024-08-15 15:05   ` Conor Dooley
  0 siblings, 0 replies; 3+ messages in thread
From: Conor Dooley @ 2024-08-15 15:05 UTC (permalink / raw)
  To: Detlev Casanova
  Cc: linux-kernel, Linus Walleij, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Heiko Stuebner, linux-gpio, devicetree,
	linux-arm-kernel, linux-rockchip, kernel

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

On Wed, Aug 14, 2024 at 06:30:38PM -0400, Detlev Casanova wrote:
> Add the compatible string as well as the rockchip,sys-grf field which is
> only used on this SoC.
> 
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

> ---
>  .../bindings/pinctrl/rockchip,pinctrl.yaml     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> index 20e806dce1ecb..28f8dc412cf92 100644
> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> @@ -45,6 +45,7 @@ properties:
>        - rockchip,rk3368-pinctrl
>        - rockchip,rk3399-pinctrl
>        - rockchip,rk3568-pinctrl
> +      - rockchip,rk3576-pinctrl
>        - rockchip,rk3588-pinctrl
>        - rockchip,rv1108-pinctrl
>        - rockchip,rv1126-pinctrl
> @@ -62,6 +63,12 @@ properties:
>        Required for at least rk3188 and rk3288. On the rk3368 this should
>        point to the PMUGRF syscon.
>  
> +  rockchip,sys-grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      The phandle of the syscon node for the SYS GRF registers.
> +      It is used on rk3576 for i3c software controlled weak pull-up.
> +
>    "#address-cells":
>      enum: [1, 2]
>  
> @@ -72,6 +79,17 @@ properties:
>  
>  allOf:
>    - $ref: pinctrl.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3576-pinctrl
> +    then:
> +      required:
> +        - rockchip,sys-grf
> +    else:
> +      properties:
> +        rockchip,sys-grf: false
>  
>  required:
>    - compatible
> -- 
> 2.46.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-08-19 15:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20240814223217.3498-1-detlev.casanova@collabora.com>
     [not found] ` <20240814223217.3498-3-detlev.casanova@collabora.com>
2024-08-15 14:05   ` [PATCH v3 2/2] pinctrl: rockchip: Add rk3576 pinctrl support Heiko Stübner
     [not found]     ` <3025828.e9J7NaK4W3@trenzalore>
2024-08-19 15:32       ` Heiko Stübner
     [not found] ` <20240814223217.3498-2-detlev.casanova@collabora.com>
2024-08-15 15:05   ` [PATCH v3 1/2] dt-bindings: pinctrl: Add rk3576 pinctrl bindings Conor Dooley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).