[PATCH v5 0/2] Fix Nvidia BlueField-3 GPIO access

[PATCH v5 0/2] Fix Nvidia BlueField-3 GPIO access

[Asmaa Mnebhi]

Fix Nvidia BlueField-3 GPIO access via libgpiod gpioset tool.
gpioset tool fails to modify the GPIO value due to the following:
1) the pinctrl-mlxbf3 driver defines mlxbf3_gpio_request_enable()
   to enable software to take control over a gpio. Only then can
   the gpio-mlxbf3 driver modify the direction and value of the
   gpio. mlxbf3_gpio_disable_free() gives control back to hardware
   and is called when the "gpioset" command is invoked.
   This cancels out the effort to change the GPIO value and
   direction. So mlxbf3_gpio_disable_free() needs to be removed.
2) the gpio-mlxbf3 driver calls gpiochip_generic_request() which
   calls mlxbf3_gpio_request_enable(). "pin_ranges" needs not to be
   empty for mlxbf3_gpio_request_enable() to be invoked. So
   gpio-mlxbf3 needs to populate "pin_ranges".


Asmaa Mnebhi (2):
  pinctrl: mlxbf3: Remove gpio_disable_free()
  gpio: mlxbf3: Support add_pin_ranges()

 drivers/gpio/gpio-mlxbf3.c       | 24 ++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-mlxbf3.c | 14 --------------
 2 files changed, 24 insertions(+), 14 deletions(-)

-- 
2.30.1

[PATCH v5 1/2] pinctrl: mlxbf3: Remove gpio_disable_free()

[Asmaa Mnebhi]

Remove support for gpio_disable_free() because it is called when the libgpiod
command "gpioset" is invoked. This gives the GPIO control back to hardware which
cancels out the effort to set the GPIO value.

Reminder of the code flow to change a GPIO value from software:
1) All GPIOs are controlled by hardware by default
2) To change the GPIO value, enable software control via a mux.
3) Once software has control over the GPIO pin, the gpio-mlxbf3 driver
   will be able to change the direction and value of the GPIO.

When the user runs "gpioset gpiochip0 0=0" for example, the gpio
pin value should change from 1 to 0. In this case, mlxbf3_gpio_request_enable()
is called via gpiochip_generic_request(). The latter switches GPIO control from
hardware to software. Then the GPIO value is changed from 1 to 0. However,
gpio_disable_free() is also called which changes control back to hardware
which changes the GPIO value back to 1.

Fixes: d11f932808d ("pinctrl: mlxbf3: Add pinctrl driver support")
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
v4->v5:
- No changes
v3->v4:
- No changes
v2->v3:
- No changes
v1->v2:
- No changes 

 drivers/pinctrl/pinctrl-mlxbf3.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mlxbf3.c b/drivers/pinctrl/pinctrl-mlxbf3.c
