Message-ID: <20251119-bacterium-banana-abcdf5c9fbc5@spud>
Date: Wed, 19 Nov 2025 18:23:55 +0000
From: Conor Dooley <conor@...nel.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Conor Dooley <conor.dooley@...rochip.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	devicetree@...r.kernel.org, Valentina.FernandezAlanis@...rochip.com,
	Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> Hi Conor,
> 
> took a quick look at this!
> 
> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@...nel.org> wrote:
> 
> > +#include "linux/dev_printk.h"
> 
> Weird but it's RFC so OK :)
> 
> > +#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
> > +#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
> > +#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)
> 
> Yeah this should work for custom props.
> 
> > +struct mpfs_pinctrl_drive_strength {
> > +       u8 ma;
> > +       u8 val;
> > +};
> > +
> > +static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
> > +       { 2,   2 },
> > +       { 4,   3 },
> > +       { 6,   4 },
> > +       { 8,   5 },
> > +       { 10,  6 },
> > +       { 12,  7 },
> > +       { 16, 10 },
> > +       { 20, 12 },
> > +};
> 
> I would probably assign field explicitly with C99 syntax, but no
> hard requirement.
> 
> { .ma = 2, .val = 2 }
> 
> BTW you can see clearly that each setting activates
> another driver stage in the silicon, each totempole giving
> 2 mA.
> 
> > +static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
> > +       { "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
> > +       { "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
> > +       { "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
> > +};
> 
> I take it these have proper documentation in the DT bindings, so users know
> exactly what they do.

Honestly, even if users don't know what they do, they should just be
making this stuff 1:1 match some MSS configurator output, where it's
very clear if you select lockdown or turn on the clamp diode. The clamp
diode is almost certainly input voltage clamping (although the
documentation on the mssio stuff doesn't say anything about it) and the
lockdwn thing is some security feature that doesn't actually do anything
on its own, but will disable the bank in question if the tamper
detection hardware gets upset.
I'm not really all that sure why this is a per-pin setting, because the
documentation about this (and the MSS configurator) treat it as being
bank wide. There's some other part of this in another register that is
actually bank wide that I only discovered yesterday, and I dunno how
they interact.
The binding doesn't really explain this stuff, other than saying what
field in the configurator to get it from. I'll add something for !RFC.

ibufmd is a different story, the only source of it I could find was
in the configurator output files. I suspect that it can be just computed
from the other dt properties and the bank voltage, but I need to find
the answer in the configurator source code I think.

> > +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> > +{
> > +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> > +
> > +       if (pin >= MPFS_PINCTRL_BANK2_START)
> > +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> > +
> > +       // 2 pins per 32-bit register
> > +       reg += (pin / 2) * 0x4;
> 
> This is a nice comment, easy to follow the code with small helpful
> things like this.
> 
> > +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> > +                                      struct pinctrl_map **maps, unsigned int *num_maps)
> 
> I saw in the cover letter that you wanted this to use more generic helpers.
> 
> If you see room for improvement of the generic code, do not hesitate.
> Doing a new driver is the only time when you actually have all these
> details in your head and can create good helpers.
> 
> > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > +       case PIN_CONFIG_INPUT_SCHMITT:
> > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> 
> Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> in the bindings if they don't make any sense, and let it just return error
> if someone tries to do that.
> 
> Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> makes sense for this hardware?

Yeah, I just didn't know if having the others was helpful, since they
say things like:
 * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
 *	schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
 *	the threshold value is given on a custom format as argument when
 *	setting pins to this mode.
which implies they should be implemented for systen not permitting
hysteresis adjustment.

> > +static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
> > +                                               unsigned long *configs, unsigned int num_configs,
> > +                                               u32 *value)
> (...)
> > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > +                       //TODO always start from val == 0, there's no reason to ever actually
> > +                       // clear anything AFAICT. @Linus, does the driver need to check mutual
> > +                       // exclusion on these, or can I drop the clearing?
> > +                       val &= ~MPFS_PINCTRL_PULL_MASK;
> > +                       val |= MPFS_PINCTRL_WPD;
> > +                       break;
> 
> I was about to say that the core checks that you don't enable pull up
> and pull down
> at the same time, but apparently that was just a dream I had.
> 
> The gpiolib however contains this in gpiod_configure_flags():
> 
>         if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
>             ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
>             ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
>                 gpiod_err(desc,
>                           "multiple pull-up, pull-down or pull-disable
> enabled, invalid configuration\n");
>                 return -EINVAL;
>         }
> 
> So there is a precedent for checking this.
> 
> So if you patch pinconf-generic.c to disallow this that'd be great, I think
> it makes most sense to do this in the core.

Yeah, seems better than doing it in my driver..

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists