[PATCH 0/4] gpio/pinctrl-rockchip: Fixes for the recently separated gpio/pinctrl driver

It looks like rk3288-veyron(-pinky) was the one device in my boardfarm
I didn't test the pinctrl/gpio patches on and it seems this one uses
some specific parts none of the other do. So when I did my v5.15-rc1
testrun I got a surprise.

Not only did the pinctrl-hogs cause a null-pointer exception but the
device also entered a reset loop a bit later in the boot.

This series addresses the issues in hopefully a nice way and should
ideally become part of 5.15 before other people run into issues.

* Patch 1 addresses the reset-loop, which is caused by a not-ideal
  check vor v1 vs. v2 controller in the debounce code
* Patch 2 is just a find when looking through the code
* Patches 3+4 address the pinctrl-hogs issue by creating a deferred
  queue where the pinctrl can temporarily store these hog settings
  if needed and the pinctrl driver can retrieve them during probe.


Heiko Stuebner (4):
  gpio/rockchip: extended debounce support is only available on v2
  gpio/rockchip: fix get_direction value handling
  pinctrl/rockchip: add a queue for deferred pin output settings on
    probe
  gpio/rockchip: fetch deferred output settings on probe

 drivers/gpio/gpio-rockchip.c       | 26 +++++++++++-
 drivers/pinctrl/pinctrl-rockchip.c | 67 ++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-rockchip.h | 10 +++++
 3 files changed, 101 insertions(+), 2 deletions(-)

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The separation of pinctrl and gpio drivers created a tiny window where
a pinconfig setting might produce a null-pointer dereference.

The affected device were rk3288-veyron devices in this case.

Pinctrl-hogs are claimed when the pinctrl driver is registered,
at which point their pinconfig settings will be applied.
At this time the now separate gpio devices will not have been created
yet and the matching driver won't have probed yet, making the gpio->foo()
call run into a null-ptr.

As probing is not really guaranteed to have been completed at a specific
time, introduce a queue that can hold the output settings until the gpio
driver has probed and will (in a separate patch) fetch the elements
of the list.

We expect the gpio driver to empty the list, but will nevertheless empty
it ourself on remove if that didn't happen.

Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/pinctrl/pinctrl-rockchip.c | 67 ++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-rockchip.h | 10 +++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index ae33e376695f..5ce260f152ce 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c

@@ -2092,6 +2092,23 @@ static bool rockchip_pinconf_pull_valid(struct rockchip_pin_ctrl *ctrl,
 	return false;
 }
 
+static int rockchip_pinconf_defer_output(struct rockchip_pin_bank *bank,
+					 unsigned int pin, u32 arg)
+{
+	struct rockchip_pin_output_deferred *cfg;
+
+	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	cfg->pin = pin;
+	cfg->arg = arg;
+
+	list_add_tail(&cfg->head, &bank->deferred_output);
+
+	return 0;
+}
+
 /* set the pin config settings for a specified pin */
 static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 				unsigned long *configs, unsigned num_configs)

@@ -2136,6 +2153,22 @@ static int rockchip_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			if (rc != RK_FUNC_GPIO)
 				return -EINVAL;
 
+			/*
+			 * Check for gpio driver not being probed yet.
+			 * The lock makes sure that either gpio-probe has completed
+			 * or the gpio driver hasn't probed yet.
+			 */
+			mutex_lock(&bank->deferred_lock);
+			if (!gpio || !gpio->direction_output) {
+				rc = rockchip_pinconf_defer_output(bank, pin - bank->pin_base, arg);
+				mutex_unlock(&bank->deferred_lock);
+				if (rc)
+					return rc;
+
+				break;
+			}
+			mutex_unlock(&bank->deferred_lock);
+
 			rc = gpio->direction_output(gpio, pin - bank->pin_base,
 						    arg);
 			if (rc)

@@ -2204,6 +2237,11 @@ static int rockchip_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 		if (rc != RK_FUNC_GPIO)
 			return -EINVAL;
 
+		if (!gpio || !gpio->get) {
+			arg = 0;
+			break;
+		}
+
 		rc = gpio->get(gpio, pin - bank->pin_base);
 		if (rc < 0)
 			return rc;

@@ -2450,6 +2488,9 @@ static int rockchip_pinctrl_register(struct platform_device *pdev,
 						pin_bank->name, pin);
 			pdesc++;
 		}
+
+		INIT_LIST_HEAD(&pin_bank->deferred_output);
+		mutex_init(&pin_bank->deferred_lock);
 	}
 
 	ret = rockchip_pinctrl_parse_dt(pdev, info);

