[PATCH 0/3] gpio: bcm-kona: Various GPIO fixups

[PATCH 0/3] gpio: bcm-kona: Various GPIO fixups

[Artur Weber]

Fixes two issues that were preventing GPIO from working correctly:

- Lock/unlock functions tried to write the wrong bit to the unlock
  registers for GPIOs with numbers larger than 32

- GPIOs only initialized as IRQs did not unlock the configuration
  registers, causing IRQ-related configuration (e.g. setting the IRQ
  type) to fail.

Also includes a minor fix to add a missing newline to an error message.

Tested on a Samsung Galaxy Grand Neo (baffinlite rev02) with a BCM23550
(DTS not yet in mainline).

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
Artur Weber (3):
      gpio: bcm-kona: Fix GPIO lock/unlock for banks above bank 0
      gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ
      gpio: bcm-kona: Add missing newline to dev_err format string

 drivers/gpio/gpio-bcm-kona.c | 73 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 13 deletions(-)
---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250130-kona-gpio-fixes-6e74595e69f2

Best regards,
-- 
Artur Weber <aweber.kernel@gmail.com>

[PATCH 1/3] gpio: bcm-kona: Fix GPIO lock/unlock for banks above bank 0

[Artur Weber]

The GPIO lock/unlock functions clear/write a bit to the relevant
register for each bank. However, due to an oversight the bit that
was being written was based on the total GPIO number, not the index
of the GPIO within the relevant bank, causing it to fail for any
GPIO above 32 (thus any GPIO for banks above bank 0).

Fix lock/unlock for these banks by using the correct bit.

Fixes: bdb93c03c550 ("gpio: bcm281xx: Centralize register locking")
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 drivers/gpio/gpio-bcm-kona.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
index 5321ef98f4427d004e62f71d00df6d49bb465ddd..77bd4ec93a231472d7bc40db9d5db12d20bb1611 100644
--- a/drivers/gpio/gpio-bcm-kona.c
+++ b/drivers/gpio/gpio-bcm-kona.c
@@ -86,11 +86,12 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
 	u32 val;
 	unsigned long flags;
 	int bank_id = GPIO_BANK(gpio);
+	int bit = GPIO_BIT(gpio);
 
 	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
 
 	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
-	val |= BIT(gpio);
+	val |= BIT(bit);
 	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
 
 	raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
@@ -102,11 +103,12 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
 	u32 val;
 	unsigned long flags;
 	int bank_id = GPIO_BANK(gpio);
+	int bit = GPIO_BIT(gpio);
 
 	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
 
 	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
-	val &= ~BIT(gpio);
+	val &= ~BIT(bit);
 	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
 
 	raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);

-- 
2.48.1

Re: [PATCH 1/3] gpio: bcm-kona: Fix GPIO lock/unlock for banks above bank 0

[Florian Fainelli]

On 1/30/25 09:10, Artur Weber wrote:
> The GPIO lock/unlock functions clear/write a bit to the relevant
> register for each bank. However, due to an oversight the bit that
> was being written was based on the total GPIO number, not the index
> of the GPIO within the relevant bank, causing it to fail for any
> GPIO above 32 (thus any GPIO for banks above bank 0).
> 
> Fix lock/unlock for these banks by using the correct bit.
> 
> Fixes: bdb93c03c550 ("gpio: bcm281xx: Centralize register locking")
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH 1/3] gpio: bcm-kona: Fix GPIO lock/unlock for banks above bank 0

[Markus Mayer]

On Thu, 30 Jan 2025 at 13:33, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 1/30/25 09:10, Artur Weber wrote:
> > The GPIO lock/unlock functions clear/write a bit to the relevant
> > register for each bank. However, due to an oversight the bit that
> > was being written was based on the total GPIO number, not the index
> > of the GPIO within the relevant bank, causing it to fail for any
> > GPIO above 32 (thus any GPIO for banks above bank 0).
> >
> > Fix lock/unlock for these banks by using the correct bit.
> >
> > Fixes: bdb93c03c550 ("gpio: bcm281xx: Centralize register locking")
> > Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Wow. That recipient list brings back memories.

Also:

Reviewed-by: Markus Mayer <mmayer@broadcom.com>

[PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Artur Weber]

The settings for all GPIOs are locked by default in bcm_kona_gpio_reset.
The settings for a GPIO are unlocked when requesting it as a GPIO, but
not when requesting it as an interrupt, causing the IRQ settings to not
get applied.

