[PATCH v2 3/4] pinctrl: aspeed: Add AST2700 pinmux support

Tue Nov 11 13:24:02 AEDT 2025

> > 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.

Okay, I will split the support for SoC0 and SoC1 into separate patches.
SoC0 will follow the existing design closely, while SoC1 will introduce
a new logic for configuring pin functions.

> >
> > 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 at aspeedtech.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.

Understood. I will modify the patch of the SoC0 to meet the concept with previous
generations. If the differences are significant, I will consider creating a
separate driver for it. But, at this moment, I believe the existing driver and macro can
be adapted to support SoC0 with minimal changes.

> > 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.

The enums are used to replace the #define M24 0, #define M25 1, … definitions in
the G6 pinctrl driver. Additionally, the number assigned to each ball name must
follow the sequence of GPIO numbering. In other words, the number assigned to each
ball name is meaningful, and the order should not be changed. Therefore, I believe
that using enums is a better approach than using #define macros.

> > + 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.

The groups and the functions can be put together, but the ball define
as the above reasons. So, I will keep the ball defines at the top with the enums.

> > 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.

Got it. I will improve the macros to avoid open-coding shifts.

> > +
> > +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.

Okay, I will format the array definition to have one entry per line and sorted.

Thanks

Best regards,
Billy Tsai