Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 19 Feb 2018 10:03:27 -0800
In-Reply-To: <83d6bd0da9254d868d3f713bd3bc282c@www.loen.fr>
References: <20170301183257.6554-1-f.fainelli@gmail.com> <83d6bd0da9254d868d3f713bd3bc282c@www.loen.fr>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH fixes v3] pinctrl: Really force states during  suspend/resume
To:     Marc Zyngier <maz@misterjones.org>
CC:     linux-kernel@vger.kernel.org, linus.walleij@linaro.org,
        swarren@nvidia.com, andy.shevchenko@gmail.com, alcooperx@gmail.com,
        linux-gpio@vger.kernel.org, Heiko Stuebner <heiko@sntech.de>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <913ED32F-36F8-4F31-9221-263DD5599FB2@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On February 19, 2018 9:25:26 AM PST, Marc Zyngier <maz@misterjones=2Eorg> w=
rote:
>Hi all,
>
>On 2017-03-01 18:32, Florian Fainelli wrote:
>> In case a platform only defaults a "default" set of pins, but not a
>> "sleep" set of pins, and this particular platform suspends and=20
>> resumes
>> in a way that the pin states are not preserved by the hardware, when=20
>> we
>> resume, we would call pinctrl_single_resume() ->=20
>> pinctrl_force_default()
>> -> pinctrl_select_state() and the first thing we do is check that the
>> pins state is the same as before, and do nothing=2E
>>
>> In order to fix this, decouple the actual state change from
>> pinctrl_select_state() and move it pinctrl_commit_state(), while=20
>> keeping
>> the p->state =3D=3D state check in pinctrl_select_state() not to change=
=20
>> the
>> caller assumptions=2E pinctrl_force_sleep() and pinctrl_force_default()
>> are updated to bypass the state check by calling=20
>> pinctrl_commit_state()=2E
>>
>> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states
>> per device")
>> Signed-off-by: Florian Fainelli <f=2Efainelli@gmail=2Ecom>
>> ---
>> Changes in v3:
>>
>> - move the state check to pinctrl_select_state
>>
>> Changes in v2:
>>
>> - rename __pinctrl_select_state to pinctrl_commit_state
>>
>>  drivers/pinctrl/core=2Ec | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core=2Ec b/drivers/pinctrl/core=2Ec
>> index fb38e208f32d=2E=2E735d8f7f9d71 100644
>> --- a/drivers/pinctrl/core=2Ec
>> +++ b/drivers/pinctrl/core=2Ec
>> @@ -992,19 +992,16 @@ struct pinctrl_state
>> *pinctrl_lookup_state(struct pinctrl *p,
>>  EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>
>>  /**
>> - * pinctrl_select_state() - select/activate/program a pinctrl state=20
>> to HW
>> + * pinctrl_commit_state() - select/activate/program a pinctrl state=20
>> to HW
>>   * @p: the pinctrl handle for the device that requests configuration
>>   * @state: the state handle to select/activate/program
>>   */
>> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state=20
>> *state)
>> +static int pinctrl_commit_state(struct pinctrl *p, struct
>> pinctrl_state *state)
>>  {
>>  	struct pinctrl_setting *setting, *setting2;
>>  	struct pinctrl_state *old_state =3D p->state;
>>  	int ret;
>>
>> -	if (p->state =3D=3D state)
>> -		return 0;
>> -
>>  	if (p->state) {
>>  		/*
>>  		 * For each pinmux setting in the old state, forget SW's record
>> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p,
>> struct pinctrl_state *state)
>>
>>  	return ret;
>>  }
>> +
>> +/**
>> + * pinctrl_select_state() - select/activate/program a pinctrl state=20
>> to HW
>> + * @p: the pinctrl handle for the device that requests configuration
>> + * @state: the state handle to select/activate/program
>> + */
>> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state=20
>> *state)
>> +{
>> +	if (p->state =3D=3D state)
>> +		return 0;
>> +
>> +	return pinctrl_commit_state(p, state);
>> +}
>>  EXPORT_SYMBOL_GPL(pinctrl_select_state);
>>
>>  static void devm_pinctrl_release(struct device *dev, void *res)
>> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map
>> const *map)
>>  int pinctrl_force_sleep(struct pinctrl_dev *pctldev)
>>  {
>>  	if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep))
>> -		return pinctrl_select_state(pctldev->p, pctldev->hog_sleep);
>> +		return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep);
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep);
>>  int pinctrl_force_default(struct pinctrl_dev *pctldev)
>>  {
>>  	if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default))
>> -		return pinctrl_select_state(pctldev->p, pctldev->hog_default);
>> +		return pinctrl_commit_state(pctldev->p, pctldev->hog_default);
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(pinctrl_force_default);

Hey Marc,

>
>I don't often go back over a year worth of LKML, but since this patch=20
>recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an=20
>anchor to report the following:
>
>It turns out that this patch completely breaks resume on my=20
>rk3399-based Chromebook=2E Most things are timing out, the box is=20
>unusable=2E And since this is my everyday tool, I'm mildly grumpy=2E Plea=
se
>
>don't break my toys! ;-) Reverting this patch on top of 4=2E16-rc2 makes=
=20
>me productive again=2E=2E=2E
>
>More seriously, I have no idea what's wrong here=2E It could be a=20
>SoC-related issue, hence Heiko on Cc=2E I'm happy to test any idea you=20
>could have=2E

Can you indicate which DTS file is used for your Chromebook model? Sorry a=
bout the breakage=2E

--=20
Florian