Fix this by making sure to unlock the right bits when an IRQ is requested.
To avoid a situation where an IRQ being released causes a lock despite
the same GPIO being used by a GPIO request or vice versa, add an unlock
counter and only lock if it reaches 0.

Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 drivers/gpio/gpio-bcm-kona.c | 69 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
index 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644
--- a/drivers/gpio/gpio-bcm-kona.c
+++ b/drivers/gpio/gpio-bcm-kona.c
@@ -69,6 +69,22 @@ struct bcm_kona_gpio {
 struct bcm_kona_gpio_bank {
 	int id;
 	int irq;
+	/*
+	 * Used to keep track of lock/unlock operations for each GPIO in the
+	 * bank.
+	 *
+	 * All GPIOs are locked by default (see bcm_kona_gpio_reset), and the
+	 * unlock count for all GPIOs is 0 by default. Each unlock increments
+	 * the counter, and each lock decrements the counter.
+	 *
+	 * The lock function only locks the GPIO once its unlock counter is
+	 * down to 0. This is necessary because the GPIO is unlocked in two
+	 * places in this driver: once for requested GPIOs, and once for
+	 * requested IRQs. Since it is possible for a GPIO to be requested
+	 * as both a GPIO and an IRQ, we need to ensure that we don't lock it
+	 * too early.
+	 */
+	u8 gpio_unlock_count[GPIO_PER_BANK];
 	/* Used in the interrupt handler */
 	struct bcm_kona_gpio *kona_gpio;
 };
@@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
 	unsigned long flags;
 	int bank_id = GPIO_BANK(gpio);
 	int bit = GPIO_BIT(gpio);
+	struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
 
-	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
+	if (bank->gpio_unlock_count[bit] == 0) {
+		dev_err(kona_gpio->gpio_chip.parent,
+			"Unbalanced locks for GPIO %u\n", gpio);
+		return;
+	}
 
-	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
-	val |= BIT(bit);
-	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
+	bank->gpio_unlock_count[bit] -= 1;
 
-	raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
+	if (bank->gpio_unlock_count[bit] == 0) {
+		raw_spin_lock_irqsave(&kona_gpio->lock, flags);
+
+		val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
+		val |= BIT(bit);
+		bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
+
+		raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
+	}
 }
 
 static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
@@ -104,14 +131,20 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
 	unsigned long flags;
 	int bank_id = GPIO_BANK(gpio);
 	int bit = GPIO_BIT(gpio);
+	struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
 
-	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
+	if (bank->gpio_unlock_count[bit] == 0) {
+		raw_spin_lock_irqsave(&kona_gpio->lock, flags);
 
-	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
-	val &= ~BIT(bit);
-	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
+		val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
+		val &= ~BIT(bit);
+		bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
 
-	raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
+
+		raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
+	}
+
+	bank->gpio_unlock_count[bit] += 1;
 }
 
 static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio)
@@ -362,6 +395,7 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d)
 
 	kona_gpio = irq_data_get_irq_chip_data(d);
 	reg_base = kona_gpio->reg_base;
+
 	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
 
 	val = readl(reg_base + GPIO_INT_MASK(bank_id));
@@ -384,6 +418,7 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d)
 
 	kona_gpio = irq_data_get_irq_chip_data(d);
 	reg_base = kona_gpio->reg_base;
+
 	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
 
 	val = readl(reg_base + GPIO_INT_MSKCLR(bank_id));
@@ -479,15 +514,25 @@ static void bcm_kona_gpio_irq_handler(struct irq_desc *desc)
 static int bcm_kona_gpio_irq_reqres(struct irq_data *d)
 {
 	struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = d->hwirq;
 
-	return gpiochip_reqres_irq(&kona_gpio->gpio_chip, d->hwirq);
+	/*
+	 * We need to unlock the GPIO before any other operations are performed
+	 * on the relevant GPIO configuration registers
+	 */
+	bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
+
+	return gpiochip_reqres_irq(&kona_gpio->gpio_chip, gpio);
 }
 
 static void bcm_kona_gpio_irq_relres(struct irq_data *d)
 {
 	struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = d->hwirq;
+
+	bcm_kona_gpio_lock_gpio(kona_gpio, gpio);
 
-	gpiochip_relres_irq(&kona_gpio->gpio_chip, d->hwirq);
+	gpiochip_relres_irq(&kona_gpio->gpio_chip, gpio);
 }
 
 static struct irq_chip bcm_gpio_irq_chip = {

-- 
2.48.1

Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Florian Fainelli]

