| 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: <20240830080931-GYB41291@gentoo>
Date: Fri, 30 Aug 2024 08:09:31 +0000
From: Yixun Lan <dlan@...too.org>
To: Inochi Amaoto <inochiama@...look.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>,
Yangyu Chen <cyy@...self.name>, Jesse Taube <jesse@...osinc.com>,
Jisheng Zhang <jszhang@...nel.org>, Icenowy Zheng <uwu@...nowy.me>,
Meng Zhang <zhangmeng.kevin@...cemit.com>,
Meng Zhang <kevin.z.m@...mail.com>, devicetree@...r.kernel.org,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 2/4] pinctrl: spacemit: add support for SpacemiT K1 SoC
Hi Inochi:
On 09:46 Fri 30 Aug , Inochi Amaoto wrote:
> On Wed, Aug 28, 2024 at 11:30:24AM GMT, Yixun Lan wrote:
> > SpacemiT's K1 SoC has a pinctrl controller which use single register
> > to describe all functions, which include bias pull up/down(strong pull),
> > drive strength, schmitter trigger, slew rate, mux mode.
> >
> > Signed-off-by: Yixun Lan <dlan@...too.org>
> > ---
> > drivers/pinctrl/Kconfig | 1 +
> > drivers/pinctrl/Makefile | 1 +
> > drivers/pinctrl/spacemit/Kconfig | 17 +
> > drivers/pinctrl/spacemit/Makefile | 3 +
> > drivers/pinctrl/spacemit/pinctrl-k1.c | 978 ++++++++++++++++++++++++++++++++++
> > drivers/pinctrl/spacemit/pinctrl-k1.h | 180 +++++++
> > 6 files changed, 1180 insertions(+)
> >
.. snip
> > +
> > +obj-$(CONFIG_PINCTRL_SPACEMIT_K1) += pinctrl-k1.o
> > diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > new file mode 100644
> > index 0000000000000..9faac5a629c38
> > --- /dev/null
> > +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
> > @@ -0,0 +1,978 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Yixun Lan <dlan@...too.org> */
> > +
> > +
.. snip
> > +static int spacemit_request_gpio(struct pinctrl_dev *pctldev,
> > + struct pinctrl_gpio_range *range,
> > + unsigned int pin)
> > +{
> > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin);
> > + void __iomem *reg;
> > +
> > + reg = spacemit_pin_to_reg(pctrl, pin);
> > + writel(spin->gpiofunc, reg);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pinmux_ops spacemit_pmx_ops = {
> > + .get_functions_count = pinmux_generic_get_function_count,
> > + .get_function_name = pinmux_generic_get_function_name,
> > + .get_function_groups = pinmux_generic_get_function_groups,
> > + .set_mux = spacemit_pmx_set_mux,
> > + .gpio_request_enable = spacemit_request_gpio,
> > + .strict = true,
> > +};
> > +
>
> > +static int spacemit_pinconf_get(struct pinctrl_dev *pctldev,
> > + unsigned int pin, unsigned long *config)
> > +{
> > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + int param = pinconf_to_config_param(*config);
> > + u32 value, arg;
> > +
> > + if (!pin)
> > + return -EINVAL;
> > +
> > + value = readl(spacemit_pin_to_reg(pctrl, pin));
> > +
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + arg = !FIELD_GET(PAD_PULL_EN, value);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + arg = FIELD_GET(PAD_PULLDOWN, value);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + arg = FIELD_GET(PAD_PULLUP, value);
> > + break;
> > + case PIN_CONFIG_DRIVE_STRENGTH:
> > + arg = FIELD_GET(PAD_DRIVE, value);
> > + break;
> > + case PIN_CONFIG_INPUT_SCHMITT:
> > + arg = FIELD_GET(PAD_SCHMITT, value);
> > + break;
> > + case PIN_CONFIG_SLEW_RATE:
> > + arg = FIELD_GET(PAD_SLEW_RATE, value);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + *config = pinconf_to_config_packed(param, arg);
> > +
> > + return 0;
> > +}
>
> return the value follows the config requirement, not the register value.
>
ok, I rework this part
> > +
> > +#define ENABLE_DRV_STRENGTH BIT(1)
> > +#define ENABLE_SLEW_RATE BIT(2)
> > +static int spacemit_pinconf_generate_config(const struct spacemit_pin *spin,
> > + unsigned long *configs,
> > + unsigned int num_configs,
> > + u32 *value)
> > +{
> > + enum spacemit_pin_io_type type;
> > + int i, param;
> > + u32 v = 0, voltage = 0, arg, val;
> > + u32 flag = 0, drv_strength, slew_rate;
> > +
> > + if (!spin)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < num_configs; i++) {
> > + param = pinconf_to_config_param(configs[i]);
> > + arg = pinconf_to_config_argument(configs[i]);
> > +
> > + switch (param) {
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + v &= ~(PAD_PULL_EN | PAD_PULLDOWN | PAD_PULLUP);
> > + v &= ~PAD_STRONG_PULL;
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + v &= ~(PAD_PULLUP | PAD_STRONG_PULL);
> > + v |= (PAD_PULL_EN | PAD_PULLDOWN);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + v &= ~PAD_PULLDOWN;
> > + v |= (PAD_PULL_EN | PAD_PULLUP);
> > +
> > + if (arg == 1)
> > + v |= PAD_STRONG_PULL;
> > + break;
> > + case PIN_CONFIG_DRIVE_STRENGTH:
> > + flag |= ENABLE_DRV_STRENGTH;
> > + drv_strength = arg;
> > + break;
> > + case PIN_CONFIG_INPUT_SCHMITT:
> > + v &= ~PAD_SCHMITT;
> > + v |= FIELD_PREP(PAD_SCHMITT, arg);
> > + break;
> > + case PIN_CONFIG_POWER_SOURCE:
> > + voltage = arg;
> > + break;
> > + case PIN_CONFIG_SLEW_RATE:
> > + if (arg) {
> > + flag |= ENABLE_SLEW_RATE;
> > + v |= PAD_SLEW_RATE_EN;
> > + slew_rate = arg;
> > + } else {
> > + v &= ~PAD_SLEW_RATE_EN;
> > + }
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (flag & ENABLE_DRV_STRENGTH) {
> > + type = spacemit_to_pin_io_type(spin);
> > +
> > + /* fix external io type */
> > + if (type == IO_TYPE_EXTERNAL) {
> > + switch (voltage) {
> > + case 1800:
> > + type = IO_TYPE_1V8;
> > + break;
> > + case 3300:
> > + type = IO_TYPE_3V3;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + val = spacemit_get_driver_strength(type, drv_strength);
> > +
> > + v &= ~PAD_DRIVE;
> > + v |= FIELD_PREP(PAD_DRIVE, val);
> > + }
> > +
> > + if (flag & ENABLE_SLEW_RATE) {
> > + /* check, driver strength & slew rate */
> > + if (flag & ENABLE_DRV_STRENGTH) {
> > + val = FIELD_GET(PAD_SLEW_RATE, v) + 2;
> > + if (slew_rate > 1 && slew_rate != val) {
> > + pr_err("slew rate conflict with drive strength\n");
> > + return -EINVAL;
> > + }
> > + } else {
> > + v &= ~PAD_SLEW_RATE;
> > + slew_rate = slew_rate > 1 ? (slew_rate - 2) : 0;
> > + v |= FIELD_PREP(PAD_SLEW_RATE, slew_rate);
> > + }
> > + }
> > +
> > + *value = v;
> > +
> > + return 0;
> > +}
> > +
> > +static int spacemit_pin_set_config(struct spacemit_pinctrl *pctrl,
> > + unsigned int pin,
> > + u32 value)
> > +{
> > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin);
> > + void __iomem *reg;
> > + unsigned long flags;
> > + unsigned int mux;
> > +
> > + if (!pin)
> > + return -EINVAL;
> > +
> > + reg = spacemit_pin_to_reg(pctrl, spin->pin);
> > +
> > + raw_spin_lock_irqsave(&pctrl->lock, flags);
> > + mux = readl_relaxed(reg) & PAD_MUX;
> > + writel_relaxed(mux | value, reg);
> > + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int spacemit_pinconf_set(struct pinctrl_dev *pctldev,
> > + unsigned int pin, unsigned long *configs,
> > + unsigned int num_configs)
> > +{
> > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin);
> > + u32 value;
> > +
> > + if (spacemit_pinconf_generate_config(spin, configs, num_configs, &value))
> > + return -EINVAL;
> > +
> > + return spacemit_pin_set_config(pctrl, pin, value);
> > +}
> > +
> > +static int spacemit_pinconf_group_set(struct pinctrl_dev *pctldev,
> > + unsigned int gsel,
> > + unsigned long *configs,
> > + unsigned int num_configs)
> > +{
> > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct spacemit_pin *spin;
> > + const struct group_desc *group;
> > + u32 value;
> > + int i;
> > +
> > + group = pinctrl_generic_get_group(pctldev, gsel);
> > + if (!group)
> > + return -EINVAL;
> > +
> > + spin = spacemit_get_pin(pctrl, group->grp.pins[0]);
> > + if (spacemit_pinconf_generate_config(spin, configs, num_configs, &value))
> > + return -EINVAL;
> > +
> > + for (i = 0; i < group->grp.npins; i++)
> > + spacemit_pin_set_config(pctrl, group->grp.pins[i], value);
> > +
> > + return 0;
> > +}
> > +
>
> > +static void spacemit_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> > + struct seq_file *seq, unsigned int pin)
> > +{
> > + struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct spacemit_pin *spin = spacemit_get_pin(pctrl, pin);
> > + enum spacemit_pin_io_type type = spacemit_to_pin_io_type(spin);
> > + void __iomem *reg;
> > + u32 value;
> > +
> > + reg = spacemit_pin_to_reg(pctrl, pin);
> > + value = readl(reg);
> > + seq_printf(seq, ", io type (%d)", type);
> > + seq_printf(seq, ", strong pull (%ld)", FIELD_GET(PAD_STRONG_PULL, value));
> > + seq_printf(seq, ", register (0x%04x)\n", value);
> > +}
>
> drop, move the "io type" to the spacemit_pctrl_dbg_show.
Ok
> "strong pull" should be handled if you use real value in
> spacemit_pinconf_get.
>
for this reason, I will just drop it
--
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55
Powered by blists - more mailing lists