diff mbox series

[1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll

Message ID 20231212033259.189718-1-aford173@gmail.com
State New
Headers
Series [1/2] drm/bridge: samsung-dsim: Set P divider based on min/max of fin pll |

Commit Message

Adam Ford Dec. 12, 2023, 3:32 a.m. UTC 
  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.

Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
Signed-off-by: Adam Ford <aford173@gmail.com>

Comments

Frieder Schrempf Dec. 12, 2023, 8:25 a.m. UTC | #1 
Hi Adam,

On 12.12.23 04:32, 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.
> 
> Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index be5914caa17d..239d253a7d71 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -573,8 +573,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);

I did some tinkering with the PLL settings the other day and this is
literally one of the things I came up with.

So I'm happy to provide:

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Regarding the P divider, I'm also wondering if there is an upper limit
for the p-value (not for the resulting fin_pll) that we should take into
account, too. The problem is that we have conflicts in the documentation
(again) so we don't really know what the correct limit would be.

There are the following ranges given in the RMs:

* 1..63 (i.MX8MM RM 13.7.8.18.4)
* 1..33 (i.MX8MM RM 13.7.10.1)
* 1..63 (i.MX8MP RM 13.2.3.1.5.2)
* 1..63 (i.MX8MP RM 13.7.2.4)

Unfortunately there are similar discrepancies for the other parameters
and limits.

Thanks
Frieder

>  
>  	for (_p = p_min; _p <= p_max; ++_p) {
>  		for (_s = 0; _s <= 5; ++_s) {
Adam Ford Dec. 12, 2023, 10:18 a.m. UTC | #2 
On Tue, Dec 12, 2023 at 2:25 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Adam,
>
> On 12.12.23 04:32, 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.
> >
> > Fixes: 846307185f0f ("drm/bridge: samsung-dsim: update PLL reference clock")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index be5914caa17d..239d253a7d71 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -573,8 +573,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);
>
> I did some tinkering with the PLL settings the other day and this is
> literally one of the things I came up with.
>
> So I'm happy to provide:
>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>

Thank you!

> Regarding the P divider, I'm also wondering if there is an upper limit
> for the p-value (not for the resulting fin_pll) that we should take into
> account, too. The problem is that we have conflicts in the documentation
> (again) so we don't really know what the correct limit would be.
>
> There are the following ranges given in the RMs:
>
> * 1..63 (i.MX8MM RM 13.7.8.18.4)
> * 1..33 (i.MX8MM RM 13.7.10.1)
> * 1..63 (i.MX8MP RM 13.2.3.1.5.2)
> * 1..63 (i.MX8MP RM 13.7.2.4)

1...63 (i.IMX8MN RM 13.7.2.4)
>
> Unfortunately there are similar discrepancies for the other parameters
> and limits.

For what it's worth, I compared these values to the NXP downstream
branch [1], and they seemed to indicate the values were as follows:

.p = { .min = 1, .max = 63, },
.m = { .min = 64, .max = 1023, },
.s = { .min = 0, .max = 5, },
.k = { .min = 0, .max = 32768, }, /* abs(k) */
.fin = { .min = 6000, .max = 300000, }, /* in KHz */
.fpref = { .min = 2000, .max = 30000, }, /* in KHz */
.fvco = { .min = 1050000, .max = 2100000, }, /* in KHz */

In a previous commit [2], I mentioned the fact that I reached out to
NXP asking about the discrepancies and my NXP Rep and I received the
response:

"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."

adam

[1] - https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/gpu/drm/imx/sec_mipi_pll_1432x.h#L38C1-L47C1
[2] - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/bridge/samsung-dsim.c?h=next-20231212&id=54f1a83c72250b182fa7722b0c5f6eb5e769598d

>
> Thanks
> Frieder
>
> >
> >       for (_p = p_min; _p <= p_max; ++_p) {
> >               for (_s = 0; _s <= 5; ++_s) {
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index be5914caa17d..239d253a7d71 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -573,8 +573,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) {