Lucas Stach
2014-Dec-24 13:23 UTC
[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence
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().> +{ > + struct of_phandle_args args; > + struct platform_device *pdev; > + int ret; > + > + ret = of_parse_phandle_with_fixed_args(dev->of_node, "mc", > + 1, 0, &args); > + if (ret) > + return ret; > + > + pdev = of_find_device_by_node(args.np); > + if (!pdev) > + return -EINVAL;This is wrong, you need to handle -EPROBE_DEFER here.> + > + *mc = platform_get_drvdata(pdev); > + if (!*mc) > + return -EINVAL; > + > + *swgroup = args.args[0]; > + > + return 0; > +} > + > static int nouveau_platform_probe(struct platform_device *pdev) > { > struct nouveau_platform_gpu *gpu; > @@ -118,6 +156,10 @@ static int nouveau_platform_probe(struct platform_device *pdev) > if (IS_ERR(gpu->clk_pwr)) > return PTR_ERR(gpu->clk_pwr); > > + err = nouveau_platform_get_mc(&pdev->dev, &gpu->mc, &gpu->swgroup); > + if (err) > + return err; > + > err = nouveau_platform_power_up(gpu); > if (err) > return err; > diff --git a/drm/nouveau_platform.h b/drm/nouveau_platform.h > index 58c28b5653d5..969dbddd786d 100644 > --- a/drm/nouveau_platform.h > +++ b/drm/nouveau_platform.h > @@ -35,6 +35,9 @@ struct nouveau_platform_gpu { > struct clk *clk_pwr; > > struct regulator *vdd; > + > + struct tegra_mc *mc; > + unsigned int swgroup; > }; > > struct nouveau_platform_device {
Vince Hsu
2014-Dec-25 02:42 UTC
[Nouveau] [PATCH nouveau 06/11] platform: complete the power up/down sequence
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> >> +{ >> + struct of_phandle_args args; >> + struct platform_device *pdev; >> + int ret; >> + >> + ret = of_parse_phandle_with_fixed_args(dev->of_node, "mc", >> + 1, 0, &args); >> + if (ret) >> + return ret; >> + >> + pdev = of_find_device_by_node(args.np); >> + if (!pdev) >> + return -EINVAL; > This is wrong, you need to handle -EPROBE_DEFER here.Indeed. Will fix.> >> + >> + *mc = platform_get_drvdata(pdev); >> + if (!*mc) >> + return -EINVAL; >> + >> + *swgroup = args.args[0]; >> + >> + return 0; >> +} >> + >> static int nouveau_platform_probe(struct platform_device *pdev) >> { >> struct nouveau_platform_gpu *gpu; >> @@ -118,6 +156,10 @@ static int nouveau_platform_probe(struct platform_device *pdev) >> if (IS_ERR(gpu->clk_pwr)) >> return PTR_ERR(gpu->clk_pwr); >> >> + err = nouveau_platform_get_mc(&pdev->dev, &gpu->mc, &gpu->swgroup); >> + if (err) >> + return err; >> + >> err = nouveau_platform_power_up(gpu); >> if (err) >> return err; >> diff --git a/drm/nouveau_platform.h b/drm/nouveau_platform.h >> index 58c28b5653d5..969dbddd786d 100644 >> --- a/drm/nouveau_platform.h >> +++ b/drm/nouveau_platform.h >> @@ -35,6 +35,9 @@ struct nouveau_platform_gpu { >> struct clk *clk_pwr; >> >> struct regulator *vdd; >> + >> + struct tegra_mc *mc; >> + unsigned int swgroup; >> }; >> >> struct nouveau_platform_device { >
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.htmlI 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>
Maybe Matching Threads
- [PATCH nouveau 06/11] platform: complete the power up/down sequence
- [PATCH nouveau 06/11] platform: complete the power up/down sequence
- [PATCH nouveau 06/11] platform: complete the power up/down sequence
- [PATCH nouveau 06/11] platform: complete the power up/down sequence
- [PATCH nouveau 06/11] platform: complete the power up/down sequence