On 1/30/25 09:10, Artur Weber wrote:
> The settings for all GPIOs are locked by default in bcm_kona_gpio_reset.
> The settings for a GPIO are unlocked when requesting it as a GPIO, but
> not when requesting it as an interrupt, causing the IRQ settings to not
> get applied.
> 
> Fix this by making sure to unlock the right bits when an IRQ is requested.
> To avoid a situation where an IRQ being released causes a lock despite
> the same GPIO being used by a GPIO request or vice versa, add an unlock
> counter and only lock if it reaches 0.
> 
> Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> ---
>   drivers/gpio/gpio-bcm-kona.c | 69 ++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 57 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
> index 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644
> --- a/drivers/gpio/gpio-bcm-kona.c
> +++ b/drivers/gpio/gpio-bcm-kona.c
> @@ -69,6 +69,22 @@ struct bcm_kona_gpio {
>   struct bcm_kona_gpio_bank {
>   	int id;
>   	int irq;
> +	/*
> +	 * Used to keep track of lock/unlock operations for each GPIO in the
> +	 * bank.
> +	 *
> +	 * All GPIOs are locked by default (see bcm_kona_gpio_reset), and the
> +	 * unlock count for all GPIOs is 0 by default. Each unlock increments
> +	 * the counter, and each lock decrements the counter.
> +	 *
> +	 * The lock function only locks the GPIO once its unlock counter is
> +	 * down to 0. This is necessary because the GPIO is unlocked in two
> +	 * places in this driver: once for requested GPIOs, and once for
> +	 * requested IRQs. Since it is possible for a GPIO to be requested
> +	 * as both a GPIO and an IRQ, we need to ensure that we don't lock it
> +	 * too early.
> +	 */
> +	u8 gpio_unlock_count[GPIO_PER_BANK];
>   	/* Used in the interrupt handler */
>   	struct bcm_kona_gpio *kona_gpio;
>   };
> @@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
>   	unsigned long flags;
>   	int bank_id = GPIO_BANK(gpio);
>   	int bit = GPIO_BIT(gpio);
> +	struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>   
> -	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +	if (bank->gpio_unlock_count[bit] == 0) {
> +		dev_err(kona_gpio->gpio_chip.parent,
> +			"Unbalanced locks for GPIO %u\n", gpio);
> +		return;

Don't you want to release &kona_gpio->lock here?

> +	}
>   
> -	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> -	val |= BIT(bit);
> -	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +	bank->gpio_unlock_count[bit] -= 1;
>   
> -	raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +	if (bank->gpio_unlock_count[bit] == 0) {
> +		raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +
> +		val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> +		val |= BIT(bit);
> +		bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +
> +		raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +	}
>   }
>   
>   static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
> @@ -104,14 +131,20 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
>   	unsigned long flags;
>   	int bank_id = GPIO_BANK(gpio);
>   	int bit = GPIO_BIT(gpio);
> +	struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>   
> -	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +	if (bank->gpio_unlock_count[bit] == 0) {
> +		raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>   
> -	val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> -	val &= ~BIT(bit);
> -	bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +		val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> +		val &= ~BIT(bit);
> +		bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
>   
> -	raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +
> +		raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +	}
> +
> +	bank->gpio_unlock_count[bit] += 1;
>   }
>   
>   static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio)
> @@ -362,6 +395,7 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d)
>   
>   	kona_gpio = irq_data_get_irq_chip_data(d);
>   	reg_base = kona_gpio->reg_base;
> +
>   	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>   
>   	val = readl(reg_base + GPIO_INT_MASK(bank_id));
> @@ -384,6 +418,7 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d)
>   
>   	kona_gpio = irq_data_get_irq_chip_data(d);
>   	reg_base = kona_gpio->reg_base;
> +
>   	raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>   
>   	val = readl(reg_base + GPIO_INT_MSKCLR(bank_id));
> @@ -479,15 +514,25 @@ static void bcm_kona_gpio_irq_handler(struct irq_desc *desc)
>   static int bcm_kona_gpio_irq_reqres(struct irq_data *d)
>   {
>   	struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = d->hwirq;
>   
> -	return gpiochip_reqres_irq(&kona_gpio->gpio_chip, d->hwirq);
> +	/*
> +	 * We need to unlock the GPIO before any other operations are performed
> +	 * on the relevant GPIO configuration registers
> +	 */
> +	bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
> +
> +	return gpiochip_reqres_irq(&kona_gpio->gpio_chip, gpio);
>   }
>   
>   static void bcm_kona_gpio_irq_relres(struct irq_data *d)
>   {
>   	struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = d->hwirq;
> +
> +	bcm_kona_gpio_lock_gpio(kona_gpio, gpio);
>   
> -	gpiochip_relres_irq(&kona_gpio->gpio_chip, d->hwirq);
> +	gpiochip_relres_irq(&kona_gpio->gpio_chip, gpio);
>   }
>   
>   static struct irq_chip bcm_gpio_irq_chip = {
> 


-- 
Florian

Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Artur Weber]

