[PATCH 0/3] Fixes and updates to pinctrl-amd

[PATCH 0/3] Fixes and updates to pinctrl-amd

[Basavaraj Natikar]

Changes include use of devm_platform_get_and_ioremap_resource,
save/restore pending irq bits and removing of
contact information.

Basavaraj Natikar (3):
  pinctrl: amd: Use devm_platform_get_and_ioremap_resource
  pinctrl: amd: Don't save/restore interrupt status and wake status bits
  pinctrl: amd: Remove contact information

 drivers/pinctrl/pinctrl-amd.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/3] pinctrl: amd: Use devm_platform_get_and_ioremap_resource

[Basavaraj Natikar]

Use devm_platform_get_and_ioremap_resource() to simplify code.

Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 0645c2c24f50..ff3d0edbea48 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -977,17 +977,12 @@ static int amd_gpio_probe(struct platform_device *pdev)
 
 	raw_spin_lock_init(&gpio_dev->lock);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
+	gpio_dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(gpio_dev->base)) {
 		dev_err(&pdev->dev, "Failed to get gpio io resource.\n");
-		return -EINVAL;
+		return PTR_ERR(gpio_dev->base);
 	}
 
-	gpio_dev->base = devm_ioremap(&pdev->dev, res->start,
-						resource_size(res));
-	if (!gpio_dev->base)
-		return -ENOMEM;
-
 	gpio_dev->irq = platform_get_irq(pdev, 0);
 	if (gpio_dev->irq < 0)
 		return gpio_dev->irq;
-- 
2.25.1

[PATCH 2/3] pinctrl: amd: Don't save/restore interrupt status and wake status bits

[Basavaraj Natikar]

Saving/restoring interrupt and wake status bits across suspend can
cause the suspend to fail if an IRQ is serviced across the
suspend cycle.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index ff3d0edbea48..40e23b5795b0 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -917,6 +917,7 @@ static int amd_gpio_suspend(struct device *dev)
 {
 	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
 	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
+	unsigned long flags;
 	int i;
 
 	for (i = 0; i < desc->npins; i++) {
@@ -925,7 +926,9 @@ static int amd_gpio_suspend(struct device *dev)
 		if (!amd_gpio_should_save(gpio_dev, pin))
 			continue;
 
-		gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin*4);
+		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+		gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin * 4) & ~PIN_IRQ_PENDING;
+		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 	}
 
 	return 0;
@@ -935,6 +938,7 @@ static int amd_gpio_resume(struct device *dev)
 {
 	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
 	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
+	unsigned long flags;
 	int i;
 
 	for (i = 0; i < desc->npins; i++) {
@@ -943,7 +947,10 @@ static int amd_gpio_resume(struct device *dev)
 		if (!amd_gpio_should_save(gpio_dev, pin))
 			continue;
 
-		writel(gpio_dev->saved_regs[i], gpio_dev->base + pin*4);
+		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+		gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4) & PIN_IRQ_PENDING;
+		writel(gpio_dev->saved_regs[i], gpio_dev->base + pin * 4);
+		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 	}
 
 	return 0;
-- 
2.25.1

RE: [PATCH 2/3] pinctrl: amd: Don't save/restore interrupt status and wake status bits

[Limonciello, Mario]

[Public]

> -----Original Message-----
> From: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
> Sent: Monday, June 13, 2022 01:41
> To: linus.walleij@linaro.org; linux-gpio@vger.kernel.org; S-k, Shyam-sundar
> <Shyam-sundar.S-k@amd.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Natikar, Basavaraj
> <Basavaraj.Natikar@amd.com>
> Subject: [PATCH 2/3] pinctrl: amd: Don't save/restore interrupt status and wake
> status bits
> 
> Saving/restoring interrupt and wake status bits across suspend can
> cause the suspend to fail if an IRQ is serviced across the
> suspend cycle.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

I think this should also pick up this tag:
Fixes: 79d2c8bede2c ("pinctrl/amd: save pin registers over suspend/resume")

> ---
>  drivers/pinctrl/pinctrl-amd.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index ff3d0edbea48..40e23b5795b0 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -917,6 +917,7 @@ static int amd_gpio_suspend(struct device *dev)
>  {
>  	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
>  	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> +	unsigned long flags;
>  	int i;
> 
>  	for (i = 0; i < desc->npins; i++) {
> @@ -925,7 +926,9 @@ static int amd_gpio_suspend(struct device *dev)
>  		if (!amd_gpio_should_save(gpio_dev, pin))
>  			continue;
> 
> -		gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin*4);
> +		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +		gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin * 4) &
> ~PIN_IRQ_PENDING;
> +		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>  	}
> 
>  	return 0;
> @@ -935,6 +938,7 @@ static int amd_gpio_resume(struct device *dev)
>  {
>  	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
>  	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> +	unsigned long flags;
>  	int i;
> 
>  	for (i = 0; i < desc->npins; i++) {
> @@ -943,7 +947,10 @@ static int amd_gpio_resume(struct device *dev)
>  		if (!amd_gpio_should_save(gpio_dev, pin))
>  			continue;
> 
> -		writel(gpio_dev->saved_regs[i], gpio_dev->base + pin*4);
> +		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +		gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4) &
> PIN_IRQ_PENDING;
> +		writel(gpio_dev->saved_regs[i], gpio_dev->base + pin * 4);
> +		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>  	}
> 
>  	return 0;
> --
> 2.25.1

Re: [PATCH 2/3] pinctrl: amd: Don't save/restore interrupt status and wake status bits

[Mario Limonciello]

On 6/13/22 09:17, Limonciello, Mario wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
>> Sent: Monday, June 13, 2022 01:41
>> To: linus.walleij@linaro.org; linux-gpio@vger.kernel.org; S-k, Shyam-sundar
>> <Shyam-sundar.S-k@amd.com>
>> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; Natikar, Basavaraj
>> <Basavaraj.Natikar@amd.com>
>> Subject: [PATCH 2/3] pinctrl: amd: Don't save/restore interrupt status and wake
>> status bits
>>
>> Saving/restoring interrupt and wake status bits across suspend can
>> cause the suspend to fail if an IRQ is serviced across the
>> suspend cycle.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> 
> I think this should also pick up this tag:
> Fixes: 79d2c8bede2c ("pinctrl/amd: save pin registers over suspend/resume")

This was confirmed to fix a bug reported to AMD gitlab with multiple 
suspend/resume cycles.  So one more tag to add:

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1333

> 
>> ---
>>   drivers/pinctrl/pinctrl-amd.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
>> index ff3d0edbea48..40e23b5795b0 100644
>> --- a/drivers/pinctrl/pinctrl-amd.c
>> +++ b/drivers/pinctrl/pinctrl-amd.c
>> @@ -917,6 +917,7 @@ static int amd_gpio_suspend(struct device *dev)
>>   {
>>   	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
>>   	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>> +	unsigned long flags;
>>   	int i;
>>
>>   	for (i = 0; i < desc->npins; i++) {
>> @@ -925,7 +926,9 @@ static int amd_gpio_suspend(struct device *dev)
>>   		if (!amd_gpio_should_save(gpio_dev, pin))
>>   			continue;
>>
>> -		gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin*4);
>> +		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>> +		gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin * 4) &
>> ~PIN_IRQ_PENDING;
>> +		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>   	}
>>
>>   	return 0;
>> @@ -935,6 +938,7 @@ static int amd_gpio_resume(struct device *dev)
>>   {
>>   	struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
>>   	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
>> +	unsigned long flags;
>>   	int i;
>>
>>   	for (i = 0; i < desc->npins; i++) {
>> @@ -943,7 +947,10 @@ static int amd_gpio_resume(struct device *dev)
>>   		if (!amd_gpio_should_save(gpio_dev, pin))
>>   			continue;
>>
>> -		writel(gpio_dev->saved_regs[i], gpio_dev->base + pin*4);
>> +		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
>> +		gpio_dev->saved_regs[i] |= readl(gpio_dev->base + pin * 4) &
>> PIN_IRQ_PENDING;
>> +		writel(gpio_dev->saved_regs[i], gpio_dev->base + pin * 4);
>> +		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>   	}
>>
>>   	return 0;
>> --
>> 2.25.1

[PATCH 3/3] pinctrl: amd: Remove contact information

[Basavaraj Natikar]

Remove contact information.

Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 40e23b5795b0..22f8a615a8b9 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -6,8 +6,6 @@
  * Authors: Ken Xue <Ken.Xue@amd.com>
  *      Wu, Jeff <Jeff.Wu@amd.com>
  *
- * Contact Information: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
- *			Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
  */
 
 #include <linux/err.h>
-- 
2.25.1

Re: [PATCH 0/3] Fixes and updates to pinctrl-amd

[Limonciello, Mario]

On 6/13/2022 01:41, Basavaraj Natikar wrote:
> Changes include use of devm_platform_get_and_ioremap_resource,
> save/restore pending irq bits and removing of
> contact information.
> 
> Basavaraj Natikar (3):
>    pinctrl: amd: Use devm_platform_get_and_ioremap_resource
>    pinctrl: amd: Don't save/restore interrupt status and wake status bits
>    pinctrl: amd: Remove contact information
> 
>   drivers/pinctrl/pinctrl-amd.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 

Linus Walleij,

Just wanted to double check this series wasn't forgotten as you get 5.20 
preparations ready.  I didn't see it in ib-v5.20-amd-pinctrl.

Re: [PATCH 0/3] Fixes and updates to pinctrl-amd

[Linus Walleij]

On Mon, Jun 13, 2022 at 8:43 AM Basavaraj Natikar
<Basavaraj.Natikar@amd.com> wrote:

> Changes include use of devm_platform_get_and_ioremap_resource,
> save/restore pending irq bits and removing of
> contact information.

Patches applied!

Yours,
Linus Walleij