From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vince Hsu Subject: Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp Date: Thu, 25 Dec 2014 10:28:08 +0800 Message-ID: <549B7638.2010405@nvidia.com> References: <1419331204-26679-1-git-send-email-vinceh@nvidia.com> <1419331204-26679-2-git-send-email-vinceh@nvidia.com> <1419426990.2179.7.camel@lynxeye.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1419426990.2179.7.camel-8ppwABl0HbeELgA04lAiVw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: nouveau-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "Nouveau" To: Lucas Stach Cc: swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org, nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, seven-FA6nBp6kBxZzu6KWmfFNGwC/G2K4zDHf@public.gmane.org List-Id: linux-tegra@vger.kernel.org T24gMTIvMjQvMjAxNCAwOToxNiBQTSwgTHVjYXMgU3RhY2ggd3JvdGU6Cj4gQW0gRGllbnN0YWcs IGRlbiAyMy4xMi4yMDE0LCAxODozOSArMDgwMCBzY2hyaWViIFZpbmNlIEhzdToKPj4gVGhlIFRl Z3JhMTI0IGFuZCBsYXRlciBUZWdyYSBTb0NzIGhhdmUgYSBzZXBhdGF0ZSByYWlsIGdhdGluZyBy ZWdpc3Rlcgo+PiB0byBlbmFibGUvZGlzYWJsZSB0aGUgY2xhbXAuIFRoZSBvcmlnaW5hbCBmdW5j dGlvbgo+PiB0ZWdyYV9wb3dlcmdhdGVfcmVtb3ZlX2NsYW1waW5nKCkgaXMgbm90IHN1ZmZpY2ll bnQgZm9yIHRoZSBlbmFibGUKPj4gZnVuY3Rpb24uIFNvIGFkZCBhIG5ldyBmdW5jdGlvbiB3aGlj aCBpcyBkZWRpY2F0ZWQgdG8gdGhlIEdQVSByYWlsCj4+IGdhdGluZy4gQWxzbyBkb24ndCByZWZl ciB0byB0aGUgcG93ZXJnYXRlIElEIHNpbmNlIHRoZSBHUFUgSUQgbWFrZXMgbm8KPj4gc2Vuc2Ug aGVyZS4KPj4KPj4gU2lnbmVkLW9mZi1ieTogVmluY2UgSHN1IDx2aW5jZWhAbnZpZGlhLmNvbT4K PiBUbyBiZSBob25lc3QgSSBkb24ndCBzZWUgdGhlIHBvaW50IG9mIHRoaXMgcGF0Y2guCj4gWW91 IGFyZSBibG9hdGluZyB0aGUgUE1DIGludGVyZmFjZSBieSBpbnRyb2R1Y2luZyBhbm90aGVyIGV4 cG9ydGVkCj4gZnVuY3Rpb24gdGhhdCBkb2VzIG5vdGhpbmcgZGlmZmVyZW50IHRoYW4gd2hhdCB0 aGUgY3VycmVudCBmdW5jdGlvbgo+IGFscmVhZHkgZG9lcy4KPgo+IElmIHlvdSBuZWVkIGEgd2F5 IHRvIGFzc2VydCB0aGUgY2xhbXAgSSB3b3VsZCBoYXZlIGV4cGVjdGVkIHlvdSB0bwo+IGludHJv ZHVjZSBhIGNvbW1vbiBmdW5jdGlvbiB0byBkbyB0aGlzIGZvciBhbGwgcG93ZXIgcGFydGl0aW9u cy4KSSB0aG91Z2h0IGFib3V0IGFkZGluZyBhbiB0ZWdyYV9wb3dlcmdhdGVfYXNzZXJ0X2NsYW1w aW5nKCksIGJ1dCB0aGF0IApkb2Vzbid0IG1ha2Ugc2Vuc2UgdG8gYWxsIHRoZSBwb3dlciBwYXJ0 aXRpb25zIGV4Y2VwdCBHUFUuIE5vdGUgdGhlIApkaWZmZXJlbmNlIGluIFRSTS4gQW55IHN1Z2dl c3Rpb24gZm9yIHRoZSBjb21tb24gZnVuY3Rpb24/Cj4KPiBPdGhlciBjb21tZW50cyBpbmxpbmUu Cj4KPiBSZWdhcmRzLAo+IEx1Y2FzCj4KPj4gLS0tCj4+ICAgZHJpdmVycy9zb2MvdGVncmEvcG1j LmMgfCAzNCArKysrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tCj4+ICAgaW5jbHVkZS9z b2MvdGVncmEvcG1jLmggfCAgMiArKwo+PiAgIDIgZmlsZXMgY2hhbmdlZCwgMjUgaW5zZXJ0aW9u cygrKSwgMTEgZGVsZXRpb25zKC0pCj4+Cj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3NvYy90ZWdy YS9wbWMuYyBiL2RyaXZlcnMvc29jL3RlZ3JhL3BtYy5jCj4+IGluZGV4IGEyYzBjZWI5NWY4Zi4u Nzc5OGM1MzBlYWQxIDEwMDY0NAo+PiAtLS0gYS9kcml2ZXJzL3NvYy90ZWdyYS9wbWMuYwo+PiAr KysgYi9kcml2ZXJzL3NvYy90ZWdyYS9wbWMuYwo+PiBAQCAtMjI1LDE3ICsyMjUsNiBAQCBpbnQg dGVncmFfcG93ZXJnYXRlX3JlbW92ZV9jbGFtcGluZyhpbnQgaWQpCj4+ICAgCQlyZXR1cm4gLUVJ TlZBTDsKPj4gICAKPj4gICAJLyoKPj4gLQkgKiBUaGUgVGVncmExMjQgR1BVIGhhcyBhIHNlcGFy YXRlIHJlZ2lzdGVyICh3aXRoIGRpZmZlcmVudCBzZW1hbnRpY3MpCj4+IC0JICogdG8gcmVtb3Zl IGNsYW1wcy4KPj4gLQkgKi8KPj4gLQlpZiAodGVncmFfZ2V0X2NoaXBfaWQoKSA9PSBURUdSQTEy NCkgewo+PiAtCQlpZiAoaWQgPT0gVEVHUkFfUE9XRVJHQVRFXzNEKSB7Cj4+IC0JCQl0ZWdyYV9w bWNfd3JpdGVsKDAsIEdQVV9SR19DTlRSTCk7Cj4+IC0JCQlyZXR1cm4gMDsKPj4gLQkJfQo+PiAt CX0KPj4gLQo+PiAtCS8qCj4+ICAgCSAqIFRlZ3JhIDIgaGFzIGEgYnVnIHdoZXJlIFBDSUUgYW5k IFZERSBjbGFtcGluZyBtYXNrcyBhcmUKPj4gICAJICogc3dhcHBlZCByZWxhdGl2ZWx5IHRvIHRo ZSBwYXJ0aXRpb24gaWRzCj4+ICAgCSAqLwo+PiBAQCAtMjUzLDYgKzI0MiwyOSBAQCBpbnQgdGVn cmFfcG93ZXJnYXRlX3JlbW92ZV9jbGFtcGluZyhpbnQgaWQpCj4+ICAgRVhQT1JUX1NZTUJPTCh0 ZWdyYV9wb3dlcmdhdGVfcmVtb3ZlX2NsYW1waW5nKTsKPj4gICAKPj4gICAvKioKPj4gKyAqIHRl Z3JhX3Bvd2VyZ2F0ZV9ncHVfc2V0X2NsYW1waW5nIC0gY29udHJvbCBHUFUtU09DIGNsYW1wcwo+ PiArICoKPj4gKyAqIFRoZSBwb3N0LVRlZ3JhMTE0IGNoaXBzIGhhdmUgYSBzZXBhcmF0ZSByYWls IGdhdGluZyByZWdpc3RlciB0byBjb25maWd1cmUKPj4gKyAqIGNsYW1wcy4KPj4gKyAqCj4+ICsg KiBAYXNzZXJ0OiB0cnVlIHRvIGFzc2VydCBjbGFtcCwgYW5kIGZhbHNlIHRvIHJlbW92ZQo+PiAr ICovCj4+ICtpbnQgdGVncmFfcG93ZXJnYXRlX2dwdV9zZXRfY2xhbXBpbmcoYm9vbCBhc3NlcnQp Cj4gVGhvc2UgZnVuY3Rpb25zIHdpdGggYSBib29sIHBhcmFtZXRlciB0byBzZXQvdW5zZXQgc29t ZXRoaW5nIGFyZSByZWFsbHkKPiBhbm5veWluZy4gUGxlYXNlIGF2b2lkIHRoaXMgcGF0dGVybi4g VGhlIG5hbWluZyBvZiB0aGUgb3JpZ2luYWwgZnVuY3Rpb24KPiBpcyBtdWNoIG1vcmUgaW50dWl0 aXZlLgpCdXQgdGhlIG9yaWdpbmFsIGZ1bmN0aW9uIGlzIG5vdCBzdWZmaWNpZW50LiBNYXliZSBh ZGQgYSAKdGVncmFfcG93ZXJnYXRlX2Fzc2VydF9ncHVfY2xhbXBpbmcoKT8gVGhhdCB3YXkgSSB3 aWxsIHByZWZlciB0byBhZGRpbmcgCm9uZSBtb3JlIHJlbW92YWwgZnVuY3Rpb24gZm9yIEdQVS4g QW5kIHRoZW4gYWdhaW4gdGhhdCdzIGEgYmxvYXQsIHRvby4KPgo+PiArewo+PiArCWlmICghcG1j LT5zb2MpCj4+ICsJCXJldHVybiAtRUlOVkFMOwo+PiArCj4+ICsJaWYgKHRlZ3JhX2dldF9jaGlw X2lkKCkgPT0gVEVHUkExMjQpIHsKPj4gKwkJdGVncmFfcG1jX3dyaXRlbChhc3NlcnQgPyAxIDog MCwgR1BVX1JHX0NOVFJMKTsKPj4gKwkJdGVncmFfcG1jX3JlYWRsKEdQVV9SR19DTlRSTCk7Cj4g WW91IGFyZSByZWFkaW5nIHRoZSByZWdpc3RlciBiYWNrIGhlcmUsIHdoaWNoIHRvIG1lIHNlZW1z IGxpa2UgeW91IGFyZQo+IHRyeWluZyB0byBtYWtlIHN1cmUgdGhhdCB0aGUgd3JpdGUgaXMgZmx1 c2hlZC4gV2h5IGlzIHRoaXMgbmVlZGVkPwo+IEkgYWxzbyBvYnNlcnZlZCB0aGUgbmVlZCB0byBk byB0aGlzIHdoaWxlIHdvcmtpbmcgb24gVGVncmExMjQgUENJZSBpbgo+IEJhcmVib3gsIG90aGVy d2lzZSB0aGUgcGFydGl0aW9uIHdvdWxkbid0IHBvd2VyIHVwLiBJIGRpZG4ndCBoYXZlIHRpbWUK PiB0byBmb2xsb3cgdXAgb24gdGhpcyB5ZXQsIHNvIGl0IHdvdWxkIGJlIG5pY2UgaWYgeW91IGNv dWxkIGV4cGxhaW4gd2h5Cj4gdGhpcyBpcyBuZWVkZWQsIG9yIGlmIHlvdSBkb24ndCBrbm93IGFz ayBIVyBhYm91dCBpdC4KVGhhdCdzIGEgcmVhZCBmZW5jZSB0byBhc3N1cmUgdGhlIHBvc3Qgb2Yg dGhlIHByZXZpb3VzIHdyaXRlcyB0aHJvdWdoIApUZWdyYSBpbnRlcmNvbm5lY3QuIChjb3B5LXBh c3RlciBmcm9tIApodHRwczovL2FuZHJvaWQuZ29vZ2xlc291cmNlLmNvbS9rZXJuZWwvdGVncmEu Z2l0LysvMjhiMTA3ZGNiM2FhMTIyZGU4ZTk0ZTQ4YWY1NDgxNDBkNTE5Mjk4ZikKPgo+PiArCQly ZXR1cm4gMDsKPj4gKwl9Cj4+ICsKPj4gKwlyZXR1cm4gLUVJTlZBTDsKPj4gK30KPj4gK0VYUE9S VF9TWU1CT0wodGVncmFfcG93ZXJnYXRlX2dwdV9zZXRfY2xhbXBpbmcpOwo+PiArCj4+ICsvKioK Pj4gICAgKiB0ZWdyYV9wb3dlcmdhdGVfc2VxdWVuY2VfcG93ZXJfdXAoKSAtIHBvd2VyIHVwIHBh cnRpdGlvbgo+PiAgICAqIEBpZDogcGFydGl0aW9uIElECj4+ICAgICogQGNsazogY2xvY2sgZm9y IHBhcnRpdGlvbgo+PiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9zb2MvdGVncmEvcG1jLmggYi9pbmNs dWRlL3NvYy90ZWdyYS9wbWMuaAo+PiBpbmRleCA2NWE5MzI3M2U3MmYuLjUzZDYyMDUyNWE5ZSAx MDA2NDQKPj4gLS0tIGEvaW5jbHVkZS9zb2MvdGVncmEvcG1jLmgKPj4gKysrIGIvaW5jbHVkZS9z b2MvdGVncmEvcG1jLmgKPj4gQEAgLTEwOSw2ICsxMDksOCBAQCBpbnQgdGVncmFfcG93ZXJnYXRl X2lzX3Bvd2VyZWQoaW50IGlkKTsKPj4gICBpbnQgdGVncmFfcG93ZXJnYXRlX3Bvd2VyX29uKGlu dCBpZCk7Cj4+ICAgaW50IHRlZ3JhX3Bvd2VyZ2F0ZV9wb3dlcl9vZmYoaW50IGlkKTsKPj4gICBp bnQgdGVncmFfcG93ZXJnYXRlX3JlbW92ZV9jbGFtcGluZyhpbnQgaWQpOwo+PiArLyogT25seSBm b3IgVGVncmExMjQgYW5kIGxhdGVyICovCj4+ICtpbnQgdGVncmFfcG93ZXJnYXRlX2dwdV9zZXRf Y2xhbXBpbmcoYm9vbCBhc3NlcnQpOwo+PiAgIAo+PiAgIC8qIE11c3QgYmUgY2FsbGVkIHdpdGgg Y2xrIGRpc2FibGVkLCBhbmQgcmV0dXJucyB3aXRoIGNsayBlbmFibGVkICovCj4+ICAgaW50IHRl Z3JhX3Bvd2VyZ2F0ZV9zZXF1ZW5jZV9wb3dlcl91cChpbnQgaWQsIHN0cnVjdCBjbGsgKmNsaywK PgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTm91dmVh dSBtYWlsaW5nIGxpc3QKTm91dmVhdUBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL25vdXZlYXUK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751949AbaLYC23 (ORCPT ); Wed, 24 Dec 2014 21:28:29 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:14452 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751692AbaLYC21 (ORCPT ); Wed, 24 Dec 2014 21:28:27 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Wed, 24 Dec 2014 18:23:03 -0800 Message-ID: <549B7638.2010405@nvidia.com> Date: Thu, 25 Dec 2014 10:28:08 +0800 From: Vince Hsu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Lucas Stach CC: , , , , , , , , , Subject: Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp References: <1419331204-26679-1-git-send-email-vinceh@nvidia.com> <1419331204-26679-2-git-send-email-vinceh@nvidia.com> <1419426990.2179.7.camel@lynxeye.de> In-Reply-To: <1419426990.2179.7.camel@lynxeye.de> X-Originating-IP: [10.19.108.126] X-ClientProxiedBy: DRBGMAIL101.nvidia.com (10.18.16.20) To drhkmail101.nvidia.com (10.25.59.15) Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/24/2014 09:16 PM, Lucas Stach wrote: > Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu: >> The Tegra124 and later Tegra SoCs have a sepatate rail gating register >> to enable/disable the clamp. The original function >> tegra_powergate_remove_clamping() is not sufficient for the enable >> function. So add a new function which is dedicated to the GPU rail >> gating. Also don't refer to the powergate ID since the GPU ID makes no >> sense here. >> >> Signed-off-by: Vince Hsu > To be honest I don't see the point of this patch. > You are bloating the PMC interface by introducing another exported > function that does nothing different than what the current function > already does. > > If you need a way to assert the clamp I would have expected you to > introduce a common function to do this for all power partitions. I thought about adding an tegra_powergate_assert_clamping(), but that doesn't make sense to all the power partitions except GPU. Note the difference in TRM. Any suggestion for the common function? > > Other comments inline. > > Regards, > Lucas > >> --- >> drivers/soc/tegra/pmc.c | 34 +++++++++++++++++++++++----------- >> include/soc/tegra/pmc.h | 2 ++ >> 2 files changed, 25 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c >> index a2c0ceb95f8f..7798c530ead1 100644 >> --- a/drivers/soc/tegra/pmc.c >> +++ b/drivers/soc/tegra/pmc.c >> @@ -225,17 +225,6 @@ int tegra_powergate_remove_clamping(int id) >> return -EINVAL; >> >> /* >> - * The Tegra124 GPU has a separate register (with different semantics) >> - * to remove clamps. >> - */ >> - if (tegra_get_chip_id() == TEGRA124) { >> - if (id == TEGRA_POWERGATE_3D) { >> - tegra_pmc_writel(0, GPU_RG_CNTRL); >> - return 0; >> - } >> - } >> - >> - /* >> * Tegra 2 has a bug where PCIE and VDE clamping masks are >> * swapped relatively to the partition ids >> */ >> @@ -253,6 +242,29 @@ int tegra_powergate_remove_clamping(int id) >> EXPORT_SYMBOL(tegra_powergate_remove_clamping); >> >> /** >> + * tegra_powergate_gpu_set_clamping - control GPU-SOC clamps >> + * >> + * The post-Tegra114 chips have a separate rail gating register to configure >> + * clamps. >> + * >> + * @assert: true to assert clamp, and false to remove >> + */ >> +int tegra_powergate_gpu_set_clamping(bool assert) > Those functions with a bool parameter to set/unset something are really > annoying. Please avoid this pattern. The naming of the original function > is much more intuitive. But the original function is not sufficient. Maybe add a tegra_powergate_assert_gpu_clamping()? That way I will prefer to adding one more removal function for GPU. And then again that's a bloat, too. > >> +{ >> + if (!pmc->soc) >> + return -EINVAL; >> + >> + if (tegra_get_chip_id() == TEGRA124) { >> + tegra_pmc_writel(assert ? 1 : 0, GPU_RG_CNTRL); >> + tegra_pmc_readl(GPU_RG_CNTRL); > You are reading the register back here, which to me seems like you are > trying to make sure that the write is flushed. Why is this needed? > I also observed the need to do this while working on Tegra124 PCIe in > Barebox, otherwise the partition wouldn't power up. I didn't have time > to follow up on this yet, so it would be nice if you could explain why > this is needed, or if you don't know ask HW about it. That's a read fence to assure the post of the previous writes through Tegra interconnect. (copy-paster from https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f) > >> + return 0; >> + } >> + >> + return -EINVAL; >> +} >> +EXPORT_SYMBOL(tegra_powergate_gpu_set_clamping); >> + >> +/** >> * tegra_powergate_sequence_power_up() - power up partition >> * @id: partition ID >> * @clk: clock for partition >> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h >> index 65a93273e72f..53d620525a9e 100644 >> --- a/include/soc/tegra/pmc.h >> +++ b/include/soc/tegra/pmc.h >> @@ -109,6 +109,8 @@ int tegra_powergate_is_powered(int id); >> int tegra_powergate_power_on(int id); >> int tegra_powergate_power_off(int id); >> int tegra_powergate_remove_clamping(int id); >> +/* Only for Tegra124 and later */ >> +int tegra_powergate_gpu_set_clamping(bool assert); >> >> /* Must be called with clk disabled, and returns with clk enabled */ >> int tegra_powergate_sequence_power_up(int id, struct clk *clk, >