[PATCH 1/2] drm/nouveau/bios/therm: handle vbioses with duplicate entries (mostly nva5)

[PATCH 1/2] drm/nouveau/bios/therm: handle vbioses with duplicate entries (mostly nva5)

[Martin Peres]

From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>

Some vbioses have extra useless entries after "the end" of the table. This is
problematic since all of the vbios I found with this issue redefine the
pwm freq divider to insane levels (52750 Hz instead of 2500), thus breaking
fan management.

The first solution to solve this mess would be to change the length of the
table. The solution I choose was simply to avoid setting the pwm freq twice
as the other redefinitions are harmless with our current parser.

Signed-off-by: Martin Peres <martin.peres-/ze9ptdxDJdQFI55V6+gNQ@public.gmane.org>
Reported-by: Mariusz Bialonczyk <manio-RFXG7K41k5GsTnJN9+BGXg@public.gmane.org>
Tested-by: Mariusz Bialonczyk <manio-RFXG7K41k5GsTnJN9+BGXg@public.gmane.org>
---
 drivers/gpu/drm/nouveau/core/subdev/bios/therm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c b/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c
index 22a2057..22ac6db 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c
@@ -184,7 +184,8 @@ nvbios_therm_fan_parse(struct nouveau_bios *bios,
 			cur_trip->fan_duty = value;
 			break;
 		case 0x26:
-			fan->pwm_freq = value;
+			if (!fan->pwm_freq)
+				fan->pwm_freq = value;
 			break;
 		case 0x3b:
 			fan->bump_period = value;
-- 
1.8.4

[PATCH 2/2] drm/nv84/therm: ack any pending IRQ at init

[Martin Peres]

From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>

This is safe because ptherm hasn't been configured yet and will be a
little further down the initialization path. Ptherm should be safe
regarding to runtime reconfiguration.

Signed-off-by: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
---
 drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
index 42ba633..8615d62 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
@@ -209,13 +209,30 @@ nv84_therm_ctor(struct nouveau_object *parent,
 	return nouveau_therm_preinit(&priv->base.base);
 }
 
+static int
+nv84_therm_init(struct nouveau_object *object)
+{
+	struct nv84_therm_priv *priv = (void *)object;
+	int ret;
+
+	ret = nouveau_therm_init(&priv->base.base);
+	if (ret)
+		return ret;
+
+	/* ACK ptherm IRQs */
+	nv_wr32(object, 0x20100, 0xffffffff);
+	nv_wr32(object, 0x1100, 0x10000); /* PBUS */
+
+	return 0;
+}
+
 struct nouveau_oclass
 nv84_therm_oclass = {
 	.handle = NV_SUBDEV(THERM, 0x84),
 	.ofuncs = &(struct nouveau_ofuncs) {
 		.ctor = nv84_therm_ctor,
 		.dtor = _nouveau_therm_dtor,
-		.init = _nouveau_therm_init,
+		.init = nv84_therm_init,
 		.fini = _nouveau_therm_fini,
 	},
 };
-- 
1.8.4

[PATCH] drm/nouveau/therm: ack any pending IRQ at init v2

[Martin Peres]

From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>

This is safe because ptherm hasn't been configured yet and will be a
little further down the initialization path. Ptherm should be safe
regarding to runtime reconfiguration.

v2:
- do not limit this patch to nv84-a3 and make it nv84+

Signed-off-by: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
---
 drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c | 19 ++++++++++++++++++-
 drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c |  4 ++++
 drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c |  4 ++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
index 42ba633..8615d62 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
@@ -209,13 +209,30 @@ nv84_therm_ctor(struct nouveau_object *parent,
 	return nouveau_therm_preinit(&priv->base.base);
 }
 
+static int
+nv84_therm_init(struct nouveau_object *object)
+{
+	struct nv84_therm_priv *priv = (void *)object;
+	int ret;
+
+	ret = nouveau_therm_init(&priv->base.base);
+	if (ret)
+		return ret;
+
+	/* ACK ptherm IRQs */
+	nv_wr32(object, 0x20100, 0xffffffff);
+	nv_wr32(object, 0x1100, 0x10000); /* PBUS */
+
+	return 0;
+}
+
 struct nouveau_oclass
 nv84_therm_oclass = {
 	.handle = NV_SUBDEV(THERM, 0x84),
 	.ofuncs = &(struct nouveau_ofuncs) {
 		.ctor = nv84_therm_ctor,
 		.dtor = _nouveau_therm_dtor,
-		.init = _nouveau_therm_init,
+		.init = nv84_therm_init,
 		.fini = _nouveau_therm_fini,
 	},
 };
diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c
index d11a7c4..f740eeb 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c
@@ -60,6 +60,10 @@ nva3_therm_init(struct nouveau_object *object)
 	}
 	nv_mask(priv, 0x00e720, 0x00000002, 0x00000000);
 
