[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Alexandre Courbot]

On Mon, Apr 14, 2014 at 5:35 PM, Ben Skeggs  wrote:
> On Fri, Apr 11, 2014 at 5:34 PM, Alexandre Courbot  
> wrote:
>> On 04/11/2014 04:31 PM, Ben Skeggs wrote:
>>>
>>> On Fri, Apr 11, 2014 at 12:46 PM, Alexandre Courbot 
>>> wrote:

 On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs  wrote:
>
> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
>  wrote:
>>
>> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
>>>
>>> GK20A's timer is directly attached to the system timer and cannot be
>>> calibrated. Skip the calibration phase on that chip since the
>>> corresponding registers do not exist.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>   drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19
>>> +--
>>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> index c0bdd10358d7..822fe0d8a871 100644
>>> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>>>if (ret)
>>>return ret;
>>>
>>> + /* gk20a does not have the calibration registers */
>>> + if (device->chipset == 0xea)
>>> + goto skip_clk_init;
>>
>>
>> I'm concerned that this won't scale in the future. Perhaps a better
>> solution would be to add a "flags" or "features" field to struct
>> nouveau_device along with feature bits such as HAS_CALIBRATION or
>> similar.
>>
>> That way we don't have to touch this code for every new future Tegra
>> chip. Unless perhaps if there's a reason to expect things to change in
>> newer generations.
>
> I've already handled this in a slightly different way in the tree I'd
> previously pointed Alex at (I think!), as I needed to do the same for
> GM107.
>
> Should just be able to use that implementation (so, just change the
> probe patch) here too.


 I will skip this patch and use your implementation then. Btw,
 shouldn't the source file for the GK20A implementation be named nvea.c
 instead of gk20a.c?
>>>
>>> For the Maxwell stuff I've been using "gm107" now too.  Since we're
>>> working with you guys these days it seems better to use the same names
>>> for things ;)
>>
>>
>> So would you like us to use the same naming scheme as well? So far all my
>> patches use "nvea.c" whenever I need to add code.
> If it's not too much of a problem at this point, then that'd be good.
> Right before I send -next for the next merge window I'll likely do a
> mass rename anyway, so if we can get your patches merged before then
> (which would be really good!), it doesn't matter much.

No problem, I will update the naming to follow what you did with the
timer driver and gm107.

Hopefully I will soon manage to carve out some time to rebase these
patches and send v2.

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Ben Skeggs]

On Fri, Apr 11, 2014 at 5:34 PM, Alexandre Courbot  
wrote:
> On 04/11/2014 04:31 PM, Ben Skeggs wrote:
>>
>> On Fri, Apr 11, 2014 at 12:46 PM, Alexandre Courbot 
>> wrote:
>>>
>>> On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs  wrote:

 On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
  wrote:
>
> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
>>
>> GK20A's timer is directly attached to the system timer and cannot be
>> calibrated. Skip the calibration phase on that chip since the
>> corresponding registers do not exist.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>   drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19
>> +--
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> index c0bdd10358d7..822fe0d8a871 100644
>> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>>if (ret)
>>return ret;
>>
>> + /* gk20a does not have the calibration registers */
>> + if (device->chipset == 0xea)
>> + goto skip_clk_init;
>
>
> I'm concerned that this won't scale in the future. Perhaps a better
> solution would be to add a "flags" or "features" field to struct
> nouveau_device along with feature bits such as HAS_CALIBRATION or
> similar.
>
> That way we don't have to touch this code for every new future Tegra
> chip. Unless perhaps if there's a reason to expect things to change in
> newer generations.

 I've already handled this in a slightly different way in the tree I'd
 previously pointed Alex at (I think!), as I needed to do the same for
 GM107.

 Should just be able to use that implementation (so, just change the
 probe patch) here too.
>>>
>>>
>>> I will skip this patch and use your implementation then. Btw,
>>> shouldn't the source file for the GK20A implementation be named nvea.c
>>> instead of gk20a.c?
>>
>> For the Maxwell stuff I've been using "gm107" now too.  Since we're
>> working with you guys these days it seems better to use the same names
>> for things ;)
>
>
> So would you like us to use the same naming scheme as well? So far all my
> patches use "nvea.c" whenever I need to add code.
If it's not too much of a problem at this point, then that'd be good.
Right before I send -next for the next merge window I'll likely do a
mass rename anyway, so if we can get your patches merged before then
(which would be really good!), it doesn't matter much.

Ben.

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Ben Skeggs]