On 30.01.2025 22:35, Florian Fainelli wrote:
> On 1/30/25 09:10, Artur Weber wrote:
>> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
>> index 
>> 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644
>> --- a/drivers/gpio/gpio-bcm-kona.c
>> +++ b/drivers/gpio/gpio-bcm-kona.c
>> ...
>> @@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct 
>> bcm_kona_gpio *kona_gpio,
>>       unsigned long flags;
>>       int bank_id = GPIO_BANK(gpio);
>>       int bit = GPIO_BIT(gpio);
>> +    struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>> -    raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>> +    if (bank->gpio_unlock_count[bit] == 0) {
>> +        dev_err(kona_gpio->gpio_chip.parent,
>> +            "Unbalanced locks for GPIO %u\n", gpio);
>> +        return;
> 
> Don't you want to release &kona_gpio->lock here?
> 

If you're talking about the "raw_spin_lock_irqsave" call above the if
condition - note that it has been removed and was actually moved below:

>> +    if (bank->gpio_unlock_count[bit] == 0) {
>> +        raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>> +
>> +        val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
>> +        val |= BIT(bit);
>> +        bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, 
>> val);
>> +
>> +        raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
>> +    }

In general, the diff for this patch did not generate very well, sorry
about that. I'll see if there's anything I can do to improve that in the
future.

Best regards
Artur

Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Florian Fainelli]

On 1/30/25 13:46, Artur Weber wrote:
> On 30.01.2025 22:35, Florian Fainelli wrote:
>> On 1/30/25 09:10, Artur Weber wrote:
>>> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
>>> index 
>>> 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644
>>> --- a/drivers/gpio/gpio-bcm-kona.c
>>> +++ b/drivers/gpio/gpio-bcm-kona.c
>>> ...
>>> @@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct 
>>> bcm_kona_gpio *kona_gpio,
>>>       unsigned long flags;
>>>       int bank_id = GPIO_BANK(gpio);
>>>       int bit = GPIO_BIT(gpio);
>>> +    struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>>> -    raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>>> +    if (bank->gpio_unlock_count[bit] == 0) {
>>> +        dev_err(kona_gpio->gpio_chip.parent,
>>> +            "Unbalanced locks for GPIO %u\n", gpio);
>>> +        return;
>>
>> Don't you want to release &kona_gpio->lock here?
>>
> 
> If you're talking about the "raw_spin_lock_irqsave" call above the if
> condition - note that it has been removed and was actually moved below:

Oh my bad, my eye corrected that - to a + somehow.

> 
>>> +    if (bank->gpio_unlock_count[bit] == 0) {
>>> +        raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>>> +
>>> +        val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
>>> +        val |= BIT(bit);
>>> +        bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, 
>>> val);
>>> +
>>> +        raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
>>> +    }
> 
> In general, the diff for this patch did not generate very well, sorry
> about that. I'll see if there's anything I can do to improve that in the
> future.

Nah that's fine, thanks!
-- 
Florian

Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Florian Fainelli]

On 1/30/25 09:10, Artur Weber wrote:
> The settings for all GPIOs are locked by default in bcm_kona_gpio_reset.
> The settings for a GPIO are unlocked when requesting it as a GPIO, but
> not when requesting it as an interrupt, causing the IRQ settings to not
> get applied.
> 
> Fix this by making sure to unlock the right bits when an IRQ is requested.
> To avoid a situation where an IRQ being released causes a lock despite
> the same GPIO being used by a GPIO request or vice versa, add an unlock
> counter and only lock if it reaches 0.
> 
> Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Markus Mayer]