+	/* ACK ptherm IRQs */
+	nv_wr32(object, 0x20100, 0xffffffff);
+	nv_wr32(object, 0x1100, 0x10000); /* PBUS */
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c
index 54c28bd..d9b5103 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c
@@ -114,6 +114,10 @@ nvd0_therm_init(struct nouveau_object *object)
 	}
 	nv_mask(priv, 0x00e720, 0x00000002, 0x00000000);
 
+	/* ACK ptherm IRQs */
+	nv_wr32(object, 0x20100, 0xffffffff);
+	nv_wr32(object, 0x1100, 0x10000); /* PBUS */
+
 	return 0;
 }
 
-- 
1.8.4

Re: [PATCH] drm/nouveau/therm: ack any pending IRQ at init v2

[Ben Skeggs]

On Sat, Aug 31, 2013 at 10:06 AM, Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> wrote:
> From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
>
> This is safe because ptherm hasn't been configured yet and will be a
> little further down the initialization path. Ptherm should be safe
> regarding to runtime reconfiguration.
Any objections to instead sticking the ACK in a single fini() hook
(along with additional code to disable all therm interrupts), and
having the IRQ handle silently ignore any non-requested IRQs (see what
the copy engine irq handler does, for example)?

Ben.
>
> v2:
> - do not limit this patch to nv84-a3 and make it nv84+
>
> Signed-off-by: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c | 19 ++++++++++++++++++-
>  drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c |  4 ++++
>  drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c |  4 ++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
> index 42ba633..8615d62 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nv84.c
> @@ -209,13 +209,30 @@ nv84_therm_ctor(struct nouveau_object *parent,
>         return nouveau_therm_preinit(&priv->base.base);
>  }
>
> +static int
> +nv84_therm_init(struct nouveau_object *object)
> +{
> +       struct nv84_therm_priv *priv = (void *)object;
> +       int ret;
> +
> +       ret = nouveau_therm_init(&priv->base.base);
> +       if (ret)
> +               return ret;
> +
> +       /* ACK ptherm IRQs */
> +       nv_wr32(object, 0x20100, 0xffffffff);
> +       nv_wr32(object, 0x1100, 0x10000); /* PBUS */
> +
> +       return 0;
> +}
> +
>  struct nouveau_oclass
>  nv84_therm_oclass = {
>         .handle = NV_SUBDEV(THERM, 0x84),
>         .ofuncs = &(struct nouveau_ofuncs) {
>                 .ctor = nv84_therm_ctor,
>                 .dtor = _nouveau_therm_dtor,
> -               .init = _nouveau_therm_init,
> +               .init = nv84_therm_init,
>                 .fini = _nouveau_therm_fini,
>         },
>  };
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c
> index d11a7c4..f740eeb 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nva3.c
> @@ -60,6 +60,10 @@ nva3_therm_init(struct nouveau_object *object)
>         }
>         nv_mask(priv, 0x00e720, 0x00000002, 0x00000000);
>
> +       /* ACK ptherm IRQs */
> +       nv_wr32(object, 0x20100, 0xffffffff);
> +       nv_wr32(object, 0x1100, 0x10000); /* PBUS */
> +
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c b/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c
> index 54c28bd..d9b5103 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/therm/nvd0.c
> @@ -114,6 +114,10 @@ nvd0_therm_init(struct nouveau_object *object)
>         }
>         nv_mask(priv, 0x00e720, 0x00000002, 0x00000000);
>
> +       /* ACK ptherm IRQs */
> +       nv_wr32(object, 0x20100, 0xffffffff);
> +       nv_wr32(object, 0x1100, 0x10000); /* PBUS */
> +
>         return 0;
>  }
>
> --
> 1.8.4
>
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau/therm: ack any pending IRQ at init v2

[Martin Peres]

