Message-ID: <005c9928cde01c8a3bf4840692eddf13f2c08282.camel@codeconstruct.com.au>
Date: Thu, 25 Sep 2025 14:18:31 +0930
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Billy Tsai <billy_tsai@...eedtech.com>, lee@...nel.org, robh@...nel.org,
 	krzk+dt@...nel.org, conor+dt@...nel.org, joel@....id.au,
 linus.walleij@...aro.org, 	brgl@...ev.pl, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, 	linux-aspeed@...ts.ozlabs.org,
 linux-kernel@...r.kernel.org, 	openbmc@...ts.ozlabs.org,
 linux-gpio@...r.kernel.org, BMC-SW@...eedtech.com
Subject: Re: [PATCH v2 3/4] pinctrl: aspeed: Add AST2700 pinmux support

Hi Billy,

On Thu, 2025-09-04 at 18:34 +0800, Billy Tsai wrote:
> This patch adds pinmux support for the AST2700, which includes two SoC
> configurations:
> - SoC0 closely resembles previous generations of ASPEED BMC SoCs, allowing
>   the reuse of existing macros and callback functions.
> - SoC1, however, introduces a new logic for configuring pin functions.
>   Therefore, new g7_set_mux and gpio_request_enable functions are
>   implemented to properly configure the pinctrl registers using the
>   pin_cfg table and to resolve GPIO request errors.

Do you mind splitting support for soc0 and soc1 into separate patches?
Having taken a brief look I think we're also due for some further
separation of the code. Particularly, isolating the patch for soc0
would be nice, as the register design for soc1 is just _so_ much nicer,
and I'd like to avoid dragging the baggage of previous generations
along with it.

> 
> The driver supports:
> - All 12 GPIO-capable pins in SoC0
> - All 212 GPIO-capable pins in SoC1
> 
> Additionally, this patch introduces several pseudo-ball definitions for
> specific configuration purposes:
> - USB function selection
> - JTAG target selection
> - PCIe RC PERST configuration
> - SGMII PHY selection
> 
> Signed-off-by: Billy Tsai <billy_tsai@...eedtech.com>
> ---
>  drivers/pinctrl/aspeed/Kconfig                |    8 +
>  drivers/pinctrl/aspeed/Makefile               |    1 +
>  .../pinctrl/aspeed/pinctrl-aspeed-g7-soc0.c   |  503 ++++
>  .../pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c   | 2523 +++++++++++++++++
>  drivers/pinctrl/aspeed/pinctrl-aspeed.c       |   47 +
>  drivers/pinctrl/aspeed/pinctrl-aspeed.h       |   11 +-
>  drivers/pinctrl/aspeed/pinmux-aspeed.h        |   35 +-

The impression I get from the changes to the latter three files here is
that the soc0 support might even be different enough to warrant
separation from the previous generations as well. You're defining
similar-but-different structs and macros to what we already have. If
the those are genuinely necessary (not yet resolved), I'd rather they
be their own driver.

