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-06 13:55 UTC

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

On Tue, Jan 06, 2015 at 08:13:01PM +0800, Vince Hsu
wrote:> 
> On 01/06/2015 07:36 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> >>On 01/05/2015 11:25 PM, Thierry Reding wrote:
> >>>>Old 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?
> This part is added in "[PATCH 2/11] memory: tegra: add mc flush
support" and
> "[PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory
> clients"
> for the hotreset registers.
Right. Perhaps calling this structure tegra_mc_hotreset and the
variables *_hotresets would be a good idea to clarify that they aren't
actually per-client but per-SWGROUP.
> >>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:
> The tegra_mc_build_swgroup() is called during driver probe, and after that
> the swgroup list is established and then the tegra_mc_find_swgroup() can
> work.
But you can reuse tegra_mc_find_swgroup() instead of open-coding it to
search the partial list. The code will be exactly the same, won't it?
> >	unsigned int swgroup = mc->soc->clients[i].swgroup;
> >	struct tegra_mc_swgroup *group;
> >
> >	group = tegra_mc_find_swgroup(mc, swgroup);
> The tegra_mc_find_swgroup() is expected to be called outside the mc driver
> and
> can only take the device node as argument because other drivers do not have
> the mc instance information unless they call tegra_mc_find_swgroup() or
> tegra_mc_find_client().
You can do that by making a low-level function that works on the struct
tegra_mc * and an unsigned int swgroup which is only used internally and
then wrap a public function around it that deals with looking up the
struct tegra_mc * and SWGROUP ID.

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/5ec67e55/attachment-0001.sig>

Vince Hsu

2015-Jan-06 14:19 UTC

head link

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

On 02:55:52PM Jan 06, Thierry Reding wrote:> * PGP Signed by an unknown key
> 
> On Tue, Jan 06, 2015 at 08:13:01PM +0800, Vince Hsu wrote:
> > 
> > On 01/06/2015 07:36 PM, Thierry Reding wrote:
> > >> Old Signed by an unknown key
> > >
> > >On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> > >>On 01/05/2015 11:25 PM, Thierry Reding wrote:
> > >>>>Old 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?
> > This part is added in "[PATCH 2/11] memory: tegra: add mc flush
support" and
> > "[PATCH 3/11] memory: tegra: add flush operation for Tegra124
memory
> > clients"
> > for the hotreset registers.
> 
> Right. Perhaps calling this structure tegra_mc_hotreset and the
> variables *_hotresets would be a good idea to clarify that they aren't
> actually per-client but per-SWGROUP.Okay.
> 
> > >>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:
> > The tegra_mc_build_swgroup() is called during driver probe, and after
that
> > the swgroup list is established and then the tegra_mc_find_swgroup()
can
> > work.
> 
> But you can reuse tegra_mc_find_swgroup() instead of open-coding it to
> search the partial list. The code will be exactly the same, won't it?That depends on how you define the tegra_mc_find_swgroup().
> 
> > >	unsigned int swgroup = mc->soc->clients[i].swgroup;
> > >	struct tegra_mc_swgroup *group;
> > >
> > >	group = tegra_mc_find_swgroup(mc, swgroup);
> > The tegra_mc_find_swgroup() is expected to be called outside the mc
driver
> > and
> > can only take the device node as argument because other drivers do not
have
> > the mc instance information unless they call tegra_mc_find_swgroup()
or
> > tegra_mc_find_client().
> 
> You can do that by making a low-level function that works on the struct
> tegra_mc * and an unsigned int swgroup which is only used internally and
> then wrap a public function around it that deals with looking up the
> struct tegra_mc * and SWGROUP ID.Now the tegra_mc_find_swgroup() looks like below:

struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node)
{
    struct of_phandle_args args;
    struct platform_device *pdev;
    struct tegra_mc *mc;
    struct tegra_mc_swgroup *sg;
    int ret;

    ret = of_parse_phandle_with_fixed_args(node, "nvidia,swgroup",
                1, 0, &args);
    if (ret)
        return ERR_PTR(ret);

    pdev = of_find_device_by_node(args.np);
    if (!pdev)
        return NULL;

    mc = platform_get_drvdata(pdev);
    if (!mc)
        return NULL;

    list_for_each_entry(sg, &mc->soc->swgroups, head) { 
        if (sg->id == args.args[0])
            return sg;
    }

    return NULL;
}
EXPORT_SYMBOL(tegra_mc_find_swgroup);     

I will add a find_swgroup() as internal funcion and add the add_swgroup()
per your suggestion. And keep the tegra_mc_find_swgroup() public. Is that
okay for you?

Thanks,
Vince

Thierry Reding

2015-Jan-06 14:24 UTC

head link

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

On Tue, Jan 06, 2015 at 10:19:49PM +0800, Vince Hsu
wrote:> On 02:55:52PM Jan 06, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jan 06, 2015 at 08:13:01PM +0800, Vince Hsu wrote:
> > > 
> > > On 01/06/2015 07:36 PM, Thierry Reding wrote:
> > > >> Old Signed by an unknown key
> > > >
> > > >On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> > > >>On 01/05/2015 11:25 PM, Thierry Reding wrote:
> > > >>>>Old 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?
> > > This part is added in "[PATCH 2/11] memory: tegra: add mc
flush support" and
> > > "[PATCH 3/11] memory: tegra: add flush operation for
Tegra124 memory
> > > clients"
> > > for the hotreset registers.
> > 
> > Right. Perhaps calling this structure tegra_mc_hotreset and the
> > variables *_hotresets would be a good idea to clarify that they
aren't
> > actually per-client but per-SWGROUP.
> Okay.
> 
> > 
> > > >>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:
> > > The tegra_mc_build_swgroup() is called during driver probe, and
after that
> > > the swgroup list is established and then the
tegra_mc_find_swgroup() can
> > > work.
> > 
> > But you can reuse tegra_mc_find_swgroup() instead of open-coding it to
> > search the partial list. The code will be exactly the same, won't
it?
> That depends on how you define the tegra_mc_find_swgroup().
> 
> > 
> > > >	unsigned int swgroup = mc->soc->clients[i].swgroup;
> > > >	struct tegra_mc_swgroup *group;
> > > >
> > > >	group = tegra_mc_find_swgroup(mc, swgroup);
> > > The tegra_mc_find_swgroup() is expected to be called outside the
mc driver
> > > and
> > > can only take the device node as argument because other drivers
do not have
> > > the mc instance information unless they call
tegra_mc_find_swgroup() or
> > > tegra_mc_find_client().
> > 
> > You can do that by making a low-level function that works on the
struct
> > tegra_mc * and an unsigned int swgroup which is only used internally
and
> > then wrap a public function around it that deals with looking up the
> > struct tegra_mc * and SWGROUP ID.
> Now the tegra_mc_find_swgroup() looks like below:
> 
> struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node)
> {
>     struct of_phandle_args args;
>     struct platform_device *pdev;
>     struct tegra_mc *mc;
>     struct tegra_mc_swgroup *sg;
>     int ret;
>
>     ret = of_parse_phandle_with_fixed_args(node,
"nvidia,swgroup",
>                 1, 0, &args);
>     if (ret)
>         return ERR_PTR(ret);
>
>     pdev = of_find_device_by_node(args.np);
>     if (!pdev)
>         return NULL;
>
>     mc = platform_get_drvdata(pdev);
>     if (!mc)
>         return NULL;
>
>     list_for_each_entry(sg, &mc->soc->swgroups, head) { 
>         if (sg->id == args.args[0])
>             return sg;
>     }
>
>     return NULL;
> }
> EXPORT_SYMBOL(tegra_mc_find_swgroup);     
> 
> I will add a find_swgroup() as internal funcion and add the add_swgroup()
> per your suggestion. And keep the tegra_mc_find_swgroup() public. Is that
> okay for you?
Sounds good.

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/ca503d8e/attachment-0001.sig>

Possibly Parallel Threads

Search for more possibly parallel 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

Possibly Parallel Threads