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

[Adam Ford]

On Tue, Dec 12, 2023 at 2:25 AM Frieder Schrempf
<frieder.schrempf@xxxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> Tested-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>

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) {
>