Hi Roy, On Fri, Nov 28, 2014 at 9:09 PM, Roy Spliet <[email protected]> wrote: > Hello Vince, > > Op 28-11-14 om 12:57 schreef Vince Hsu: > >> Hi Roy, >> >> On 11/28/2014 07:25 PM, Roy Spliet wrote: >>> >>> Hello Vince, >>> >>> One minor question inline. >>> >>> Op 28-11-14 om 12:13 schreef Vince Hsu: >>>> >>>> The voltage value are calculated by the hardware characterized >>>> result. >>>> >>>> Signed-off-by: Vince Hsu <[email protected]> >>>> --- >>>> >>>> Resend this patch with the fuse change and proper patch prefix >>>> per Thierry's request. >>>> >>>> drm/Kbuild | 1 + >>>> drm/core/subdev/volt/gk20a.c | 1 + >>>> nvkm/engine/device/nve0.c | 1 + >>>> nvkm/include/subdev/volt.h | 1 + >>>> nvkm/subdev/clock/gk20a.c | 15 ++++ >>>> nvkm/subdev/volt/gk20a.c | 202 >>>> +++++++++++++++++++++++++++++++++++++++++++ >>>> 6 files changed, 221 insertions(+) >>>> create mode 120000 drm/core/subdev/volt/gk20a.c >>>> create mode 100644 nvkm/subdev/volt/gk20a.c >>>> >>>> diff --git a/drm/Kbuild b/drm/Kbuild >>>> index 728bc5b66b29..7c49e6655066 100644 >>>> --- a/drm/Kbuild >>>> +++ b/drm/Kbuild >>>> @@ -225,6 +225,7 @@ nouveau-y += core/subdev/vm/nvc0.o >>>> nouveau-y += core/subdev/volt/base.o >>>> nouveau-y += core/subdev/volt/gpio.o >>>> nouveau-y += core/subdev/volt/nv40.o >>>> +nouveau-y += core/subdev/volt/gk20a.o >>>> nouveau-y += core/engine/falcon.o >>>> nouveau-y += core/engine/xtensa.o >>>> diff --git a/drm/core/subdev/volt/gk20a.c b/drm/core/subdev/volt/gk20a.c >>>> new file mode 120000 >>>> index 000000000000..2894eb1ede13 >>>> --- /dev/null >>>> +++ b/drm/core/subdev/volt/gk20a.c >>>> @@ -0,0 +1 @@ >>>> +../../../../nvkm/subdev/volt/gk20a.c >>>> \ No newline at end of file >>>> diff --git a/nvkm/engine/device/nve0.c b/nvkm/engine/device/nve0.c >>>> index b1b2e484ecfa..674da1f095b2 100644 >>>> --- a/nvkm/engine/device/nve0.c >>>> +++ b/nvkm/engine/device/nve0.c >>>> @@ -179,6 +179,7 @@ nve0_identify(struct nouveau_device *device) >>>> device->oclass[NVDEV_ENGINE_GR ] = gk20a_graph_oclass; >>>> device->oclass[NVDEV_ENGINE_COPY2 ] = &nve0_copy2_oclass; >>>> device->oclass[NVDEV_ENGINE_PERFMON] = &nve0_perfmon_oclass; >>>> + device->oclass[NVDEV_SUBDEV_VOLT ] = &gk20a_volt_oclass; >>>> break; >>>> case 0xf0: >>>> device->cname = "GK110"; >>>> diff --git a/nvkm/include/subdev/volt.h b/nvkm/include/subdev/volt.h >>>> index 820b62ffd75b..67db5e58880d 100644 >>>> --- a/nvkm/include/subdev/volt.h >>>> +++ b/nvkm/include/subdev/volt.h >>>> @@ -52,6 +52,7 @@ int _nouveau_volt_init(struct nouveau_object *); >>>> #define _nouveau_volt_fini _nouveau_subdev_fini >>>> extern struct nouveau_oclass nv40_volt_oclass; >>>> +extern struct nouveau_oclass gk20a_volt_oclass; >>>> int nouveau_voltgpio_init(struct nouveau_volt *); >>>> int nouveau_voltgpio_get(struct nouveau_volt *); >>>> diff --git a/nvkm/subdev/clock/gk20a.c b/nvkm/subdev/clock/gk20a.c >>>> index 82abbea2be12..fb4fad374bdd 100644 >>>> --- a/nvkm/subdev/clock/gk20a.c >>>> +++ b/nvkm/subdev/clock/gk20a.c >>>> @@ -470,76 +470,91 @@ gk20a_pstates[] = { >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 72000, >>>> + .voltage = 0, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 108000, >>>> + .voltage = 1, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 180000, >>>> + .voltage = 2, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 252000, >>>> + .voltage = 3, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 324000, >>>> + .voltage = 4, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 396000, >>>> + .voltage = 5, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 468000, >>>> + .voltage = 6, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 540000, >>>> + .voltage = 7, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 612000, >>>> + .voltage = 8, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 648000, >>>> + .voltage = 9, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 684000, >>>> + .voltage = 10, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 708000, >>>> + .voltage = 11, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 756000, >>>> + .voltage = 12, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 804000, >>>> + .voltage = 13, >>>> }, >>>> }, >>>> { >>>> .base = { >>>> .domain[nv_clk_src_gpc] = 852000, >>>> + .voltage = 14, >>>> }, >>>> }, >>>> }; >>> >>> >>> Is there a particular reason why this table is hard-coded rather than >>> stored in the device tree? It doesn't seem to differ much between different >>> gk20a's (but this might change with the denver-core version?), but I do >>> anticipate a lot of "code" duplication when post-K1 cores are released and >>> supported. >> >> Hmmm.. That's probably because I just realized we have some other example >> like the bios subdev is using the device tress stuff. The table is chip >> specific, not board specific. Should we put it in device tree? >> And the table should be the same between all the gk20a's although there is >> no guarantee. If the post-K1 cores also integrate the gk20a, they should use >> the same driver including this file. If not, I believe we will have some >> clever way to handle that. That's unlikely to happen though. :) >> >> Thanks, >> Vince > > > I'm not sure if I completely understand your reply, so forgive me if I am > stating some obvious things: > The reason why I brought this up is because, the way I see it, DTS is the > replacement for (V)BIOS on ARM platforms, giving a set of parameters that > drivers (nouveau) can use for that particular instance (the Tegra K1 SoC) of > some more generic IP (gk20a). All the other devices nouveau supports have a > VBIOS to describe this kind of information to us, hence we haven't seen this > before. For CPUs there are plenty of examples though of such params defined > in DT: in arch/arm/boot/dts/ : imx6qdl.dtsi documents the min and max volt > for regulators, while the CPUs have a little freq<->volt mapping in > imx6q.dtsi. GPUs are new in a sense that NVIDIA is the first to actively > support upstream development (thanks!) > Secondly, we should keep in mind that DT is not tied to Linux; I believe > Linaro's long term goal is to take the DT from the Linux tree and maintain > it as a separate tree, to be used with U-boot, *BSD, maybe even Windows. > These kind of parameters are not very platform-dependent and although they > seem like a little detail that's easy to reproduce on every platform, > looking at the sheer size of the VBIOS data that could mean a *lot* of > duplication. > I bring this up not because I think I know better, but rather because I > believe it's a good discussion to have now that there still is little legacy > on ARM SoCs in nouveau. DTS is still a work-in-progress, and at this moment > we have the opportunity to consider what needs to be documented in there and > what doesn't. I would actually love to hear other, more experienced > developers chime in as well (Rob Clark? Ben Skeggs? Linaro folks?) and see > how they feel.
Thanks for raising this point. I agree with your interpretation that DT is comparable to the VBIOS in desktop GPUs. The question then becomes whether this data can vary between different GK20A-using boards (and in this case this should probably be part of DT) or not (in which case I would advocate having this static information in the driver itself). Since I don't expect different GK20A-using chips to require different voltage for given frequencies, my gut feeling for the moment is that having this information in the driver is fine. I have added a few other NVIDIA people to gather thoughts. IIUC this question also applies to other tables in this patch that depend on frequency, like gk20a_cvb_coef. Huge tables of data are generally frowned-upon by the DT folks, but there are already examples of this (see for instance http://article.gmane.org/gmane.linux.kernel/1831719 ). _______________________________________________ Nouveau mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/nouveau
