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

[Mario Limonciello <mario.limonciello@amd.com>]

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