On Thu, 30 Jan 2025 at 09:10, Artur Weber <aweber.kernel@gmail.com> wrote:

> diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
> index 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644
> --- a/drivers/gpio/gpio-bcm-kona.c
> +++ b/drivers/gpio/gpio-bcm-kona.c
> @@ -69,6 +69,22 @@ struct bcm_kona_gpio {
>  struct bcm_kona_gpio_bank {
>         int id;
>         int irq;
> +       /*
> +        * Used to keep track of lock/unlock operations for each GPIO in the
> +        * bank.
> +        *
> +        * All GPIOs are locked by default (see bcm_kona_gpio_reset), and the
> +        * unlock count for all GPIOs is 0 by default. Each unlock increments
> +        * the counter, and each lock decrements the counter.
> +        *
> +        * The lock function only locks the GPIO once its unlock counter is
> +        * down to 0. This is necessary because the GPIO is unlocked in two
> +        * places in this driver: once for requested GPIOs, and once for
> +        * requested IRQs. Since it is possible for a GPIO to be requested
> +        * as both a GPIO and an IRQ, we need to ensure that we don't lock it
> +        * too early.
> +        */
> +       u8 gpio_unlock_count[GPIO_PER_BANK];
>         /* Used in the interrupt handler */
>         struct bcm_kona_gpio *kona_gpio;
>  };
> @@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio,
>         unsigned long flags;
>         int bank_id = GPIO_BANK(gpio);
>         int bit = GPIO_BIT(gpio);
> +       struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>
> -       raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +       if (bank->gpio_unlock_count[bit] == 0) {
> +               dev_err(kona_gpio->gpio_chip.parent,
> +                       "Unbalanced locks for GPIO %u\n", gpio);
> +               return;
> +       }
>
> -       val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> -       val |= BIT(bit);
> -       bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +       bank->gpio_unlock_count[bit] -= 1;

Not a big deal or a show-stopper, but this could be
          bank->gpio_unlock_count[bit]--;
or, better yet,
          --bank->gpio_unlock_count[bit];

And a bit further down...

> -       raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +       if (bank->gpio_unlock_count[bit] == 0) {
> +               raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +
> +               val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> +               val |= BIT(bit);
> +               bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +
> +               raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +       }
>  }
>
>  static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
> @@ -104,14 +131,20 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio,
>         unsigned long flags;
>         int bank_id = GPIO_BANK(gpio);
>         int bit = GPIO_BIT(gpio);
> +       struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id];
>
> -       raw_spin_lock_irqsave(&kona_gpio->lock, flags);
> +       if (bank->gpio_unlock_count[bit] == 0) {
> +               raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>
> -       val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> -       val &= ~BIT(bit);
> -       bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
> +               val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id));
> +               val &= ~BIT(bit);
> +               bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val);
>
> -       raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +
> +               raw_spin_unlock_irqrestore(&kona_gpio->lock, flags);
> +       }
> +
> +       bank->gpio_unlock_count[bit] += 1;

...this could be
          ++bank->gpio_unlock_count[bit];

>  }
>
>  static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio)
> @@ -362,6 +395,7 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d)
>
>         kona_gpio = irq_data_get_irq_chip_data(d);
>         reg_base = kona_gpio->reg_base;
> +
>         raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>
>         val = readl(reg_base + GPIO_INT_MASK(bank_id));
> @@ -384,6 +418,7 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d)
>
>         kona_gpio = irq_data_get_irq_chip_data(d);
>         reg_base = kona_gpio->reg_base;
> +
>         raw_spin_lock_irqsave(&kona_gpio->lock, flags);
>
>         val = readl(reg_base + GPIO_INT_MSKCLR(bank_id));
> @@ -479,15 +514,25 @@ static void bcm_kona_gpio_irq_handler(struct irq_desc *desc)
>  static int bcm_kona_gpio_irq_reqres(struct irq_data *d)
>  {
>         struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
> +       unsigned int gpio = d->hwirq;
>
> -       return gpiochip_reqres_irq(&kona_gpio->gpio_chip, d->hwirq);
> +       /*
> +        * We need to unlock the GPIO before any other operations are performed
> +        * on the relevant GPIO configuration registers
> +        */
> +       bcm_kona_gpio_unlock_gpio(kona_gpio, gpio);
> +
> +       return gpiochip_reqres_irq(&kona_gpio->gpio_chip, gpio);
>  }
>
>  static void bcm_kona_gpio_irq_relres(struct irq_data *d)
>  {
>         struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d);
> +       unsigned int gpio = d->hwirq;

