linux-kernel - Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150105152552.GH12010@ulmo.nvidia.com>
Date:	Mon, 5 Jan 2015 16:25:54 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Vince Hsu <vinceh@...dia.com>
Cc:	Lucas Stach <dev@...xeye.de>, swarren@...dotorg.org,
	gnurou@...il.com, bskeggs@...hat.com, martin.peres@...e.fr,
	seven@...rod-online.com, samuel.pitoiset@...il.com,
	nouveau@...ts.freedesktop.org, linux-tegra@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH nouveau 06/11] platform: complete the power up/down
 sequence

On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
> 
> On 12/24/2014 09:23 PM, Lucas Stach wrote:
> >Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> >>This patch adds some missing pieces of the rail gaing/ungating sequence that
> >>can improve the stability in theory.
> >>
> >>Signed-off-by: Vince Hsu <vinceh@...dia.com>
> >>---
> >>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>  drm/nouveau_platform.h |  3 +++
> >>  2 files changed, 45 insertions(+)
> >>
> >>diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> >>index 68788b17a45c..527fe2358fc9 100644
> >>--- a/drm/nouveau_platform.c
> >>+++ b/drm/nouveau_platform.c
> >>@@ -25,9 +25,11 @@
> >>  #include <linux/module.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/of.h>
> >>+#include <linux/of_platform.h>
> >>  #include <linux/reset.h>
> >>  #include <linux/regulator/consumer.h>
> >>  #include <soc/tegra/fuse.h>
> >>+#include <soc/tegra/mc.h>
> >>  #include <soc/tegra/pmc.h>
> >>  #include "nouveau_drm.h"
> >>@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
> >>  	reset_control_deassert(gpu->rst);
> >>  	udelay(10);
> >>+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> >>+	udelay(10);
> >>+
> >>  	return 0;
> >>  err_clamp:
> >>@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>  {
> >>  	int err;
> >>+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> >>+	udelay(10);
> >>+
> >>+	err = tegra_powergate_gpu_set_clamping(true);
> >>+	if (err)
> >>+		return err;
> >>+	udelay(10);
> >>+
> >>  	reset_control_assert(gpu->rst);
> >>  	udelay(10);
> >>@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>  	return 0;
> >>  }
> >>+static int nouveau_platform_get_mc(struct device *dev,
> >>+		struct tegra_mc **mc, unsigned int *swgroup)
> >Uhm, no. If this is needed this has to be a Tegra MC function and not
> >burried into nouveau code. You are using knowledge about the internal
> >workings of the MC driver here.
> >
> >Also this should probably only take the Dt node pointer as argument and
> >return a something like a tegra_mc_client struct that contains both the
> >MC device pointer and the swgroup so you can pass that to
> >tegra_mc_flush().
> Good idea. I will have something as below in V2 if there is no other
> comments for this.
> 
> tegra_mc_client *tegra_mc_find_client(struct device_node *node)
> {
>     ...
>     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
>     ...
> }
> 
> There were some discussion about this few weeks ago. I'm not sure whether we
> have some conclusion/implementation though. Thierry?
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html

I don't think client is a good fit here. Flushing is done per SWGROUP
(on all clients of the SWGROUP). So I think we'll want something like:

	gpu@0,57000000 {
		...
		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
		...
	};

In the DT and return a struct tegra_mc_swgroup along the lines of:

	struct tegra_mc_client {
		unsigned int id;
		unsigned int swgroup;

		struct list_head list;
	};

	struct tegra_mc_swgroup {
		struct list_head clients;
		unsigned int id;
	};

Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
structures, each representing a memory client pertaining to the
SWGROUP.

We probably don't want to expose these structures publicly, an opaque
type should be enough. Then you can use functions like:

	struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node);

At some point we may even need something like:

	struct tegra_mc_client *tegra_mc_find_client(struct device_node *node,
						     const char *name);

And DT content like this:

	gpu@0,57000000 {
		...
		nvidia,memory-clients = <&mc 0x58>, <&mc 0x59>;
		nvidia,memory-client-names = "read", "write";
		...
	};

This could be useful for latency allowance programming, but we can cross
that bridge when we come to it.

Thierry

Content of type "application/pgp-signature" skipped