Re: [PATCH 2/7] pinctrl: bcm: Add STB family pin controller driver

[Stefan Wahren <wahrenst@gmx.net>]

Hi,

Am 31.07.24 um 08:28 schrieb Ivan T. Ivanov:
> This driver provide pin muxing and configuration functionality
> for BCM2712 SoC used by RPi5. According to [1] this chips is
> incarnation of chip used in Broadcom STB  product line.
>
> [1] https://lore.kernel.org/lkml/f6601f73-cb22-4ba3-88c5-241be8421fc3@broadcom.com/
>
> Cc: Jonathan Bell <jonathan@raspberrypi.com>
> Cc: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Ivan T. Ivanov <iivanov@suse.de>
> ---
>   drivers/pinctrl/bcm/Kconfig           |   13 +
>   drivers/pinctrl/bcm/Makefile          |    1 +
>   drivers/pinctrl/bcm/pinctrl-brcmstb.c | 1217 +++++++++++++++++++++++++
>   3 files changed, 1231 insertions(+)
>   create mode 100644 drivers/pinctrl/bcm/pinctrl-brcmstb.c
>
> diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig
> index 35b51ce4298e..2e1b8d444f58 100644
> --- a/drivers/pinctrl/bcm/Kconfig
> +++ b/drivers/pinctrl/bcm/Kconfig
> @@ -3,6 +3,19 @@
>   # Broadcom pinctrl drivers
>   #
>
> +config PINCTRL_BRCMSTB
> +	tristate "Broadcom STB product line pin controller driver"
> +	depends on OF && (ARCH_BRCMSTB || COMPILE_TEST)
> +	select PINMUX
> +	select PINCONF
> +	select GENERIC_PINCONF
> +	help
> +	  This driver provide pin muxing and configuration functionality
> +	  for Broadcom STB product line chipsets. BCM2712 SoC is one of these
> +	  chipsets.
> +
> +	  If M is selected the module will be called pinctrl-brcmstb.
> +
>   config PINCTRL_BCM281XX
>   	bool "Broadcom BCM281xx pinctrl driver"
>   	depends on OF && (ARCH_BCM_MOBILE || COMPILE_TEST)
> diff --git a/drivers/pinctrl/bcm/Makefile b/drivers/pinctrl/bcm/Makefile
> index 82b868ec1471..5f790c14cc4c 100644
> --- a/drivers/pinctrl/bcm/Makefile
> +++ b/drivers/pinctrl/bcm/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_PINCTRL_BCM6358)		+= pinctrl-bcm6358.o
>   obj-$(CONFIG_PINCTRL_BCM6362)		+= pinctrl-bcm6362.o
>   obj-$(CONFIG_PINCTRL_BCM6368)		+= pinctrl-bcm6368.o
>   obj-$(CONFIG_PINCTRL_BCM63268)		+= pinctrl-bcm63268.o
> +obj-$(CONFIG_PINCTRL_BRCMSTB)		+= pinctrl-brcmstb.o
>   obj-$(CONFIG_PINCTRL_IPROC_GPIO)	+= pinctrl-iproc-gpio.o
>   obj-$(CONFIG_PINCTRL_CYGNUS_MUX)	+= pinctrl-cygnus-mux.o
>   obj-$(CONFIG_PINCTRL_NS)		+= pinctrl-ns.o
> diff --git a/drivers/pinctrl/bcm/pinctrl-brcmstb.c b/drivers/pinctrl/bcm/pinctrl-brcmstb.c
> new file mode 100644
> index 000000000000..143c8c2e1d6d
> --- /dev/null
> +++ b/drivers/pinctrl/bcm/pinctrl-brcmstb.c
> @@ -0,0 +1,1217 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Broadcom brcmstb GPIO units (pinctrl only)
> + *
Maybe you want to add your copyright here
> + * Copyright (C) 2021-3 Raspberry Pi Ltd.
> + * Copyright (C) 2012 Chris Boot, Simon Arlott, Stephen Warren
> + *
> + * Based heavily on the BCM2835 GPIO & pinctrl driver, which was inspired by:
> + * pinctrl-nomadik.c, please see original file for copyright information
> + * pinctrl-tegra.c, please see original file for copyright information
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#define BRCMSTB_PULL_NONE	0
> +#define BRCMSTB_PULL_DOWN	1
> +#define BRCMSTB_PULL_UP		2
> +#define BRCMSTB_PULL_MASK	0x3
> +
> +#define BRCMSTB_FSEL_COUNT	9
> +#define BRCMSTB_FSEL_MASK	0xf
> +
> +#define FUNC(f) \
> +	[func_##f] = #f
> +
> +#define PIN(i, f1, f2, f3, f4, f5, f6, f7, f8) \
> +	[i] = { \
> +		.funcs = { \
> +			func_##f1, \
> +			func_##f2, \
> +			func_##f3, \
> +			func_##f4, \
> +			func_##f5, \
> +			func_##f6, \
> +			func_##f7, \
> +			func_##f8, \
> +		}, \
> +	}
> +
> +#define MUX_BIT_VALID		0x8000
> +#define REG_BIT_INVALID		0xffff
> +
> +#define BIT_TO_REG(b)		(((b) >> 5) << 2)
> +#define BIT_TO_SHIFT(b)	((b) & 0x1f)
> +
> +#define MUX_BIT(mr, mb)		(MUX_BIT_VALID + ((mr) * 4) * 8 + (mb) * 4)
> +#define GPIO_REGS(n, mr, mb, pr, pb) \
> +	[n] = { MUX_BIT(mr, mb), ((pr) * 4) * 8 + (pb) * 2 }
Please avoid these cryptic names: mr, mb, pr, pb
> +
> +#define EMMC_REGS(n, pr, pb) \
> +	[n] = { 0, ((pr) * 4) * 8 + (pb) * 2 }
> +
> +#define AGPIO_REGS(n, mr, mb, pr, pb) \
> +	[n] = { MUX_BIT(mr, mb), ((pr) * 4) * 8 + (pb) * 2 }
> +
> +#define SGPIO_REGS(n, mr, mb) \
> +	[(n) + 32] = { MUX_BIT(mr, mb), REG_BIT_INVALID }
> +
> +#define GPIO_PIN(a)		PINCTRL_PIN(a, "gpio" #a)
> +#define AGPIO_PIN(a)		PINCTRL_PIN(a, "aon_gpio" #a)
> +#define SGPIO_PIN(a)		PINCTRL_PIN((a) + 32, "aon_sgpio" #a)
> +
> +struct pin_regs {
> +	u16 mux_bit;
> +	u16 pad_bit;
> +};
> +
> +struct brcmstb_pin_funcs {
> +	u8 funcs[BRCMSTB_FSEL_COUNT - 1];
> +};
> +
> +struct brcmstb_pinctrl {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct pinctrl_dev *pctl_dev;
> +	struct pinctrl_desc pctl_desc;
> +	const struct pin_regs *pin_regs;
> +	const struct brcmstb_pin_funcs *pin_funcs;
> +	const char *const *gpio_groups;
> +	struct pinctrl_gpio_range gpio_range;
> +	/* Protect FSEL registers */
> +	spinlock_t lock;
fsel_lock ?
> +};
> +
> ...
> +
> +static const struct brcmstb_pin_funcs bcm2712_d0_aon_gpio_pin_funcs[] = {
> +	PIN(0, ir_in, vc_spi0, vc_uart0, vc_i2c3, uart0, vc_i2c0, _, _),
> +	PIN(1, vc_pwm0, vc_spi0, vc_uart0, vc_i2c3, uart0, aon_pwm, vc_i2c0, vc_pwm1),
> +	PIN(2, vc_pwm0, vc_spi0, vc_uart0, ctl_hdmi_5v, uart0, aon_pwm, ir_in, vc_pwm1),
> +	PIN(3, ir_in, vc_spi0, vc_uart0, uart0, sd_card_g, aon_gpclk, _, _),
> +	PIN(4, gpclk0, vc_spi0, pm_led_out, aon_pwm, sd_card_g, vc_pwm0, _, _),
> +	PIN(5, gpclk1, ir_in, aon_pwm, sd_card_g, vc_pwm0, _, _, _),
> +	PIN(6, uart1, vc_uart2, ctl_hdmi_5v, gpclk2, vc_spi3, _, _, _),
> +	PIN(7, _, _, _, _, _, _, _, _),
In case this pin is reserved or something a comment would be nice
> +	PIN(8, uart1, vc_uart2, ctl_hdmi_5v, vc_spi0, vc_spi3, _, _, _),
> +	PIN(9, uart1, vc_uart2, vc_uart0, aon_pwm, vc_spi0, vc_uart2, vc_spi3, _),
> +	PIN(10, _, _, _, _, _, _, _, _),
> +	PIN(11, _, _, _, _, _, _, _, _),
same as above
> +	PIN(12, uart1, vc_uart2, vc_uart0, vc_spi0, usb_pwr, vc_uart2, vc_spi3, _),
> +	PIN(13, bsc_m1, vc_uart0, uui, vc_spi0, arm_jtag, vc_uart2, vc_i2c3, _),
> +	PIN(14, bsc_m1, aon_gpclk, vc_uart0, uui, vc_spi0, arm_jtag, vc_uart2, vc_i2c3),
> +};
> +
> ...
> +
> +static inline u32 brcmstb_reg_rd(struct brcmstb_pinctrl *pc, unsigned int reg)
> +{
> +	return readl(pc->base + reg);
> +}
> +
> +static inline void brcmstb_reg_wr(struct brcmstb_pinctrl *pc, unsigned int reg,
> +				  u32 val)
> +{
> +	writel(val, pc->base + reg);
> +}
> +
> +static enum brcmstb_funcs brcmstb_pinctrl_fsel_get(struct brcmstb_pinctrl *pc,
> +						   unsigned int pin)
> +{
> +	u32 bit = pc->pin_regs[pin].mux_bit;
> +	enum brcmstb_funcs func;
> +	int fsel;
> +	u32 val;
> +
> +	if (!bit)
> +		return func_gpio;
> +
> +	bit &= ~MUX_BIT_VALID;
> +
> +	val = brcmstb_reg_rd(pc, BIT_TO_REG(bit));
> +	fsel = (val >> BIT_TO_SHIFT(bit)) & BRCMSTB_FSEL_MASK;
> +	func = pc->pin_funcs[pin].funcs[fsel];
> +
> +	if (func >= func_count)
> +		func = (enum brcmstb_funcs)fsel;
> +
> +	dev_dbg(pc->dev, "get %04x: %08x (%u => %s)\n",
> +		BIT_TO_REG(bit), val, pin,
> +		brcmstb_func_names[func]);
> +
> +	return func;
> +}
> +
> +static void brcmstb_pinctrl_fsel_set(struct brcmstb_pinctrl *pc,
> +				     unsigned int pin, enum brcmstb_funcs func)
I would prefer this function returns errors as int instead of silently
ignore them
> +{
> +	u32 bit = pc->pin_regs[pin].mux_bit, val;
> +	const u8 *pin_funcs;
> +	unsigned long flags;
> +	int fsel;
> +	int cur;
> +	int i;
> +
> +	if (!bit || func >= func_count)
> +		return;
> +
> +	bit &= ~MUX_BIT_VALID;
> +
> +	fsel = BRCMSTB_FSEL_COUNT;
> +
> +	if (func >= BRCMSTB_FSEL_COUNT) {
> +		/* Convert to an fsel number */
> +		pin_funcs = pc->pin_funcs[pin].funcs;
> +		for (i = 1; i < BRCMSTB_FSEL_COUNT; i++) {
> +			if (pin_funcs[i - 1] == func) {
> +				fsel = i;
> +				break;
> +			}
> +		}
> +	} else {
> +		fsel = (enum brcmstb_funcs)func;
> +	}
> +
> +	if (fsel >= BRCMSTB_FSEL_COUNT)
> +		return;
> +
> +	spin_lock_irqsave(&pc->lock, flags);
> +
> +	val = brcmstb_reg_rd(pc, BIT_TO_REG(bit));
> +	cur = (val >> BIT_TO_SHIFT(bit)) & BRCMSTB_FSEL_MASK;
> +
> +	dev_dbg(pc->dev, "read %04x: %08x (%u => %s)\n",
> +		BIT_TO_REG(bit), val, pin,
> +		brcmstb_func_names[cur]);
> +
> +	if (cur != fsel) {
> +		val &= ~(BRCMSTB_FSEL_MASK << BIT_TO_SHIFT(bit));
> +		val |= fsel << BIT_TO_SHIFT(bit);
> +
> +		dev_dbg(pc->dev, "write %04x: %08x (%u <= %s)\n",
> +			BIT_TO_REG(bit), val, pin,
> +			brcmstb_func_names[fsel]);
> +		brcmstb_reg_wr(pc, BIT_TO_REG(bit), val);
> +	}
> +
> +	spin_unlock_irqrestore(&pc->lock, flags);
> +}
> +
> +static int brcmstb_pctl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pc->pctl_desc.npins;
> +}
> +
> +static const char *brcmstb_pctl_get_group_name(struct pinctrl_dev *pctldev,
> +					       unsigned int selector)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pc->gpio_groups[selector];
> +}
> +
> +static int brcmstb_pctl_get_group_pins(struct pinctrl_dev *pctldev,
> +				       unsigned int selector,
> +				       const unsigned int **pins,
> +				       unsigned int *num_pins)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*pins = &pc->pctl_desc.pins[selector].number;
> +	*num_pins = 1;
> +
> +	return 0;
> +}
> +
> +static void brcmstb_pctl_pin_dbg_show(struct pinctrl_dev *pctldev,
> +				      struct seq_file *s, unsigned int offset)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +	enum brcmstb_funcs fsel = brcmstb_pinctrl_fsel_get(pc, offset);
> +	const char *fname = brcmstb_func_names[fsel];
> +
> +	seq_printf(s, "function %s", fname);
> +}
> +
> +static void brcmstb_pctl_dt_free_map(struct pinctrl_dev *pctldev,
> +				     struct pinctrl_map *maps,
> +				     unsigned int num_maps)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_maps; i++)
> +		if (maps[i].type == PIN_MAP_TYPE_CONFIGS_PIN)
> +			kfree(maps[i].data.configs.configs);
> +
> +	kfree(maps);
> +}
> +
> +static const struct pinctrl_ops brcmstb_pctl_ops = {
> +	.get_groups_count = brcmstb_pctl_get_groups_count,
> +	.get_group_name = brcmstb_pctl_get_group_name,
> +	.get_group_pins = brcmstb_pctl_get_group_pins,
> +	.pin_dbg_show = brcmstb_pctl_pin_dbg_show,
> +	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> +	.dt_free_map = brcmstb_pctl_dt_free_map,
> +};
> +
> +static int brcmstb_pmx_free(struct pinctrl_dev *pctldev, unsigned int offset)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	/* disable by setting to GPIO */
> +	brcmstb_pinctrl_fsel_set(pc, offset, func_gpio);
> +	return 0;
> +}
> +
> +static int brcmstb_pmx_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	return func_count;
> +}
> +
> +static const char *brcmstb_pmx_get_function_name(struct pinctrl_dev *pctldev,
> +						 unsigned int selector)
> +{
> +	return (selector < func_count) ? brcmstb_func_names[selector] : NULL;
> +}
> +
> +static int brcmstb_pmx_get_function_groups(struct pinctrl_dev *pctldev,
> +					   unsigned int selector,
> +					   const char *const **groups,
> +					   unsigned *const num_groups)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = pc->gpio_groups;
> +	*num_groups = pc->pctl_desc.npins;
> +
> +	return 0;
> +}
> +
> +static int brcmstb_pmx_set(struct pinctrl_dev *pctldev,
> +			   unsigned int func_selector,
> +			   unsigned int group_selector)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +	const struct pinctrl_desc *pctldesc = &pc->pctl_desc;
> +	const struct pinctrl_pin_desc *pindesc;
> +
> +	if (group_selector >= pctldesc->npins)
> +		return -EINVAL;
> +
> +	pindesc = &pctldesc->pins[group_selector];
> +	brcmstb_pinctrl_fsel_set(pc, pindesc->number, func_selector);
return brcmstb_pinctrl_fsel_set
> +
> +	return 0;
> +}
> +
> +static int brcmstb_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> +					   struct pinctrl_gpio_range *range,
> +					   unsigned int pin)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	brcmstb_pinctrl_fsel_set(pc, pin, func_gpio);
> +
> +	return 0;
return brcmstb_pinctrl_fsel_set
> +}
> +
> +static void brcmstb_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
> +					  struct pinctrl_gpio_range *range,
> +					  unsigned int offset)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +	/* disable by setting to GPIO */
> +	brcmstb_pinctrl_fsel_set(pc, offset, func_gpio);
> +}
> +
> +static const struct pinmux_ops brcmstb_pmx_ops = {
> +	.free = brcmstb_pmx_free,
> +	.get_functions_count = brcmstb_pmx_get_functions_count,
> +	.get_function_name = brcmstb_pmx_get_function_name,
> +	.get_function_groups = brcmstb_pmx_get_function_groups,
> +	.set_mux = brcmstb_pmx_set,
> +	.gpio_request_enable = brcmstb_pmx_gpio_request_enable,
> +	.gpio_disable_free = brcmstb_pmx_gpio_disable_free,
> +};
> +
> +static unsigned int brcmstb_pull_config_get(struct brcmstb_pinctrl *pc,
> +					    unsigned int pin)
> +{
> +	u32 bit = pc->pin_regs[pin].pad_bit, val;
> +
> +	if (bit == REG_BIT_INVALID)
> +		return BRCMSTB_PULL_NONE;
> +
> +	val = brcmstb_reg_rd(pc, BIT_TO_REG(bit));
> +	return (val >> BIT_TO_SHIFT(bit)) & BRCMSTB_PULL_MASK;
> +}
> +
> +static void brcmstb_pull_config_set(struct brcmstb_pinctrl *pc,
> +				    unsigned int pin, unsigned int arg)
The same here, returning an error would be better
> +{
> +	u32 bit = pc->pin_regs[pin].pad_bit, val;
> +	unsigned long flags;
> +
> +	if (bit == REG_BIT_INVALID) {
> +		dev_warn(pc->dev, "Can't set pulls for %s\n",
> +			 pc->gpio_groups[pin]);
> +		return;
> +	}
> +
> +	spin_lock_irqsave(&pc->lock, flags);
> +
> +	val = brcmstb_reg_rd(pc, BIT_TO_REG(bit));
> +	val &= ~(BRCMSTB_PULL_MASK << BIT_TO_SHIFT(bit));
> +	val |= (arg << BIT_TO_SHIFT(bit));
> +	brcmstb_reg_wr(pc, BIT_TO_REG(bit), val);
> +
> +	spin_unlock_irqrestore(&pc->lock, flags);
> +}
> +
> +static int brcmstb_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			       unsigned long *config)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	u32 arg;
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		arg = (brcmstb_pull_config_get(pc, pin) == BRCMSTB_PULL_NONE);
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		arg = (brcmstb_pull_config_get(pc, pin) == BRCMSTB_PULL_DOWN);
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		arg = (brcmstb_pull_config_get(pc, pin) == BRCMSTB_PULL_UP);
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +
> +	return 0;
> +}
> +
> +static int brcmstb_pinconf_set(struct pinctrl_dev *pctldev,
> +			       unsigned int pin, unsigned long *configs,
> +			       unsigned int num_configs)
> +{
> +	struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +	u32 param, arg;
> +	int i;
> +
> +	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:
> +			brcmstb_pull_config_set(pc, pin, BRCMSTB_PULL_NONE);
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			brcmstb_pull_config_set(pc, pin, BRCMSTB_PULL_DOWN);
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			brcmstb_pull_config_set(pc, pin, BRCMSTB_PULL_UP);
In case brcmstb_pull_config_set would return an error, we shouldn't
ignore it here

Thanks
> +			break;
> +		default:
> +			return -ENOTSUPP;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +