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

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.

On Fri, Apr 11, 2014 at 5:34 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
> 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.
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.

On Mon, Apr 14, 2014 at 5:35 PM, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On Fri, Apr 11, 2014 at 5:34 PM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
>> 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.
> 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.