On Fri, Apr 11, 2014 at 12:46 PM, Alexandre Courbot  wrote:
> On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs  wrote:
>> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
>>  wrote:
>>> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
 GK20A's timer is directly attached to the system timer and cannot be
 calibrated. Skip the calibration phase on that chip since the
 corresponding registers do not exist.

 Signed-off-by: Alexandre Courbot 
 ---
  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 +--
  1 file changed, 13 insertions(+), 6 deletions(-)

 diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c 
 b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
 index c0bdd10358d7..822fe0d8a871 100644
 --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
 +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
 @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
   if (ret)
   return ret;

 + /* gk20a does not have the calibration registers */
 + if (device->chipset == 0xea)
 + goto skip_clk_init;
>>>
>>> I'm concerned that this won't scale in the future. Perhaps a better
>>> solution would be to add a "flags" or "features" field to struct
>>> nouveau_device along with feature bits such as HAS_CALIBRATION or
>>> similar.
>>>
>>> That way we don't have to touch this code for every new future Tegra
>>> chip. Unless perhaps if there's a reason to expect things to change in
>>> newer generations.
>> I've already handled this in a slightly different way in the tree I'd
>> previously pointed Alex at (I think!), as I needed to do the same for
>> GM107.
>>
>> Should just be able to use that implementation (so, just change the
>> probe patch) here too.
>
> I will skip this patch and use your implementation then. Btw,
> shouldn't the source file for the GK20A implementation be named nvea.c
> instead of gk20a.c?
For the Maxwell stuff I've been using "gm107" now too.  Since we're
working with you guys these days it seems better to use the same names
for things ;)

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Alexandre Courbot]

On 04/11/2014 04:31 PM, Ben Skeggs wrote:
> On Fri, Apr 11, 2014 at 12:46 PM, Alexandre Courbot  
> wrote:
>> On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs  wrote:
>>> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
>>>  wrote:
 On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
> GK20A's timer is directly attached to the system timer and cannot be
> calibrated. Skip the calibration phase on that chip since the
> corresponding registers do not exist.
>
> Signed-off-by: Alexandre Courbot 
> ---
>   drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 
> +--
>   1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c 
> b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> index c0bdd10358d7..822fe0d8a871 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>if (ret)
>return ret;
>
> + /* gk20a does not have the calibration registers */
> + if (device->chipset == 0xea)
> + goto skip_clk_init;

 I'm concerned that this won't scale in the future. Perhaps a better
 solution would be to add a "flags" or "features" field to struct
 nouveau_device along with feature bits such as HAS_CALIBRATION or
 similar.

 That way we don't have to touch this code for every new future Tegra
 chip. Unless perhaps if there's a reason to expect things to change in
 newer generations.
>>> I've already handled this in a slightly different way in the tree I'd
>>> previously pointed Alex at (I think!), as I needed to do the same for
>>> GM107.
>>>
>>> Should just be able to use that implementation (so, just change the
>>> probe patch) here too.
>>
>> I will skip this patch and use your implementation then. Btw,
>> shouldn't the source file for the GK20A implementation be named nvea.c
>> instead of gk20a.c?
> For the Maxwell stuff I've been using "gm107" now too.  Since we're
> working with you guys these days it seems better to use the same names
> for things ;)

So would you like us to use the same naming scheme as well? So far all 
my patches use "nvea.c" whenever I need to add code.

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Alexandre Courbot]

On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs  wrote:
> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
>  wrote:
>> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
>>> GK20A's timer is directly attached to the system timer and cannot be
>>> calibrated. Skip the calibration phase on that chip since the
>>> corresponding registers do not exist.
>>>
>>> Signed-off-by: Alexandre Courbot 
>>> ---
>>>  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 +--
>>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c 
>>> b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> index c0bdd10358d7..822fe0d8a871 100644
>>> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>>> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>>>   if (ret)
>>>   return ret;
>>>
>>> + /* gk20a does not have the calibration registers */
>>> + if (device->chipset == 0xea)
>>> + goto skip_clk_init;
>>
>> I'm concerned that this won't scale in the future. Perhaps a better
>> solution would be to add a "flags" or "features" field to struct
>> nouveau_device along with feature bits such as HAS_CALIBRATION or
>> similar.
>>
>> That way we don't have to touch this code for every new future Tegra
>> chip. Unless perhaps if there's a reason to expect things to change in
>> newer generations.
> I've already handled this in a slightly different way in the tree I'd
> previously pointed Alex at (I think!), as I needed to do the same for
> GM107.
>
> Should just be able to use that implementation (so, just change the
> probe patch) here too.

