Re: [PATCH V2 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll
On 12.02.2024 00:09, Adam Ford wrote:
> The P divider should be set based on the min and max values of
> the fin pll which may vary between different platforms.
> These ranges are defined per platform, but hard-coded values
> were used instead which resulted in a smaller range available
> on the i.MX8M[MNP] than what was possible.
>
> As noted by Frieder, there are descripencies between the reference
> manuals of the Mini, Nano and Plus, so I reached out to my NXP
> rep and got the following response regarding the varing notes
> in the documentation.
>
> "Yes it is definitely wrong, the one that is part of the NOTE in
> MIPI_DPHY_M_PLLPMS register table against PMS_P, PMS_M and PMS_S is
> not correct. I will report this to Doc team, the one customer should
> be take into account is the Table 13-40 DPHY PLL Parameters and the
> Note above."
>
> With this patch, the clock rates now match the values used in NXP's
> downstream kernel.
>
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford
> Reviewed-by: Frieder Schrempf
> Tested-by: Frieder Schrempf
Feel free to add:
Tested-by: Marek Szyprowski
although all Exynos based boards have panels with fixed timings afair.
> ---
> V2: Only update the commit message to reflect why these values
> were chosen. No code change present
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae..8476650c477c 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -574,8 +574,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct
> samsung_dsim *dsi,
> u16 _m, best_m;
> u8 _s, best_s;
>
> - p_min = DIV_ROUND_UP(fin, (12 * MHZ));
> - p_max = fin / (6 * MHZ);
> + p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
> + p_max = fin / (driver_data->pll_fin_min * MHZ);
>
> for (_p = p_min; _p <= p_max; ++_p) {
> for (_s = 0; _s <= 5; ++_s) {
Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland
Re: [PATCH V2 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll
On 2/12/24 12:09 AM, Adam Ford wrote:
The P divider should be set based on the min and max values of
the fin pll which may vary between different platforms.
These ranges are defined per platform, but hard-coded values
were used instead which resulted in a smaller range available
on the i.MX8M[MNP] than what was possible.
As noted by Frieder, there are descripencies between the reference
manuals of the Mini, Nano and Plus, so I reached out to my NXP
rep and got the following response regarding the varing notes
in the documentation.
"Yes it is definitely wrong, the one that is part of the NOTE in
MIPI_DPHY_M_PLLPMS register table against PMS_P, PMS_M and PMS_S is
not correct. I will report this to Doc team, the one customer should
be take into account is the Table 13-40 DPHY PLL Parameters and the
Note above."
With this patch, the clock rates now match the values used in NXP's
downstream kernel.
Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
Signed-off-by: Adam Ford
Reviewed-by: Frieder Schrempf
Tested-by: Frieder Schrempf
---
V2: Only update the commit message to reflect why these values
were chosen. No code change present
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 95fedc68b0ae..8476650c477c 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -574,8 +574,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct
samsung_dsim *dsi,
u16 _m, best_m;
u8 _s, best_s;
- p_min = DIV_ROUND_UP(fin, (12 * MHZ));
- p_max = fin / (6 * MHZ);
+ p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
The parenthesis around driver_data... are not needed.
With that fixed:
Reviewed-by: Marek Vasut
Re: [PATCH V2 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll
On Sun, Feb 11, 2024 at 5:09 PM Adam Ford wrote:
>
> The P divider should be set based on the min and max values of
> the fin pll which may vary between different platforms.
> These ranges are defined per platform, but hard-coded values
> were used instead which resulted in a smaller range available
> on the i.MX8M[MNP] than what was possible.
>
> As noted by Frieder, there are descripencies between the reference
> manuals of the Mini, Nano and Plus, so I reached out to my NXP
> rep and got the following response regarding the varing notes
> in the documentation.
>
> "Yes it is definitely wrong, the one that is part of the NOTE in
> MIPI_DPHY_M_PLLPMS register table against PMS_P, PMS_M and PMS_S is
> not correct. I will report this to Doc team, the one customer should
> be take into account is the Table 13-40 DPHY PLL Parameters and the
> Note above."
>
> With this patch, the clock rates now match the values used in NXP's
> downstream kernel.
>
Inki,
Any change either or both of these patches could get applied? It's
been several months since the first submission.
Fabio - Do you have an 8MP-evk that you could test to see if there is
any impact? Between these two patches, it fixes the 720p@60 and
likely others too.
adam
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford
> Reviewed-by: Frieder Schrempf
> Tested-by: Frieder Schrempf
> ---
> V2: Only update the commit message to reflect why these values
> were chosen. No code change present
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae..8476650c477c 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -574,8 +574,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct
> samsung_dsim *dsi,
> u16 _m, best_m;
> u8 _s, best_s;
>
> - p_min = DIV_ROUND_UP(fin, (12 * MHZ));
> - p_max = fin / (6 * MHZ);
> + p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
> + p_max = fin / (driver_data->pll_fin_min * MHZ);
>
> for (_p = p_min; _p <= p_max; ++_p) {
> for (_s = 0; _s <= 5; ++_s) {
> --
> 2.43.0
>
[PATCH V2 1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll
The P divider should be set based on the min and max values of
the fin pll which may vary between different platforms.
These ranges are defined per platform, but hard-coded values
were used instead which resulted in a smaller range available
on the i.MX8M[MNP] than what was possible.
As noted by Frieder, there are descripencies between the reference
manuals of the Mini, Nano and Plus, so I reached out to my NXP
rep and got the following response regarding the varing notes
in the documentation.
"Yes it is definitely wrong, the one that is part of the NOTE in
MIPI_DPHY_M_PLLPMS register table against PMS_P, PMS_M and PMS_S is
not correct. I will report this to Doc team, the one customer should
be take into account is the Table 13-40 DPHY PLL Parameters and the
Note above."
With this patch, the clock rates now match the values used in NXP's
downstream kernel.
Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
Signed-off-by: Adam Ford
Reviewed-by: Frieder Schrempf
Tested-by: Frieder Schrempf
---
V2: Only update the commit message to reflect why these values
were chosen. No code change present
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 95fedc68b0ae..8476650c477c 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -574,8 +574,8 @@ static unsigned long samsung_dsim_pll_find_pms(struct
samsung_dsim *dsi,
u16 _m, best_m;
u8 _s, best_s;
- p_min = DIV_ROUND_UP(fin, (12 * MHZ));
- p_max = fin / (6 * MHZ);
+ p_min = DIV_ROUND_UP(fin, (driver_data->pll_fin_max * MHZ));
+ p_max = fin / (driver_data->pll_fin_min * MHZ);
for (_p = p_min; _p <= p_max; ++_p) {
for (_s = 0; _s <= 5; ++_s) {
--
2.43.0
