All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: "Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	"Len Brown" <lenb@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>,
	linux-pwm@vger.kernel.org,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	linux-acpi@vger.kernel.org
Subject: [PATCH v10 08/17] pwm: crc: Fix period / duty_cycle times being off by a factor of 256
Date: Thu,  3 Sep 2020 13:23:28 +0200	[thread overview]
Message-ID: <20200903112337.4113-9-hdegoede@redhat.com> (raw)
In-Reply-To: <20200903112337.4113-1-hdegoede@redhat.com>

While looking into adding atomic-pwm support to the pwm-crc driver I
noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and
there is a clock-divider which divides this with a value between 1-128,
and there are 256 duty-cycle steps.

The pwm-crc code before this commit assumed that a clock-divider
setting of 1 means that the PWM output is running at 6 MHZ, if that
is true, where do these 256 duty-cycle steps come from?

This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that
seems unlikely for a PMIC which is using a silicon process optimized for
power-switching transistors. It is way more likely that there is an 8
bit counter for the duty cycle which acts as an extra fixed divider
wrt the PWM output frequency.

The main user of the pwm-crc driver is the i915 GPU driver which uses it
for backlight control. Lets compare the PWM register values set by the
video-BIOS (the GOP), assuming the extra fixed divider is present versus
the PWM frequency specified in the Video-BIOS-Tables:

Device:		PWM Hz set by BIOS	PWM Hz specified in VBT
Asus T100TA 	200			200
Asus T100HA 	200			200
Lenovo Miix 2 8	23437			20000
Toshiba WT8-A	23437			20000

So as we can see if we assume the extra division by 256 then the register
values set by the GOP are an exact match for the VBT values, where as
otherwise the values would be of by a factor of 256.

This commit fixes the period / duty_cycle calculations to take the
extra division by 256 into account.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define
---
 drivers/pwm/pwm-crc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 272eeb071147..c056eb9b858c 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -21,8 +21,8 @@
 
 #define PWM_MAX_LEVEL		0xFF
 
-#define PWM_BASE_CLK		6000000  /* 6 MHz */
-#define PWM_MAX_PERIOD_NS	21333    /* 46.875KHz */
+#define PWM_BASE_CLK_MHZ	6	/* 6 MHz */
+#define PWM_MAX_PERIOD_NS	5461333	/* 183 Hz */
 
 /**
  * struct crystalcove_pwm - Crystal Cove PWM controller
@@ -72,7 +72,7 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
 
 		/* changing the clk divisor, need to disable fisrt */
 		crc_pwm_disable(c, pwm);
-		clk_div = PWM_BASE_CLK * period_ns / NSEC_PER_SEC;
+		clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_USEC);
 
 		regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
 					clk_div | PWM_OUTPUT_ENABLE);
-- 
2.28.0


WARNING: multiple messages have this Message-ID (diff)
From: Hans de Goede <hdegoede@redhat.com>
To: "Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	"Len Brown" <lenb@kernel.org>
Cc: linux-pwm@vger.kernel.org, linux-acpi@vger.kernel.org,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org,
	Hans de Goede <hdegoede@redhat.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: [PATCH v10 08/17] pwm: crc: Fix period / duty_cycle times being off by a factor of 256
Date: Thu,  3 Sep 2020 13:23:28 +0200	[thread overview]
Message-ID: <20200903112337.4113-9-hdegoede@redhat.com> (raw)
In-Reply-To: <20200903112337.4113-1-hdegoede@redhat.com>

While looking into adding atomic-pwm support to the pwm-crc driver I
noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and
there is a clock-divider which divides this with a value between 1-128,
and there are 256 duty-cycle steps.

The pwm-crc code before this commit assumed that a clock-divider
setting of 1 means that the PWM output is running at 6 MHZ, if that
is true, where do these 256 duty-cycle steps come from?

This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that
seems unlikely for a PMIC which is using a silicon process optimized for
power-switching transistors. It is way more likely that there is an 8
bit counter for the duty cycle which acts as an extra fixed divider
wrt the PWM output frequency.

The main user of the pwm-crc driver is the i915 GPU driver which uses it
for backlight control. Lets compare the PWM register values set by the
video-BIOS (the GOP), assuming the extra fixed divider is present versus
the PWM frequency specified in the Video-BIOS-Tables:

Device:		PWM Hz set by BIOS	PWM Hz specified in VBT
Asus T100TA 	200			200
Asus T100HA 	200			200
Lenovo Miix 2 8	23437			20000
Toshiba WT8-A	23437			20000

So as we can see if we assume the extra division by 256 then the register
values set by the GOP are an exact match for the VBT values, where as
otherwise the values would be of by a factor of 256.

This commit fixes the period / duty_cycle calculations to take the
extra division by 256 into account.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define
---
 drivers/pwm/pwm-crc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 272eeb071147..c056eb9b858c 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -21,8 +21,8 @@
 
 #define PWM_MAX_LEVEL		0xFF
 
-#define PWM_BASE_CLK		6000000  /* 6 MHz */
-#define PWM_MAX_PERIOD_NS	21333    /* 46.875KHz */
+#define PWM_BASE_CLK_MHZ	6	/* 6 MHz */
+#define PWM_MAX_PERIOD_NS	5461333	/* 183 Hz */
 
 /**
  * struct crystalcove_pwm - Crystal Cove PWM controller
@@ -72,7 +72,7 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
 
 		/* changing the clk divisor, need to disable fisrt */
 		crc_pwm_disable(c, pwm);
-		clk_div = PWM_BASE_CLK * period_ns / NSEC_PER_SEC;
+		clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_USEC);
 
 		regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
 					clk_div | PWM_OUTPUT_ENABLE);
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: Hans de Goede <hdegoede@redhat.com>
To: "Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	"Len Brown" <lenb@kernel.org>
Cc: linux-pwm@vger.kernel.org, linux-acpi@vger.kernel.org,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>
Subject: [Intel-gfx] [PATCH v10 08/17] pwm: crc: Fix period / duty_cycle times being off by a factor of 256
Date: Thu,  3 Sep 2020 13:23:28 +0200	[thread overview]
Message-ID: <20200903112337.4113-9-hdegoede@redhat.com> (raw)
In-Reply-To: <20200903112337.4113-1-hdegoede@redhat.com>

While looking into adding atomic-pwm support to the pwm-crc driver I
noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and
there is a clock-divider which divides this with a value between 1-128,
and there are 256 duty-cycle steps.

The pwm-crc code before this commit assumed that a clock-divider
setting of 1 means that the PWM output is running at 6 MHZ, if that
is true, where do these 256 duty-cycle steps come from?

This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that
seems unlikely for a PMIC which is using a silicon process optimized for
power-switching transistors. It is way more likely that there is an 8
bit counter for the duty cycle which acts as an extra fixed divider
wrt the PWM output frequency.

The main user of the pwm-crc driver is the i915 GPU driver which uses it
for backlight control. Lets compare the PWM register values set by the
video-BIOS (the GOP), assuming the extra fixed divider is present versus
the PWM frequency specified in the Video-BIOS-Tables:

Device:		PWM Hz set by BIOS	PWM Hz specified in VBT
Asus T100TA 	200			200
Asus T100HA 	200			200
Lenovo Miix 2 8	23437			20000
Toshiba WT8-A	23437			20000

So as we can see if we assume the extra division by 256 then the register
values set by the GOP are an exact match for the VBT values, where as
otherwise the values would be of by a factor of 256.

This commit fixes the period / duty_cycle calculations to take the
extra division by 256 into account.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define
---
 drivers/pwm/pwm-crc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 272eeb071147..c056eb9b858c 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -21,8 +21,8 @@
 
 #define PWM_MAX_LEVEL		0xFF
 
-#define PWM_BASE_CLK		6000000  /* 6 MHz */
-#define PWM_MAX_PERIOD_NS	21333    /* 46.875KHz */
+#define PWM_BASE_CLK_MHZ	6	/* 6 MHz */
+#define PWM_MAX_PERIOD_NS	5461333	/* 183 Hz */
 
 /**
  * struct crystalcove_pwm - Crystal Cove PWM controller
@@ -72,7 +72,7 @@ static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
 
 		/* changing the clk divisor, need to disable fisrt */
 		crc_pwm_disable(c, pwm);
-		clk_div = PWM_BASE_CLK * period_ns / NSEC_PER_SEC;
+		clk_div = PWM_BASE_CLK_MHZ * period_ns / (256 * NSEC_PER_USEC);
 
 		regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
 					clk_div | PWM_OUTPUT_ENABLE);
-- 
2.28.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2020-09-03 15:21 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-03 11:23 [PATCH v10 00/17] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Hans de Goede
2020-09-03 11:23 ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23 ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 01/17] ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 02/17] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once (at activation) Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 03/17] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare() Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 04/17] pwm: lpss: Add range limit check for the base_unit register value Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 05/17] pwm: lpss: Add pwm_lpss_prepare_enable() helper Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 06/17] pwm: lpss: Make pwm_lpss_apply() not rely on existing hardware state Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 12:44   ` Andy Shevchenko
2020-09-03 12:44     ` [Intel-gfx] " Andy Shevchenko
2020-09-03 12:44     ` Andy Shevchenko
2020-09-03 11:23 ` [PATCH v10 07/17] pwm: lpss: Remove suspend/resume handlers Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 12:48   ` Andy Shevchenko
2020-09-03 12:48     ` [Intel-gfx] " Andy Shevchenko
2020-09-03 12:48     ` Andy Shevchenko
2020-09-03 12:56     ` Andy Shevchenko
2020-09-03 12:56       ` [Intel-gfx] " Andy Shevchenko
2020-09-03 12:56       ` Andy Shevchenko
2020-09-03 13:09       ` Hans de Goede
2020-09-03 13:09         ` [Intel-gfx] " Hans de Goede
2020-09-03 13:09         ` Hans de Goede
2020-09-03 11:23 ` Hans de Goede [this message]
2020-09-03 11:23   ` [Intel-gfx] [PATCH v10 08/17] pwm: crc: Fix period / duty_cycle times being off by a factor of 256 Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 09/17] pwm: crc: Fix off-by-one error in the clock-divider calculations Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 10/17] pwm: crc: Fix period changes not having any effect Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 11/17] pwm: crc: Enable/disable PWM output on enable/disable Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 12/17] pwm: crc: Implement apply() method to support the new atomic PWM API Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 13/17] pwm: crc: Implement get_state() method Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 14/17] drm/i915: panel: Add get_vbt_pwm_freq() helper Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 15/17] drm/i915: panel: Honor the VBT PWM frequency for devs with an external PWM controller Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 16/17] drm/i915: panel: Honor the VBT PWM min setting " Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:23 ` [PATCH v10 17/17] drm/i915: panel: Use atomic PWM API " Hans de Goede
2020-09-03 11:23   ` [Intel-gfx] " Hans de Goede
2020-09-03 11:23   ` Hans de Goede
2020-09-03 11:54 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API Patchwork
2020-09-03 12:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-09-03 20:10 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200903112337.4113-9-hdegoede@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=jani.nikula@linux.intel.com \
    --cc=joonas.lahtinen@linux.intel.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=rjw@rjwysocki.net \
    --cc=rodrigo.vivi@intel.com \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.