| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3025828.e9J7NaK4W3@trenzalore>
Date: Mon, 19 Aug 2024 11:01:50 -0400
From: Detlev Casanova <detlev.casanova@...labora.com>
To: linux-kernel@...r.kernel.org,
Heiko Stübner <heiko@...ech.de>
Cc: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
kernel@...labora.com, Steven Liu <steven.liu@...k-chips.com>
Subject: Re: [PATCH v3 2/2] pinctrl: rockchip: Add rk3576 pinctrl support
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@...k-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@...k-chips.com>
> > [rebase, reword commit message]
> > Signed-off-by: Detlev Casanova <detlev.casanova@...labora.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, ®, &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
Powered by blists - more mailing lists