[v4,07/12] drm: bridge: samsung-dsim: Fix PLL_P (PMS_P) offset
diff mbox series

Message ID 20220829184031.1863663-8-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: bridge: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki Aug. 29, 2022, 6:40 p.m. UTC 
The i.MX 8M Mini Applications Processor Reference Manual, Rev. 3, 11/2020
with 13.7.10.1 Master PLL PMS Value setting Register mentioned PMS_P offset
range from BIT[18-13] and the upstream driver is using the same offset.

However, offset 13 is not working on i.MX8M Mini platforms but downstream
NXP driver is using 14 [1] and it is working with i.MX8M Mini SoC.

Not sure about whether it is reference manual documentation or something
else but this patch trusts the downstream code and fixes the PLL_P offset.

[1] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/bridge/sec-dsim.c?h=imx_5.4.47_2.2.0#n211

v4, v3, v2:
* none

v1:
* updated commit message
* add downstream driver link

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++++--
 include/drm/bridge/samsung-dsim.h     |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Adam Ford Aug. 29, 2022, 7:42 p.m. UTC | #1 
On Mon, Aug 29, 2022 at 1:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> The i.MX 8M Mini Applications Processor Reference Manual, Rev. 3, 11/2020
> with 13.7.10.1 Master PLL PMS Value setting Register mentioned PMS_P offset
> range from BIT[18-13] and the upstream driver is using the same offset.
>
> However, offset 13 is not working on i.MX8M Mini platforms but downstream
> NXP driver is using 14 [1] and it is working with i.MX8M Mini SoC.

From the line you highlighted in the link, the downstream NXP ones
shows 13 if I'm reading it correctly.

#define PLLCTRL_SET_P(x) REG_PUT(x, 18, 13)

From what I can tell the PMS calculation here needs to be updated for
the Mini because the ranges of the FCO calculator are different.  Took
your series and tweaked it a bit [2] which changes a few settings, and
the PMS calculator appears to more closely match the values I get from
the NXP one.  I think it could be further tweaked because p min and
p_max also have changed.

>
> Not sure about whether it is reference manual documentation or something
> else but this patch trusts the downstream code and fixes the PLL_P offset.
>
> [1] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/bridge/sec-dsim.c?h=imx_5.4.47_2.2.0#n211
>

[2] -https://github.com/aford173/linux/commit/a5fa184160ec9ea45a7546eaa0d8b8fc760cf3d9

