diff mbox series

[12/18] pinctrl: bcm2835: Add support for BCM2711 pull-up functionality

Message ID 1563776607-8368-3-git-send-email-wahrenst@gmx.net (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Stefan Wahren July 22, 2019, 6:23 a.m. UTC 
The BCM2711 has a new way of selecting the pull-up/pull-down setting
for a GPIO pin. The registers used for the BCM2835, GP_PUD and
GP_PUDCLKn0, are no longer connected. A new set of registers,
GP_GPIO_PUP_PDN_CNTRL_REGx must be used. This commit will add
a new compatible string "brcm,bcm2711-gpio" and the kernel
driver will use it to select which method is used to select
pull-up/pull-down.

This patch based on a patch by Al Cooper which was intended for the
BCM7211. This is a bugfixed and improved version.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 105 ++++++++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 5 deletions(-)

--
2.7.4

Comments

Linus Walleij Aug. 5, 2019, 9:38 a.m. UTC | #1 
On Mon, Jul 22, 2019 at 8:24 AM Stefan Wahren <wahrenst@gmx.net> wrote:

> The BCM2711 has a new way of selecting the pull-up/pull-down setting
> for a GPIO pin. The registers used for the BCM2835, GP_PUD and
> GP_PUDCLKn0, are no longer connected. A new set of registers,
> GP_GPIO_PUP_PDN_CNTRL_REGx must be used. This commit will add
> a new compatible string "brcm,bcm2711-gpio" and the kernel
> driver will use it to select which method is used to select
> pull-up/pull-down.
>
> This patch based on a patch by Al Cooper which was intended for the
> BCM7211. This is a bugfixed and improved version.
>
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>

Patch applied.

I think I complained about some other version of this patch, this one
looks entirely acceptable.

Can we get rid of custom pull settings etc from the upstream device
trees so we don't set bad examples? I have a strong urge to
throw in a pr_warn() about any use of it.

Yours,
Linus Walleij
Stefan Wahren Aug. 11, 2019, 8:15 p.m. UTC | #2 
Am 05.08.19 um 11:38 schrieb Linus Walleij:
> On Mon, Jul 22, 2019 at 8:24 AM Stefan Wahren <wahrenst@gmx.net> wrote:
>
>> The BCM2711 has a new way of selecting the pull-up/pull-down setting
>> for a GPIO pin. The registers used for the BCM2835, GP_PUD and
>> GP_PUDCLKn0, are no longer connected. A new set of registers,
>> GP_GPIO_PUP_PDN_CNTRL_REGx must be used. This commit will add
>> a new compatible string "brcm,bcm2711-gpio" and the kernel
>> driver will use it to select which method is used to select
>> pull-up/pull-down.
>>
>> This patch based on a patch by Al Cooper which was intended for the
>> BCM7211. This is a bugfixed and improved version.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> Patch applied.
Thanks
>
> I think I complained about some other version of this patch, this one
> looks entirely acceptable.
>
> Can we get rid of custom pull settings etc from the upstream device
> trees so we don't set bad examples? I have a strong urge to
> throw in a pr_warn() about any use of it.

Ironically, my pre-RFC version tried to convert all BCM2835 pinmux
settings to generic ones. Unfortunately it seems that i made a mistake,
because it didn't work as expected. Since we stumpled above more and
more other issues (not relevant to pinctrl) during upstream review, i
decided to start with legacy pull-up support, so we can fix this later
in the devicetree for both platforms (currently BCM2711 uses most of the
old BCM2835 pinmuxes including the legacy stuff). So yes my plan is to
fix this soon.

Stefan

>
> Yours,
> Linus Walleij
Stefan Wahren Sept. 27, 2019, 7:28 p.m. UTC | #3 
Hi Linus,

Am 11.08.19 um 22:15 schrieb Stefan Wahren:
> Am 05.08.19 um 11:38 schrieb Linus Walleij:
>> On Mon, Jul 22, 2019 at 8:24 AM Stefan Wahren <wahrenst@gmx.net> wrote:
>>
>>> The BCM2711 has a new way of selecting the pull-up/pull-down setting
>>> for a GPIO pin. The registers used for the BCM2835, GP_PUD and
>>> GP_PUDCLKn0, are no longer connected. A new set of registers,
>>> GP_GPIO_PUP_PDN_CNTRL_REGx must be used. This commit will add
>>> a new compatible string "brcm,bcm2711-gpio" and the kernel
>>> driver will use it to select which method is used to select
>>> pull-up/pull-down.
>>>
>>> This patch based on a patch by Al Cooper which was intended for the
>>> BCM7211. This is a bugfixed and improved version.
>>>
>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> Patch applied.
> Thanks
>> I think I complained about some other version of this patch, this one
>> looks entirely acceptable.
>>
>> Can we get rid of custom pull settings etc from the upstream device
>> trees so we don't set bad examples? I have a strong urge to
>> throw in a pr_warn() about any use of it.
> Ironically, my pre-RFC version tried to convert all BCM2835 pinmux
> settings to generic ones. Unfortunately it seems that i made a mistake,
> because it didn't work as expected. Since we stumpled above more and
> more other issues (not relevant to pinctrl) during upstream review, i
> decided to start with legacy pull-up support, so we can fix this later
> in the devicetree for both platforms (currently BCM2711 uses most of the
> old BCM2835 pinmuxes including the legacy stuff). So yes my plan is to
> fix this soon.

today i had the time to try this out. Instead of the following:

    i2c0_gpio0: i2c0_gpio0 {
        brcm,pins = <0 1>;
        brcm,function = <BCM2835_FSEL_ALT0>;
        brcm,pull = <BCM2835_PUD_UP
                 BCM2835_PUD_OFF>;
    }

you want this?

    i2c0_gpio0: i2c0_gpio0 {
        pin-sda {
            function = "alt0";
            pins = "gpio0";
            bias-pull-up;
        };
        pin-scl {
            function = "alt0";
            pins = "gpio1";
            bias-disable;
        };
    };

Unfortunately i don't know U-Boot is handle the BCM2835 specific pinctrl
functions.

>
> Stefan
>
>> Yours,
>> Linus Walleij
Linus Walleij Sept. 27, 2019, 8:15 p.m. UTC | #4 
On Fri, Sep 27, 2019 at 9:29 PM Stefan Wahren <wahrenst@gmx.net> wrote:

> today i had the time to try this out. Instead of the following:
>
>     i2c0_gpio0: i2c0_gpio0 {
>         brcm,pins = <0 1>;
>         brcm,function = <BCM2835_FSEL_ALT0>;
>         brcm,pull = <BCM2835_PUD_UP
>                  BCM2835_PUD_OFF>;
>     }
>
> you want this?
>
>     i2c0_gpio0: i2c0_gpio0 {
>         pin-sda {
>             function = "alt0";
>             pins = "gpio0";
>             bias-pull-up;
>         };
>         pin-scl {
>             function = "alt0";
>             pins = "gpio1";
>             bias-disable;
>         };
>     };

Yes that looks much better. In my opinion.

I understand that it puts some developers off because of being
more lines or excessively verbose, so to be on the clear,
verboseness in itself is not the goal.

The goal is universal
portability: i.e. it should not matter one bit whether I work
on an 2001 Intel StrongARM SoC, a 2019 Broadcom SoC
or a 2011 ST-Ericsson SoC: I will understand what bias-disable;
or bias-pull-up; means, which lowers the threshold to
maintenance.

Opaque macros, however helpfully named, still creates
a higher cognitive resistance and stresses developers.

> Unfortunately i don't know U-Boot is handle the BCM2835 specific pinctrl
> functions.

I think it would be nice if boot loaders avoid to forking the
standards, but I suppose it will invariably happen.
Just keep in mind the IETF motto "rough consensus and
running code".

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 183d1ff..a493205 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -57,15 +57,24 @@ 
 #define GPAFEN0		0x88	/* Pin Async Falling Edge Detect */
 #define GPPUD		0x94	/* Pin Pull-up/down Enable */
 #define GPPUDCLK0	0x98	/* Pin Pull-up/down Enable Clock */
+#define GP_GPIO_PUP_PDN_CNTRL_REG0 0xe4 /* 2711 Pin Pull-up/down select */

 #define FSEL_REG(p)		(GPFSEL0 + (((p) / 10) * 4))
 #define FSEL_SHIFT(p)		(((p) % 10) * 3)
 #define GPIO_REG_OFFSET(p)	((p) / 32)
 #define GPIO_REG_SHIFT(p)	((p) % 32)

+#define PUD_2711_MASK		0x3
+#define PUD_2711_REG_OFFSET(p)	((p) / 16)
+#define PUD_2711_REG_SHIFT(p)	(((p) % 16) * 2)
+
 /* argument: bcm2835_pinconf_pull */
 #define BCM2835_PINCONF_PARAM_PULL	(PIN_CONFIG_END + 1)

+#define BCM2711_PULL_NONE	0x0
+#define BCM2711_PULL_UP		0x1
+#define BCM2711_PULL_DOWN	0x2
+
 struct bcm2835_pinctrl {
 	struct device *dev;
 	void __iomem *base;
@@ -975,6 +984,77 @@  static const struct pinconf_ops bcm2835_pinconf_ops = {
 	.pin_config_set = bcm2835_pinconf_set,
 };

+static void bcm2711_pull_config_set(struct bcm2835_pinctrl *pc,
+				    unsigned int pin, unsigned int arg)
+{
+	u32 shifter;
+	u32 value;
+	u32 off;
+
+	off = PUD_2711_REG_OFFSET(pin);
+	shifter = PUD_2711_REG_SHIFT(pin);
+
+	value = bcm2835_gpio_rd(pc, GP_GPIO_PUP_PDN_CNTRL_REG0 + (off * 4));
+	value &= ~(PUD_2711_MASK << shifter);
+	value |= (arg << shifter);
+	bcm2835_gpio_wr(pc, GP_GPIO_PUP_PDN_CNTRL_REG0 + (off * 4), value);
+}
+
+static int bcm2711_pinconf_set(struct pinctrl_dev *pctldev,
+			       unsigned int pin, unsigned long *configs,
+			       unsigned int num_configs)
+{
+	struct bcm2835_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) {
+		/* convert legacy brcm,pull */
+		case BCM2835_PINCONF_PARAM_PULL:
+			if (arg == BCM2835_PUD_UP)
+				arg = BCM2711_PULL_UP;
+			else if (arg == BCM2835_PUD_DOWN)
+				arg = BCM2711_PULL_DOWN;
+			else
+				arg = BCM2711_PULL_NONE;
+
+			bcm2711_pull_config_set(pc, pin, arg);
+			break;
+
+		/* Set pull generic bindings */
+		case PIN_CONFIG_BIAS_DISABLE:
+			bcm2711_pull_config_set(pc, pin, BCM2711_PULL_NONE);
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			bcm2711_pull_config_set(pc, pin, BCM2711_PULL_DOWN);
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			bcm2711_pull_config_set(pc, pin, BCM2711_PULL_UP);
+			break;
+
+		/* Set output-high or output-low */
+		case PIN_CONFIG_OUTPUT:
+			bcm2835_gpio_set_bit(pc, arg ? GPSET0 : GPCLR0, pin);
+			break;
+
+		default:
+			return -ENOTSUPP;
+		}
+	} /* for each config */
+
+	return 0;
+}
+
+static const struct pinconf_ops bcm2711_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = bcm2835_pinconf_get,
+	.pin_config_set = bcm2711_pinconf_set,
+};
+
 static struct pinctrl_desc bcm2835_pinctrl_desc = {
 	.name = MODULE_NAME,
 	.pins = bcm2835_gpio_pins,
@@ -990,6 +1070,18 @@  static struct pinctrl_gpio_range bcm2835_pinctrl_gpio_range = {
 	.npins = BCM2835_NUM_GPIOS,
 };

+static const struct of_device_id bcm2835_pinctrl_match[] = {
+	{
+		.compatible = "brcm,bcm2835-gpio",
+		.data = &bcm2835_pinconf_ops,
+	},
+	{
+		.compatible = "brcm,bcm2711-gpio",
+		.data = &bcm2711_pinconf_ops,
+	},
+	{}
+};
+
 static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -997,6 +1089,8 @@  static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 	struct bcm2835_pinctrl *pc;
 	struct resource iomem;
 	int err, i;
+	const struct of_device_id *match;
+
 	BUILD_BUG_ON(ARRAY_SIZE(bcm2835_gpio_pins) != BCM2835_NUM_GPIOS);
 	BUILD_BUG_ON(ARRAY_SIZE(bcm2835_gpio_groups) != BCM2835_NUM_GPIOS);

@@ -1073,6 +1167,12 @@  static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 					     bcm2835_gpio_irq_handler);
 	}

+	match = of_match_node(bcm2835_pinctrl_match, pdev->dev.of_node);
+	if (match) {
+		bcm2835_pinctrl_desc.confops =
+			(const struct pinconf_ops *)match->data;
+	}
+
 	pc->pctl_dev = devm_pinctrl_register(dev, &bcm2835_pinctrl_desc, pc);
 	if (IS_ERR(pc->pctl_dev)) {
 		gpiochip_remove(&pc->gpio_chip);
@@ -1087,11 +1187,6 @@  static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 	return 0;
 }

-static const struct of_device_id bcm2835_pinctrl_match[] = {
-	{ .compatible = "brcm,bcm2835-gpio" },
-	{}
-};
-
 static struct platform_driver bcm2835_pinctrl_driver = {
 	.probe = bcm2835_pinctrl_probe,
 	.driver = {