>  7 files changed, 3123 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc0.c
>  create mode 100644 drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c
> 
> diff --git a/drivers/pinctrl/aspeed/Kconfig b/drivers/pinctrl/aspeed/Kconfig
> index 1a4e5b9ed471..16743091a139 100644
> --- a/drivers/pinctrl/aspeed/Kconfig
> +++ b/drivers/pinctrl/aspeed/Kconfig
> @@ -31,3 +31,11 @@ config PINCTRL_ASPEED_G6
>  	help
>  	  Say Y here to enable pin controller support for Aspeed's 6th
>  	  generation SoCs. GPIO is provided by a separate GPIO driver.
> +
> +config PINCTRL_ASPEED_G7
> +	bool "Aspeed G7 SoC pin control"
> +	depends on (ARCH_ASPEED || COMPILE_TEST) && OF
> +	select PINCTRL_ASPEED
> +	help
> +	  Say Y here to enable pin controller support for Aspeed's 7th
> +	  generation SoCs. GPIO is provided by a separate GPIO driver.
> diff --git a/drivers/pinctrl/aspeed/Makefile b/drivers/pinctrl/aspeed/Makefile
> index db2a7600ae2b..1713f678a984 100644
> --- a/drivers/pinctrl/aspeed/Makefile
> +++ b/drivers/pinctrl/aspeed/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_PINCTRL_ASPEED)	+= pinctrl-aspeed.o pinmux-aspeed.o
>  obj-$(CONFIG_PINCTRL_ASPEED_G4)	+= pinctrl-aspeed-g4.o
>  obj-$(CONFIG_PINCTRL_ASPEED_G5)	+= pinctrl-aspeed-g5.o
>  obj-$(CONFIG_PINCTRL_ASPEED_G6)	+= pinctrl-aspeed-g6.o
> +obj-$(CONFIG_PINCTRL_ASPEED_G7) += pinctrl-aspeed-g7-soc0.o pinctrl-aspeed-g7-soc1.o
> \ No newline at end of file
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc0.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc0.c
> new file mode 100644
> index 000000000000..86da889cc010
> --- /dev/null
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc0.c
> @@ -0,0 +1,503 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include "pinctrl-aspeed.h"
> +#include "../pinctrl-utils.h"
> +
> +#define SCU200 0x200 /* System Reset Control #1  */
> +
> +#define SCU400 0x400 /* Multi-function Pin Control #1  */
> +#define SCU404 0x404 /* Multi-function Pin Control #2  */
> +#define SCU408 0x408 /* Multi-function Pin Control #3  */
> +#define SCU40C 0x40C /* Multi-function Pin Control #3  */
> +#define SCU410 0x410 /* USB Multi-function Control Register  */
> +#define SCU414 0x414 /* VGA Function Control Register  */
> +
> +#define SCU480 0x480 /* GPIO18A0 IO Control Register */
> +#define SCU484 0x484 /* GPIO18A1 IO Control Register */
> +#define SCU488 0x488 /* GPIO18A2 IO Control Register */
> +#define SCU48C 0x48c /* GPIO18A3 IO Control Register */
> +#define SCU490 0x490 /* GPIO18A4 IO Control Register */
> +#define SCU494 0x494 /* GPIO18A5 IO Control Register */
> +#define SCU498 0x498 /* GPIO18A6 IO Control Register */
> +#define SCU49C 0x49c /* GPIO18A7 IO Control Register */
> +#define SCU4A0 0x4A0 /* GPIO18B0 IO Control Register */
> +#define SCU4A4 0x4A4 /* GPIO18B1 IO Control Register */
> +#define SCU4A8 0x4A8 /* GPIO18B2 IO Control Register */
> +#define SCU4AC 0x4AC /* GPIO18B3 IO Control Register */
> +
> +enum {
> +	AC14,
> +	AE15,

Are the ball enums necessary? Historically we haven't needed it as the
undefined macro symbols were just used for token-pasting or
stringification purposes.

> +	AD14,
> +	AE14,
> +	AF14,
> +	AB13,
> +	AB14,
> +	AF15,
> +	AF13,
> +	AC13,
> +	AD13,
> +	AE13,
> +	PORTA_U3, // SCU410[1:0]
> +	PORTA_U2, // SCU410[3:2]
> +	PORTB_U3, // SCU410[5:4]
> +	PORTB_U2, // SCU410[7:6]
> +	PORTA_U3_XHCI, // SCU410[9]
> +	PORTA_U2_XHCI, // SCU410[9]
> +	PORTB_U3_XHCI, // SCU410[10]
> +	PORTB_U2_XHCI, // SCU410[10]
> +	PORTA_MODE, // SCU410[25:24]
> +	PORTB_MODE, // SCU410[29:28]
> +	PORTA_U2_PHY,
> +	PORTA_U3_PHY,
> +	PORTB_U2_PHY,
> +	PORTB_U3_PHY,
> +	JTAG_PORT,
> +	PCIERC0_PERST,
> +	PCIERC1_PERST,
> +};
> +
> +GROUP_DECL(EMMCG1, AC14, AE15, AD14);
> +GROUP_DECL(EMMCG4, AC14, AE15, AD14, AE14, AF14, AB13);
> +GROUP_DECL(EMMCG8, AC14, AE15, AD14, AE14, AF14, AB13, AF13, AC13, AD13, AE13);
> +GROUP_DECL(EMMCWPN, AF15);
> +GROUP_DECL(EMMCCDN, AB14);
> +GROUP_DECL(VGADDC, AD13, AE13);
> +GROUP_DECL(VB1, AC14, AE15, AD14, AE14);
> +GROUP_DECL(VB0, AF15, AB14, AF13, AC13);

For the previous generation drivers my philosophy was "keep things that
go together together," so signal descriptors, groups and functions were
all located around the definition for one or a set of balls.

Given I'm potentially escaping maintenance of ASPEED pinctrl drivers
for the 2700 onwards I won't complain too much, but was this a specific
choice to break from that approach? You're defining all the groups,
then all the functions, then all the configurations. 

*snip*


> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c
> new file mode 100644
> index 000000000000..7c5a5e208f63
> --- /dev/null
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g7-soc1.c
> 

*snip*

> +
> +FUNCFG_DESCL(C16, PIN_CFG(ESPI1, SCU400, GENMASK(2, 0), 1),
> +	     PIN_CFG(LPC1, SCU400, GENMASK(2, 0), 2),
> +	     PIN_CFG(SD, SCU400, GENMASK(2, 0), 3),
> +	     PIN_CFG(DI2C0, SCU400, GENMASK(2, 0), 4),
> +	     PIN_CFG(VPI, SCU400, GENMASK(2, 0), 5));
> +FUNCFG_DESCL(C14, PIN_CFG(ESPI1, SCU400, GENMASK(6, 4), (1 << 4)),
> +	     PIN_CFG(LPC1, SCU400, GENMASK(6, 4), (2 << 4)),
> +	     PIN_CFG(SD, SCU400, GENMASK(6, 4), (3 << 4)),
> +	     PIN_CFG(DI2C1, SCU400, GENMASK(6, 4), (4 << 4)),
> +	     PIN_CFG(VPI, SCU400, GENMASK(6, 4), (5 << 4)));
> +FUNCFG_DESCL(C11, PIN_CFG(ESPI1, SCU400, GENMASK(10, 8), (1 << 8)),
> +	     PIN_CFG(LPC1, SCU400, GENMASK(10, 8), (2 << 8)),
> +	     PIN_CFG(SD, SCU400, GENMASK(10, 8), (3 << 8)),
> +	     PIN_CFG(DI2C3, SCU400, GENMASK(10, 8), (4 << 8)),
> +	     PIN_CFG(VPI, SCU400, GENMASK(10, 8), (5 << 8)));

If we're going to continue the macro soup we need to take the
opportunity to clean this up. You shouldn't need to open-code the
shifts like this, the data model needs more thought.

*snip*

> +
> +static const struct aspeed_g7_pincfg pin_cfg[] = {
> +	PINCFG_PIN(C16),    PINCFG_PIN(C14),   PINCFG_PIN(C11),
> +	PINCFG_PIN(D9),	    PINCFG_PIN(F14),   PINCFG_PIN(D10),

My preference is that this array definition be one entry per line,
sorted, that way we don't get weird alignment or reflow affecting the
remainder of the struct if we change things in the middle.

Cheers,

Andrew

Powered by blists - more mailing lists