From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyas B Prabhu Subject: Re: [PATCH 3/4] powernv: cpuidle: Redesign idle states management Date: Mon, 17 Nov 2014 11:19:44 +0530 Message-ID: <54698C78.9050005@linux.vnet.ibm.com> References: <1415030910-5799-1-git-send-email-shreyas@linux.vnet.ibm.com> <1415030910-5799-4-git-send-email-shreyas@linux.vnet.ibm.com> <54630362.7050007@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <54630362.7050007@linux.vnet.ibm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" To: Preeti U Murthy , linux-kernel@vger.kernel.org Cc: Paul Mackerras , "Rafael J. Wysocki" , linuxppc-dev@lists.ozlabs.org, linux-pm@vger.kernel.org List-Id: linux-pm@vger.kernel.org SGkgUHJlZXRpLAoKT24gV2VkbmVzZGF5IDEyIE5vdmVtYmVyIDIwMTQgMTI6MjEgUE0sIFByZWV0 aSBVIE11cnRoeSB3cm90ZToKPiBIaSBTaHJleWFzLAo+IAo+IE9uIDExLzAzLzIwMTQgMDk6Mzgg UE0sIFNocmV5YXMgQi4gUHJhYmh1IHdyb3RlOgo+PiBkaWZmIC0tZ2l0IGEvYXJjaC9wb3dlcnBj L2tlcm5lbC9pZGxlX3Bvd2VyNy5TIGIvYXJjaC9wb3dlcnBjL2tlcm5lbC9pZGxlX3Bvd2VyNy5T Cj4+IGluZGV4IDI4M2M2MDMuLmRmMTFhY2IgMTAwNjQ0Cj4+IC0tLSBhL2FyY2gvcG93ZXJwYy9r ZXJuZWwvaWRsZV9wb3dlcjcuUwo+PiArKysgYi9hcmNoL3Bvd2VycGMva2VybmVsL2lkbGVfcG93 ZXI3LlMKPj4gIF9HTE9CQUwocG93ZXI3X2lkbGUpCj4+ICAJLyogTm93IGNoZWNrIGlmIHVzZXIg b3IgYXJjaCBlbmFibGVkIE5BUCBtb2RlICovCj4+IEBAIC0xNDEsNDkgKzE5MiwxNiBAQCBfR0xP QkFMKHBvd2VyN19pZGxlKQo+PiAgCj4+ICBfR0xPQkFMKHBvd2VyN19uYXApCj4+ICAJbXIJcjQs cjMKPj4gLQlsaQlyMywwCj4+ICsJbGkJcjMsMQo+IAo+IFRoZSBjb21tZW50IGF0IHRoZSB0b3Ag b2YgdGhpcyBmaWxlIHN0YXRlcyAwIGZvciBuYXAgYW5kIDEgZm9yIHNsZWVwLgo+IFlvdSB3aWxs IG5lZWQgdG8gY2hhbmdlIHRoYXQuIEFzIGFuIGFsdGVybmF0aXZlIEkgd291bGQgc3VnZ2VzdCB1 c2luZwo+IHRoZSBtYWNyb3MgdGhhdCB5b3UgaGF2ZSBhbHJlYWR5IGRlZmluZWQ6UE5WX1RIUkVB RF9OQVAgYW5kCj4gUE5WX1RIUkVBRF9TTEVFUCB0byB3cml0ZSB0byByMyBhYm92ZSBhbmQgcmVt b3ZlIHRoZSBsaW5lcyB0aGF0IHNheSAwCj4gZm9yIG5hcCBhbmQgMSBmb3Igc2xlZXAgaW4gdGhl IGNvbW1lbnRzLgoKTXkgYmFkLCBJIGhhZCBjaGFuZ2VkIHRoZSBjb21tZW50IGluIHRoZSBuZXh0 IGNvbW1pdC4gSSdsbCBtYWtlIHRoZSBjaGFuZ2UuCj4gCj4+ICAJYglwb3dlcjdfcG93ZXJzYXZl X2NvbW1vbgo+PiAgCS8qIE5vIHJldHVybiAqLwo+PiAgCj4gCj4gPHNuaXA+Cj4gCj4+IEBAIC0y MTAsMTIgKzIyNiw5MSBAQCBfR0xPQkFMKHBvd2VyN193YWtldXBfdGJfbG9zcykKPj4gIEJFR0lO X0ZUUl9TRUNUSU9OCj4+ICAJQ0hFQ0tfSE1JX0lOVEVSUlVQVAo+PiAgRU5EX0ZUUl9TRUNUSU9O X0lGU0VUKENQVV9GVFJfSFZNT0RFKQo+PiArCj4+ICsJbGkJcjcsMQo+PiArCW1mc3ByCXI4LFNQ Uk5fUElSCj4+ICsJLyoKPj4gKwkgKiBUaGUgbGFzdCAzIGJpdHMgb2YgUElSIHJlcHJlc2VudHMg dGhlIHRocmVhZCBpZCBvZiBhIGNwdQo+PiArCSAqIGluIHBvd2VyOC4gVGhpcyB3aWxsIG5lZWQg YWRqdXN0aW5nIGZvciBwb3dlcjcuCj4+ICsJICovCj4+ICsJYW5kaS4JcjgscjgsMHgwNwkJLyog R2V0IHRocmVhZCBpZCBpbnRvIHI4ICovCj4+ICsJcm90bGQJcjcscjcscjgKPj4gKwkvKiByNyBu b3cgaGFzICd0aHJlYWRfaWQndGggYml0IHNldCAqLwo+PiArCj4+ICsJbGQJcjE0LFBBQ0FfQ09S RV9JRExFX1NUQVRFX1BUUihyMTMpCj4+ICtsd2FyeF9sb29wMjoKPj4gKwlsd2FyeAlyMTUsMCxy MTQKPj4gKwlhbmRpLglyOSxyMTUsUE5WX0NPUkVfSURMRV9MT0NLX0JJVAo+PiArCS8qCj4+ICsJ ICogTG9jayBiaXQgaXMgc2V0IGluIG9uZSBvZiB0aGUgMiBjYXNlcy0KPj4gKwkgKiBhLiBJbiB0 aGUgc2xlZXAvd2lua2xlIGVudGVyIHBhdGgsIHRoZSBsYXN0IHRocmVhZCBpcyBleGVjdXRpbmcK Pj4gKwkgKiBmYXN0c2xlZXAgd29ya2Fyb3VuZCBjb2RlLgo+PiArCSAqIGIuIEluIHRoZSB3YWtl IHVwIHBhdGgsIGFub3RoZXIgdGhyZWFkIGlzIGV4ZWN1dGluZyBmYXN0c2xlZXAKPj4gKwkgKiB3 b3JrYXJvdW5kIHVuZG8gY29kZSBvciByZXN5bmNpbmcgdGltZWJhc2Ugb3IgcmVzdG9yaW5nIGNv bnRleHQKPj4gKwkgKiBJbiBlaXRoZXIgY2FzZSBsb29wIHVudGlsIHRoZSBsb2NrIGJpdCBpcyBj bGVhcmVkLgo+PiArCSAqLwo+PiArCWJuZQlsd2FyeF9sb29wMgo+PiArCj4+ICsJY21wd2kJY3Iy LHIxNSwwCj4+ICsJb3IJcjE1LHIxNSxyNwkJLyogU2V0IHRocmVhZCBiaXQgKi8KPj4gKwo+PiAr CWJlcQljcjIsZmlyc3RfdGhyZWFkCj4+ICsKPj4gKwkvKiBOb3QgZmlyc3QgdGhyZWFkIGluIGNv cmUgdG8gd2FrZSB1cCAqLwo+PiArCXN0d2N4LglyMTUsMCxyMTQKPj4gKwlibmUtCWx3YXJ4X2xv b3AyCj4+ICsJYgljb21tb25fZXhpdAo+PiArCj4+ICtmaXJzdF90aHJlYWQ6Cj4+ICsJLyogRmly c3QgdGhyZWFkIGluIGNvcmUgdG8gd2FrZXVwICovCj4+ICsJb3JpCXIxNSxyMTUsUE5WX0NPUkVf SURMRV9MT0NLX0JJVAo+PiArCXN0d2N4LglyMTUsMCxyMTQKPj4gKwlibmUtCWx3YXJ4X2xvb3Ay Cj4+ICsKPj4gKwlMT0FEX1JFR19BRERSKHIzLCBwbnZfbmVlZF9mYXN0c2xlZXBfd29ya2Fyb3Vu ZCkKPj4gKwlsYnoJcjMsMChyMykKPj4gKwljbXB3aQlyMywxCj4+ICsJLyogIHNraXAgZmFzdHNs ZWVwIHdvcmthcm91bmQgaWYgaXRzIG5vdCBuZWVkZWQgKi8KPj4gKwlibmUJdGltZWJhc2VfcmVz eW5jCj4+ICsKPj4gKwkvKiBVbmRvIGZhc3Qgc2xlZXAgd29ya2Fyb3VuZCAqLwo+PiArCW1mY3IJ cjE2CS8qIEJhY2t1cCBDUiBpbnRvIGEgbm9uLXZvbGF0aWxlIHJlZ2lzdGVyICovCj4gCj4gRG9u J3QgeW91IHdhbnQgdG8gZG8gdGhpcyBeXiBiZWZvcmUgY2FsbGluZyBvcGFsX2NhbGxfcmVhbG1v ZGUgZm9yCj4gdGltZWJhc2UgcmVzeW5jIGJlbG93IGFsc28/Cj4gCkkgYW0gbm90IHVzaW5nIGFu eSBvZiB0aGUgQ1IgcmVnaXN0ZXJzIGFmdGVyIHRoZSB0aW1lYmFzZSByZXN5bmMgT1BBTApjYWxs LiBUaG91Z2ggaW4gdGhlIG5leHQgY29tbWl0IHdoZXJlIEkgZG8gdXNlIHRoZSBDUnMsIEkgcmVz dG9yZSB0aGVtCmFmdGVyIHRoZSBPUEFMIGNhbGwuCgo+PiArCWxpCXIzLDEKPj4gKwlsaQlyNCww Cj4+ICsJbGkJcjAsT1BBTF9DT05GSUdfQ1BVX0lETEVfU1RBVEUKPj4gKwlibAlvcGFsX2NhbGxf cmVhbG1vZGUKPj4gKwltdGNyCXIxNgkvKiBSZXN0b3JlIENSICovCj4+ICsKPj4gKwkvKiBEbyB0 aW1lYmFzZSByZXN5bmMgaWYgd2UgYXJlIHdha2luZyB1cCBmcm9tIHNsZWVwLiBVc2UgY3IxIHZh bHVlCj4+ICsJICogc2V0IGluIGV4Y2VwdGlvbnMtNjRzLlMgKi8KPj4gKwlibGUJY3IxLGNsZWFy X2xvY2sKPj4gKwo+PiArdGltZWJhc2VfcmVzeW5jOgo+PiAgCS8qIFRpbWUgYmFzZSByZS1zeW5j ICovCj4+IC0JbGkJcjMsT1BBTF9SRVNZTkNfVElNRUJBU0UKPj4gKwlsaQlyMCxPUEFMX1JFU1lO Q19USU1FQkFTRQo+PiAgCWJsCW9wYWxfY2FsbF9yZWFsbW9kZTsKPj4gLQo+PiBkaWZmIC0tZ2l0 IGEvYXJjaC9wb3dlcnBjL3BsYXRmb3Jtcy9wb3dlcm52L3NldHVwLmMgYi9hcmNoL3Bvd2VycGMv cGxhdGZvcm1zL3Bvd2VybnYvc2V0dXAuYwo+PiBpbmRleCAzNGM2NjY1Li45ODBjOTY0IDEwMDY0 NAo+PiAtLS0gYS9hcmNoL3Bvd2VycGMvcGxhdGZvcm1zL3Bvd2VybnYvc2V0dXAuYwo+PiArKysg Yi9hcmNoL3Bvd2VycGMvcGxhdGZvcm1zL3Bvd2VybnYvc2V0dXAuYwo+PiBAQCAtMzYsNiArMzYs OCBAQAo+PiAgI2luY2x1ZGUgPGFzbS9vcGFsLmg+Cj4+ICAjaW5jbHVkZSA8YXNtL2tleGVjLmg+ Cj4+ICAjaW5jbHVkZSA8YXNtL3NtcC5oPgo+PiArI2luY2x1ZGUgPGFzbS9jcHV0aHJlYWRzLmg+ Cj4+ICsjaW5jbHVkZSA8YXNtL2NwdWlkbGUuaD4KPj4gIAo+PiAgI2luY2x1ZGUgInBvd2VybnYu aCIKPj4gIAo+PiBAQCAtMjkyLDExICsyOTQsNTUgQEAgc3RhdGljIHZvaWQgX19pbml0IHBudl9z ZXR1cF9tYWNoZGVwX3J0YXModm9pZCkKPj4gIAo+PiAgc3RhdGljIHUzMiBzdXBwb3J0ZWRfY3B1 aWRsZV9zdGF0ZXM7Cj4+ICAKPj4gK3N0YXRpYyB2b2lkIHBudl9hbGxvY19pZGxlX2NvcmVfc3Rh dGVzKHZvaWQpCj4+ICt7Cj4+ICsJaW50IGksIGo7Cj4+ICsJaW50IG5yX2NvcmVzID0gY3B1X25y X2NvcmVzKCk7Cj4+ICsJdTMyICpjb3JlX2lkbGVfc3RhdGU7Cj4+ICsKPj4gKwkvKgo+PiArCSAq IERlZXAgaWRsZSBzdGF0ZXMgbGlrZSBzbGVlcCBhbmQgd2lua2xlIGFyZSBwZXIgY29yZSBpZGxl IHN0YXRlcy4KPj4gKwkgKiBBIGNvcmUgZW50ZXJzIHRoZXNlIHN0YXRlcyBvbmx5IHdoZW4gYWxs IHRoZSB0aHJlYWRzIGVudGVyIGVpdGhlcgo+PiArCSAqIHRoZSBwYXJ0aWN1bGFyIGlkbGUgc3Rh dGUgb3IgYSBkZWVwZXIgb25lLiBUaGVyZSBhcmUgdGFza3MgbGlrZQo+PiArCSAqIGZhc3RzbGVl cCBoYXJkd2FyZSBidWcgd29ya2Fyb3VuZCBhbmQgaHlwZXJ2aXNvciBjb3JlIHN0YXRlIHNhdmUK Pj4gKwkgKiB3aGljaCBoYXZlIHRvIGJlIGRvbmUgb25seSBieSB0aGUgbGFzdCB0aHJlYWQgb2Yg dGhlIGNvcmUgZW50ZXJpbmcKPj4gKwkgKiBkZWVwIGlkbGUgc3RhdGUgYW5kIHNpbWlsYXJseSB0 YXNrcyBsaWtlIHRpbWViYXNlIHJlc3luYywgaHlwZXJ2aXNvcgo+PiArCSAqIGNvcmUgcmVnaXN0 ZXIgcmVzdG9yZSB0aGF0IGhhdmUgdG8gYmUgZG9uZSBvbmx5IGJ5IHRoZSBmaXJzdCB0aHJlYWQK Pj4gKwkgKiB3YWtpbmcgdXAgZnJvbSB0aGVzZSBzdGF0ZXMuIEludHJvZHVjaW5nIGNvcmVfaWRs ZV9zdGF0ZSwgYSBwZXIgY29yZQo+PiArCSAqIHN0cnVjdHVyZSB3aGljaCB3aWxsIGtlZXAgdHJh Y2sgdGhyZWFkcyBlbnRlcmluZyBpZGxlIHN0YXRlcyBkZWVwZXIKPj4gKwkgKiB0aGFuIHNsZWVw Lgo+IAo+IFNpbmNlIHlvdSBhbHJlYWR5IGhhdmUgZXhwbGFpbmVkIF5eIGluIHRoZSBjaGFuZ2Vs b2csIHlvdSBkbyBub3QgbmVlZCB0bwo+IGVsYWJvcmF0ZSBpdCBoZXJlLgo+IAo+PiArCSAqIGNv cmVfaWRsZV9zdGF0ZSAtIEZpcnN0IDggYml0cyB0cmFjayB0aGUgaWRsZSBzdGF0ZSBvZiBlYWNo IHRocmVhZAo+PiArCSAqIG9mIHRoZSBjb3JlLiBUaGUgOHRoIGJpdCBpcyB0aGUgbG9jayBiaXQu IEluaXRpYWxseSBhbGwgdGhyZWFkIGJpdHMKPj4gKwkgKiBhcmUgc2V0LiBUaGV5IGFyZSBjbGVh cmVkIHdoZW4gdGhlIHRocmVhZCBlbnRlcnMgZGVlcCBpZGxlIHN0YXRlCj4+ICsJICogbGlrZSBz bGVlcCBhbmQgd2lua2xlLiBJbml0aWFsbHkgdGhlIGxvY2sgYml0IGlzIGNsZWFyZWQuCj4gCj4g eW91IGNhbiBzaW1wbHkgaGF2ZSB0aGUgY29tbWVudCBhYm91dCB0aGUgYml0cyBvZiBjb3JlX2lk bGVfc3RhdGUKPiB3aXRob3V0IGhhdmluZyB0byBtZW50aW9uIGFib3V0IHdoZW4gdGhleSBhcmUg Y2xlYXJlZCBldGMuLgpPa2F5LiBXaWxsIG1ha2UgdGhlIGNoYW5nZS4KPiAKPj4gKwkgKiBUaGUg bG9jayBiaXQgaGFzIDIgcHVycG9zZXMKPj4gKwkgKiBhLiBXaGlsZSB0aGUgZmlyc3QgdGhyZWFk IGlzIHJlc3RvcmluZyBjb3JlIHN0YXRlLCBpdCBwcmV2ZW50cwo+PiArCSAqIGZyb20gb3RoZXIg dGhyZWFkcyBpbiB0aGUgY29yZSBmcm9tIHN3aXRjaGluZyB0byBwcmNvZXNzIGNvbnRleHQuCj4+ ICsJICogYi4gV2hpbGUgdGhlIGxhc3QgdGhyZWFkIGluIHRoZSBjb3JlIGlzIHNhdmluZyB0aGUg Y29yZSBzdGF0ZSwgaXQKPj4gKwkgKiBwcmV2ZW50IGEgZGlmZmVyZW50IHRocmVhZCBmcm9tIHdh a2luZyB1cC4KPiAKPiBUaGUgYWJvdmUgdHdvIHBvaW50cyBhcmUgdXNlZnVsLiBBcyBmYXIgYXMg SSBzZWUgYmVzaWRlcyBleHBsYWluaW5nIHRoZQo+IGJpdHMgb2YgY29yZV9pZGxlX3N0YXRlIHN0 cnVjdHVyZSBhbmQgdGhlIHB1cnBvc2Ugb2YgbG9jayBiaXQgdGhlIHJlc3QKPiBvZiB0aGUgY29t bWVudHMgaXMgcmVkdW5kYW50LiBBIGdpdC1ibGFtZSB3aWxsIGxldCBwZW9wbGUga25vdyB3aHkg YWxsCj4gdGhpcyBpcyBuZWVkZWQuIFRoZSBjb21tZW50IHNlY3Rpb24gc2hvdWxkIG5vdCBiZSB1 c2VkIHVwIGZvciB0aGlzCj4gcHVycG9zZSBJTU8uCj4gCj4gUmVnYXJkcwo+IFByZWV0aSBVIE11 cnRoeQo+IAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18K TGludXhwcGMtZGV2IG1haWxpbmcgbGlzdApMaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZwpo dHRwczovL2xpc3RzLm96bGFicy5vcmcvbGlzdGluZm8vbGludXhwcGMtZGV2 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp08.au.ibm.com (e23smtp08.au.ibm.com [202.81.31.141]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 8CDF61A01D7 for ; Mon, 17 Nov 2014 16:49:58 +1100 (AEDT) Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 17 Nov 2014 15:49:57 +1000 Received: from d23relay07.au.ibm.com (d23relay07.au.ibm.com [9.190.26.37]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id DE2C53578068 for ; Mon, 17 Nov 2014 16:49:53 +1100 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay07.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sAH5nWfx25952276 for ; Mon, 17 Nov 2014 16:49:32 +1100 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sAH5nrYb020177 for ; Mon, 17 Nov 2014 16:49:53 +1100 Message-ID: <54698C78.9050005@linux.vnet.ibm.com> Date: Mon, 17 Nov 2014 11:19:44 +0530 From: Shreyas B Prabhu MIME-Version: 1.0 To: Preeti U Murthy , linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] powernv: cpuidle: Redesign idle states management References: <1415030910-5799-1-git-send-email-shreyas@linux.vnet.ibm.com> <1415030910-5799-4-git-send-email-shreyas@linux.vnet.ibm.com> <54630362.7050007@linux.vnet.ibm.com> In-Reply-To: <54630362.7050007@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Cc: Paul Mackerras , "Rafael J. Wysocki" , linuxppc-dev@lists.ozlabs.org, linux-pm@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Preeti, On Wednesday 12 November 2014 12:21 PM, Preeti U Murthy wrote: > Hi Shreyas, > > On 11/03/2014 09:38 PM, Shreyas B. Prabhu wrote: >> diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S >> index 283c603..df11acb 100644 >> --- a/arch/powerpc/kernel/idle_power7.S >> +++ b/arch/powerpc/kernel/idle_power7.S >> _GLOBAL(power7_idle) >> /* Now check if user or arch enabled NAP mode */ >> @@ -141,49 +192,16 @@ _GLOBAL(power7_idle) >> >> _GLOBAL(power7_nap) >> mr r4,r3 >> - li r3,0 >> + li r3,1 > > The comment at the top of this file states 0 for nap and 1 for sleep. > You will need to change that. As an alternative I would suggest using > the macros that you have already defined:PNV_THREAD_NAP and > PNV_THREAD_SLEEP to write to r3 above and remove the lines that say 0 > for nap and 1 for sleep in the comments. My bad, I had changed the comment in the next commit. I'll make the change. > >> b power7_powersave_common >> /* No return */ >> > > > >> @@ -210,12 +226,91 @@ _GLOBAL(power7_wakeup_tb_loss) >> BEGIN_FTR_SECTION >> CHECK_HMI_INTERRUPT >> END_FTR_SECTION_IFSET(CPU_FTR_HVMODE) >> + >> + li r7,1 >> + mfspr r8,SPRN_PIR >> + /* >> + * The last 3 bits of PIR represents the thread id of a cpu >> + * in power8. This will need adjusting for power7. >> + */ >> + andi. r8,r8,0x07 /* Get thread id into r8 */ >> + rotld r7,r7,r8 >> + /* r7 now has 'thread_id'th bit set */ >> + >> + ld r14,PACA_CORE_IDLE_STATE_PTR(r13) >> +lwarx_loop2: >> + lwarx r15,0,r14 >> + andi. r9,r15,PNV_CORE_IDLE_LOCK_BIT >> + /* >> + * Lock bit is set in one of the 2 cases- >> + * a. In the sleep/winkle enter path, the last thread is executing >> + * fastsleep workaround code. >> + * b. In the wake up path, another thread is executing fastsleep >> + * workaround undo code or resyncing timebase or restoring context >> + * In either case loop until the lock bit is cleared. >> + */ >> + bne lwarx_loop2 >> + >> + cmpwi cr2,r15,0 >> + or r15,r15,r7 /* Set thread bit */ >> + >> + beq cr2,first_thread >> + >> + /* Not first thread in core to wake up */ >> + stwcx. r15,0,r14 >> + bne- lwarx_loop2 >> + b common_exit >> + >> +first_thread: >> + /* First thread in core to wakeup */ >> + ori r15,r15,PNV_CORE_IDLE_LOCK_BIT >> + stwcx. r15,0,r14 >> + bne- lwarx_loop2 >> + >> + LOAD_REG_ADDR(r3, pnv_need_fastsleep_workaround) >> + lbz r3,0(r3) >> + cmpwi r3,1 >> + /* skip fastsleep workaround if its not needed */ >> + bne timebase_resync >> + >> + /* Undo fast sleep workaround */ >> + mfcr r16 /* Backup CR into a non-volatile register */ > > Don't you want to do this ^^ before calling opal_call_realmode for > timebase resync below also? > I am not using any of the CR registers after the timebase resync OPAL call. Though in the next commit where I do use the CRs, I restore them after the OPAL call. >> + li r3,1 >> + li r4,0 >> + li r0,OPAL_CONFIG_CPU_IDLE_STATE >> + bl opal_call_realmode >> + mtcr r16 /* Restore CR */ >> + >> + /* Do timebase resync if we are waking up from sleep. Use cr1 value >> + * set in exceptions-64s.S */ >> + ble cr1,clear_lock >> + >> +timebase_resync: >> /* Time base re-sync */ >> - li r3,OPAL_RESYNC_TIMEBASE >> + li r0,OPAL_RESYNC_TIMEBASE >> bl opal_call_realmode; >> - >> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c >> index 34c6665..980c964 100644 >> --- a/arch/powerpc/platforms/powernv/setup.c >> +++ b/arch/powerpc/platforms/powernv/setup.c >> @@ -36,6 +36,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "powernv.h" >> >> @@ -292,11 +294,55 @@ static void __init pnv_setup_machdep_rtas(void) >> >> static u32 supported_cpuidle_states; >> >> +static void pnv_alloc_idle_core_states(void) >> +{ >> + int i, j; >> + int nr_cores = cpu_nr_cores(); >> + u32 *core_idle_state; >> + >> + /* >> + * Deep idle states like sleep and winkle are per core idle states. >> + * A core enters these states only when all the threads enter either >> + * the particular idle state or a deeper one. There are tasks like >> + * fastsleep hardware bug workaround and hypervisor core state save >> + * which have to be done only by the last thread of the core entering >> + * deep idle state and similarly tasks like timebase resync, hypervisor >> + * core register restore that have to be done only by the first thread >> + * waking up from these states. Introducing core_idle_state, a per core >> + * structure which will keep track threads entering idle states deeper >> + * than sleep. > > Since you already have explained ^^ in the changelog, you do not need to > elaborate it here. > >> + * core_idle_state - First 8 bits track the idle state of each thread >> + * of the core. The 8th bit is the lock bit. Initially all thread bits >> + * are set. They are cleared when the thread enters deep idle state >> + * like sleep and winkle. Initially the lock bit is cleared. > > you can simply have the comment about the bits of core_idle_state > without having to mention about when they are cleared etc.. Okay. Will make the change. > >> + * The lock bit has 2 purposes >> + * a. While the first thread is restoring core state, it prevents >> + * from other threads in the core from switching to prcoess context. >> + * b. While the last thread in the core is saving the core state, it >> + * prevent a different thread from waking up. > > The above two points are useful. As far as I see besides explaining the > bits of core_idle_state structure and the purpose of lock bit the rest > of the comments is redundant. A git-blame will let people know why all > this is needed. The comment section should not be used up for this > purpose IMO. > > Regards > Preeti U Murthy > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751852AbaKQGIv (ORCPT ); Mon, 17 Nov 2014 01:08:51 -0500 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:37807 "EHLO e23smtp08.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751813AbaKQGIt (ORCPT ); Mon, 17 Nov 2014 01:08:49 -0500 Message-ID: <54698C78.9050005@linux.vnet.ibm.com> Date: Mon, 17 Nov 2014 11:19:44 +0530 From: Shreyas B Prabhu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: Preeti U Murthy , linux-kernel@vger.kernel.org CC: linux-pm@vger.kernel.org, "Rafael J. Wysocki" , Paul Mackerras , linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH 3/4] powernv: cpuidle: Redesign idle states management References: <1415030910-5799-1-git-send-email-shreyas@linux.vnet.ibm.com> <1415030910-5799-4-git-send-email-shreyas@linux.vnet.ibm.com> <54630362.7050007@linux.vnet.ibm.com> In-Reply-To: <54630362.7050007@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14111706-0029-0000-0000-0000009C14CB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Preeti, On Wednesday 12 November 2014 12:21 PM, Preeti U Murthy wrote: > Hi Shreyas, > > On 11/03/2014 09:38 PM, Shreyas B. Prabhu wrote: >> diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S >> index 283c603..df11acb 100644 >> --- a/arch/powerpc/kernel/idle_power7.S >> +++ b/arch/powerpc/kernel/idle_power7.S >> _GLOBAL(power7_idle) >> /* Now check if user or arch enabled NAP mode */ >> @@ -141,49 +192,16 @@ _GLOBAL(power7_idle) >> >> _GLOBAL(power7_nap) >> mr r4,r3 >> - li r3,0 >> + li r3,1 > > The comment at the top of this file states 0 for nap and 1 for sleep. > You will need to change that. As an alternative I would suggest using > the macros that you have already defined:PNV_THREAD_NAP and > PNV_THREAD_SLEEP to write to r3 above and remove the lines that say 0 > for nap and 1 for sleep in the comments. My bad, I had changed the comment in the next commit. I'll make the change. > >> b power7_powersave_common >> /* No return */ >> > > > >> @@ -210,12 +226,91 @@ _GLOBAL(power7_wakeup_tb_loss) >> BEGIN_FTR_SECTION >> CHECK_HMI_INTERRUPT >> END_FTR_SECTION_IFSET(CPU_FTR_HVMODE) >> + >> + li r7,1 >> + mfspr r8,SPRN_PIR >> + /* >> + * The last 3 bits of PIR represents the thread id of a cpu >> + * in power8. This will need adjusting for power7. >> + */ >> + andi. r8,r8,0x07 /* Get thread id into r8 */ >> + rotld r7,r7,r8 >> + /* r7 now has 'thread_id'th bit set */ >> + >> + ld r14,PACA_CORE_IDLE_STATE_PTR(r13) >> +lwarx_loop2: >> + lwarx r15,0,r14 >> + andi. r9,r15,PNV_CORE_IDLE_LOCK_BIT >> + /* >> + * Lock bit is set in one of the 2 cases- >> + * a. In the sleep/winkle enter path, the last thread is executing >> + * fastsleep workaround code. >> + * b. In the wake up path, another thread is executing fastsleep >> + * workaround undo code or resyncing timebase or restoring context >> + * In either case loop until the lock bit is cleared. >> + */ >> + bne lwarx_loop2 >> + >> + cmpwi cr2,r15,0 >> + or r15,r15,r7 /* Set thread bit */ >> + >> + beq cr2,first_thread >> + >> + /* Not first thread in core to wake up */ >> + stwcx. r15,0,r14 >> + bne- lwarx_loop2 >> + b common_exit >> + >> +first_thread: >> + /* First thread in core to wakeup */ >> + ori r15,r15,PNV_CORE_IDLE_LOCK_BIT >> + stwcx. r15,0,r14 >> + bne- lwarx_loop2 >> + >> + LOAD_REG_ADDR(r3, pnv_need_fastsleep_workaround) >> + lbz r3,0(r3) >> + cmpwi r3,1 >> + /* skip fastsleep workaround if its not needed */ >> + bne timebase_resync >> + >> + /* Undo fast sleep workaround */ >> + mfcr r16 /* Backup CR into a non-volatile register */ > > Don't you want to do this ^^ before calling opal_call_realmode for > timebase resync below also? > I am not using any of the CR registers after the timebase resync OPAL call. Though in the next commit where I do use the CRs, I restore them after the OPAL call. >> + li r3,1 >> + li r4,0 >> + li r0,OPAL_CONFIG_CPU_IDLE_STATE >> + bl opal_call_realmode >> + mtcr r16 /* Restore CR */ >> + >> + /* Do timebase resync if we are waking up from sleep. Use cr1 value >> + * set in exceptions-64s.S */ >> + ble cr1,clear_lock >> + >> +timebase_resync: >> /* Time base re-sync */ >> - li r3,OPAL_RESYNC_TIMEBASE >> + li r0,OPAL_RESYNC_TIMEBASE >> bl opal_call_realmode; >> - >> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c >> index 34c6665..980c964 100644 >> --- a/arch/powerpc/platforms/powernv/setup.c >> +++ b/arch/powerpc/platforms/powernv/setup.c >> @@ -36,6 +36,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "powernv.h" >> >> @@ -292,11 +294,55 @@ static void __init pnv_setup_machdep_rtas(void) >> >> static u32 supported_cpuidle_states; >> >> +static void pnv_alloc_idle_core_states(void) >> +{ >> + int i, j; >> + int nr_cores = cpu_nr_cores(); >> + u32 *core_idle_state; >> + >> + /* >> + * Deep idle states like sleep and winkle are per core idle states. >> + * A core enters these states only when all the threads enter either >> + * the particular idle state or a deeper one. There are tasks like >> + * fastsleep hardware bug workaround and hypervisor core state save >> + * which have to be done only by the last thread of the core entering >> + * deep idle state and similarly tasks like timebase resync, hypervisor >> + * core register restore that have to be done only by the first thread >> + * waking up from these states. Introducing core_idle_state, a per core >> + * structure which will keep track threads entering idle states deeper >> + * than sleep. > > Since you already have explained ^^ in the changelog, you do not need to > elaborate it here. > >> + * core_idle_state - First 8 bits track the idle state of each thread >> + * of the core. The 8th bit is the lock bit. Initially all thread bits >> + * are set. They are cleared when the thread enters deep idle state >> + * like sleep and winkle. Initially the lock bit is cleared. > > you can simply have the comment about the bits of core_idle_state > without having to mention about when they are cleared etc.. Okay. Will make the change. > >> + * The lock bit has 2 purposes >> + * a. While the first thread is restoring core state, it prevents >> + * from other threads in the core from switching to prcoess context. >> + * b. While the last thread in the core is saving the core state, it >> + * prevent a different thread from waking up. > > The above two points are useful. As far as I see besides explaining the > bits of core_idle_state structure and the purpose of lock bit the rest > of the comments is redundant. A git-blame will let people know why all > this is needed. The comment section should not be used up for this > purpose IMO. > > Regards > Preeti U Murthy >