Since you added a comment to bcm_kona_gpio_irq_reqres(), would it make
sense to add one here too? Just another nitpick and not a big deal
either way.

> +       bcm_kona_gpio_lock_gpio(kona_gpio, gpio);
>
> -       gpiochip_relres_irq(&kona_gpio->gpio_chip, d->hwirq);
> +       gpiochip_relres_irq(&kona_gpio->gpio_chip, gpio);
>  }
>
>  static struct irq_chip bcm_gpio_irq_chip = {
>
> --
> 2.48.1
>

Since I am okay either way with regards to my three nitpicks:

Reviewed-by: Markus Mayer <mmayer@broadcom.com>

[PATCH 3/3] gpio: bcm-kona: Add missing newline to dev_err format string

[Artur Weber]

Add a missing newline to the format string of the "Couldn't get IRQ
for bank..." error message.

Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 drivers/gpio/gpio-bcm-kona.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c
index eeaa921df6f072129dbdf1c73d6da2bd7c1fe716..724db3434d277c5f9ea36b0d050e34c451787e4a 100644
--- a/drivers/gpio/gpio-bcm-kona.c
+++ b/drivers/gpio/gpio-bcm-kona.c
@@ -661,7 +661,7 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev)
 		bank->irq = platform_get_irq(pdev, i);
 		bank->kona_gpio = kona_gpio;
 		if (bank->irq < 0) {
-			dev_err(dev, "Couldn't get IRQ for bank %d", i);
+			dev_err(dev, "Couldn't get IRQ for bank %d\n", i);
 			ret = -ENOENT;
 			goto err_irq_domain;
 		}

-- 
2.48.1

Re: [PATCH 3/3] gpio: bcm-kona: Add missing newline to dev_err format string

[Florian Fainelli]

On 1/30/25 09:10, Artur Weber wrote:
> Add a missing newline to the format string of the "Couldn't get IRQ
> for bank..." error message.
> 
> Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH 3/3] gpio: bcm-kona: Add missing newline to dev_err format string

[Markus Mayer]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On Thu, 30 Jan 2025 at 10:41, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 1/30/25 09:10, Artur Weber wrote:
> > Add a missing newline to the format string of the "Couldn't get IRQ
> > for bank..." error message.
> >
> > Fixes: 757651e3d60e ("gpio: bcm281xx: Add GPIO driver")
> > Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Markus Mayer <mmayer@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH 0/3] gpio: bcm-kona: Various GPIO fixups

[Linus Walleij]

Hi Artur,

On Thu, Jan 30, 2025 at 6:10 PM Artur Weber <aweber.kernel@gmail.com> wrote:

> Fixes two issues that were preventing GPIO from working correctly:
>
> - Lock/unlock functions tried to write the wrong bit to the unlock
>   registers for GPIOs with numbers larger than 32
>
> - GPIOs only initialized as IRQs did not unlock the configuration
>   registers, causing IRQ-related configuration (e.g. setting the IRQ
>   type) to fail.
>
> Also includes a minor fix to add a missing newline to an error message.
>
> Tested on a Samsung Galaxy Grand Neo (baffinlite rev02) with a BCM23550
> (DTS not yet in mainline).
>
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Looks good to me, do you want to resend the patches picking up the
Acks or do you want me to apply as-is?

Should this go in as urgent fixes (-rcN) or as nonurgent?

The DTS not being mainline suggests nonurgent but are there
other systems suffering?

Yours,
Linus Walleij

Re: [PATCH 0/3] gpio: bcm-kona: Various GPIO fixups

[Linus Walleij]

On Thu, Feb 6, 2025 at 10:34 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jan 30, 2025 at 6:10 PM Artur Weber <aweber.kernel@gmail.com> wrote:
>
> > Fixes two issues that were preventing GPIO from working correctly:
> >
> > - Lock/unlock functions tried to write the wrong bit to the unlock
> >   registers for GPIOs with numbers larger than 32
> >
> > - GPIOs only initialized as IRQs did not unlock the configuration
> >   registers, causing IRQ-related configuration (e.g. setting the IRQ
> >   type) to fail.
> >
> > Also includes a minor fix to add a missing newline to an error message.
> >
> > Tested on a Samsung Galaxy Grand Neo (baffinlite rev02) with a BCM23550
> > (DTS not yet in mainline).
> >
> > Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
>
> Looks good to me, do you want to resend the patches picking up the
> Acks or do you want me to apply as-is?