@@ -2716,6 +2757,31 @@ static int rockchip_pinctrl_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int rockchip_pinctrl_remove(struct platform_device *pdev)
+{
+	struct rockchip_pinctrl *info = platform_get_drvdata(pdev);
+	struct rockchip_pin_bank *bank;
+	struct rockchip_pin_output_deferred *cfg;
+	int i;
+
+	of_platform_depopulate(&pdev->dev);
+
+	for (i = 0; i < info->ctrl->nr_banks; i++) {
+		bank = &info->ctrl->pin_banks[i];
+
+		mutex_lock(&bank->deferred_lock);
+		while (!list_empty(&bank->deferred_output)) {
+			cfg = list_first_entry(&bank->deferred_output,
+					       struct rockchip_pin_output_deferred, head);
+			list_del(&cfg->head);
+			kfree(cfg);
+		}
+		mutex_unlock(&bank->deferred_lock);
+	}
+
+	return 0;
+}
+
 static struct rockchip_pin_bank px30_pin_banks[] = {
 	PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", IOMUX_SOURCE_PMU,
 					     IOMUX_SOURCE_PMU,

@@ -3175,6 +3241,7 @@ static const struct of_device_id rockchip_pinctrl_dt_match[] = {
 
 static struct platform_driver rockchip_pinctrl_driver = {
 	.probe		= rockchip_pinctrl_probe,
+	.remove		= rockchip_pinctrl_remove,
 	.driver = {
 		.name	= "rockchip-pinctrl",
 		.pm = &rockchip_pinctrl_dev_pm_ops,

diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
index 589d4d2a98c9..91f10279d084 100644
--- a/drivers/pinctrl/pinctrl-rockchip.h
+++ b/drivers/pinctrl/pinctrl-rockchip.h

@@ -141,6 +141,8 @@ struct rockchip_drv {
  * @toggle_edge_mode: bit mask to toggle (falling/rising) edge mode
  * @recalced_mask: bit mask to indicate a need to recalulate the mask
  * @route_mask: bits describing the routing pins of per bank
+ * @deferred_output: gpio output settings to be done after gpio bank probed
+ * @deferred_lock: mutex for the deferred_output shared btw gpio and pinctrl
  */
 struct rockchip_pin_bank {
 	struct device			*dev;

@@ -169,6 +171,8 @@ struct rockchip_pin_bank {
 	u32				toggle_edge_mode;
 	u32				recalced_mask;
 	u32				route_mask;
+	struct list_head		deferred_output;
+	struct mutex			deferred_lock;
 };
 
 /**

@@ -243,6 +247,12 @@ struct rockchip_pin_config {
 	unsigned int		nconfigs;
 };
 
+struct rockchip_pin_output_deferred {
+	struct list_head head;
+	unsigned int pin;
+	u32 arg;
+};
+
 /**
  * struct rockchip_pin_group: represent group of pins of a pinmux function.
  * @name: name of the pin group, used to lookup the group.

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Fetch the output settings the pinctrl driver may have created
for pinctrl hogs and set the relevant pins as requested.

Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpio/gpio-rockchip.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 3335bd57761d..cf1b465db8c3 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c

@@ -689,6 +689,7 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 	struct device_node *pctlnp = of_get_parent(np);
 	struct pinctrl_dev *pctldev = NULL;
 	struct rockchip_pin_bank *bank = NULL;
+	struct rockchip_pin_output_deferred *cfg;
 	static int gpio;
 	int id, ret;
 

@@ -716,12 +717,33 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Prevent clashes with a deferred output setting
+	 * being added right at this moment.
+	 */
+	mutex_lock(&bank->deferred_lock);
+
 	ret = rockchip_gpiolib_register(bank);
 	if (ret) {
 		clk_disable_unprepare(bank->clk);
+		mutex_unlock(&bank->deferred_lock);
 		return ret;
 	}
 
+	while (!list_empty(&bank->deferred_output)) {
+		cfg = list_first_entry(&bank->deferred_output,
+				       struct rockchip_pin_output_deferred, head);
+		list_del(&cfg->head);
+
+		ret = rockchip_gpio_direction_output(&bank->gpio_chip, cfg->pin, cfg->arg);
+		if (ret)
+			dev_warn(dev, "setting output pin %u to %u failed\n", cfg->pin, cfg->arg);
+
+		kfree(cfg);
+	}
+
+	mutex_unlock(&bank->deferred_lock);
+
 	platform_set_drvdata(pdev, bank);
 	dev_info(dev, "probed %pOF\n", np);
 

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The gpio driver runs into issues on v1 gpio blocks, as the db_clk
and the whole extended debounce support is only ever defined on v2.
So checking for the IS_ERR on the db_clk is not enough, as it will
be NULL on v1.

Fix this by adding the needed condition for v2 first before checking
the existence of the db_clk.

This caused my rk3288-veyron-pinky to enter a reboot loop when it
tried to enable the power-key as adc-key device.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpio/gpio-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 036b2d959503..16d9bf7188e3 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c

@@ -195,7 +195,7 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 	unsigned int cur_div_reg;
 	u64 div;
 
-	if (!IS_ERR(bank->db_clk)) {
+	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
 		div_debounce_support = true;
 		freq = clk_get_rate(bank->db_clk);
 		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The function uses the newly introduced rockchip_gpio_readl_bit()
which directly returns the actual value of the requeste bit.
So using the existing bit-wise check for the bit inside the value
will always return 0.

Fix this by dropping the bit manipulation on the result.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpio/gpio-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 16d9bf7188e3..3335bd57761d 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c

@@ -141,7 +141,7 @@ static int rockchip_gpio_get_direction(struct gpio_chip *chip,
 	u32 data;
 
 	data = rockchip_gpio_readl_bit(bank, offset, bank->gpio_regs->port_ddr);
-	if (data & BIT(offset))
+	if (data)
 		return GPIO_LINE_DIRECTION_OUT;
 
 	return GPIO_LINE_DIRECTION_IN;

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

The gpio driver runs into issues on v1 gpio blocks, as the db_clk
and the whole extended debounce support is only ever defined on v2.
So checking for the IS_ERR on the db_clk is not enough, as it will
be NULL on v1.

Fix this by adding the needed condition for v2 first before checking
the existence of the db_clk.

This caused my rk3288-veyron-pinky to enter a reboot loop when it
tried to enable the power-key as adc-key device.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Reviewed-by: Linus Walleij <redacted>
This can be applied by Bartosz to the GPIO subsystem.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

The function uses the newly introduced rockchip_gpio_readl_bit()
which directly returns the actual value of the requeste bit.
So using the existing bit-wise check for the bit inside the value
will always return 0.

Fix this by dropping the bit manipulation on the result.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Reviewed-by: Linus Walleij <redacted>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

The separation of pinctrl and gpio drivers created a tiny window where
a pinconfig setting might produce a null-pointer dereference.

The affected device were rk3288-veyron devices in this case.

Pinctrl-hogs are claimed when the pinctrl driver is registered,
at which point their pinconfig settings will be applied.
At this time the now separate gpio devices will not have been created
yet and the matching driver won't have probed yet, making the gpio->foo()
call run into a null-ptr.

As probing is not really guaranteed to have been completed at a specific
time, introduce a queue that can hold the output settings until the gpio
driver has probed and will (in a separate patch) fetch the elements
of the list.

We expect the gpio driver to empty the list, but will nevertheless empty
it ourself on remove if that didn't happen.

Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Hm this is not very elegant but what can we do.
Tentatively applied for fixes.

Can we not use device links to get around this?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

Fetch the output settings the pinctrl driver may have created
for pinctrl hogs and set the relevant pins as requested.

Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Since this patch depends on patch 4/4 I applied this to the pinctrl
tree as well.

I still think this looks a bit kludgy but can't think of anything better
right now and we need a fix for the problem so this goes in.

But we need to think of something better,

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Linus,

Am Samstag, 18. September 2021, 01:38:08 CEST schrieb Linus Walleij:

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

quoted

Fetch the output settings the pinctrl driver may have created
for pinctrl hogs and set the relevant pins as requested.

Fixes: 9ce9a02039de ("pinctrl/rockchip: drop the gpio related codes")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>

Since this patch depends on patch 4/4 I applied this to the pinctrl
tree as well.

I still think this looks a bit kludgy but can't think of anything better
right now and we need a fix for the problem so this goes in.

But we need to think of something better,

I'm all ears :-) . And yes I do agree with you that this is not very
elegant right now.

The issue is that the pinconf part for PIN_CONFIG_OUTPUT is actually
using the gpio controller to realize this setting. So when this ends up
in a pinctrl-hog, stuff explodes while probing the first pinctrl part.

I guess one way would be to somehow only do the pinctrl-hogs
_after_ all parts have probed.


Thinking about this, the component framework may be one option?
And then adding a pinctr-register / init+enable variant where the
pinctrl hogs can be aquired separately, not as part of pinctrl_enable?

Or maybe I'm thinking way too complex and a way easier solution
is around the corner ;-) .


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Sat, Sep 18, 2021 at 2:00 AM Heiko Stübner [off-list ref] wrote:

The issue is that the pinconf part for PIN_CONFIG_OUTPUT is actually
using the gpio controller to realize this setting. So when this ends up
in a pinctrl-hog, stuff explodes while probing the first pinctrl part.

The Nomadik driver has something similar, I came up with a solution
ages ago which isn't elegant either, so it's not like I'm any better :/

commit ab4a936247561cd998913bab5f15e3d3eaed1f9e
"pinctrl: nomadik: assure GPIO chips are populated"

Thinking about this, the component framework may be one option?
And then adding a pinctr-register / init+enable variant where the
pinctrl hogs can be aquired separately, not as part of pinctrl_enable?

Check out my commit, but the component framework is what we
should ideally use (IMO) when drivers depend on each other
so I think you are right.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

The gpio driver runs into issues on v1 gpio blocks, as the db_clk
and the whole extended debounce support is only ever defined on v2.
So checking for the IS_ERR on the db_clk is not enough, as it will
be NULL on v1.

Fix this by adding the needed condition for v2 first before checking
the existence of the db_clk.

This caused my rk3288-veyron-pinky to enter a reboot loop when it
tried to enable the power-key as adc-key device.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---

Queued for fixes. Thanks!

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 14, 2021 at 12:49 AM Heiko Stuebner [off-list ref] wrote:

The function uses the newly introduced rockchip_gpio_readl_bit()
which directly returns the actual value of the requeste bit.
So using the existing bit-wise check for the bit inside the value
will always return 0.

Fix this by dropping the bit manipulation on the result.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---

Queued for fixes. Thanks!

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel