From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S937810AbdAKNUx (ORCPT ); Wed, 11 Jan 2017 08:20:53 -0500 Received: from mail-qt0-f195.google.com ([209.85.216.195]:33877 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936903AbdAKNTa (ORCPT ); Wed, 11 Jan 2017 08:19:30 -0500 MIME-Version: 1.0 In-Reply-To: <20170111111814.GJ14217@n2100.armlinux.org.uk> References: <20170106065947.30631-1-wenyou.yang@atmel.com> <20170106065947.30631-2-wenyou.yang@atmel.com> <20170110161821.vp673jyfqx6s76pg@piout.net> <20170111111814.GJ14217@n2100.armlinux.org.uk> From: Jean-Jacques Hiblot Date: Wed, 11 Jan 2017 14:18:48 +0100 Message-ID: Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle To: Russell King - ARM Linux Cc: Wenyou Yang , Alexandre Belloni , Mark Rutland , devicetree , Nicolas Ferre , Linux Kernel Mailing List , "robh+dt" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v0BDMWBg001322 2017-01-11 12:18 GMT+01:00 Russell King - ARM Linux : > On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote: >> 2017-01-11 9:15 GMT+01:00 : >> > Hi Jean-Jacques, >> > >> >> -----Original Message----- >> >> From: Jean-Jacques Hiblot [mailto:jjhiblot@gmail.com] >> >> Sent: 2017年1月11日 0:51 >> >> To: Alexandre Belloni >> >> Cc: Wenyou Yang - A41535 ; Mark Rutland >> >> ; devicetree ; Russell >> >> King ; Wenyou Yang - A41535 >> >> ; Nicolas Ferre ; >> >> Linux Kernel Mailing List ; Rob Herring >> >> ; linux-arm-kernel@lists.infradead.org >> >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle >> >> >> >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni >> >> : >> >> > I though a bit more about it, and I don't really like the new >> >> > compatible string. I don't feel this should be necessary. >> >> > >> >> > What about the following: >> >> > >> >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index >> >> > b4332b727e9c..0333aca63e44 100644 >> >> > --- a/arch/arm/mach-at91/pm.c >> >> > +++ b/arch/arm/mach-at91/pm.c >> >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void); static >> >> > struct { >> >> > unsigned long uhp_udp_mask; >> >> > int memctrl; >> >> > + bool has_l2_cache; >> >> > } at91_pm_data; >> >> > >> >> > void __iomem *at91_ramc_base[2]; >> >> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void) >> >> > u32 lpr0, lpr1 = 0; >> >> > u32 saved_lpr0, saved_lpr1 = 0; >> >> > >> >> >> >> > + if (at91_pm_data.has_l2_cache) { >> >> > + flush_cache_all(); >> >> what is the point of calling flush_cache_all() here ? Do we really care that dirty >> >> data in L1 is written to DDR ? I may be missing something but to me it's just extra >> >> latency. >> > >> > Are you mean use outer_flush_all() to flush all cache lines in the outer cache only? >> >> Yes that's what I meant. You see, you don't flush the cache for >> sama5d3 so it shouldn't be required either for sam5d4. You should be >> able to test it quickly and see if L1 flush is indeed required by >> replacing flush_cache_all() with outer_flush_all(). BTW is highly >> probable that L2 cache flush is done in outer_disable() so calling >> outer_flush_all() is probably no required. > > Please don't. Read the comments in the code, and understand the APIs > that you're suggesting people use _before_ making the suggestion: > > /** > * outer_flush_all - clean and invalidate all cache lines in the outer cache > * > * Note: depending on implementation, this may not be atomic - it must > * only be called with interrupts disabled and no other active outer > * cache masters. > * > * It is intended that this function is only used by implementations > * needing to override the outer_cache.disable() method due to security. > * (Some implementations perform this as a clean followed by an invalidate.) > */ > > So, outer_flush_all() should not be called except from L2 cache code > implementing the outer_disable() function - it's not intended for > platforms to use. OK. My bad. I didn't understand the comments. > > There are, however, sadly three users of outer_flush_all() which have > crept in through arm-soc, that should be outer_disable() instead. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Jacques Hiblot Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle Date: Wed, 11 Jan 2017 14:18:48 +0100 Message-ID: References: <20170106065947.30631-1-wenyou.yang@atmel.com> <20170106065947.30631-2-wenyou.yang@atmel.com> <20170110161821.vp673jyfqx6s76pg@piout.net> <20170111111814.GJ14217@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170111111814.GJ14217@n2100.armlinux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Russell King - ARM Linux Cc: Mark Rutland , devicetree , Wenyou Yang , Nicolas Ferre , Linux Kernel Mailing List , robh+dt , Alexandre Belloni , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org MjAxNy0wMS0xMSAxMjoxOCBHTVQrMDE6MDAgUnVzc2VsbCBLaW5nIC0gQVJNIExpbnV4IDxsaW51 eEBhcm1saW51eC5vcmcudWs+Ogo+IE9uIFdlZCwgSmFuIDExLCAyMDE3IGF0IDEyOjA1OjA1UE0g KzAxMDAsIEplYW4tSmFjcXVlcyBIaWJsb3Qgd3JvdGU6Cj4+IDIwMTctMDEtMTEgOToxNSBHTVQr MDE6MDAgIDxXZW55b3UuWWFuZ0BtaWNyb2NoaXAuY29tPjoKPj4gPiBIaSBKZWFuLUphY3F1ZXMs Cj4+ID4KPj4gPj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0KPj4gPj4gRnJvbTogSmVhbi1K YWNxdWVzIEhpYmxvdCBbbWFpbHRvOmpqaGlibG90QGdtYWlsLmNvbV0KPj4gPj4gU2VudDogMjAx N+W5tDHmnIgxMeaXpSAwOjUxCj4+ID4+IFRvOiBBbGV4YW5kcmUgQmVsbG9uaSA8YWxleGFuZHJl LmJlbGxvbmlAZnJlZS1lbGVjdHJvbnMuY29tPgo+PiA+PiBDYzogV2VueW91IFlhbmcgLSBBNDE1 MzUgPFdlbnlvdS5ZYW5nQG1pY3JvY2hpcC5jb20+OyBNYXJrIFJ1dGxhbmQKPj4gPj4gPG1hcmsu cnV0bGFuZEBhcm0uY29tPjsgZGV2aWNldHJlZSA8ZGV2aWNldHJlZUB2Z2VyLmtlcm5lbC5vcmc+ OyBSdXNzZWxsCj4+ID4+IEtpbmcgPGxpbnV4QGFybS5saW51eC5vcmcudWs+OyBXZW55b3UgWWFu ZyAtIEE0MTUzNQo+PiA+PiA8V2VueW91LllhbmdAbWljcm9jaGlwLmNvbT47IE5pY29sYXMgRmVy cmUgPG5pY29sYXMuZmVycmVAYXRtZWwuY29tPjsKPj4gPj4gTGludXggS2VybmVsIE1haWxpbmcg TGlzdCA8bGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZz47IFJvYiBIZXJyaW5nCj4+ID4+IDxy b2JoK2R0QGtlcm5lbC5vcmc+OyBsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcK Pj4gPj4gU3ViamVjdDogUmU6IFtQQVRDSCAxLzNdIEFSTTogYXQ5MTogZmx1c2ggdGhlIEwyIGNh Y2hlIGJlZm9yZSBlbnRlcmluZyBjcHUgaWRsZQo+PiA+Pgo+PiA+PiAyMDE3LTAxLTEwIDE3OjE4 IEdNVCswMTowMCBBbGV4YW5kcmUgQmVsbG9uaQo+PiA+PiA8YWxleGFuZHJlLmJlbGxvbmlAZnJl ZS1lbGVjdHJvbnMuY29tPjoKPj4gPj4gPiBJIHRob3VnaCBhIGJpdCBtb3JlIGFib3V0IGl0LCBh bmQgSSBkb24ndCByZWFsbHkgbGlrZSB0aGUgbmV3Cj4+ID4+ID4gY29tcGF0aWJsZSBzdHJpbmcu IEkgZG9uJ3QgZmVlbCB0aGlzIHNob3VsZCBiZSBuZWNlc3NhcnkuCj4+ID4+ID4KPj4gPj4gPiBX aGF0IGFib3V0IHRoZSBmb2xsb3dpbmc6Cj4+ID4+ID4KPj4gPj4gPiBkaWZmIC0tZ2l0IGEvYXJj aC9hcm0vbWFjaC1hdDkxL3BtLmMgYi9hcmNoL2FybS9tYWNoLWF0OTEvcG0uYyBpbmRleAo+PiA+ PiA+IGI0MzMyYjcyN2U5Yy4uMDMzM2FjYTYzZTQ0IDEwMDY0NAo+PiA+PiA+IC0tLSBhL2FyY2gv YXJtL21hY2gtYXQ5MS9wbS5jCj4+ID4+ID4gKysrIGIvYXJjaC9hcm0vbWFjaC1hdDkxL3BtLmMK Pj4gPj4gPiBAQCAtNTMsNiArNTMsNyBAQCBleHRlcm4gdm9pZCBhdDkxX3BpbmN0cmxfZ3Bpb19y ZXN1bWUodm9pZCk7ICBzdGF0aWMKPj4gPj4gPiBzdHJ1Y3Qgewo+PiA+PiA+ICAgICAgICAgdW5z aWduZWQgbG9uZyB1aHBfdWRwX21hc2s7Cj4+ID4+ID4gICAgICAgICBpbnQgbWVtY3RybDsKPj4g Pj4gPiArICAgICAgIGJvb2wgaGFzX2wyX2NhY2hlOwo+PiA+PiA+ICB9IGF0OTFfcG1fZGF0YTsK Pj4gPj4gPgo+PiA+PiA+ICB2b2lkIF9faW9tZW0gKmF0OTFfcmFtY19iYXNlWzJdOwo+PiA+PiA+ IEBAIC0yNjcsNiArMjY4LDExIEBAIHN0YXRpYyB2b2lkIGF0OTFfZGRyX3N0YW5kYnkodm9pZCkK Pj4gPj4gPiAgICAgICAgIHUzMiBscHIwLCBscHIxID0gMDsKPj4gPj4gPiAgICAgICAgIHUzMiBz YXZlZF9scHIwLCBzYXZlZF9scHIxID0gMDsKPj4gPj4gPgo+PiA+Pgo+PiA+PiA+ICsgICAgICAg aWYgKGF0OTFfcG1fZGF0YS5oYXNfbDJfY2FjaGUpIHsKPj4gPj4gPiArICAgICAgICAgICAgICAg Zmx1c2hfY2FjaGVfYWxsKCk7Cj4+ID4+IHdoYXQgaXMgdGhlIHBvaW50IG9mIGNhbGxpbmcgZmx1 c2hfY2FjaGVfYWxsKCkgaGVyZSA/IERvIHdlIHJlYWxseSBjYXJlIHRoYXQgZGlydHkKPj4gPj4g ZGF0YSBpbiBMMSBpcyB3cml0dGVuIHRvIEREUiA/IEkgbWF5IGJlIG1pc3Npbmcgc29tZXRoaW5n IGJ1dCB0byBtZSBpdCdzIGp1c3QgZXh0cmEKPj4gPj4gbGF0ZW5jeS4KPj4gPgo+PiA+IEFyZSB5 b3UgbWVhbiB1c2Ugb3V0ZXJfZmx1c2hfYWxsKCkgdG8gZmx1c2ggYWxsIGNhY2hlIGxpbmVzIGlu IHRoZSBvdXRlciBjYWNoZSBvbmx5Pwo+Pgo+PiBZZXMgdGhhdCdzIHdoYXQgSSBtZWFudC4gWW91 IHNlZSwgeW91IGRvbid0IGZsdXNoIHRoZSBjYWNoZSBmb3IKPj4gc2FtYTVkMyBzbyBpdCBzaG91 bGRuJ3QgYmUgcmVxdWlyZWQgZWl0aGVyIGZvciBzYW01ZDQuIFlvdSBzaG91bGQgYmUKPj4gYWJs ZSB0byB0ZXN0IGl0IHF1aWNrbHkgYW5kIHNlZSBpZiBMMSBmbHVzaCBpcyBpbmRlZWQgcmVxdWly ZWQgYnkKPj4gcmVwbGFjaW5nICBmbHVzaF9jYWNoZV9hbGwoKSB3aXRoIG91dGVyX2ZsdXNoX2Fs bCgpLiBCVFcgaXMgaGlnaGx5Cj4+IHByb2JhYmxlIHRoYXQgTDIgY2FjaGUgZmx1c2ggaXMgZG9u ZSBpbiBvdXRlcl9kaXNhYmxlKCkgc28gY2FsbGluZwo+PiBvdXRlcl9mbHVzaF9hbGwoKSBpcyBw cm9iYWJseSBubyByZXF1aXJlZC4KPgo+IFBsZWFzZSBkb24ndC4gIFJlYWQgdGhlIGNvbW1lbnRz IGluIHRoZSBjb2RlLCBhbmQgdW5kZXJzdGFuZCB0aGUgQVBJcwo+IHRoYXQgeW91J3JlIHN1Z2dl c3RpbmcgcGVvcGxlIHVzZSBfYmVmb3JlXyBtYWtpbmcgdGhlIHN1Z2dlc3Rpb246Cj4KPiAvKioK PiAgKiBvdXRlcl9mbHVzaF9hbGwgLSBjbGVhbiBhbmQgaW52YWxpZGF0ZSBhbGwgY2FjaGUgbGlu ZXMgaW4gdGhlIG91dGVyIGNhY2hlCj4gICoKPiAgKiBOb3RlOiBkZXBlbmRpbmcgb24gaW1wbGVt ZW50YXRpb24sIHRoaXMgbWF5IG5vdCBiZSBhdG9taWMgLSBpdCBtdXN0Cj4gICogb25seSBiZSBj YWxsZWQgd2l0aCBpbnRlcnJ1cHRzIGRpc2FibGVkIGFuZCBubyBvdGhlciBhY3RpdmUgb3V0ZXIK PiAgKiBjYWNoZSBtYXN0ZXJzLgo+ICAqCj4gICogSXQgaXMgaW50ZW5kZWQgdGhhdCB0aGlzIGZ1 bmN0aW9uIGlzIG9ubHkgdXNlZCBieSBpbXBsZW1lbnRhdGlvbnMKPiAgKiBuZWVkaW5nIHRvIG92 ZXJyaWRlIHRoZSBvdXRlcl9jYWNoZS5kaXNhYmxlKCkgbWV0aG9kIGR1ZSB0byBzZWN1cml0eS4K PiAgKiAoU29tZSBpbXBsZW1lbnRhdGlvbnMgcGVyZm9ybSB0aGlzIGFzIGEgY2xlYW4gZm9sbG93 ZWQgYnkgYW4gaW52YWxpZGF0ZS4pCj4gICovCj4KPiBTbywgb3V0ZXJfZmx1c2hfYWxsKCkgc2hv dWxkIG5vdCBiZSBjYWxsZWQgZXhjZXB0IGZyb20gTDIgY2FjaGUgY29kZQo+IGltcGxlbWVudGlu ZyB0aGUgb3V0ZXJfZGlzYWJsZSgpIGZ1bmN0aW9uIC0gaXQncyBub3QgaW50ZW5kZWQgZm9yCj4g cGxhdGZvcm1zIHRvIHVzZS4KCk9LLiBNeSBiYWQuIEkgZGlkbid0IHVuZGVyc3RhbmQgdGhlIGNv bW1lbnRzLgoKPgo+IFRoZXJlIGFyZSwgaG93ZXZlciwgc2FkbHkgdGhyZWUgdXNlcnMgb2Ygb3V0 ZXJfZmx1c2hfYWxsKCkgd2hpY2ggaGF2ZQo+IGNyZXB0IGluIHRocm91Z2ggYXJtLXNvYywgdGhh dCBzaG91bGQgYmUgb3V0ZXJfZGlzYWJsZSgpIGluc3RlYWQuCj4KPiAtLQo+IFJNSydzIFBhdGNo IHN5c3RlbTogaHR0cDovL3d3dy5hcm1saW51eC5vcmcudWsvZGV2ZWxvcGVyL3BhdGNoZXMvCj4g RlRUQyBicm9hZGJhbmQgZm9yIDAuOG1pbGUgbGluZTogY3VycmVudGx5IGF0IDkuNk1icHMgZG93 biA0MDBrYnBzIHVwCj4gYWNjb3JkaW5nIHRvIHNwZWVkdGVzdC5uZXQuCgpfX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxp bmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3Rz LmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: jjhiblot@gmail.com (Jean-Jacques Hiblot) Date: Wed, 11 Jan 2017 14:18:48 +0100 Subject: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle In-Reply-To: <20170111111814.GJ14217@n2100.armlinux.org.uk> References: <20170106065947.30631-1-wenyou.yang@atmel.com> <20170106065947.30631-2-wenyou.yang@atmel.com> <20170110161821.vp673jyfqx6s76pg@piout.net> <20170111111814.GJ14217@n2100.armlinux.org.uk> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 2017-01-11 12:18 GMT+01:00 Russell King - ARM Linux : > On Wed, Jan 11, 2017 at 12:05:05PM +0100, Jean-Jacques Hiblot wrote: >> 2017-01-11 9:15 GMT+01:00 : >> > Hi Jean-Jacques, >> > >> >> -----Original Message----- >> >> From: Jean-Jacques Hiblot [mailto:jjhiblot at gmail.com] >> >> Sent: 2017?1?11? 0:51 >> >> To: Alexandre Belloni >> >> Cc: Wenyou Yang - A41535 ; Mark Rutland >> >> ; devicetree ; Russell >> >> King ; Wenyou Yang - A41535 >> >> ; Nicolas Ferre ; >> >> Linux Kernel Mailing List ; Rob Herring >> >> ; linux-arm-kernel at lists.infradead.org >> >> Subject: Re: [PATCH 1/3] ARM: at91: flush the L2 cache before entering cpu idle >> >> >> >> 2017-01-10 17:18 GMT+01:00 Alexandre Belloni >> >> : >> >> > I though a bit more about it, and I don't really like the new >> >> > compatible string. I don't feel this should be necessary. >> >> > >> >> > What about the following: >> >> > >> >> > diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index >> >> > b4332b727e9c..0333aca63e44 100644 >> >> > --- a/arch/arm/mach-at91/pm.c >> >> > +++ b/arch/arm/mach-at91/pm.c >> >> > @@ -53,6 +53,7 @@ extern void at91_pinctrl_gpio_resume(void); static >> >> > struct { >> >> > unsigned long uhp_udp_mask; >> >> > int memctrl; >> >> > + bool has_l2_cache; >> >> > } at91_pm_data; >> >> > >> >> > void __iomem *at91_ramc_base[2]; >> >> > @@ -267,6 +268,11 @@ static void at91_ddr_standby(void) >> >> > u32 lpr0, lpr1 = 0; >> >> > u32 saved_lpr0, saved_lpr1 = 0; >> >> > >> >> >> >> > + if (at91_pm_data.has_l2_cache) { >> >> > + flush_cache_all(); >> >> what is the point of calling flush_cache_all() here ? Do we really care that dirty >> >> data in L1 is written to DDR ? I may be missing something but to me it's just extra >> >> latency. >> > >> > Are you mean use outer_flush_all() to flush all cache lines in the outer cache only? >> >> Yes that's what I meant. You see, you don't flush the cache for >> sama5d3 so it shouldn't be required either for sam5d4. You should be >> able to test it quickly and see if L1 flush is indeed required by >> replacing flush_cache_all() with outer_flush_all(). BTW is highly >> probable that L2 cache flush is done in outer_disable() so calling >> outer_flush_all() is probably no required. > > Please don't. Read the comments in the code, and understand the APIs > that you're suggesting people use _before_ making the suggestion: > > /** > * outer_flush_all - clean and invalidate all cache lines in the outer cache > * > * Note: depending on implementation, this may not be atomic - it must > * only be called with interrupts disabled and no other active outer > * cache masters. > * > * It is intended that this function is only used by implementations > * needing to override the outer_cache.disable() method due to security. > * (Some implementations perform this as a clean followed by an invalidate.) > */ > > So, outer_flush_all() should not be called except from L2 cache code > implementing the outer_disable() function - it's not intended for > platforms to use. OK. My bad. I didn't understand the comments. > > There are, however, sadly three users of outer_flush_all() which have > crept in through arm-soc, that should be outer_disable() instead. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.