index d9944e6a0af9..0e852a0d5ec2 100644
--- a/drivers/pinctrl/pinctrl-mlxbf3.c
+++ b/drivers/pinctrl/pinctrl-mlxbf3.c
@@ -223,26 +223,12 @@ static int mlxbf3_gpio_request_enable(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static void mlxbf3_gpio_disable_free(struct pinctrl_dev *pctldev,
-				    struct pinctrl_gpio_range *range,
-				    unsigned int offset)
-{
-	struct mlxbf3_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev);
-
-	/* disable GPIO functionality by giving control back to hardware */
-	if (offset < MLXBF3_NGPIOS_GPIO0)
-		writel(BIT(offset), priv->fw_ctrl_clr0);
-	else
-		writel(BIT(offset % MLXBF3_NGPIOS_GPIO0), priv->fw_ctrl_clr1);
-}
-
 static const struct pinmux_ops mlxbf3_pmx_ops = {
 	.get_functions_count = mlxbf3_pmx_get_funcs_count,
 	.get_function_name = mlxbf3_pmx_get_func_name,
 	.get_function_groups = mlxbf3_pmx_get_groups,
 	.set_mux = mlxbf3_pmx_set,
 	.gpio_request_enable = mlxbf3_gpio_request_enable,
-	.gpio_disable_free = mlxbf3_gpio_disable_free,
 };
 
 static struct pinctrl_desc mlxbf3_pin_desc = {
-- 
2.30.1

[PATCH v5 2/2] gpio: mlxbf3: Support add_pin_ranges()

[Asmaa Mnebhi]

Support add_pin_ranges() so that pinctrl_gpio_request() can be called.
The GPIO value is not modified when the user runs the "gpioset" tool.
This is because when gpiochip_generic_request is invoked by the gpio-mlxbf3
driver, "pin_ranges" is empty so it skips "pinctrl_gpio_request()".
pinctrl_gpio_request() is essential in the code flow because it changes the
mux value so that software has control over modifying the GPIO value.
Adding add_pin_ranges() creates a dependency on the pinctrl-mlxbf3.c driver.

Fixes: cd33f216d24 ("gpio: mlxbf3: Add gpio driver support")
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v4->v5:
- Add "Reviewed-By" Tag
v3->v4:
- Drop the common define for MLXBF3_GPIO_MAX_PINS_BLOCK0
v2->v3:
- Replace boolean logic by clear switch statement logic in
  mlxbf3_gpio_add_pin_ranges()
v1->v2:
- Cleanup mlxbf3_gpio_add_pin_ranges()

 drivers/gpio/gpio-mlxbf3.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c
index e30cee108986..0a5f241a8352 100644
--- a/drivers/gpio/gpio-mlxbf3.c
+++ b/drivers/gpio/gpio-mlxbf3.c
@@ -19,6 +19,8 @@
  * gpio[1]: HOST_GPIO32->HOST_GPIO55
  */
 #define MLXBF3_GPIO_MAX_PINS_PER_BLOCK 32
+#define MLXBF3_GPIO_MAX_PINS_BLOCK0    32
+#define MLXBF3_GPIO_MAX_PINS_BLOCK1    24
 
 /*
  * fw_gpio[x] block registers and their offset
@@ -158,6 +160,26 @@ static const struct irq_chip gpio_mlxbf3_irqchip = {
 	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
+static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip)
+{
+	unsigned int id;
+
+	switch(chip->ngpio) {
+	case MLXBF3_GPIO_MAX_PINS_BLOCK0:
+		id = 0;
+		break;
+	case MLXBF3_GPIO_MAX_PINS_BLOCK1:
+		id = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return gpiochip_add_pin_range(chip, "MLNXBF34:00",
+			chip->base, id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK,
+			chip->ngpio);
+}
+
 static int mlxbf3_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -197,6 +219,7 @@ static int mlxbf3_gpio_probe(struct platform_device *pdev)
 	gc->request = gpiochip_generic_request;
 	gc->free = gpiochip_generic_free;
 	gc->owner = THIS_MODULE;
+	gc->add_pin_ranges = mlxbf3_gpio_add_pin_ranges;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq >= 0) {
@@ -243,6 +266,7 @@ static struct platform_driver mlxbf3_gpio_driver = {
 };
 module_platform_driver(mlxbf3_gpio_driver);
 
+MODULE_SOFTDEP("pre: pinctrl-mlxbf3");
 MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
 MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
 MODULE_LICENSE("Dual BSD/GPL");
-- 
2.30.1

Re: [PATCH v5 2/2] gpio: mlxbf3: Support add_pin_ranges()

[Bartosz Golaszewski]

On Fri, Aug 18, 2023 at 6:43 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> Support add_pin_ranges() so that pinctrl_gpio_request() can be called.
> The GPIO value is not modified when the user runs the "gpioset" tool.
> This is because when gpiochip_generic_request is invoked by the gpio-mlxbf3
> driver, "pin_ranges" is empty so it skips "pinctrl_gpio_request()".
> pinctrl_gpio_request() is essential in the code flow because it changes the
> mux value so that software has control over modifying the GPIO value.
> Adding add_pin_ranges() creates a dependency on the pinctrl-mlxbf3.c driver.
>
> Fixes: cd33f216d24 ("gpio: mlxbf3: Add gpio driver support")
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> v4->v5:
> - Add "Reviewed-By" Tag
> v3->v4:
> - Drop the common define for MLXBF3_GPIO_MAX_PINS_BLOCK0
> v2->v3:
> - Replace boolean logic by clear switch statement logic in
>   mlxbf3_gpio_add_pin_ranges()
> v1->v2:
> - Cleanup mlxbf3_gpio_add_pin_ranges()
>
>  drivers/gpio/gpio-mlxbf3.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mlxbf3.c b/drivers/gpio/gpio-mlxbf3.c
> index e30cee108986..0a5f241a8352 100644
> --- a/drivers/gpio/gpio-mlxbf3.c
> +++ b/drivers/gpio/gpio-mlxbf3.c
> @@ -19,6 +19,8 @@
>   * gpio[1]: HOST_GPIO32->HOST_GPIO55
>   */
>  #define MLXBF3_GPIO_MAX_PINS_PER_BLOCK 32
> +#define MLXBF3_GPIO_MAX_PINS_BLOCK0    32
> +#define MLXBF3_GPIO_MAX_PINS_BLOCK1    24
>
>  /*
>   * fw_gpio[x] block registers and their offset
> @@ -158,6 +160,26 @@ static const struct irq_chip gpio_mlxbf3_irqchip = {
>         GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>
> +static int mlxbf3_gpio_add_pin_ranges(struct gpio_chip *chip)
> +{
> +       unsigned int id;
> +
> +       switch(chip->ngpio) {
> +       case MLXBF3_GPIO_MAX_PINS_BLOCK0:
> +               id = 0;
> +               break;
> +       case MLXBF3_GPIO_MAX_PINS_BLOCK1:
> +               id = 1;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return gpiochip_add_pin_range(chip, "MLNXBF34:00",
> +                       chip->base, id * MLXBF3_GPIO_MAX_PINS_PER_BLOCK,
> +                       chip->ngpio);
> +}
> +
>  static int mlxbf3_gpio_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -197,6 +219,7 @@ static int mlxbf3_gpio_probe(struct platform_device *pdev)
>         gc->request = gpiochip_generic_request;
>         gc->free = gpiochip_generic_free;
>         gc->owner = THIS_MODULE;
> +       gc->add_pin_ranges = mlxbf3_gpio_add_pin_ranges;
>
>         irq = platform_get_irq(pdev, 0);
>         if (irq >= 0) {
> @@ -243,6 +266,7 @@ static struct platform_driver mlxbf3_gpio_driver = {
>  };
>  module_platform_driver(mlxbf3_gpio_driver);
>
> +MODULE_SOFTDEP("pre: pinctrl-mlxbf3");
>  MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
>  MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
>  MODULE_LICENSE("Dual BSD/GPL");
> --
> 2.30.1
>

It's not clear to me whether this depends on patch 1? If only at
run-time then I guess Linus and I can take the two patches through
ours respective trees?

Bart

RE: [PATCH v5 2/2] gpio: mlxbf3: Support add_pin_ranges()

[Asmaa Mnebhi]

> > +MODULE_SOFTDEP("pre: pinctrl-mlxbf3");
> >  MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
> > MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
> > MODULE_LICENSE("Dual BSD/GPL");
> > --
> > 2.30.1
> >
> 
> It's not clear to me whether this depends on patch 1? If only at run-time then I
> guess Linus and I can take the two patches through ours respective trees?

Indeed from a build point of view, there is no dependency so you could take the 2 patches through your respective tree. However, at run-time, the gpio-mlxbf3.c driver fails to load without the pinctrl-mlxbf3.c driver. Should I add a "depends on" in the Kconfig? Then you will have to include both patches in your tree.

Thanks.
Asmaa

Re: [PATCH v5 2/2] gpio: mlxbf3: Support add_pin_ranges()

[Bartosz Golaszewski]

On Mon, Aug 21, 2023 at 2:55 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> > > +MODULE_SOFTDEP("pre: pinctrl-mlxbf3");
> > >  MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
> > > MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
> > > MODULE_LICENSE("Dual BSD/GPL");
> > > --
> > > 2.30.1
> > >
> >
> > It's not clear to me whether this depends on patch 1? If only at run-time then I
> > guess Linus and I can take the two patches through ours respective trees?
>
> Indeed from a build point of view, there is no dependency so you could take the 2 patches through your respective tree. However, at run-time, the gpio-mlxbf3.c driver fails to load without the pinctrl-mlxbf3.c driver. Should I add a "depends on" in the Kconfig? Then you will have to include both patches in your tree.
>

Linus, are you fine with me taking this patch? It will not break the
build and with you taking the other one, next will be fine too.

Bart

Re: [PATCH v5 2/2] gpio: mlxbf3: Support add_pin_ranges()

[Linus Walleij]

On Mon, Aug 21, 2023 at 5:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Mon, Aug 21, 2023 at 2:55 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
> >
> > > > +MODULE_SOFTDEP("pre: pinctrl-mlxbf3");
> > > >  MODULE_DESCRIPTION("NVIDIA BlueField-3 GPIO Driver");
> > > > MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>");
> > > > MODULE_LICENSE("Dual BSD/GPL");
> > > > --
> > > > 2.30.1
> > > >
> > >
> > > It's not clear to me whether this depends on patch 1? If only at run-time then I
> > > guess Linus and I can take the two patches through ours respective trees?
> >
> > Indeed from a build point of view, there is no dependency so you could take the 2 patches through your respective tree. However, at run-time, the gpio-mlxbf3.c driver fails to load without the pinctrl-mlxbf3.c driver. Should I add a "depends on" in the Kconfig? Then you will have to include both patches in your tree.
> >
>
> Linus, are you fine with me taking this patch? It will not break the
> build and with you taking the other one, next will be fine too.

Yep pick this one, I applied 1/2 to the pinctrl tree now.

Yours,
Linus Walleij

Re: [PATCH v5 0/2] Fix Nvidia BlueField-3 GPIO access

[Linus Walleij]

On Fri, Aug 18, 2023 at 6:43 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> Fix Nvidia BlueField-3 GPIO access via libgpiod gpioset tool.
> gpioset tool fails to modify the GPIO value due to the following:
> 1) the pinctrl-mlxbf3 driver defines mlxbf3_gpio_request_enable()
>    to enable software to take control over a gpio. Only then can
>    the gpio-mlxbf3 driver modify the direction and value of the
>    gpio. mlxbf3_gpio_disable_free() gives control back to hardware
>    and is called when the "gpioset" command is invoked.
>    This cancels out the effort to change the GPIO value and
>    direction. So mlxbf3_gpio_disable_free() needs to be removed.
> 2) the gpio-mlxbf3 driver calls gpiochip_generic_request() which
>    calls mlxbf3_gpio_request_enable(). "pin_ranges" needs not to be
>    empty for mlxbf3_gpio_request_enable() to be invoked. So
>    gpio-mlxbf3 needs to populate "pin_ranges".

This patch set looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij