[PATCH 0/5] gpio: thunderx: Marvell GPIO changes.

[PATCH 0/5] gpio: thunderx: Marvell GPIO changes.

[Piyush Malgujar]

This patch includes the following changes:

- Using irqsave/irqrestore spinlock variants to avoid any potential
  deadlock in case of GPIO as IRQ
- Introducing a new device tree option 'pin-cfg' which can be used to setup
  mode of one or several GPIO pins. It can be used to choose pin's
  function, filtes, polarity and direction.
- Extending PIN_SEL_MASK to cover for otx2 platform
- In case of level irq, the current handle is not unmasking the GPIO
  interrupt, so using handle_level_irq, which will allow next interrupt
  to be handled.

All the changes have been internally verified on Marvell otx2 platforms.

Piyush Malgujar (5):
  gpio: thunderx: avoid potential deadlock
  dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option
  gpio: thunderx: Configure GPIO pins at probe
  gpio: thunderx: extend PIN_SEL_MASK to cover otx2 platform
  gpio: thunderx: change handler for level interrupt

 .../bindings/gpio/gpio-thunderx.txt           |  4 ++
 drivers/gpio/gpio-thunderx.c                  | 53 +++++++++++++++----
 2 files changed, 47 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH 1/5] gpio: thunderx: avoid potential deadlock

[Piyush Malgujar]

Using irqsave/irqrestore locking variants to avoid any deadlock.

Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/gpio/gpio-thunderx.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 9f66deab46eaa99d05413a996b585284c433574d..bb2b40e4033b00134af35592b6b7c7f83cf6c737 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -104,16 +104,17 @@ static int thunderx_gpio_request(struct gpio_chip *chip, unsigned int line)
 static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
 {
 	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+	unsigned long flags;
 
 	if (!thunderx_gpio_is_gpio(txgpio, line))
 		return -EIO;
 
-	raw_spin_lock(&txgpio->lock);
+	raw_spin_lock_irqsave(&txgpio->lock, flags);
 	clear_bit(line, txgpio->invert_mask);
 	clear_bit(line, txgpio->od_mask);
 	writeq(txgpio->line_entries[line].fil_bits,
 	       txgpio->register_base + bit_cfg_reg(line));
-	raw_spin_unlock(&txgpio->lock);
+	raw_spin_unlock_irqrestore(&txgpio->lock, flags);
 	return 0;
 }
 
@@ -135,11 +136,12 @@ static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
 {
 	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
 	u64 bit_cfg = txgpio->line_entries[line].fil_bits | GPIO_BIT_CFG_TX_OE;
+	unsigned long flags;
 
 	if (!thunderx_gpio_is_gpio(txgpio, line))
 		return -EIO;
 
-	raw_spin_lock(&txgpio->lock);
+	raw_spin_lock_irqsave(&txgpio->lock, flags);
 
 	thunderx_gpio_set(chip, line, value);
 
@@ -151,7 +153,7 @@ static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
 
 	writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line));
 
-	raw_spin_unlock(&txgpio->lock);
+	raw_spin_unlock_irqrestore(&txgpio->lock, flags);
 	return 0;
 }
 
@@ -188,11 +190,12 @@ static int thunderx_gpio_set_config(struct gpio_chip *chip,
 	int ret = -ENOTSUPP;
 	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
 	void __iomem *reg = txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET;
+	unsigned long flags;
 
 	if (!thunderx_gpio_is_gpio(txgpio, line))
 		return -EIO;
 
-	raw_spin_lock(&txgpio->lock);
+	raw_spin_lock_irqsave(&txgpio->lock, flags);
 	orig_invert = test_bit(line, txgpio->invert_mask);
 	new_invert  = orig_invert;
 	orig_od = test_bit(line, txgpio->od_mask);
@@ -243,7 +246,7 @@ static int thunderx_gpio_set_config(struct gpio_chip *chip,
 	default:
 		break;
 	}
-	raw_spin_unlock(&txgpio->lock);
+	raw_spin_unlock_irqrestore(&txgpio->lock, flags);
 
 	/*
 	 * If currently output and OPEN_DRAIN changed, install the new
@@ -329,6 +332,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
 	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
 	struct thunderx_line *txline =
 		&txgpio->line_entries[irqd_to_hwirq(d)];
+	unsigned long flags;
 	u64 bit_cfg;
 
 	irqd_set_trigger_type(d, flow_type);
@@ -342,7 +346,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
 		irq_set_handler_locked(d, handle_fasteoi_mask_irq);
 	}
 
-	raw_spin_lock(&txgpio->lock);
+	raw_spin_lock_irqsave(&txgpio->lock, flags);
 	if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
 		bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
 		set_bit(txline->line, txgpio->invert_mask);
@@ -351,7 +355,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
 	}
 	clear_bit(txline->line, txgpio->od_mask);
 	writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(txline->line));
-	raw_spin_unlock(&txgpio->lock);
+	raw_spin_unlock_irqrestore(&txgpio->lock, flags);
 
 	return IRQ_SET_MASK_OK;
 }
-- 
2.17.1

[PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Piyush Malgujar]

Add support for pin-cfg to configure GPIO Pins

Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
index 3f883ae29d116887e702ead20b26a25f9d2349d5..05f0be98afdcae941ff8a24c3fdabd8af83ccb87 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
@@ -14,6 +14,9 @@ Optional Properties:
                     "interrupt-controller" is present.
   - First cell is the GPIO pin number relative to the controller.
   - Second cell is triggering flags as defined in interrupts.txt.
+- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
+  - First cell is the GPIO pin number
+  - Second cell is a value written to GPIO_BIT_CFG register at driver probe.
 
 Example:
 
@@ -24,4 +27,5 @@ gpio_6_0: gpio@6,0 {
 	#gpio-cells = <2>;
 	interrupt-controller;
 	#interrupt-cells = <2>;
+	pin-cfg = <57 0x2300000>, <58 0x2500000>;
 };
-- 
2.17.1

[PATCH 3/5] gpio: thunderx: Configure GPIO pins at probe

[Piyush Malgujar]

Add support to configure GPIO pins using DTS 'pin-cfg'

Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/gpio/gpio-thunderx.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index bb2b40e4033b00134af35592b6b7c7f83cf6c737..451c412512450fea717937376002d2ba35d1c508 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -426,6 +426,32 @@ static void *thunderx_gpio_populate_parent_alloc_info(struct gpio_chip *chip,
 	return info;
 }
 
+static void thunderx_gpio_pinsel(struct device *dev,
+				 struct thunderx_gpio *txgpio)
+{
+	struct device_node *node;
+	const __be32 *pinsel;
+	int npins, rlen, i;
+	u32 pin, sel;
+
+	node = dev_of_node(dev);
+	if (!node)
+		return;
+
+	pinsel = of_get_property(node, "pin-cfg", &rlen);
+	if (!pinsel || rlen % 2)
+		return;
+
+	npins = rlen / sizeof(__be32) / 2;
+
+	for (i = 0; i < npins; i++) {
+		pin = of_read_number(pinsel++, 1);
+		sel = of_read_number(pinsel++, 1);
+		dev_dbg(dev, "Set GPIO pin %d CFG register to %x\n", pin, sel);
+		writeq(sel, txgpio->register_base + bit_cfg_reg(pin));
+	}
+}
+
 static int thunderx_gpio_probe(struct pci_dev *pdev,
 			       const struct pci_device_id *id)
 {
@@ -548,6 +574,9 @@ static int thunderx_gpio_probe(struct pci_dev *pdev,
 	if (err)
 		goto out;
 
+	/* Configure default functions of GPIO pins */
+	thunderx_gpio_pinsel(dev, txgpio);
+
 	/* Push on irq_data and the domain for each line. */
 	for (i = 0; i < ngpio; i++) {
 		struct irq_fwspec fwspec;
-- 
2.17.1

[PATCH 4/5] gpio: thunderx: extend PIN_SEL_MASK to cover otx2 platform

[Piyush Malgujar]

Extending the PIN_SEL_MASK to support otx2 platform which was
earlier RAZ.

Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/gpio/gpio-thunderx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 451c412512450fea717937376002d2ba35d1c508..87ab1ad7e652347a67b7747ea497b944498a8b41 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -32,7 +32,7 @@
 #define  GPIO_BIT_CFG_FIL_CNT_SHIFT	4
 #define  GPIO_BIT_CFG_FIL_SEL_SHIFT	8
 #define  GPIO_BIT_CFG_TX_OD		BIT(12)
-#define  GPIO_BIT_CFG_PIN_SEL_MASK	GENMASK(25, 16)
+#define  GPIO_BIT_CFG_PIN_SEL_MASK	GENMASK(26, 16)
 #define GPIO_INTR	0x800
 #define  GPIO_INTR_INTR			BIT(0)
 #define  GPIO_INTR_INTR_W1S		BIT(1)
-- 
2.17.1

[PATCH 5/5] gpio: thunderx: change handler for level interrupt

[Piyush Malgujar]

The current level interrupt handler is masking the GPIO interrupt
and not unmasking it, to resolve that, handle_level_irq is used.

Signed-off-by: Witold Sadowski <wsadowski@marvell.com>
Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 drivers/gpio/gpio-thunderx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 87ab1ad7e652347a67b7747ea497b944498a8b41..b1063e53ceb8edf26ca1a6ecab8035aad62128a1 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -343,7 +343,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
 		irq_set_handler_locked(d, handle_fasteoi_ack_irq);
 		bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
 	} else {
-		irq_set_handler_locked(d, handle_fasteoi_mask_irq);
+		irq_set_handler_locked(d, handle_level_irq);
 	}
 
 	raw_spin_lock_irqsave(&txgpio->lock, flags);
-- 
2.17.1

Re: [PATCH 3/5] gpio: thunderx: Configure GPIO pins at probe

[kernel test robot]

Hi Piyush,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on linusw-gpio/for-next linux/master v5.18-rc4 next-20220427]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Piyush-Malgujar/gpio-thunderx-Marvell-GPIO-changes/20220427-225001
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 46cf2c613f4b10eb12f749207b0fd2c1bfae3088
config: alpha-randconfig-r005-20220427 (https://download.01.org/0day-ci/archive/20220428/202204280405.DMzMLx60-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/31a85ad65112e3ed61aa418772670eb96a881a4f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Piyush-Malgujar/gpio-thunderx-Marvell-GPIO-changes/20220427-225001
        git checkout 31a85ad65112e3ed61aa418772670eb96a881a4f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/gpio/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-thunderx.c: In function 'thunderx_gpio_pinsel':
>> drivers/gpio/gpio-thunderx.c:448:23: error: implicit declaration of function 'of_read_number' [-Werror=implicit-function-declaration]
     448 |                 pin = of_read_number(pinsel++, 1);
         |                       ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/of_read_number +448 drivers/gpio/gpio-thunderx.c

   428	
   429	static void thunderx_gpio_pinsel(struct device *dev,
   430					 struct thunderx_gpio *txgpio)
   431	{
   432		struct device_node *node;
   433		const __be32 *pinsel;
   434		int npins, rlen, i;
   435		u32 pin, sel;
   436	
   437		node = dev_of_node(dev);
   438		if (!node)
   439			return;
   440	
   441		pinsel = of_get_property(node, "pin-cfg", &rlen);
   442		if (!pinsel || rlen % 2)
   443			return;
   444	
   445		npins = rlen / sizeof(__be32) / 2;
   446	
   447		for (i = 0; i < npins; i++) {
 > 448			pin = of_read_number(pinsel++, 1);
   449			sel = of_read_number(pinsel++, 1);
   450			dev_dbg(dev, "Set GPIO pin %d CFG register to %x\n", pin, sel);
   451			writeq(sel, txgpio->register_base + bit_cfg_reg(pin));
   452		}
   453	}
   454	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Linus Walleij]

On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:

> Add support for pin-cfg to configure GPIO Pins
>
> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++

Would be nice to rewrite this binding in YAML

>    - First cell is the GPIO pin number relative to the controller.
>    - Second cell is triggering flags as defined in interrupts.txt.
> +- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
> +  - First cell is the GPIO pin number
> +  - Second cell is a value written to GPIO_BIT_CFG register at driver probe.

Just poking magic hex values into some random register as
part of a binding is not a good idea.

This looks like trying to reinvent the pin config subsystem.

GPIO is using the standard pin configurations in the second cell of
the handle, use them in this driver as well and add new ones if we
need.

You find the existing flags here:
include/dt-bindings/gpio/gpio.h

If you need something more sophisticated than a simple flag, I think
you need to implement proper pin config.

Yours,
Linus Walleij

Re: [PATCH 5/5] gpio: thunderx: change handler for level interrupt

[Linus Walleij]

On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:

> The current level interrupt handler is masking the GPIO interrupt
> and not unmasking it, to resolve that, handle_level_irq is used.
>
> Signed-off-by: Witold Sadowski <wsadowski@marvell.com>
> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/5] gpio: thunderx: avoid potential deadlock

[Bartosz Golaszewski]

On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:
>
> Using irqsave/irqrestore locking variants to avoid any deadlock.
>

I see you'll be resending this anyway so would you mind providing an
example of a deadlock that is possible with no-irqsave variants?
Thanks.

Bart

> Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> ---

Re: [PATCH 1/5] gpio: thunderx: avoid potential deadlock

[Piyush Malgujar]

On Mon, May 02, 2022 at 01:18:49PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> >
> > Using irqsave/irqrestore locking variants to avoid any deadlock.
> >
> 
> I see you'll be resending this anyway so would you mind providing an
> example of a deadlock that is possible with no-irqsave variants?
> Thanks.
> 
> Bart
>
Hi Bartosz,

Thanks for the review.

Please find below the issue scenario:
In the case when HARDIRQ-safe -> HARDIRQ-unsafe lock order is detected
and interrupt occurs, deadlock could occur.

========================================================
WARNING: possible irq lock inversion dependency detected
5.18.0-rc6 #4 Not tainted
--------------------------------------------------------
swapper/3/0 just changed the state of lock:
ffff000110904cd8 (lock_class){-...}-{2:2}, at: handle_fasteoi_ack_irq+0x2c/0x1b0
but this lock took another, HARDIRQ-unsafe lock in the past:
 (&txgpio->lock){+.+.}-{2:2}


and interrupts could create inverse lock ordering between them.


other info that might help us debug this:
 Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&txgpio->lock);
                               local_irq_disable();
                               lock(lock_class);
                               lock(&txgpio->lock);
  <Interrupt>
    lock(lock_class);

 *** DEADLOCK ***

==========================================================

Thanks,
Piyush
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > ---

Re: [PATCH 1/5] gpio: thunderx: avoid potential deadlock

[Bartosz Golaszewski]

On Wed, May 25, 2022 at 3:17 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:
>
> On Mon, May 02, 2022 at 01:18:49PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> > >
> > > Using irqsave/irqrestore locking variants to avoid any deadlock.
> > >
> >
> > I see you'll be resending this anyway so would you mind providing an
> > example of a deadlock that is possible with no-irqsave variants?
> > Thanks.
> >
> > Bart
> >
> Hi Bartosz,
>
> Thanks for the review.
>
> Please find below the issue scenario:
> In the case when HARDIRQ-safe -> HARDIRQ-unsafe lock order is detected
> and interrupt occurs, deadlock could occur.
>
> ========================================================
> WARNING: possible irq lock inversion dependency detected
> 5.18.0-rc6 #4 Not tainted
> --------------------------------------------------------
> swapper/3/0 just changed the state of lock:
> ffff000110904cd8 (lock_class){-...}-{2:2}, at: handle_fasteoi_ack_irq+0x2c/0x1b0
> but this lock took another, HARDIRQ-unsafe lock in the past:
>  (&txgpio->lock){+.+.}-{2:2}
>
>
> and interrupts could create inverse lock ordering between them.
>
>
> other info that might help us debug this:
>  Possible interrupt unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(&txgpio->lock);
>                                local_irq_disable();
>                                lock(lock_class);
>                                lock(&txgpio->lock);
>   <Interrupt>
>     lock(lock_class);
>
>  *** DEADLOCK ***
>
> ==========================================================
>
> Thanks,
> Piyush
> > > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > > ---

Thanks. What I meant exactly was: resend it with that info in the
commit message.

Bart

Re: [PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Piyush Malgujar]

Hi Linus,

Thanks for reviewing.

On Mon, May 02, 2022 at 12:15:34AM +0200, Linus Walleij wrote:
> On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> 
> > Add support for pin-cfg to configure GPIO Pins
> >
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
> 
> Would be nice to rewrite this binding in YAML
> 

Sure, will take care in V2.

> >    - First cell is the GPIO pin number relative to the controller.
> >    - Second cell is triggering flags as defined in interrupts.txt.
> > +- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
> > +  - First cell is the GPIO pin number
> > +  - Second cell is a value written to GPIO_BIT_CFG register at driver probe.
> 
> Just poking magic hex values into some random register as
> part of a binding is not a good idea.
> 
> This looks like trying to reinvent the pin config subsystem.
> 
> GPIO is using the standard pin configurations in the second cell of
> the handle, use them in this driver as well and add new ones if we
> need.
> 
> You find the existing flags here:
> include/dt-bindings/gpio/gpio.h
> 
> If you need something more sophisticated than a simple flag, I think
> you need to implement proper pin config.
> 
> Yours,
> Linus Walleij

The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
pin function, selecting which signal is reported to GPIO output or which signal GPIO
input need to connect, filters, XOR and output mode.
We will define new entry specific to thunderx GPIO usage, instead of pin-cfg.

Thanks,
Piyush

Re: [PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Linus Walleij]

On Fri, Jun 3, 2022 at 11:06 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:

> The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
> It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
> pin function, selecting which signal is reported to GPIO output or which signal GPIO
> input need to connect, filters, XOR and output mode.

Then implement pin control for this driver instead of inventing a custom hack?
https://docs.kernel.org/driver-api/pin-control.html

Several drivers implement pin control with a GPIO front-end, for example:
drivers/gpio/gpio-pl061.c is used as a front end with
drivers/pinctrl/pinctrl-single.c

There are also composite drivers in drivers/pinctrl that implement both
pincontrol (incl muxing) and GPIO, such as drivers/pinctrl/pinctrl-sx150x.c

Yours,
Linus Walleij

Re: [PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Piyush Malgujar]

On Fri, Jun 03, 2022 at 12:35:57PM +0200, Linus Walleij wrote:
> On Fri, Jun 3, 2022 at 11:06 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> 
> > The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
> > It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
> > pin function, selecting which signal is reported to GPIO output or which signal GPIO
> > input need to connect, filters, XOR and output mode.
> 
> Then implement pin control for this driver instead of inventing a custom hack?
> https://docs.kernel.org/driver-api/pin-control.html
> 
> Several drivers implement pin control with a GPIO front-end, for example:
> drivers/gpio/gpio-pl061.c is used as a front end with
> drivers/pinctrl/pinctrl-single.c
> 
> There are also composite drivers in drivers/pinctrl that implement both
> pincontrol (incl muxing) and GPIO, such as drivers/pinctrl/pinctrl-sx150x.c
> 
> Yours,
> Linus Walleij

Hi Linus,

Thanks for the reply.
But as in this case, we expect a 32 bit reg value via DTS for this driver
only from user with internal understanding of marvell soc and this reg bit
value can have many different combinations as the register fields can vary
for different marvell SoCs.
This patch just reads the reg value from DTS and writes it to the register.

Thanks,
Piyush

Re: [PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Linus Walleij]

On Mon, Jun 13, 2022 at 10:04 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:

> Thanks for the reply.
> But as in this case, we expect a 32 bit reg value via DTS for this driver
> only from user with internal understanding of marvell soc and this reg bit
> value can have many different combinations as the register fields can vary
> for different marvell SoCs.
> This patch just reads the reg value from DTS and writes it to the register.

I understand that this is convenient but it does not use the right kernel
abstractions and it does not use device tree bindings the right way
either.

Rewrite the patches using definitions and fine control and move away
from magic numbers to be poked into registers.

Yours,
Linus Walleij

Re: [PATCH 2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

[Krzysztof Kozlowski]

On 26/06/2022 00:59, Linus Walleij wrote:
> On Mon, Jun 13, 2022 at 10:04 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> 
>> Thanks for the reply.
>> But as in this case, we expect a 32 bit reg value via DTS for this driver
>> only from user with internal understanding of marvell soc and this reg bit
>> value can have many different combinations as the register fields can vary
>> for different marvell SoCs.
>> This patch just reads the reg value from DTS and writes it to the register.
> 
> I understand that this is convenient but it does not use the right kernel
> abstractions and it does not use device tree bindings the right way
> either.
> 
> Rewrite the patches using definitions and fine control and move away
> from magic numbers to be poked into registers.

+1

Let's don't repeat the same pattern Samsung pinctrl has.

Best regards,
Krzysztof