Actually it's Bartosz applying, heh confused this for pinctrl.

But most comments go anyway I think.

Yours,
Linus Walleij

Re: [PATCH 0/3] gpio: bcm-kona: Various GPIO fixups

[Artur Weber]

On 6.02.2025 10:34, Linus Walleij wrote:
> Hi Artur,
> 
> On Thu, Jan 30, 2025 at 6:10 PM Artur Weber <aweber.kernel@gmail.com> wrote:
> 
>> Fixes two issues that were preventing GPIO from working correctly:
>>
>> - Lock/unlock functions tried to write the wrong bit to the unlock
>>    registers for GPIOs with numbers larger than 32
>>
>> - GPIOs only initialized as IRQs did not unlock the configuration
>>    registers, causing IRQ-related configuration (e.g. setting the IRQ
>>    type) to fail.
>>
>> Also includes a minor fix to add a missing newline to an error message.
>>
>> Tested on a Samsung Galaxy Grand Neo (baffinlite rev02) with a BCM23550
>> (DTS not yet in mainline).
>>
>> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> 
> Looks good to me, do you want to resend the patches picking up the
> Acks or do you want me to apply as-is?

I'll send a v2 in a second with the style fixes suggested by Markus[1].
After that, it should be good to go.

> Should this go in as urgent fixes (-rcN) or as nonurgent?
> 
> The DTS not being mainline suggests nonurgent but are there
> other systems suffering?

Non-urgent is fine. AFAICT the only upstream-supported Broadcom Kona
devices at the moment are old Broadcom dev boards; while they
technically are also affected by the bug, I doubt anyone's actively
using them. As for the devices I'm testing with, there's still quite
a few fixups I plan to make before I consider them ready to upstream.

Best regards
Artur

[1] https://lore.kernel.org/lkml/CAGt4E5sqd_Aojk+boD5K5EiRfOsiU+jYY5EV0DP6TFut291HnQ@mail.gmail.com/

Re: [PATCH 0/3] gpio: bcm-kona: Various GPIO fixups

[Artur Weber]

On 6.02.2025 16:37, Artur Weber wrote:
> On 6.02.2025 10:34, Linus Walleij wrote:
>> Hi Artur,
>>
>> On Thu, Jan 30, 2025 at 6:10 PM Artur Weber <aweber.kernel@gmail.com> 
>> wrote:
>>
>>> Fixes two issues that were preventing GPIO from working correctly:
>>>
>>> - Lock/unlock functions tried to write the wrong bit to the unlock
>>>    registers for GPIOs with numbers larger than 32
>>>
>>> - GPIOs only initialized as IRQs did not unlock the configuration
>>>    registers, causing IRQ-related configuration (e.g. setting the IRQ
>>>    type) to fail.
>>>
>>> Also includes a minor fix to add a missing newline to an error message.
>>>
>>> Tested on a Samsung Galaxy Grand Neo (baffinlite rev02) with a BCM23550
>>> (DTS not yet in mainline).
>>>
>>> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
>>
>> Looks good to me, do you want to resend the patches picking up the
>> Acks or do you want me to apply as-is?
> 
> I'll send a v2 in a second with the style fixes suggested by Markus[1].
> After that, it should be good to go.
> 
>> Should this go in as urgent fixes (-rcN) or as nonurgent?
>>
>> The DTS not being mainline suggests nonurgent but are there
>> other systems suffering?
> 
> Non-urgent is fine. AFAICT the only upstream-supported Broadcom Kona
> devices at the moment are old Broadcom dev boards; while they
> technically are also affected by the bug, I doubt anyone's actively
> using them. As for the devices I'm testing with, there's still quite
> a few fixups I plan to make before I consider them ready to upstream.
> 
> Best regards
> Artur
> 
> [1] https://lore.kernel.org/lkml/ 
> CAGt4E5sqd_Aojk+boD5K5EiRfOsiU+jYY5EV0DP6TFut291HnQ@mail.gmail.com/

v2 sent: https://lore.kernel.org/lkml/20250206-kona-gpio-fixes-v2-0-409135eab780@gmail.com/T/#t

Best regards
Artur