On 04/09/2013 03:52, Ben Skeggs wrote:
> On Sat, Aug 31, 2013 at 10:06 AM, Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> wrote:
>> From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
>>
>> This is safe because ptherm hasn't been configured yet and will be a
>> little further down the initialization path. Ptherm should be safe
>> regarding to runtime reconfiguration.
> Any objections to instead sticking the ACK in a single fini() hook
> (along with additional code to disable all therm interrupts), and
> having the IRQ handle silently ignore any non-requested IRQs (see what
> the copy engine irq handler does, for example)?
>
> Ben.

Hi Ben,

Sorry for the late answer. This all perfectly make sense. Please have a 
look at the patches I've just sent.

Re: [PATCH 1/2] drm/nouveau/bios/therm: handle vbioses with duplicate entries (mostly nva5)

[Martin Peres]

On 31/08/2013 01:58, Martin Peres wrote:
> From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
>
> Some vbioses have extra useless entries after "the end" of the table. This is
> problematic since all of the vbios I found with this issue redefine the
> pwm freq divider to insane levels (52750 Hz instead of 2500), thus breaking
> fan management.
>
> The first solution to solve this mess would be to change the length of the
> table. The solution I choose was simply to avoid setting the pwm freq twice
> as the other redefinitions are harmless with our current parser.
>
> Signed-off-by: Martin Peres <martin.peres-/ze9ptdxDJdQFI55V6+gNQ@public.gmane.org>

Sorry, the email should be ammended to martin.peres-BktLLJ5BOkI@public.gmane.org (only one b).

Re: [PATCH 1/2] drm/nouveau/bios/therm: handle vbioses with duplicate entries (mostly nva5)

[Ben Skeggs]

On Sat, Aug 31, 2013 at 9:58 AM, Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> wrote:
> From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
>
> Some vbioses have extra useless entries after "the end" of the table. This is
> problematic since all of the vbios I found with this issue redefine the
> pwm freq divider to insane levels (52750 Hz instead of 2500), thus breaking
> fan management.
>
> The first solution to solve this mess would be to change the length of the
> table. The solution I choose was simply to avoid setting the pwm freq twice
> as the other redefinitions are harmless with our current parser.
I'm still *really* not convinced we're using the thermal table correctly....

However, this makes sense for our current handling, so, merged.

Thanks,
Ben.

>
> Signed-off-by: Martin Peres <martin.peres-/ze9ptdxDJdQFI55V6+gNQ@public.gmane.org>
> Reported-by: Mariusz Bialonczyk <manio-RFXG7K41k5GsTnJN9+BGXg@public.gmane.org>
> Tested-by: Mariusz Bialonczyk <manio-RFXG7K41k5GsTnJN9+BGXg@public.gmane.org>
> ---
>  drivers/gpu/drm/nouveau/core/subdev/bios/therm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c b/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c
> index 22a2057..22ac6db 100644
> --- a/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c
> +++ b/drivers/gpu/drm/nouveau/core/subdev/bios/therm.c
> @@ -184,7 +184,8 @@ nvbios_therm_fan_parse(struct nouveau_bios *bios,
>                         cur_trip->fan_duty = value;
>                         break;
>                 case 0x26:
> -                       fan->pwm_freq = value;
> +                       if (!fan->pwm_freq)
> +                               fan->pwm_freq = value;
>                         break;
>                 case 0x3b:
>                         fan->bump_period = value;
> --
> 1.8.4
>
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] drm/nouveau/bios/therm: handle vbioses with duplicate entries (mostly nva5)

[Martin Peres]

On 04/09/2013 03:55, Ben Skeggs wrote:
> On Sat, Aug 31, 2013 at 9:58 AM, Martin Peres <martin.peres-GANU6spQydw@public.gmane.org> wrote:
>> From: Martin Peres <martin.peres-BktLLJ5BOkI@public.gmane.org>
>>
>> Some vbioses have extra useless entries after "the end" of the table. This is
>> problematic since all of the vbios I found with this issue redefine the
>> pwm freq divider to insane levels (52750 Hz instead of 2500), thus breaking
>> fan management.
>>
>> The first solution to solve this mess would be to change the length of the
>> table. The solution I choose was simply to avoid setting the pwm freq twice
>> as the other redefinitions are harmless with our current parser.
> I'm still *really* not convinced we're using the thermal table correctly....
>
> However, this makes sense for our current handling, so, merged.
>
> Thanks,
> Ben.

Well, I do agree that the first versions of the vbios table really felt 
like a script but this isn't the case anymore (not sure how I tested 
this though as it was more than a year ago).