I will skip this patch and use your implementation then. Btw,
shouldn't the source file for the GK20A implementation be named nvea.c
instead of gk20a.c?

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Ben Skeggs]

On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
 wrote:
> On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
>> GK20A's timer is directly attached to the system timer and cannot be
>> calibrated. Skip the calibration phase on that chip since the
>> corresponding registers do not exist.
>>
>> Signed-off-by: Alexandre Courbot 
>> ---
>>  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 +--
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c 
>> b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> index c0bdd10358d7..822fe0d8a871 100644
>> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
>> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>>   if (ret)
>>   return ret;
>>
>> + /* gk20a does not have the calibration registers */
>> + if (device->chipset == 0xea)
>> + goto skip_clk_init;
>
> I'm concerned that this won't scale in the future. Perhaps a better
> solution would be to add a "flags" or "features" field to struct
> nouveau_device along with feature bits such as HAS_CALIBRATION or
> similar.
>
> That way we don't have to touch this code for every new future Tegra
> chip. Unless perhaps if there's a reason to expect things to change in
> newer generations.
I've already handled this in a slightly different way in the tree I'd
previously pointed Alex at (I think!), as I needed to do the same for
GM107.

Should just be able to use that implementation (so, just change the
probe patch) here too.

Thanks,
Ben.

>
> Thierry
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Thierry Reding]

On Mon, Mar 24, 2014 at 05:42:24PM +0900, Alexandre Courbot wrote:
> GK20A's timer is directly attached to the system timer and cannot be
> calibrated. Skip the calibration phase on that chip since the
> corresponding registers do not exist.
> 
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c 
> b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> index c0bdd10358d7..822fe0d8a871 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
> @@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
>   if (ret)
>   return ret;
>  
> + /* gk20a does not have the calibration registers */
> + if (device->chipset == 0xea)
> + goto skip_clk_init;

I'm concerned that this won't scale in the future. Perhaps a better
solution would be to add a "flags" or "features" field to struct
nouveau_device along with feature bits such as HAS_CALIBRATION or
similar.

That way we don't have to touch this code for every new future Tegra
chip. Unless perhaps if there's a reason to expect things to change in
newer generations.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

[Alexandre Courbot]

GK20A's timer is directly attached to the system timer and cannot be
calibrated. Skip the calibration phase on that chip since the
corresponding registers do not exist.

Signed-off-by: Alexandre Courbot 
---
 drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c 
b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
index c0bdd10358d7..822fe0d8a871 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/timer/nv04.c
@@ -185,6 +185,10 @@ nv04_timer_init(struct nouveau_object *object)
if (ret)
return ret;

+   /* gk20a does not have the calibration registers */
+   if (device->chipset == 0xea)
+   goto skip_clk_init;
+
/* aim for 31.25MHz, which gives us nanosecond timestamps */
d = 100 / 32;

@@ -235,20 +239,23 @@ nv04_timer_init(struct nouveau_object *object)
d >>= 1;
}

-   /* restore the time before suspend */
-   lo = priv->suspend_time;
-   hi = (priv->suspend_time >> 32);
-
nv_debug(priv, "input frequency : %dHz\n", f);
nv_debug(priv, "input multiplier: %d\n", m);
nv_debug(priv, "numerator   : 0x%08x\n", n);
nv_debug(priv, "denominator : 0x%08x\n", d);
nv_debug(priv, "timer frequency : %dHz\n", (f * m) * d / n);
-   nv_debug(priv, "time low: 0x%08x\n", lo);
-   nv_debug(priv, "time high   : 0x%08x\n", hi);

nv_wr32(priv, NV04_PTIMER_NUMERATOR, n);
nv_wr32(priv, NV04_PTIMER_DENOMINATOR, d);
+
+skip_clk_init:
+   /* restore the time before suspend */
+   lo = priv->suspend_time;
+   hi = (priv->suspend_time >> 32);
+
+   nv_debug(priv, "time low: 0x%08x\n", lo);
+   nv_debug(priv, "time high   : 0x%08x\n", hi);
+
nv_wr32(priv, NV04_PTIMER_INTR_0, 0x);
nv_wr32(priv, NV04_PTIMER_INTR_EN_0, 0x);
nv_wr32(priv, NV04_PTIMER_TIME_1, hi);
-- 
1.9.1