Nouveau - Apr 2014 - [PATCH 02/12] drm/nouveau/timer: skip calibration on GK20A

On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
> <thierry.reding at gmail.com> 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 <acourbot at nvidia.com>
>>> ---
>>>  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?

On Fri, Apr 11, 2014 at 12:46 PM, Alexandre Courbot <gnurou at gmail.com>
wrote:
> On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs <skeggsb at gmail.com>
wrote:
>> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
>> <thierry.reding at gmail.com> 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 <acourbot at nvidia.com>
>>>> ---
>>>>  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 ;)

On 04/11/2014 04:31 PM, Ben Skeggs wrote:
> On Fri, Apr 11, 2014 at 12:46 PM, Alexandre Courbot <gnurou at
gmail.com> wrote:
>> On Wed, Mar 26, 2014 at 1:19 PM, Ben Skeggs <skeggsb at
gmail.com> wrote:
>>> On Tue, Mar 25, 2014 at 7:54 AM, Thierry Reding
>>> <thierry.reding at gmail.com> 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 <acourbot at
nvidia.com>
>>>>> ---
>>>>>   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.