> v4, v3, v2:
> * none
>
> v1:
> * updated commit message
> * add downstream driver link
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 10 ++++++++--
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index b6883a6d4681..b6d17c0c9e58 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -168,7 +168,7 @@
>  /* DSIM_PLLCTRL */
>  #define DSIM_FREQ_BAND(x)              ((x) << 24)
>  #define DSIM_PLL_EN                    (1 << 23)
> -#define DSIM_PLL_P(x)                  ((x) << 13)
> +#define DSIM_PLL_P(x, offset)          ((x) << (offset))
>  #define DSIM_PLL_M(x)                  ((x) << 4)
>  #define DSIM_PLL_S(x)                  ((x) << 1)
>
> @@ -368,6 +368,7 @@ static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = {
>         .max_freq = 1000,
>         .wait_for_reset = 1,
>         .num_bits_resol = 11,
> +       .pll_p_offset = 13,
>         .reg_values = reg_values,
>         .quirks = DSIM_QUIRK_PLAT_DATA,
>  };
> @@ -381,6 +382,7 @@ static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
>         .max_freq = 1000,
>         .wait_for_reset = 1,
>         .num_bits_resol = 11,
> +       .pll_p_offset = 13,
>         .reg_values = reg_values,
>         .quirks = DSIM_QUIRK_PLAT_DATA,
>  };
> @@ -392,6 +394,7 @@ static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
>         .max_freq = 1000,
>         .wait_for_reset = 1,
>         .num_bits_resol = 11,
> +       .pll_p_offset = 13,
>         .reg_values = reg_values,
>         .quirks = DSIM_QUIRK_PLAT_DATA,
>  };
> @@ -404,6 +407,7 @@ static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
>         .max_freq = 1500,
>         .wait_for_reset = 0,
>         .num_bits_resol = 12,
> +       .pll_p_offset = 13,
>         .reg_values = exynos5433_reg_values,
>         .quirks = DSIM_QUIRK_PLAT_DATA,
>  };
> @@ -416,6 +420,7 @@ static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
>         .max_freq = 1500,
>         .wait_for_reset = 1,
>         .num_bits_resol = 12,
> +       .pll_p_offset = 13,
>         .reg_values = exynos5422_reg_values,
>         .quirks = DSIM_QUIRK_PLAT_DATA,
>  };
> @@ -563,7 +568,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>         writel(driver_data->reg_values[PLL_TIMER],
>                         dsi->reg_base + driver_data->plltmr_reg);
>
> -       reg = DSIM_PLL_EN | DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s);
> +       reg = DSIM_PLL_EN | DSIM_PLL_P(p, driver_data->pll_p_offset) |
> +             DSIM_PLL_M(m) | DSIM_PLL_S(s);
>
>         if (driver_data->has_freqband) {
>                 static const unsigned long freq_bands[] = {
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index e15fbfd49efe..95d3f89aec4f 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -47,6 +47,7 @@ struct samsung_dsim_driver_data {
>         unsigned int max_freq;
>         unsigned int wait_for_reset;
>         unsigned int num_bits_resol;
> +       unsigned int pll_p_offset;
>         const unsigned int *reg_values;
>         enum samsung_dsim_quirks quirks;
>  };
> --
> 2.25.1
>
Jagan Teki Sept. 14, 2022, 8:22 a.m. UTC | #2 
On Tue, Aug 30, 2022 at 1:12 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Aug 29, 2022 at 1:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > The i.MX 8M Mini Applications Processor Reference Manual, Rev. 3, 11/2020
> > with 13.7.10.1 Master PLL PMS Value setting Register mentioned PMS_P offset
> > range from BIT[18-13] and the upstream driver is using the same offset.
> >
> > However, offset 13 is not working on i.MX8M Mini platforms but downstream
> > NXP driver is using 14 [1] and it is working with i.MX8M Mini SoC.
>
> From the line you highlighted in the link, the downstream NXP ones
> shows 13 if I'm reading it correctly.
>
> #define PLLCTRL_SET_P(x) REG_PUT(x, 18, 13)

PMS_P value set in sec_mipi_dsim_check_pll_out using PLLCTRL_SET_P()
with offset 13 and then an additional offset of one bit added in
sec_mipi_dsim_config_pll via PLLCTRL_SET_PMS().

Please check this for additional information
https://github.com/fschrempf/linux/commit/bbb3549a99e162ef6ad4c83d5fd4d39a6daa6d56

I'll update the additional information in commit messages in v5.

Thanks,
Jagan.

Patch
diff mbox series 

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index b6883a6d4681..b6d17c0c9e58 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -168,7 +168,7 @@ 
 /* DSIM_PLLCTRL */
 #define DSIM_FREQ_BAND(x)		((x) << 24)
 #define DSIM_PLL_EN			(1 << 23)
-#define DSIM_PLL_P(x)			((x) << 13)
+#define DSIM_PLL_P(x, offset)		((x) << (offset))
 #define DSIM_PLL_M(x)			((x) << 4)
 #define DSIM_PLL_S(x)			((x) << 1)
 
@@ -368,6 +368,7 @@  static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = {
 	.max_freq = 1000,
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
+	.pll_p_offset = 13,
 	.reg_values = reg_values,
 	.quirks = DSIM_QUIRK_PLAT_DATA,
 };
@@ -381,6 +382,7 @@  static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
 	.max_freq = 1000,
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
+	.pll_p_offset = 13,
 	.reg_values = reg_values,
 	.quirks = DSIM_QUIRK_PLAT_DATA,
 };
@@ -392,6 +394,7 @@  static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
 	.max_freq = 1000,
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
+	.pll_p_offset = 13,
 	.reg_values = reg_values,
 	.quirks = DSIM_QUIRK_PLAT_DATA,
 };
@@ -404,6 +407,7 @@  static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
 	.max_freq = 1500,
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
+	.pll_p_offset = 13,
 	.reg_values = exynos5433_reg_values,
 	.quirks = DSIM_QUIRK_PLAT_DATA,
 };
@@ -416,6 +420,7 @@  static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
 	.max_freq = 1500,
 	.wait_for_reset = 1,
 	.num_bits_resol = 12,
+	.pll_p_offset = 13,
 	.reg_values = exynos5422_reg_values,
 	.quirks = DSIM_QUIRK_PLAT_DATA,
 };
@@ -563,7 +568,8 @@  static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 	writel(driver_data->reg_values[PLL_TIMER],
 			dsi->reg_base + driver_data->plltmr_reg);
 
-	reg = DSIM_PLL_EN | DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s);
+	reg = DSIM_PLL_EN | DSIM_PLL_P(p, driver_data->pll_p_offset) |
+	      DSIM_PLL_M(m) | DSIM_PLL_S(s);
 
 	if (driver_data->has_freqband) {
 		static const unsigned long freq_bands[] = {
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index e15fbfd49efe..95d3f89aec4f 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -47,6 +47,7 @@  struct samsung_dsim_driver_data {
 	unsigned int max_freq;
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
+	unsigned int pll_p_offset;
 	const unsigned int *reg_values;
 	enum samsung_dsim_quirks quirks;
 };