| 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: <CAMuHMdVig3M_UY8i=+gJD-gkW90tQiS_cNDJqdRe5e-Z8kNoDg@mail.gmail.com>
Date: Wed, 22 May 2024 17:29:58 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.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>,
Magnus Damm <magnus.damm@...il.com>, linux-renesas-soc@...r.kernel.org,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 13/13] pinctrl: renesas: pinctrl-rzg2l: Add support for
RZ/V2H SoC
Hi Prabhakar,
On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Add pinctrl driver support for RZ/V2H(P) SoC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> RFC->v2
> - Renamed renesas-rzv2h,output-impedance -> renesas,output-impedance
> - Dropped IOLH groups
> - Fixed dedicated pin configs
> - Updated r9a09g057_variable_pin_cfg
> - Added support OEN
> - Added support for bias settings
> - Added function pointers for pwpr (un)lock
> - Added support for slew-rate
Thanks for the update!
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -59,6 +59,10 @@
> #define PIN_CFG_OEN BIT(15)
> #define PIN_CFG_VARIABLE BIT(16)
> #define PIN_CFG_NOGPIO_INT BIT(17)
> +#define PIN_CFG_OPEN_DRAIN BIT(18)
Or PIN_CFG_NOD, to match the docs?
You can always add a comment if the meaning is unclear:
/* N-ch Open Drain */
> +#define PIN_CFG_SCHMIT_CTRL BIT(19)
SCHMITT (double T). Or just call it PIN_CFG_SMT, to match the docs?
/* Schmitt-trigger input control */
> +#define PIN_CFG_ELC BIT(20)
/* Event Link Control */
> +#define PIN_CFG_IOLH_RZV2H BIT(21)
>
> #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
> (PIN_CFG_IOLH_##group | \
> @@ -73,6 +77,10 @@
> #define RZG3S_MPXED_PIN_FUNCS(group) (RZG2L_MPXED_COMMON_PIN_FUNCS(group) | \
> PIN_CFG_SOFT_PS)
>
> +#define RZV2H_MPXED_PIN_FUNCS (RZG2L_MPXED_COMMON_PIN_FUNCS(RZV2H) | \
> + PIN_CFG_OPEN_DRAIN | \
> + PIN_CFG_SR)
I think you can include PIN_CFG_SCHMIT_CTRL here, and thus drop it
from all tables below?
> +
> #define RZG2L_MPXED_ETH_PIN_FUNCS(x) ((x) | \
> PIN_CFG_FILONOFF | \
> PIN_CFG_FILNUM | \
> @@ -128,13 +136,15 @@
> #define ETH_POC(off, ch) ((off) + (ch) * 4)
> #define QSPI (0x3008)
> #define ETH_MODE (0x3018)
> +#define PFC_OEN (0x3C40) /* known on RZ/V2H(P) only */
>
> #define PVDD_2500 2 /* I/O domain voltage 2.5V */
> #define PVDD_1800 1 /* I/O domain voltage <= 1.8V */
> #define PVDD_3300 0 /* I/O domain voltage >= 3.3V */
>
> #define PWPR_B0WI BIT(7) /* Bit Write Disable */
FWIW, this should be PWPR_BOWI (O like in Oscar, not 0 = Zero).
> -#define PWPR_PFCWE BIT(6) /* PFC Register Write Enable */
> +#define BIT(6) /* PFC (and PMC on RZ/V2H) Register Write Enable */
As this bit is called differently on RZ/V2H, and there are different
code paths to handle PWPR on RZ/V2H vs. RZ/G2L, please add an extra
definition for PWPR_REGWE_A, and use that in RZ/V2H-specific
functions.
> +#define PWPR_REGWE_B BIT(5) /* OEN Register Write Enable, known only in RZ/V2H(P) */
>
> #define PM_MASK 0x03
> #define PFC_MASK 0x07
\
> @@ -330,6 +353,8 @@ struct rzg2l_pinctrl {
> spinlock_t lock; /* lock read/write registers */
> struct mutex mutex; /* serialize adding groups and functions */
>
> + raw_spinlock_t pwpr_lock; /* serialize PWPR register access */
Do you need this lock?
I.e. can't you use the existing .lock above instead? (see below)
> +
> struct rzg2l_pinctrl_pin_settings *settings;
> struct rzg2l_pinctrl_reg_cache *cache;
> struct rzg2l_pinctrl_reg_cache *dedicated_cache;
> @@ -480,6 +538,19 @@ static void rzg2l_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *
> writeb(val, addr);
> }
>
> +static void rzv2h_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr)
> +{
> + const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> + u8 pwpr;
> +
> + raw_spin_lock(&pctrl->pwpr_lock);
rzg2l_pinctrl_data.pmc_write() is always called with rzg2l_pinctrl.lock
held.
> + pwpr = readb(pctrl->base + regs->pwpr);
> + writeb(pwpr | PWPR_PFCWE, pctrl->base + regs->pwpr);
PWPR_REGWE_A
> + writeb(val, addr);
> + writeb(pwpr & ~PWPR_PFCWE, pctrl->base + regs->pwpr);
PWPR_REGWE_A
> + raw_spin_unlock(&pctrl->pwpr_lock);
> +}
> +
> static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> u8 pin, u8 off, u8 func)
> {
> +static u8 rzv2h_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, u32 offset)
> +{
> + static const char * const pin_names[] = { "ET0_TXC_TXCLK", "ET1_TXC_TXCLK",
> + "XSPI0_RESET0N", "XSPI0_CS0N",
> + "XSPI0_CKN", "XSPI0_CKP" };
static const char * const pin_names[] = {
"ET0_TXC_TXCLK", "ET1_TXC_TXCLK", "XSPI0_RESET0N",
"XSPI0_CS0N", "XSPI0_CKN", "XSPI0_CKP"
};
> + const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset];
> + u8 bit_array[] = { 0, 1, 2, 3, 4, 5 };
Do you need this identity-transforming array? ;-)
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(bit_array); i++) {
ARRAY_SIZE(pin_names)
> + if (!strcmp(pin_desc->name, pin_names[i]))
> + return bit_array[i];
return i;
> + }
> +
> + /* Should not happen. */
> + return 0;
> +}
> +
> +static u32 rzv2h_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin)
> +{
> + u8 bit;
> +
> + if (!(caps & PIN_CFG_OEN))
> + return 0;
> +
> + bit = rzv2h_pin_to_oen_bit(pctrl, offset);
> +
> + return !(readb(pctrl->base + PFC_OEN) & BIT(bit));
> +}
> +
> +static int rzv2h_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen)
> +{
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> + unsigned long flags;
> + u8 val, bit;
> + u8 pwpr;
> +
> + if (!(caps & PIN_CFG_OEN))
> + return -EINVAL;
> +
> + bit = rzv2h_pin_to_oen_bit(pctrl, offset);
> + spin_lock_irqsave(&pctrl->lock, flags);
> + val = readb(pctrl->base + PFC_OEN);
> + if (oen)
> + val &= ~BIT(bit);
> + else
> + val |= BIT(bit);
> +
> + raw_spin_lock(&pctrl->pwpr_lock);
rzg2l_pinctrl.lock is taken above.
> + pwpr = readb(pctrl->base + regs->pwpr);
> + writeb(pwpr | PWPR_REGWE_B, pctrl->base + regs->pwpr);
> + writeb(val, pctrl->base + PFC_OEN);
> + writeb(pwpr & ~PWPR_REGWE_B, pctrl->base + regs->pwpr);
> + raw_spin_unlock(&pctrl->pwpr_lock);
> + spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> + return 0;
> +}
> @@ -2747,6 +3098,32 @@ static void rzg2l_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl)
> writel(PWPR_B0WI, pctrl->base + regs->pwpr); /* B0WI=1, PFCWE=0 */
> }
>
> +static void rzv2h_pwpr_pfc_unlock(struct rzg2l_pinctrl *pctrl)
> +{
> + const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> + u8 pwpr;
> +
> + /*
> + * lock is acquired in pfc unlock call back and then released in
> + * pfc lock callback
> + */
> + raw_spin_lock(&pctrl->pwpr_lock);
Except for rzg2l_pinctrl_pm_setup_pfc(), this function is always
called while holding rzg2l_pinctrl.lock. So I think you can just
take rzg2l_pinctrl.lock in rzg2l_pinctrl_pm_setup_pfc(), and get rid
of pwpr_lock?
> + /* Set the PWPR register to allow PFC and PMC register to write */
> + pwpr = readb(pctrl->base + regs->pwpr);
> + writeb(PWPR_PFCWE | pwpr, pctrl->base + regs->pwpr);
PWPR_REGWE_A
> +}
> +
> +static void rzv2h_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl)
> +{
> + const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> + u8 pwpr;
> +
> + /* Set the PWPR register to be write-protected */
> + pwpr = readb(pctrl->base + regs->pwpr);
> + writeb(pwpr & ~PWPR_PFCWE, pctrl->base + regs->pwpr);
PWPR_REGWE_A
> + raw_spin_unlock(&pctrl->pwpr_lock);
> +}
> +
> static const struct rzg2l_hwcfg rzg2l_hwcfg = {
> .regs = {
> .pwpr = 0x3014,
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists