thr3ads.net - Nouveau - [Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence [Jan 2015]

If this information is useful, please help other people find it:
Share via:

Thierry Reding

2015-Jan-05 15:25 UTC

[Nouveau] [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 at nvidia.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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150105/12ed85fd/attachment-0001.sig>

Vince Hsu

2015-Jan-06 09:34 UTC

head link

[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence

On 01/05/2015 11:25 PM, Thierry Reding wrote:> * PGP Signed by an unknown key
>
> 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 at nvidia.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 at 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.Based on your suggestion above, I created a struct tegra_mc_swgroup:

struct tegra_mc_swgroup {
     unsigned int id;
     struct tegra_mc *mc;
     struct list_head head;
     struct list_head clients;
};

And added the list head in the struct tegra_mc_soc.

struct tegra_mc_soc {
     struct tegra_mc_client *clients;
     unsigned int num_clients;

     struct tegra_mc_hr *hr_clients;
     unsigned int num_hr_clients;

     struct list_head swgroups;
...

Created one function to build the swgroup list.

static int tegra_mc_build_swgroup(struct tegra_mc *mc)
{
     int i;

     for (i = 0; i < mc->soc->num_clients; i++) {
         struct tegra_mc_swgroup *sg;
         bool found = false;

         list_for_each_entry(sg, &mc->soc->swgroups, head) {
             if (sg->id == mc->soc->clients[i].swgroup) {
                 found = true;
                 break;
             }
         }

         if (!found) {
             sg = devm_kzalloc(mc->dev, sizeof(*sg), GFP_KERNEL);
             if (!sg)
                 return -ENOMEM;

             sg->id = mc->soc->clients[i].swgroup;
             sg->mc = mc;
             list_add_tail(&sg->head, &mc->soc->swgroups);
             INIT_LIST_HEAD(&sg->clients);
         }

         list_add_tail(&mc->soc->clients[i].head,
&sg->clients);
     }

     return 0;
}
>
> 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);And then I can use the tegra_find_swgroup() in GK20A driver to get the 
swgroup
and flush the memory clients by tegra_mc_flush(swgroup).

One problem is that the mc_soc and mc_clients are are defined as const. 
To build
the swgroup list dynamically, I have to discard the const. I guess you 
won't like that. :(

Thanks,
Vince
> 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 at 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
>
> * Unknown Key
> * 0x7F3EB3A1

Thierry Reding

2015-Jan-06 11:36 UTC

head link

[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence

On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu
wrote:> 
> On 01/05/2015 11:25 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >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 at nvidia.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 at 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.
> Based on your suggestion above, I created a struct tegra_mc_swgroup:
> 
> struct tegra_mc_swgroup {
>     unsigned int id;
>     struct tegra_mc *mc;
>     struct list_head head;
>     struct list_head clients;
> };
> 
> And added the list head in the struct tegra_mc_soc.
> 
> struct tegra_mc_soc {
>     struct tegra_mc_client *clients;
>     unsigned int num_clients;
> 
>     struct tegra_mc_hr *hr_clients;
>     unsigned int num_hr_clients;
Why do you still need these?
>     struct list_head swgroups;
This doesn't belong in struct tegra_mc_soc because that's meant to be
static information about the specific variant of the memory-controller.
Put it in struct tegra_mc instead.
> ...
> 
> Created one function to build the swgroup list.
> 
> static int tegra_mc_build_swgroup(struct tegra_mc *mc)
> {
>     int i;
> 
>     for (i = 0; i < mc->soc->num_clients; i++) {
>         struct tegra_mc_swgroup *sg;
>         bool found = false;
> 
>         list_for_each_entry(sg, &mc->soc->swgroups, head) {
>             if (sg->id == mc->soc->clients[i].swgroup) {
>                 found = true;
>                 break;
>             }
>         }
Can't you use your new tegra_mc_find_swgroup() function here? That way
you could turn it into something slightly more readable:

	unsigned int swgroup = mc->soc->clients[i].swgroup;
	struct tegra_mc_swgroup *group;

	group = tegra_mc_find_swgroup(mc, swgroup);
	if (!group) {
		/* allocates and adds to mc->swgroups */
		group = tegra_mc_add_swgroup(mc, swgroup);
		if (!group)
			return -ENOMEM;
	}

	list_add_tail(&group->list, &mc->swgroups);

where tegra_mc_add_swgroup() is something like this:

	group = devm_kzalloc(mc->dev, sizeof(*group), GFP_KERNEL);
	if (!group)
		return NULL;

	INIT_LIST_HEAD(&group->clients);
	group->mc = mc;
	group->id = id;

I don't like very much how this duplicates information that is already
available in tegra_mc_soc, but I can't think of a better way to couple
the SWGROUP ID with the struct tegra_mc *, so I think we'll have to
proceed with something like the above.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150106/aa8526ed/attachment.sig>

Apparently Analagous Threads

Search for more apparently analagous threads

Nouveau - Jan 2015 - [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence

Apparently Analagous Threads